lkml.org 
[lkml]   [2023]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v7 1/7] i2c: add I2C Address Translator (ATR) support
    From
    On 19/01/2023 13:35, Luca Ceresoli wrote:
    > Hi Tomi, Andy,
    >
    > On Thu, 19 Jan 2023 12:09:57 +0200
    > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
    >
    >> On 19/01/2023 10:21, Luca Ceresoli wrote:
    >>
    >> <snip>
    >>
    >>>>>>> +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?

    > 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 =).

    > 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?

    That said, I'm also fine with i2c_atr_set_drvdata if that's the popular
    opinion (between the three of us, so far ;).

    Tomi

    \
     
     \ /
      Last update: 2023-03-26 23:47    [W:5.331 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site