| 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: <9b8204f2-107a-477e-a822-c1649af12d02@baylibre.com>
Date: Mon, 27 Jan 2025 19:29:47 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
marcelo.schmitt@...log.com, jic23@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, jonath4nns@...il.com,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 07/16] iio: adc: ad7768-1: convert driver to use regmap
On 1/27/25 9:12 AM, Jonathan Santos wrote:
> Convert the AD7768-1 driver to use the regmap API for register
> access. This change simplifies and standardizes register interactions,
> reducing code duplication and improving maintainability.
>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
> ---
> v2 Changes:
> * New patch in v2.
> ---
> drivers/iio/adc/ad7768-1.c | 82 +++++++++++++++++++++++++++-----------
> 1 file changed, 58 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 95ba89435652..fb8d6fae5f8a 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -12,6 +12,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/sysfs.h>
> #include <linux/spi/spi.h>
> @@ -153,6 +154,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>
> struct ad7768_state {
> struct spi_device *spi;
> + struct regmap *regmap;
> struct regulator *vref;
> struct mutex lock;
> struct clk *mclk;
> @@ -176,12 +178,17 @@ struct ad7768_state {
> } data __aligned(IIO_DMA_MINALIGN);
> };
>
> -static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
> - unsigned int len)
> +static int ad7768_spi_reg_read(void *context, unsigned int addr,
> + unsigned int *val)
> {
> - unsigned int shift;
> + struct iio_dev *dev = context;
> + struct ad7768_state *st;
> + unsigned int shift, len;
> int ret;
>
> + st = iio_priv(dev);
This can be combined with the variable declaration.
> + /* Regular value size is 1 Byte, but 3 Bytes for ADC data */
Probably not currently needed but COEFF_DATA register is also 3 bytes.
> + len = (addr == AD7768_REG_ADC_DATA) ? 3 : 1;
> shift = 32 - (8 * len);
> st->data.d8[0] = AD7768_RD_FLAG_MSK(addr);
>
> @@ -190,13 +197,19 @@ static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
> if (ret < 0)
> return ret;
>
> - return (be32_to_cpu(st->data.d32) >> shift);
> + *val = be32_to_cpu(st->data.d32) >> shift;
> +
> + return 0;
> }
>
> -static int ad7768_spi_reg_write(struct ad7768_state *st,
> +static int ad7768_spi_reg_write(void *context,
> unsigned int addr,
> unsigned int val)
> {
> + struct iio_dev *dev = context;
> + struct ad7768_state *st;
> +
> + st = iio_priv(dev);
> st->data.d8[0] = AD7768_WR_FLAG_MSK(addr);
> st->data.d8[1] = val & 0xFF;
>
> @@ -206,16 +219,16 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
> static int ad7768_set_mode(struct ad7768_state *st,
> enum ad7768_conv_mode mode)
> {
> - int regval;
> + int regval, ret;
>
> - regval = ad7768_spi_reg_read(st, AD7768_REG_CONVERSION, 1);
> - if (regval < 0)
> - return regval;
> + ret = regmap_read(st->regmap, AD7768_REG_CONVERSION, ®val);
> + if (ret)
> + return ret;
>
> regval &= ~AD7768_CONV_MODE_MSK;
> regval |= AD7768_CONV_MODE(mode);
>
> - return ad7768_spi_reg_write(st, AD7768_REG_CONVERSION, regval);
> + return regmap_write(st->regmap, AD7768_REG_CONVERSION, regval);
> }
>
> static int ad7768_scan_direct(struct iio_dev *indio_dev)
> @@ -234,9 +247,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> if (!ret)
> return -ETIMEDOUT;
>
> - readval = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
> - if (readval < 0)
> - return readval;
> + ret = regmap_read(st->regmap, AD7768_REG_ADC_DATA, &readval);
> + if (ret)
> + return ret;
> +
> /*
> * Any SPI configuration of the AD7768-1 can only be
> * performed in continuous conversion mode.
> @@ -258,13 +272,11 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
>
> mutex_lock(&st->lock);
> if (readval) {
> - ret = ad7768_spi_reg_read(st, reg, 1);
> - if (ret < 0)
> + ret = regmap_read(st->regmap, reg, readval);
> + if (ret)
> goto err_unlock;
Can drop the if and goto.
> - *readval = ret;
> - ret = 0;
> } else {
> - ret = ad7768_spi_reg_write(st, reg, writeval);
> + ret = regmap_write(st->regmap, reg, writeval);
> }
> err_unlock:
> mutex_unlock(&st->lock);
> @@ -283,7 +295,7 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
> else
> mode = AD7768_DIG_FIL_DEC_RATE(dec_rate);
>
> - ret = ad7768_spi_reg_write(st, AD7768_REG_DIGITAL_FILTER, mode);
> + ret = regmap_write(st->regmap, AD7768_REG_DIGITAL_FILTER, mode);
> if (ret < 0)
> return ret;
>
> @@ -320,7 +332,7 @@ static int ad7768_set_freq(struct ad7768_state *st,
> */
> pwr_mode = AD7768_PWR_MCLK_DIV(ad7768_clk_config[idx].mclk_div) |
> AD7768_PWR_PWRMODE(ad7768_clk_config[idx].pwrmode);
> - ret = ad7768_spi_reg_write(st, AD7768_REG_POWER_CLOCK, pwr_mode);
> + ret = regmap_write(st->regmap, AD7768_REG_POWER_CLOCK, pwr_mode);
> if (ret < 0)
> return ret;
>
> @@ -447,11 +459,11 @@ static int ad7768_setup(struct ad7768_state *st)
> * to 10. When the sequence is detected, the reset occurs.
> * See the datasheet, page 70.
> */
> - ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x3);
> + ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x3);
> if (ret)
> return ret;
>
> - ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x2);
> + ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x2);
> if (ret)
> return ret;
>
> @@ -509,18 +521,19 @@ static int ad7768_buffer_postenable(struct iio_dev *indio_dev)
> * continuous read mode. Subsequent data reads do not require an
> * initial 8-bit write to query the ADC_DATA register.
> */
> - return ad7768_spi_reg_write(st, AD7768_REG_INTERFACE_FORMAT, 0x01);
> + return regmap_write(st->regmap, AD7768_REG_INTERFACE_FORMAT, 0x01);
> }
>
> static int ad7768_buffer_predisable(struct iio_dev *indio_dev)
> {
> struct ad7768_state *st = iio_priv(indio_dev);
> + unsigned int regval;
Intention could be more clear by calling this "unused". Otherwise, it can look
like a bug if you don't fully understand what the comment below means.
>
> /*
> * To exit continuous read mode, perform a single read of the ADC_DATA
> * reg (0x2C), which allows further configuration of the device.
> */
> - return ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
> + return regmap_read(st->regmap, AD7768_REG_ADC_DATA, ®val);
> }
>
> static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> @@ -563,6 +576,20 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
> return 0;
> }
>
> +static const struct regmap_bus ad7768_regmap_bus = {
> + .reg_write = ad7768_spi_reg_write,
> + .reg_read = ad7768_spi_reg_read,
> + .reg_format_endian_default = REGMAP_ENDIAN_BIG,
> + .val_format_endian_default = REGMAP_ENDIAN_BIG,
The bus read function is calling be32_to_cpu(), so we probably want to remove
that or change the default here.
> +};
> +
> +static const struct regmap_config ad7768_regmap_config = {
> + .name = "ad7768-1",
> + .reg_bits = 8,
> + .val_bits = 8,
Should this be 24 since the largest registers are 24-bit?
Another option could be to just use a regular spi_*() API for that register
instead of regmap_*() and avoid trying to do something that regmap doesn't
really handle.
Or we could possibly use regmap_bulk_read(), but that feels a bit hacky too
since it isn't actually how that function was intended to be used.
> + .max_register = AD7768_REG_MCLK_COUNTER,
> +};
> +
> static int ad7768_probe(struct spi_device *spi)
> {
> struct ad7768_state *st;
> @@ -592,6 +619,13 @@ static int ad7768_probe(struct spi_device *spi)
>
> st->spi = spi;
>
> + st->regmap = devm_regmap_init(&spi->dev,
> + &ad7768_regmap_bus, indio_dev,
> + &ad7768_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> + "Failed to initialize regmap");
> +
> st->vref = devm_regulator_get(&spi->dev, "vref");
> if (IS_ERR(st->vref))
> return PTR_ERR(st->vref);
Powered by blists - more mailing lists