by Luca Ceresoli

[permalink] [raw]

Subject: Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support

Hi Matti,

On 16/03/22 15:11, Vaittinen, Matti wrote:
> Hi dee Ho peeps!
>
> On 2/6/22 13:59, Luca Ceresoli wrote:
>> An ATR is a device that looks similar to an i2c-mux: it has an I2C
>> slave "upstream" port and N master "downstream" ports, and forwards
>> transactions from upstream to the appropriate downstream port. But is
>> is different in that the forwarded transaction has a different slave
>> address. The address used on the upstream bus is called the "alias"
>> and is (potentially) different from the physical slave address of the
>> downstream chip.
>>
>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
>> implementing ATR features in a device driver. The helper takes care or
>> adapter creation/destruction and translates addresses at each transaction.
>>
>
> snip
>
>> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
>> index 438905e2a1d0..c6d1a345ea6d 100644
>> --- a/drivers/i2c/Kconfig
>> +++ b/drivers/i2c/Kconfig
>> @@ -71,6 +71,15 @@ config I2C_MUX
>>
>> source "drivers/i2c/muxes/Kconfig"
>>
>> +config I2C_ATR
>> + tristate "I2C Address Translator (ATR) support"
>> + help
>> + Enable support for I2C Address Translator (ATR) chips.
>> +
>> + An ATR allows accessing multiple I2C busses from a single
>> + physical bus via address translation instead of bus selection as
>> + i2c-muxes do.
>> +
>
> I continued playing with the ROHM (de-)serializer and ended up having
> .config where the I2C_ATR was ='m', while my ATR driver was ='y' even
> though it selects the I2C_ATR.
>
> Yep, most probably my error somewhere.
>
> Anyways, this made me think that most of the I2C_ATR users are likely to
> just silently select the I2C_ATR, right? The I2C_ATR has no much reason
> to be compiled in w/o users, right? So perhaps the menu entry for
> selecting the I2C_ATR could be dropped(?) Do we really need this entry
> in already long list of configs to be manually picked?

Maybe we could make it a blind option, sure. The only reason it could be
useful that it's visible is that one might implement a user driver could
be written out of tree. I don't care very much about that, but it is
possible. Maybe it's the reason for I2C_MUX to be a visible option too.
Peter?

>> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
>> + const struct i2c_atr_ops *ops, int max_adapters)
>> +{
>> + struct i2c_atr *atr;
>> +
>> + if (!ops || !ops->attach_client || !ops->detach_client)
>> + return ERR_PTR(-EINVAL);
>> +
>
> I believe that most of the attach_client implementations will have
> similar approach of allocating and populating an address-pool and
> searching for first unused address. As a 'further dev' it'd be great to
> see a common helper implementation for attach/detach - perhaps so that
> the atr drivers would only need to specify the slave-address
> configuration register(s) / mask and the use a 'generic' attach/detach
> helpers. Well, just thinking how to reduce the code from actual IC
> drivers but this is really not something that is required during this
> initial series :)
>
> Also, devm-variants would be great - although that falls to the same
> category of things that do not need to be done immediately - but would
> perhaps be worth considering in the future.

Both of your proposals make sense, however I did deliberately not
generalize too much because I knew only one chipset. I don't like trying
to generalize for an unpredictable future use case, it generally leads
(me) to generalizing in the wrong direction. That means you'd be very
welcome to propose helpers and/or devm variants, possibly in the same
patchset as the first Rohm serdes driver. ;)

> Reviewed-by: Matti Vaittinen <[email protected]>

Thanks for your review!

--
Luca

2022-03-17 04:20:33

by Matti Vaittinen

[permalink] [raw]

Subject: Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support

Hi dee Ho peeps!

On 2/6/22 13:59, Luca Ceresoli wrote:
> An ATR is a device that looks similar to an i2c-mux: it has an I2C
> slave "upstream" port and N master "downstream" ports, and forwards
> transactions from upstream to the appropriate downstream port. But is
> is different in that the forwarded transaction has a different slave
> address. The address used on the upstream bus is called the "alias"
> and is (potentially) different from the physical slave address of the
> downstream chip.
>
> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
> implementing ATR features in a device driver. The helper takes care or
> adapter creation/destruction and translates addresses at each transaction.
>

snip

> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 438905e2a1d0..c6d1a345ea6d 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -71,6 +71,15 @@ config I2C_MUX
>
> source "drivers/i2c/muxes/Kconfig"
>
> +config I2C_ATR
> + tristate "I2C Address Translator (ATR) support"
> + help
> + Enable support for I2C Address Translator (ATR) chips.
> +
> + An ATR allows accessing multiple I2C busses from a single
> + physical bus via address translation instead of bus selection as
> + i2c-muxes do.
> +

I continued playing with the ROHM (de-)serializer and ended up having
.config where the I2C_ATR was ='m', while my ATR driver was ='y' even
though it selects the I2C_ATR.

Yep, most probably my error somewhere.

Anyways, this made me think that most of the I2C_ATR users are likely to
just silently select the I2C_ATR, right? The I2C_ATR has no much reason
to be compiled in w/o users, right? So perhaps the menu entry for
selecting the I2C_ATR could be dropped(?) Do we really need this entry
in already long list of configs to be manually picked?

snip

> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> + const struct i2c_atr_ops *ops, int max_adapters)
> +{
> + struct i2c_atr *atr;
> +
> + if (!ops || !ops->attach_client || !ops->detach_client)
> + return ERR_PTR(-EINVAL);
> +

I believe that most of the attach_client implementations will have
similar approach of allocating and populating an address-pool and
searching for first unused address. As a 'further dev' it'd be great to
see a common helper implementation for attach/detach - perhaps so that
the atr drivers would only need to specify the slave-address
configuration register(s) / mask and the use a 'generic' attach/detach
helpers. Well, just thinking how to reduce the code from actual IC
drivers but this is really not something that is required during this
initial series :)

Also, devm-variants would be great - although that falls to the same
category of things that do not need to be done immediately - but would
perhaps be worth considering in the future.

so, perhaps reconsider the Kconfig but for what-ever it is worth:

Reviewed-by: Matti Vaittinen <[email protected]>

Yours
Matti

--
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~