Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `ma35d1_adc_update_scan_mode` incorrectly rejects valid scan masks that include the timestamp channel.
- [High] A timed-out conversion can leave a pending ISR that spuriously completes the next conversion, leading to stale data.
- [Low] The driver uses non-existent macros `IIO_DEV_ACQUIRE_DIRECT_MODE` and `IIO_DEV_ACQUIRE_FAILED`.
- [Low] The driver attempts to assign to a non-existent `format` field in `struct iio_scan_type`.
- [Low] The `MA35D1_EADC` Kconfig entry lacks an architecture or `COMPILE_TEST` dependency.
- [Low] Mixing `guard()` with `goto` for error handling violates `cleanup.h` rules.
--

--- Patch [2]: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver ---
commit e4312453e3f2955d13e6052b2ffd1d6a2a5739a1
Author: Chi-Wen Weng <cwweng@nuvoton.com>

iio: adc: Add Nuvoton MA35D1 EADC driver
    
This commit adds an IIO driver for the Nuvoton MA35D1 Enhanced ADC
controller. It supports direct raw reads and triggered buffered capture
using an external IIO trigger.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ea3ba13973928..c657b3627a11a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> @@ -844,6 +844,18 @@ config LTC2497
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2497.
>  
> +config MA35D1_EADC
> +	tristate "Nuvoton MA35D1 EADC driver"

[Severity: Low]
Is this missing a dependency on the architecture or COMPILE_TEST?
Without something like depends on ARCH_MA35 || COMPILE_TEST, this will
prompt users during kernel configuration on all architectures.

> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select REGMAP_MMIO
[ ... ]
> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 0000000000000..b2733e816c49d
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c
[ ... ]
> +static int ma35d1_adc_read_conversion(struct ma35d1_adc *adc,
> +				      const struct iio_chan_spec *chan, u16 *raw)
> +{
[ ... ]
> +	timeout = wait_for_completion_timeout(&adc->completion,
> +					      MA35D1_EADC_TIMEOUT);
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		return ret;
> +
> +	if (!timeout)
> +		return -ETIMEDOUT;

[Severity: High]
Does this timeout path need a call to synchronize_irq()?

If the hardware interrupt fires right as the timeout expires, the completion
could be signalled by the delayed ISR after we return -ETIMEDOUT. The next
conversion would call reinit_completion(), but the pending ISR might
immediately complete it, returning stale data or failing.

> +
> +	ret = regmap_read(adc->regmap, MA35D1_EADC_DAT(0), &val);
[ ... ]
> +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2, long mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	u16 raw;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;

[Severity: Low]
Will this compile? The IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED
macros do not appear to exist in the IIO subsystem. 

Should this use iio_device_claim_direct_mode() or 
iio_device_claim_direct_scoped() instead?

> +
> +		guard(mutex)(&adc->lock);
> +		if (adc->suspended)
[ ... ]
> +static int ma35d1_adc_update_scan_mode(struct iio_dev *indio_dev,
> +				       const unsigned long *scan_mask)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	const struct iio_chan_spec *scan_chan = NULL;
> +	unsigned long bit;
> +	unsigned int count;
> +
> +	count = 0;
> +	for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
> +		scan_chan = &indio_dev->channels[bit];
> +		count++;
> +	}
> +
> +	if (count != 1 || scan_chan->type != IIO_VOLTAGE)
> +		return -EINVAL;

[Severity: High]
Does this logic unintentionally reject valid scan masks that include the
timestamp channel?

If a userspace application enables one voltage channel and the timestamp
channel, the mask will have 2 bits set. The loop will increment count
for both, causing count != 1 to trigger and reject the configuration.

> +
> +	adc->scan_chan = scan_chan;
[ ... ]
> +static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	IIO_DECLARE_BUFFER_WITH_TS(u16, scan, 1) = { };
> +	u16 raw;
> +	int ret;
> +
> +	guard(mutex)(&adc->lock);
> +	if (adc->suspended || !adc->scan_chan)
> +		goto done;

[Severity: Low]
This isn't a runtime bug, but does this violate the cleanup.h guidelines?

The documentation for cleanup helpers states that we should never mix
goto-based error handling with guard() declarations in the same function.

> +
> +	ret = ma35d1_adc_read_conversion(adc, adc->scan_chan, &raw);
[ ... ]
> +static void ma35d1_adc_init_channel(struct iio_chan_spec *chan, u32 vinp,
> +				    u32 vinn, unsigned int scan_index,
> +				    bool differential)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->channel = vinp;
> +	chan->scan_index = scan_index;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					 BIT(IIO_CHAN_INFO_SCALE);
> +	chan->scan_type.format = differential ? IIO_SCAN_FORMAT_SIGNED_INT :
> +					      IIO_SCAN_FORMAT_UNSIGNED_INT;

[Severity: Low]
Does struct iio_scan_type have a format field? 

It looks like the IIO API uses a char sign field instead, and
IIO_SCAN_FORMAT_SIGNED_INT does not exist. This will likely cause a build
failure.

> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +	chan->scan_type.endianness = IIO_CPU;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713081127.115197-1-cwweng.linux@gmail.com?part=2