LinuxLists.cc - [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

2023-01-10 16:16:35

Subject: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

From: Hugo Villeneuve <[email protected]>

The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
digital converter (ADC) with an I2C interface.

Datasheet: https://www.ti.com/lit/gpn/ads7924

Signed-off-by: Hugo Villeneuve <[email protected]>
---
MAINTAINERS | 7 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads7924.c | 492 +++++++++++++++++++++++++++++++++++
4 files changed, 511 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads7924.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ff472ca1244..94eb68076f0c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20776,6 +20776,13 @@ M: Robert Richter <[email protected]>
S: Odd Fixes
F: drivers/gpio/gpio-thunderx.c

+TI ADS7924 ADC DRIVER
+M: Hugo Villeneuve <[email protected]>
+L: [email protected]
+S: Supported
+F: Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
+F: drivers/iio/adc/ti-ads7924.c
+
TI AM437X VPFE DRIVER
M: "Lad, Prabhakar" <[email protected]>
L: [email protected]
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7d6e74189a2d..99468c64daf9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1208,6 +1208,17 @@ config TI_ADS1015
This driver can also be built as a module. If so, the module will be
called ti-ads1015.

+config TI_ADS7924
+ tristate "Texas Instruments ADS7924 ADC"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ If you say yes here you get support for Texas Instruments ADS7924
+ 4 channels, 12-bit I2C ADC chip.
+
+ This driver can also be built as a module. If so, the module will be
+ called ti-ads7924.
+
config TI_ADS7950
tristate "Texas Instruments ADS7950 ADC driver"
depends on SPI && GPIOLIB
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 6e08415c3f3a..1e5bdf47a091 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
+obj-$(CONFIG_TI_ADS7924) += ti-ads7924.o
obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
new file mode 100644
index 000000000000..8384f3451236
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -0,0 +1,492 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C
+ *
+ * iio/adc/ti-ads7924.c
+ * Author: Hugo Villeneuve <[email protected]>
+ * Copyright 2022 DimOnOff
+ *
+ * based on iio/adc/ti-ads1015.c
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/ads7924
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+#define ADS7924_CHANNELS 4
+#define ADS7924_BITS 12
+#define ADS7924_DATA_SHIFT 4
+
+/* Registers. */
+#define ADS7924_MODECNTRL_REG 0x00
+#define ADS7924_INTCNTRL_REG 0x01
+#define ADS7924_DATA0_U_REG 0x02
+#define ADS7924_DATA0_L_REG 0x03
+#define ADS7924_DATA1_U_REG 0x04
+#define ADS7924_DATA1_L_REG 0x05
+#define ADS7924_DATA2_U_REG 0x06
+#define ADS7924_DATA2_L_REG 0x07
+#define ADS7924_DATA3_U_REG 0x08
+#define ADS7924_DATA3_L_REG 0x09
+#define ADS7924_ULR0_REG 0x0A
+#define ADS7924_LLR0_REG 0x0B
+#define ADS7924_ULR1_REG 0x0C
+#define ADS7924_LLR1_REG 0x0D
+#define ADS7924_ULR2_REG 0x0E
+#define ADS7924_LLR2_REG 0x0F
+#define ADS7924_ULR3_REG 0x10
+#define ADS7924_LLR3_REG 0x11
+#define ADS7924_INTCONFIG_REG 0x12
+#define ADS7924_SLPCONFIG_REG 0x13
+#define ADS7924_ACQCONFIG_REG 0x14
+#define ADS7924_PWRCONFIG_REG 0x15
+#define ADS7924_RESET_REG 0x16
+
+/*
+ * Register address INC bit: when set to '1', the register address is
+ * automatically incremented after every register read which allows convenient
+ * reading of multiple registers. Set INC to '0' when reading a single register.
+ */
+#define ADS7924_AUTO_INCREMENT_BIT BIT(7)
+
+#define ADS7924_MODECNTRL_MODE_MASK GENMASK(7, 2)
+
+#define ADS7924_MODECNTRL_SEL_MASK GENMASK(1, 0)
+
+#define ADS7924_CFG_INTPOL_BIT 1
+#define ADS7924_CFG_INTTRIG_BIT 0
+
+#define ADS7924_CFG_INTPOL_MASK BIT(ADS7924_CFG_INTPOL_BIT)
+#define ADS7924_CFG_INTTRIG_MASK BIT(ADS7924_CFG_INTTRIG_BIT)
+
+/* Interrupt pin polarity */
+#define ADS7924_CFG_INTPOL_LOW 0
+#define ADS7924_CFG_INTPOL_HIGH 1
+
+/* Interrupt pin signaling */
+#define ADS7924_CFG_INTTRIG_LEVEL 0
+#define ADS7924_CFG_INTTRIG_EDGE 1
+
+/* Mode control values */
+#define ADS7924_MODECNTRL_IDLE 0x00
+#define ADS7924_MODECNTRL_AWAKE 0x20
+#define ADS7924_MODECNTRL_MANUAL_SINGLE 0x30
+#define ADS7924_MODECNTRL_MANUAL_SCAN 0x32
+#define ADS7924_MODECNTRL_AUTO_SINGLE 0x31
+#define ADS7924_MODECNTRL_AUTO_SCAN 0x33
+#define ADS7924_MODECNTRL_AUTO_SINGLE_SLEEP 0x39
+#define ADS7924_MODECNTRL_AUTO_SCAN_SLEEP 0x3B
+#define ADS7924_MODECNTRL_AUTO_BURST_SLEEP 0x3F
+
+#define ADS7924_ACQTIME_MASK GENMASK(4, 0)
+
+#define ADS7924_PWRUPTIME_MASK GENMASK(4, 0)
+
+/*
+ * The power-up time is allowed to elapse whenever the device has been shutdown
+ * in idle mode. Power-up time can allow external circuits, such as an
+ * operational amplifier, between the MUXOUT and ADCIN pins to turn on.
+ * The nominal time programmed by the PUTIME[4:0] register bits is given by:
+ * t PU = PWRUPTIME[4:0] × 2 μs
+ * If a power-up time is not required, set the bits to '0' to effectively bypass.
+ */
+#define ADS7924_PWRUPTIME_US 0 /* Bypass (0us). */
+
+/*
+ * Acquisition Time according to ACQTIME[4:0] register bits.
+ * The Acquisition Time is given by:
+ * t ACQ = (ACQTIME[4:0] × 2 μs) + 6 μs
+ * Using default value of 0 for ACQTIME[4:0] results in a minimum acquisition
+ * time of 6us.
+ */
+#define ADS7924_ACQTIME_US 6
+
+/* The conversion time is always 4μs and cannot be programmed by the user. */
+#define ADS7924_CONVTIME_US 4
+
+#define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
+ ADS7924_CONVTIME_US)
+
+#define ADS7924_SLEEP_DELAY_MS 2000
+
+#define ADS7924_V_CHAN(_chan, _addr) { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = _chan, \
+ .address = _addr, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .datasheet_name = "AIN"#_chan, \
+}
+
+struct ads7924_data {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *vref_reg;
+
+ /* GPIO descriptor for device hard-reset pin. */
+ struct gpio_desc *reset_gpio;
+
+ /*
+ * Protects ADC ops, e.g: concurrent sysfs/buffered
+ * data reads, configuration updates
+ */
+ struct mutex lock;
+
+ /*
+ * Set to true when the ADC is switched to the continuous-conversion
+ * mode and exits from a power-down state. This flag is used to avoid
+ * getting the stale result from the conversion register.
+ */
+ bool conv_invalid;
+};
+
+static bool ads7924_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case ADS7924_MODECNTRL_REG:
+ case ADS7924_INTCNTRL_REG:
+ case ADS7924_ULR0_REG:
+ case ADS7924_LLR0_REG:
+ case ADS7924_ULR1_REG:
+ case ADS7924_LLR1_REG:
+ case ADS7924_ULR2_REG:
+ case ADS7924_LLR2_REG:
+ case ADS7924_ULR3_REG:
+ case ADS7924_LLR3_REG:
+ case ADS7924_INTCONFIG_REG:
+ case ADS7924_SLPCONFIG_REG:
+ case ADS7924_ACQCONFIG_REG:
+ case ADS7924_PWRCONFIG_REG:
+ case ADS7924_RESET_REG:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config ads7924_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = ADS7924_RESET_REG,
+ .writeable_reg = ads7924_is_writeable_reg,
+};
+
+static const struct iio_chan_spec ads7924_channels[] = {
+ ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
+ ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
+ ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
+ ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
+};
+
+static
+int ads7924_get_adc_result(struct ads7924_data *data,
+ struct iio_chan_spec const *chan, int *val)
+{
+ int ret;
+ __be16 be_val;
+
+ if (chan->channel < 0 || chan->channel >= ADS7924_CHANNELS)
+ return -EINVAL;
+
+ if (data->conv_invalid) {
+ int conv_time;
+
+ conv_time = ADS7924_TOTAL_CONVTIME_US;
+ /* Allow 10% for internal clock inaccuracy. */
+ conv_time += conv_time / 10;
+ usleep_range(conv_time, conv_time + 1);
+ data->conv_invalid = false;
+ }
+
+ ret = regmap_raw_read(data->regmap, ADS7924_AUTO_INCREMENT_BIT |
+ chan->address, &be_val, sizeof(be_val));
+ if (ret)
+ return ret;
+
+ *val = be16_to_cpu(be_val);
+ *val = *val >> ADS7924_DATA_SHIFT;
+
+ return ret;
+}
+
+static int ads7924_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ int ret, vref_uv;
+ struct ads7924_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW: {
+ mutex_lock(&data->lock);
+ ret = ads7924_get_adc_result(data, chan, val);
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ dev_dbg(data->dev, "%s error1\n", __func__);
+ else
+ ret = IIO_VAL_INT;
+ break;
+ }
+ case IIO_CHAN_INFO_SCALE:
+ vref_uv = regulator_get_voltage(data->vref_reg);
+ if (vref_uv < 0) {
+ /* dummy regulator "get_voltage" returns -EINVAL */
+ ret = -EINVAL;
+ } else {
+ *val = vref_uv / 1000; /* Convert reg voltage to mV */
+ *val2 = ADS7924_BITS;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static const struct iio_info ads7924_info = {
+ .read_raw = ads7924_read_raw,
+};
+
+static int ads7924_get_channels_config(struct i2c_client *client,
+ struct iio_dev *indio_dev)
+{
+ struct ads7924_data *priv = iio_priv(indio_dev);
+ struct device *dev = priv->dev;
+ struct fwnode_handle *node;
+ int num_channels = 0;
+
+ device_for_each_child_node(dev, node) {
+ u32 pval;
+ unsigned int channel;
+
+ if (fwnode_property_read_u32(node, "reg", &pval)) {
+ dev_err(dev, "invalid reg on %pfw\n", node);
+ continue;
+ }
+
+ channel = pval;
+ if (channel >= ADS7924_CHANNELS) {
+ dev_err(dev, "invalid channel index %d on %pfw\n",
+ channel, node);
+ continue;
+ }
+
+ num_channels++;
+ }
+
+ if (num_channels > 0) {
+ dev_dbg(dev, "found %d ADC channels\n", num_channels);
+ return 0;
+ } else {
+ return -EINVAL;
+ }
+}
+
+static int ads7924_set_conv_mode(struct ads7924_data *data, int mode)
+{
+ int ret;
+ unsigned int mode_field;
+ struct device *dev = data->dev;
+
+ /*
+ * When switching between modes, be sure to first select the Awake mode
+ * and then switch to the desired mode. This procedure ensures the
+ * internal control logic is properly synchronized.
+ */
+ if (mode != ADS7924_MODECNTRL_IDLE) {
+ mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK,
+ ADS7924_MODECNTRL_AWAKE);
+
+ ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
+ ADS7924_MODECNTRL_MODE_MASK,
+ mode_field);
+ if (ret) {
+ dev_warn(dev, "failed to set awake mode (%pe)\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+ }
+
+ mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK, mode);
+
+ ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
+ ADS7924_MODECNTRL_MODE_MASK, mode_field);
+ if (ret)
+ dev_warn(dev, "failed to set mode %d (%pe)\n", mode,
+ ERR_PTR(ret));
+
+ return ret;
+}
+
+static int ads7924_reset(struct iio_dev *indio_dev)
+{
+ struct ads7924_data *data = iio_priv(indio_dev);
+
+ if (data->reset_gpio) {
+ gpiod_set_value(data->reset_gpio, 1); /* Assert. */
+ /* Educated guess: assert time not specified in datasheet... */
+ mdelay(100);
+ gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
+ } else {
+ int ret;
+
+ /*
+ * A write of 10101010 to this register will generate a
+ * software reset of the ADS7924.
+ */
+ ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+};
+
+static void ads7924_reg_disable(void *data)
+{
+ regulator_disable(data);
+}
+
+static void ads7924_set_idle_mode(void *data)
+{
+ ads7924_set_conv_mode(data, ADS7924_MODECNTRL_IDLE);
+}
+
+static int ads7924_probe(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev;
+ struct ads7924_data *data;
+ struct device *dev = &client->dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+
+ data->dev = dev;
+
+ /* Initialize the reset GPIO as output with an initial value of 0. */
+ data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(data->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
+ "failed to get request reset GPIO\n");
+
+ mutex_init(&data->lock);
+
+ indio_dev->name = "ads7924";
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ indio_dev->channels = ads7924_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
+ indio_dev->info = &ads7924_info;
+
+ ret = ads7924_get_channels_config(client, indio_dev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to get channels configuration\n");
+
+ data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config);
+ if (IS_ERR(data->regmap))
+ return dev_err_probe(dev, PTR_ERR(data->regmap),
+ "failed to init regmap\n");
+
+ data->vref_reg = devm_regulator_get(dev, "vref");
+ if (IS_ERR(data->vref_reg))
+ return dev_err_probe(dev, PTR_ERR(data->vref_reg),
+ "failed to get vref regulator\n");
+
+ ret = regulator_enable(data->vref_reg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to enable regulator\n");
+
+ ret = devm_add_action_or_reset(dev, ads7924_reg_disable, data->vref_reg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to add regulator disable action\n");
+
+ ret = ads7924_reset(indio_dev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to reset device\n");
+
+ ret = devm_add_action_or_reset(dev, ads7924_set_idle_mode, data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to add idle mode action\n");
+
+ ret = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_AUTO_SCAN);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to set conversion mode\n");
+
+ /* Use minimum signal acquire time. */
+ ret = regmap_update_bits(data->regmap, ADS7924_ACQCONFIG_REG,
+ ADS7924_ACQTIME_MASK,
+ FIELD_PREP(ADS7924_ACQTIME_MASK, 0));
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to configure signal acquire time\n");
+
+ /* Disable power-up time. */
+ ret = regmap_update_bits(data->regmap, ADS7924_PWRCONFIG_REG,
+ ADS7924_PWRUPTIME_MASK,
+ FIELD_PREP(ADS7924_PWRUPTIME_MASK, 0));
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to configure power-up time\n");
+
+ data->conv_invalid = true;
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to register IIO device\n");
+
+ return 0;
+}
+
+static const struct i2c_device_id ads7924_id[] = {
+ { "ads7924", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ads7924_id);
+
+static const struct of_device_id ads7924_of_match[] = {
+ { .compatible = "ti,ads7924", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ads7924_of_match);
+
+static struct i2c_driver ads7924_driver = {
+ .driver = {
+ .name = "ads7924",
+ .of_match_table = ads7924_of_match,
+ },
+ .probe_new = ads7924_probe,
+ .id_table = ads7924_id,
+};
+
+module_i2c_driver(ads7924_driver);
+
+MODULE_AUTHOR("Hugo Villeneuve <[email protected]>");
+MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver");
+MODULE_LICENSE("GPL");
--
2.30.2

2023-01-10 19:20:04

by Christophe JAILLET

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

Le 10/01/2023 à 17:01, Hugo Villeneuve a écrit :
> From: Hugo Villeneuve <[email protected]>
>
> The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> digital converter (ADC) with an I2C interface.
>
> Datasheet: https://www.ti.com/lit/gpn/ads7924
>
> Signed-off-by: Hugo Villeneuve <[email protected]>
> ---

Hi,

should there be a v3, a few nits below.

CJ

> MAINTAINERS | 7 +
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads7924.c | 492 +++++++++++++++++++++++++++++++++++
> 4 files changed, 511 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads7924.c
>

[...]

> +static int ads7924_get_channels_config(struct i2c_client *client,
> + struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *priv = iio_priv(indio_dev);
> + struct device *dev = priv->dev;
> + struct fwnode_handle *node;
> + int num_channels = 0;
> +
> + device_for_each_child_node(dev, node) {
> + u32 pval;
> + unsigned int channel;
> +
> + if (fwnode_property_read_u32(node, "reg", &pval)) {
> + dev_err(dev, "invalid reg on %pfw\n", node);
> + continue;
> + }
> +
> + channel = pval;
> + if (channel >= ADS7924_CHANNELS) {
> + dev_err(dev, "invalid channel index %d on %pfw\n",
> + channel, node);
> + continue;
> + }
> +
> + num_channels++;
> + }
> +
> + if (num_channels > 0) {
> + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> + return 0;
> + } else {
> + return -EINVAL;
> + }

if (num_channels <= 0)
return -EINVAL;

dev_dbg(dev, "found %d ADC channels\n", num_channels);
return 0;

is much more usual.

> +}
> +

[...]

> +static int ads7924_reset(struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *data = iio_priv(indio_dev);
> +
> + if (data->reset_gpio) {
> + gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> + /* Educated guess: assert time not specified in datasheet... */
> + mdelay(100);
> + gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
> + } else {
> + int ret;

having 'ret' near 'struct ads7924_data *data' is more usual and saves 1 LoC.

> +
> + /*
> + * A write of 10101010 to this register will generate a
> + * software reset of the ADS7924.
> + */
> + ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +};

2023-01-10 23:20:11

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

Hi Hugo,

I love your patch! Yet something to improve:

[auto build test ERROR on bfcae956d9b50ea0e221cefc171604c569017d7e]

url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/iio-adc-ti-ads7924-add-Texas-Instruments-ADS7924-driver/20230111-000513
base: bfcae956d9b50ea0e221cefc171604c569017d7e
patch link: https://lore.kernel.org/r/20230110160124.3853593-2-hugo%40hugovil.com
patch subject: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver
config: sh-allmodconfig
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/b462acbd9ff0d0b52029ea3d060693cda131836d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hugo-Villeneuve/iio-adc-ti-ads7924-add-Texas-Instruments-ADS7924-driver/20230111-000513
git checkout b462acbd9ff0d0b52029ea3d060693cda131836d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/iio/adc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/iio/adc/ti-ads7924.c: In function 'ads7924_set_conv_mode':
>> drivers/iio/adc/ti-ads7924.c:313:30: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
313 | mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK,
| ^~~~~~~~~~
cc1: some warnings being treated as errors

vim +/FIELD_PREP +313 drivers/iio/adc/ti-ads7924.c

300
301 static int ads7924_set_conv_mode(struct ads7924_data *data, int mode)
302 {
303 int ret;
304 unsigned int mode_field;
305 struct device *dev = data->dev;
306
307 /*
308 * When switching between modes, be sure to first select the Awake mode
309 * and then switch to the desired mode. This procedure ensures the
310 * internal control logic is properly synchronized.
311 */
312 if (mode != ADS7924_MODECNTRL_IDLE) {
> 313 mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK,
314 ADS7924_MODECNTRL_AWAKE);
315
316 ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
317 ADS7924_MODECNTRL_MODE_MASK,
318 mode_field);
319 if (ret) {
320 dev_warn(dev, "failed to set awake mode (%pe)\n",
321 ERR_PTR(ret));
322 return ret;
323 }
324 }
325
326 mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK, mode);
327
328 ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
329 ADS7924_MODECNTRL_MODE_MASK, mode_field);
330 if (ret)
331 dev_warn(dev, "failed to set mode %d (%pe)\n", mode,
332 ERR_PTR(ret));
333
334 return ret;
335 }
336

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Attachments:

(No filename) (3.30 kB)
config (250.76 kB)
Download all attachments

2023-01-11 00:46:52

by Hugo Villeneuve

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

On Tue, 10 Jan 2023 19:50:42 +0100
Christophe JAILLET <[email protected]> wrote:

> Le 10/01/2023 ? 17:01, Hugo Villeneuve a ?crit?:
> > From: Hugo Villeneuve <[email protected]>
> >
> > The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> > digital converter (ADC) with an I2C interface.
> >
> > Datasheet: https://www.ti.com/lit/gpn/ads7924
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
> > ---
>
> Hi,
>
> should there be a v3, a few nits below.

Hi,
there will definitely be a V3 (missing include), thanks for the input, I will incorporate these changes into V3.

Hugo.

>
> CJ
>
> > MAINTAINERS | 7 +
> > drivers/iio/adc/Kconfig | 11 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-ads7924.c | 492 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 511 insertions(+)
> > create mode 100644 drivers/iio/adc/ti-ads7924.c
> >
>
> [...]
>
> > +static int ads7924_get_channels_config(struct i2c_client *client,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct ads7924_data *priv = iio_priv(indio_dev);
> > + struct device *dev = priv->dev;
> > + struct fwnode_handle *node;
> > + int num_channels = 0;
> > +
> > + device_for_each_child_node(dev, node) {
> > + u32 pval;
> > + unsigned int channel;
> > +
> > + if (fwnode_property_read_u32(node, "reg", &pval)) {
> > + dev_err(dev, "invalid reg on %pfw\n", node);
> > + continue;
> > + }
> > +
> > + channel = pval;
> > + if (channel >= ADS7924_CHANNELS) {
> > + dev_err(dev, "invalid channel index %d on %pfw\n",
> > + channel, node);
> > + continue;
> > + }
> > +
> > + num_channels++;
> > + }
> > +
> > + if (num_channels > 0) {
> > + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> > + return 0;
> > + } else {
> > + return -EINVAL;
> > + }
>
> if (num_channels <= 0)
> return -EINVAL;
>
> dev_dbg(dev, "found %d ADC channels\n", num_channels);
> return 0;
>
> is much more usual.
>
> > +}
> > +
>
> [...]
>
> > +static int ads7924_reset(struct iio_dev *indio_dev)
> > +{
> > + struct ads7924_data *data = iio_priv(indio_dev);
> > +
> > + if (data->reset_gpio) {
> > + gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> > + /* Educated guess: assert time not specified in datasheet... */
> > + mdelay(100);
> > + gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
> > + } else {
> > + int ret;
>
> having 'ret' near 'struct ads7924_data *data' is more usual and saves 1 LoC.
>
> > +
> > + /*
> > + * A write of 10101010 to this register will generate a
> > + * software reset of the ADS7924.
> > + */
> > + ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +};
>
>

--
Hugo Villeneuve <[email protected]>

2023-01-12 19:28:27

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

2023-01-12 19:56:05

by Hugo Villeneuve

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

On Tue, 10 Jan 2023 11:01:23 -0500
Hugo Villeneuve <[email protected]> wrote:

> From: Hugo Villeneuve <[email protected]>
>
> The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> digital converter (ADC) with an I2C interface.
>
> Datasheet: https://www.ti.com/lit/gpn/ads7924
>
> Signed-off-by: Hugo Villeneuve <[email protected]>
> ---
> MAINTAINERS | 7 +
> drivers/iio/adc/Kconfig | 11 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads7924.c | 492 +++++++++++++++++++++++++++++++++++
> 4 files changed, 511 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads7924.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ff472ca1244..94eb68076f0c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20776,6 +20776,13 @@ M: Robert Richter <[email protected]>
> S: Odd Fixes
> F: drivers/gpio/gpio-thunderx.c
>
> +TI ADS7924 ADC DRIVER
> +M: Hugo Villeneuve <[email protected]>
> +L: [email protected]
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> +F: drivers/iio/adc/ti-ads7924.c
> +
> TI AM437X VPFE DRIVER
> M: "Lad, Prabhakar" <[email protected]>
> L: [email protected]
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7d6e74189a2d..99468c64daf9 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1208,6 +1208,17 @@ config TI_ADS1015
> This driver can also be built as a module. If so, the module will be
> called ti-ads1015.
>
> +config TI_ADS7924
> + tristate "Texas Instruments ADS7924 ADC"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + If you say yes here you get support for Texas Instruments ADS7924
> + 4 channels, 12-bit I2C ADC chip.
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-ads7924.
> +
> config TI_ADS7950
> tristate "Texas Instruments ADS7950 ADC driver"
> depends on SPI && GPIOLIB
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 6e08415c3f3a..1e5bdf47a091 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> +obj-$(CONFIG_TI_ADS7924) += ti-ads7924.o
> obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> new file mode 100644
> index 000000000000..8384f3451236
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7924.c
> @@ -0,0 +1,492 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C
> + *
> + * iio/adc/ti-ads7924.c
> + * Author: Hugo Villeneuve <[email protected]>
> + * Copyright 2022 DimOnOff
> + *
> + * based on iio/adc/ti-ads1015.c
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * Datasheet: https://www.ti.com/lit/gpn/ads7924
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#define ADS7924_CHANNELS 4
> +#define ADS7924_BITS 12
> +#define ADS7924_DATA_SHIFT 4
> +
> +/* Registers. */
> +#define ADS7924_MODECNTRL_REG 0x00
> +#define ADS7924_INTCNTRL_REG 0x01
> +#define ADS7924_DATA0_U_REG 0x02
> +#define ADS7924_DATA0_L_REG 0x03
> +#define ADS7924_DATA1_U_REG 0x04
> +#define ADS7924_DATA1_L_REG 0x05
> +#define ADS7924_DATA2_U_REG 0x06
> +#define ADS7924_DATA2_L_REG 0x07
> +#define ADS7924_DATA3_U_REG 0x08
> +#define ADS7924_DATA3_L_REG 0x09
> +#define ADS7924_ULR0_REG 0x0A
> +#define ADS7924_LLR0_REG 0x0B
> +#define ADS7924_ULR1_REG 0x0C
> +#define ADS7924_LLR1_REG 0x0D
> +#define ADS7924_ULR2_REG 0x0E
> +#define ADS7924_LLR2_REG 0x0F
> +#define ADS7924_ULR3_REG 0x10
> +#define ADS7924_LLR3_REG 0x11
> +#define ADS7924_INTCONFIG_REG 0x12
> +#define ADS7924_SLPCONFIG_REG 0x13
> +#define ADS7924_ACQCONFIG_REG 0x14
> +#define ADS7924_PWRCONFIG_REG 0x15
> +#define ADS7924_RESET_REG 0x16
> +
> +/*
> + * Register address INC bit: when set to '1', the register address is
> + * automatically incremented after every register read which allows convenient
> + * reading of multiple registers. Set INC to '0' when reading a single register.
> + */
> +#define ADS7924_AUTO_INCREMENT_BIT BIT(7)
> +
> +#define ADS7924_MODECNTRL_MODE_MASK GENMASK(7, 2)
> +
> +#define ADS7924_MODECNTRL_SEL_MASK GENMASK(1, 0)
> +
> +#define ADS7924_CFG_INTPOL_BIT 1
> +#define ADS7924_CFG_INTTRIG_BIT 0
> +
> +#define ADS7924_CFG_INTPOL_MASK BIT(ADS7924_CFG_INTPOL_BIT)
> +#define ADS7924_CFG_INTTRIG_MASK BIT(ADS7924_CFG_INTTRIG_BIT)
> +
> +/* Interrupt pin polarity */
> +#define ADS7924_CFG_INTPOL_LOW 0
> +#define ADS7924_CFG_INTPOL_HIGH 1
> +
> +/* Interrupt pin signaling */
> +#define ADS7924_CFG_INTTRIG_LEVEL 0
> +#define ADS7924_CFG_INTTRIG_EDGE 1
> +
> +/* Mode control values */
> +#define ADS7924_MODECNTRL_IDLE 0x00
> +#define ADS7924_MODECNTRL_AWAKE 0x20
> +#define ADS7924_MODECNTRL_MANUAL_SINGLE 0x30
> +#define ADS7924_MODECNTRL_MANUAL_SCAN 0x32
> +#define ADS7924_MODECNTRL_AUTO_SINGLE 0x31
> +#define ADS7924_MODECNTRL_AUTO_SCAN 0x33
> +#define ADS7924_MODECNTRL_AUTO_SINGLE_SLEEP 0x39
> +#define ADS7924_MODECNTRL_AUTO_SCAN_SLEEP 0x3B
> +#define ADS7924_MODECNTRL_AUTO_BURST_SLEEP 0x3F
> +
> +#define ADS7924_ACQTIME_MASK GENMASK(4, 0)
> +
> +#define ADS7924_PWRUPTIME_MASK GENMASK(4, 0)
> +
> +/*
> + * The power-up time is allowed to elapse whenever the device has been shutdown
> + * in idle mode. Power-up time can allow external circuits, such as an
> + * operational amplifier, between the MUXOUT and ADCIN pins to turn on.
> + * The nominal time programmed by the PUTIME[4:0] register bits is given by:
> + * t PU = PWRUPTIME[4:0] × 2 μs
> + * If a power-up time is not required, set the bits to '0' to effectively bypass.
> + */
> +#define ADS7924_PWRUPTIME_US 0 /* Bypass (0us). */
> +
> +/*
> + * Acquisition Time according to ACQTIME[4:0] register bits.
> + * The Acquisition Time is given by:
> + * t ACQ = (ACQTIME[4:0] × 2 μs) + 6 μs
> + * Using default value of 0 for ACQTIME[4:0] results in a minimum acquisition
> + * time of 6us.
> + */
> +#define ADS7924_ACQTIME_US 6
> +
> +/* The conversion time is always 4μs and cannot be programmed by the user. */
> +#define ADS7924_CONVTIME_US 4
> +
> +#define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
> + ADS7924_CONVTIME_US)
> +
> +#define ADS7924_SLEEP_DELAY_MS 2000
> +
> +#define ADS7924_V_CHAN(_chan, _addr) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = _chan, \
> + .address = _addr, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .datasheet_name = "AIN"#_chan, \
> +}
> +
> +struct ads7924_data {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regulator *vref_reg;
> +
> + /* GPIO descriptor for device hard-reset pin. */
> + struct gpio_desc *reset_gpio;
> +
> + /*
> + * Protects ADC ops, e.g: concurrent sysfs/buffered
> + * data reads, configuration updates
> + */
> + struct mutex lock;
> +
> + /*
> + * Set to true when the ADC is switched to the continuous-conversion
> + * mode and exits from a power-down state. This flag is used to avoid
> + * getting the stale result from the conversion register.
> + */
> + bool conv_invalid;
> +};
> +
> +static bool ads7924_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case ADS7924_MODECNTRL_REG:
> + case ADS7924_INTCNTRL_REG:
> + case ADS7924_ULR0_REG:
> + case ADS7924_LLR0_REG:
> + case ADS7924_ULR1_REG:
> + case ADS7924_LLR1_REG:
> + case ADS7924_ULR2_REG:
> + case ADS7924_LLR2_REG:
> + case ADS7924_ULR3_REG:
> + case ADS7924_LLR3_REG:
> + case ADS7924_INTCONFIG_REG:
> + case ADS7924_SLPCONFIG_REG:
> + case ADS7924_ACQCONFIG_REG:
> + case ADS7924_PWRCONFIG_REG:
> + case ADS7924_RESET_REG:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static const struct regmap_config ads7924_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ADS7924_RESET_REG,
> + .writeable_reg = ads7924_is_writeable_reg,
> +};
> +
> +static const struct iio_chan_spec ads7924_channels[] = {
> + ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
> + ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
> + ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
> + ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
> +};
> +
> +static
> +int ads7924_get_adc_result(struct ads7924_data *data,
> + struct iio_chan_spec const *chan, int *val)
> +{
> + int ret;
> + __be16 be_val;
> +
> + if (chan->channel < 0 || chan->channel >= ADS7924_CHANNELS)
> + return -EINVAL;
> +
> + if (data->conv_invalid) {
> + int conv_time;
> +
> + conv_time = ADS7924_TOTAL_CONVTIME_US;
> + /* Allow 10% for internal clock inaccuracy. */
> + conv_time += conv_time / 10;
> + usleep_range(conv_time, conv_time + 1);
> + data->conv_invalid = false;
> + }
> +
> + ret = regmap_raw_read(data->regmap, ADS7924_AUTO_INCREMENT_BIT |
> + chan->address, &be_val, sizeof(be_val));
> + if (ret)
> + return ret;
> +
> + *val = be16_to_cpu(be_val);
> + *val = *val >> ADS7924_DATA_SHIFT;
> +
> + return ret;
> +}
> +
> +static int ads7924_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + int ret, vref_uv;
> + struct ads7924_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW: {
> + mutex_lock(&data->lock);
> + ret = ads7924_get_adc_result(data, chan, val);
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + dev_dbg(data->dev, "%s error1\n", __func__);
> + else
> + ret = IIO_VAL_INT;
> + break;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + vref_uv = regulator_get_voltage(data->vref_reg);
> + if (vref_uv < 0) {
> + /* dummy regulator "get_voltage" returns -EINVAL */
> + ret = -EINVAL;
> + } else {
> + *val = vref_uv / 1000; /* Convert reg voltage to mV */
> + *val2 = ADS7924_BITS;
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info ads7924_info = {
> + .read_raw = ads7924_read_raw,
> +};
> +
> +static int ads7924_get_channels_config(struct i2c_client *client,
> + struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *priv = iio_priv(indio_dev);
> + struct device *dev = priv->dev;
> + struct fwnode_handle *node;
> + int num_channels = 0;
> +
> + device_for_each_child_node(dev, node) {
> + u32 pval;
> + unsigned int channel;
> +
> + if (fwnode_property_read_u32(node, "reg", &pval)) {
> + dev_err(dev, "invalid reg on %pfw\n", node);
> + continue;
> + }
> +
> + channel = pval;
> + if (channel >= ADS7924_CHANNELS) {
> + dev_err(dev, "invalid channel index %d on %pfw\n",
> + channel, node);
> + continue;
> + }
> +
> + num_channels++;
> + }
> +
> + if (num_channels > 0) {
> + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> + return 0;
> + } else {
> + return -EINVAL;
> + }
> +}
> +
> +static int ads7924_set_conv_mode(struct ads7924_data *data, int mode)
> +{
> + int ret;
> + unsigned int mode_field;
> + struct device *dev = data->dev;
> +
> + /*
> + * When switching between modes, be sure to first select the Awake mode
> + * and then switch to the desired mode. This procedure ensures the
> + * internal control logic is properly synchronized.
> + */
> + if (mode != ADS7924_MODECNTRL_IDLE) {
> + mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK,
> + ADS7924_MODECNTRL_AWAKE);
> +
> + ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
> + ADS7924_MODECNTRL_MODE_MASK,
> + mode_field);
> + if (ret) {
> + dev_warn(dev, "failed to set awake mode (%pe)\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> + }
> +
> + mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK, mode);
> +
> + ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
> + ADS7924_MODECNTRL_MODE_MASK, mode_field);
> + if (ret)
> + dev_warn(dev, "failed to set mode %d (%pe)\n", mode,
> + ERR_PTR(ret));
> +
> + return ret;
> +}
> +
> +static int ads7924_reset(struct iio_dev *indio_dev)
> +{
> + struct ads7924_data *data = iio_priv(indio_dev);
> +
> + if (data->reset_gpio) {
> + gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> + /* Educated guess: assert time not specified in datasheet... */
> + mdelay(100);
> + gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
> + } else {
> + int ret;
> +
> + /*
> + * A write of 10101010 to this register will generate a
> + * software reset of the ADS7924.
> + */
> + ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +};
> +
> +static void ads7924_reg_disable(void *data)
> +{
> + regulator_disable(data);
> +}
> +
> +static void ads7924_set_idle_mode(void *data)
> +{
> + ads7924_set_conv_mode(data, ADS7924_MODECNTRL_IDLE);
> +}
> +
> +static int ads7924_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct ads7924_data *data;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;

Would it be a good idea to also use dev_err_probe() here?

Hugo.

> +
> + data = iio_priv(indio_dev);
> +
> + data->dev = dev;
> +
> + /* Initialize the reset GPIO as output with an initial value of 0. */
> + data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(data->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
> + "failed to get request reset GPIO\n");
> +
> + mutex_init(&data->lock);
> +
> + indio_dev->name = "ads7924";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + indio_dev->channels = ads7924_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
> + indio_dev->info = &ads7924_info;
> +
> + ret = ads7924_get_channels_config(client, indio_dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to get channels configuration\n");
> +
> + data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(dev, PTR_ERR(data->regmap),
> + "failed to init regmap\n");
> +
> + data->vref_reg = devm_regulator_get(dev, "vref");
> + if (IS_ERR(data->vref_reg))
> + return dev_err_probe(dev, PTR_ERR(data->vref_reg),
> + "failed to get vref regulator\n");
> +
> + ret = regulator_enable(data->vref_reg);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to enable regulator\n");
> +
> + ret = devm_add_action_or_reset(dev, ads7924_reg_disable, data->vref_reg);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to add regulator disable action\n");
> +
> + ret = ads7924_reset(indio_dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to reset device\n");
> +
> + ret = devm_add_action_or_reset(dev, ads7924_set_idle_mode, data);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to add idle mode action\n");
> +
> + ret = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_AUTO_SCAN);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to set conversion mode\n");
> +
> + /* Use minimum signal acquire time. */
> + ret = regmap_update_bits(data->regmap, ADS7924_ACQCONFIG_REG,
> + ADS7924_ACQTIME_MASK,
> + FIELD_PREP(ADS7924_ACQTIME_MASK, 0));
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to configure signal acquire time\n");
> +
> + /* Disable power-up time. */
> + ret = regmap_update_bits(data->regmap, ADS7924_PWRCONFIG_REG,
> + ADS7924_PWRUPTIME_MASK,
> + FIELD_PREP(ADS7924_PWRUPTIME_MASK, 0));
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to configure power-up time\n");
> +
> + data->conv_invalid = true;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to register IIO device\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id ads7924_id[] = {
> + { "ads7924", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ads7924_id);
> +
> +static const struct of_device_id ads7924_of_match[] = {
> + { .compatible = "ti,ads7924", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ads7924_of_match);
> +
> +static struct i2c_driver ads7924_driver = {
> + .driver = {
> + .name = "ads7924",
> + .of_match_table = ads7924_of_match,
> + },
> + .probe_new = ads7924_probe,
> + .id_table = ads7924_id,
> +};
> +
> +module_i2c_driver(ads7924_driver);
> +
> +MODULE_AUTHOR("Hugo Villeneuve <[email protected]>");
> +MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver");
> +MODULE_LICENSE("GPL");
> --
> 2.30.2
>
>

--
Hugo Villeneuve <[email protected]>

2023-01-12 19:57:26

by Hugo Villeneuve

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

2023-01-12 20:53:06

by Hugo Villeneuve

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

On Thu, 12 Jan 2023 19:00:30 +0000
Jonathan Cameron <[email protected]> wrote:

> On Tue, 10 Jan 2023 11:01:23 -0500
> Hugo Villeneuve <[email protected]> wrote:
>
> > From: Hugo Villeneuve <[email protected]>
> >
> > The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> > digital converter (ADC) with an I2C interface.
> >
> > Datasheet: https://www.ti.com/lit/gpn/ads7924
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
>
> Hi Hugo,
>
> A few minor additional comments from me as you are doing a v3
> (I might just have tidied them up whilst applying otherwise to save
> us all time!)

No problem, I will add them to my v3.

> Thanks,
>
> Jonathan
>
> > diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> > new file mode 100644
> > index 000000000000..8384f3451236
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads7924.c
> > @@ -0,0 +1,492 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C
> > + *
> > + * iio/adc/ti-ads7924.c
>
> Not a huge amount of point in having a file name in the file. I don't mind
> if you really want it though.

Removed.

>
> > + * Author: Hugo Villeneuve <[email protected]>
> > + * Copyright 2022 DimOnOff
> > + *
> > + * based on iio/adc/ti-ads1015.c
> > + * Copyright (c) 2016, Intel Corporation.
> > + *
> > + * Datasheet: https://www.ti.com/lit/gpn/ads7924
> > + */
>
> > +
> > +#define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
> > + ADS7924_CONVTIME_US)
> > +
> > +#define ADS7924_SLEEP_DELAY_MS 2000
>
> Unused, so drop for now.

Removed.

> > +static bool ads7924_is_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > + switch (reg) {
> > + case ADS7924_MODECNTRL_REG:
> > + case ADS7924_INTCNTRL_REG:
> > + case ADS7924_ULR0_REG:
> > + case ADS7924_LLR0_REG:
> > + case ADS7924_ULR1_REG:
> > + case ADS7924_LLR1_REG:
> > + case ADS7924_ULR2_REG:
> > + case ADS7924_LLR2_REG:
> > + case ADS7924_ULR3_REG:
> > + case ADS7924_LLR3_REG:
> > + case ADS7924_INTCONFIG_REG:
> > + case ADS7924_SLPCONFIG_REG:
> > + case ADS7924_ACQCONFIG_REG:
> > + case ADS7924_PWRCONFIG_REG:
> > + case ADS7924_RESET_REG:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static const struct regmap_config ads7924_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = ADS7924_RESET_REG,
> > + .writeable_reg = ads7924_is_writeable_reg,
> > +};
> > +
> > +static const struct iio_chan_spec ads7924_channels[] = {
> > + ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
> > + ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
> > + ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
> > + ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
> > +};
> > +
> > +static
> > +int ads7924_get_adc_result(struct ads7924_data *data,
>
> Drop the unusual line break as not needed (probably refactoring left over!)
> static int ...

Done.

> > + struct iio_chan_spec const *chan, int *val)
> > +{
> > + int ret;
> > + __be16 be_val;
> > +
> > + if (chan->channel < 0 || chan->channel >= ADS7924_CHANNELS)
> > + return -EINVAL;
> > +
> > + if (data->conv_invalid) {
> > + int conv_time;
> > +
> > + conv_time = ADS7924_TOTAL_CONVTIME_US;
> > + /* Allow 10% for internal clock inaccuracy. */
> > + conv_time += conv_time / 10;
> > + usleep_range(conv_time, conv_time + 1);
> > + data->conv_invalid = false;
> > + }
> > +
> > + ret = regmap_raw_read(data->regmap, ADS7924_AUTO_INCREMENT_BIT |
> > + chan->address, &be_val, sizeof(be_val));
> > + if (ret)
> > + return ret;
> > +
> > + *val = be16_to_cpu(be_val);
> > + *val = *val >> ADS7924_DATA_SHIFT;
> *val = be16_to_cpu(be_val) >> ADS7924_DATA_SHIFT;
>
> return 0;

Yes, simpler and clearer. Done

>
> > +
> > + return ret;
> > +}
> > +
> > +static int ads7924_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long mask)
> > +{
> > + int ret, vref_uv;
> > + struct ads7924_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW: {
> > + mutex_lock(&data->lock);
> > + ret = ads7924_get_adc_result(data, chan, val);
> > + mutex_unlock(&data->lock);
> > + if (ret < 0)
> > + dev_dbg(data->dev, "%s error1\n", __func__);
> > + else
> > + ret = IIO_VAL_INT;
> if (ret < 0) {
> ...
> return ret;
> }
>
> return IIO_VAL_INT;
>

Done. I also have removed the not-very-informative "error1" message...

> > + break;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + vref_uv = regulator_get_voltage(data->vref_reg);
> > + if (vref_uv < 0) {
> > + /* dummy regulator "get_voltage" returns -EINVAL */
> > + ret = -EINVAL;
> return -EINVAL;
> > + } else {
> > + *val = vref_uv / 1000; /* Convert reg voltage to mV */
> > + *val2 = ADS7924_BITS;
> > + ret = IIO_VAL_FRACTIONAL_LOG2;
> return IIO_VAL_FR...
>
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> return -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}

Done. With these changes, I propose to also remove last "return ret" (like in rcar-gyroadc.c). Then, maybe also remove break statements?

> > +
> > +static const struct iio_info ads7924_info = {
> > + .read_raw = ads7924_read_raw,
> The tabbed indent doesn't add anything so
> .read_raw = ads...
>
> Aligning things is often a bad idea anyway as it just adds noise when inevitably
> the alignment needs increasing... Definitely not worth doing for one item.

Ok, done.

> > +};
> > +
> > +static int ads7924_get_channels_config(struct i2c_client *client,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct ads7924_data *priv = iio_priv(indio_dev);
> > + struct device *dev = priv->dev;
> > + struct fwnode_handle *node;
> > + int num_channels = 0;
> > +
> > + device_for_each_child_node(dev, node) {
> > + u32 pval;
> > + unsigned int channel;
> > +
> > + if (fwnode_property_read_u32(node, "reg", &pval)) {
> > + dev_err(dev, "invalid reg on %pfw\n", node);
> > + continue;
> > + }
> > +
> > + channel = pval;
> > + if (channel >= ADS7924_CHANNELS) {
> > + dev_err(dev, "invalid channel index %d on %pfw\n",
> > + channel, node);
> > + continue;
> > + }
> > +
> > + num_channels++;
> > + }
> > +
> > + if (num_channels > 0) {
> > + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> > + return 0;
> > + } else {
>
> As per other review. Give us what we expect which is error paths
> as out of line.

Already done as suggested by Christophe:

if (num_channels <= 0)
return -EINVAL;

dev_dbg(dev, "found %d ADC channels\n", num_channels);
return 0;

Although I do not fully understand what you mean by "...error paths as out of line"? Do you mean to drop the debug message?

>
> > + return -EINVAL;
> > + }
> > +}
>
> > +static int ads7924_reset(struct iio_dev *indio_dev)
> > +{
> > + struct ads7924_data *data = iio_priv(indio_dev);
> > +
> > + if (data->reset_gpio) {
> > + gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> > + /* Educated guess: assert time not specified in datasheet... */
> > + mdelay(100);
> > + gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
>
> No real point in shared exit path. Early returns mean less code to look at
> when following particular flows which is always nice.

Agreed, done.

> return 0;
>
> > + } else {
>
> if returning above, no need to have the else {

Done

>
> > + int ret;
> > +
> > + /*
> > + * A write of 10101010 to this register will generate a
> > + * software reset of the ADS7924.
> > + */
> > + ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> > + if (ret)
> > + return ret;
> return regmap_write(....)
>
> > + }
> > +
> > + return 0;
> and drop this.

Done. also remove now unused variable "ret".

> > +};
>
> > +static int ads7924_probe(struct i2c_client *client)
> > +{
>
> ...
>
> > + ret = devm_add_action_or_reset(dev, ads7924_set_idle_mode, data);
>
> The _or_reset() part of this is there so that if this function fails
> it will call the callback before returning an error code. As a result
> you will turn the device, before you've ever turned it on.
>
> Register the callback after turning it on, so ...

Makes sense, done.

> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to add idle mode action\n");
> > +
> > + ret = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_AUTO_SCAN);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to set conversion mode\n");
>
> ... here.

Thank you a lot for all your comments and meaningfull explanations!

--
Hugo Villeneuve <[email protected]>

2023-01-13 14:52:13

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

> > > + break;
> > > + }
> > > + case IIO_CHAN_INFO_SCALE:
> > > + vref_uv = regulator_get_voltage(data->vref_reg);
> > > + if (vref_uv < 0) {
> > > + /* dummy regulator "get_voltage" returns -EINVAL */
> > > + ret = -EINVAL;
> > return -EINVAL;
> > > + } else {
> > > + *val = vref_uv / 1000; /* Convert reg voltage to mV */
> > > + *val2 = ADS7924_BITS;
> > > + ret = IIO_VAL_FRACTIONAL_LOG2;
> > return IIO_VAL_FR...
> >
> > > + }
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > return -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + return ret;
> > > +}
>
> Done. With these changes, I propose to also remove last "return ret" (like in rcar-gyroadc.c). Then, maybe also remove break statements?

Definitely to both. I was just being lazy whilst commenting ;) No breaks after returns, and as you
have noted, the last return ret is unreachable.

> > > +
> > > + if (num_channels > 0) {
> > > + dev_dbg(dev, "found %d ADC channels\n", num_channels);
> > > + return 0;
> > > + } else {
> >
> > As per other review. Give us what we expect which is error paths
> > as out of line.
>
> Already done as suggested by Christophe:
>
> if (num_channels <= 0)
> return -EINVAL;

This is an out of line error path because it's indented more than the normal
flow.

>
> dev_dbg(dev, "found %d ADC channels\n", num_channels);
> return 0;
>
> Although I do not fully understand what you mean by "...error paths as out of line"? Do you mean to drop the debug message?
See above.

>
> >
> > > + return -EINVAL;
> > > + }
> > > +}