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

Jonathan Cameron
2026-02-14T18:56:46Z

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");
>
>