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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux