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]>
---
.../bindings/iio/adc/ti,ads1100.yaml | 37 +++++++++++++++++++
1 file changed, 37 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..ad30af8453a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
@@ -0,0 +1,37 @@
+# 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 <[email protected]>
+
+description: |
+ Datasheet at: https://www.ti.com/lit/gpn/ads1100
+
+properties:
+ compatible:
+ enum:
+ - ti,ads1100
+ - ti,ads1000
+
+required:
+ - compatible
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@49 {
+ compatible = "ti,ads1100";
+ reg = <0x49>;
+ };
+ };
+...
--
2.17.1
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]>
---
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads1100.c | 467 +++++++++++++++++++++++++++++++++++
3 files changed, 480 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads1100.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 63f80d747cbd..bc1918d87f8e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1207,6 +1207,18 @@ 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
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ 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..0b0d3e5b6bd6
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADS1100 - Texas Instruments Analog-to-Digital Converter
+ *
+ * Copyright (c) 2023, Topic Embedded Products
+ *
+ * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
+ */
+
+#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>
+#include <linux/iio/sysfs.h>
+
+#define ADS1100_DRV_NAME "ads1100"
+
+/* 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 (BIT(3) | BIT(2))
+#define ADS1100_DR_SHIFT 2
+/* Gain */
+#define ADS1100_PGA_MASK (BIT(1) | BIT(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_scale[] = {2048, 8192, 16384, 32768};
+static const int ads1100_gain[] = {1, 2, 4, 8};
+
+struct ads1100_data {
+ struct i2c_client *client;
+ struct regulator *reg_vdd;
+ struct mutex lock;
+ u8 config;
+ bool supports_data_rate; /* Only the ADS1100 can select the rate */
+};
+
+static const struct iio_chan_spec ads1100_channel = {
+ .type = IIO_VOLTAGE,
+ .differential = 0,
+ .indexed = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .info_mask_shared_by_all_available =
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .shift = 0,
+ .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;
+
+ 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_set_conv_mode(struct ads1100_data *data, u8 flag)
+{
+ return ads1100_set_config_bits(data, ADS1100_CFG_SC, flag);
+};
+
+static int ads1100_data_rate_index(struct ads1100_data *data)
+{
+ return (data->config & ADS1100_DR_MASK) >> ADS1100_DR_SHIFT;
+}
+
+static int ads1100_pga_index(struct ads1100_data *data)
+{
+ return (data->config & ADS1100_PGA_MASK);
+}
+
+/* Calculate full-scale value */
+static int ads1100_full_scale(struct ads1100_data *data)
+{
+ return ads1100_data_rate_scale[ads1100_data_rate_index(data)] *
+ ads1100_gain[ads1100_pga_index(data)];
+
+}
+
+#ifdef CONFIG_PM
+static int ads1100_set_power_state(struct ads1100_data *data, bool on)
+{
+ int ret;
+ struct device *dev = &data->client->dev;
+
+ if (on) {
+ ret = pm_runtime_resume_and_get(dev);
+ } else {
+ pm_runtime_mark_last_busy(dev);
+ ret = pm_runtime_put_autosuspend(dev);
+ }
+
+ return ret < 0 ? ret : 0;
+}
+
+#else /* !CONFIG_PM */
+
+static int ads1100_set_power_state(struct ads1100_data *data, bool on)
+{
+ return 0;
+}
+
+#endif /* !CONFIG_PM */
+
+static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
+{
+ int ret;
+ u8 buffer[2];
+
+ if (chan != 0)
+ return -EINVAL;
+
+ ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
+ if (ret < 0) {
+ dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+ return ret;
+ }
+
+ *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
+ return 0;
+}
+
+static int ads1100_set_gain(struct ads1100_data *data, int gain)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ads1100_gain); ++i) {
+ if (ads1100_gain[i] == gain) {
+ return ads1100_set_config_bits(
+ data, ADS1100_PGA_MASK, i);
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
+{
+ int i;
+ int 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, i << ADS1100_DR_SHIFT);
+ }
+ }
+
+ return -EINVAL;
+}
+
+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_HARDWAREGAIN:
+ *type = IIO_VAL_INT;
+ *vals = ads1100_gain;
+ *length = ARRAY_SIZE(ads1100_gain);
+ 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);
+
+ dev_info(&data->client->dev, "%s %ld\n", __func__, mask);
+
+ mutex_lock(&data->lock);
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ break;
+
+ ret = ads1100_set_power_state(data, true);
+ if (ret < 0)
+ goto release_direct;
+
+ ret = ads1100_get_adc_result(data, chan->address, val);
+ if (ret < 0) {
+ ads1100_set_power_state(data, false);
+ goto release_direct;
+ }
+
+ ret = ads1100_set_power_state(data, false);
+ if (ret < 0)
+ goto release_direct;
+
+ ret = IIO_VAL_INT;
+release_direct:
+ iio_device_release_direct_mode(indio_dev);
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ ret = regulator_get_voltage(data->reg_vdd);
+ if (ret > 0) {
+ /* full-scale is the supply voltage (microvolts now) */
+ *val = ret / 1000; /* millivolts, range 27000..50000 */
+ *val2 = 1000 * ads1100_full_scale(data);
+ ret = IIO_VAL_FRACTIONAL;
+ }
+ break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = ads1100_data_rate[ads1100_data_rate_index(data)];
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ *val = ads1100_gain[ads1100_pga_index(data)];
+ *val2 = 0;
+ 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_HARDWAREGAIN:
+ ret = ads1100_set_gain(data, val);
+ 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 = !!(buffer[2] & ADS1100_DR_MASK);
+
+ return 0;
+}
+
+static int ads1100_probe(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev;
+ struct ads1100_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->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_DRV_NAME;
+ 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(&client->dev, "vdd");
+ if (IS_ERR(data->reg_vdd))
+ return PTR_ERR(data->reg_vdd);
+
+ ret = regulator_enable(data->reg_vdd);
+ if (ret < 0)
+ return ret;
+
+ ret = ads1100_setup(data);
+ if (ret) {
+ dev_err(&client->dev, "Failed to communicate with device\n");
+ goto exit_regulator;
+ }
+
+ ret = pm_runtime_set_active(&client->dev);
+ if (ret)
+ goto exit_regulator;
+
+ pm_runtime_set_autosuspend_delay(&client->dev, ADS1100_SLEEP_DELAY_MS);
+ pm_runtime_use_autosuspend(&client->dev);
+ pm_runtime_enable(&client->dev);
+
+ ret = iio_device_register(indio_dev);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to register IIO device\n");
+ return ret;
+ }
+
+ return 0;
+
+exit_regulator:
+ regulator_disable(data->reg_vdd);
+ return ret;
+}
+
+static void ads1100_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct ads1100_data *data = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+
+ ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
+
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+}
+
+#ifdef CONFIG_PM
+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_conv_mode(data, 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_conv_mode(data, ADS1100_CONTINUOUS);
+}
+#endif
+
+static const struct dev_pm_ops ads1100_pm_ops = {
+ SET_RUNTIME_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_DRV_NAME,
+ .of_match_table = ads1100_of_match,
+ .pm = &ads1100_pm_ops,
+ },
+ .probe_new = ads1100_probe,
+ .remove = ads1100_remove,
+ .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
On Fri, Feb 17, 2023 at 10:31:28AM +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.
Any Datasheet link available?
> Signed-off-by: Mike Looijmans <[email protected]>
...
> +#define ADS1100_DR_MASK (BIT(3) | BIT(2))
GENMASK()
...
> +#define ADS1100_PGA_MASK (BIT(1) | BIT(0))
Ditto.
...
> +static const int ads1100_data_rate[] = {128, 32, 16, 8};
> +static const int ads1100_data_rate_scale[] = {2048, 8192, 16384, 32768};
> +static const int ads1100_gain[] = {1, 2, 4, 8};
Do you need all of them as tables? They all can be derived from a single table
or without any table at all (just three values).
...
> +static const struct iio_chan_spec ads1100_channel = {
> + .type = IIO_VOLTAGE,
> + .differential = 0,
> + .indexed = 0,
No need.
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .shift = 0,
No need.
> + .endianness = IIO_CPU,
> + },
> + .datasheet_name = "AIN",
> +};
...
> + u8 config = (data->config & ~mask) | value;
Traditional pattern is
u8 config = (data->config & ~mask) | (value & mask);
> +#ifdef CONFIG_PM
Why?
> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
> +{
> + int ret;
> + struct device *dev = &data->client->dev;
> +
> + if (on) {
> + ret = pm_runtime_resume_and_get(dev);
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + ret = pm_runtime_put_autosuspend(dev);
Yes, in !CONFIG_PM this will return an error, but why do you care?
> + }
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +#else /* !CONFIG_PM */
> +
> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
> +{
> + return 0;
> +}
> +
> +#endif /* !CONFIG_PM */
...
> +static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
> +{
> + int ret;
> + u8 buffer[2];
__be16 buffer;
> +
> + if (chan != 0)
> + return -EINVAL;
> +
> + ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> + if (ret < 0) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return ret;
> + }
> +
> + *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
(s16)be16_to_cpu();
But (s16) looks suspicious. Should you use sign_extend32()?
> + return 0;
> +}
...
> +static int ads1100_set_gain(struct ads1100_data *data, int gain)
> +{
> + int i;
unsigned
> + for (i = 0; i < ARRAY_SIZE(ads1100_gain); ++i) {
Pre-increment in the loops is non-standard in the kernel.
Why do you need that?
> + if (ads1100_gain[i] == gain) {
> + return ads1100_set_config_bits(
> + data, ADS1100_PGA_MASK, i);
Strange indentation.
> + }
> + }
> +
> + return -EINVAL;
> +}
...
> +static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
Same comments as per above.
...
> + dev_info(&data->client->dev, "%s %ld\n", __func__, mask);
Useless noise in the logs.
...
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to register IIO device\n");
> + return ret;
return dev_err_probe();
> + }
...
> +#ifdef CONFIG_PM
Drop it and use proper macros below.
> +#endif
...
> +static const struct dev_pm_ops ads1100_pm_ops = {
> + SET_RUNTIME_PM_OPS(ads1100_runtime_suspend,
> + ads1100_runtime_resume, NULL)
> +};
...here and...
...
> + .pm = &ads1100_pm_ops,
...here.
...
> +
Redundant blank line.
> +module_i2c_driver(ads1100_driver);
--
With Best Regards,
Andy Shevchenko
On Fri, 17 Feb 2023 10:31:27 +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.
>
> Signed-off-by: Mike Looijmans <[email protected]>
>
> ---
>
> .../bindings/iio/adc/ti,ads1100.yaml | 37 +++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/ti,ads1100.example.dtb: adc@49: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Thanks for your quick feedback. Replies below (skipping signature added
by borked mailserver)
I'll post a v2, want to do some testing first with the changes and I'll
have hardware access by the end of next week.
No further comment from me means I agree and will change as requested.
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 17-02-2023 13:29, Andy Shevchenko wrote:
> On Fri, Feb 17, 2023 at 10:31:28AM +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.
> Any Datasheet link available?
Yes, will add a friendly link.
> ...
>
>> +static const int ads1100_data_rate[] = {128, 32, 16, 8};
>> +static const int ads1100_data_rate_scale[] = {2048, 8192, 16384, 32768};
>> +static const int ads1100_gain[] = {1, 2, 4, 8};
> Do you need all of them as tables? They all can be derived from a single table
> or without any table at all (just three values).
I think the "data_rate" tables make the driver smaller in size, it's not
a straight power-of-two list. I want them anyway for passing as "avail"
sequences.
The "gain" is a simple power-of-two and might be replaced with code, but
makes the "avail" more complex. Even in this case, the code to generate
the list will be larger than the list itself, and I need the list in
memory for the IIO_AVAIL function anyway.
> ...
>
>> +#ifdef CONFIG_PM
> Why?
I based this on the ADS1015 driver. Which appears to be outdated, I'll
convert to DEFINE_RUNTIME_DEV_PM_OPS
+ int ret;
>> + u8 buffer[2];
> __be16 buffer;
>
>> +
>> + if (chan != 0)
>> + return -EINVAL;
>> +
>> + ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
> (s16)be16_to_cpu();
>
> But (s16) looks suspicious. Should you use sign_extend32()?
The chip always returns a 16-bit 2's complement value, even when only 12
bits are actually used. I'll use sign_extend anyway, just to improve
readability and take away doubts such as these.
--
Mike Looijmans
On Fri, 17 Feb 2023 10:31:28 +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,
I probably overlapped a lot in here with what Andy said, so just ignore
anything you've already fixed.
Biggest thing is HARDWAREGAIN.
That is very rarely used with ADCs. It exists to support cases where
the gain is not directly coupled to the thing being measured (light
sensitivity of a time of flight sensor for example). Userspace expects
to use SCALE to both control amplifier gains and to multiply with
the _raw value to get a value in the real world units.
It's a bit fiddly to do the computation, but typically at start up time
you work out what the combination of each PGA gain and the reference
voltage means for the scales available and stash that somewhere to use later.
As per the docs in Documentation/ABI/testing/sysfs-bus-iio userspace is
expected to get real units (here millivolts) from
(_raw + _offset) * _scale
where _offset = 0 if not provided. Hence any front end amplification needs
to be taken into account for _scale and _scale_available
Thanks,
Jonathan
>
> ---
>
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads1100.c | 467 +++++++++++++++++++++++++++++++++++
> 3 files changed, 480 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads1100.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 63f80d747cbd..bc1918d87f8e 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1207,6 +1207,18 @@ 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
> + select IIO_BUFFER
No sign of buffers yet so shouldn't need the dependency
> + select IIO_TRIGGERED_BUFFER
> + 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..0b0d3e5b6bd6
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -0,0 +1,467 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADS1100 - Texas Instruments Analog-to-Digital Converter
> + *
> + * Copyright (c) 2023, Topic Embedded Products
> + *
> + * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
> + */
> +
> +#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>
> +#include <linux/iio/sysfs.h>
I doubt you need this last header. It's only needed for
non standard ABI definitions.
> +
> +#define ADS1100_DRV_NAME "ads1100"
Just put this inline. There is no particular
reason the driver name and the iio device name will be the
same so I'd prefer to see the string in each place rather
than using a define hidden up here.
> +
> +/* 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 (BIT(3) | BIT(2))
GENMASK(3, 2)
same for similar cases.
> +#define ADS1100_DR_SHIFT 2
> +/* Gain */
> +#define ADS1100_PGA_MASK (BIT(1) | BIT(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_scale[] = {2048, 8192, 16384, 32768};
> +static const int ads1100_gain[] = {1, 2, 4, 8};
> +
> +struct ads1100_data {
> + struct i2c_client *client;
> + struct regulator *reg_vdd;
> + struct mutex lock;
> + u8 config;
> + bool supports_data_rate; /* Only the ADS1100 can select the rate */
> +};
> +
> +static const struct iio_chan_spec ads1100_channel = {
> + .type = IIO_VOLTAGE,
> + .differential = 0,
> + .indexed = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
Hardware gain is normally only used for cases where the gain is not
directly related to scale. Things like the gain on a signal that is
being used for measuring time of flight. Here you should probably
just be using SCALE.
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .shift = 0,
Default of 0 is kind of obvious for shift, so no need to explicitly set it.
C will fill it with a 0 anyway.
> + .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;
> +
> + 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_set_conv_mode(struct ads1100_data *data, u8 flag)
> +{
> + return ads1100_set_config_bits(data, ADS1100_CFG_SC, flag);
This wrapper doesn't feel very useful. Make the call directly.
> +};
> +
> +static int ads1100_data_rate_index(struct ads1100_data *data)
> +{
> + return (data->config & ADS1100_DR_MASK) >> ADS1100_DR_SHIFT;
FIELD_GET()
> +}
> +
> +static int ads1100_pga_index(struct ads1100_data *data)
> +{
> + return (data->config & ADS1100_PGA_MASK);
No need for brackets
Also just use FIELD_GET() instead.
> +}
> +
> +/* Calculate full-scale value */
> +static int ads1100_full_scale(struct ads1100_data *data)
> +{
> + return ads1100_data_rate_scale[ads1100_data_rate_index(data)] *
> + ads1100_gain[ads1100_pga_index(data)];
> +
> +}
> +
> +#ifdef CONFIG_PM
> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
> +{
> + int ret;
> + struct device *dev = &data->client->dev;
This function makes the code harder to read. Just make the
pm_runtime_* calls directly but for the put don't check the
return value (as it will be set to an error when runtime PM not built)
It also tends to be unhelpful to fail the caller based on failure
to autosuspend.
> +
> + if (on) {
> + ret = pm_runtime_resume_and_get(dev);
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + ret = pm_runtime_put_autosuspend(dev);
> + }
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +#else /* !CONFIG_PM */
> +
> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
> +{
> + return 0;
> +}
> +
> +#endif /* !CONFIG_PM */
> +
> +static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
> +{
> + int ret;
> + u8 buffer[2];
__le16 buffer;
> +
> + if (chan != 0)
> + return -EINVAL;
> +
> + ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> + if (ret < 0) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return ret;
> + }
> +
> + *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
*val = sign_extend32(le16_to_cpu(), 15);
> + return 0;
> +}
> +
> +static int ads1100_set_gain(struct ads1100_data *data, int gain)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ads1100_gain); ++i) {
> + if (ads1100_gain[i] == gain) {
> + return ads1100_set_config_bits(
> + data, ADS1100_PGA_MASK, i);
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
> +{
> + int i;
> + int 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, i << ADS1100_DR_SHIFT);
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +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_HARDWAREGAIN:
> + *type = IIO_VAL_INT;
> + *vals = ads1100_gain;
> + *length = ARRAY_SIZE(ads1100_gain);
> + 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);
> +
> + dev_info(&data->client->dev, "%s %ld\n", __func__, mask);
Definitely shouldn't be having prints in here. I guess left over
from debugging during development.
> +
> + mutex_lock(&data->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + break;
> +
> + ret = ads1100_set_power_state(data, true);
As above. Make the runtime pm calls directly for better readablity.
> + if (ret < 0)
> + goto release_direct;
> +
> + ret = ads1100_get_adc_result(data, chan->address, val);
> + if (ret < 0) {
> + ads1100_set_power_state(data, false);
As you won't be checking pm_runtime_put_autosuspend() return value
(see above) you can do this as
ret = ads110_get_adc_result(...)
pm_runtime_put_autosuspend()
if (ret < 0) {
}
rather than having the autosuspend call in two places.
> + goto release_direct;
> + }
> +
> + ret = ads1100_set_power_state(data, false);
> + if (ret < 0)
> + goto release_direct;
> +
> + ret = IIO_VAL_INT;
> +release_direct:
labels within case statements are not nice to read. I'd just factor
some of this code out to a separate function so you don't need to
do that. Ideally leave the claim_direct out side of the function
and then all the returns within it become simpler.
> + iio_device_release_direct_mode(indio_dev);
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + ret = regulator_get_voltage(data->reg_vdd);
> + if (ret > 0) {
> + /* full-scale is the supply voltage (microvolts now) */
> + *val = ret / 1000; /* millivolts, range 27000..50000 */
> + *val2 = 1000 * ads1100_full_scale(data);
This doesn't seem to take into account the PGA gain? Userspace is only going
to apply _raw * _scale to get the real world units.
> + ret = IIO_VAL_FRACTIONAL;
> + }
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = ads1100_data_rate[ads1100_data_rate_index(data)];
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + *val = ads1100_gain[ads1100_pga_index(data)];
> + *val2 = 0;
> + 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_HARDWAREGAIN:
> + ret = ads1100_set_gain(data, val);
As above, would expect this to be SCALE for an ADC.
> + 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 = !!(buffer[2] & ADS1100_DR_MASK);
FIELD_GET() != 0
is probably more readable.
> +
> + return 0;
> +}
> +
> +static int ads1100_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct ads1100_data *data;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->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_DRV_NAME;
> + 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(&client->dev, "vdd");
> + if (IS_ERR(data->reg_vdd))
> + return PTR_ERR(data->reg_vdd);
> +
> + ret = regulator_enable(data->reg_vdd);
> + if (ret < 0)
> + return ret;
As below. You should be able to use devm_add_action_or_reset()
to disable the regulator in the remove path (and on error after this
point).
> +
> + ret = ads1100_setup(data);
> + if (ret) {
> + dev_err(&client->dev, "Failed to communicate with device\n");
> + goto exit_regulator;
> + }
> +
> + ret = pm_runtime_set_active(&client->dev);
> + if (ret)
> + goto exit_regulator;
> +
> + pm_runtime_set_autosuspend_delay(&client->dev, ADS1100_SLEEP_DELAY_MS);
> + pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_enable(&client->dev);
As below comment suggests, you can use devm_pm_runtime_enable()
which also fixes that fact you aren't turning off autosuspend
(which is a bug as the documentation for use_autosuspend states)
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to register IIO device\n");
> + return ret;
> + }
> +
> + return 0;
> +
> +exit_regulator:
> + regulator_disable(data->reg_vdd);
> + return ret;
> +}
> +
> +static void ads1100_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct ads1100_data *data = iio_priv(indio_dev);
> +
Always read a remove path with it in mind that runtime pm might not
be in use at all. Everything must still work.
As such, I'd expect to see a disable of the regulator in here
(or even better in a devm_add_action_or_reset())
Also would expect to first ensure the device is in a known
state with a pm_runtime_get() - which will be a noop if
runtime pm isn't in use (and hence the device is powered up between
probe and remove)
> + iio_device_unregister(indio_dev);
> +
> + ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
> +
> + pm_runtime_disable(&client->dev);
I'd expect runtime pm to be disable before messing with the conv mode.
With a little care you can use devm_runtime_pm_enable()
> + pm_runtime_set_suspended(&client->dev);
I'd expect this to be handled immediately after the
set of the conv mode to suspended. Which will be fine if you
have moved the runtime pm before that.
Once those are together you can just group them in a devm_add_action_or_reset()
callback and make the whole remove process devm managed.
> +}
> +
> +#ifdef CONFIG_PM
Addressed in other branch of this thread. So I won't comment more.
> +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_conv_mode(data, 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_conv_mode(data, ADS1100_CONTINUOUS);
> +}
> +#endif
> +
> +static const struct dev_pm_ops ads1100_pm_ops = {
> + SET_RUNTIME_PM_OPS(ads1100_runtime_suspend,
> + ads1100_runtime_resume, NULL)
> +};
> +
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 v2"...
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 18-02-2023 17:48, Jonathan Cameron wrote:
> On Fri, 17 Feb 2023 10:31:28 +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,
>
> I probably overlapped a lot in here with what Andy said, so just ignore
> anything you've already fixed.
>
> Biggest thing is HARDWAREGAIN.
> That is very rarely used with ADCs. It exists to support cases where
> the gain is not directly coupled to the thing being measured (light
> sensitivity of a time of flight sensor for example). Userspace expects
> to use SCALE to both control amplifier gains and to multiply with
> the _raw value to get a value in the real world units.
>
> It's a bit fiddly to do the computation, but typically at start up time
> you work out what the combination of each PGA gain and the reference
> voltage means for the scales available and stash that somewhere to use later.
Complicating factor with this ADC is that the final gain value depends
on the sampling rate as well as the power supply voltage. Which would
lead to the "available" list being different after changing the sample
rate and confusing userspace. If a userspace app would read the
available sample rates and gains, and then proceeded to set them, the
results would be weird, as the order in which they were set would
matter. Setting the gain after setting the sample rate would likely
result in an EINVAL error because the selected gain is no longer applicable.
To me it seemed more logical to provide the analog amplification and
sample rate as separate, unrelated values.
> As per the docs in Documentation/ABI/testing/sysfs-bus-iio userspace is
> expected to get real units (here millivolts) from
>
> (_raw + _offset) * _scale
> where _offset = 0 if not provided. Hence any front end amplification needs
> to be taken into account for _scale and _scale_available
Have to check the math, but I think I made a mistake with the final
result being "Volts" instead of "millivolts".
>
> Thanks,
>
> Jonathan
>
>> ---
>>
>> drivers/iio/adc/Kconfig | 12 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ti-ads1100.c | 467 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 480 insertions(+)
>> create mode 100644 drivers/iio/adc/ti-ads1100.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 63f80d747cbd..bc1918d87f8e 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -1207,6 +1207,18 @@ 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
>> + select IIO_BUFFER
> No sign of buffers yet so shouldn't need the dependency
>
>> + select IIO_TRIGGERED_BUFFER
>> + 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..0b0d3e5b6bd6
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads1100.c
>> @@ -0,0 +1,467 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * ADS1100 - Texas Instruments Analog-to-Digital Converter
>> + *
>> + * Copyright (c) 2023, Topic Embedded Products
>> + *
>> + * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
>> + */
>> +
>> +#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>
>> +#include <linux/iio/sysfs.h>
> I doubt you need this last header. It's only needed for
> non standard ABI definitions.
>
>> +
>> +#define ADS1100_DRV_NAME "ads1100"
> Just put this inline. There is no particular
> reason the driver name and the iio device name will be the
> same so I'd prefer to see the string in each place rather
> than using a define hidden up here.
>> +
>> +/* 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 (BIT(3) | BIT(2))
> GENMASK(3, 2)
>
> same for similar cases.
>
>> +#define ADS1100_DR_SHIFT 2
>> +/* Gain */
>> +#define ADS1100_PGA_MASK (BIT(1) | BIT(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_scale[] = {2048, 8192, 16384, 32768};
>> +static const int ads1100_gain[] = {1, 2, 4, 8};
>> +
>> +struct ads1100_data {
>> + struct i2c_client *client;
>> + struct regulator *reg_vdd;
>> + struct mutex lock;
>> + u8 config;
>> + bool supports_data_rate; /* Only the ADS1100 can select the rate */
>> +};
>> +
>> +static const struct iio_chan_spec ads1100_channel = {
>> + .type = IIO_VOLTAGE,
>> + .differential = 0,
>> + .indexed = 0,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_shared_by_all =
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> + .info_mask_shared_by_all_available =
>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> Hardware gain is normally only used for cases where the gain is not
> directly related to scale. Things like the gain on a signal that is
> being used for measuring time of flight. Here you should probably
> just be using SCALE.
In this case, SCALE depends on SAMP_FREQ as well as GAIN. Which will be
very confusing to userspace.
Hoping for some leeway here...
>
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> + .scan_type = {
>> + .sign = 's',
>> + .realbits = 16,
>> + .storagebits = 16,
>> + .shift = 0,
> Default of 0 is kind of obvious for shift, so no need to explicitly set it.
> C will fill it with a 0 anyway.
>
>> + .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;
>> +
>> + 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_set_conv_mode(struct ads1100_data *data, u8 flag)
>> +{
>> + return ads1100_set_config_bits(data, ADS1100_CFG_SC, flag);
> This wrapper doesn't feel very useful. Make the call directly.
>
>
>> +};
>> +
>> +static int ads1100_data_rate_index(struct ads1100_data *data)
>> +{
>> + return (data->config & ADS1100_DR_MASK) >> ADS1100_DR_SHIFT;
> FIELD_GET()
>> +}
>> +
>> +static int ads1100_pga_index(struct ads1100_data *data)
>> +{
>> + return (data->config & ADS1100_PGA_MASK);
> No need for brackets
>
> Also just use FIELD_GET() instead.
>
>> +}
>> +
>> +/* Calculate full-scale value */
>> +static int ads1100_full_scale(struct ads1100_data *data)
>> +{
>> + return ads1100_data_rate_scale[ads1100_data_rate_index(data)] *
>> + ads1100_gain[ads1100_pga_index(data)];
>> +
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
>> +{
>> + int ret;
>> + struct device *dev = &data->client->dev;
>
> This function makes the code harder to read. Just make the
> pm_runtime_* calls directly but for the put don't check the
> return value (as it will be set to an error when runtime PM not built)
>
> It also tends to be unhelpful to fail the caller based on failure
> to autosuspend.
Power management was copied from the ads1015 driver, which in retrospect
isn't up to modern standards. Needs some more overhaul.
>
>> +
>> + if (on) {
>> + ret = pm_runtime_resume_and_get(dev);
>> + } else {
>> + pm_runtime_mark_last_busy(dev);
>> + ret = pm_runtime_put_autosuspend(dev);
>> + }
>> +
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +#else /* !CONFIG_PM */
>> +
>> +static int ads1100_set_power_state(struct ads1100_data *data, bool on)
>> +{
>> + return 0;
>> +}
>> +
>> +#endif /* !CONFIG_PM */
>> +
>> +static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>> +{
>> + int ret;
>> + u8 buffer[2];
> __le16 buffer;
>
>> +
>> + if (chan != 0)
>> + return -EINVAL;
>> +
>> + ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
> *val = sign_extend32(le16_to_cpu(), 15);
Looks be16 to me though...
>
>> + return 0;
>> +}
>> +
>> +static int ads1100_set_gain(struct ads1100_data *data, int gain)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(ads1100_gain); ++i) {
>> + if (ads1100_gain[i] == gain) {
>> + return ads1100_set_config_bits(
>> + data, ADS1100_PGA_MASK, i);
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>> +{
>> + int i;
>> + int 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, i << ADS1100_DR_SHIFT);
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +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_HARDWAREGAIN:
>> + *type = IIO_VAL_INT;
>> + *vals = ads1100_gain;
>> + *length = ARRAY_SIZE(ads1100_gain);
>> + 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);
>> +
>> + dev_info(&data->client->dev, "%s %ld\n", __func__, mask);
> Definitely shouldn't be having prints in here. I guess left over
> from debugging during development.
Guessed correctly yeah.
>
>> +
>> + mutex_lock(&data->lock);
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> + break;
>> +
>> + ret = ads1100_set_power_state(data, true);
> As above. Make the runtime pm calls directly for better readablity.
>
>> + if (ret < 0)
>> + goto release_direct;
>> +
>> + ret = ads1100_get_adc_result(data, chan->address, val);
>> + if (ret < 0) {
>> + ads1100_set_power_state(data, false);
> As you won't be checking pm_runtime_put_autosuspend() return value
> (see above) you can do this as
>
> ret = ads110_get_adc_result(...)
> pm_runtime_put_autosuspend()
> if (ret < 0) {
> }
> rather than having the autosuspend call in two places.
Makes sense. I'll peek at other drivers for inspiration.
>
>> + goto release_direct;
>> + }
>> +
>> + ret = ads1100_set_power_state(data, false);
>> + if (ret < 0)
>> + goto release_direct;
>> +
>> + ret = IIO_VAL_INT;
>> +release_direct:
> labels within case statements are not nice to read. I'd just factor
> some of this code out to a separate function so you don't need to
> do that. Ideally leave the claim_direct out side of the function
> and then all the returns within it become simpler.
>
>
>> + iio_device_release_direct_mode(indio_dev);
>> + break;
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = regulator_get_voltage(data->reg_vdd);
>> + if (ret > 0) {
>> + /* full-scale is the supply voltage (microvolts now) */
>> + *val = ret / 1000; /* millivolts, range 27000..50000 */
>> + *val2 = 1000 * ads1100_full_scale(data);
> This doesn't seem to take into account the PGA gain? Userspace is only going
> to apply _raw * _scale to get the real world units.
The "full_scale" method also applies the PGA.
>
>> + ret = IIO_VAL_FRACTIONAL;
>> + }
>> + break;
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + *val = ads1100_data_rate[ads1100_data_rate_index(data)];
>> + ret = IIO_VAL_INT;
>> + break;
>> + case IIO_CHAN_INFO_HARDWAREGAIN:
>> + *val = ads1100_gain[ads1100_pga_index(data)];
>> + *val2 = 0;
>> + 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_HARDWAREGAIN:
>> + ret = ads1100_set_gain(data, val);
> As above, would expect this to be SCALE for an ADC.
>
>> + 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 = !!(buffer[2] & ADS1100_DR_MASK);
> FIELD_GET() != 0
> is probably more readable.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int ads1100_probe(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct ads1100_data *data;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->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_DRV_NAME;
>> + 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(&client->dev, "vdd");
>> + if (IS_ERR(data->reg_vdd))
>> + return PTR_ERR(data->reg_vdd);
>> +
>> + ret = regulator_enable(data->reg_vdd);
>> + if (ret < 0)
>> + return ret;
> As below. You should be able to use devm_add_action_or_reset()
> to disable the regulator in the remove path (and on error after this
> point).
>
>> +
>> + ret = ads1100_setup(data);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to communicate with device\n");
>> + goto exit_regulator;
>> + }
>> +
>> + ret = pm_runtime_set_active(&client->dev);
>> + if (ret)
>> + goto exit_regulator;
>> +
>> + pm_runtime_set_autosuspend_delay(&client->dev, ADS1100_SLEEP_DELAY_MS);
>> + pm_runtime_use_autosuspend(&client->dev);
>> + pm_runtime_enable(&client->dev);
> As below comment suggests, you can use devm_pm_runtime_enable()
> which also fixes that fact you aren't turning off autosuspend
> (which is a bug as the documentation for use_autosuspend states)
>
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "Failed to register IIO device\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +
>> +exit_regulator:
>> + regulator_disable(data->reg_vdd);
>> + return ret;
>> +}
>> +
>> +static void ads1100_remove(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> + struct ads1100_data *data = iio_priv(indio_dev);
>> +
> Always read a remove path with it in mind that runtime pm might not
> be in use at all. Everything must still work.
> As such, I'd expect to see a disable of the regulator in here
> (or even better in a devm_add_action_or_reset())
>
> Also would expect to first ensure the device is in a known
> state with a pm_runtime_get() - which will be a noop if
> runtime pm isn't in use (and hence the device is powered up between
> probe and remove)
>
>
>> + iio_device_unregister(indio_dev);
>> +
>> + ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
>> +
>> + pm_runtime_disable(&client->dev);
> I'd expect runtime pm to be disable before messing with the conv mode.
> With a little care you can use devm_runtime_pm_enable()
Setting the conv mode involves I2C traffic. After runtime_disable, the
power supply to the chip may have been disabled, leading to
communication errors on the I2C bus. Hence I thought it appropriate to
write the config register before turning off power.
>
>> + pm_runtime_set_suspended(&client->dev);
> I'd expect this to be handled immediately after the
> set of the conv mode to suspended. Which will be fine if you
> have moved the runtime pm before that.
>
> Once those are together you can just group them in a devm_add_action_or_reset()
> callback and make the whole remove process devm managed.
I'll try to do that - using devm is surely the better option.
>
>> +}
>> +
>> +#ifdef CONFIG_PM
> Addressed in other branch of this thread. So I won't comment more.
>
>> +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_conv_mode(data, 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_conv_mode(data, ADS1100_CONTINUOUS);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops ads1100_pm_ops = {
>> + SET_RUNTIME_PM_OPS(ads1100_runtime_suspend,
>> + ads1100_runtime_resume, NULL)
>> +};
>> +
--
Mike Looijmans
On Sat, 25 Feb 2023 12:03:05 +0100
Mike Looijmans <[email protected]> wrote:
> 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 v2"...
One 'nice to have' when replying where you have chunks like that
is to just crop them out so it's easier to spot the interesting bits.
I've done that below.
>
>
>
> 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 18-02-2023 17:48, Jonathan Cameron wrote:
> > On Fri, 17 Feb 2023 10:31:28 +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,
> >
> > I probably overlapped a lot in here with what Andy said, so just ignore
> > anything you've already fixed.
> >
> > Biggest thing is HARDWAREGAIN.
> > That is very rarely used with ADCs. It exists to support cases where
> > the gain is not directly coupled to the thing being measured (light
> > sensitivity of a time of flight sensor for example). Userspace expects
> > to use SCALE to both control amplifier gains and to multiply with
> > the _raw value to get a value in the real world units.
> >
> > It's a bit fiddly to do the computation, but typically at start up time
> > you work out what the combination of each PGA gain and the reference
> > voltage means for the scales available and stash that somewhere to use later.
>
> Complicating factor with this ADC is that the final gain value depends
> on the sampling rate as well as the power supply voltage. Which would
> lead to the "available" list being different after changing the sample
> rate and confusing userspace. If a userspace app would read the
> available sample rates and gains, and then proceeded to set them, the
> results would be weird, as the order in which they were set would
> matter. Setting the gain after setting the sample rate would likely
> result in an EINVAL error because the selected gain is no longer applicable.
>
> To me it seemed more logical to provide the analog amplification and
> sample rate as separate, unrelated values.
It may be logical, but it isn't how the IIO ABI has ever worked and it doesn't
extend to more complex cases. It's in general true that a PGA will result
in a change to the scale that userspace needs to apply. There are devices
where it doesn't. There are lots of things that at first glance 'could'
affect scale but often do it in complex non linear ways that userspace
simply can't know about - hence if we are pushing calculations into userspace
we need it to just be (_raw + _offset) * _scale.
Note that there is some wiggle room with how raw "raw" is.
There are two solutions that have been taken in drivers.
1) The above software flow is broken as any ABI write in IIO is allowed
to affect other ABI elements. This is less than ideal though.
2) Let the scale free float. So the attempt is to keep as close as possible
to the set value as other things change (i.e. the sampling frequency).
The scale_avail reflects current settings of everything else, and indeed
changes with other ABI wirtes (this is quite common) but the interface is
made more intuitive by matching as closely as possible. Thus if you change the
sampling rate and the scale changes then you attempt to modify the PGA
to keep it approximately the same. Obviously it clamps at end points
but nothing we can do about that.
However, having said that, I don't 'think' we need either here...
A common thing to do for scale vs sampling rate (which is typically
either oversampling based, or based on another SAR cycle) is to just shift
the raw data to incorporate it - a common sensor design is to justify it
so that the unused bits are the LSBs - so may be a case of just not shifting
it to compensate for the datarate change.. That's not true here if I read
the datasheet correctly, but a simple
sysfs raw read value == raw_value << (16 - bits for data rate) should fix that.
Interesting the datasheet argues they deliberately right aligned and sign extended
to allow oversampling without shifts, though counter argument is they made everyone
who wants a real scale apply a shift. I guess it depends on the expected use case.
> >> +
> >> +static const struct iio_chan_spec ads1100_channel = {
> >> + .type = IIO_VOLTAGE,
> >> + .differential = 0,
> >> + .indexed = 0,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> + .info_mask_shared_by_all =
> >> + BIT(IIO_CHAN_INFO_SCALE) |
> >> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> >> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> >> + .info_mask_shared_by_all_available =
> >> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> > Hardware gain is normally only used for cases where the gain is not
> > directly related to scale. Things like the gain on a signal that is
> > being used for measuring time of flight. Here you should probably
> > just be using SCALE.
>
> In this case, SCALE depends on SAMP_FREQ as well as GAIN. Which will be
> very confusing to userspace.
>
> Hoping for some leeway here...
Sorry no. Though I think applying the shift as mentioned above deals
with your data rate dependent scaling and makes this all a lot easier.
> >> +
> >> + if (chan != 0)
> >> + return -EINVAL;
> >> +
> >> + ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> >> + if (ret < 0) {
> >> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + *val = (s16)(((u16)buffer[0] << 8) | buffer[1]);
> > *val = sign_extend32(le16_to_cpu(), 15);
>
> Looks be16 to me though...
:) Oops - you are of course correct.
...
> >> + case IIO_CHAN_INFO_SCALE:
> >> + ret = regulator_get_voltage(data->reg_vdd);
> >> + if (ret > 0) {
> >> + /* full-scale is the supply voltage (microvolts now) */
> >> + *val = ret / 1000; /* millivolts, range 27000..50000 */
> >> + *val2 = 1000 * ads1100_full_scale(data);
> > This doesn't seem to take into account the PGA gain? Userspace is only going
> > to apply _raw * _scale to get the real world units.
>
> The "full_scale" method also applies the PGA.
Indeed. I misread that code.
...
> >> + iio_device_unregister(indio_dev);
> >> +
> >> + ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
> >> +
> >> + pm_runtime_disable(&client->dev);
> > I'd expect runtime pm to be disable before messing with the conv mode.
> > With a little care you can use devm_runtime_pm_enable()
>
> Setting the conv mode involves I2C traffic. After runtime_disable, the
> power supply to the chip may have been disabled, leading to
> communication errors on the I2C bus. Hence I thought it appropriate to
> write the config register before turning off power.
pm_runtime_disable() is disabling the runtime management of the power
not the power itself. That you need to do after turning off the
management (thus avoiding any races)
>
>
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 25-02-2023 18:01, Jonathan Cameron wrote:
> On Sat, 25 Feb 2023 12:03:05 +0100
> Mike Looijmans <[email protected]> wrote:
>
>> 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 v2"...
> One 'nice to have' when replying where you have chunks like that
> is to just crop them out so it's easier to spot the interesting bits.
>
> I've done that below.
>
>> On 18-02-2023 17:48, Jonathan Cameron wrote:
>>> On Fri, 17 Feb 2023 10:31:28 +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,
>>>
>>> I probably overlapped a lot in here with what Andy said, so just ignore
>>> anything you've already fixed.
>>>
>>> Biggest thing is HARDWAREGAIN.
>>> That is very rarely used with ADCs. It exists to support cases where
>>> the gain is not directly coupled to the thing being measured (light
>>> sensitivity of a time of flight sensor for example). Userspace expects
>>> to use SCALE to both control amplifier gains and to multiply with
>>> the _raw value to get a value in the real world units.
>>>
>>> It's a bit fiddly to do the computation, but typically at start up time
>>> you work out what the combination of each PGA gain and the reference
>>> voltage means for the scales available and stash that somewhere to use later.
>> Complicating factor with this ADC is that the final gain value depends
>> on the sampling rate as well as the power supply voltage. Which would
>> lead to the "available" list being different after changing the sample
>> rate and confusing userspace. If a userspace app would read the
>> available sample rates and gains, and then proceeded to set them, the
>> results would be weird, as the order in which they were set would
>> matter. Setting the gain after setting the sample rate would likely
>> result in an EINVAL error because the selected gain is no longer applicable.
>>
>> To me it seemed more logical to provide the analog amplification and
>> sample rate as separate, unrelated values.
> It may be logical, but it isn't how the IIO ABI has ever worked and it doesn't
> extend to more complex cases. It's in general true that a PGA will result
> in a change to the scale that userspace needs to apply. There are devices
> where it doesn't. There are lots of things that at first glance 'could'
> affect scale but often do it in complex non linear ways that userspace
> simply can't know about - hence if we are pushing calculations into userspace
> we need it to just be (_raw + _offset) * _scale.
> Note that there is some wiggle room with how raw "raw" is.
>
> There are two solutions that have been taken in drivers.
> 1) The above software flow is broken as any ABI write in IIO is allowed
> to affect other ABI elements. This is less than ideal though.
> 2) Let the scale free float. So the attempt is to keep as close as possible
> to the set value as other things change (i.e. the sampling frequency).
> The scale_avail reflects current settings of everything else, and indeed
> changes with other ABI wirtes (this is quite common) but the interface is
> made more intuitive by matching as closely as possible. Thus if you change the
> sampling rate and the scale changes then you attempt to modify the PGA
> to keep it approximately the same. Obviously it clamps at end points
> but nothing we can do about that.
>
> However, having said that, I don't 'think' we need either here...
> A common thing to do for scale vs sampling rate (which is typically
> either oversampling based, or based on another SAR cycle) is to just shift
> the raw data to incorporate it - a common sensor design is to justify it
> so that the unused bits are the LSBs - so may be a case of just not shifting
> it to compensate for the datarate change.. That's not true here if I read
> the datasheet correctly, but a simple
> sysfs raw read value == raw_value << (16 - bits for data rate) should fix that.
I agree, a bit of shifting (pun intended) is by far the better solution.
> Interesting the datasheet argues they deliberately right aligned and sign extended
> to allow oversampling without shifts, though counter argument is they made everyone
> who wants a real scale apply a shift. I guess it depends on the expected use case.
My guess is that the chip internally always runs at 128Hz and simply
adds the sampled values together in a 16-bit register for the lower
sampling rates. Someone came up with that datasheet text later on.
>>>> +
>>>> +static const struct iio_chan_spec ads1100_channel = {
>>>> + .type = IIO_VOLTAGE,
>>>> + .differential = 0,
>>>> + .indexed = 0,
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> + .info_mask_shared_by_all =
>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>> + .info_mask_shared_by_all_available =
>>>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>>> Hardware gain is normally only used for cases where the gain is not
>>> directly related to scale. Things like the gain on a signal that is
>>> being used for measuring time of flight. Here you should probably
>>> just be using SCALE.
>> In this case, SCALE depends on SAMP_FREQ as well as GAIN. Which will be
>> very confusing to userspace.
>>
>> Hoping for some leeway here...
> Sorry no. Though I think applying the shift as mentioned above deals
> with your data rate dependent scaling and makes this all a lot easier.
True.
...
>>>> + iio_device_unregister(indio_dev);
>>>> +
>>>> + ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
>>>> +
>>>> + pm_runtime_disable(&client->dev);
>>> I'd expect runtime pm to be disable before messing with the conv mode.
>>> With a little care you can use devm_runtime_pm_enable()
>> Setting the conv mode involves I2C traffic. After runtime_disable, the
>> power supply to the chip may have been disabled, leading to
>> communication errors on the I2C bus. Hence I thought it appropriate to
>> write the config register before turning off power.
> pm_runtime_disable() is disabling the runtime management of the power
> not the power itself. That you need to do after turning off the
> management (thus avoiding any races)
Ah. But does that mean I'd need to disable the Vdd power supply here as
well?
--
Mike Looijmans
On Sun, 26 Feb 2023 09:24:02 +0100
Mike Looijmans <[email protected]> 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: [email protected]
> W: http://www.topic.nl
>
> Please consider the environment before printing this e-mail
> On 25-02-2023 18:01, Jonathan Cameron wrote:
> > On Sat, 25 Feb 2023 12:03:05 +0100
> > Mike Looijmans <[email protected]> wrote:
> >
> >> 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 v2"...
> > One 'nice to have' when replying where you have chunks like that
> > is to just crop them out so it's easier to spot the interesting bits.
> >
> > I've done that below.
> >
> >> On 18-02-2023 17:48, Jonathan Cameron wrote:
> >>> On Fri, 17 Feb 2023 10:31:28 +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,
> >>>
> >>> I probably overlapped a lot in here with what Andy said, so just ignore
> >>> anything you've already fixed.
> >>>
> >>> Biggest thing is HARDWAREGAIN.
> >>> That is very rarely used with ADCs. It exists to support cases where
> >>> the gain is not directly coupled to the thing being measured (light
> >>> sensitivity of a time of flight sensor for example). Userspace expects
> >>> to use SCALE to both control amplifier gains and to multiply with
> >>> the _raw value to get a value in the real world units.
> >>>
> >>> It's a bit fiddly to do the computation, but typically at start up time
> >>> you work out what the combination of each PGA gain and the reference
> >>> voltage means for the scales available and stash that somewhere to use later.
> >> Complicating factor with this ADC is that the final gain value depends
> >> on the sampling rate as well as the power supply voltage. Which would
> >> lead to the "available" list being different after changing the sample
> >> rate and confusing userspace. If a userspace app would read the
> >> available sample rates and gains, and then proceeded to set them, the
> >> results would be weird, as the order in which they were set would
> >> matter. Setting the gain after setting the sample rate would likely
> >> result in an EINVAL error because the selected gain is no longer applicable.
> >>
> >> To me it seemed more logical to provide the analog amplification and
> >> sample rate as separate, unrelated values.
> > It may be logical, but it isn't how the IIO ABI has ever worked and it doesn't
> > extend to more complex cases. It's in general true that a PGA will result
> > in a change to the scale that userspace needs to apply. There are devices
> > where it doesn't. There are lots of things that at first glance 'could'
> > affect scale but often do it in complex non linear ways that userspace
> > simply can't know about - hence if we are pushing calculations into userspace
> > we need it to just be (_raw + _offset) * _scale.
> > Note that there is some wiggle room with how raw "raw" is.
> >
> > There are two solutions that have been taken in drivers.
> > 1) The above software flow is broken as any ABI write in IIO is allowed
> > to affect other ABI elements. This is less than ideal though.
> > 2) Let the scale free float. So the attempt is to keep as close as possible
> > to the set value as other things change (i.e. the sampling frequency).
> > The scale_avail reflects current settings of everything else, and indeed
> > changes with other ABI wirtes (this is quite common) but the interface is
> > made more intuitive by matching as closely as possible. Thus if you change the
> > sampling rate and the scale changes then you attempt to modify the PGA
> > to keep it approximately the same. Obviously it clamps at end points
> > but nothing we can do about that.
> >
> > However, having said that, I don't 'think' we need either here...
> > A common thing to do for scale vs sampling rate (which is typically
> > either oversampling based, or based on another SAR cycle) is to just shift
> > the raw data to incorporate it - a common sensor design is to justify it
> > so that the unused bits are the LSBs - so may be a case of just not shifting
> > it to compensate for the datarate change.. That's not true here if I read
> > the datasheet correctly, but a simple
> > sysfs raw read value == raw_value << (16 - bits for data rate) should fix that.
>
> I agree, a bit of shifting (pun intended) is by far the better solution.
>
> > Interesting the datasheet argues they deliberately right aligned and sign extended
> > to allow oversampling without shifts, though counter argument is they made everyone
> > who wants a real scale apply a shift. I guess it depends on the expected use case.
>
> My guess is that the chip internally always runs at 128Hz and simply
> adds the sampled values together in a 16-bit register for the lower
> sampling rates. Someone came up with that datasheet text later on.
>
> >>>> +
> >>>> +static const struct iio_chan_spec ads1100_channel = {
> >>>> + .type = IIO_VOLTAGE,
> >>>> + .differential = 0,
> >>>> + .indexed = 0,
> >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >>>> + .info_mask_shared_by_all =
> >>>> + BIT(IIO_CHAN_INFO_SCALE) |
> >>>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> >>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> >>>> + .info_mask_shared_by_all_available =
> >>>> + BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> >>> Hardware gain is normally only used for cases where the gain is not
> >>> directly related to scale. Things like the gain on a signal that is
> >>> being used for measuring time of flight. Here you should probably
> >>> just be using SCALE.
> >> In this case, SCALE depends on SAMP_FREQ as well as GAIN. Which will be
> >> very confusing to userspace.
> >>
> >> Hoping for some leeway here...
> > Sorry no. Though I think applying the shift as mentioned above deals
> > with your data rate dependent scaling and makes this all a lot easier.
>
> True.
>
> ...
> >>>> + iio_device_unregister(indio_dev);
> >>>> +
> >>>> + ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
> >>>> +
> >>>> + pm_runtime_disable(&client->dev);
> >>> I'd expect runtime pm to be disable before messing with the conv mode.
> >>> With a little care you can use devm_runtime_pm_enable()
> >> Setting the conv mode involves I2C traffic. After runtime_disable, the
> >> power supply to the chip may have been disabled, leading to
> >> communication errors on the I2C bus. Hence I thought it appropriate to
> >> write the config register before turning off power.
> > pm_runtime_disable() is disabling the runtime management of the power
> > not the power itself. That you need to do after turning off the
> > management (thus avoiding any races)
>
> Ah. But does that mean I'd need to disable the Vdd power supply here as
> well?
>
Yes. I'd missed that.
Jonathan