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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 May 2026 09:37:00 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 05/29, Jonathan Cameron wrote:
> > On Thu, 28 May 2026 12:03:52 -0300
> > Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> >   
> > > Support for LTC2378-20 and similar analog-to-digital converters.
> > > 
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>  
> >   
> ...
> > >  
> > > +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.

Yes. I'd do that.  Almost no one will build without that anyway.

Does the code remove the single read if the gpio isn't available?
Sounds like it probably should.

> 
> >   
> ...
> > > +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.

We aren't talking addreses here we are talking about handling of a 4 byte s32
(which is also what the sign extend is operating on).

> 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.

It's operating on an s32 so say we extend 0xfabc from bit 15

be is 

[0x00] [0x00] [0xfa] [0xbc] =>  [0xff] [0xff] [0xfa] [0xbc]

le

[0xbc] [0xfa] [0x00] [0x00] =>  [0xbc] [0xfa] [0xff] [0xff]

Which are both 0xfffffabc as an s32 (in hex obviously)

Arguement in this case is the value is actually in the top few
bytes so the ones that are 0 in the example above.


> 
> > > +	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

Happens to us all.  Always do a last sanity check with an LLVM build
(it finds way more than GCC currently!) one patch at a time.

> Will fix this and all other suggestions.
> 
> Thanks