Message-Id: <D2ZIG2NK223D.J9VK1MWOICE3@baylibre.com>
Date: Fri, 26 Jul 2024 15:38:24 +0200
From: "Esteban Blanc" <eblanc@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>,
 <baylibre-upstreaming@...ups.io>, "Lars-Peter Clausen" <lars@...afoo.de>,
 "Michael Hennerich" <Michael.Hennerich@...log.com>, "Jonathan Cameron"
 <jic23@...nel.org>, "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski"
 <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Nuno Sa"
 <nuno.sa@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "David Lechner" <dlechner@...libre.com>
Subject: Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

Hi Nuno,

> > +			struct {
> > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];
>
> Not sure common makes sense as it comes aggregated with the sample. Maybe this
> could as simple as:
>
> struct {
> 	s32 val;
> 	u64 timestamp __aligned(8);
> } rx_data ...

See below my answer on channels order and storagebits.

> So, from the datasheet, figure 39 we have something like a multiplexer where we
> can have:
>
> - averaged data;
> - normal differential;
> - test pattern (btw, useful to have it in debugfs - but can come later);
> - 8 common mode bits;
>
> While the average, normal and test pattern are really mutual exclusive, the
> common mode voltage is different in the way that it's appended to differential
> sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
> for us to see them as different channels from a SW perspective (even more since
> gain and offset only apply to the differential data). But there are a couple of
> things I don't like (have concerns):
>
> * You're pushing the CM channels into the end. So when we a 2 channel device
> we'll have:
>
>  in_voltage0 - diff
>  in_voltage1 - diff
>  in_voltage2 - CM associated with chan0
>  in_voltage0 - CM associated with chan1
>
> I think we could make it so the CM channel comes right after the channel where
> it's data belongs too. So for example, odd channels would be CM channels (and
> labels could also make sense).

I must agree with you it would make more sense.

> Other thing that came to mind is if we could somehow use differential = true
> here. Having something like:
>
> in_voltage1_in_voltage0_raw - diff data
> ...
> And the only thing for CM would be:
>
> in_voltage1_raw
> in_voltage1_scale
>
> (not sure if the above is doable with ext_info - maybe only with device_attrs)
>
> The downside of the above is that we don't have a way to separate the scan
> elements. Meaning that we don't have a way to specify the scan_type for both the
> common mode and differential voltage. That said, I wonder if it is that useful
> to buffer the common mode stuff? Alternatively, we could just have the scan_type
> for the diff data and apps really wanting the CM voltage could still access the
> raw data. Not pretty, I know...

At the moment the way I "separate" them is by looking at the
`active_scan_mask`. If the user asked for differential channel only, I put the
chip in differential only mode. If all the channels are asked, I put
the chip in differential + common mode. This way there is no need to
separate anything in differential mode. See below for an example where
this started.

> However, even if we go with the two separate channels there's one thing that
> concerns me. Right now we have diff data with 32 for storage bits and CM data
> with 8 storage bits which means the sample will be 40 bits and in reality we
> just have 32. Sure, now we have SW buffering so we can make it work but the
> ultimate goal is to support HW buffering where we won't be able to touch the
> sample and thus we can't lie about the sample size. Could you run any test with
> this approach on a HW buffer setup? 

Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
- Ch0 diff with 24 bits of realbits and 24 bits of storagebits
- Ch0 cm with 8 bits of realbits and 8 bits of storagebits
- Ch1 diff with 24 bits of realbits and 24 bits of storagebits
- Ch1 cm with 8 bits of realbits and 8 bits of storagebits
ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
by the chip.

The problem I faced when trying to do this in this series is that IIO doesn't
seem to like 24 storagebits and the data would get garbled. In diff only
mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
the data is also garbled. I used iio-oscilloscope software to test this setup.
Here is the output with iio_readdev:
```
# iio_readdev -s 1 ad4630-24 voltage0
WARNING: High-speed mode not enabled
Unable to refill buffer: Invalid argument (22)
```

I think this is happening when computing the padding to align ch1 diff.
In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
and ch1 diff (AD4630-24 in differential mode). The output is 5. As
specified in linux/align.h:
> @a is a power of 2
In our case `a` is `length`, and 3 is not a power of 2.

It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
bits shift.

Intrestingly, a similar setup works great on AD4630-16:
- Ch0 diff with 16 bits of realbits and 16 bits of storagebits
- Ch0 cm with 8 bits of realbits and 8 bits of storagebits
- Ch1 diff with 16 bits of realbits and 16 bits of storagebits
- Ch1 cm with 8 bits of realbits and 8 bits of storagebits

In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
4, everything is fine. The output of iio-oscilloscope is as expected,
a clean sinewave and iio_readdev does not throw an error.

All this to say that at the moment, I'm not sure how I will be able to
put the CM byte in a separate channel for AD4630-24 without buffering it.
It would be useful to return a "packed" buffer.

> > +static int ad4030_conversion(struct ad4030_state *st, const struct
> > iio_chan_spec *chan)
> > +{
> > +	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits)
> > +
> > +			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM
> > ||
> > +			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ?
> > 1 : 0)) *
> > +			     st->chip->num_channels;
> > +	struct spi_transfer xfer = {
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = bytes_to_read,
> > +	};
> > +	unsigned char byte_index;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > +	ndelay(AD4030_TCNVH_NS);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
> > +
> > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > +	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> > +		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))
>
> You should guarantee that st->mode is not invalid...

I don't see a code path where this could be the case. It's initialized
at 0 in the probe (a valid value) and always set before `ad4030_conversion`
in `ad4030_set_mode` called by `ad4030_buffer_preenable`. If the value
is invalid, this is a driver error, not an invalid input from the user.
Should I put an assert then?

> > +		return ret;
> > +
> > +	byte_index = st->chip->precision_bits == 16 ? 3 : 4;
> > +	for (i = 0; i < st->chip->num_channels; i++)
>
> So even for a single channel conversion we are going through all?

Yes. For ad4030-24 (this patch), that ok since num_channels = 1.
For AD4630, we are getting the 2 channels samples anyway. Current
allowed `scan_mask` are ch0 and ch1 (diff mode) or ch0 to ch3 (diff +
common mode). The only case I could optimized currently is if I allowed
scan_mask such as: ch0 & ch2, ch0 & ch1 & ch2, ch0 & ch1 & ch3. I'm not
sure it makes sense to allow such `scan_mask` since the HW does not
support it.
What do you think?

> > +static int ad4030_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long info)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>
> Oh this is not neat :(. I guess there's still no work to make conditional guards
> to look more as the typical pattern...

Yeah... At first I put checks inside each function. Then I put guards
inside each function. But at the end, almost every arm of the switch
case needs to be in direct mode so... I can put it back in the function,
as you wish.

> > +		switch (info) {
> > +		case IIO_CHAN_INFO_RAW:
> > +			return ad4030_single_conversion(indio_dev, chan,
> > val);
> > +
> > +		case IIO_CHAN_INFO_SCALE:
> > +			*val = (st->vref_uv * 2) / MILLI;
> > +			*val2 = st->chip->precision_bits;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
>
> I don't think this applies to CM?

Indded. Scale is not available for the CM channels.

> > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > +{
> > +	unsigned int grade;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > +	if (ret)
> > +		return ret;
> > +
> > +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > +	if (grade != st->chip->grade)
> > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > +				"Unknown grade(%u) for %s\n", grade,
> > +				st->chip->name);
>
> I think in here we still want to proceed and just print a warning...

I've put this in because it's a quick and easy way to check:
- If the SPI communication is working,
- If the correct chip is installed. If it's not the correct one, we can
  be sure that the parameters used to get any data out the chip will be
  wrong and the data completely useless.

If you feel that there is a use case I missed I will put a warning
instead, no problem.


All the comments I skipped and not replied to will be fixed in a V2.

Best regards,

-- 
Esteban "Skallwar" Blanc
BayLibre

Powered by blists - more mailing lists