Re: [PATCH v3 4/8] iio: adc: Add support for ad4052

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 21 Jun 2025 11:13:58 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/21/25 11:08 AM, Jonathan Cameron wrote:
> > On Mon, 16 Jun 2025 09:54:52 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 6/14/25 5:08 AM, Jonathan Cameron wrote:  
> >>> On Tue, 10 Jun 2025 09:34:37 +0200
> >>> Jorge Marques <jorge.marques@analog.com> wrote:
> >>>     
> >>>> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, successive
> >>>> approximation register (SAR) analog-to-digital converter (ADC) that
> >>>> enables low-power, high-density data acquisition solutions without
> >>>> sacrificing precision. This ADC offers a unique balance of performance
> >>>> and power efficiency, plus innovative features for seamlessly switching
> >>>> between high-resolution and low-power modes tailored to the immediate
> >>>> needs of the system. The AD4052/AD4058/AD4050/AD4056 are ideal for
> >>>> battery-powered, compact data acquisition and edge sensing applications.
> >>>>    
> >>
> >> ...
> >>  
> >>>> +static int ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> >>>> +				   struct iio_chan_spec const *chan)
> >>>> +{
> >>>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>>> +	const struct iio_scan_type *scan_type;
> >>>> +	struct spi_transfer *xfer = &st->xfer;
> >>>> +
> >>>> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> >>>> +	if (IS_ERR(scan_type))
> >>>> +		return PTR_ERR(scan_type);
> >>>> +
> >>>> +	xfer->rx_buf = st->raw;
> >>>> +	xfer->bits_per_word = scan_type->realbits;
> >>>> +	xfer->len = scan_type->realbits == 24 ? 4 : 2;    
> >>>
> >>> This is a little odd. I'm not sure what happens with len not dividing
> >>> into a whole number of bits per word chunks.
> >>> Maybe a comment?    
> >>
> >> Even better, there is now spi_bpw_to_bytes() for this.
> >>  
> >>>     
> >>>> +	xfer->speed_hz = AD4052_SPI_MAX_ADC_XFER_SPEED(st->vio_uv);
> >>>> +
> >>>> +	return 0;
> >>>> +}    
> >>>
> >>>     
> >>
> >> ...
> >>  
> >>>     
> >>>> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> >>>> +{
> >>>> +	struct spi_device *spi = st->spi;
> >>>> +	struct spi_transfer t_cnv = {};
> >>>> +	int ret;
> >>>> +
> >>>> +	reinit_completion(&st->completion);
> >>>> +
> >>>> +	if (st->cnv_gp) {
> >>>> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> >>>> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> >>>> +	} else {
> >>>> +		ret = spi_sync_transfer(spi, &t_cnv, 1);    
> >>>
> >>> Add a comment for this.   I can't immediately spot documentation on what
> >>> a content free transfer actually does.  I assume pulses the chip select?
> >>> is that true for all SPI controllers?    
> >>
> >> Should be. Setting .delay in the xfer would also make it more
> >> clear that this is doing.
> >>  
> >>>     
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +	/*
> >>>> +	 * Single sample read should be used only for oversampling and
> >>>> +	 * sampling frequency pairs that take less than 1 sec.
> >>>> +	 */
> >>>> +	if (st->gp1_irq) {
> >>>> +		ret = wait_for_completion_timeout(&st->completion,
> >>>> +						  msecs_to_jiffies(1000));
> >>>> +		if (!ret)
> >>>> +			return -ETIMEDOUT;
> >>>> +	}
> >>>> +
> >>>> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	if (st->xfer.len == 2)
> >>>> +		*val = sign_extend32(*(u16 *)(st->raw), 15);
> >>>> +	else
> >>>> +		*val = sign_extend32(*(u32 *)(st->raw), 23);
> >>>> +
> >>>> +	return ret;
> >>>> +}    
> >>>     
> >>
> >> ...
> >>  
> >>>> +
> >>>> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> >>>> +				     unsigned int writeval, unsigned int *readval)
> >>>> +{
> >>>> +	struct ad4052_state *st = iio_priv(indio_dev);
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!iio_device_claim_direct(indio_dev))    
> >>>
> >>> For these guards in the debugfs callback, please add a comment on why they
> >>> are needed.   We've had a lot of questions about these recently and I'd
> >>> like it to be clear to people when they should cut and paste these and when
> >>> not.    
> >>
> >> The reason I started doing this is that running the iio_info command attemps
> >> to read register 0x00 via the debug attribute of every single iio device. So
> >> if you run iio_info during a buffered read, and 0x00 is a valid register, it
> >> would break things without this check.
> >>
> >> Ideally, general purpose commands wouldn't be poking debug registers, but
> >> that isn't the case. But I suppose we could "fix" iio_info instead.
> >>  
> > 
> > Please do fix iio_info.  It absolutely should not be poking the debug interfaces
> > except on specific debug calls.  The user has to know they may be shooting themselves
> > in the foot.
> > 
> > I'm not sure why a read of that register would break buffered capture though.
> > Is it a volatile register or is there a sequencing problem with multiple
> > accesses in this driver?  If it is multiple accesses then that should be
> > prevented via a local lock, not whether we are in buffer mode or not.  
> 
> IIRC, this was particularly a problem on chips that have a separate data
> capture mode and reading a register exits data capture mode.

Those ones I'm fine with just having a comment that hopefully means it
doesn't get cut and paste somewhere inappropriate!

Jonathan

> 
> > 
> > So I'm fine with this defense where it is necessary for all register
> > accesses, but I would like to see comments on why it is necessary.
> > 
> > Jonathan
> >   
> >>>     
> >>>> +		return -EBUSY;
> >>>> +
> >>>> +	if (readval)
> >>>> +		ret = regmap_read(st->regmap, reg, readval);
> >>>> +	else
> >>>> +		ret = regmap_write(st->regmap, reg, writeval);
> >>>> +	iio_device_release_direct(indio_dev);
> >>>> +	return ret;
> >>>> +}    
> >>>     
> >   
>