Re: [PATCH] iio: ad7380: add support for SPI offload

[David Lechner <dlechner@baylibre.com>]

On 2/22/25 11:31 AM, David Lechner wrote:
> On 2/20/25 12:03 PM, Angelo Dureghello wrote:
>> From: Angelo Dureghello <adureghello@baylibre.com>
>>
>> Add support for SPI offload to the ad7380 driver. SPI offload allows
>> sampling data at the max sample rate (2MSPS with one SDO line).
>>
>> This is developed and tested against the ADI example FPGA design for
>> this family of ADCs [1].
>>
>> [1]: http://analogdevicesinc.github.io/hdl/projects/ad738x_fmc/index.html
>>
>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>> ---
> 

...

>> +#define _AD7380_OFFLOAD_CHANNEL(index, bits, diff, sign, gain) {		\
>> +	.type = IIO_VOLTAGE,							\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                          \
>> +		((gain) ? BIT(IIO_CHAN_INFO_SCALE) : 0) |			\
>> +		((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)),			\
>> +	.info_mask_shared_by_type = ((gain) ? 0 : BIT(IIO_CHAN_INFO_SCALE)) |   \
>> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |				\
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),					\
> 
> Not sure if this is worth troubling with, but it might make more sense to make
> IIO_CHAN_INFO_SAMP_FREQ info_mask_separate instead of info_mask_shared_by_type,
> at least in the case of the single-ended chips.
> 
> This family of chips does simultaneous conversions so shared_by_type (or shared_by_all)
> would typically be the right thing to do here. However, the single-ended versions
> of these chips also have a multiplexer, so there are 2 banks of simultaneously
> sampled inputs. So the effective sample rate as far as IIO is concerned would
> actually be 1/2 of the sampling_frequency attribute value.
> 
> Since we have a channel mask restriction where we force all channels in a bank
> to be enabled at once, I think it would work to make IIO_CHAN_INFO_SAMP_FREQ
> info_mask_separate where the reported sampling frequency is the conversion rate
> divided by the number of channels in a bank.
> 

Hi Jonathan,

You might have missed this since v2 was sent before you had a chance to review
v1. I am testing the chip with the mux now, so I was curious if you had any
wisdom to add here. The way we implemented it feels a little odd to me with
sampling_frequency as info_mask_shared_by_type instead of info_mask_separate
or info_mask_shared_by_all like on most other chips I've worked with so far.

I found a bug in this series that I need to send a fix for, so if we decide
there is a better way here, now would be a good time to do it.

>> +	.info_mask_shared_by_type_available =					\
>> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |				\
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),					\
>> +	.indexed = 1,                                                           \
>> +	.differential = (diff),                                                 \
>> +	.channel = (diff) ? (2 * (index)) : (index),                            \
>> +	.channel2 = (diff) ? (2 * (index) + 1) : 0,                             \
>> +	.scan_index = (index),                                                  \
>> +	.has_ext_scan_type = 1,                                                 \
>> +	.ext_scan_type = ad7380_scan_type_##bits##_##sign##_offload,            \
>> +	.num_ext_scan_type =                                                    \
>> +		ARRAY_SIZE(ad7380_scan_type_##bits##_##sign##_offload),		\
>> +	.event_spec = ad7380_events,                                            \
>> +	.num_event_specs = ARRAY_SIZE(ad7380_events),                           \
>> +}
>> +