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@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.
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
