Re: [PATCH v2 4/6] iio: amplifiers: ad8366: add device tree support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Jan 26, 2026 at 01:51:05PM +0000, Rodrigo Alencar via B4 Relay wrote:

> Add device-tree support by dropping 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;
> 
> With this, switch cases on the device type were dropped:
> - probe() function adjusted accordingly;
> - Simplified read_raw() and write_raw() callbacks;

> - mutex_lock()/mutex_unlock() replaced for guard(mutex)() to allow
>   moving to early returns;

Shouldn't this be in a separate change? I dunno. Let Jonathan to decide.

...

> +static size_t ad8366_pack_code(struct ad8366_state *st)
> +{
> +	u8 ch_a = bitrev8(st->ch[0] & 0x3F);
> +	u8 ch_b = bitrev8(st->ch[1] & 0x3F);

GENMASK() in both cases? But I don't see why ch_a needs this at all,
isn't the 2 LSBs are not used anyway?

Also missed header inclusion for this? And also perhaps sorting headers first
to see what's there and what needs to be updated (ideally another patch to move
to IWYU principle).

> +	st->data[0] = ch_b >> 4;
> +	st->data[1] = (ch_b << 4) | (ch_a >> 2);
> +	return 2;
> +}

...

>  static int ad8366_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val,
>  			   int *val2,
> -			   long m)
> +			   long mask)

Seems like unrelated change.

...

> -	/* Values in dB */

Do you think this comment is useless? To me looks like a stray change.

>  	if (val < 0)
>  		gain = (val * 1000) - (val2 / 1000);

...

> +	st->info = spi_get_device_match_data(spi);

> +	if (!st->info)
> +		return dev_err_probe(dev, -EINVAL, "Invalid device info\n");

Only useful for the developer, dead code in production.

-- 
With Best Regards,
Andy Shevchenko