RE: [PATCH v3 4/4] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Saturday, March 14, 2026 9:37 PM

...

> > +
> > +	if (args[0] != AD4691_TRIGGER_EVENT_BUSY &&
> > +	    args[0] != AD4691_TRIGGER_EVENT_DATA_READY)
> 
> What is the difference between BUSY and DATA_READY?
> 

Perhaps BUSY won't be used at all, since indeed this question got me
thinking of its necessity.

> > +		return false;
> > +

...

> > +			xfer[num_xfers].cs_change = 1;
> > +			xfer[num_xfers].cs_change_delay.value = 1000;
> 
> This needs an explantion of where the number comes from. I would expect
> 430 ns
> based on max value of t_CONV from the datasheet.
> 

You are right about this. I was playing with this value at testing and
Forgot to change it back, my bad...

> > +			xfer[num_xfers].cs_change_delay.unit =

...

> > +	} else {
> > +		/*
> > +		 * CNV_CLOCK_MODE: single transfer per channel (2-byte cmd
> +
> > +		 * 2-byte data = 4 bytes, one 32-bit SPI Engine DMA word).
> > +		 * AVG_IN registers are used; RX layout: [cmd_hi, cmd_lo,
> d_hi, d_lo]
> 
> These comments are confusing. What it actually appears we are doing is
> doing a 16-bit write and then a 16-bit read. I assume we are doing it
> using 32-bit words for efficiny so that we only have 1/2 of the number
> of xfers required to do it separately.
> 
> TX layout: [cmd_hi, cmd_lo, ignore, ignore]
> RX layout: [ignore, ignore, data_hi, data_lo]
> 

I will make sure to update the comment as you are saying here, since
TX and RX should be explained separately. Having them both as
'RX layout' is a mistake.

> 
> > +		 */
> > +		for (i = 0; i < n_active; i++) {
> > +			unsigned int reg;
> > +			int ch = active_chans[i];
> > +
> > +			reg = AD4691_AVG_IN(ch);
> > +
> 	put_unaligned_be32(FIELD_PREP(AD4691_MSG_ADDR_HI, (reg >> 8)
> | 0x80) |
> 
> Mixing FIELD_PREP() and bit ops looks wrong.

Will stick to bit ops in this case, then.