| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250125143942.3df83c3d@jic23-huawei>
Date: Sat, 25 Jan 2025 14:39:42 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: Alisa-Dariana Roman <alisa.roman@...log.com>, Jonathan Cameron
<Jonathan.Cameron@...wei.com>, David Lechner <dlechner@...libre.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, Lars-Peter
Clausen <lars@...afoo.de>, Michael Hennerich
<Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof
Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Linus
Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v2 2/2] iio: adc: ad7191: add AD7191
On Wed, 22 Jan 2025 15:20:40 +0200
Alisa-Dariana Roman <alisadariana@...il.com> 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@...log.com>
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);
> +}
Powered by blists - more mailing lists