Re: [PATCH v7 05/17] iio: adc: Add AD7768 and AD7768-4 core support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The AD7768_REG_CH_STANDBY register is incorrectly programmed using logical channel indices instead of physical channel bitmasks for the AD7768-4 variant.
--

commit 09736b063e77e92595bbf9636bddc775b728127b
Author: Janani Sunil <janani.sunil@analog.com>

iio: adc: Add AD7768 and AD7768-4 core support

This commit adds core support for the AD7768 and AD7768-4 simultaneous
sampling ADCs. It provides SPI protocol configuration, channels and input
buffer parsing, IIO backend connection, and runtime PM support.

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> new file mode 100644
> index 0000000000000..3cb58c2554a31
> --- /dev/null
> +++ b/drivers/iio/adc/ad7768.c
[ ... ]
> +static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
> +{
> +	return GENMASK(st->chip_info->num_channels - 1, 0);
> +}

[Severity: High]
Does this logic in ad7768_all_standby_mask() in drivers/iio/adc/ad7768.c
compute the correct standby mask for the AD7768-4? The GENMASK macro returns
a logical mask (0x0F for 4 channels), but shouldn't it return the physical
mask (0x33, mapping to physical channels 0, 1, 4, 5)? Returning the logical
mask here might cause issues with standby register updates for AD7768-4
logical channels 2 and 3.

[ ... ]
> +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned long channel_mask;
> +	unsigned long standby_mask;
> +	int ret;
> +
> +	channel_mask = ad7768_all_standby_mask(st);
> +	standby_mask = channel_mask & ~*scan_mask;
> +
> +	/*
> +	 * Crystal excitation requires channel 4 on AD7768 or channel 2 on
> +	 * AD7768-4 to remain active.
> +	 */
> +	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
> +		__clear_bit(st->chip_info->num_channels / 2, &standby_mask);
> +
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +				 channel_mask, standby_mask);

[Severity: High]
Could this update in ad7768_update_scan_mode() be writing to the wrong
hardware bits? Because it uses the logical channel mask, it appears to write
to reserved bits for the AD7768-4. 

Additionally, clearing st->chip_info->num_channels / 2 clears logical bit 2
for the AD7768-4, but shouldn't it clear physical bit 4 to keep the XTAL clock
channel active? Failing to keep the correct physical channel active could stop
the master clock and break the device when using the XTAL clock source.

[ ... ]
> +static int ad7768_parse_config(struct iio_dev *indio_dev,
> +			       struct device *dev)
> +{
[ ... ]
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		u32 channel;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &channel);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to parse reg of %pfwP\n",
> +					     child);
> +
> +		if (channel >= st->chip_info->num_channels)
> +			return dev_err_probe(dev, -ECHRNG,
> +					     "Invalid channel %u in firmware\n",
> +					     channel);
> +
> +		ret = regmap_clear_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +					BIT(channel));

[Severity: High]
Does this initialization in ad7768_parse_config() use the raw logical channel
index instead of the physical mapping? By using BIT(channel) here, it looks
like it writes to reserved bits instead of the actual physical channels for
AD7768-4 channels 2 and 3, which could leave those channels permanently in
standby and lead to data loss.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-ad7768-driver-v7-0-7f70b07627e1@analog.com?part=5