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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ypaiae3flszokbvrk773pzcqxx53j6amjnhbkwq3oeopnmlyv5@2b3wfzqlpqtd>
Date: Tue, 25 Feb 2025 10:48:25 +0100
From: Angelo Dureghello <adureghello@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Nuno Sá <nuno.sa@...log.com>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: ad7380: add support for SPI offload

Hi,

On 22.02.2025 11:31, David Lechner wrote:
> On 2/20/25 12:03 PM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@...libre.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@...libre.com>
> > ---
> 
> We forgot to also update Documentation/iio/ad7380.rst. We can follow up
> with a separate patch later though.
> 
> >  drivers/iio/adc/Kconfig  |   2 +
> >  drivers/iio/adc/ad7380.c | 509 +++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 475 insertions(+), 36 deletions(-)
> > 
> 
> ...
> 
> >  #define _AD7380_CHANNEL(index, bits, diff, sign, gain) {			\
> >  	.type = IIO_VOLTAGE,							\
> >  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> > @@ -237,48 +335,123 @@ static const struct iio_scan_type ad7380_scan_type_16_u[] = {
> >  	.num_event_specs = ARRAY_SIZE(ad7380_events),				\
> >  }
> >  
> > +#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.
>

so if i understand properly,

for ad7386/7/8 i should use info_mask_separate, so that if a single bank
(called st->ch in the driver) is enabled, user can control that specific
bank sample rate. But user can also set the device in sequencer mode, so in this
case the sample rate is unique. So we could find 2 different sample rates in use
and the management of this seems probably unusual.

Could have sense to stay as we are now ?
 
> > +	.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),                           \
> > +}
> > +