Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[Heikki Krogerus]

On Mon, Jul 27, 2026 at 03:05:35PM +0200, Sasha Finkelstein wrote:
> On Jul 27, 2026, at 14:56, Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> > 
> > Hi,
> > 
> > On Sat, Jul 25, 2026 at 06:20:57PM +0200, Sasha Finkelstein wrote:
> >> From: Alyssa Milburn <amilburn@xxxxxxxx>
> >> 
> >> Make the core driver more bus-agnostic to prepare for SPMI variants of
> >> the tipd chip
> >> 
> >> Signed-off-by: Alyssa Milburn <amilburn@xxxxxxxx>
> >> Signed-off-by: Sasha Finkelstein <k@xxxxxxxxxxxxxx>
> >> ---
> >> drivers/usb/typec/tipd/Makefile           |   2 +-
> >> drivers/usb/typec/tipd/{core.c => core.h} | 109 ++++++++++++++++++++++++++++---------------------------------------------------------------------------------
> > 
> > That has to be a mistake, right? You don't move code into a header
> > like that.
> 
> Yes, it was a bad idea, already fixed in a v2 that will be sent in the
> near future.
> 
> >> +static struct tps6598x *tps6598x_from_device(struct device *dev)
> >> +{
> >> + struct i2c_client *client = i2c_verify_client(dev);
> >> + struct tps6598x *tps = i2c_get_clientdata(client);
> >> + return tps;
> >> +}
> > 
> > You should not need anything like that.
> 
> This should make more sense together with the following patch, as it
> can get the tps6598x from either the i2c or spmi backend.

You would only need this in the PM callbacks, and those you need to
keep in the glue drivers for i2c and spmi. The core.c can export
common functions for suspend and resume like I told you.

> >> --- /dev/null
> >> +++ b/drivers/usb/typec/tipd/i2c.c
> >> @@ -0,0 +1,86 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Driver for TI TPS6598x USB Power Delivery controller family
> >> + *
> >> + * Copyright (C) 2017, Intel Corporation
> >> + * Author: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> >> + */
> > 
> > This driver is not coming from me.
> 
> I kept the attribution as both core and i2c are your driver, but split
> into two. Should I have done something else?

This will not be the same module as the core. You will have separate
modules for the core and for both glue driver.

Note. You can also refactor the core a little so that by default it
works with i2c, but it also exports the init/probe function so that it
can be used as a library with spmi. In that way you don't need to add
the i2c.c at all. But the you will depend on i2c also when you use the
spmi.

Thanks,

-- 
heikki