On Fri, Aug 23, 2024 at 10:19 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > On Mon, 19 Aug 2024 09:47:17 +0300 > Alexandru Ardelean <aardelean@xxxxxxxxxxxx> wrote: > > > The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B. > > The main difference between AD7606C-16 & AD7606C-18 is the precision in > > bits (16 vs 18). > > Because of that, some scales need to be defined for the 18-bit variants, as > > they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants). > > > > Because the AD7606C-16,18 also supports bipolar & differential channels, > > for SW-mode, the default range of 10 V or ±10V should be set at probe. > > On reset, the default range (in the registers) is set to value 0x3 which > > corresponds to '±10 V single-ended range', regardless of bipolar or > > differential configuration. > > > > Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B. > > > > And the AD7606C-18 variant offers 18-bit precision. The unfortunate effect > > of this 18-bit sample size, is that there is no simple/neat way to get the > > samples into a 32-bit array without having to do a home-brewed bit-buffer. > > The ADC must read all samples (from all 8 channels) in order to get the > > N-th sample (this could be reworked to do up-to-N-th sample for scan-direct). > > There doesn't seem to be any quick-trick to be usable to pad the samples > > up to at least 24 bits. > > Even the optional status-header is 8-bits, which would mean 26-bits of data > > per sample. > > That means that when using a simple SPI controller (which can usually read > > 8 bit multiples) a simple bit-buffer trick is required. > > > > Datasheet links: > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf > > > > Signed-off-by: Alexandru Ardelean <aardelean@xxxxxxxxxxxx> > > A few minor things. If we can just start with 18 bit word spi controllers only > maybe that's worth doing to make things simpler. Will go for 18 bit word SPI controllers-only for now. > > > +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev, > > + ad7606c_chan_setup_cb_t chan_setup_cb) > > +{ > > + unsigned int num_channels = indio_dev->num_channels - 1; > > + struct ad7606_state *st = iio_priv(indio_dev); > > + bool chan_configured[AD760X_MAX_CHANNELS]; > = {}; > and drop the memset. ack > > > + struct device *dev = st->dev; > > + int ret; > > + u32 ch; > > + > > + /* We need to hook this first */ > > + ret = st->bops->sw_mode_config(indio_dev); > > + if (ret) > > + return ret; > > + > > + indio_dev->info = &ad7606c_info_sw_mode; > > + > > + memset(chan_configured, 0, sizeof(chan_configured)); > > + > > + device_for_each_child_node_scoped(dev, child) { > > + bool bipolar, differential; > > + > > + ret = fwnode_property_read_u32(child, "reg", &ch); > > + if (ret) > > + continue; > > + > > + if (ch >= num_channels) { > > + dev_warn(st->dev, > > + "Invalid channel number (ignoring): %d\n", ch); > > + continue; > > + } > > + > > + bipolar = fwnode_property_present(child, "bipolar"); > > + differential = fwnode_property_present(child, "diff-channel"); > > + > > + chan_setup_cb(st, ch, bipolar, differential); > > + chan_configured[ch] = true; > > + } > > + > > + /* Apply default configuration to unconfigured (via DT) channels */ > > + for (ch = 0; ch < num_channels; ch++) { > > + struct ad7606_chan_scale *cs; > > + unsigned int *scale_avail_show; > > + int i; > > + > > + if (!chan_configured[ch]) > > + chan_setup_cb(st, ch, false, false); > > + > > + /* AD7606C supports different scales per channel */ > > + cs = &st->chan_scales[ch]; > > + > > + scale_avail_show = devm_kcalloc(st->dev, cs->num_scales * 2, > > + sizeof(*scale_avail_show), > > + GFP_KERNEL); > > Maybe just make it big enough for worst case and stick it in st always? > How big can it get? So, that would be 16 channels x 8 bytes-per-scale x 5 = 640 bytes. Not too bad. > > > > > + if (!scale_avail_show) > > + return -ENOMEM; > > + > > + /* Generate a scale_avail list for showing to userspace */ > > + for (i = 0; i < cs->num_scales; i++) { > > + scale_avail_show[i * 2] = 0; > > + scale_avail_show[i * 2 + 1] = cs->scale_avail[i]; > > + } > > + > > + cs->scale_avail_show = scale_avail_show; > > + } > > + > > + return 0; > > +} > > > > > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c > > index dd0075c97c24..73a7b0007bf8 100644 > > --- a/drivers/iio/adc/ad7606_spi.c > > +++ b/drivers/iio/adc/ad7606_spi.c > > @@ -45,6 +45,8 @@ > > > > > +static int ad7606_spi_read_block18to32(struct device *dev, > > + int count, void *buf) > > +{ > > + struct spi_device *spi = to_spi_device(dev); > > + u32 i, bit_buffer, buf_size, bit_buf_size; > > + u32 *data = buf; > > + u8 *bdata = buf; > > + int j, ret; > > + > > + /** > Not kernel doc. /* > > + * With the 18 bit ADC variants (here) is that we can't assume that all > > + * SPI controllers will pad 18-bit sequences into 32-bit arrays, > > + * so we need to do a bit of buffer magic here. > > + * Alternatively, we can have a variant of this function that works > > + * for SPI controllers that can pad 18-bit samples into 32-bit arrays. > > + */ > > + > > + /* Write 'count' bytes to the right, to not overwrite samples */ > > + bdata += count; > > + > > + /* Read 24 bits only, as we'll only get samples of 18 bits each */ > > + buf_size = count * 3; > > + ret = spi_read(spi, bdata, buf_size); > > + if (ret < 0) { > > + dev_err(&spi->dev, "SPI read error\n"); > > + return ret; > > + } > > + > > + bit_buffer = 0; > > + bit_buf_size = 0; > > + for (j = 0, i = 0; i < buf_size; i++) { > > + u32 sample; > > + > > + bit_buffer = (bit_buffer << 8) | bdata[i]; > > + bit_buf_size += 8; > > + > > + if (bit_buf_size < 18) > > + continue; > > + > > + bit_buf_size -= 18; > > + sample = (bit_buffer >> bit_buf_size) & AD7606C_18_SAMPLE_MASK; > > + data[j++] = sign_extend32(sample, 17); > > + > > + if (j == count) > > + break; > > + } > > + > > + return 0; > > +} >
