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); > > }