Re: [PATCH v3 1/2] iio: adc: mcp3422: describe parts with chip_info

[Jonathan Cameron <jic23@kernel.org>]

On Sat,  3 Oct 2026 03:16:46 +0300
Yuval Saar <thefireking@gmail.com> wrote:

> The driver encodes MCP3421-8 differences as integer ids and switches
> on them in probe. Point i2c_device_id.driver_data at a per-chip
> structure instead, and keep the channel list and 3 SPS support there.
> 
> Compile tested. No hardware.

Ok. This is a little marginal for a patch to do without any form
of test.  You could look at the various ways this sort of patch
can be tested.  Personally I'd probably hack just the DT into qemu
but that's because it is the tool I am familiar with.  

Otherwise the below is preexisting issues but ones that your code
is touching on so I'd like them fixed as part of this.

Thanks,

Jonathan


> 
> Assisted-by: LLM
> Signed-off-by: Yuval Saar <thefireking@gmail.com>

> ---
>  drivers/iio/adc/mcp3422.c | 103 ++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index 36ba00edf..92825aaf2 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -320,7 +374,6 @@ static const struct iio_info mcp3422_info = {
>  
>  static int mcp3422_probe(struct i2c_client *client)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	struct iio_dev *indio_dev;
>  	struct mcp3422 *adc;
>  	int err;
> @@ -335,33 +388,17 @@ static int mcp3422_probe(struct i2c_client *client)
>  
>  	adc = iio_priv(indio_dev);
>  	adc->i2c = client;
> -	adc->id = (u8)(id->driver_data);
> +	adc->chip_info = i2c_get_match_data(client);

For the one ID that is currently in the of_match_table, this will return NULL
so it's not a bug as that will then fallback to doing of_device_id matching
but is not how this stuff is intended to work.  See below.

> +	if (!adc->chip_info)
> +		return -ENODEV;
>  
>  	mutex_init(&adc->lock);

>  
>  	/* meaningful default configuration */
>  	config = MCP3422_CONT_SAMPLING |
> @@ -382,14 +419,14 @@ static int mcp3422_probe(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id mcp3422_id[] = {
> -	{ .name = "mcp3421", .driver_data = 1 },
> -	{ .name = "mcp3422", .driver_data = 2 },
> -	{ .name = "mcp3423", .driver_data = 3 },
> -	{ .name = "mcp3424", .driver_data = 4 },
> -	{ .name = "mcp3425", .driver_data = 5 },
> -	{ .name = "mcp3426", .driver_data = 6 },
> -	{ .name = "mcp3427", .driver_data = 7 },
> -	{ .name = "mcp3428", .driver_data = 8 },
> +	{ .name = "mcp3421", .driver_data = (kernel_ulong_t)&mcp3421_chip_info },
> +	{ .name = "mcp3422", .driver_data = (kernel_ulong_t)&mcp3422_chip_info },
> +	{ .name = "mcp3423", .driver_data = (kernel_ulong_t)&mcp3423_chip_info },
> +	{ .name = "mcp3424", .driver_data = (kernel_ulong_t)&mcp3424_chip_info },
> +	{ .name = "mcp3425", .driver_data = (kernel_ulong_t)&mcp3425_chip_info },
> +	{ .name = "mcp3426", .driver_data = (kernel_ulong_t)&mcp3426_chip_info },
> +	{ .name = "mcp3427", .driver_data = (kernel_ulong_t)&mcp3427_chip_info },
> +	{ .name = "mcp3428", .driver_data = (kernel_ulong_t)&mcp3428_chip_info },

Any idea why this driver has only one of_table_id entry?  I'd generally expect
that to mirror what we have here.  The issue goes all the way back but let us
clean it up as part of this series.  Then use i2c_get_match_data() which will
check for matches in each type of firmware table, starting here with the of
one.


>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, mcp3422_id);