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

[permalink] [raw]

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

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 <[email protected]>

---

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 <[email protected]>");
+MODULE_DESCRIPTION("Texas Instruments ADS1100 ADC driver");
+MODULE_LICENSE("GPL");
--
2.17.1

2023-02-28 10:35:03

by Krzysztof Kozlowski

[permalink] [raw]

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

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 <[email protected]>
>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> "reg" property is mandatory.
> Add vdd-supply and #io-channel-cells

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2023-03-01 15:30:14

[permalink] [raw]

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

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

2023-03-02 07:49:41

[permalink] [raw]

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

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: [email protected]
W: http://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

2023-03-02 13:17:13

by Lars-Peter Clausen

[permalink] [raw]

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

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/

2023-03-02 13:20:51

by Lars-Peter Clausen

[permalink] [raw]

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

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.

2023-03-02 14:23:26

[permalink] [raw]

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

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

2023-03-02 14:53:27

[permalink] [raw]

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

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

2023-03-04 17:20:03

[permalink] [raw]

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

On Thu, 2 Mar 2023 05:20:38 -0800
Lars-Peter Clausen <[email protected]> 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

2023-03-04 17:26:32

[permalink] [raw]

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

On Thu, 2 Mar 2023 16:23:14 +0200
Andy Shevchenko <[email protected]> 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;

2023-03-04 17:58:05

[permalink] [raw]

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

On Tue, 28 Feb 2023 07:31:51 +0100
Mike Looijmans <[email protected]> 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 <[email protected]>

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

2023-03-06 06:31:55

[permalink] [raw]

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

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: [email protected]
W: http://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 <[email protected]> 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 <[email protected]>
> 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

2023-03-06 11:22:05

[permalink] [raw]

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

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: [email protected]
W: http://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 <[email protected]> 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 <[email protected]>
> 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

2023-03-06 12:09:32

[permalink] [raw]

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

On Sat, Mar 04, 2023 at 05:26:18PM +0000, Jonathan Cameron wrote:
> On Thu, 2 Mar 2023 16:23:14 +0200
> Andy Shevchenko <[email protected]> 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

2023-03-06 12:11:58

[permalink] [raw]

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

On Sat, Mar 04, 2023 at 05:57:51PM +0000, Jonathan Cameron wrote:
> On Tue, 28 Feb 2023 07:31:51 +0100
> Mike Looijmans <[email protected]> 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

2023-03-06 12:16:34

[permalink] [raw]

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

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 <[email protected]> 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

2023-03-06 12:21:09

[permalink] [raw]

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

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 <[email protected]> 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

2023-03-06 12:56:35

[permalink] [raw]

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

2023-03-06 13:20:08

[permalink] [raw]

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

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 <[email protected]> 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

2023-03-12 14:53:10