| 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: <Z8ryhMV1lYZeZVCB@debian-BULLSEYE-live-builder-AMD64>
Date: Fri, 7 Mar 2025 10:20:04 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
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, linus.walleij@...aro.org,
brgl@...ev.pl, lgirdwood@...il.com, broonie@...nel.org,
dlechner@...libre.com, jonath4nns@...il.com
Subject: Re: [PATCH v4 08/17] iio: adc: ad7768-1: convert driver to use regmap
On 03/06, 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.
>
> Create two regmap configurations, one for 8-bit register values and
> other for 24-bit register values.
>
> Since we are using regmap now, define the remaining registers from 0x32
> to 0x34.
>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
> ---
> v4 Changes:
> * Add `REGMAP24` to the register macros with 24-bit value.
> * Add `select REGMAP_SPI` line to the Kconfig.
>
> v3 Changes:
> * Included a second register map for the 24-bit register values.
> * Added register tables to separate the 24-bit from the 8-bit values.
>
> v2 Changes:
> * New patch in v2.
> ---
> drivers/iio/adc/Kconfig | 1 +
> drivers/iio/adc/ad7768-1.c | 151 +++++++++++++++++++++++++------------
> 2 files changed, 104 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 849c90203071..a2fdb7e03a66 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -277,6 +277,7 @@ config AD7766
> config AD7768_1
> tristate "Analog Devices AD7768-1 ADC driver"
> depends on SPI
> + select REGMAP_SPI
> select IIO_BUFFER
> select IIO_TRIGGER
> select IIO_TRIGGERED_BUFFER
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index f5509a0a36ab..04a26e5b7d5c 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>
> @@ -53,12 +54,15 @@
> #define AD7768_REG_SPI_DIAG_ENABLE 0x28
> #define AD7768_REG_ADC_DIAG_ENABLE 0x29
> #define AD7768_REG_DIG_DIAG_ENABLE 0x2A
> -#define AD7768_REG_ADC_DATA 0x2C
> +#define AD7768_REG24_ADC_DATA 0x2C
> #define AD7768_REG_MASTER_STATUS 0x2D
> #define AD7768_REG_SPI_DIAG_STATUS 0x2E
> #define AD7768_REG_ADC_DIAG_STATUS 0x2F
> #define AD7768_REG_DIG_DIAG_STATUS 0x30
> #define AD7768_REG_MCLK_COUNTER 0x31
> +#define AD7768_REG_COEFF_CONTROL 0x32
> +#define AD7768_REG24_COEFF_DATA 0x33
> +#define AD7768_REG_ACCESS_KEY 0x34
>
> /* AD7768_REG_POWER_CLOCK */
> #define AD7768_PWR_MCLK_DIV_MSK GENMASK(5, 4)
> @@ -153,6 +157,8 @@ static const struct iio_chan_spec ad7768_channels[] = {
>
> struct ad7768_state {
> struct spi_device *spi;
> + struct regmap *regmap;
> + struct regmap *regmap24;
> struct regulator *vref;
> struct clk *mclk;
> unsigned int mclk_freq;
> @@ -175,46 +181,76 @@ struct ad7768_state {
> } data __aligned(IIO_DMA_MINALIGN);
> };
>
> -static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
> - unsigned int len)
> -{
> - unsigned int shift;
> - int ret;
> +static const struct regmap_range ad7768_regmap_rd_ranges[] = {
> + regmap_reg_range(AD7768_REG_CHIP_TYPE, AD7768_REG_DIG_DIAG_ENABLE),
> + regmap_reg_range(AD7768_REG_MASTER_STATUS, AD7768_REG_COEFF_CONTROL),
> + regmap_reg_range(AD7768_REG_ACCESS_KEY, AD7768_REG_ACCESS_KEY),
> +};
>
> - shift = 32 - (8 * len);
> - st->data.d8[0] = AD7768_RD_FLAG_MSK(addr);
> +static const struct regmap_access_table ad7768_regmap_rd_table = {
> + .yes_ranges = ad7768_regmap_rd_ranges,
> + .n_yes_ranges = ARRAY_SIZE(ad7768_regmap_rd_ranges),
> +};
>
> - ret = spi_write_then_read(st->spi, st->data.d8, 1,
> - &st->data.d32, len);
> - if (ret < 0)
> - return ret;
> +static const struct regmap_range ad7768_regmap_wr_ranges[] = {
> + regmap_reg_range(AD7768_REG_SCRATCH_PAD, AD7768_REG_SCRATCH_PAD),
> + regmap_reg_range(AD7768_REG_INTERFACE_FORMAT, AD7768_REG_GPIO_WRITE),
> + regmap_reg_range(AD7768_REG_OFFSET_HI, AD7768_REG_DIG_DIAG_ENABLE),
> + regmap_reg_range(AD7768_REG_SPI_DIAG_STATUS, AD7768_REG_SPI_DIAG_STATUS),
> + regmap_reg_range(AD7768_REG_COEFF_CONTROL, AD7768_REG_COEFF_CONTROL),
> +};
>
> - return (be32_to_cpu(st->data.d32) >> shift);
> -}
> +static const struct regmap_access_table ad7768_regmap_wr_table = {
> + .yes_ranges = ad7768_regmap_wr_ranges,
> + .n_yes_ranges = ARRAY_SIZE(ad7768_regmap_wr_ranges),
> +};
>
> -static int ad7768_spi_reg_write(struct ad7768_state *st,
> - unsigned int addr,
> - unsigned int val)
> -{
> - st->data.d8[0] = AD7768_WR_FLAG_MSK(addr);
> - st->data.d8[1] = val & 0xFF;
> +static const struct regmap_config ad7768_regmap_config = {
> + .name = "ad7768-1-8",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .read_flag_mask = BIT(6),
> + .rd_table = &ad7768_regmap_rd_table,
> + .wr_table = &ad7768_regmap_wr_table,
> + .max_register = AD7768_REG_ACCESS_KEY,
> + .use_single_write = true,
> + .use_single_read = true,
> +};
>
> - return spi_write(st->spi, st->data.d8, 2);
> -}
> +static const struct regmap_range ad7768_regmap24_rd_ranges[] = {
> + regmap_reg_range(AD7768_REG24_ADC_DATA, AD7768_REG24_ADC_DATA),
> + regmap_reg_range(AD7768_REG24_COEFF_DATA, AD7768_REG24_COEFF_DATA),
So, this device has only two registers that are 24-bit size?
Also, one of those is the ADC_DATA register which you will probably want
to read with optimized SPI messages in the future (devm_spi_optimize_message()).
That makes me wonder if the 24-bit regmap worth's the boiler plate to have it.
Does the driver access AD7768_REG24_COEFF_DATA after the patches from this
series is applied? If not, maybe drop the 24-bit regmap and implement ADC_DATA
with usual spi_message/spi_transfer interfaces?
> +};
>
> -static int ad7768_set_mode(struct ad7768_state *st,
> - enum ad7768_conv_mode mode)
> -{
> - int regval;
> +static const struct regmap_access_table ad7768_regmap24_rd_table = {
> + .yes_ranges = ad7768_regmap24_rd_ranges,
> + .n_yes_ranges = ARRAY_SIZE(ad7768_regmap24_rd_ranges),
> +};
>
> - regval = ad7768_spi_reg_read(st, AD7768_REG_CONVERSION, 1);
> - if (regval < 0)
> - return regval;
> +static const struct regmap_range ad7768_regmap24_wr_ranges[] = {
> + regmap_reg_range(AD7768_REG24_COEFF_DATA, AD7768_REG24_COEFF_DATA),
> +};
>
> - regval &= ~AD7768_CONV_MODE_MSK;
> - regval |= AD7768_CONV_MODE(mode);
> +static const struct regmap_access_table ad7768_regmap24_wr_table = {
> + .yes_ranges = ad7768_regmap24_wr_ranges,
> + .n_yes_ranges = ARRAY_SIZE(ad7768_regmap24_wr_ranges),
> +};
> +
> +static const struct regmap_config ad7768_regmap24_config = {
> + .name = "ad7768-1-24",
> + .reg_bits = 8,
> + .val_bits = 24,
> + .read_flag_mask = BIT(6),
> + .rd_table = &ad7768_regmap24_rd_table,
> + .wr_table = &ad7768_regmap24_wr_table,
> + .max_register = AD7768_REG24_COEFF_DATA,
> +};
>
> - return ad7768_spi_reg_write(st, AD7768_REG_CONVERSION, regval);
> +static int ad7768_set_mode(struct ad7768_state *st,
> + enum ad7768_conv_mode mode)
> +{
> + return regmap_update_bits(st->regmap, AD7768_REG_CONVERSION,
> + AD7768_CONV_MODE_MSK, AD7768_CONV_MODE(mode));
> }
>
> static int ad7768_scan_direct(struct iio_dev *indio_dev)
> @@ -233,9 +269,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->regmap24, AD7768_REG24_ADC_DATA, &readval);
> + if (ret)
> + return ret;
> +
> /*
> * Any SPI configuration of the AD7768-1 can only be
> * performed in continuous conversion mode.
> @@ -259,16 +296,23 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
> if (ret)
> return ret;
>
> + ret = -EINVAL;
> if (readval) {
> - ret = ad7768_spi_reg_read(st, reg, 1);
> - if (ret < 0)
> - goto err_release;
> - *readval = ret;
> - ret = 0;
> + if (regmap_check_range_table(st->regmap, reg, &ad7768_regmap_rd_table))
> + ret = regmap_read(st->regmap, reg, readval);
> +
> + if (regmap_check_range_table(st->regmap24, reg, &ad7768_regmap24_rd_table))
> + ret = regmap_read(st->regmap24, reg, readval);
> +
> } else {
> - ret = ad7768_spi_reg_write(st, reg, writeval);
> + if (regmap_check_range_table(st->regmap, reg, &ad7768_regmap_wr_table))
> + ret = regmap_write(st->regmap, reg, writeval);
> +
> + if (regmap_check_range_table(st->regmap24, reg, &ad7768_regmap24_wr_table))
> + ret = regmap_write(st->regmap24, reg, writeval);
> +
> }
> -err_release:
> +
> iio_device_release_direct_mode(indio_dev);
>
> return ret;
> @@ -285,7 +329,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;
>
> @@ -322,7 +366,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;
>
> @@ -449,11 +493,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;
>
> @@ -508,18 +552,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 unused;
>
> /*
> * 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->regmap24, AD7768_REG24_ADC_DATA, &unused);
> }
>
> static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> @@ -590,6 +635,16 @@ static int ad7768_probe(struct spi_device *spi)
>
> st->spi = spi;
>
> + st->regmap = devm_regmap_init_spi(spi, &ad7768_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> + "Failed to initialize regmap");
> +
> + st->regmap24 = devm_regmap_init_spi(spi, &ad7768_regmap24_config);
> + if (IS_ERR(st->regmap24))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap24),
> + "Failed to initialize regmap24");
> +
> st->vref = devm_regulator_get(&spi->dev, "vref");
> if (IS_ERR(st->vref))
> return PTR_ERR(st->vref);
> --
> 2.34.1
>
Powered by blists - more mailing lists