[PATCH v3 0/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

[PATCH v3 0/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hello,
this patch series adds the driver for the Texas Instruments ADS7924.

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

Patch 1 contains the driver for the ADS7924.

Patch 2 add the dt-bindings for the ADS7924.

I have tested the driver using the Texas Instruments ADS7924EVM board connected
to a Variscite Symphony EVK with a IMX8MN NANO SOM:
  - Tested reset pin Ok
  - Tested regulator setup Ok
  - Tested reading sysfs in_voltage_scale Ok
  - Tested reading sysfs in_voltageX_raw (x=0 to 3) Ok

Thank you.

Link: [v1] https://lore.kernel.org/linux-iio/20221222203610.2571287-1-hugo@hugovil.com/
Link: [v2] https://lore.kernel.org/linux-iio/20230110160124.3853593-1-hugo@hugovil.com/

Changes for V3:
- Rebase on linux-6.2-rc1
- Refactor to follow usual coding practices as per review comments.
- Fix missing include (reported by kernel test robot).
- Fix DT bindings errors.
- Removed unused define.
- Remove breaks after return.
- Remove debug message in ads7924_get_channels_config().
- Remove braces around swich case IIO_CHAN_INFO_RAW.
- Remove comments about regulator_get_voltage() failing for dummy regulator.

Changes for V2:
- Dropped patch "iio: adc: Kconfig: add SPI interface mention to AD7924
  description"
- Fixed comments style
- Removed unused defines/variables/etc related to buffered support (no buffered
  support for the moment).
- Convert of-specific code to use the generic firmware property accessors in
  include/linux/property.h.
- Use FIELD_GET / FIELD_PREP for bit operations/defines
- Simplified conversion result registers definitions/usage.
- Now using mutex lock/unlock only for INFO_RAW switch branch
- Use dev_err_probe() in all return paths of ads7924_probe()
- Removed ads7924_remove() after adding callbacks with
  devm_add_action_or_reset().
- Change iio_device_register() to devm_iio_device_register().
- Add the legacy i2c_device_id table
- DT bindings: reorder entries, fix indentation,improve comments, add interrupt
  line

Hugo Villeneuve (2):
  iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver
  dt-bindings: iio: adc: add Texas Instruments ADS7924

 .../bindings/iio/adc/ti,ads7924.yaml          | 112 +++++
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads7924.c                  | 474 ++++++++++++++++++
 5 files changed, 605 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
 create mode 100644 drivers/iio/adc/ti-ads7924.c


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.30.2

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

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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 <hvilleneuve@dimonoff.com>
---
 MAINTAINERS                  |   7 +
 drivers/iio/adc/Kconfig      |  11 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads7924.c | 474 +++++++++++++++++++++++++++++++++++
 4 files changed, 493 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7924.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f61eb221415b..1a22d2124ad5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20774,6 +20774,13 @@ M:	Robert Richter <rric@kernel.org>
 S:	Odd Fixes
 F:	drivers/gpio/gpio-thunderx.c
 
+TI ADS7924 ADC DRIVER
+M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
+L:	linux-iio@vger.kernel.org
+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" <prabhakar.csengg@gmail.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 63f80d747cbd..5bf4bf16df80 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1207,6 +1207,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 4ef41a7dfac6..6b2a2b4ad9ce 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..c24fae4ef8e0
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Texas Instruments ADS7924 ADC, 12-bit, 4-Channels, I2C
+ *
+ * Author: Hugo Villeneuve <hvilleneuve@dimonoff.com>
+ * 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/bitfield.h>
+#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_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) >> ADS7924_DATA_SHIFT;
+
+	return 0;
+}
+
+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)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		vref_uv = regulator_get_voltage(data->vref_reg);
+		if (vref_uv < 0)
+			return -EINVAL;
+
+		*val =  vref_uv / 1000; /* Convert reg voltage to mV */
+		*val2 = ADS7924_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+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)
+		return -EINVAL;
+
+	return 0;
+}
+
+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. */
+		return 0;
+	}
+
+	/*
+	 * A write of 10101010 to this register will generate a
+	 * software reset of the ADS7924.
+	 */
+	return regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
+};
+
+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 dev_err_probe(dev, -ENOMEM,
+				     "failed to allocate iio device\n");
+
+	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 = ads7924_set_conv_mode(data, ADS7924_MODECNTRL_AUTO_SCAN);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to set conversion mode\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");
+
+	/* 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 <hvilleneuve@dimonoff.com>");
+MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

[PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add device tree bindings document for the Texas Instruments ADS7924
ADC.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 .../bindings/iio/adc/ti,ads7924.yaml          | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
new file mode 100644
index 000000000000..24bbf95383b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads7924.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI ADS7924 4 channels 12 bits I2C analog to digital converter
+
+maintainers:
+  - Hugo Villeneuve <hvilleneuve@dimonoff.com>
+
+description: |
+  Texas Instruments ADS7924 4 channels 12 bits I2C analog to digital converter
+
+  Specifications:
+    https://www.ti.com/lit/gpn/ads7924
+
+properties:
+  compatible:
+    const: ti,ads7924
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      The regulator supply for the ADC reference voltage (AVDD)
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "#io-channel-cells":
+    const: 1
+
+patternProperties:
+  "^channel@[0-3]+$":
+    $ref: adc.yaml
+
+    description: |
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. It can have up to 4 channels numbered from 0 to 3.
+        items:
+          - minimum: 0
+            maximum: 3
+
+      label:
+        description: |
+          Unique name to identify the channel.
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vref-supply
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@48 {
+            compatible = "ti,ads7924";
+            reg = <0x48>;
+            vref-supply = <&ads7924_reg>;
+            reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-parent = <&gpio>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            channel@0 {
+                reg = <0>;
+                label = "CH0";
+            };
+            channel@1 {
+                reg = <1>;
+                label = "CH1";
+            };
+            channel@2 {
+                reg = <2>;
+                label = "CH2";
+            };
+            channel@3 {
+                reg = <3>;
+                label = "CH3";
+            };
+        };
+    };
+...
-- 
2.30.2

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

[Jonathan Cameron]

On Fri, 13 Jan 2023 14:49:58 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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

This counts as a normal tag, so there shouldn't be blank line between
it and the SOB.

A few other small things inline noticed on this read through.
I can fix these up whilst applying if nothing else comes up for v3
and DT binding reviewers are happy.  If you are doing a v4 for
other reasons, please address these comments in that.

Jonathan



> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
...
> diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> new file mode 100644
> index 000000000000..c24fae4ef8e0
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7924.c
> @@ -0,0 +1,474 @@

...

> +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)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		vref_uv = regulator_get_voltage(data->vref_reg);
> +		if (vref_uv < 0)
> +			return -EINVAL;

Better to return the error value from regulator_get_voltage() rather
than replace it with -EINVAL.

> +
> +		*val =  vref_uv / 1000; /* Convert reg voltage to mV */
> +		*val2 = ADS7924_BITS;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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)

How would it be less than 0?  if (!num_channels) works fine I think.

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +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));

As below.

> +			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));

Why warning? Seems like a fairly critical error to me.
dev_err() more appropriate perhaps.

> +
> +	return ret;
> +}
> +

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

[Hugo Villeneuve]

On Sat, 14 Jan 2023 16:28:41 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 13 Jan 2023 14:49:58 -0500
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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
> 
> This counts as a normal tag, so there shouldn't be blank line between
> it and the SOB.

Fixed.

> 
> A few other small things inline noticed on this read through.
> I can fix these up whilst applying if nothing else comes up for v3
> and DT binding reviewers are happy.  If you are doing a v4 for
> other reasons, please address these comments in that.

Ok,
I have already prepared a V4 with all these changes, just in case. Thank you for your comments.


> Jonathan
> 
> 
> 
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ...
> > diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> > new file mode 100644
> > index 000000000000..c24fae4ef8e0
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads7924.c
> > @@ -0,0 +1,474 @@
> 
> ...
> 
> > +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)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		vref_uv = regulator_get_voltage(data->vref_reg);
> > +		if (vref_uv < 0)
> > +			return -EINVAL;
> 
> Better to return the error value from regulator_get_voltage() rather
> than replace it with -EINVAL.

Of course, done.

> > +
> > +		*val =  vref_uv / 1000; /* Convert reg voltage to mV */
> > +		*val2 = ADS7924_BITS;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +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)
> 
> How would it be less than 0?  if (!num_channels) works fine I think.

Done.

> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +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));
> 
> As below.

Agreed, converted to dev_err.

> 
> > +			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));
> 
> Why warning? Seems like a fairly critical error to me.
> dev_err() more appropriate perhaps.

Agreed, converted to dev_err.

> > +
> > +	return ret;
> > +}
> > +
> 
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Krzysztof Kozlowski]

On 13/01/2023 20:49, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Add device tree bindings document for the Texas Instruments ADS7924
> ADC.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  .../bindings/iio/adc/ti,ads7924.yaml          | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> new file mode 100644
> index 000000000000..24bbf95383b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads7924.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI ADS7924 4 channels 12 bits I2C analog to digital converter
> +
> +maintainers:
> +  - Hugo Villeneuve <hvilleneuve@dimonoff.com>
> +
> +description: |
> +  Texas Instruments ADS7924 4 channels 12 bits I2C analog to digital converter
> +
> +  Specifications:
> +    https://www.ti.com/lit/gpn/ads7924
> +
> +properties:
> +  compatible:
> +    const: ti,ads7924
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      The regulator supply for the ADC reference voltage (AVDD)
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +patternProperties:
> +  "^channel@[0-3]+$":
> +    $ref: adc.yaml
> +
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 4 channels numbered from 0 to 3.
> +        items:
> +          - minimum: 0
> +            maximum: 3
> +
> +      label:
> +        description: |
> +          Unique name to identify the channel.

Drop description, it's coming from adc.yaml. Just "label: true"

> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false

You are not allowing anything else from adc.yaml. Is it on purpose?

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - vref-supply
> +  - "#address-cells"
> +  - "#size-cells"
> +

Best regards,
Krzysztof

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Hugo Villeneuve]

On Sun, 15 Jan 2023 15:57:21 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 13/01/2023 20:49, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Add device tree bindings document for the Texas Instruments ADS7924
> > ADC.
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  .../bindings/iio/adc/ti,ads7924.yaml          | 112 ++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > new file mode 100644
> > index 000000000000..24bbf95383b4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > @@ -0,0 +1,112 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/ti,ads7924.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI ADS7924 4 channels 12 bits I2C analog to digital converter
> > +
> > +maintainers:
> > +  - Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > +
> > +description: |
> > +  Texas Instruments ADS7924 4 channels 12 bits I2C analog to digital converter
> > +
> > +  Specifications:
> > +    https://www.ti.com/lit/gpn/ads7924
> > +
> > +properties:
> > +  compatible:
> > +    const: ti,ads7924
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vref-supply:
> > +    description:
> > +      The regulator supply for the ADC reference voltage (AVDD)
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +patternProperties:
> > +  "^channel@[0-3]+$":
> > +    $ref: adc.yaml
> > +
> > +    description: |
> > +      Represents the external channels which are connected to the ADC.
> > +
> > +    properties:
> > +      reg:
> > +        description: |
> > +          The channel number. It can have up to 4 channels numbered from 0 to 3.
> > +        items:
> > +          - minimum: 0
> > +            maximum: 3
> > +
> > +      label:
> > +        description: |
> > +          Unique name to identify the channel.
> 
> Drop description, it's coming from adc.yaml. Just "label: true"

Done.

> > +
> > +    required:
> > +      - reg
> > +
> > +    additionalProperties: false
> 
> You are not allowing anything else from adc.yaml. Is it on purpose?

I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)

But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:

  bipolar
  diff-channels
  settling-time-us
  oversampling-ratio

So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?

Thank you,
Hugo.


> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vref-supply
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> 
> Best regards,
> Krzysztof
> 
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Hugo Villeneuve]

On Sun, 15 Jan 2023 16:43:26 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 15 Jan 2023 11:22:05 -0500
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> > On Sun, 15 Jan 2023 15:57:21 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> > > On 13/01/2023 20:49, Hugo Villeneuve wrote:  
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > Add device tree bindings document for the Texas Instruments ADS7924
> > > > ADC.
> > > > 
> > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > ---
> > > >  .../bindings/iio/adc/ti,ads7924.yaml          | 112 ++++++++++++++++++
> > > >  1 file changed, 112 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > > new file mode 100644
> > > > index 000000000000..24bbf95383b4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > > @@ -0,0 +1,112 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/adc/ti,ads7924.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: TI ADS7924 4 channels 12 bits I2C analog to digital converter
> > > > +
> > > > +maintainers:
> > > > +  - Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > +
> > > > +description: |
> > > > +  Texas Instruments ADS7924 4 channels 12 bits I2C analog to digital converter
> > > > +
> > > > +  Specifications:
> > > > +    https://www.ti.com/lit/gpn/ads7924
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: ti,ads7924
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  vref-supply:
> > > > +    description:
> > > > +      The regulator supply for the ADC reference voltage (AVDD)
> > > > +
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +  "#io-channel-cells":
> > > > +    const: 1
> > > > +
> > > > +patternProperties:
> > > > +  "^channel@[0-3]+$":
> > > > +    $ref: adc.yaml
> > > > +
> > > > +    description: |
> > > > +      Represents the external channels which are connected to the ADC.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description: |
> > > > +          The channel number. It can have up to 4 channels numbered from 0 to 3.
> > > > +        items:
> > > > +          - minimum: 0
> > > > +            maximum: 3
> > > > +
> > > > +      label:
> > > > +        description: |
> > > > +          Unique name to identify the channel.  
> > > 
> > > Drop description, it's coming from adc.yaml. Just "label: true"  
> > 
> > Done.
> > 
> > > > +
> > > > +    required:
> > > > +      - reg
> > > > +
> > > > +    additionalProperties: false  
> > > 
> > > You are not allowing anything else from adc.yaml. Is it on purpose?  
> > 
> > I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)
> > 
> > But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:
> > 
> >   bipolar
> >   diff-channels
> >   settling-time-us
> >   oversampling-ratio
> > 
> > So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?
> 
> Makes sense to me.  Whilst there are lots of things a channel can support, most
> of them are hardware related and not universal.

Ok, I think I am finally beginning to see the light here :)

So I will then leave "additionalProperties: false".

I will send a V4 soon with all the latest changes.

Thank you,
Hugo.


> Jonathan
> 
> > 
> > Thank you,
> > Hugo.
> > 
> > 
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - vref-supply
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +  
> > > 
> > > Best regards,
> > > Krzysztof
> > > 
> > >   
> > 
> > 
> 
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Jonathan Cameron]

On Sun, 15 Jan 2023 11:22:05 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Sun, 15 Jan 2023 15:57:21 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 13/01/2023 20:49, Hugo Villeneuve wrote:  
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > Add device tree bindings document for the Texas Instruments ADS7924
> > > ADC.
> > > 
> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > ---
> > >  .../bindings/iio/adc/ti,ads7924.yaml          | 112 ++++++++++++++++++
> > >  1 file changed, 112 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > new file mode 100644
> > > index 000000000000..24bbf95383b4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
> > > @@ -0,0 +1,112 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/ti,ads7924.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI ADS7924 4 channels 12 bits I2C analog to digital converter
> > > +
> > > +maintainers:
> > > +  - Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > +
> > > +description: |
> > > +  Texas Instruments ADS7924 4 channels 12 bits I2C analog to digital converter
> > > +
> > > +  Specifications:
> > > +    https://www.ti.com/lit/gpn/ads7924
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ti,ads7924
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vref-supply:
> > > +    description:
> > > +      The regulator supply for the ADC reference voltage (AVDD)
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +  "#io-channel-cells":
> > > +    const: 1
> > > +
> > > +patternProperties:
> > > +  "^channel@[0-3]+$":
> > > +    $ref: adc.yaml
> > > +
> > > +    description: |
> > > +      Represents the external channels which are connected to the ADC.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +          The channel number. It can have up to 4 channels numbered from 0 to 3.
> > > +        items:
> > > +          - minimum: 0
> > > +            maximum: 3
> > > +
> > > +      label:
> > > +        description: |
> > > +          Unique name to identify the channel.  
> > 
> > Drop description, it's coming from adc.yaml. Just "label: true"  
> 
> Done.
> 
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +    additionalProperties: false  
> > 
> > You are not allowing anything else from adc.yaml. Is it on purpose?  
> 
> I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)
> 
> But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:
> 
>   bipolar
>   diff-channels
>   settling-time-us
>   oversampling-ratio
> 
> So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?

Makes sense to me.  Whilst there are lots of things a channel can support, most
of them are hardware related and not universal.

Jonathan

> 
> Thank you,
> Hugo.
> 
> 
> > > +
> > > +additionalProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - vref-supply
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +  
> > 
> > Best regards,
> > Krzysztof
> > 
> >   
> 
> 

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Krzysztof Kozlowski]

On 15/01/2023 17:32, Hugo Villeneuve wrote:
>>>>> +    required:
>>>>> +      - reg
>>>>> +
>>>>> +    additionalProperties: false  
>>>>
>>>> You are not allowing anything else from adc.yaml. Is it on purpose?  
>>>
>>> I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)
>>>
>>> But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:
>>>
>>>   bipolar
>>>   diff-channels
>>>   settling-time-us
>>>   oversampling-ratio
>>>
>>> So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?
>>
>> Makes sense to me.  Whilst there are lots of things a channel can support, most
>> of them are hardware related and not universal.
> 
> Ok, I think I am finally beginning to see the light here :)
> 
> So I will then leave "additionalProperties: false".
> 
> I will send a V4 soon with all the latest changes.
> 

Just to clarify - we talk about hardware, not your Linux driver. What
your driver uses or doesn't, should not matter here that much.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Hugo Villeneuve]

On Sun, 15 Jan 2023 20:17:24 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 15/01/2023 17:32, Hugo Villeneuve wrote:
> >>>>> +    required:
> >>>>> +      - reg
> >>>>> +
> >>>>> +    additionalProperties: false  
> >>>>
> >>>> You are not allowing anything else from adc.yaml. Is it on purpose?  
> >>>
> >>> I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)
> >>>
> >>> But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:
> >>>
> >>>   bipolar
> >>>   diff-channels
> >>>   settling-time-us
> >>>   oversampling-ratio
> >>>
> >>> So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?
> >>
> >> Makes sense to me.  Whilst there are lots of things a channel can support, most
> >> of them are hardware related and not universal.
> > 
> > Ok, I think I am finally beginning to see the light here :)
> > 
> > So I will then leave "additionalProperties: false".
> > 
> > I will send a V4 soon with all the latest changes.
> > 
> 
> Just to clarify - we talk about hardware, not your Linux driver. What
> your driver uses or doesn't, should not matter here that much.

Hi,
the following properties are definitely not supported by the hardware:
    bipolar
    diff-channels
    oversampling-ratio

does this means that we should add these lines?
    bipolar: false
    diff-channels: false
    oversampling-ratio: false

as for settling-time-us, I am not sure of its usage and if its related to this hardware.

Hugo.


> Best regards,
> Krzysztof
> 
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add Texas Instruments ADS7924

[Jonathan Cameron]

On Sun, 15 Jan 2023 15:11:39 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Sun, 15 Jan 2023 20:17:24 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 15/01/2023 17:32, Hugo Villeneuve wrote:  
> > >>>>> +    required:
> > >>>>> +      - reg
> > >>>>> +
> > >>>>> +    additionalProperties: false    
> > >>>>
> > >>>> You are not allowing anything else from adc.yaml. Is it on purpose?    
> > >>>
> > >>> I am really not an expert with this Yaml stuff, and reading the documentation makes me probably more confused than before reading it :)
> > >>>
> > >>> But one thing that is for sure is that these other properties in adc.yaml are not used in my driver:
> > >>>
> > >>>   bipolar
> > >>>   diff-channels
> > >>>   settling-time-us
> > >>>   oversampling-ratio
> > >>>
> > >>> So is it Ok then to use "additionalProperties: false"? I think so, but what is your recommandation?  
> > >>
> > >> Makes sense to me.  Whilst there are lots of things a channel can support, most
> > >> of them are hardware related and not universal.  
> > > 
> > > Ok, I think I am finally beginning to see the light here :)
> > > 
> > > So I will then leave "additionalProperties: false".
> > > 
> > > I will send a V4 soon with all the latest changes.
> > >   
> > 
> > Just to clarify - we talk about hardware, not your Linux driver. What
> > your driver uses or doesn't, should not matter here that much.  

Indeed. The hardware does not support bipolar inputs, differential channels
or oversampling ratios as all of those require specific silicon that
is not in this particular device.

Technically you could emulate oversampling but there is no reason to do
that in kernel and no drivers do so + then it would not be a feature of
the hardware anyway so wouldn't belong in DT.

> 
> Hi,
> the following properties are definitely not supported by the hardware:
>     bipolar
>     diff-channels
>     oversampling-ratio
> 
> does this means that we should add these lines?
>     bipolar: false
>     diff-channels: false
>     oversampling-ratio: false
> 
> as for settling-time-us, I am not sure of its usage and if its related to this hardware.

That's used for devices that will delay their ADC sampling if there is a mux that is changing
which pin is being internally connected to a single shared ADC.  It requires hardware
support, which isn't present in this device.

Jonathan


> 
> Hugo.
> 
> 
> > Best regards,
> > Krzysztof
> > 
> >   
> 
>