On Fri, 14 Mar 2025 15:03:43 -0500 David Lechner <dlechner@xxxxxxxxxxxx> wrote: > On 2/22/25 11:31 AM, David Lechner wrote: > > On 2/20/25 12:03 PM, Angelo Dureghello wrote: > >> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxxx> > >> --- > > > > ... > > >> +#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 saw the message and decided not to comment as I'd not really had time to think about it. There is a messy corner here with the forced enabling that might bite us in future that I've not really thought about before. It would give a non-intuitive result. If we had that situation for a non SPI offload case (i.e. going through the iio_push_to_buffers* path (not sure if we do) then the driver might enable multiple channels when only one is turned on by userspace and where userspace then mysteriously sees half the frequency of what it expects. In that case the only scheme that I think works is to do shared_by_all and have it update depending on underlying enabled channels. Not sure that applies here or not though. It is definitely always valid to do a single attribute sampling_frequency, if that reflects how often we get a full scan. It may not provide as much information or fine grained enough control in some cases though. We have support for more complex sequencers that leave gaps where this is the only option. I.e. they take all N channels in a burst then sleep for P usecs before doing it again. I've wondered a few times if we should describe the sampling points in a scan in some fashion. Would anything use that info? It wouldn't help for control though as the constraints would be too hard to describe for anything other than read only report of what is configured. There are really fun devices with say 2 simultaneous sampling ADCs that can measure any of X channels (including in theory the same one). When a complex sequencer is present or we have software based sequencing, then that gets really hard to describe. Jonathan > > 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), \ > >> +} > >> + >
