Re: [PATCH v2 2/4] iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs

[Marcelo Schmitt]

On 05/29, Jonathan Cameron wrote:
> On Thu, 28 May 2026 12:03:52 -0300
> Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:
> 
> > Support for LTC2378-20 and similar analog-to-digital converters.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> 
...
> >  
> > +config LTC2378
> > +	tristate "Analog Devices LTC2378 ADC driver"
> > +	depends on SPI
> > +	depends on GPIOLIB || PWM
> 
> Triggered sashiko:
> https://sashiko.dev/#/patchset/cover.1779976379.git.marcelo.schmitt%40analog.com
> This dependency is odd enough that I think I'd add a comment on why.
> Also bring in the dependency on PWM in the patch that needs it not this one.
> 
> Will it build without gpiolib? If so I'd prefer that to be a || COMPILE_TEST
> just to get a little more coverage.
> 
The intent was to allow using the ADC with offload PWM triggered setup
even if the GPIO is not provided/enabled. We would be able to do the buffered
reads, but the single-shot read would be debatable since we would very likely
trigger many conversions in between a pwm_enable()/pwm_disable(). In the doubt,
I left the single-read procedure untouched specially because there's also the
case where the GPIO is there and so the single-read would work normally.

     +-------------+         +-------------+
     |         CNV |<-----+--| GPIO        |
     |             |      +--| PWM0        |
     |             |         |             |
     |             |      +--| PWM1        |
     |             |      |  +-------------+
     |             |      +->| TRIGGER     |
     |             |         |             |
     |     ADC     |         |    SPI      |
     |             |         | controller  |
     |             |         |             |
     |         SDI |<--------| SDO         |
     |         SDO |-------->| SDI         |
     |        SCLK |<--------| SCLK        |
     +-------------+         +-------------+

Maybe I'm going too much flexible trying to allow all those combinations?
Simplest way to avoid what sashiko points as garbage read is to go straight
'depends on GPIOLIB' and check the GPIO must always be there.

> 
...
> > +static int ltc2378_convert_and_acquire(struct ltc2378_state *st)
> > +{
> > +	int ret;
> > +
> > +	/* Cause a rising edge of CNV to initiate a new ADC conversion */
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> 
> Should we check for errors on setting the gpio?
> 
> > +	fsleep(4);
> > +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ltc2378_channel_single_read(const struct iio_chan_spec *chan,
> > +				       struct ltc2378_state *st, int *val)
> > +{
> > +	const struct iio_scan_type *scan_type = &chan->scan_type;
> > +	u32 sample;
> > +	int ret;
> > +
> > +	ret = ltc2378_convert_and_acquire(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (scan_type->realbits > 16)
> > +		sample = st->scan.data.sample_buf32;
> 
> Sashikio has an interesting theory here that you need a shift. I can't immediately
> spot why it is wrong.  Given we are clocking in 32bits I assume the first
> res_bits are valid then we get a bunch of garbage after that. Given it's MSB first
> the SPI controller should put those in the top bits of the resulting u32 with the
> garbage at the bottom. So shouldn't we shift them down by 32 - resolution or something
> like that? + in buffered case provide the relevant shift value (in later patches)
> 
The only way I'm seing to get sample_buf32 with ADC data in the MSB is if the
CPU endianness is BE. Otherwise (LE), the data bits would be put into the lower
addresses when SPI puts the word in CPU endianness. But, if we do unconditional
shift, won't that break sign_extend32() for LE CPUs? Well, I'll anyway double
check the data for all sample_buf16/sample_buf32 buffer/non-buffer use cases.

> > +	else
> > +		sample = st->scan.data.sample_buf16;
> > +
> > +	if (scan_type->format == IIO_SCAN_FORMAT_SIGNED_INT)
> > +		*val = sign_extend32(sample, scan_type->realbits - 1);
> > +	else
> > +		*val = sample;
> > +
> > +	return 0;
> > +}
> > +
...

> case IIO_CHAN_INFO_RAW: {
> 
> > +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> > +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> > +			return -EBUSY;
> > +
> > +		ret = ltc2378_channel_single_read(chan, st, val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +
> }
> 
> As that IIO_DEV_ACQUIRE_DIRECT_MODE() is declaring a local variable
> so there is something go out of scope and result in cleanup.
> LLVM moans about this, GCC just does the wrong thing which can in theory
> at least result in an underflow.
> You'll get build bot warnings on this - bug sashiko caught it.
> 

Oops I guess I started mis-squashing commits after a certain number of rebases XD
Will fix this and all other suggestions.

Thanks