Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

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

Hi Wolfram, Tomi,

On Tue, 18 Apr 2023 16:25:02 +0200
Wolfram Sang <wsa@kernel.org> wrote:

> Hi Tomi, hi Luca,
> 
> as mentioned on IRC already, good move to use bus notifiers here and
> drop the generic attach/detach callbacks. Those were a show stopper for
> me. This version is nicely self contained. I like that!
> 
> > diff --git a/Documentation/i2c/index.rst b/Documentation/i2c/index.rst
> > index 6270f1fd7d4e..aaf33d1315f4 100644
> > --- a/Documentation/i2c/index.rst
> > +++ b/Documentation/i2c/index.rst
> > @@ -16,6 +16,7 @@ Introduction
> >     instantiating-devices
> >     busses/index
> >     i2c-topology
> > +   muxes/i2c-atr  
> 
> The muxes-dir is only for the description of mux drivers. I'd prefer to
> have this document not in the sub-dir. Also, renaming the document to
> "address-translations.rst" might be worth discussing.
> 
> >     muxes/i2c-mux-gpio
> >     i2c-sysfs
> >  
> > diff --git a/Documentation/i2c/muxes/i2c-atr.rst b/Documentation/i2c/muxes/i2c-atr.rst
> > new file mode 100644
> > index 000000000000..da226fd4de63
> > --- /dev/null
> > +++ b/Documentation/i2c/muxes/i2c-atr.rst
> > @@ -0,0 +1,97 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=====================
> > +Kernel driver i2c-atr  
> 
> Maybe "I2C address translations"?

Even better: "I2C address translator dirvers", or just "I2C address
translators"? Here we document a facility to implement a driver for
an address translator, and discussion on "address translation" in
general is just a part of it. Same for the filename.

Uh, I guess this was a journey in the realm of nitpicking, sorry... :)

> > +Description
> > +-----------
> > +
> > +An I2C Address Translator (ATR) is a device with an I2C slave parent
> > +("upstream") port and N I2C master child ("downstream") ports, and
> > +forwards transactions from upstream to the appropriate downstream port
> > +with a modified slave address. The address used on the parent bus is
> > +called the "alias" and is (potentially) different from the physical
> > +slave address of the child bus. Address translation is done by the
> > +hardware.
> > +
> > +An ATR looks similar to an i2c-mux except:
> > + - the address on the parent and child busses can be different
> > + - there is normally no need to select the child port; the alias used on the
> > +   parent bus implies it
> > +
> > +The ATR functionality can be provided by a chip with many other
> > +features. This file provides a helper to implement an ATR within your  
> 
> I'd like to get rid of all "your". Maybe "client driver" here?

I agree.

> > +
> > +I2C ATR functions and data structures
> > +-------------------------------------
> > +  
> 
> ...
> 
> > +/**
> > + * struct i2c_atr_cli2alias_pair - Holds the alias assigned to a client.  
> 
> I stumbled over this one because "cli" is "command line interface" for
> me... The long version isn't much longer: 'i2c_atr_client_alias_pair'
> But I'd be also fine with: 'i2c_atr_alias_pair'

The short one looks better to me. The only "alias" involved in ATRs is
the client alias, thus no ambiguity.

Best regards,
Luca

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