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