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

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Wed, 22 Jan 2025 15:20:40 +0200
Alisa-Dariana Roman <alisadariana@xxxxxxxxx> wrote:

> AD7191 is a pin-programmable, ultralow 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@xxxxxxxxxx>
Hi Alisa-Dariana,

A few additional comments inline. Hopefully not too much overlap
with other reviews

Jonathan

> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..dd8151ad3f3f
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> @@ -0,0 +1,570 @@


> +static int set_cs(struct ad_sigma_delta *sigma_delta, int pull_down)
> +{
> +	struct spi_transfer t = {
> +		.len = 0,
> +		.cs_change = pull_down,
> +	};
> +	struct spi_message m;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);

spi_message_init_with_transfers() given there is no spi_sync_transfers_locked()
and probably not enough users to make it worth adding.


> +
> +	return spi_sync_locked(sigma_delta->spi, &m);
> +}
>

> +static int ad7191_gpio_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +
> +	if (device_property_read_u32(dev, "adi,odr-state", &st->odr_state) == 0) {
> +		if (st->odr_state > AD7191_MAX_ODR_STATE)
> +			return dev_err_probe(dev, -EINVAL, "Invalid ODR state.\n");
> +
> +		dev_info(dev, "ODR is pin-strapped to %d\n", st->odr_state);

dev_dbg for all these. 

> +		st->odr_gpios = NULL;
> +	} else {
> +		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");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,pga-state", &st->pga_state) == 0) {
> +		if (st->odr_state > AD7191_MAX_PGA_STATE)
> +			return dev_err_probe(dev, -EINVAL, "Invalid PGA state.\n");
> +
> +		dev_info(dev, "PGA is pin-strapped to %d\n", st->pga_state);
> +		st->pga_gpios = NULL;
> +	} else {
> +		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");
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,clksel-state", &st->clksel_state) == 0) {
> +		dev_info(dev, "CLKSEL is pin-strapped to %d\n", st->clksel_state);
> +		st->clksel_gpio = NULL;
> +	} else {
> +		st->clksel_gpio = devm_gpiod_get(dev, "clksel", GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->clksel_gpio))
> +			return dev_err_probe(dev, PTR_ERR(st->clksel_gpio),
> +					     "Failed to get clksel gpio.\n");
> +	}
> +
> +	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_clock_setup(struct ad7191_state *st)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	u8 clksel_value;
> +
> +	st->mclk = devm_clk_get_enabled(dev, "mclk");
> +	if (IS_ERR(st->mclk)) {
> +		if (PTR_ERR(st->mclk) != -ENOENT)
> +			return dev_err_probe(dev, PTR_ERR(st->mclk),
> +					     "Failed to get mclk.\n");
> +
> +		/*
> +		 * No external clock found, default to internal clock.
Single line comment if anything.  Kind of obvious from code anyway
so could drop the comment..

> +		 */
> +		clksel_value = AD7191_INT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_INT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the internal clock, CLKSEL must be high.\n");
> +
> +		dev_info(dev, "Using internal clock.\n");

dev_dbg() for this stuff.  Driver shouldn't be that noisy about details like this

> +	} else {
> +		clksel_value = AD7191_EXT_CLK_ENABLE;
> +		if (!st->clksel_gpio && st->clksel_state != AD7191_EXT_CLK_ENABLE)
> +			return dev_err_probe(dev, -EINVAL,
> +				"Invalid CLKSEL state. To use the external clock, CLKSEL must be low.\n");
> +
> +		dev_info(dev, "Using external clock.\n");
> +	}
> +
> +	if (st->clksel_gpio)
> +		gpiod_set_value(st->clksel_gpio, clksel_value);
> +
> +	return 0;
> +}
> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +	u64 scale_uv;
> +	const int gain[4] = {1, 8, 64, 128};
> +	int i, ret;
> +
> +	ret = ad7191_init_regulators(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_gpio_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_clock_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Sampling frequencies in Hz, available in the documentation, Table 5.

I'd just go with
	 /* Sampling frequencies in Hz, see Table 5 */

Unlikely it would be anywhere else and 'in the documentation' isn't
very specific.

> +	 */
> +	st->samp_freq_avail[0] = 120;
> +	st->samp_freq_avail[1] = 60;
> +	st->samp_freq_avail[2] = 50;
> +	st->samp_freq_avail[3] = 10;
> +
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); 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[i][1] = do_div(scale_uv, NANO);
> +		st->scale_avail[i][0] = scale_uv;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7191_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long m)
> +{
> +	struct ad7191_state *st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:

As below.  Make the scope explicit with {}

> +			guard(mutex)(&st->lock);
> +			*val = st->scale_avail[st->pga_state][0];
> +			*val2 = st->scale_avail[st->pga_state][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_TEMP:
> +			*val = 0;
> +			*val2 = NANO / AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val -= 273 * AD7191_TEMP_CODES_PER_DEGREE;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->samp_freq_avail[st->odr_state];
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}

> +
> +static int __ad7191_write_raw(struct ad7191_state *st,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
scope should be defined and obvious if you want to use guard.

	case IIO_CHAN_INFO_SCALE: {
		guard(mutex)(&st->lock);
...

		return -EINVAL;
	}
> +		guard(mutex)(&st->lock);
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +			if (val2 != st->scale_avail[i][1])
> +				continue;
> +			return ad7191_set_gain(st, i);
> +		}
> +		return -EINVAL;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!val)
> +			return -EINVAL;
> +
> +		guard(mutex)(&st->lock);

Same here.

> +		for (i = 0; i < ARRAY_SIZE(st->samp_freq_avail); i++) {
> +			if (val != st->samp_freq_avail[i])
> +				continue;
> +			return ad7191_set_samp_freq(st, i);
> +		}
> +		return -EINVAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int ad7191_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ad7191_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!spi->irq) {
> +		dev_err(dev, "no IRQ?\n");

Can't the ad_sigma_delta library figure this out for all devices?
Feels odd to check it here when this code never directly uses it.


> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad7191";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7191_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7191_channels);
> +	indio_dev->info = &ad7191_info;
> +
> +	ad_sd_init(&st->sd, indio_dev, spi, &ad7191_sigma_delta_info);
> +
> +	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad7191_setup(indio_dev, dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}