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]

Message-ID: <ZD6oPq+Na/80E7Mv@shikoro>
Date:   Tue, 18 Apr 2023 16:25:02 +0200
From:   Wolfram Sang <wsa@...nel.org>
To:     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>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        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 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"?

> +=====================
> +
> +Author: Luca Ceresoli <luca@...aceresoli.net>
> +Author: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> +
> +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?

> +driver.

...

> +Usage:
> +
> + 1. In your driver (typically in the probe function) add an ATR by
> +    calling i2c_atr_new() passing your attach/detach callbacks
> + 2. When the attach callback is called pick an appropriate alias,
> +    configure it in your chip and return the chosen alias in the
> +    alias_id parameter
> + 3. When the detach callback is called, deconfigure the alias from
> +    your chip and put it back in the pool for later usage

Remove all "your", please. Some can simply go, I'd say. The others
replaced by "the".

> +
> +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'

> + * @node:   List node
> + * @client: Pointer to the client on the child bus
> + * @alias:  I2C alias address assigned by the driver.
> + *          This is the address that will be used to issue I2C transactions
> + *          on the parent (physical) bus.
> + */

> +EXPORT_SYMBOL_NS_GPL(i2c_atr_add_adapter, I2C_ATR);

EXPORT_SYMBOL_GPL, please. We can then later think about using an I2C
namespace for all I2C symbols.

Pretty high level comments only so far. I'll keep at it this week and
might come back with more detailed comments. But in general, this looks
quite good to go. Moving the alias pool handling to here is the biggest
question I have.

Thank you for your patience!

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)