Date:   Fri, 17 Mar 2023 10:16:06 +0100
From:   Luca Ceresoli <luca.ceresoli@bootlin.com>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Wolfram Sang <wsa@kernel.org>
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: <20230317101606.69602bba@booty>
In-Reply-To: <20230222132907.594690-2-tomi.valkeinen@ideasonboard.com>
References: <20230222132907.594690-1-tomi.valkeinen@ideasonboard.com>
        <20230222132907.594690-2-tomi.valkeinen@ideasonboard.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Tomi, Wolfram,

On Wed, 22 Feb 2023 15:29:00 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> From: Luca Ceresoli <luca@lucaceresoli.net>
> 
> 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 it
> 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.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Wolfram, I think Tomi improved this work as much as currently possible
and this patch now looks extremely good to me. I wish we had this in
mainline soon. Does it make sense for me to send a Reviewed-by tag,
given I already have a S-o-b one?

I have a few _extremely_ minor notes below, but I hope they won't
slow down merging this work. They can definitely be addressed as a
follow-up patch after merging this.

Thank you a lot Tomi for having persisted in improving the ATR code!

> 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
> +=====================
> +
> +Author: Luca Ceresoli <luca@lucaceresoli.net>
> +Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.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
> +driver.
> +
> +The ATR creates a new I2C "child" adapter on each child bus. Adding
> +devices on the child bus ends up in invoking the driver code to select
> +an available alias. Maintaining an appropriate pool of available aliases
> +and picking one for each new device is up to the driver implementer. The
> +ATR maintains an table of currently assigned alias and uses it to modify

s/an table/a table/

> +all I2C transactions directed to devices on the child buses.
> +
> +A typical example follows.
> +
> +Topology::
> +
> +                      Slave X @ 0x10
> +              .-----.   |
> +  .-----.     |     |---+---- B
> +  | CPU |--A--| ATR |
> +  `-----'     |     |---+---- C
> +              `-----'   |
> +                      Slave Y @ 0x10
> +
> +Alias table:
> +
> +A, B and C are three physical I2C busses, electrically independent from
> +each other. The ATR receives the transactions initiated on bus A and
> +propagates them on bus B or bus C or none depending on the device address
> +in the transaction and based on the alias table.
> +
> +Alias table:
> +
> +.. table::
> +
> +   ===============   =====
> +   Client            Alias
> +   ===============   =====
> +   X (bus B, 0x10)   0x20
> +   Y (bus C, 0x10)   0x30
> +   ===============   =====
> +
> +Transaction:
> +
> + - Slave X driver sends a transaction (on adapter B), slave address 0x10

s/sends/requests/ is possibly better to clarify there is still no
electrical transaction yet at this step, as we are still in software.

> + - ATR driver finds slave X is on bus B and has alias 0x20, rewrites
> +   messages with address 0x20, forwards to adapter A
> + - Physical I2C transaction on bus A, slave address 0x20
> + - ATR chip detects transaction on address 0x20, finds it in table,
> +   propagates transaction on bus B with address translated to 0x10,
> +   keeps clock streched on bus A waiting for reply
> + - Slave X chip (on bus B) detects transaction at its own physical
> +   address 0x10 and replies normally
> + - ATR chip stops clock stretching and forwards reply on bus A,
> +   with address translated back to 0x20
> + - ATR driver receives the reply, rewrites messages with address 0x10
> +   as they were initially
> + - Slave X driver gets back the msgs[], with reply and address 0x10
> +
> +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
> +
> +I2C ATR functions and data structures
> +-------------------------------------
> +
> +.. kernel-doc:: include/linux/i2c-atr.h

...

> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> new file mode 100644
> index 000000000000..5ab890b83670
> --- /dev/null
> +++ b/drivers/i2c/i2c-atr.c
> @@ -0,0 +1,548 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I2C Address Translator
> + *
> + * Copyright (c) 2019,2022 Luca Ceresoli <luca@lucaceresoli.net>
> + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + *
> + * Originally based on i2c-mux.c

Not quite anymore I think... should this line be removed?

> +/**
> + * struct i2c_atr - The I2C ATR instance
> + * @parent:    The parent &struct i2c_adapter
> + * @dev:       The device that owns the I2C ATR instance
> + * @ops:       &struct i2c_atr_ops
> + * @priv:      Private driver data, set with i2c_atr_set_driver_data()
> + * @algo:      The &struct i2c_algorithm for adapters
> + * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
> + * @max_adapters: Maximum number of adapters this I2C ATR can have
> + * @adapter:   Array of adapters
> + */
> +struct i2c_atr {
> +	struct i2c_adapter *parent;
> +	struct device *dev;
> +	const struct i2c_atr_ops *ops;
> +
> +	void *priv;
> +
> +	struct i2c_algorithm algo;
> +	/* lock for the I2C bus segment (see struct i2c_lock_operations) */

This comment is identical to the one in the kerneldoc comments just
above, I'd just remove it.

> +	struct mutex lock;
> +	int max_adapters;
> +
> +	struct notifier_block i2c_nb;

Undocumented?

...

> +void i2c_atr_delete(struct i2c_atr *atr)
> +{

Maybe here we could iterate over atr->adapter[] and if any is != NULL
just call BUG_ON() or WARN()?

> +	bus_unregister_notifier(&i2c_bus_type, &atr->i2c_nb);
> +	mutex_destroy(&atr->lock);
> +	kfree(atr);
> +}
> +EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR);

...

> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> new file mode 100644
> index 000000000000..7596f70ce1ab
> --- /dev/null
> +++ b/include/linux/i2c-atr.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * I2C Address Translator
> + *
> + * Copyright (c) 2019,2022 Luca Ceresoli <luca@lucaceresoli.net>
> + * Copyright (c) 2022,2023 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + *
> + * Based on i2c-mux.h

As above, this does not apply very much anymore as it did in v1.

...

> +/**
> + * i2c_atr_delete - Delete an I2C ATR helper.
> + * @atr: I2C ATR helper to be deleted.
> + *
> + * Precondition: all the adapters added with i2c_atr_add_adapter() mumst be

s/mumst/must/

Best regards,
Luca

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