Re: [PATCH 4/5] ad7380: enable sequencer for single-ended parts

[Julien Stephan]

Le dim. 28 juil. 2024 à 18:36, Jonathan Cameron <jic23@xxxxxxxxxx> a écrit :
>
> On Fri, 26 Jul 2024 17:20:09 +0200
> Julien Stephan <jstephan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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);
> >  }