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

From: Peter Rosin <peda@axentia.se>
To: Biju Das <biju.das.jz@bp.renesas.com>,
	Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use i2c_get_match_data()
Date: Sun, 23 Jul 2023 12:36:16 +0200	[thread overview]
Message-ID: <cf71c4ab-58c4-fef8-2ce3-5ca32f9f2f91@axentia.se> (raw)
In-Reply-To: <OS0PR01MB592290B83D0EEA1928DB4D2A863DA@OS0PR01MB5922.jpnprd01.prod.outlook.com>

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;