| Message ID | 20230721071603.158114-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] iio: potentiometer: mcp4018: Use i2c_get_match_data() | expand |
On Fri, 21 Jul 2023 08:16:03 +0100 Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> Applied to the togreg branch of iio.git and pushed out as testing for autobuilders to have a poke at it. Thanks, Jonathan > --- > 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], \ > +} > + > 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); > > indio_dev->info = &mcp4018_info; > indio_dev->channels = &mcp4018_channel;
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@bp.renesas.com> > --- > 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? 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... #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. Cheers, Peter > > indio_dev->info = &mcp4018_info; > indio_dev->channels = &mcp4018_channel;
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@bp.renesas.com> > > --- > > 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;
On Sun, 23 Jul 2023 06:12:20 +0000 Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > > --- > > > 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;
Hi! 2023-07-23 at 08:12, Biju Das 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@bp.renesas.com> >>> --- >>> 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?? If saving lines is your goal, you have failed with this +18-16 patch, even with this compressed way of expressing things that could be expressed more clearly with an extra line. Not that a couple of extra lines would have mattered if the change would have otherwise been an improvement. And no, I don't think the cast is better than the existing code. We could of course argue about that, but it quickly descends into a bikeshed discussion. My point is that this patch trades one ugliness for another while bringing in no real change. It is thus not a clear improvement to me, and I question its value. What is the point? Why not work on something that is going to make a real difference, such as unifying the module device tables so that drivers don't need to add almost-duplicated tables, or something, instead of only doing minor syntax changes for expressing the same thing? With a single unified table, it would be very natural to have the same match data everywhere... > >> 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 I have of course read that already. Even if it is simple, the new API is decidedly more complex than old, since the new API wraps the old and adds stuff around it. But do note that I am not pushing hard for adding a NULL-check. Cheers, Peter > > Cheers, > Biju > >>> >>> indio_dev->info = &mcp4018_info; >>> indio_dev->channels = &mcp4018_channel;
Hi Peter Rosin, Thanks for the feedback. > Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use > i2c_get_match_data() > > Hi! > > 2023-07-23 at 08:12, Biju Das 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@bp.renesas.com> > >>> --- > >>> 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?? > > If saving lines is your goal, you have failed with this +18-16 patch, > even with this compressed way of expressing things that could be > expressed more clearly with an extra line. Not that a couple of extra > lines would have mattered if the change would have otherwise been an > improvement. And no, I don't think the cast is better than the existing > code. We could of course argue about that, but it quickly descends into > a bikeshed discussion. > > My point is that this patch trades one ugliness for another while > bringing in no real change. It is thus not a clear improvement to me, > and I question its value. What is the point? > > Why not work on something that is going to make a real difference, such > as unifying the module device tables so that drivers don't need to add > almost-duplicated tables, or something, instead of only doing minor > syntax changes for expressing the same thing? With a single unified > table, it would be very natural to have the same match data > everywhere... A single API for getting match data for OF/ACPI/I2C match tables. Jonathan asked me to do this change, that is the reason I have posted this patch. Cheers, Biju > > > > >> 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://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Flinux-renesas-soc%2F20230717145623.473cffca%40booty%2F&d > > ata=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C8aeb0ccf13944b668d9908db8 > > b68ad17%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63825705391950373 > > 3%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6 > > Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=JoNl4I8FzcvgcGhppE75W2Ae7 > > 7bkdioKq5CjOHTtsn0%3D&reserved=0 > > > > I am leaving subsystem maintainer to take final word on this. > > > > The new API is very simple [2] > > [2] > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix > > ir.bootlin.com%2Flinux%2Fv6.5-rc2%2Fsource%2Fdrivers%2Fi2c%2Fi2c-core- > > base.c%23L124&data=05%7C01%7Cbiju.das.jz%40bp.renesas.com%7C8aeb0ccf13 > > 944b668d9908db8b68ad17%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C63 > > 8257053919503733%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV > > 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GVYVg%2Fo7 > > AxujITu4AvgDkRKymYw%2BGtv%2FGPDmSPBn%2FZU%3D&reserved=0 > > I have of course read that already. Even if it is simple, the new API is > decidedly more complex than old, since the new API wraps the old and > adds stuff around it. > > But do note that I am not pushing hard for adding a NULL-check. > > Cheers, > Peter > > > > > Cheers, > > Biju > > > >>> > >>> indio_dev->info = &mcp4018_info; > >>> indio_dev->channels = &mcp4018_channel;
Hi Peter Rosin, > Subject: RE: [PATCH v2] iio: potentiometer: mcp4018: Use > i2c_get_match_data() > > Hi Peter Rosin, > > Thanks for the feedback. > > > Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use > > i2c_get_match_data() > > > > Hi! > > > > 2023-07-23 at 08:12, Biju Das 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@bp.renesas.com> > > >>> --- > > >>> 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?? > > > > If saving lines is your goal, you have failed with this +18-16 patch, > > even with this compressed way of expressing things that could be > > expressed more clearly with an extra line. Not that a couple of extra > > lines would have mattered if the change would have otherwise been an > > improvement. And no, I don't think the cast is better than the > > existing code. We could of course argue about that, but it quickly > > descends into a bikeshed discussion. > > > > My point is that this patch trades one ugliness for another while > > bringing in no real change. It is thus not a clear improvement to me, > > and I question its value. What is the point? > > > > Why not work on something that is going to make a real difference, > > such as unifying the module device tables so that drivers don't need > > to add almost-duplicated tables, or something, instead of only doing > > minor syntax changes for expressing the same thing? With a single > > unified table, it would be very natural to have the same match data > > everywhere... > > A single API for getting match data for OF/ACPI/I2C match tables. > Jonathan asked me to do this change, that is the reason I have posted > this patch. You could split mcp4018_cfg[] as individual struct struct mcp4018_cfg and drop enum mcp4018_type altogether as an optimization. With your current macro code, dropping enum is not possible. Cheers, Biju
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], \ +} + 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); indio_dev->info = &mcp4018_info; indio_dev->channels = &mcp4018_channel;
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@bp.renesas.com> --- 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(-)