On Sun, 23 Jul 2023 06:12:20 +0000
Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> Hi Peter Rosin,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use
> > i2c_get_match_data()
> >
> > Hi!
> >
> > 2023-07-21 at 09:16, Biju Das wrote:
> > > Replace of_device_get_match_data() and i2c_match_id() by i2c_get_match
> > > _data() by making similar I2C and DT-based matching table.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> > > v1->v2:
> > > * Added similar similar I2C and DT-based matching table.
> > > * Fixed typo i2c_get_match_data(dev)->i2c_get_match_data(client).
> > > * Dropped error check as all tables have data pointers.
> > >
> > > Note:
> > > This patch is only compile tested.
> > > ---
> > > drivers/iio/potentiometer/mcp4018.c | 34
> > > +++++++++++++++--------------
> > > 1 file changed, 18 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/potentiometer/mcp4018.c
> > > b/drivers/iio/potentiometer/mcp4018.c
> > > index 89daecc90305..b064e86ecce8 100644
> > > --- a/drivers/iio/potentiometer/mcp4018.c
> > > +++ b/drivers/iio/potentiometer/mcp4018.c
> > > @@ -99,20 +99,24 @@ static const struct iio_info mcp4018_info = {
> > > .write_raw = mcp4018_write_raw,
> > > };
> > >
> > > +#define MCP4018_ID_TABLE(name, cfg) { \
> > > + name, .driver_data = (kernel_ulong_t)&mcp4018_cfg[cfg], \
> > > +}
> >
> > It is inconsistent to have a named field for .driver_data but not for
> > .name. Also, I dislike the cast and wonder if the trivial simplification
> > in probe() is really worth this churn when that ugly cast is needed?
>
> It saving lines of code and better than, &mcp4018_cfg[i2c_match_id(mcp4018_id, client)>driver_data]; right??
>
>
> >The
> > reason the two tables differ and do not both have pointers already is
> > precisely my dislike for that cast.
>
> >
> > Anyway, something like this instead? Or _name instead of id?
> > Whatever...
>
> OK, will use _name.
I dropped previous version of this and the other patch (as comment applies
there as well) from the togreg branch of iio.git
>
> >
> > #define MCP4018_ID_TABLE(id, cfg) { \
> > .name = id, \
> > .driver_data = (kernel_ulong_t)&mcp4018_cfg[cfg], \
> > }
> >
> > > +
> > > static const struct i2c_device_id mcp4018_id[] = {
> > > - { "mcp4017-502", MCP4018_502 },
> > > - { "mcp4017-103", MCP4018_103 },
> > > - { "mcp4017-503", MCP4018_503 },
> > > - { "mcp4017-104", MCP4018_104 },
> > > - { "mcp4018-502", MCP4018_502 },
> > > - { "mcp4018-103", MCP4018_103 },
> > > - { "mcp4018-503", MCP4018_503 },
> > > - { "mcp4018-104", MCP4018_104 },
> > > - { "mcp4019-502", MCP4018_502 },
> > > - { "mcp4019-103", MCP4018_103 },
> > > - { "mcp4019-503", MCP4018_503 },
> > > - { "mcp4019-104", MCP4018_104 },
> > > - {}
> > > + MCP4018_ID_TABLE("mcp4017-502", MCP4018_502),
> > > + MCP4018_ID_TABLE("mcp4017-103", MCP4018_103),
> > > + MCP4018_ID_TABLE("mcp4017-503", MCP4018_503),
> > > + MCP4018_ID_TABLE("mcp4017-104", MCP4018_104),
> > > + MCP4018_ID_TABLE("mcp4018-502", MCP4018_502),
> > > + MCP4018_ID_TABLE("mcp4018-103", MCP4018_103),
> > > + MCP4018_ID_TABLE("mcp4018-503", MCP4018_503),
> > > + MCP4018_ID_TABLE("mcp4018-104", MCP4018_104),
> > > + MCP4018_ID_TABLE("mcp4019-502", MCP4018_502),
> > > + MCP4018_ID_TABLE("mcp4019-103", MCP4018_103),
> > > + MCP4018_ID_TABLE("mcp4019-503", MCP4018_503),
> > > + MCP4018_ID_TABLE("mcp4019-104", MCP4018_104),
> > > + { /* sentinel */ }
> > > };
> > > MODULE_DEVICE_TABLE(i2c, mcp4018_id);
> > >
> > > @@ -157,9 +161,7 @@ static int mcp4018_probe(struct i2c_client
> > *client)
> > > i2c_set_clientdata(client, indio_dev);
> > > data->client = client;
> > >
> > > - data->cfg = device_get_match_data(dev);
> > > - if (!data->cfg)
> > > - data->cfg = &mcp4018_cfg[i2c_match_id(mcp4018_id, client)-
> > >driver_data];
> > > + data->cfg = i2c_get_match_data(client);
> >
> > NULL-check here? I know the original i2c_match_id call is not checked
> > for non-matches, but that feels like a simpler function than
> > i2c_get_match_data. And less prone to changes.
> >
> > Same comments of course applies to the mcp4531 patch as well.
>
> Some subsystem people doesn't want error check as all tables have data pointers. See [1]
>
> [1] https://lore.kernel.org/linux-renesas-soc/20230717145623.473cffca@booty/
>
> I am leaving subsystem maintainer to take final word on this.
I'm fine either way. It seems unlikely any changes to that function will introduce
a path for a NULL return if there isn't one there today, but you never know I guess.
However we don't normally spend too much effort protecting against potential
future new return values.
>
> The new API is very simple [2]
> [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L124
>
> Cheers,
> Biju
>
> > >
> > > indio_dev->info = &mcp4018_info;
> > > indio_dev->channels = &mcp4018_channel;
