Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 19 Apr 2023 09:13:26 +0200
From:   Luca Ceresoli <luca.ceresoli@bootlin.com>
To:     Wolfram Sang <wsa@kernel.org>,
        Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        Matti Vaittinen <Matti.Vaittinen@fi.rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Peter Rosin <peda@axentia.se>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Michael Tretter <m.tretter@pengutronix.de>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Mike Pagano <mpagano@gentoo.org>,
        Krzysztof =?UTF-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>,
        Marek Vasut <marex@denx.de>,
        Satish Nagireddy <satish.nagireddy@getcruise.com>,
        Luca Ceresoli <luca@lucaceresoli.net>
Subject: Re: [PATCH v10 1/8] i2c: add I2C Address Translator (ATR) support
Message-ID: <20230419091326.1327018d@booty>
In-Reply-To: <ZD6oPq+Na/80E7Mv@shikoro>
References: <20230222132907.594690-1-tomi.valkeinen@ideasonboard.com>
        <20230222132907.594690-2-tomi.valkeinen@ideasonboard.com>
        <ZD6oPq+Na/80E7Mv@shikoro>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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