Re: [PATCH v4 03/10] iio: adc: add the ti-ads1262 driver

["Kurt Borja" <kuurtb@gmail.com>]

On Sat Aug 29, 2026 at 11:02 PM -03, Jonathan Cameron wrote:
>> commit 6093db1ff45e9e897720bc6b278550ffbef67e85
>> Author: Kurt Borja <kuurtb@gmail.com>
>> 
>> iio: adc: add the ti-ads1262 driver
>> 
>> Add the ti-ads1262 driver with initial support for the primary ADC (ADC1).
>> Various features such as accurate timeout delays, per-channel reference
>> sources, scale, offset, and diagnostics are intentionally left out for
>> future support.
>
> One particularly interesting point from sashiko about regmap debugfs.
>
>> 
>> > diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
>> > new file mode 100644
>> > --- /dev/null
>> > +++ b/drivers/iio/adc/ti-ads1262.c
>> 
>
>> > +static int ads1262_channel_read(struct iio_dev *indio_dev,
>> > +				const struct iio_chan_spec *spec, __be32 *val)
>> > +{
>> > +	struct ads1262 *st = iio_priv(indio_dev);
>> > +	int ret;
>> > +
>> > +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>> > +	if (IIO_DEV_ACQUIRE_FAILED(claim))
>> > +		return -EBUSY;
>> > +
>> > +	ret = ads1262_set_runmode(st, ADS1262_RUNMODE_PULSE);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	ret = ads1262_channel_enable(st, spec);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	reinit_completion(&st->drdy);
>> > +
>> > +	ret = ads1262_dev_start_one(st);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	ret = ads1262_wait_for_conversion(st);
>> > +	if (ret)
>> > +		return ret;
>> 
>> [Severity: Medium]
>> Does this error path leave the hardware running if the wait is interrupted?
>> 
>> If ads1262_wait_for_conversion() returns an error (like -ERESTARTSYS from
>> a signal), the driver exits immediately without stopping the hardware. If a
>> subsequent read calls reinit_completion(&st->drdy), but the abandoned
>> hardware conversion finishes and asserts the DRDY interrupt right after,
>> could the new read prematurely fetch stale data from the old conversion?
>> 
>> Should this path ensure the hardware is stopped and synchronized with the
>> interrupt handler on timeout or interrupt?
>
> For me this is obscure enough I'm not sure we care about stale data.

Interesting issue... Hmmm, yeah I say we leave it like that too. Stale
data would not be the end of the world here.

>> > +static int ads1262_regmap_read(void *context, const void *reg_buf,
>> > +			       size_t reg_size, void *val_buf, size_t val_size)
>> > +{
>> > +	struct ads1262 *st = context;
>> > +	u8 tx[2];
>> > +
>> > +	lockdep_assert_held(&st->xfer_lock);
>> 
>> [Severity: Medium]
>> Will this lockdep assertion fail if the register is read via debugfs?
>> 
>> Because ads1262_regmap_config does not set disable_locking = true and does
>> not provide custom lock/unlock callbacks, regmap uses its own internal
>> mutex. When regmap's debugfs interface is read, the regmap core acquires
>> its internal mutex and calls the driver's ads1262_regmap_read() callback
>> without holding st->xfer_lock.
>> 
>> Could this bypass the xfer_lock, triggering the lockdep splat and allowing
>> debugfs SPI operations to interleave with driver-initiated SPI transactions?
>
> This is a corner I'd not thought about in adding lockdep markings to these
> functions.  Up to you how you want to fix it.

The way to fix this would be to either bring the xfer_lock back into the
regmap callbacks or allocate the regmap_config dynamically and give it
the xfer_lock so the core can deal with it.

For me I'd prefer to do the first but David raised a concern about the
xfer_lock being in the callback. Maybe David can give us his opinion
here?

-- 
Thanks,
 ~ Kurt