| 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
| ||
|
Message-ID: <CAEHHSvbDSet2pFqc6TPCioq7ShVwXQfhxYSkgDDy3a+55X0AJg@mail.gmail.com>
Date: Tue, 30 Jul 2024 09:34:13 +0200
From: Julien Stephan <jstephan@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
David Lechner <dlechner@...libre.com>, Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 4/5] ad7380: enable sequencer for single-ended parts
Le dim. 28 juil. 2024 à 18:36, Jonathan Cameron <jic23@...nel.org> a écrit :
>
> On Fri, 26 Jul 2024 17:20:09 +0200
> Julien Stephan <jstephan@...libre.com> wrote:
>
> > ad7386/7/8(-4) single-ended parts have a 2:1 mux in front of each ADC.
> >
> > From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> > export 4 channels and ad7386-4/7-4/8-4 export 8 channels. First inputs
> > of muxes correspond to the first half of IIO channels (i.e 0-1 or 0-3)
> > and second inputs correspond to second half (i.e 2-3 or 4-7)
> >
> > Currently, the driver supports only sampling first half OR second half of
> > the IIO channels. To enable sampling all channels simultaneously, these
> > parts have an internal sequencer that automatically cycle through the
> > mux entries.
> >
> > When enabled, the maximum throughput is divided by two. Moreover, the ADCs
> > need additional settling time, so we add an extra CS toggle to correctly
> > propagate setting, and an additional spi transfer to read the second
> > half.
> >
> > Signed-off-by: Julien Stephan <jstephan@...libre.com>
> Hi Julien,
>
> All looks good. Main comment is a suggestion that we add a core
> interface to get the index of the active_scan_mask if it is built
> from available_scan_masks. That will avoid the mask matching code
> in here.
>
> Implementation for now would be a simple bit of pointer
> arithmetic after checking available_scan_masks is set.
>
> Jonathan
>
> > ---
> > drivers/iio/adc/ad7380.c | 164 ++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 121 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > index 25d42fff1839..11ed010431cf 100644
> > --- a/drivers/iio/adc/ad7380.c
> > +++ b/drivers/iio/adc/ad7380.c
> > @@ -33,7 +33,7 @@
>
> > @@ -290,16 +291,22 @@ static const unsigned long ad7380_4_channel_scan_masks[] = {
> > *
> > * Since this is simultaneous sampling for AinX0 OR AinX1 we have two separate
> > * scan masks.
> > + * When sequencer mode is enabled, chip automatically cycle through
>
> cycles
>
> > + * AinX0 and AinX1 channels. From an IIO point of view, we ca enable all
> > + * channels, at the cost of an extra read, thus dividing the maximum rate by
> > + * two.
> > */
>
> ...
>
> > * DMA (thus cache coherency maintenance) requires the transfer buffers
> > * to live in their own cache lines.
> > @@ -609,33 +619,47 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
> > static void ad7380_update_xfers(struct ad7380_state *st,
> > const struct iio_scan_type *scan_type)
> > {
> > - /*
> > - * First xfer only triggers conversion and has to be long enough for
> > - * all conversions to complete, which can be multiple conversion in the
> > - * case of oversampling. Technically T_CONVERT_X_NS is lower for some
> > - * chips, but we use the maximum value for simplicity for now.
> > - */
> > - if (st->oversampling_ratio > 1)
> > - st->xfer[0].delay.value = T_CONVERT_0_NS + T_CONVERT_X_NS *
> > - (st->oversampling_ratio - 1);
> > - else
> > - st->xfer[0].delay.value = T_CONVERT_NS;
> > -
> > - st->xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > + struct spi_transfer *xfer = st->seq ? st->seq_xfer : st->normal_xfer;
> > + unsigned int t_convert = T_CONVERT_NS;
> >
> > /*
> > - * Second xfer reads all channels. Data size depends on if resolution
> > - * boost is enabled or not.
> > + * In the case of oversampling, conversion time is higher than in normal
> > + * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
> > + * the maximum value for simplicity for now.
> > */
> > - st->xfer[1].bits_per_word = scan_type->realbits;
> > - st->xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> > - st->chip_info->num_simult_channels;
> > + if (st->oversampling_ratio > 1)
> > + t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
> > + (st->oversampling_ratio - 1);
> > +
> > + if (st->seq) {
> > + xfer[0].delay.value = xfer[1].delay.value = t_convert;
> > + xfer[0].delay.unit = xfer[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> > + xfer[2].bits_per_word = xfer[3].bits_per_word =
> > + scan_type->realbits;
> > + xfer[2].len = xfer[3].len =
> > + BITS_TO_BYTES(scan_type->storagebits) *
> > + st->chip_info->num_simult_channels;
> > + xfer[3].rx_buf = xfer[2].rx_buf + xfer[2].len;
> > + /* Additional delay required here when oversampling is enabled */
> > + if (st->oversampling_ratio > 1)
> > + xfer[2].delay.value = t_convert;
> > + else
> > + xfer[2].delay.value = 0;
> > + xfer[2].delay.unit = SPI_DELAY_UNIT_NSECS;
> > + } else {
> > + xfer[0].delay.value = t_convert;
> > + xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > + xfer[1].bits_per_word = scan_type->realbits;
> > + xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> > + st->chip_info->num_simult_channels;
> > + }
> > }
> >
> > static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
> > {
> > struct ad7380_state *st = iio_priv(indio_dev);
> > const struct iio_scan_type *scan_type;
> > + struct spi_message *msg = &st->normal_msg;
> >
> > /*
> > * Currently, we always read all channels at the same time. The scan_type
> > @@ -646,34 +670,62 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
> > return PTR_ERR(scan_type);
> >
> > if (st->chip_info->has_mux) {
> > - unsigned int num_simult_channels = st->chip_info->num_simult_channels;
> > + unsigned int num_simult_channels =
> > + st->chip_info->num_simult_channels;
>
> Unrelated change. Push this back to the earlier patch (or leave it alone - whether
> it matters for readability is debatable anyway, so I think this is fine either way).
>
> > unsigned long active_scan_mask = *indio_dev->active_scan_mask;
> > unsigned int ch = 0;
> > int ret;
> >
> > /*
> > * Depending on the requested scan_mask and current state,
> > - * we need to change CH bit to sample correct data.
> > + * we need to either change CH bit, or enable sequencer mode
> > + * to sample correct data.
> > + * Sequencer mode is enabled if active mask corresponds to all
> > + * IIO channels enabled. Otherwise, CH bit is set.
> > */
> > - if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> > - num_simult_channels))
> > - ch = 1;
> > + if (active_scan_mask == GENMASK(2 * num_simult_channels - 1, 0)) {
>
> Whilst it's an implementation detail that you can (IIRC) just compare the active_scan_mask
> address with that of your available_scan_masks array entries, maybe it's worth providing
> an interface that gets the index of that array?
>
> int iio_active_scan_mask_index(struct iio_dev *)
> that returns an error if available_scan_masks isn't set.
Hi Jonathan,
I'll send a v2 of this series in a couple of days, with all comments
fixed and I'll try to implement an iio_active_scan_mask_index
function.
Cheers
Julien
>
> We know the active_scan_mask will always be selected from the available ones
> so this interface should be fine even if we change how they are handled internally
> in the future.
>
> That would then make all these matches simpler.
>
> > + ret = regmap_update_bits(st->regmap,
> > + AD7380_REG_ADDR_CONFIG1,
> > + AD7380_CONFIG1_SEQ,
> > + FIELD_PREP(AD7380_CONFIG1_SEQ, 1));
> > + msg = &st->seq_msg;
> > + st->seq = true;
> > + } else {
> > + if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> > + num_simult_channels))
> > + ch = 1;
> > +
> > + ret = ad7380_set_ch(st, ch);
> > + }
> >
> > - ret = ad7380_set_ch(st, ch);
> > if (ret)
> > return ret;
>
> I'd just duplicate this if (ret) check as the two calls are very different so to
> me this doesn't make logical sense (even if it works).
>
> > }
> >
> > ad7380_update_xfers(st, scan_type);
> >
> > - return spi_optimize_message(st->spi, &st->msg);
> > + return spi_optimize_message(st->spi, msg);
> > }
Powered by blists - more mailing lists