On Wed, 29 Jan 2025 16:29:03 +0200
Alisa-Dariana Roman <alisadariana@xxxxxxxxx> 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@xxxxxxxxxx>
Hi Alisa-Dariana
Seeing as a v4 is needed for the available thing you mention
in the cover letter, a few minor things inline I might otherwise
have just tidied up whilst applying.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> new file mode 100644
> index 000000000000..b6c1d5c25783
> --- /dev/null
> +++ b/drivers/iio/adc/ad7191.c
> +
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +#include <linux/iio/iio.h>
> +
> +#define ad_sigma_delta_to_ad7191(sigmad) container_of((sigmad), struct ad7191_state, sd)
#define ad_sigma_delta_to_ad7191(sigmad) \
container_of((sigmad), struct ad7191_state, sd)
Given it is very long otherwise.
> +
> +#define AD7191_TEMP_CODES_PER_DEGREE 2815
> +
> +#define AD7191_EXT_CLK_ENABLE 0
> +#define AD7191_INT_CLK_ENABLE 1
> +
> +#define AD7191_CHAN_MASK BIT(0)
> +#define AD7191_TEMP_MASK BIT(1)
> +
> +enum ad7191_channel {
> + AD7191_CH_AIN1_AIN2,
> + AD7191_CH_AIN3_AIN4,
> + AD7191_CH_TEMP
Add a trailing comma. Maybe there will be more channels on some
compatible part that we add after this.
> +};
> +
> +/*
> + * NOTE:
> + * The AD7191 features a dual-use data out ready DOUT/RDY output.
> + * In order to avoid contentions on the SPI bus, it's therefore necessary
> + * to use SPI bus locking.
> + *
> + * The DOUT/RDY output must also be wired to an interrupt-capable GPIO.
> + *
> + * The SPI controller's chip select must be connected to the PDOWN pin
> + * of the ADC. When CS (PDOWN) is high, it powers down the device and
> + * resets the internal circuitry.
> + */
> +
> +struct ad7191_state {
> + struct ad_sigma_delta sd;
> + struct mutex lock; // to protect sensor state
Only use old style /* */ in IIO code with exception of the SPDX
corner case. This is just a consistency thing hanging over
from when that was only thing used in the kernel.
> +
> + 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 pga_index;
> + u32 scale_avail[4][2];
> + u32 odr_index;
> + u32 samp_freq_avail[4];
> +
> + struct clk *mclk;
> +};
> +
> +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};
Space after { and before } in all such places.
This is just my local IIO preference as nice to pick a standard.
I often just tweak this whilst applying but seeing as you
are going to be doing a v4 anyway please tidy it up!
> + /* Gain options, see Table 7 */
> + const int gain[4] = {1, 8, 64, 128};
> + int odr_value, pga_value, i, ret;
> + u64 scale_uv;
...
> +
> +static int ad7191_setup(struct iio_dev *indio_dev, struct device *dev)
> +{
> + struct ad7191_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad7191_init_regulators(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7191_config_setup(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7191_clock_setup(st);
> + if (ret)
> + return ret;
> +
> + return 0;
return ad7191_clock_setup(st);
and save a few lines of code.
> +}
> +
> +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: {
> + guard(mutex)(&st->lock);
> + *val = st->scale_avail[st->pga_index][0];
> + *val2 = st->scale_avail[st->pga_index][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_index];
> + return IIO_VAL_INT;
> + }
> +
Can't get here so drop this. I guess the bot hasn't tested this one yet
or we should have seen unreachable warnings.
> + return -EINVAL;
> +}
> +
> +static int ad7191_set_gain(struct ad7191_state *st, int gain_index)
> +{
> + unsigned long value = gain_index;
> +
> + if (!st->pga_gpios)
> + return -EPERM;
> +
> + st->pga_index = gain_index;
> +
> + return gpiod_set_array_value_cansleep(st->pga_gpios->ndescs,
> + st->pga_gpios->desc,
> + st->pga_gpios->info, &value);
> +
> + return 0;
double return. Drop this bonus one!
> +}
> +
