Re: [PATCH v7 05/17] iio: adc: Add AD7768 and AD7768-4 core support
From: sashiko-bot@kernel.org
Date: 2026-09-10 17:56:48
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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