Re: [PATCH v5 3/5] staging: iio: adc: ad7816: Introduce chip_info and use pointer matching

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 19 Apr 2025 21:49:08 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:

> Introduce struct ad7816_chip_info to centralize static properties (e.g.
> name, max channels) that differ between chip variants (AD7816/7/8) but
> are constant for any specific type.
> 
> Store pointers to these instances in the of_device_id (.data) and
> spi_device_id (driver_data) tables. Retrieve the pointer in probe()
> using the firmware-independent device_get_match_data() and store it in
> the ad7816_state struct.
> 
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
Hi Gabriel

A few minor things inline.

I think this went a little too far in splitting up changes and you should combine
patches 3 and 4 to avoid a nasty intermediate bit of code.

Jonathan

> ---
>  drivers/staging/iio/adc/ad7816.c | 55 +++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index cad2e55aff3f9..39310ade770d0 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -41,8 +41,28 @@
>   * struct ad7816_state - chip specific information
>   */
>  
> +struct ad7816_chip_info {
> +	const char *name;
> +	u8 max_channels;

Something called max_channels should be the number of channels.
If you called it max_channel then it could be used for the maximum channel
number present. I assume that is what you have here as otherwise the
ad7816 has no channels which seems odd!

> +};
> +
> +static const struct ad7816_chip_info ad7816_info_ad7816 = {
> +	.name = "ad7816",
> +	.max_channels = 0,
> +};
> +
> +static const struct ad7816_chip_info ad7817_info_ad7817 = {
> +	.name = "ad7817",
> +	.max_channels = 3,
> +};
> +
> +static const struct ad7816_chip_info ad7818_info_ad7818 = {
> +	.name = "ad7818",
> +	.max_channels = 1,
> +};
> +
>  struct ad7816_state {
> -	kernel_ulong_t id;
> +	const struct ad7816_chip_info *chip_info;
>  	struct spi_device *spi_dev;
>  	struct gpio_desc *rdwr_pin;
>  	struct gpio_desc *convert_pin;
> @@ -52,12 +72,6 @@ struct ad7816_state {
>  	u8  mode;
>  };
>  
> -enum ad7816_type {
> -	ID_AD7816,
> -	ID_AD7817,
> -	ID_AD7818,
> -};
> -
>  /*
>   * ad7816 data access by SPI
>   */
> @@ -84,7 +98,7 @@ static int ad7816_spi_read(struct ad7816_state *chip, u16 *data)
>  		gpiod_set_value(chip->convert_pin, 1);
>  	}
>  
> -	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> +	if (chip->chip_info == &ad7816_info_ad7816 || chip->chip_info == &ad7817_info_ad7817) {

Hmm. I'd be tempted to squash the next patch with this one simply to avoid this
horrible intermediate state where you match on pointers.

It is all part of moving from the enum over to data so I think is
fine in one patch.

>  		while (gpiod_get_value(chip->busy_pin))
>  			cpu_relax();
>  	}
> @@ -353,6 +367,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  {
>  	struct ad7816_state *chip;
>  	struct iio_dev *indio_dev;
> +	const struct ad7816_chip_info *info;
>  	int i, ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi_dev->dev, sizeof(*chip));
> @@ -362,11 +377,15 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  	/* this is only used for device removal purposes */
>  	dev_set_drvdata(&spi_dev->dev, indio_dev);
>  
> +	info = device_get_match_data(&spi_dev->dev);
> +	if (!info)
> +		return -ENODEV;
> +	chip->chip_info = info;
> +
>  	chip->spi_dev = spi_dev;
>  	for (i = 0; i <= AD7816_CS_MAX; i++)
>  		chip->oti_data[i] = 203;
>  
> -	chip->id = spi_get_device_id(spi_dev)->driver_data;
>  	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
>  	if (IS_ERR(chip->rdwr_pin)) {
>  		ret = PTR_ERR(chip->rdwr_pin);
> @@ -382,7 +401,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  			ret);
>  		return ret;
>  	}
> -	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> +	if (chip->chip_info == &ad7816_info_ad7816 || chip->chip_info == &ad7817_info_ad7817) {
>  		chip->busy_pin = devm_gpiod_get(&spi_dev->dev, "busy",
>  						GPIOD_IN);
>  		if (IS_ERR(chip->busy_pin)) {
> @@ -393,7 +412,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  		}
>  	}
>  
> -	indio_dev->name = spi_get_device_id(spi_dev)->name;
> +	indio_dev->name = chip->chip_info->name;
>  	indio_dev->info = &ad7816_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> @@ -420,18 +439,18 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  }
>  
>  static const struct of_device_id ad7816_of_match[] = {
> -	{ .compatible = "adi,ad7816", },
> -	{ .compatible = "adi,ad7817", },
> -	{ .compatible = "adi,ad7818", },
> +	{ .compatible = "adi,ad7816", .data = &ad7816_info_ad7816 },
> +	{ .compatible = "adi,ad7817", .data = &ad7817_info_ad7817 },
> +	{ .compatible = "adi,ad7818", .data = &ad7818_info_ad7818 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ad7816_of_match);
>  
>  static const struct spi_device_id ad7816_id[] = {
> -	{ "ad7816", ID_AD7816 },
> -	{ "ad7817", ID_AD7817 },
> -	{ "ad7818", ID_AD7818 },
> -	{}
> +	{ "ad7816", (kernel_ulong_t)&ad7816_info_ad7816 },
> +	{ "ad7817", (kernel_ulong_t)&ad7817_info_ad7817 },
> +	{ "ad7818", (kernel_ulong_t)&ad7818_info_ad7818 },
> +	{ }
>  };
>  
>  MODULE_DEVICE_TABLE(spi, ad7816_id);