Re: [PATCH v3 0/9] misc: Support TI FPC202 dual-port controller

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Romain, Tomi,

On Tue, 03 Dec 2024 09:42:07 +0100
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hi Tomi,
> 
> On vendredi 29 novembre 2024 13:01:58 heure normale d’Europe centrale Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 25/11/2024 10:45, Romain Gantois wrote:  
> > > Hello everyone,
> > > 
> > > This is version three of my series which adds support for the TI FPC202
> > > dual-port controller. This is an unusual kind of device which is used as a
> > > low-speed signal aggregator for various types of SFP-like hardware ports.
> > > 
> > > The FPC202 exposes an I2C, or SPI (not supported in this series) control
> > > interface, which can be used to access two downstream I2C busses, along
> > > with a set of low-speed GPIO signals for each port. It also has I2C
> > > address
> > > translation (ATR) features, which allow multiple I2C devices with the same
> > > address (e.g. SFP EEPROMs at address 0x50) to be accessed from the
> > > upstream
> > > control interface on different addresses.
> > > 
> > > I've chosen to add this driver to the misc subsystem, as it doesn't
> > > strictly belong in either the i2c or gpio sybsystem, and as far as I know
> > > it is the first device of its kind to be added to the kernel.
> > > 
> > > Along with the FPC202 driver itself, this series also adds support for
> > > dynamic address translation to the i2c-atr module. This allows I2C address
> > > translators to update their translation table on-the-fly when they receive
> > > transactions to unmapped clients. This feature is needed by the FPC202
> > > driver to access up to three logical I2C devices per-port, given that the
> > > FPC202 address translation table only has two address slots.  
> > 
> > While the FPD-Link devices are quite different than the TPC202, I wonder
> > what's the difference wrt. the ATR... Afaics, the difference is that the
> > FPC202 has 2 slots whereas UB960 has 8. So if you have 3+ remote devices
> > on FPC202, you get problems, or if you have 9+ devices on UB960, you get
> > problems.
> > 
> > Yet this series adds a I2C_ATR_FLAG_DYNAMIC_C2A flag which the driver
> > needs to set, and the i2c-atr has different code paths depending on the
> > flag. In other words, either the driver author (if it's a hardcoded
> > flag) or the driver (if it's set dynamically) is assumed to know how
> > many remote devices there are, and whether that flag is needed.
> > 
> > On the other hand, if I consider I2C_ATR_FLAG_DYNAMIC_C2A meaning that
> > the device can support dynamically changing the ATR, then it makes more
> > sense, and also UB960 should set the flag.
> >   
> 
> Indeed, the need for dynamic address translation isn't solely determined by
> the ATR model. It's also determined by the number of logical I2C devices
> connected to the downstream ports. In that sense, hardcoding the flag in the
> ATR driver doesn't seem completely appropriate.
> 
> However, you could reasonably imagine that some future ATR models won't
> support hot-swapping aliases at runtime. In this case, this flag will be
> necessary at the very least as a capability flag i.e. "this ATR model can do
> dynamic translation but it's not necessarily activated by default".
> 
> > But then I wonder, do we even have cases with ATRs that need to be
> > programmed once at init time, and cannot be changed afterwards? If not,
> > then the I2C_ATR_FLAG_DYNAMIC_C2A can be the default, and the
> > non-I2C_ATR_FLAG_DYNAMIC_C2A code can be dropped. Actually, even the
> > current upstream i2c-atr is dynamic in a sense: the clients are attached
> > via the i2c_atr_bus_notifier_call(), one by one.
> >   
> 
> Indeed, if an ATR component doesn't support hot-swapping of aliases, then
> it will be broken anyway if a device attaches after the ATR's been initialized.
> Maybe we should just assume that all supported ATR's should be capable of
> modifying their translation table after initialization then.

I think this is a reasonable assumption, and so we should not implement
support for "non-dynamic ATRs" unless (until?) there is a valid use
case.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com