RE: [PATCH v10 3/6] iio: adc: ad4691: add triggered buffer support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Tuesday, May 12, 2026 6:45 PM
> To: Radu Sabau via B4 Relay <devnull+radu.sabau.

...

> > The CNV Burst Mode sampling frequency (PWM period) is exposed as a
> > buffer-level attribute via IIO_DEVICE_ATTR.
> >
> > Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> 
> Sashiko pointed out you have a buffer that is big endian but
> chan_spec doesn't reflect that.  That should have generated obvious
> garbage output (unless you are actually testing on a be machine!)
> 

I thought I had IIO_BE in chan_spec,, my bad. I either forgot to copy the last
kernel image upon testing (though I don't know why I would remove that line)
or I did something wrong upon rebasing on the patches.

> Various other things came up, some of which I thought were in previous
> reviews - but maybe I'm confusing drivers.
> 
> Thanks
> 
> Jonathan
> 
> > @@ -204,7 +230,14 @@ static const struct ad4691_chip_info
> ad4694_chip_info = {
> >  struct ad4691_state {
> >  	const struct ad4691_chip_info *info;
> >  	struct regmap *regmap;
> > +	struct spi_device *spi;

...

> > +	/*
> > +	 * Append a 4-byte state-reset transfer [addr_hi, addr_lo,
> > +	 * STATE_RESET_ALL, OSC_EN=1]. CS is asserted throughout, so
> > +	 * ADDR_DESCENDING writes byte[3]=1 to OSC_EN_REG (0x180) as a
> > +	 * deliberate side-write, keeping the oscillator enabled.
> > +	 */
> > +	put_unaligned_be16(AD4691_STATE_RESET_REG, st->scan_tx_reset);
> > +	st->scan_tx_reset[2] = AD4691_STATE_RESET_ALL;
> > +	st->scan_tx_reset[3] = 1;
> > +	st->scan_xfers[2 * k].tx_buf = st->scan_tx_reset;
> > +	st->scan_xfers[2 * k].len = sizeof(st->scan_tx_reset);
> > +	st->scan_xfers[2 * k].cs_change = 1;
> 
> Our old friend - cs_change = 1 is very rarely the right thing to do on a
> final message.  I thought this came up in an earlier version.
> 

I thought I had this removed, it must have been lost.

> > +	spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> > +
> > +	ret = spi_optimize_message(st->spi, &st->scan_msg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> > +			   bitmap_read(indio_dev->active_scan_mask, 0,
> > +				       iio_get_masklength(indio_dev)));

...

> > +static irqreturn_t ad4691_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct ad4691_state *st = iio_priv(indio_dev);
> > +
> > +	ad4691_read_scan(indio_dev, pf->timestamp);
> > +	if (!st->manual_mode)
> > +		enable_irq(st->irq);
> 
> Maybe it was a different driver but I thought I commented on this before.
> There are a bunch of races if you reenable this here - needs to be
> in the trigger reenable callback.
> (Sashiko is pointing this out as well with more detail on what those
> races are)  The short story is that you can race and have a trigger between
> the enable and the notify_done which will be dropped on the floor meaning
> we never get in here again - IIRC there is (rather convoluted) code to handle
> that
> corner case in via the reenable callback and a work item.
> 

I also thought I had this covered, it appears it is not...

> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const struct iio_info ad4691_info = {
> >  	.read_raw = &ad4691_read_raw,
> >  	.write_raw = &ad4691_write_raw,
> >  	.read_avail = &ad4691_read_avail,
> >  	.debugfs_reg_access = &ad4691_reg_access,
> > +	.validate_trigger = iio_validate_own_trigger,
> 

...

> >  static int ad4691_probe(struct spi_device *spi)
> >  {
> >  	struct device *dev = &spi->dev;
> > @@ -663,6 +1200,7 @@ static int ad4691_probe(struct spi_device *spi)
> >  		return -ENOMEM;
> >
> >  	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> >  	st->info = spi_get_device_match_data(spi);
> >  	if (!st->info)
> >  		return -ENODEV;
> > @@ -692,8 +1230,9 @@ static int ad4691_probe(struct spi_device *spi)
> >  	indio_dev->info = &ad4691_info;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >
> > -	indio_dev->channels = st->info->sw_info->channels;
> > -	indio_dev->num_channels = st->info->sw_info->num_channels;
> 
> You've lost me here. Where are these now set?
> 

At this point I am starting to think that whatever I have sent is different from
whatever I tested. It seems like changes I have running on the kernel image
do not appear here. I am pretty sure I have done something wrong while
rebasing, I am very sorry for that. I'll try and send a better/cleaner version
next time.

> > +	ret = ad4691_setup_triggered_buffer(indio_dev, st);
> > +	if (ret)
> > +		return ret;
> >
> >  	return devm_iio_device_register(dev, indio_dev);
> >  }
> >