[PATCH v3 1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000

[PATCH v3 1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

The ADS1100 is a 16-bit ADC (at 8 samples per second).
The ADS1000 is similar, but has a fixed data rate.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

(no changes since v2)

Changes in v2:
"reg" property is mandatory.
Add vdd-supply and #io-channel-cells

 .../bindings/iio/adc/ti,ads1100.yaml          | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
new file mode 100644
index 000000000000..970ccab15e1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI ADS1100/ADS1000 single channel I2C analog to digital converter
+
+maintainers:
+  - Mike Looijmans <mike.looijmans@topic.nl>
+
+description: |
+  Datasheet at: https://www.ti.com/lit/gpn/ads1100
+
+properties:
+  compatible:
+    enum:
+      - ti,ads1100
+      - ti,ads1000
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  "#io-channel-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@49 {
+            compatible = "ti,ads1100";
+            reg = <0x49>;
+        };
+    };
+...
-- 
2.17.1

[PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

The ADS1100 is a 16-bit ADC (at 8 samples per second).
The ADS1000 is similar, but has a fixed data rate.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

Changes in v3:
Add #include linux/bitfield.h and linux/bits.h

Changes in v2:
Remove "driver for" from title
Use proper PM_RUNTIME macros
Fix indents
Use dev_err_probe()
Unsigned index and post-increment
Use GENMASK and FIELD_GET
remove ads1100_set_conv_mode
remove Kconfig BUFFER dependencies
remove unused #include
Set SCALE instead of HARDWAREGAIN
Use devm_add_action_or_reset

 drivers/iio/adc/Kconfig      |  10 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads1100.c | 434 +++++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads1100.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 63f80d747cbd..257efb25a92e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1207,6 +1207,16 @@ config TI_ADS1015
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1015.
 
+config TI_ADS1100
+	tristate "Texas Instruments ADS1100 and ADS1000 ADC"
+	depends on I2C
+	help
+	  If you say yes here you get support for Texas Instruments ADS1100 and
+	  ADS1000 ADC chips.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ti-ads1100.
+
 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..61ef600fab99 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_ADS1100) += ti-ads1100.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-ads1100.c b/drivers/iio/adc/ti-ads1100.c
new file mode 100644
index 000000000000..1898bee9b784
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -0,0 +1,434 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADS1100 - Texas Instruments Analog-to-Digital Converter
+ *
+ * Copyright (c) 2023, Topic Embedded Products
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/ads1100
+ * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+/* The ADS1100 has a single byte config register */
+
+/* Conversion in progress bit */
+#define ADS1100_CFG_ST_BSY	BIT(7)
+/* Single conversion bit */
+#define ADS1100_CFG_SC		BIT(4)
+/* Data rate */
+#define ADS1100_DR_MASK		GENMASK(3, 2)
+/* Gain */
+#define ADS1100_PGA_MASK	GENMASK(1, 0)
+
+#define ADS1100_CONTINUOUS	0
+#define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
+
+#define ADS1100_SLEEP_DELAY_MS	2000
+
+static const int ads1100_data_rate[] = {128, 32, 16, 8};
+static const int ads1100_data_rate_bits[] = {12, 14, 15, 16};
+
+struct ads1100_data {
+	struct i2c_client *client;
+	struct regulator *reg_vdd;
+	struct mutex lock;
+	int scale_avail[2 * 4]; /* 4 gain settings */
+	u8 config;
+	bool supports_data_rate; /* Only the ADS1100 can select the rate */
+};
+
+static const struct iio_chan_spec ads1100_channel = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_all =
+				BIT(IIO_CHAN_INFO_SCALE) |
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.info_mask_shared_by_all_available =
+				BIT(IIO_CHAN_INFO_SCALE) |
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_type = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+		.endianness = IIO_CPU,
+	},
+	.datasheet_name = "AIN",
+};
+
+static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
+{
+	int ret;
+	u8 config = (data->config & ~mask) | (value & mask);
+
+	if (data->config == config)
+		return 0; /* Already done */
+
+	ret = i2c_master_send(data->client, &config, 1);
+	if (ret < 0)
+		return ret;
+
+	data->config = config;
+	return 0;
+};
+
+static int ads1100_data_bits(struct ads1100_data *data)
+{
+	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
+}
+
+static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
+{
+	int ret;
+	__be16 buffer;
+	s16 value;
+
+	if (chan != 0)
+		return -EINVAL;
+
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+
+	pm_runtime_mark_last_busy(&data->client->dev);
+	pm_runtime_put_autosuspend(&data->client->dev);
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+		return ret;
+	}
+
+	/* Value is always 16-bit 2's complement */
+	value = be16_to_cpu(buffer);
+	/* Shift result to compensate for bit resolution vs. sample rate */
+	value <<= 16 - ads1100_data_bits(data);
+	*val = sign_extend32(value, 15);
+
+	return 0;
+}
+
+static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
+{
+	int microvolts;
+	int gain;
+	int i;
+
+	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
+	if (val)
+		return -EINVAL;
+
+	microvolts = regulator_get_voltage(data->reg_vdd);
+	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
+	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
+
+	for (i = 0; i < 4; i++) {
+		if (BIT(i) == gain) {
+			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
+{
+	unsigned int i;
+	unsigned int size;
+
+	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
+	for (i = 0; i < size; ++i) {
+		if (ads1100_data_rate[i] == rate) {
+			return ads1100_set_config_bits(
+					data, ADS1100_DR_MASK,
+					FIELD_PREP(ADS1100_DR_MASK, i));
+		}
+	}
+
+	return -EINVAL;
+}
+
+static void ads1100_calc_scale_avail(struct ads1100_data *data)
+{
+	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
+		data->scale_avail[i * 2] = millivolts;
+		data->scale_avail[i * 2 + 1] = 15 + i;
+	}
+}
+
+static int ads1100_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	struct ads1100_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_VOLTAGE)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT;
+		*vals = ads1100_data_rate;
+		if (data->supports_data_rate)
+			*length = ARRAY_SIZE(ads1100_data_rate);
+		else
+			*length = 1;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_FRACTIONAL_LOG2;
+		*vals = data->scale_avail;
+		*length = ARRAY_SIZE(data->scale_avail);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1100_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	int ret;
+	struct ads1100_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			break;
+
+		ret = ads1100_get_adc_result(data, chan->address, val);
+		if (ret >= 0)
+			ret = IIO_VAL_INT;
+		iio_device_release_direct_mode(indio_dev);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		/* full-scale is the supply voltage in millivolts */
+		*val = regulator_get_voltage(data->reg_vdd) / 1000;
+		*val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
+		ret = IIO_VAL_FRACTIONAL_LOG2;
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = ads1100_data_rate[
+				FIELD_GET(ADS1100_DR_MASK, data->config)];
+		ret = IIO_VAL_INT;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int ads1100_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct ads1100_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = ads1100_set_scale(data, val, val2);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = ads1100_set_data_rate(data, chan->address, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static const struct iio_info ads1100_info = {
+	.read_avail	= ads1100_read_avail,
+	.read_raw	= ads1100_read_raw,
+	.write_raw	= ads1100_write_raw,
+};
+
+static int ads1100_setup(struct ads1100_data *data)
+{
+	int ret;
+	u8 buffer[3];
+
+	/* Setup continuous sampling mode at 8sps */
+	buffer[0] = ADS1100_DR_MASK | ADS1100_CONTINUOUS;
+	ret = i2c_master_send(data->client, buffer, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
+	if (ret < 0)
+		return ret;
+
+	/* Config register returned in third byte, strip away the busy status */
+	data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
+
+	/* Detect the sample rate capability by checking the DR bits */
+	data->supports_data_rate = FIELD_GET(ADS1100_DR_MASK, buffer[2]) != 0;
+
+	return 0;
+}
+
+static void ads1100_reg_disable(void *reg)
+{
+	regulator_disable(reg);
+}
+
+static void ads1100_disable_continuous(void *data)
+{
+	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
+}
+
+static int ads1100_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct ads1100_data *data;
+	struct device *dev = &client->dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+
+	indio_dev->name = "ads1100";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &ads1100_channel;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ads1100_info;
+
+	data->reg_vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(data->reg_vdd))
+		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
+				     "Failed to get vdd regulator\n");
+
+	ret = regulator_enable(data->reg_vdd);
+	if (ret < 0)
+		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
+				     "Failed to enable vdd regulator\n");
+
+	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
+	if (ret)
+		return ret;
+
+	ret = ads1100_setup(data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to communicate with device\n");
+
+	ret = devm_add_action_or_reset(dev, ads1100_disable_continuous, data);
+	if (ret)
+		return ret;
+
+	ads1100_calc_scale_avail(data);
+
+	pm_runtime_set_autosuspend_delay(dev, ADS1100_SLEEP_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
+
+	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 int ads1100_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct ads1100_data *data = iio_priv(indio_dev);
+
+	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
+	regulator_disable(data->reg_vdd);
+
+	return 0;
+}
+
+static int ads1100_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct ads1100_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(data->reg_vdd);
+	if (ret) {
+		dev_err(&data->client->dev, "Failed to enable Vdd\n");
+		return ret;
+	}
+
+	/*
+	 * We'll always change the mode bit in the config register, so there is
+	 * no need here to "force" a write to the config register. If the device
+	 * has been power-cycled, we'll re-write its config register now.
+	 */
+	return ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_CONTINUOUS);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
+				 ads1100_runtime_suspend,
+				 ads1100_runtime_resume,
+				 NULL);
+
+static const struct i2c_device_id ads1100_id[] = {
+	{ "ads1100", },
+	{ "ads1000", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ads1100_id);
+
+static const struct of_device_id ads1100_of_match[] = {
+	{ .compatible = "ti,ads1100", },
+	{ .compatible = "ti,ads1000", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ads1100_of_match);
+
+static struct i2c_driver ads1100_driver = {
+	.driver = {
+		.name = "ads1100",
+		.of_match_table = ads1100_of_match,
+		.pm = pm_ptr(&ads1100_pm_ops),
+	},
+	.probe_new	= ads1100_probe,
+	.id_table	= ads1100_id,
+};
+
+module_i2c_driver(ads1100_driver);
+
+MODULE_AUTHOR("Mike Looijmans <mike.looijmans@topic.nl>");
+MODULE_DESCRIPTION("Texas Instruments ADS1100 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:
> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> The ADS1000 is similar, but has a fixed data rate.

...

> +	/* Shift result to compensate for bit resolution vs. sample rate */
> +	value <<= 16 - ads1100_data_bits(data);
> +	*val = sign_extend32(value, 15);

Why not simply

	*val = sign_extend32(value, ads1100_data_bits(data) - 1);

?

(Double check for off-by-one usage)

...

> +	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */

Can you use more math / plain English to describe the formula? Otherwise we can
see the very same in the code and point of the comment is doubtful.

> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;

Something from units.h?

...

> +	for (i = 0; i < 4; i++) {
> +		if (BIT(i) == gain) {

ffs()/__ffs() (look at the documentation for the difference and use proper one).

> +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> +			return 0;
> +		}
> +	}

...

> +	for (i = 0; i < size; ++i) {

Why pre-increment?

> +		if (ads1100_data_rate[i] == rate) {
> +			return ads1100_set_config_bits(
> +					data, ADS1100_DR_MASK,

Strange indentation.

> +					FIELD_PREP(ADS1100_DR_MASK, i));
> +		}

Do you need {} ?

> +	}

...

> +	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;

units.h?

...

> +		data->scale_avail[i * 2] = millivolts;

I would write ' * 2 + 0]', but it's up to you.

> +		data->scale_avail[i * 2 + 1] = 15 + i;

...

> +		*val = regulator_get_voltage(data->reg_vdd) / 1000;

units.h?

...

> +		*val = ads1100_data_rate[
> +				FIELD_GET(ADS1100_DR_MASK, data->config)];

Strange indentation, just use a single line.

...

> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret < 0)

Why ' < 0'?

> +		return dev_err_probe(dev, ret,
> +				     "Failed to register IIO device\n");

...

> +static int ads1100_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct ads1100_data *data = iio_priv(indio_dev);
> +
> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
> +	regulator_disable(data->reg_vdd);

Wrong devm / non-devm ordering.

> +	return 0;
> +}

...

> +static const struct i2c_device_id ads1100_id[] = {
> +	{ "ads1100", },
> +	{ "ads1000", },

Inner commas are not needed.

> +	{}
> +};

...

> +static const struct of_device_id ads1100_of_match[] = {
> +	{ .compatible = "ti,ads1100", },
> +	{ .compatible = "ti,ads1000", },

Ditto.

> +	{}
> +};

...

> +

Redundant blank line.

> +module_i2c_driver(ads1100_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

Comments below. Mailserver has a top-post fetish and will inject a 
signature here somewhere...

No further comment from me means "agree, will implement in v4"...



Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 01-03-2023 16:30, Andy Shevchenko wrote:
> On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
> ...
>
>> +	/* Shift result to compensate for bit resolution vs. sample rate */
>> +	value <<= 16 - ads1100_data_bits(data);
>> +	*val = sign_extend32(value, 15);
> Why not simply
>
> 	*val = sign_extend32(value, ads1100_data_bits(data) - 1);
>
> ?

As discussed with  Jonathan Cameron, the register is right-justified and 
the number of bits depend on the data rate. Rather than having the 
"scale" change when the sample rate changes, we chose to adjust the 
sample result so it's always left-justified.


>> +	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
> Can you use more math / plain English to describe the formula? Otherwise we can
> see the very same in the code and point of the comment is doubtful.

I'll try to explain it better.


>
>> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
> Something from units.h?

Could put MILLI here, but I doubt if that improves things. Its actually 
MICRO/(MICRO/MILLI) given the explanation above... Not helping much...


> ...
>
>> +	for (i = 0; i < 4; i++) {
>> +		if (BIT(i) == gain) {
> ffs()/__ffs() (look at the documentation for the difference and use proper one).

Thought of it, but I'd rather have it return EINVAL for attempting to 
set the analog gain to "7" (0nly 1,2,4,8 allowed).


> ...
>> +	for (i = 0; i < size; ++i) {
> Why pre-increment?

Spent too much time with other coding guidelines, missed this one... 
Will change.


>
> ...
>
>> +	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
> units.h?

Should I write:

regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS);

I doubt that improves readability.

> ...
> ...
>
>> +static int ads1100_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +	struct ads1100_data *data = iio_priv(indio_dev);
>> +
>> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
>> +	regulator_disable(data->reg_vdd);
> Wrong devm / non-devm ordering.

Don't understand your remark, can you explain further please?

devm / non-devm ordering would be related to the "probe" function. As 
far as I can tell, I'm not allocating resources after the devm calls. 
And the "remove" is empty.



-- 
Mike Looijmans

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Lars-Peter Clausen]

On 3/1/23 23:49, Mike Looijmans wrote:
>
>
>> ...
>> ...
>>
>>> +static int ads1100_runtime_suspend(struct device *dev)
>>> +{
>>> +    struct iio_dev *indio_dev = 
>>> i2c_get_clientdata(to_i2c_client(dev));
>>> +    struct ads1100_data *data = iio_priv(indio_dev);
>>> +
>>> +    ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
>>> +    regulator_disable(data->reg_vdd);
>> Wrong devm / non-devm ordering.
>
> Don't understand your remark, can you explain further please?
>
> devm / non-devm ordering would be related to the "probe" function. As 
> far as I can tell, I'm not allocating resources after the devm calls. 
> And the "remove" is empty.

Strictly speaking we need to unregister the IIO device before disabling 
the regulator, otherwise there is a small window where the IIO device 
still exists, but doesn't work anymore. This is a very theoretical 
scenario though.

You are lucky :) There is a new function 
`devm_regulator_get_enable()`[1], which will manage the 
regulator_disable() for you. Using that will also reduce the boilerplate 
in `probe()` a bit

- Lars

[1] https://lwn.net/Articles/904383/

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Lars-Peter Clausen]

On 3/2/23 05:16, Lars-Peter Clausen wrote:
> On 3/1/23 23:49, Mike Looijmans wrote:
>>
>>
>>> ...
>>> ...
>>>
>>>> +static int ads1100_runtime_suspend(struct device *dev)
>>>> +{
>>>> +    struct iio_dev *indio_dev = 
>>>> i2c_get_clientdata(to_i2c_client(dev));
>>>> +    struct ads1100_data *data = iio_priv(indio_dev);
>>>> +
>>>> +    ads1100_set_config_bits(data, ADS1100_CFG_SC, 
>>>> ADS1100_SINGLESHOT);
>>>> +    regulator_disable(data->reg_vdd);
>>> Wrong devm / non-devm ordering.
>>
>> Don't understand your remark, can you explain further please?
>>
>> devm / non-devm ordering would be related to the "probe" function. As 
>> far as I can tell, I'm not allocating resources after the devm calls. 
>> And the "remove" is empty.
>
> Strictly speaking we need to unregister the IIO device before 
> disabling the regulator, otherwise there is a small window where the 
> IIO device still exists, but doesn't work anymore. This is a very 
> theoretical scenario though.
>
> You are lucky :) There is a new function 
> `devm_regulator_get_enable()`[1], which will manage the 
> regulator_disable() for you. Using that will also reduce the 
> boilerplate in `probe()` a bit
>
> - Lars
>
> [1] https://lwn.net/Articles/904383/
>
Sorry, just saw that Andy's comment was on the suspend() function, not 
remove(). In that case there is of course no need for any devm things. 
But still a good idea to use `devm_regulator_get_enable()` in probe for 
the boiler plate.

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Thu, Mar 02, 2023 at 05:20:38AM -0800, Lars-Peter Clausen wrote:
> On 3/2/23 05:16, Lars-Peter Clausen wrote:
> > On 3/1/23 23:49, Mike Looijmans wrote:

...

> > > > > +static int ads1100_runtime_suspend(struct device *dev)
> > > > > +{
> > > > > +    struct iio_dev *indio_dev =
> > > > > i2c_get_clientdata(to_i2c_client(dev));
> > > > > +    struct ads1100_data *data = iio_priv(indio_dev);
> > > > > +
> > > > > +    ads1100_set_config_bits(data, ADS1100_CFG_SC,
> > > > > ADS1100_SINGLESHOT);
> > > > > +    regulator_disable(data->reg_vdd);
> > > > Wrong devm / non-devm ordering.
> > > 
> > > Don't understand your remark, can you explain further please?
> > > 
> > > devm / non-devm ordering would be related to the "probe" function.
> > > As far as I can tell, I'm not allocating resources after the devm
> > > calls. And the "remove" is empty.
> > 
> > Strictly speaking we need to unregister the IIO device before disabling
> > the regulator, otherwise there is a small window where the IIO device
> > still exists, but doesn't work anymore. This is a very theoretical
> > scenario though.
> > 
> > You are lucky :) There is a new function
> > `devm_regulator_get_enable()`[1], which will manage the
> > regulator_disable() for you. Using that will also reduce the boilerplate
> > in `probe()` a bit
> > 
> > [1] https://lwn.net/Articles/904383/
> > 
> Sorry, just saw that Andy's comment was on the suspend() function, not
> remove(). In that case there is of course no need for any devm things. But
> still a good idea to use `devm_regulator_get_enable()` in probe for the
> boiler plate.

Yeah, sorry, I mistakenly took it as ->remove().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Jonathan Cameron]

On Thu, 2 Mar 2023 05:20:38 -0800
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/2/23 05:16, Lars-Peter Clausen wrote:
> > On 3/1/23 23:49, Mike Looijmans wrote:  
> >>
> >>  
> >>> ...
> >>> ...
> >>>  
> >>>> +static int ads1100_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> +    struct iio_dev *indio_dev = 
> >>>> i2c_get_clientdata(to_i2c_client(dev));
> >>>> +    struct ads1100_data *data = iio_priv(indio_dev);
> >>>> +
> >>>> +    ads1100_set_config_bits(data, ADS1100_CFG_SC, 
> >>>> ADS1100_SINGLESHOT);
> >>>> +    regulator_disable(data->reg_vdd);  
> >>> Wrong devm / non-devm ordering.  
> >>
> >> Don't understand your remark, can you explain further please?
> >>
> >> devm / non-devm ordering would be related to the "probe" function. As 
> >> far as I can tell, I'm not allocating resources after the devm calls. 
> >> And the "remove" is empty.  
> >
> > Strictly speaking we need to unregister the IIO device before 
> > disabling the regulator, otherwise there is a small window where the 
> > IIO device still exists, but doesn't work anymore. This is a very 
> > theoretical scenario though.
> >
> > You are lucky :) There is a new function 
> > `devm_regulator_get_enable()`[1], which will manage the 
> > regulator_disable() for you. Using that will also reduce the 
> > boilerplate in `probe()` a bit
> >
> > - Lars
> >
> > [1] https://lwn.net/Articles/904383/
> >  
> Sorry, just saw that Andy's comment was on the suspend() function, not 
> remove(). In that case there is of course no need for any devm things. 
> But still a good idea to use `devm_regulator_get_enable()` in probe for 
> the boiler plate.
> 
You can't because (annoyingly) devem_regulator_get_enable() doesn't
provide you access to the struct regulator that you need to be able
to turn it of for power management.
That case only works for the leave the power on all the time cases.

Jonathan

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote:
> On 01-03-2023 16:30, Andy Shevchenko wrote:
> > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:

...

> > > +	/* Shift result to compensate for bit resolution vs. sample rate */
> > > +	value <<= 16 - ads1100_data_bits(data);
> > > +	*val = sign_extend32(value, 15);
> > Why not simply
> > 
> > 	*val = sign_extend32(value, ads1100_data_bits(data) - 1);
> > 
> > ?
> 
> As discussed with  Jonathan Cameron, the register is right-justified and the
> number of bits depend on the data rate. Rather than having the "scale"
> change when the sample rate changes, we chose to adjust the sample result so
> it's always left-justified.

Hmm... OK, but it adds unneeded code I think.

...

> > > +	for (i = 0; i < 4; i++) {
> > > +		if (BIT(i) == gain) {
> > ffs()/__ffs() (look at the documentation for the difference and use proper one).
> 
> Thought of it, but I'd rather have it return EINVAL for attempting to set
> the analog gain to "7" (0nly 1,2,4,8 allowed).

I'm not sure what you are implying.

You have open coded something that has already to be a function which on some
architectures become a single assembly instruction.

That said, drop your for-loop if-cond and use one of the proposed directly.
Then you may compare the result to what ever you want to be a limit and return
whatever error code you want to.

...

> > > +	for (i = 0; i < size; ++i) {
> > Why pre-increment?
> 
> Spent too much time with other coding guidelines, missed this one... Will
> change.

I don't remember that's in coding guidelines, but it's standard practice in the
Linux kernel project. Yeah, we have a few hundreds of the pre-increments, but
reasons may be quite different for those.

...

> > > +	int millivolts = regulator_get_voltage(data->reg_vdd) / 1000;
> > units.h?
> 
> Should I write:
> 
> regulator_get_voltage(data->reg_vdd) / (MICROS / MILLIS);
> 
> I doubt that improves readability.

Yeah, it should be something like MICROVOLT_PER_MILLIVOLT.
But it's not defined yet.

...

> > > +static int ads1100_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > > +	struct ads1100_data *data = iio_priv(indio_dev);
> > > +
> > > +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
> > > +	regulator_disable(data->reg_vdd);
> > Wrong devm / non-devm ordering.
> 
> Don't understand your remark, can you explain further please?
> 
> devm / non-devm ordering would be related to the "probe" function. As far as
> I can tell, I'm not allocating resources after the devm calls. And the
> "remove" is empty.

Ah, it's my mistake, I misread it as ->remove().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Jonathan Cameron]

On Thu, 2 Mar 2023 16:23:14 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote:
> > On 01-03-2023 16:30, Andy Shevchenko wrote:  
> > > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:  
> 
> ...
> 
> > > > +	/* Shift result to compensate for bit resolution vs. sample rate */
> > > > +	value <<= 16 - ads1100_data_bits(data);
> > > > +	*val = sign_extend32(value, 15);  
> > > Why not simply
> > > 
> > > 	*val = sign_extend32(value, ads1100_data_bits(data) - 1);
> > > 
> > > ?  
> > 
> > As discussed with  Jonathan Cameron, the register is right-justified and the
> > number of bits depend on the data rate. Rather than having the "scale"
> > change when the sample rate changes, we chose to adjust the sample result so
> > it's always left-justified.  
> 
> Hmm... OK, but it adds unneeded code I think.

There isn't a way to do it in one go that I can think of.
The first statement is multiplying the value by a power of 2, not just sign extending it.
You could sign extend first then shift to do the multiply, but ends up same amount
of code.

It does look a bit like a weird open coded sign extension though so I can see where
the confusion came from!

> 
> ...
> 
> > > > +	for (i = 0; i < 4; i++) {
> > > > +		if (BIT(i) == gain) {  
> > > ffs()/__ffs() (look at the documentation for the difference and use proper one).  
> > 
> > Thought of it, but I'd rather have it return EINVAL for attempting to set
> > the analog gain to "7" (0nly 1,2,4,8 allowed).  
> 
> I'm not sure what you are implying.
> 
> You have open coded something that has already to be a function which on some
> architectures become a single assembly instruction.
> 
> That said, drop your for-loop if-cond and use one of the proposed directly.
> Then you may compare the result to what ever you want to be a limit and return
> whatever error code you want to

Agreed, could do it with appropriate ffs() followed by if (BIT(i) != gain) return -EINVAL;

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Sat, Mar 04, 2023 at 05:26:18PM +0000, Jonathan Cameron wrote:
> On Thu, 2 Mar 2023 16:23:14 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 02, 2023 at 08:49:22AM +0100, Mike Looijmans wrote:
> > > On 01-03-2023 16:30, Andy Shevchenko wrote:  
> > > > On Tue, Feb 28, 2023 at 07:31:51AM +0100, Mike Looijmans wrote:  

...

> > > > > +	/* Shift result to compensate for bit resolution vs. sample rate */
> > > > > +	value <<= 16 - ads1100_data_bits(data);
> > > > > +	*val = sign_extend32(value, 15);  
> > > > Why not simply
> > > > 
> > > > 	*val = sign_extend32(value, ads1100_data_bits(data) - 1);
> > > > 
> > > > ?  
> > > 
> > > As discussed with  Jonathan Cameron, the register is right-justified and the
> > > number of bits depend on the data rate. Rather than having the "scale"
> > > change when the sample rate changes, we chose to adjust the sample result so
> > > it's always left-justified.  
> > 
> > Hmm... OK, but it adds unneeded code I think.
> 
> There isn't a way to do it in one go that I can think of.
> The first statement is multiplying the value by a power of 2, not just sign extending it.
> You could sign extend first then shift to do the multiply, but ends up same amount
> of code.
> 
> It does look a bit like a weird open coded sign extension though so I can see where
> the confusion came from!

I see, for the negative value both approaches will work, for the positive
the original one will provide a multiplied value.

Yeah, doesn't seem to be a subject to the (micro-)optimizations.

...

> > > > > +	for (i = 0; i < 4; i++) {
> > > > > +		if (BIT(i) == gain) {  
> > > > ffs()/__ffs() (look at the documentation for the difference and use proper one).  
> > > 
> > > Thought of it, but I'd rather have it return EINVAL for attempting to set
> > > the analog gain to "7" (0nly 1,2,4,8 allowed).  
> > 
> > I'm not sure what you are implying.
> > 
> > You have open coded something that has already to be a function which on some
> > architectures become a single assembly instruction.
> > 
> > That said, drop your for-loop if-cond and use one of the proposed directly.
> > Then you may compare the result to what ever you want to be a limit and return
> > whatever error code you want to
> 
> Agreed, could do it with appropriate ffs() followed by if (BIT(i) != gain) return -EINVAL;

I meant something different.

	i = ffs(gain); // or __ffs(gain)?
	if (i >= 4)
		return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Jonathan Cameron]

On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> The ADS1000 is similar, but has a fixed data rate.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Hi Mike,

A few minor things + one request for a test as trying to chase a possible
ref count overflow around the runtime_pm was giving me a enough of a headache
that it's easier to ask you just to poke it and see.  If it doesn't fail as
I expect I'll take a closer look!

Jonathan


> +static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> +{
> +	int microvolts;
> +	int gain;
> +	int i;
> +
> +	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
> +	if (val)
> +		return -EINVAL;
> +
> +	microvolts = regulator_get_voltage(data->reg_vdd);
> +	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (BIT(i) == gain) {
> +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> +			return 0;
> +		}
> +	}
Andy's suggestion of something like..
	if (!gain)
		return -EINVAL;
	i = ffs(gain);
	if (i >= 4 || BIT(i) != gain)
		return -EINVAL;

	ads...

Is perhaps nicer than the loop.

	 
> +
> +	return -EINVAL;
> +}


> +static void ads1100_disable_continuous(void *data)
> +{
> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
> +}
> +
> +static int ads1100_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ads1100_data *data;
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata()
below by taking advantage of the fact you have a local dev pointer.

	dev_set_drvdata(dev, indio_dev);
and no confusing mix is left.  Of course it's doing the same thing but to my
mind slightly nicer to use the same one.

> +	data->client = client;
> +	mutex_init(&data->lock);
> +
> +	indio_dev->name = "ads1100";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &ads1100_channel;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ads1100_info;
> +
> +	data->reg_vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(data->reg_vdd))
> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> +				     "Failed to get vdd regulator\n");
> +
> +	ret = regulator_enable(data->reg_vdd);
> +	if (ret < 0)
> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> +				     "Failed to enable vdd regulator\n");
> +
> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> +	if (ret)
> +		return ret;

Please could you check a subtle interaction of runtime pm and this devm managed
flow.

I think we can hit the following flow.
1) In runtime suspend (wait long enough for this to happen).
2) Unbind the driver (rmmod will do)
3) During the unbind we exit suspend then enter it again before we call remove
   (that's just part of the normal remove flow).
4) We then end up calling regulator disable when it's already disabled.

We've traditionally avoided that by having the remove explicitly call
pm_runtime_get_sync() before we then disable runtime pm.  I don't
think that happens with devm_pm_runtime_enable() but I could be missing
a path where it does.

If the sequence goes wrong you should get a warning about an unbalanced regulator
disable.  The fix would be an extra devm_add_action_or_reset() before the
devm_iio_device_register() below that just calls pm_runtime_get_sync()
to force the state to on.

Gah. These subtle paths always give me a headache.
We don't normally have too much problem with this because many
runtime_resume / suspend functions don't change reference counts.



> +
> +	ret = ads1100_setup(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to communicate with device\n");
> +
> +	ret = devm_add_action_or_reset(dev, ads1100_disable_continuous, data);
> +	if (ret)
> +		return ret;
> +
> +	ads1100_calc_scale_avail(data);
> +
> +	pm_runtime_set_autosuspend_delay(dev, ADS1100_SLEEP_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable pm_runtime\n");
> +
> +	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 int ads1100_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

Fine to just short cut this dance with 
struct iio_dev *indio_dev = dev_get_drvdata(dev);

It's a bit nasty from a readability point of view, but the pattern is
so common we've kind of gotten used to it.



> +	struct ads1100_data *data = iio_priv(indio_dev);

As you don't need the indio_dev, can combine all this into

	struct ads110_data *data = iio_priv(dev_get_drvdata(dev));

> +
> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
> +	regulator_disable(data->reg_vdd);
> +
> +	return 0;
> +}
> +
> +static int ads1100_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

As above.

> +	struct ads1100_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_enable(data->reg_vdd);
> +	if (ret) {
> +		dev_err(&data->client->dev, "Failed to enable Vdd\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * We'll always change the mode bit in the config register, so there is
> +	 * no need here to "force" a write to the config register. If the device
> +	 * has been power-cycled, we'll re-write its config register now.
> +	 */
> +	return ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_CONTINUOUS);
> +}
> +

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 04-03-2023 18:57, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
>
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> Hi Mike,
>
> A few minor things + one request for a test as trying to chase a possible
> ref count overflow around the runtime_pm was giving me a enough of a headache
> that it's easier to ask you just to poke it and see.  If it doesn't fail as
> I expect I'll take a closer look!

Will do, but it may take a few days to get access to the hardware again. 
I'll report on that later.

>> +static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>> +{
>> +	int microvolts;
>> +	int gain;
>> +	int i;
>> +
>> +	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
>> +	if (val)
>> +		return -EINVAL;
>> +
>> +	microvolts = regulator_get_voltage(data->reg_vdd);
>> +	/* Calculate: gain = ((microvolts / 1000) / (val2 / 1000000)) >> 15 */
>> +	gain = ((microvolts + BIT(14)) >> 15) * 1000 / val2;
>> +
>> +	for (i = 0; i < 4; i++) {
>> +		if (BIT(i) == gain) {
>> +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
>> +			return 0;
>> +		}
>> +	}
> Andy's suggestion of something like..
> 	if (!gain)
> 		return -EINVAL;
> 	i = ffs(gain);
> 	if (i >= 4 || BIT(i) != gain)
> 		return -EINVAL;
>
> 	ads...
>
> Is perhaps nicer than the loop.

Yes, takes out a loop.


>
>> +static void ads1100_disable_continuous(void *data)
>> +{
>> +	ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_SINGLESHOT);
>> +}
>> +
>> +static int ads1100_probe(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct ads1100_data *data;
>> +	struct device *dev = &client->dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
> You can avoid the slightly nasty mix of i2c_set_clientdata vs dev_get_drvdata()
> below by taking advantage of the fact you have a local dev pointer.
>
> 	dev_set_drvdata(dev, indio_dev);
> and no confusing mix is left.  Of course it's doing the same thing but to my
> mind slightly nicer to use the same one.

Since I don't need indio_dev anywhere, I might as well say this directly:

dev_set_drvdata(dev, data);

(Only the pm_ routines use this)

> ...
>

-- 
Mike Looijmans

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 04-03-2023 18:57, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
>
>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>> The ADS1000 is similar, but has a fixed data rate.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> Hi Mike,
>
> A few minor things + one request for a test as trying to chase a possible
> ref count overflow around the runtime_pm was giving me a enough of a headache
> that it's easier to ask you just to poke it and see.  If it doesn't fail as
> I expect I'll take a closer look!
>
> Jonathan
>
> ...
>> +	data->client = client;
>> +	mutex_init(&data->lock);
>> +
>> +	indio_dev->name = "ads1100";
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = &ads1100_channel;
>> +	indio_dev->num_channels = 1;
>> +	indio_dev->info = &ads1100_info;
>> +
>> +	data->reg_vdd = devm_regulator_get(dev, "vdd");
>> +	if (IS_ERR(data->reg_vdd))
>> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
>> +				     "Failed to get vdd regulator\n");
>> +
>> +	ret = regulator_enable(data->reg_vdd);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
>> +				     "Failed to enable vdd regulator\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
>> +	if (ret)
>> +		return ret;
> Please could you check a subtle interaction of runtime pm and this devm managed
> flow.
>
> I think we can hit the following flow.
> 1) In runtime suspend (wait long enough for this to happen).
> 2) Unbind the driver (rmmod will do)
> 3) During the unbind we exit suspend then enter it again before we call remove
>     (that's just part of the normal remove flow).
> 4) We then end up calling regulator disable when it's already disabled.
>
> We've traditionally avoided that by having the remove explicitly call
> pm_runtime_get_sync() before we then disable runtime pm.  I don't
> think that happens with devm_pm_runtime_enable() but I could be missing
> a path where it does.
>
> If the sequence goes wrong you should get a warning about an unbalanced regulator
> disable.  The fix would be an extra devm_add_action_or_reset() before the
> devm_iio_device_register() below that just calls pm_runtime_get_sync()
> to force the state to on.
>
> Gah. These subtle paths always give me a headache.
> We don't normally have too much problem with this because many
> runtime_resume / suspend functions don't change reference counts.

Just did this test, waited a few seconds, checked 
/sys/kernel/debug/regulator... that the regulator had been disabled.

Then executed:
echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind

to unload the driver, and no messages were added to the kernel log.

I could see the driver going away and removing itself from iio and 
regulators.

Tried this a couple of times (using bind/unbind), and no problem reported.

Hopes this helps with your headaches...

-- 
Mike Looijmans

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Jonathan Cameron]

On Mon, 6 Mar 2023 12:21:44 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topicproducts.com
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail
> On 04-03-2023 18:57, Jonathan Cameron wrote:
> > On Tue, 28 Feb 2023 07:31:51 +0100
> > Mike Looijmans <mike.looijmans@topic.nl> wrote:
> >  
> >> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> >> The ADS1000 is similar, but has a fixed data rate.
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>  
> > Hi Mike,
> >
> > A few minor things + one request for a test as trying to chase a possible
> > ref count overflow around the runtime_pm was giving me a enough of a headache
> > that it's easier to ask you just to poke it and see.  If it doesn't fail as
> > I expect I'll take a closer look!
> >
> > Jonathan
> >
> > ...  
> >> +	data->client = client;
> >> +	mutex_init(&data->lock);
> >> +
> >> +	indio_dev->name = "ads1100";
> >> +	indio_dev->modes = INDIO_DIRECT_MODE;
> >> +	indio_dev->channels = &ads1100_channel;
> >> +	indio_dev->num_channels = 1;
> >> +	indio_dev->info = &ads1100_info;
> >> +
> >> +	data->reg_vdd = devm_regulator_get(dev, "vdd");
> >> +	if (IS_ERR(data->reg_vdd))
> >> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> >> +				     "Failed to get vdd regulator\n");
> >> +
> >> +	ret = regulator_enable(data->reg_vdd);
> >> +	if (ret < 0)
> >> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> >> +				     "Failed to enable vdd regulator\n");
> >> +
> >> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> >> +	if (ret)
> >> +		return ret;  
> > Please could you check a subtle interaction of runtime pm and this devm managed
> > flow.
> >
> > I think we can hit the following flow.
> > 1) In runtime suspend (wait long enough for this to happen).
> > 2) Unbind the driver (rmmod will do)
> > 3) During the unbind we exit suspend then enter it again before we call remove
> >     (that's just part of the normal remove flow).
> > 4) We then end up calling regulator disable when it's already disabled.
> >
> > We've traditionally avoided that by having the remove explicitly call
> > pm_runtime_get_sync() before we then disable runtime pm.  I don't
> > think that happens with devm_pm_runtime_enable() but I could be missing
> > a path where it does.
> >
> > If the sequence goes wrong you should get a warning about an unbalanced regulator
> > disable.  The fix would be an extra devm_add_action_or_reset() before the
> > devm_iio_device_register() below that just calls pm_runtime_get_sync()
> > to force the state to on.
> >
> > Gah. These subtle paths always give me a headache.
> > We don't normally have too much problem with this because many
> > runtime_resume / suspend functions don't change reference counts.  
> 
> Just did this test, waited a few seconds, checked 
> /sys/kernel/debug/regulator... that the regulator had been disabled.
> 
> Then executed:
> echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind
> 
> to unload the driver, and no messages were added to the kernel log.
> 
> I could see the driver going away and removing itself from iio and 
> regulators.
> 
> Tried this a couple of times (using bind/unbind), and no problem reported.
> 
> Hopes this helps with your headaches...

Thanks!

> 

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:

...

> > +	for (i = 0; i < 4; i++) {
> > +		if (BIT(i) == gain) {
> > +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> > +			return 0;
> > +		}
> > +	}
> Andy's suggestion of something like..
> 	if (!gain)
> 		return -EINVAL;
> 	i = ffs(gain);
> 	if (i >= 4 || BIT(i) != gain)
> 		return -EINVAL;
> 
> 	ads...
> 
> Is perhaps nicer than the loop.

Even better:

	if (!gain || !is_power_of_2(gain))
		return -EINVAL;

	i = ffs(gain);
	if (i >= 4)
		return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Mon, Mar 06, 2023 at 02:11:48PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> > On Tue, 28 Feb 2023 07:31:51 +0100
> > Mike Looijmans <mike.looijmans@topic.nl> wrote:

...

> > > +	for (i = 0; i < 4; i++) {
> > > +		if (BIT(i) == gain) {
> > > +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> > > +			return 0;
> > > +		}
> > > +	}
> > Andy's suggestion of something like..
> > 	if (!gain)
> > 		return -EINVAL;
> > 	i = ffs(gain);
> > 	if (i >= 4 || BIT(i) != gain)
> > 		return -EINVAL;
> > 
> > 	ads...
> > 
> > Is perhaps nicer than the loop.
> 
> Even better:
> 
> 	if (!gain || !is_power_of_2(gain))
> 		return -EINVAL;

Or if you want to combine all checks:

	if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain))
		return -EINVAL;

	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain));
	return 0;

(You can play with bloat-o-meter for the code generation and see which one is
 better from that aspect)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Mon, Mar 06, 2023 at 02:16:24PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 02:11:48PM +0200, Andy Shevchenko wrote:
> > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> > > On Tue, 28 Feb 2023 07:31:51 +0100
> > > Mike Looijmans <mike.looijmans@topic.nl> wrote:

...

> > > > +	for (i = 0; i < 4; i++) {
> > > > +		if (BIT(i) == gain) {
> > > > +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
> > > > +			return 0;
> > > > +		}
> > > > +	}
> > > Andy's suggestion of something like..
> > > 	if (!gain)
> > > 		return -EINVAL;
> > > 	i = ffs(gain);
> > > 	if (i >= 4 || BIT(i) != gain)
> > > 		return -EINVAL;
> > > 
> > > 	ads...
> > > 
> > > Is perhaps nicer than the loop.
> > 
> > Even better:
> > 
> > 	if (!gain || !is_power_of_2(gain))
> > 		return -EINVAL;
> 
> Or if you want to combine all checks:

> 	if (clamp_val(gain, BIT(0), BIT(3)) != gain || !is_power_of_2(gain))
> 		return -EINVAL;

Just a side note. I have been wanting to have is_in_range() for a long time.
Perhaps we can add a such to the kernel (math.h)

/* Check if in the range [start .. end] */
#define	is_in_range(value, start, end)	(value >= start && value <= end)

With it, the above will probably better look

	if (!is_in_range(gain, BIT(0), BIT(3) || !is_power_of_2(gain))

> 	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain));
> 	return 0;
> 
> (You can play with bloat-o-meter for the code generation and see which one is
>  better from that aspect)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 06-03-2023 13:11, Andy Shevchenko wrote:
> On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
>> On Tue, 28 Feb 2023 07:31:51 +0100
>> Mike Looijmans <mike.looijmans@topic.nl> wrote:
> ...
>
>>> +	for (i = 0; i < 4; i++) {
>>> +		if (BIT(i) == gain) {
>>> +			ads1100_set_config_bits(data, ADS1100_PGA_MASK, i);
>>> +			return 0;
>>> +		}
>>> +	}
>> Andy's suggestion of something like..
>> 	if (!gain)
>> 		return -EINVAL;
>> 	i = ffs(gain);
>> 	if (i >= 4 || BIT(i) != gain)
>> 		return -EINVAL;
>>
>> 	ads...
>>
>> Is perhaps nicer than the loop.
> Even better:
>
> 	if (!gain || !is_power_of_2(gain))
> 		return -EINVAL;
>
> 	i = ffs(gain);
> 	if (i >= 4)
> 		return -EINVAL;
>
I'd guess that "is_power_of_2" is about the same complexity as "ffs".

If we want smaller code, in retrospect, I'd vote for omitting that 
power-of-two check altogether.

The IIO device reports this for a 3v3 Vdd:
# cat /sys/bus/iio/devices/iio\:device1/scale_available
0.100708007 0.050354003 0.025177001 0.012588500
# echo 0.012588500 > /sys/bus/iio/devices/iio:device1/scale

That last statement results in val2=12588 in this call.
The whole point of this exercise is that the value '0.012588' 
corresponds to a gain of '8' here. There's already quite a bit of 
rounding going on, since writing to the scale turns the value into 
"micro" scale.

Also, looking at the "avail" table. the values for "7" and "8" are much 
closer together than "3" and "4".

And the correct formula for the inverse of "gain = BIT(i);" is "i = 
ffs(gain) - 1;" because ffs(1) == 1

So I propose this code:

     if (gain <= 0 || gain > 8)
         return -EINVAL;

     regval = ffs(gain) - 1;
     ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);


-- 
Mike Looijmans

Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Andy Shevchenko]

On Mon, Mar 06, 2023 at 01:56:15PM +0100, Mike Looijmans wrote:
> On 06-03-2023 13:11, Andy Shevchenko wrote:
> > On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> > > On Tue, 28 Feb 2023 07:31:51 +0100
> > > Mike Looijmans <mike.looijmans@topic.nl> wrote:

...

> So I propose this code:
> 
>     if (gain <= 0 || gain > 8)

Maybe BIT(0) and BIT(3) will be more explicit.
Otherwise I'm fine with it.

>         return -EINVAL;
> 
>     regval = ffs(gain) - 1;
>     ads1100_set_config_bits(data, ADS1100_PGA_MASK, regval);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] dt-bindings: iio: adc: Add TI ADS1100 and ADS1000

[Krzysztof Kozlowski]

On 28/02/2023 07:31, Mike Looijmans wrote:
> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> The ADS1000 is similar, but has a fixed data rate.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> "reg" property is mandatory.
> Add vdd-supply and #io-channel-cells


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof