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,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2] iio: potentiometer: mcp4018: Use i2c_get_match_data()
Date: Sat, 22 Jul 2023 23:39:31 +0200	[thread overview]
Message-ID: <6e5a099e-57bb-7eb3-212b-494b5c30e4d7@axentia.se> (raw)
In-Reply-To: <20230721071603.158114-1-biju.das.jz@bp.renesas.com>

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;