linux-kernel - Re: [RFCv3 2/6] 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: <a8796cde-e97b-7157-33ac-1b6020053c5d@fi.rohmeurope.com>
Date:   Wed, 16 Mar 2022 14:11:27 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Luca Ceresoli <luca@...aceresoli.net>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
CC:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Peter Rosin <peda@...ntia.se>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        "Mutanen, Mikko" <Mikko.Mutanen@...rohmeurope.com>
Subject: Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support

Hi dee Ho peeps!

On 2/6/22 13:59, Luca Ceresoli wrote:
> 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 is
> 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.
> 

snip

> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 438905e2a1d0..c6d1a345ea6d 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -71,6 +71,15 @@ config I2C_MUX
>   
>   source "drivers/i2c/muxes/Kconfig"
>   
> +config I2C_ATR
> +	tristate "I2C Address Translator (ATR) support"
> +	help
> +	  Enable support for I2C Address Translator (ATR) chips.
> +
> +	  An ATR allows accessing multiple I2C busses from a single
> +	  physical bus via address translation instead of bus selection as
> +	  i2c-muxes do.
> +

I continued playing with the ROHM (de-)serializer and ended up having 
.config where the I2C_ATR was ='m', while my ATR driver was ='y' even 
though it selects the I2C_ATR.

Yep, most probably my error somewhere.

Anyways, this made me think that most of the I2C_ATR users are likely to 
just silently select the I2C_ATR, right? The I2C_ATR has no much reason 
to be compiled in w/o users, right? So perhaps the menu entry for 
selecting the I2C_ATR could be dropped(?) Do we really need this entry 
in already long list of configs to be manually picked?


snip

> +struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> +			    const struct i2c_atr_ops *ops, int max_adapters)
> +{
> +	struct i2c_atr *atr;
> +
> +	if (!ops || !ops->attach_client || !ops->detach_client)
> +		return ERR_PTR(-EINVAL);
> +

I believe that most of the attach_client implementations will have 
similar approach of allocating and populating an address-pool and 
searching for first unused address. As a 'further dev' it'd be great to 
see a common helper implementation for attach/detach - perhaps so that 
the atr drivers would only need to specify the slave-address 
configuration register(s) / mask and the use a 'generic' attach/detach 
helpers. Well, just thinking how to reduce the code from actual IC 
drivers but this is really not something that is required during this 
initial series :)

Also, devm-variants would be great - although that falls to the same 
category of things that do not need to be done immediately - but would 
perhaps be worth considering in the future.

so, perhaps reconsider the Kconfig but for what-ever it is worth:

Reviewed-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>


Yours
	Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~