Re: [PATCH v4 2/3] iio: adc: ad7191: add AD7191

[David Lechner <dlechner@baylibre.com>]

On 2/3/25 7:31 AM, Alisa-Dariana Roman wrote:
> AD7191 is a pin-programmable, ultra-low noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

...

> +struct ad7191_state {
> +	struct ad_sigma_delta		sd;
> +	struct mutex			lock; /* Protect device state */
> +
> +	struct gpio_descs		*odr_gpios;
> +	struct gpio_descs		*pga_gpios;
> +	struct gpio_desc		*temp_gpio;
> +	struct gpio_desc		*chan_gpio;
> +
> +	u16				int_vref_mv;
> +	u32				scale_avail_gpio[4][2];
> +	u32				scale_avail_pinstrap[1][2];
> +	const u32			(*scale_avail)[2];

This feels a bit reduant to have two arrays and then a pointer to one of those
arrays. We could just have a single static const array of 4 and use that in both
cases. (also see further comments later)

> +	size_t				scale_avail_size;
> +	u32				scale_index;
> +	u32				samp_freq_avail_gpio[4];
> +	u32				samp_freq_avail_pinstrap[1];
> +	const u32			*samp_freq_avail;

ditto

> +	size_t				samp_freq_avail_size;
> +	u32				samp_freq_index;
> +
> +	struct clk			*mclk;
> +};
> +
> +static int ad7191_set_channel(struct ad_sigma_delta *sd, unsigned int address)
> +{
> +	struct ad7191_state *st = ad_sigma_delta_to_ad7191(sd);
> +	u8 temp_gpio_val, chan_gpio_val;
> +
> +	if (!FIELD_FIT(AD7191_CHAN_MASK | AD7191_TEMP_MASK, address))
> +		return -EINVAL;
> +
> +	chan_gpio_val = FIELD_GET(AD7191_CHAN_MASK, address);
> +	temp_gpio_val = FIELD_GET(AD7191_TEMP_MASK, address);
> +
> +	gpiod_set_value(st->chan_gpio, chan_gpio_val);
> +	gpiod_set_value(st->temp_gpio, temp_gpio_val);
> +
> +	return 0;
> +}
> +
...

> +
> +static int ad7191_config_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +	/* Sampling frequencies in Hz, see Table 5 */
> +	const int samp_freq[4] = { 120, 60, 50, 10 };

As per my earlier suggestion, we can make this static const...

> +	/* Gain options, see Table 7 */
> +	const int gain[4] = { 1, 8, 64, 128 };

ditto

> +	int odr_value, odr_index, pga_value, pga_index, i, ret;
> +	u64 scale_uv;
> +
> +	st->samp_freq_index = 0;
> +	st->scale_index = 0;
> +
> +	ret = device_property_read_u32(dev, "adi,odr-value", &odr_value);

Shoud also check if (ret && ret != -EINVAL) first to catch other errors like
someone put a string in the .dts instead of a u32.

> +	if (ret == -EINVAL) {
> +		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->odr_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> +					     "Failed to get odr gpios.\n");
> +
> +		for (i = 0; i < ARRAY_SIZE(samp_freq); i++)
> +			st->samp_freq_avail_gpio[i] = samp_freq[i];
> +
> +		st->samp_freq_avail = st->samp_freq_avail_gpio;
> +		st->samp_freq_avail_size = ARRAY_SIZE(st->samp_freq_avail_gpio);

...then here instead of copying...

		st->samp_freq_avail = samp_freq;
		st->samp_freq_avail_size = ARRAY_SIZE(samp_freq);

> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(samp_freq); i++) {
> +			if (odr_value != samp_freq[i])
> +				continue;
> +			odr_index = i;

missing break;?

Also, should we error if match not found? Otherwise we could have uninitalized
odr_index;

> +		}
> +
> +		st->samp_freq_avail_pinstrap[0] = samp_freq[odr_index];
> +
> +		st->samp_freq_avail = st->samp_freq_avail_pinstrap;
> +		st->samp_freq_avail_size = ARRAY_SIZE(st->samp_freq_avail_pinstrap);
> +

and here...

		st->samp_freq_avail = &samp_freq[odr_index];
		st->samp_freq_avail_size = 1;

> +		st->odr_gpios = NULL;
> +	}
> +
> +	ret = device_property_read_u32(dev, "adi,pga-value", &pga_value);

ditto about error checking

> +	if (ret == -EINVAL) {
> +		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->pga_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> +					     "Failed to get pga gpios.\n");
> +
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail_gpio); i++) {
> +			scale_uv = ((u64)st->int_vref_mv * NANO) >>
> +				(indio_dev->channels[0].scan_type.realbits - 1);
> +			do_div(scale_uv, gain[i]);
> +			st->scale_avail_gpio[i][1] = do_div(scale_uv, NANO);
> +			st->scale_avail_gpio[i][0] = scale_uv;
> +		}
> +
> +		st->scale_avail = st->scale_avail_gpio;
> +		st->scale_avail_size = ARRAY_SIZE(st->scale_avail_gpio);
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(gain); i++) {
> +			if (pga_value != gain[i])
> +				continue;
> +			pga_index = i;
> +		}
> +
> +		scale_uv = ((u64)st->int_vref_mv * NANO) >>
> +			(indio_dev->channels[0].scan_type.realbits - 1);
> +		do_div(scale_uv, gain[pga_index]);
> +		st->scale_avail_pinstrap[0][1] = do_div(scale_uv, NANO);
> +		st->scale_avail_pinstrap[0][0] = scale_uv;
> +
> +		st->scale_avail = st->scale_avail_pinstrap;
> +		st->scale_avail_size = ARRAY_SIZE(st->scale_avail_pinstrap);
> +
> +		st->pga_gpios = NULL;
> +	}

and ditto about st->scale_avail and pinstrap matching for loop

> +
> +	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->temp_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->temp_gpio),
> +				     "Failed to get temp gpio.\n");
> +
> +	st->chan_gpio = devm_gpiod_get(dev, "chan", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->chan_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->chan_gpio),
> +				     "Failed to get chan gpio.\n");
> +
> +	return 0;
> +}
> +

...

> +
> +static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
> +{
> +	unsigned long value = gain_index;
> +
> +	st->scale_index = gain_index;
> +
> +	return gpiod_set_array_value_cansleep(st->pga_gpios->ndescs,
> +					      st->pga_gpios->desc,
> +					      st->pga_gpios->info, &value);
> +}

Depending on timing, we might be able to take advantage of [1].

But it isn't merged yet and needs another revision and you are very fast, so
don't wait on it. ;-)

[1]: https://lore.kernel.org/linux-iio/20250131-gpio-set-array-helper-v1-0-991c8ccb4d6e@baylibre.com/


> +
> +static const struct iio_chan_spec ad7191_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.address = AD7191_CH_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),

Should this one be info_mask_separate?

Since this is a multiplexed ADC and not simelutaneous sampling, I would expect
that if the ORD pins are set to 10Hz (0.1s period), then a buffered read with
all channels enabled would take 0.3s to do the 3 samples (effective sample rate
of 3.33Hz), but if only one channel was enabled in the buffer, then it only
takes 0.1s to do all of the samples (effective sample rate is 10Hz).

The iio convention is to use info_mask_separate for the sampling_frequency
attribute to indicate that the rate only applies to each individual channel
and not the combined rate to do one "set" of samples for all enabled channels.

A sampling_frequency attribute that was shared_by_all would mean that on each
period equivlent to this rate, all samples are read no matter how many channels
were enabled for a buffered read.

> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 32,
> +			.endianness = IIO_BE,
> +		},
> +	},