| 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: <20250131181437.00000097@huawei.com>
Date: Fri, 31 Jan 2025 18:14:37 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Esteban Blanc <eblanc@...libre.com>
CC: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
<Michael.Hennerich@...log.com>, Nuno Sá
<nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
<linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v3 2/6] iio: adc: ad4030: add driver for ad4030-24
On Thu, 30 Jan 2025 12:08:26 +0100
Esteban Blanc <eblanc@...libre.com> wrote:
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
>
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
>
> Signed-off-by: Esteban Blanc <eblanc@...libre.com>
Hi Esteban,
Just one thing in here that actually matters. Question about scaling of
the common channel. The others I could tidy up whilst applying if
nothing much else comes up.
Jonathan
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e06424b7f2590d28a57943949b070cd7e185fbb7
> --- /dev/null
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#define AD4030_REG_INTERFACE_CONFIG_A 0x00
> +#define AD4030_REG_INTERFACE_CONFIG_A_SW_RESET (BIT(0) | BIT(7))
> +#define AD4030_REG_INTERFACE_CONFIG_B 0x01
> +#define AD4030_REG_DEVICE_CONFIG 0x02
> +#define AD4030_REG_CHIP_TYPE 0x03
> +#define AD4030_REG_PRODUCT_ID_L 0x04
> +#define AD4030_REG_PRODUCT_ID_H 0x05
> +#define AD4030_REG_CHIP_GRADE 0x06
> +#define AD4030_REG_CHIP_GRADE_AD4030_24_GRADE 0x10
> +#define AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE GENMASK(7, 3)
> +#define AD4030_REG_SCRATCH_PAD 0x0A
> +#define AD4030_REG_SPI_REVISION 0x0B
> +#define AD4030_REG_VENDOR_L 0x0C
> +#define AD4030_REG_VENDOR_H 0x0D
> +#define AD4030_REG_STREAM_MODE 0x0E
> +#define AD4030_REG_INTERFACE_CONFIG_C 0x10
> +#define AD4030_REG_INTERFACE_STATUS_A 0x11
> +#define AD4030_REG_EXIT_CFG_MODE 0x14
> +#define AD4030_REG_EXIT_CFG_MODE_EXIT_MSK BIT(0)
> +#define AD4030_REG_AVG 0x15
> +#define AD4030_REG_AVG_MASK_AVG_SYNC BIT(7)
> +#define AD4030_REG_AVG_MASK_AVG_VAL GENMASK(4, 0)
> +#define AD4030_REG_OFFSET_X0_0 0x16
> +#define AD4030_REG_OFFSET_X0_1 0x17
> +#define AD4030_REG_OFFSET_X0_2 0x18
> +#define AD4030_REG_OFFSET_X1_0 0x19
> +#define AD4030_REG_OFFSET_X1_1 0x1A
> +#define AD4030_REG_OFFSET_X1_2 0x1B
> +#define AD4030_REG_OFFSET_BYTES_NB 3
> +#define AD4030_REG_OFFSET_CHAN(ch) (AD4030_REG_OFFSET_X0_2 + \
> + (AD4030_REG_OFFSET_BYTES_NB * \
> + (ch)))
Similar to below. I'd put the implementation on a new linle.
> +#define AD4030_REG_GAIN_X0_LSB 0x1C
> +#define AD4030_REG_GAIN_X0_MSB 0x1D
> +#define AD4030_REG_GAIN_X1_LSB 0x1E
> +#define AD4030_REG_GAIN_X1_MSB 0x1F
> +#define AD4030_REG_GAIN_MAX_GAIN 1999970
> +#define AD4030_REG_GAIN_BYTES_NB 2
> +#define AD4030_REG_GAIN_CHAN(ch) (AD4030_REG_GAIN_X0_MSB + \
> + (AD4030_REG_GAIN_BYTES_NB * \
> + (ch)))
#define AD4030_REG_GAIN_CHAN(ch) \
(AD4030_REG_GAIN_X0_MSB + (AD4030_REG_GAIN_BYTES_NB * (ch)))
Is perhaps more readable?
> +enum ad4030_out_mode {
> + AD4030_OUT_DATA_MD_DIFF,
> + AD4030_OUT_DATA_MD_16_DIFF_8_COM,
> + AD4030_OUT_DATA_MD_24_DIFF_8_COM,
> + AD4030_OUT_DATA_MD_30_AVERAGED_DIFF,
> + AD4030_OUT_DATA_MD_32_PATTERN
Trivial but add a trailing comma. It's not obviously a terminating
entry so even though we know there won't be other values convention is
to have the comma.
> +};
>
> +
> +static int ad4030_get_chan_scale(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2)
> +{
> + struct ad4030_state *st = iio_priv(indio_dev);
> +
> + if (chan->differential) {
> + *val = (st->vref_uv * 2) / MILLI;
> + *val2 = st->chip->precision_bits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
> + *val = st->vref_uv / 256;
This is a bit non obvious.
A comment on this scaling might be good to have.
Particularly the lack of / MILLI
(I think that's a bug?)
> + return IIO_VAL_INT;
> +}
> +static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct ad4030_state *st = iio_priv(indio_dev);
0-day noticed this is not used.
> + int ret;
This doesn't do anything useful either.
> +
> + ret = ad4030_set_mode(indio_dev, *indio_dev->active_scan_mask);
> + if (ret)
> + return ret;
return ad4030_set_mode();
> +
> + return 0;
> +}
> +
> +static int ad4030_regulators_get(struct ad4030_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + static const char * const ids[] = { "vdd-5v", "vdd-1v8" };
> + int ret;
> +
> + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> + st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> + if (st->vio_uv < 0)
> + return dev_err_probe(dev, st->vio_uv,
> + "Failed to enable and read vio voltage\n");
> +
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> + if (st->vref_uv < 0) {
> + if (st->vref_uv != -ENODEV)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to read ref voltage\n");
> +
> + /* if not using optional REF, the internal REFIN must be used */
> + st->vref_uv = devm_regulator_get_enable_read_voltage(dev,
> + "refin");
If refin is internal. How are we reading it's voltage?
Ah. It's not. It's just buffered. Drop 'internal' above and this is fine.
> + if (st->vref_uv < 0)
> + return dev_err_probe(dev, st->vref_uv,
> + "Failed to read refin voltage\n");
> + }
> +
> + return 0;
> +}
> +
> +static int ad4030_reset(struct ad4030_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct gpio_desc *reset;
> + int ret;
> +
> + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset))
> + return dev_err_probe(dev, PTR_ERR(reset),
> + "Failed to get reset GPIO\n");
> +
> + if (reset) {
> + ndelay(50);
> + gpiod_set_value_cansleep(reset, 0);
Could make trivial change to
return 0;
}
return regmap_write()
> + } else {
> + ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> + AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Powered by blists - more mailing lists