RE: [PATCH v2] iio: potentiometer: mcp4018: Use i2c_get_match_data()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux