Re: [PATCH v7 1/7] i2c: add I2C Address Translator (ATR) support

Hi Laurent,

On Fri, 20 Jan 2023 11:55:21 +0200
Laurent Pinchart [off-list ref] wrote:

Hello,

On Thu, Jan 19, 2023 at 02:00:56PM +0100, Luca Ceresoli wrote:

quoted

On Thu, 19 Jan 2023 14:22:26 +0200 Tomi Valkeinen wrote:  

quoted

On 19/01/2023 13:35, Luca Ceresoli wrote:  

quoted

On Thu, 19 Jan 2023 12:09:57 +0200 Tomi Valkeinen wrote:  

quoted

On 19/01/2023 10:21, Luca Ceresoli wrote:

<snip>
   

quoted

quoted

quoted

quoted

quoted

+void i2c_atr_set_driver_data(struct i2c_atr *atr, void *data)
+{
+	atr->priv = data;
+}
+EXPORT_SYMBOL_NS_GPL(i2c_atr_set_driver_data, I2C_ATR);
+
+void *i2c_atr_get_driver_data(struct i2c_atr *atr)
+{
+	return atr->priv;
+}
+EXPORT_SYMBOL_NS_GPL(i2c_atr_get_driver_data, I2C_ATR);    

Just to be sure: Is it really _driver_ data and not _device instance_ data?    

It is device instance data indeed. I don't remember why this got
changed, but in v3 it was i2c_atr_set_clientdata().    

It's me who was and is against calling it clientdata due to possible
confusion with i2c_set/get_clientdata() that is about *driver data*.
I missed that time the fact that this is about device instance data.
I dunno which name would be better in this case, i2c_atr_set/get_client_priv() ?    

Not sure I'm following you here. The i2c_atr_set_clientdata() name was
given for similarity with i2c_set_clientdata(). The latter wraps
dev_set_drvdata(), which sets `struct device`->driver_data. There is
one driver_data per each `struct device` instance, not per each driver.
The same goes for i2c_atr_set_driver_data(): there is one priv pointer
per each `struct i2c_atr` instance.    

I'm a bit confused. What is "driver data" and what is "device instance
data"?

This deals with the driver's private data, where the "driver" is the
owner/creator of the i2c-atr. The i2c-atr itself doesn't have a device
(it's kind of part of the owner's device), and there's no driver in
i2c-atr.c

I don't like "client" here, as it reminds me of i2c_client (especially
as we're in i2c context).

What about i2c_atr_set_user_data()? Or "owner_data"?    

Ah, only now I got the point Andy made initially about "client" not
being an appropriate word.

In i2c we have:

   i2c_set_clientdata(struct i2c_client *client, void *data)
           ^^^^^^~~~~            ^^^^^^                ~~~~

so "client" clearly makes sense there, now here.    

Isn't that also used by the i2c_client? A driver which handles an i2c 
device is the "i2c client", in a sense?
  

quoted

The same logic applied here would lead to:

   i2c_atr_set_atrdata(struct i2c_atr *atr, void *data)
               ^^^~~~~            ^^^             ~~~~

which makes sense but it is a ugly IMO.    

Here, I think, there's a bit of a difference to the i2c_client case, as 
we have a separate component for the i2c-atr. Although I guess one can 
argue that the top level driver is the ATR driver, as it handles the HW, 
and i2c-atr.c is just a set of helpers, so... I don't know =).
  

quoted

So I think i2c_atr_get_driver_data() in this v7 makes sense, it's to
set the data that the ATR driver instance needs.

This is coherent with logic in spi/spi.h:

   spi_set_drvdata(struct spi_device *spi, void *data)

except for the abbreviation ("_drvdata" vs "_driver_data").

Andy, Tomi, would i2c_atr_set_drvdata() be OK for you, just like SPI
does?    

Well, I'm good with the current i2c_atr_set_driver_data(). If all agrees 
that it's "driver data", I'd rather keep it like that. I find this 
"drvdata" style very odd. Why no underscore between drv and data? Why 
abbreviate drv, it doesn't really help anything here?  

Agreed, I'm OK with either form of "driver data".  

Have you considered allowing drivers to embed i2c_atr in a larger
structure, instead of forcing allocation through i2c_atr_new() ? Drivers
could then use container_of() instead of the get/set driver/device data
accessors.

Off the top of my head I don't see a good reason to not do it, and it
would be nice to have indeed.

For the sake of historical discussion, I guess I didn't do initially
just because my starting point was i2c-mux where allocation is dynamic.
But i2c_mux_alloc() also takes a 'int sizeof_priv' parameter to
allocate some extra space for private driver data. I don't love that
approach but it probably makes sense for mux devices which tend to be
very simple, not for the ATR where chips are definitely complex. Indeed
embedded i2c_atr in the larger driver-specific struct seems the best
option.

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