diff mbox series

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

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

Commit Message

Biju Das July 21, 2023, 7:16 a.m. UTC
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(-)

Comments

Jonathan Cameron July 22, 2023, 5:18 p.m. UTC | #1
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;
Peter Rosin July 22, 2023, 9:39 p.m. UTC | #2
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;
Biju Das July 23, 2023, 6:12 a.m. UTC | #3
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;
Jonathan Cameron July 23, 2023, 9:12 a.m. UTC | #4
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;
Peter Rosin July 23, 2023, 10:36 a.m. UTC | #5
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;
Biju Das July 23, 2023, 10:41 a.m. UTC | #6
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;
Biju Das July 23, 2023, 11:50 a.m. UTC | #7
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 mbox series

Patch

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;