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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2023 09:13:26 +0200
From:   Luca Ceresoli <luca.ceresoli@...tlin.com>
To:     Wolfram Sang <wsa@...nel.org>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>,
        Satish Nagireddy <satish.nagireddy@...cruise.com>,
        Luca Ceresoli <luca@...aceresoli.net>
Subject: Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support

Hi Wolfram, Tomi,

On Tue, 18 Apr 2023 16:25:02 +0200
Wolfram Sang <wsa@...nel.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