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.
>
> #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.
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;
