Re: [PATCH v4 08/11] iio: amplifiers: ad8366: prepare for device-tree support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Feb 2026 19:42:08 +0000
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Drop switch case on the enum ID in favor of extended chip info table,
> containing:
> - gain_step, indicating with sign the start of the code range;
> - num_channels, to indicate the number IIO channels;
> - pack_code() function to describe how SPI buffer is populated;
> 
> Which allowed for a simplified read_raw() and write_raw() callbacks.
> The probe() function was adjusted accordingly.

Making the reset call on all devices is a material change (probably fine)
that should be called out here.

> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>

> @@ -48,60 +53,58 @@ struct ad8366_state {
>  	unsigned char		data[2] __aligned(IIO_DMA_MINALIGN);
>  };
>  
> +static size_t ad8366_pack_code(struct ad8366_state *st)

See below. To me this is doing too much hidden stuff from point
of view of the caller. I think it needs some more explicit parameters.

> +{
> +	u8 ch_a = bitrev8(st->ch[0]) >> 2;
> +	u8 ch_b = bitrev8(st->ch[1]) >> 2;
> +
> +	put_unaligned_be16((ch_b << 6) | ch_a, &st->data[0]);
> +	return sizeof(__be16);
> +}

>  
> -static int ad8366_write(struct iio_dev *indio_dev,
> -			unsigned char ch_a, unsigned char ch_b)
> +static int ad8366_write_code(struct ad8366_state *st)
>  {
> -	struct ad8366_state *st = iio_priv(indio_dev);
> -	int ret;
> +	const struct ad8366_info *inf = st->info;
>  
> -	switch (st->type) {
> -	case ID_AD8366:
> -		ch_a = bitrev8(ch_a & 0x3F);
> -		ch_b = bitrev8(ch_b & 0x3F);
> +	if (inf->pack_code)
> +		spi_write(st->spi, st->data, inf->pack_code(st));

Check return value?

I'm also confused that this function now does two writes whereas it only
used to do one. 

This is a really confusing call as inf->pack_code() has side effects
that might not be obvious out here.  I'd have that callback explicitly
take the inputs and the output array so it's obvious what it can affect.
		count = inf->pack_code(st->chan, 2, st->chan_data);
or something along those lines.

>  
> -		st->data[0] = ch_b >> 4;
> -		st->data[1] = (ch_b << 4) | (ch_a >> 2);
> -		break;
> -	case ID_ADA4961:
> -		st->data[0] = ch_a & 0x1F;
> -		break;
> -	case ID_ADL5240:
> -		st->data[0] = (ch_a & 0x3F);
> -		break;
> -	case ID_HMC792:
> -	case ID_HMC1119:
> -		st->data[0] = ch_a;
> -		break;
> -	}
> -
> -	ret = spi_write(st->spi, st->data, indio_dev->num_channels);
> -	if (ret < 0)
> -		dev_err(&indio_dev->dev, "write failed (%d)", ret);
> -
> -	return ret;
> +	st->data[0] = st->ch[0];
> +	return spi_write(st->spi, st->data, 1);
>  }

>  static int ad8366_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -261,35 +229,20 @@ static int ad8366_probe(struct spi_device *spi)
>  		return dev_err_probe(dev, ret, "Failed to get regulator\n");
>  
>  	st->spi = spi;
> -	st->type = spi_get_device_id(spi)->driver_data;
> +	st->info = &ad8366_infos[spi_get_device_id(spi)->driver_data];
>  
> -	switch (st->type) {
> -	case ID_AD8366:
> -		indio_dev->channels = ad8366_channels;
> -		indio_dev->num_channels = ARRAY_SIZE(ad8366_channels);
> -		break;
> -	case ID_ADA4961:
> -	case ID_ADL5240:
> -	case ID_HMC792:
> -	case ID_HMC1119:
> -		rstc = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> -		if (IS_ERR(rstc))
> -			return dev_err_probe(dev, PTR_ERR(rstc),
> -					     "Failed to get reset controller\n");
> +	rstc = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);

I was surprised not to see this change in the patch description.

I guess it's fine because if the chip doesn't have a reset it looks the same
as one that isn't wired?  Add a comment to say not all devices have resets or
add a flag to the info structure and go back to calling it only for devices
that at least have a reset pin.



> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "Failed to get reset controller\n");
>  
> -		indio_dev->channels = ada4961_channels;
> -		indio_dev->num_channels = ARRAY_SIZE(ada4961_channels);
> -		break;
> -	default:
> -		return dev_err_probe(dev, -EINVAL, "Invalid device ID\n");
> -	}
> -
> -	st->info = &ad8366_infos[st->type];
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->info = &ad8366_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad8366_channels;
> +	indio_dev->num_channels = st->info->num_channels;
>  
> -	ret = ad8366_write(indio_dev, 0, 0);
> +	ret = ad8366_write_code(st);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "failed to write initial gain\n");
>  
>