[PATCH 0/2] hwmon: Add TI ads1000/ads1100 driver package

[PATCH 0/2] hwmon: Add TI ads1000/ads1100 driver package

[Serge Semin]

This a simple single-channel ADC device similar to ads1100 series,
but with much less capabilities. These ADCs are working over i2c-interface
with just one differential input and with configurable 12-16 bits
resolution. Sample rate is fixed to 128 for ads1000 and can vary from 8
to 128 for ads1100. Vdd value reference value must be supplied so to
properly translate the sampled code to the real voltage.

Even though ads1000/ads1100 devices seem more like ads1015 series, they
in fact pretty much different. First of all ads1000/ads1100 got less
capabilities: just one port, no configurations of digital comparator, no
input multi-channel multiplexer, smaller PGA and data-rate ranges.
In addition they haven't got internal voltage reference, but instead
are created to use Vdd pin voltage. Finally the output code value is
provided in different format. As a result it was much easier for
development and for future support to create a separate driver, which
is opensoureced by means of this patchset.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>


Serge Semin (2):
  dt-bindings: hwmon: Add DT bindings for TI ads1000/ads1100 ADCs
  hwmon: Add ads1000/ads1100 voltage ADCs driver

 .../devicetree/bindings/hwmon/ads1000.txt     |  61 ++++
 Documentation/hwmon/ads1000.rst               |  72 ++++
 MAINTAINERS                                   |   8 +
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/ads1000.c                       | 320 ++++++++++++++++++
 include/linux/platform_data/ads1000.h         |  20 ++
 7 files changed, 492 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ads1000.txt
 create mode 100644 Documentation/hwmon/ads1000.rst
 create mode 100644 drivers/hwmon/ads1000.c
 create mode 100644 include/linux/platform_data/ads1000.h

-- 
2.21.0

[PATCH 1/2] dt-bindings: hwmon: Add DT bindings for TI ads1000/ads1100 ADCs

[Serge Semin]

Add dt-binding documentation for the Texas Instruments ads1000/ads1100 ADCs
driver.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../devicetree/bindings/hwmon/ads1000.txt     | 61 ++++++++++++++++
 Documentation/hwmon/ads1000.rst               | 72 +++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ads1000.txt
 create mode 100644 Documentation/hwmon/ads1000.rst

diff --git a/Documentation/devicetree/bindings/hwmon/ads1000.txt b/Documentation/devicetree/bindings/hwmon/ads1000.txt
new file mode 100644
index 000000000000..3907b7da9b33
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ads1000.txt
@@ -0,0 +1,61 @@
+ADS1000/ADS1100 (I2C)
+
+This device is a 12-16 bit A-D converter with 1 input.
+
+The inputs can be used either as a differential pair of Vin+ Vin- or as a single
+ended sensor for Vin+ GND. The inputs mode is platform-dependent and isn't
+configured by software in any case.
+
+Device A-D converter sensitivity can be configured using two parameters:
+ - pga is the programmable gain amplifier
+    0: x1 (default) 
+    1: x2
+    2: x4
+    3: x8
+ - data_rate in samples per second also affecting the output code accuracy
+    0: 128SPS - +/- Vdd*0.488mV (default, ads1000 accepts this rate only)
+    1: 32SPS  - +/- Vdd*0.122mV
+    2: 16SPS  - +/- Vdd*0.061mV
+    3: 8SPS   - +/- Vdd*0.030mV
+   Since this parameter also affects the output accuracy, be aware the greater
+   SPS the worse accuracy.
+
+As a result the output value is calculated by the next formulae:
+dVin = Cod * Vdd / (PGA * max(|Cod|)), where
+max(|Cod|) - maximum possible value of the output code, which depends on the SPS
+setting from the table above.
+
+The ADS1000/ADS1100 dts-node:
+
+  Required properties:
+   - compatible : must be "ti,ads1000" or "ti,ads1100"
+   - reg : I2C bus address of the device
+   - #address-cells : must be <1>
+   - #size-cells : must be <0>
+   - vdd-supply : regulator for reference supply voltage (usually fixed)
+
+  Optional properties:
+   - ti,gain : the programmable gain amplifier setting
+   - ti,datarate : the converter data rate
+   - ti,voltage-divider : <R1 R2> Ohms inbound voltage dividers,
+     so dVin = (R1 + R2)/R2 * dVin
+
+Example:
+
+vdd_5v0: fixedregulator@0 {
+	compatible = "regulator-fixed";
+	regulator-name = "vdd-ref";
+	regulator-min-microvolt = <5000000>;
+	regulator-max-microvolt = <5000000>;
+	regulator-always-on;
+};
+
+tiadc: ads1000@48 {
+	compatible = "ti,ads1000";
+	reg = <0x48>;
+
+	vdd-supply = <&vdd_5v0>;
+	ti,gain = <0>;
+	ti,voltage-divider = <31600 3600>;
+};
+
diff --git a/Documentation/hwmon/ads1000.rst b/Documentation/hwmon/ads1000.rst
new file mode 100644
index 000000000000..fcfe52d5d64d
--- /dev/null
+++ b/Documentation/hwmon/ads1000.rst
@@ -0,0 +1,72 @@
+Kernel driver ads1000
+=====================
+
+Supported chips:
+
+  * Texas Instruments ADS1000
+
+    Prefix: 'ads1000'
+
+    Datasheet: Publicly available at the Texas Instruments website:
+
+               http://www.ti.com/lit/ds/symlink/ads1000.pdf
+
+  * Texas Instruments ADS1100
+
+    Prefix: 'ads1100'
+
+    Datasheet: Publicly available at the Texas Instruments website:
+
+               http://www.ti.com/lit/ds/symlink/ads1100.pdf
+
+Authors:
+	Serge Semin <fancer.lancer@gmail.com>
+
+Description
+-----------
+
+This driver implements support for the Texas Instruments ADS1000/ADS1100 ADCs.
+
+This device is a 12-16 bit A-D converter with 1 input.
+
+The inputs can be used either as a differential pair of Vin+ Vin- or as a single
+ended sensor for Vin+ GND. The inputs mode is platform-dependent and isn't
+configured by software in any case.
+
+Platform Data
+-------------
+
+In linux/platform_data/ads1000.h platform data is defined to be of
+the following fields:
+
+ - pga is the programmable gain amplifier.
+
+    - 0: x1
+    - 1: x2
+    - 2: x4
+    - 3: x8
+
+ - data_rate in samples per second also affecting the output code accuracy.
+
+    - 0: 128SPS - +/- Vdd*0.488mV (ads1000 accepts this rate only)
+    - 1: 32SPS  - +/- Vdd*0.122mV
+    - 2: 16SPS  - +/- Vdd*0.061mV
+    - 3: 8SPS   - +/- Vdd*0.030mV
+   Since this parameter also affects the output accuracy, be aware the greater
+   SPS the worse accuracy.
+
+ - vdd is a pointer to the voltage regulator with reference voltage source.
+
+ - divider is an array of inbound voltage dividers in <R1 R2> Ohms, if each of
+   them is non-zero then the output voltage will be modified as follows:
+   dVin = (R1 + R2)/R2 * dVin.
+
+As a result the output value is calculated by the next formulae:
+dVin = Cod * Vdd / (PGA * max(|Cod|)), where max(|Cod|) - maximum possible
+value of the output code, which depends on the SPS setting from data_rate.
+
+Devicetree
+----------
+
+Configuration is also possible via devicetree:
+Documentation/devicetree/bindings/hwmon/ads1000.txt
-- 
2.21.0

[PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Serge Semin]

These are simple Texas Instruments ADC working over i2c-interface with
just one differential input and with configurable 12-16 bits resolution.
Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
ads1100. Vdd value reference value must be supplied so to properly
translate the sampled code to the real voltage. All of these configs are
implemented in the device drivers for hwmon subsystem. The next dts
properties should be specified to comply the device platform setup:
 - vdd-supply - voltage regulator connected to the Vdd pin of the device
 - ti,gain - programmable gain amplifier
 - ti,datarate - converter data rate
 - ti,voltage-divider - possible resistors-base external divider
See bindings documentation file for details.

Even though these devices seem more like ads1015 series, they
in fact pretty much different. First of all ads1000/ads1100 got less
capabilities: just one port, no configurations of digital comparator, no
input multi-channel multiplexer, smaller PGA and data-rate ranges.
In addition they haven't got internal voltage reference, but instead
are created to use Vdd pin voltage. Finally the output code value is
provided in different format. As a result it was much easier for
development and for future support to create a separate driver.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 MAINTAINERS                           |   8 +
 drivers/hwmon/Kconfig                 |  10 +
 drivers/hwmon/Makefile                |   1 +
 drivers/hwmon/ads1000.c               | 320 ++++++++++++++++++++++++++
 include/linux/platform_data/ads1000.h |  20 ++
 5 files changed, 359 insertions(+)
 create mode 100644 drivers/hwmon/ads1000.c
 create mode 100644 include/linux/platform_data/ads1000.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ce573aaa04df..5c3a8107ef1a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -517,6 +517,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/video/backlight/adp8860_bl.c
 
+ADS1000 HARDWARE MONITOR DRIVER
+M:	Serge Semin <fancer.lancer@gmail.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/ads1000.rst
+F:	drivers/hwmon/ads1000.c
+F:	include/linux/platform_data/ads1000.h
+
 ADS1015 HARDWARE MONITOR DRIVER
 M:	Dirk Eibach <eibach@gdsys.de>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1915a18b537b..a1220cc48f2f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
 	  This driver can also be built as a module. If so, the module
 	  will be called adc128d818.
 
+config SENSORS_ADS1000
+	tristate "Texas Instruments ADS1000"
+	depends on I2C
+	help
+	  If you say yes here you get support for Texas Instruments
+	  ADS1000/ADS1100 12-16-bit single channel ADC device.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ads1000.
+
 config SENSORS_ADS1015
 	tristate "Texas Instruments ADS1015"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8db472ea04f0..2cd82f6c651e 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
 obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
+obj-$(CONFIG_SENSORS_ADS1000)	+= ads1000.o
 obj-$(CONFIG_SENSORS_ADS1015)	+= ads1015.o
 obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
 obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
diff --git a/drivers/hwmon/ads1000.c b/drivers/hwmon/ads1000.c
new file mode 100644
index 000000000000..a88b738f56bd
--- /dev/null
+++ b/drivers/hwmon/ads1000.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for ADS1000/ADS1100 12-16-bit ADC
+ *
+ * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
+ *
+ * Based on the ads1015 driver by Dirk Eibach.
+ *
+ * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads1000.pdf
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_data/ads1000.h>
+
+/* Data rates scale table */
+static const unsigned int scale_table[4] = {
+	2048, 8192, 16384, 32768
+};
+
+/* Minimal data rates in samples per second */
+static const unsigned int data_rate_table[4] = {
+	100, 25, 12, 5
+};
+
+#define ADS1000_DEFAULT_PGA 0
+#define ADS1000_DEFAULT_DATA_RATE 0
+#define ADS1000_DEFAULT_R1_DIVIDER 0
+#define ADS1000_DEFAULT_R2_DIVIDER 0
+
+enum ads1000_chips {
+	ads1000,
+	ads1100,
+};
+
+struct ads1000 {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	struct i2c_client *client;
+	struct ads1000_platform_data data;
+	enum ads1000_chips id;
+};
+
+static inline int ads1000_enable_vdd(struct ads1000 *priv)
+{
+	return regulator_enable(priv->data.vdd);
+}
+
+static inline int ads1000_get_vdd(struct ads1000 *priv)
+{
+	return regulator_get_voltage(priv->data.vdd);
+}
+
+static int ads1000_read_adc(struct ads1000 *priv)
+{
+	struct i2c_client *client = priv->client;
+	unsigned int delay_ms;
+	u8 data[3] = {0};
+	int res;
+
+	mutex_lock(&priv->update_lock);
+
+	delay_ms = DIV_ROUND_UP(1000, data_rate_table[priv->data.data_rate]);
+
+	/* setup and start single conversion */
+	data[2] |= (1 << 7) | (1 << 4);
+	data[2] |= priv->data.pga;
+	data[2] |= priv->data.data_rate << 2;
+
+	res = i2c_master_send(client, &data[2], 1);
+	if (res < 0)
+		goto err_unlock;
+
+	/* wait until conversion finished */
+	msleep(delay_ms);
+	res = i2c_master_recv(client, data, 3);
+	if (res < 0)
+		goto err_unlock;
+
+	if (data[2] & (1 << 7)) {
+		res = -EIO;
+		goto err_unlock;
+	}
+
+	res = ((u16)data[0] << 8) | data[1];
+
+err_unlock:
+	mutex_unlock(&priv->update_lock);
+
+	return res;
+}
+
+static int ads1000_reg_to_mv(struct ads1000 *priv, s16 reg)
+{
+	unsigned int *divider = priv->data.divider;
+	int voltage = ads1000_get_vdd(priv);
+	int gain = 1 << priv->data.pga;
+	int c = 0;
+
+	voltage = reg*DIV_ROUND_CLOSEST(voltage, 1000);
+	gain = gain*scale_table[priv->data.data_rate];
+	voltage = DIV_ROUND_CLOSEST(voltage, gain);
+
+	if (divider[0] && divider[1]) {
+		c = divider[0]*voltage;
+		c = DIV_ROUND_CLOSEST(c, (int)divider[1]);
+	}
+
+	return voltage + c;
+}
+
+static ssize_t show_in(struct device *dev, struct device_attribute *da,
+		       char *buf)
+{
+	struct ads1000 *priv = dev_get_drvdata(dev);
+	int res;
+
+	res = ads1000_read_adc(priv);
+	if (res < 0)
+		return res;
+
+	return sprintf(buf, "%d\n", ads1000_reg_to_mv(priv, res));
+}
+
+static SENSOR_DEVICE_ATTR(in0_input, 0444, show_in, NULL, 0);
+
+static struct attribute *ads1000_attrs[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ads1000);
+
+static struct ads1000 *ads1000_create_priv(struct i2c_client *client,
+					   const struct i2c_device_id *id)
+{
+	struct ads1000 *priv;
+
+	priv = devm_kzalloc(&client->dev, sizeof(struct ads1000),
+			    GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	if (client->dev.of_node)
+		priv->id = (enum ads1000_chips)
+			of_device_get_match_data(&client->dev);
+	else
+		priv->id = id->driver_data;
+
+	i2c_set_clientdata(client, priv);
+	priv->client = client;
+	mutex_init(&priv->update_lock);
+
+	return priv;
+}
+
+#ifdef CONFIG_OF
+static int ads1000_get_config_of(struct ads1000 *priv)
+{
+	struct i2c_client *client = priv->client;
+	struct device_node *node = client->dev.of_node;
+	u32 divider[2];
+	u32 val;
+
+	if (!node)
+		return -EINVAL;
+
+	if (!of_property_read_u32(node, "ti,gain", &val))
+		priv->data.pga = val;
+
+	if (!of_property_read_u32(node, "ti,datarate", &val))
+		priv->data.data_rate = val;
+
+	if (!of_property_read_u32_array(node, "ti,voltage-divider",
+					divider, 2)) {
+		priv->data.divider[0] = divider[0];
+		priv->data.divider[1] = divider[1];
+	}
+
+	priv->data.vdd = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(priv->data.vdd))
+		return PTR_ERR(priv->data.vdd);
+
+	return 0;
+}
+#endif
+
+static int ads1000_get_config(struct ads1000 *priv)
+{
+	struct i2c_client *client = priv->client;
+	struct ads1000_platform_data *pdata = dev_get_platdata(&client->dev);
+
+	priv->data.pga = ADS1000_DEFAULT_PGA;
+	priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
+	priv->data.divider[0] = ADS1000_DEFAULT_R1_DIVIDER;
+	priv->data.divider[1] = ADS1000_DEFAULT_R2_DIVIDER;
+
+	/* prefer platform data */
+	if (pdata) {
+		memcpy(&priv->data, pdata, sizeof(priv->data));
+	} else {
+#ifdef CONFIG_OF
+		int ret;
+
+		ret = ads1000_get_config_of(priv);
+		if (ret)
+			return ret;
+#endif
+	}
+
+	if (!priv->data.vdd) {
+		dev_err(&client->dev, "No VDD regulator\n");
+		return -EINVAL;
+	}
+
+	if (priv->data.pga > 4) {
+		dev_err(&client->dev, "Invalid gain, using default\n");
+		priv->data.pga = ADS1000_DEFAULT_PGA;
+	}
+
+	if (priv->data.data_rate > 4) {
+		dev_err(&client->dev, "Invalid datarate, using default\n");
+		priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
+	}
+
+	if (priv->id == ads1000 && priv->data.data_rate != 0) {
+		dev_warn(&client->dev, "ADC data rate can be 128SPS only\n");
+		priv->data.data_rate = 0;
+	}
+
+	return 0;
+}
+
+static int ads1000_set_config(struct ads1000 *priv)
+{
+	u8 data = 0;
+	int ret;
+
+	/* disable continuous conversion */
+	data |= (1 << 4);
+	data |= priv->data.pga;
+	data |= priv->data.data_rate << 2;
+
+	ret = i2c_master_send(priv->client, &data, 1);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ads1000_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct ads1000 *priv;
+	int ret;
+
+	priv = ads1000_create_priv(client, id);
+	if (IS_ERR(priv))
+		return PTR_ERR(priv);
+
+	ret = ads1000_get_config(priv);
+	if (ret)
+		return ret;
+
+	ret = ads1000_enable_vdd(priv);
+	if (ret)
+		return ret;
+
+	ret = ads1000_set_config(priv);
+	if (ret)
+		return ret;
+
+	priv->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
+				client->name, priv, ads1000_groups);
+	if (IS_ERR(priv->hwmon_dev))
+		return PTR_ERR(priv->hwmon_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id ads1000_id[] = {
+	{ "ads1000",  ads1000},
+	{ "ads1100",  ads1100},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ads1000_id);
+
+static const struct of_device_id ads1000_of_match[] = {
+	{
+		.compatible = "ti,ads1000",
+		.data = (void *)ads1000
+	},
+	{
+		.compatible = "ti,ads1100",
+		.data = (void *)ads1100
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ads1000_of_match);
+
+static struct i2c_driver ads1000_driver = {
+	.driver = {
+		.name = "ads1000",
+		.of_match_table = of_match_ptr(ads1000_of_match),
+	},
+	.probe = ads1000_probe,
+	.id_table = ads1000_id,
+};
+module_i2c_driver(ads1000_driver);
+
+MODULE_AUTHOR("Serge Semin <fancer.lancer@gmail.com>");
+MODULE_DESCRIPTION("ADS1000 driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/ads1000.h b/include/linux/platform_data/ads1000.h
new file mode 100644
index 000000000000..979670483537
--- /dev/null
+++ b/include/linux/platform_data/ads1000.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Platform Data for ADS1000/ADS1100 12-16-bit ADC
+ *
+ * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
+ */
+
+#ifndef LINUX_ADS1000_H
+#define LINUX_ADS1000_H
+
+#include <linux/regulator/consumer.h>
+
+struct ads1000_platform_data {
+	unsigned int pga;
+	unsigned int data_rate;
+	struct regulator *vdd;
+	unsigned int divider[2];
+};
+
+#endif /* LINUX_ADS1000_H */
-- 
2.21.0

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Guenter Roeck]

Hi,

On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> These are simple Texas Instruments ADC working over i2c-interface with
> just one differential input and with configurable 12-16 bits resolution.
> Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> ads1100. Vdd value reference value must be supplied so to properly
> translate the sampled code to the real voltage. All of these configs are
> implemented in the device drivers for hwmon subsystem. The next dts
> properties should be specified to comply the device platform setup:
>  - vdd-supply - voltage regulator connected to the Vdd pin of the device
>  - ti,gain - programmable gain amplifier
>  - ti,datarate - converter data rate
>  - ti,voltage-divider - possible resistors-base external divider
> See bindings documentation file for details.
> 
> Even though these devices seem more like ads1015 series, they
> in fact pretty much different. First of all ads1000/ads1100 got less
> capabilities: just one port, no configurations of digital comparator, no
> input multi-channel multiplexer, smaller PGA and data-rate ranges.
> In addition they haven't got internal voltage reference, but instead
> are created to use Vdd pin voltage. Finally the output code value is
> provided in different format. As a result it was much easier for
> development and for future support to create a separate driver.
> 

This chicp doesn't have any real hardware monitoring characteristics
(no limit registers). It seems to be better suited to be implemented
as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
should work just fine.

Jonathan, what do you think ?

Thanks,
Guenter

> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  MAINTAINERS                           |   8 +
>  drivers/hwmon/Kconfig                 |  10 +
>  drivers/hwmon/Makefile                |   1 +
>  drivers/hwmon/ads1000.c               | 320 ++++++++++++++++++++++++++
>  include/linux/platform_data/ads1000.h |  20 ++
>  5 files changed, 359 insertions(+)
>  create mode 100644 drivers/hwmon/ads1000.c
>  create mode 100644 include/linux/platform_data/ads1000.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ce573aaa04df..5c3a8107ef1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -517,6 +517,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/video/backlight/adp8860_bl.c
>  
> +ADS1000 HARDWARE MONITOR DRIVER
> +M:	Serge Semin <fancer.lancer@gmail.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/ads1000.rst
> +F:	drivers/hwmon/ads1000.c
> +F:	include/linux/platform_data/ads1000.h
> +
>  ADS1015 HARDWARE MONITOR DRIVER
>  M:	Dirk Eibach <eibach@gdsys.de>
>  L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1915a18b537b..a1220cc48f2f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
>  	  This driver can also be built as a module. If so, the module
>  	  will be called adc128d818.
>  
> +config SENSORS_ADS1000
> +	tristate "Texas Instruments ADS1000"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Texas Instruments
> +	  ADS1000/ADS1100 12-16-bit single channel ADC device.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called ads1000.
> +
>  config SENSORS_ADS1015
>  	tristate "Texas Instruments ADS1015"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8db472ea04f0..2cd82f6c651e 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
>  obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
>  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
>  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
> +obj-$(CONFIG_SENSORS_ADS1000)	+= ads1000.o
>  obj-$(CONFIG_SENSORS_ADS1015)	+= ads1015.o
>  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
>  obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
> diff --git a/drivers/hwmon/ads1000.c b/drivers/hwmon/ads1000.c
> new file mode 100644
> index 000000000000..a88b738f56bd
> --- /dev/null
> +++ b/drivers/hwmon/ads1000.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for ADS1000/ADS1100 12-16-bit ADC
> + *
> + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> + *
> + * Based on the ads1015 driver by Dirk Eibach.
> + *
> + * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads1000.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/ads1000.h>
> +
> +/* Data rates scale table */
> +static const unsigned int scale_table[4] = {
> +	2048, 8192, 16384, 32768
> +};
> +
> +/* Minimal data rates in samples per second */
> +static const unsigned int data_rate_table[4] = {
> +	100, 25, 12, 5
> +};
> +
> +#define ADS1000_DEFAULT_PGA 0
> +#define ADS1000_DEFAULT_DATA_RATE 0
> +#define ADS1000_DEFAULT_R1_DIVIDER 0
> +#define ADS1000_DEFAULT_R2_DIVIDER 0
> +
> +enum ads1000_chips {
> +	ads1000,
> +	ads1100,
> +};
> +
> +struct ads1000 {
> +	struct device *hwmon_dev;
> +	struct mutex update_lock;
> +	struct i2c_client *client;
> +	struct ads1000_platform_data data;
> +	enum ads1000_chips id;
> +};
> +
> +static inline int ads1000_enable_vdd(struct ads1000 *priv)
> +{
> +	return regulator_enable(priv->data.vdd);
> +}
> +
> +static inline int ads1000_get_vdd(struct ads1000 *priv)
> +{
> +	return regulator_get_voltage(priv->data.vdd);
> +}
> +
> +static int ads1000_read_adc(struct ads1000 *priv)
> +{
> +	struct i2c_client *client = priv->client;
> +	unsigned int delay_ms;
> +	u8 data[3] = {0};
> +	int res;
> +
> +	mutex_lock(&priv->update_lock);
> +
> +	delay_ms = DIV_ROUND_UP(1000, data_rate_table[priv->data.data_rate]);
> +
> +	/* setup and start single conversion */
> +	data[2] |= (1 << 7) | (1 << 4);
> +	data[2] |= priv->data.pga;
> +	data[2] |= priv->data.data_rate << 2;
> +
> +	res = i2c_master_send(client, &data[2], 1);
> +	if (res < 0)
> +		goto err_unlock;
> +
> +	/* wait until conversion finished */
> +	msleep(delay_ms);
> +	res = i2c_master_recv(client, data, 3);
> +	if (res < 0)
> +		goto err_unlock;
> +
> +	if (data[2] & (1 << 7)) {
> +		res = -EIO;
> +		goto err_unlock;
> +	}
> +
> +	res = ((u16)data[0] << 8) | data[1];
> +
> +err_unlock:
> +	mutex_unlock(&priv->update_lock);
> +
> +	return res;
> +}
> +
> +static int ads1000_reg_to_mv(struct ads1000 *priv, s16 reg)
> +{
> +	unsigned int *divider = priv->data.divider;
> +	int voltage = ads1000_get_vdd(priv);
> +	int gain = 1 << priv->data.pga;
> +	int c = 0;
> +
> +	voltage = reg*DIV_ROUND_CLOSEST(voltage, 1000);
> +	gain = gain*scale_table[priv->data.data_rate];
> +	voltage = DIV_ROUND_CLOSEST(voltage, gain);
> +
> +	if (divider[0] && divider[1]) {
> +		c = divider[0]*voltage;
> +		c = DIV_ROUND_CLOSEST(c, (int)divider[1]);
> +	}
> +
> +	return voltage + c;
> +}
> +
> +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> +		       char *buf)
> +{
> +	struct ads1000 *priv = dev_get_drvdata(dev);
> +	int res;
> +
> +	res = ads1000_read_adc(priv);
> +	if (res < 0)
> +		return res;
> +
> +	return sprintf(buf, "%d\n", ads1000_reg_to_mv(priv, res));
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, 0444, show_in, NULL, 0);
> +
> +static struct attribute *ads1000_attrs[] = {
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(ads1000);
> +
> +static struct ads1000 *ads1000_create_priv(struct i2c_client *client,
> +					   const struct i2c_device_id *id)
> +{
> +	struct ads1000 *priv;
> +
> +	priv = devm_kzalloc(&client->dev, sizeof(struct ads1000),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (client->dev.of_node)
> +		priv->id = (enum ads1000_chips)
> +			of_device_get_match_data(&client->dev);
> +	else
> +		priv->id = id->driver_data;
> +
> +	i2c_set_clientdata(client, priv);
> +	priv->client = client;
> +	mutex_init(&priv->update_lock);
> +
> +	return priv;
> +}
> +
> +#ifdef CONFIG_OF
> +static int ads1000_get_config_of(struct ads1000 *priv)
> +{
> +	struct i2c_client *client = priv->client;
> +	struct device_node *node = client->dev.of_node;
> +	u32 divider[2];
> +	u32 val;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	if (!of_property_read_u32(node, "ti,gain", &val))
> +		priv->data.pga = val;
> +
> +	if (!of_property_read_u32(node, "ti,datarate", &val))
> +		priv->data.data_rate = val;
> +
> +	if (!of_property_read_u32_array(node, "ti,voltage-divider",
> +					divider, 2)) {
> +		priv->data.divider[0] = divider[0];
> +		priv->data.divider[1] = divider[1];
> +	}
> +
> +	priv->data.vdd = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(priv->data.vdd))
> +		return PTR_ERR(priv->data.vdd);
> +
> +	return 0;
> +}
> +#endif
> +
> +static int ads1000_get_config(struct ads1000 *priv)
> +{
> +	struct i2c_client *client = priv->client;
> +	struct ads1000_platform_data *pdata = dev_get_platdata(&client->dev);
> +
> +	priv->data.pga = ADS1000_DEFAULT_PGA;
> +	priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> +	priv->data.divider[0] = ADS1000_DEFAULT_R1_DIVIDER;
> +	priv->data.divider[1] = ADS1000_DEFAULT_R2_DIVIDER;
> +
> +	/* prefer platform data */
> +	if (pdata) {
> +		memcpy(&priv->data, pdata, sizeof(priv->data));
> +	} else {
> +#ifdef CONFIG_OF
> +		int ret;
> +
> +		ret = ads1000_get_config_of(priv);
> +		if (ret)
> +			return ret;
> +#endif
> +	}
> +
> +	if (!priv->data.vdd) {
> +		dev_err(&client->dev, "No VDD regulator\n");
> +		return -EINVAL;
> +	}
> +
> +	if (priv->data.pga > 4) {
> +		dev_err(&client->dev, "Invalid gain, using default\n");
> +		priv->data.pga = ADS1000_DEFAULT_PGA;
> +	}
> +
> +	if (priv->data.data_rate > 4) {
> +		dev_err(&client->dev, "Invalid datarate, using default\n");
> +		priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> +	}
> +
> +	if (priv->id == ads1000 && priv->data.data_rate != 0) {
> +		dev_warn(&client->dev, "ADC data rate can be 128SPS only\n");
> +		priv->data.data_rate = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ads1000_set_config(struct ads1000 *priv)
> +{
> +	u8 data = 0;
> +	int ret;
> +
> +	/* disable continuous conversion */
> +	data |= (1 << 4);
> +	data |= priv->data.pga;
> +	data |= priv->data.data_rate << 2;
> +
> +	ret = i2c_master_send(priv->client, &data, 1);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int ads1000_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct ads1000 *priv;
> +	int ret;
> +
> +	priv = ads1000_create_priv(client, id);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	ret = ads1000_get_config(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1000_enable_vdd(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1000_set_config(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
> +				client->name, priv, ads1000_groups);
> +	if (IS_ERR(priv->hwmon_dev))
> +		return PTR_ERR(priv->hwmon_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ads1000_id[] = {
> +	{ "ads1000",  ads1000},
> +	{ "ads1100",  ads1100},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ads1000_id);
> +
> +static const struct of_device_id ads1000_of_match[] = {
> +	{
> +		.compatible = "ti,ads1000",
> +		.data = (void *)ads1000
> +	},
> +	{
> +		.compatible = "ti,ads1100",
> +		.data = (void *)ads1100
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ads1000_of_match);
> +
> +static struct i2c_driver ads1000_driver = {
> +	.driver = {
> +		.name = "ads1000",
> +		.of_match_table = of_match_ptr(ads1000_of_match),
> +	},
> +	.probe = ads1000_probe,
> +	.id_table = ads1000_id,
> +};
> +module_i2c_driver(ads1000_driver);
> +
> +MODULE_AUTHOR("Serge Semin <fancer.lancer@gmail.com>");
> +MODULE_DESCRIPTION("ADS1000 driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/ads1000.h b/include/linux/platform_data/ads1000.h
> new file mode 100644
> index 000000000000..979670483537
> --- /dev/null
> +++ b/include/linux/platform_data/ads1000.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Platform Data for ADS1000/ADS1100 12-16-bit ADC
> + *
> + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> + */
> +
> +#ifndef LINUX_ADS1000_H
> +#define LINUX_ADS1000_H
> +
> +#include <linux/regulator/consumer.h>
> +
> +struct ads1000_platform_data {
> +	unsigned int pga;
> +	unsigned int data_rate;
> +	struct regulator *vdd;
> +	unsigned int divider[2];
> +};
> +
> +#endif /* LINUX_ADS1000_H */

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Jonathan Cameron]

On Thu, 30 May 2019 05:55:10 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
> 
> On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > These are simple Texas Instruments ADC working over i2c-interface with
> > just one differential input and with configurable 12-16 bits resolution.
> > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > ads1100. Vdd value reference value must be supplied so to properly
> > translate the sampled code to the real voltage. All of these configs are
> > implemented in the device drivers for hwmon subsystem. The next dts
> > properties should be specified to comply the device platform setup:
> >  - vdd-supply - voltage regulator connected to the Vdd pin of the device
> >  - ti,gain - programmable gain amplifier
> >  - ti,datarate - converter data rate
> >  - ti,voltage-divider - possible resistors-base external divider
> > See bindings documentation file for details.
> > 
> > Even though these devices seem more like ads1015 series, they
> > in fact pretty much different. First of all ads1000/ads1100 got less
> > capabilities: just one port, no configurations of digital comparator, no
> > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > In addition they haven't got internal voltage reference, but instead
> > are created to use Vdd pin voltage. Finally the output code value is
> > provided in different format. As a result it was much easier for
> > development and for future support to create a separate driver.
> >   
> 
> This chicp doesn't have any real hardware monitoring characteristics
> (no limit registers). It seems to be better suited to be implemented
> as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> should work just fine.
> 
> Jonathan, what do you think ?
Sorry for slow response, was on vacation.

Agreed, this looks like a standard multipurpose ADC so probably more suited
to IIO. Whether you bother with a buffered /chardev interface or not given it
is a fairly slow device is a separate question (can always be added later
when someone wants it).

Note the voltage-divider in the DT properties is something that should
have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
for that, in this case using the voltage divider binding.

gain and datarate are both characteristics that should be controlled from
userspace rather than via a binding.

Thanks,

Jonathan
> 
> Thanks,
> Guenter
> 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  MAINTAINERS                           |   8 +
> >  drivers/hwmon/Kconfig                 |  10 +
> >  drivers/hwmon/Makefile                |   1 +
> >  drivers/hwmon/ads1000.c               | 320 ++++++++++++++++++++++++++
> >  include/linux/platform_data/ads1000.h |  20 ++
> >  5 files changed, 359 insertions(+)
> >  create mode 100644 drivers/hwmon/ads1000.c
> >  create mode 100644 include/linux/platform_data/ads1000.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ce573aaa04df..5c3a8107ef1a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -517,6 +517,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
> >  S:	Supported
> >  F:	drivers/video/backlight/adp8860_bl.c
> >  
> > +ADS1000 HARDWARE MONITOR DRIVER
> > +M:	Serge Semin <fancer.lancer@gmail.com>
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/hwmon/ads1000.rst
> > +F:	drivers/hwmon/ads1000.c
> > +F:	include/linux/platform_data/ads1000.h
> > +
> >  ADS1015 HARDWARE MONITOR DRIVER
> >  M:	Dirk Eibach <eibach@gdsys.de>
> >  L:	linux-hwmon@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 1915a18b537b..a1220cc48f2f 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called adc128d818.
> >  
> > +config SENSORS_ADS1000
> > +	tristate "Texas Instruments ADS1000"
> > +	depends on I2C
> > +	help
> > +	  If you say yes here you get support for Texas Instruments
> > +	  ADS1000/ADS1100 12-16-bit single channel ADC device.
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called ads1000.
> > +
> >  config SENSORS_ADS1015
> >  	tristate "Texas Instruments ADS1015"
> >  	depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 8db472ea04f0..2cd82f6c651e 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
> >  obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
> >  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
> >  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
> > +obj-$(CONFIG_SENSORS_ADS1000)	+= ads1000.o
> >  obj-$(CONFIG_SENSORS_ADS1015)	+= ads1015.o
> >  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
> >  obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
> > diff --git a/drivers/hwmon/ads1000.c b/drivers/hwmon/ads1000.c
> > new file mode 100644
> > index 000000000000..a88b738f56bd
> > --- /dev/null
> > +++ b/drivers/hwmon/ads1000.c
> > @@ -0,0 +1,320 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for ADS1000/ADS1100 12-16-bit ADC
> > + *
> > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > + *
> > + * Based on the ads1015 driver by Dirk Eibach.
> > + *
> > + * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads1000.pdf
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_data/ads1000.h>
> > +
> > +/* Data rates scale table */
> > +static const unsigned int scale_table[4] = {
> > +	2048, 8192, 16384, 32768
> > +};
> > +
> > +/* Minimal data rates in samples per second */
> > +static const unsigned int data_rate_table[4] = {
> > +	100, 25, 12, 5
> > +};
> > +
> > +#define ADS1000_DEFAULT_PGA 0
> > +#define ADS1000_DEFAULT_DATA_RATE 0
> > +#define ADS1000_DEFAULT_R1_DIVIDER 0
> > +#define ADS1000_DEFAULT_R2_DIVIDER 0
> > +
> > +enum ads1000_chips {
> > +	ads1000,
> > +	ads1100,
> > +};
> > +
> > +struct ads1000 {
> > +	struct device *hwmon_dev;
> > +	struct mutex update_lock;
> > +	struct i2c_client *client;
> > +	struct ads1000_platform_data data;
> > +	enum ads1000_chips id;
> > +};
> > +
> > +static inline int ads1000_enable_vdd(struct ads1000 *priv)
> > +{
> > +	return regulator_enable(priv->data.vdd);
> > +}
> > +
> > +static inline int ads1000_get_vdd(struct ads1000 *priv)
> > +{
> > +	return regulator_get_voltage(priv->data.vdd);
> > +}
> > +
> > +static int ads1000_read_adc(struct ads1000 *priv)
> > +{
> > +	struct i2c_client *client = priv->client;
> > +	unsigned int delay_ms;
> > +	u8 data[3] = {0};
> > +	int res;
> > +
> > +	mutex_lock(&priv->update_lock);
> > +
> > +	delay_ms = DIV_ROUND_UP(1000, data_rate_table[priv->data.data_rate]);
> > +
> > +	/* setup and start single conversion */
> > +	data[2] |= (1 << 7) | (1 << 4);
> > +	data[2] |= priv->data.pga;
> > +	data[2] |= priv->data.data_rate << 2;
> > +
> > +	res = i2c_master_send(client, &data[2], 1);
> > +	if (res < 0)
> > +		goto err_unlock;
> > +
> > +	/* wait until conversion finished */
> > +	msleep(delay_ms);
> > +	res = i2c_master_recv(client, data, 3);
> > +	if (res < 0)
> > +		goto err_unlock;
> > +
> > +	if (data[2] & (1 << 7)) {
> > +		res = -EIO;
> > +		goto err_unlock;
> > +	}
> > +
> > +	res = ((u16)data[0] << 8) | data[1];
> > +
> > +err_unlock:
> > +	mutex_unlock(&priv->update_lock);
> > +
> > +	return res;
> > +}
> > +
> > +static int ads1000_reg_to_mv(struct ads1000 *priv, s16 reg)
> > +{
> > +	unsigned int *divider = priv->data.divider;
> > +	int voltage = ads1000_get_vdd(priv);
> > +	int gain = 1 << priv->data.pga;
> > +	int c = 0;
> > +
> > +	voltage = reg*DIV_ROUND_CLOSEST(voltage, 1000);
> > +	gain = gain*scale_table[priv->data.data_rate];
> > +	voltage = DIV_ROUND_CLOSEST(voltage, gain);
> > +
> > +	if (divider[0] && divider[1]) {
> > +		c = divider[0]*voltage;
> > +		c = DIV_ROUND_CLOSEST(c, (int)divider[1]);
> > +	}
> > +
> > +	return voltage + c;
> > +}
> > +
> > +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> > +		       char *buf)
> > +{
> > +	struct ads1000 *priv = dev_get_drvdata(dev);
> > +	int res;
> > +
> > +	res = ads1000_read_adc(priv);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	return sprintf(buf, "%d\n", ads1000_reg_to_mv(priv, res));
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR(in0_input, 0444, show_in, NULL, 0);
> > +
> > +static struct attribute *ads1000_attrs[] = {
> > +	&sensor_dev_attr_in0_input.dev_attr.attr,
> > +	NULL
> > +};
> > +ATTRIBUTE_GROUPS(ads1000);
> > +
> > +static struct ads1000 *ads1000_create_priv(struct i2c_client *client,
> > +					   const struct i2c_device_id *id)
> > +{
> > +	struct ads1000 *priv;
> > +
> > +	priv = devm_kzalloc(&client->dev, sizeof(struct ads1000),
> > +			    GFP_KERNEL);
> > +	if (!priv)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	if (client->dev.of_node)
> > +		priv->id = (enum ads1000_chips)
> > +			of_device_get_match_data(&client->dev);
> > +	else
> > +		priv->id = id->driver_data;
> > +
> > +	i2c_set_clientdata(client, priv);
> > +	priv->client = client;
> > +	mutex_init(&priv->update_lock);
> > +
> > +	return priv;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static int ads1000_get_config_of(struct ads1000 *priv)
> > +{
> > +	struct i2c_client *client = priv->client;
> > +	struct device_node *node = client->dev.of_node;
> > +	u32 divider[2];
> > +	u32 val;
> > +
> > +	if (!node)
> > +		return -EINVAL;
> > +
> > +	if (!of_property_read_u32(node, "ti,gain", &val))
> > +		priv->data.pga = val;
> > +
> > +	if (!of_property_read_u32(node, "ti,datarate", &val))
> > +		priv->data.data_rate = val;
> > +
> > +	if (!of_property_read_u32_array(node, "ti,voltage-divider",
> > +					divider, 2)) {
> > +		priv->data.divider[0] = divider[0];
> > +		priv->data.divider[1] = divider[1];
> > +	}
> > +
> > +	priv->data.vdd = devm_regulator_get(&client->dev, "vdd");
> > +	if (IS_ERR(priv->data.vdd))
> > +		return PTR_ERR(priv->data.vdd);
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static int ads1000_get_config(struct ads1000 *priv)
> > +{
> > +	struct i2c_client *client = priv->client;
> > +	struct ads1000_platform_data *pdata = dev_get_platdata(&client->dev);
> > +
> > +	priv->data.pga = ADS1000_DEFAULT_PGA;
> > +	priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > +	priv->data.divider[0] = ADS1000_DEFAULT_R1_DIVIDER;
> > +	priv->data.divider[1] = ADS1000_DEFAULT_R2_DIVIDER;
> > +
> > +	/* prefer platform data */
> > +	if (pdata) {
> > +		memcpy(&priv->data, pdata, sizeof(priv->data));
> > +	} else {
> > +#ifdef CONFIG_OF
> > +		int ret;
> > +
> > +		ret = ads1000_get_config_of(priv);
> > +		if (ret)
> > +			return ret;
> > +#endif
> > +	}
> > +
> > +	if (!priv->data.vdd) {
> > +		dev_err(&client->dev, "No VDD regulator\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (priv->data.pga > 4) {
> > +		dev_err(&client->dev, "Invalid gain, using default\n");
> > +		priv->data.pga = ADS1000_DEFAULT_PGA;
> > +	}
> > +
> > +	if (priv->data.data_rate > 4) {
> > +		dev_err(&client->dev, "Invalid datarate, using default\n");
> > +		priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > +	}
> > +
> > +	if (priv->id == ads1000 && priv->data.data_rate != 0) {
> > +		dev_warn(&client->dev, "ADC data rate can be 128SPS only\n");
> > +		priv->data.data_rate = 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ads1000_set_config(struct ads1000 *priv)
> > +{
> > +	u8 data = 0;
> > +	int ret;
> > +
> > +	/* disable continuous conversion */
> > +	data |= (1 << 4);
> > +	data |= priv->data.pga;
> > +	data |= priv->data.data_rate << 2;
> > +
> > +	ret = i2c_master_send(priv->client, &data, 1);
> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> > +static int ads1000_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct ads1000 *priv;
> > +	int ret;
> > +
> > +	priv = ads1000_create_priv(client, id);
> > +	if (IS_ERR(priv))
> > +		return PTR_ERR(priv);
> > +
> > +	ret = ads1000_get_config(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ads1000_enable_vdd(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ads1000_set_config(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	priv->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
> > +				client->name, priv, ads1000_groups);
> > +	if (IS_ERR(priv->hwmon_dev))
> > +		return PTR_ERR(priv->hwmon_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ads1000_id[] = {
> > +	{ "ads1000",  ads1000},
> > +	{ "ads1100",  ads1100},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ads1000_id);
> > +
> > +static const struct of_device_id ads1000_of_match[] = {
> > +	{
> > +		.compatible = "ti,ads1000",
> > +		.data = (void *)ads1000
> > +	},
> > +	{
> > +		.compatible = "ti,ads1100",
> > +		.data = (void *)ads1100
> > +	},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ads1000_of_match);
> > +
> > +static struct i2c_driver ads1000_driver = {
> > +	.driver = {
> > +		.name = "ads1000",
> > +		.of_match_table = of_match_ptr(ads1000_of_match),
> > +	},
> > +	.probe = ads1000_probe,
> > +	.id_table = ads1000_id,
> > +};
> > +module_i2c_driver(ads1000_driver);
> > +
> > +MODULE_AUTHOR("Serge Semin <fancer.lancer@gmail.com>");
> > +MODULE_DESCRIPTION("ADS1000 driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/platform_data/ads1000.h b/include/linux/platform_data/ads1000.h
> > new file mode 100644
> > index 000000000000..979670483537
> > --- /dev/null
> > +++ b/include/linux/platform_data/ads1000.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Platform Data for ADS1000/ADS1100 12-16-bit ADC
> > + *
> > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > + */
> > +
> > +#ifndef LINUX_ADS1000_H
> > +#define LINUX_ADS1000_H
> > +
> > +#include <linux/regulator/consumer.h>
> > +
> > +struct ads1000_platform_data {
> > +	unsigned int pga;
> > +	unsigned int data_rate;
> > +	struct regulator *vdd;
> > +	unsigned int divider[2];
> > +};
> > +
> > +#endif /* LINUX_ADS1000_H */  

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Guenter Roeck]

On Mon, Jun 03, 2019 at 12:11:17PM +0100, Jonathan Cameron wrote:
> On Thu, 30 May 2019 05:55:10 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Hi,
> > 
> > On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > > These are simple Texas Instruments ADC working over i2c-interface with
> > > just one differential input and with configurable 12-16 bits resolution.
> > > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > > ads1100. Vdd value reference value must be supplied so to properly
> > > translate the sampled code to the real voltage. All of these configs are
> > > implemented in the device drivers for hwmon subsystem. The next dts
> > > properties should be specified to comply the device platform setup:
> > >  - vdd-supply - voltage regulator connected to the Vdd pin of the device
> > >  - ti,gain - programmable gain amplifier
> > >  - ti,datarate - converter data rate
> > >  - ti,voltage-divider - possible resistors-base external divider
> > > See bindings documentation file for details.
> > > 
> > > Even though these devices seem more like ads1015 series, they
> > > in fact pretty much different. First of all ads1000/ads1100 got less
> > > capabilities: just one port, no configurations of digital comparator, no
> > > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > > In addition they haven't got internal voltage reference, but instead
> > > are created to use Vdd pin voltage. Finally the output code value is
> > > provided in different format. As a result it was much easier for
> > > development and for future support to create a separate driver.
> > >   
> > 
> > This chicp doesn't have any real hardware monitoring characteristics
> > (no limit registers). It seems to be better suited to be implemented
> > as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> > should work just fine.
> > 
> > Jonathan, what do you think ?
> Sorry for slow response, was on vacation.
> 
> Agreed, this looks like a standard multipurpose ADC so probably more suited
> to IIO. Whether you bother with a buffered /chardev interface or not given it
> is a fairly slow device is a separate question (can always be added later
> when someone wants it).
> 
> Note the voltage-divider in the DT properties is something that should
> have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
> for that, in this case using the voltage divider binding.
> 
> gain and datarate are both characteristics that should be controlled from
> userspace rather than via a binding.
> 

In summary: Serge, please re-implement the driver as iio adc driver.

Thanks,
Guenter

> Thanks,
> 
> Jonathan
> > 
> > Thanks,
> > Guenter
> > 
> > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > ---
> > >  MAINTAINERS                           |   8 +
> > >  drivers/hwmon/Kconfig                 |  10 +
> > >  drivers/hwmon/Makefile                |   1 +
> > >  drivers/hwmon/ads1000.c               | 320 ++++++++++++++++++++++++++
> > >  include/linux/platform_data/ads1000.h |  20 ++
> > >  5 files changed, 359 insertions(+)
> > >  create mode 100644 drivers/hwmon/ads1000.c
> > >  create mode 100644 include/linux/platform_data/ads1000.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index ce573aaa04df..5c3a8107ef1a 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -517,6 +517,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
> > >  S:	Supported
> > >  F:	drivers/video/backlight/adp8860_bl.c
> > >  
> > > +ADS1000 HARDWARE MONITOR DRIVER
> > > +M:	Serge Semin <fancer.lancer@gmail.com>
> > > +L:	linux-hwmon@vger.kernel.org
> > > +S:	Maintained
> > > +F:	Documentation/hwmon/ads1000.rst
> > > +F:	drivers/hwmon/ads1000.c
> > > +F:	include/linux/platform_data/ads1000.h
> > > +
> > >  ADS1015 HARDWARE MONITOR DRIVER
> > >  M:	Dirk Eibach <eibach@gdsys.de>
> > >  L:	linux-hwmon@vger.kernel.org
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 1915a18b537b..a1220cc48f2f 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
> > >  	  This driver can also be built as a module. If so, the module
> > >  	  will be called adc128d818.
> > >  
> > > +config SENSORS_ADS1000
> > > +	tristate "Texas Instruments ADS1000"
> > > +	depends on I2C
> > > +	help
> > > +	  If you say yes here you get support for Texas Instruments
> > > +	  ADS1000/ADS1100 12-16-bit single channel ADC device.
> > > +
> > > +	  This driver can also be built as a module.  If so, the module
> > > +	  will be called ads1000.
> > > +
> > >  config SENSORS_ADS1015
> > >  	tristate "Texas Instruments ADS1015"
> > >  	depends on I2C
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index 8db472ea04f0..2cd82f6c651e 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
> > >  obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
> > >  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
> > >  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
> > > +obj-$(CONFIG_SENSORS_ADS1000)	+= ads1000.o
> > >  obj-$(CONFIG_SENSORS_ADS1015)	+= ads1015.o
> > >  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
> > >  obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
> > > diff --git a/drivers/hwmon/ads1000.c b/drivers/hwmon/ads1000.c
> > > new file mode 100644
> > > index 000000000000..a88b738f56bd
> > > --- /dev/null
> > > +++ b/drivers/hwmon/ads1000.c
> > > @@ -0,0 +1,320 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for ADS1000/ADS1100 12-16-bit ADC
> > > + *
> > > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > > + *
> > > + * Based on the ads1015 driver by Dirk Eibach.
> > > + *
> > > + * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads1000.pdf
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> > > +#include <linux/err.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_data/ads1000.h>
> > > +
> > > +/* Data rates scale table */
> > > +static const unsigned int scale_table[4] = {
> > > +	2048, 8192, 16384, 32768
> > > +};
> > > +
> > > +/* Minimal data rates in samples per second */
> > > +static const unsigned int data_rate_table[4] = {
> > > +	100, 25, 12, 5
> > > +};
> > > +
> > > +#define ADS1000_DEFAULT_PGA 0
> > > +#define ADS1000_DEFAULT_DATA_RATE 0
> > > +#define ADS1000_DEFAULT_R1_DIVIDER 0
> > > +#define ADS1000_DEFAULT_R2_DIVIDER 0
> > > +
> > > +enum ads1000_chips {
> > > +	ads1000,
> > > +	ads1100,
> > > +};
> > > +
> > > +struct ads1000 {
> > > +	struct device *hwmon_dev;
> > > +	struct mutex update_lock;
> > > +	struct i2c_client *client;
> > > +	struct ads1000_platform_data data;
> > > +	enum ads1000_chips id;
> > > +};
> > > +
> > > +static inline int ads1000_enable_vdd(struct ads1000 *priv)
> > > +{
> > > +	return regulator_enable(priv->data.vdd);
> > > +}
> > > +
> > > +static inline int ads1000_get_vdd(struct ads1000 *priv)
> > > +{
> > > +	return regulator_get_voltage(priv->data.vdd);
> > > +}
> > > +
> > > +static int ads1000_read_adc(struct ads1000 *priv)
> > > +{
> > > +	struct i2c_client *client = priv->client;
> > > +	unsigned int delay_ms;
> > > +	u8 data[3] = {0};
> > > +	int res;
> > > +
> > > +	mutex_lock(&priv->update_lock);
> > > +
> > > +	delay_ms = DIV_ROUND_UP(1000, data_rate_table[priv->data.data_rate]);
> > > +
> > > +	/* setup and start single conversion */
> > > +	data[2] |= (1 << 7) | (1 << 4);
> > > +	data[2] |= priv->data.pga;
> > > +	data[2] |= priv->data.data_rate << 2;
> > > +
> > > +	res = i2c_master_send(client, &data[2], 1);
> > > +	if (res < 0)
> > > +		goto err_unlock;
> > > +
> > > +	/* wait until conversion finished */
> > > +	msleep(delay_ms);
> > > +	res = i2c_master_recv(client, data, 3);
> > > +	if (res < 0)
> > > +		goto err_unlock;
> > > +
> > > +	if (data[2] & (1 << 7)) {
> > > +		res = -EIO;
> > > +		goto err_unlock;
> > > +	}
> > > +
> > > +	res = ((u16)data[0] << 8) | data[1];
> > > +
> > > +err_unlock:
> > > +	mutex_unlock(&priv->update_lock);
> > > +
> > > +	return res;
> > > +}
> > > +
> > > +static int ads1000_reg_to_mv(struct ads1000 *priv, s16 reg)
> > > +{
> > > +	unsigned int *divider = priv->data.divider;
> > > +	int voltage = ads1000_get_vdd(priv);
> > > +	int gain = 1 << priv->data.pga;
> > > +	int c = 0;
> > > +
> > > +	voltage = reg*DIV_ROUND_CLOSEST(voltage, 1000);
> > > +	gain = gain*scale_table[priv->data.data_rate];
> > > +	voltage = DIV_ROUND_CLOSEST(voltage, gain);
> > > +
> > > +	if (divider[0] && divider[1]) {
> > > +		c = divider[0]*voltage;
> > > +		c = DIV_ROUND_CLOSEST(c, (int)divider[1]);
> > > +	}
> > > +
> > > +	return voltage + c;
> > > +}
> > > +
> > > +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> > > +		       char *buf)
> > > +{
> > > +	struct ads1000 *priv = dev_get_drvdata(dev);
> > > +	int res;
> > > +
> > > +	res = ads1000_read_adc(priv);
> > > +	if (res < 0)
> > > +		return res;
> > > +
> > > +	return sprintf(buf, "%d\n", ads1000_reg_to_mv(priv, res));
> > > +}
> > > +
> > > +static SENSOR_DEVICE_ATTR(in0_input, 0444, show_in, NULL, 0);
> > > +
> > > +static struct attribute *ads1000_attrs[] = {
> > > +	&sensor_dev_attr_in0_input.dev_attr.attr,
> > > +	NULL
> > > +};
> > > +ATTRIBUTE_GROUPS(ads1000);
> > > +
> > > +static struct ads1000 *ads1000_create_priv(struct i2c_client *client,
> > > +					   const struct i2c_device_id *id)
> > > +{
> > > +	struct ads1000 *priv;
> > > +
> > > +	priv = devm_kzalloc(&client->dev, sizeof(struct ads1000),
> > > +			    GFP_KERNEL);
> > > +	if (!priv)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	if (client->dev.of_node)
> > > +		priv->id = (enum ads1000_chips)
> > > +			of_device_get_match_data(&client->dev);
> > > +	else
> > > +		priv->id = id->driver_data;
> > > +
> > > +	i2c_set_clientdata(client, priv);
> > > +	priv->client = client;
> > > +	mutex_init(&priv->update_lock);
> > > +
> > > +	return priv;
> > > +}
> > > +
> > > +#ifdef CONFIG_OF
> > > +static int ads1000_get_config_of(struct ads1000 *priv)
> > > +{
> > > +	struct i2c_client *client = priv->client;
> > > +	struct device_node *node = client->dev.of_node;
> > > +	u32 divider[2];
> > > +	u32 val;
> > > +
> > > +	if (!node)
> > > +		return -EINVAL;
> > > +
> > > +	if (!of_property_read_u32(node, "ti,gain", &val))
> > > +		priv->data.pga = val;
> > > +
> > > +	if (!of_property_read_u32(node, "ti,datarate", &val))
> > > +		priv->data.data_rate = val;
> > > +
> > > +	if (!of_property_read_u32_array(node, "ti,voltage-divider",
> > > +					divider, 2)) {
> > > +		priv->data.divider[0] = divider[0];
> > > +		priv->data.divider[1] = divider[1];
> > > +	}
> > > +
> > > +	priv->data.vdd = devm_regulator_get(&client->dev, "vdd");
> > > +	if (IS_ERR(priv->data.vdd))
> > > +		return PTR_ERR(priv->data.vdd);
> > > +
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > > +static int ads1000_get_config(struct ads1000 *priv)
> > > +{
> > > +	struct i2c_client *client = priv->client;
> > > +	struct ads1000_platform_data *pdata = dev_get_platdata(&client->dev);
> > > +
> > > +	priv->data.pga = ADS1000_DEFAULT_PGA;
> > > +	priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > > +	priv->data.divider[0] = ADS1000_DEFAULT_R1_DIVIDER;
> > > +	priv->data.divider[1] = ADS1000_DEFAULT_R2_DIVIDER;
> > > +
> > > +	/* prefer platform data */
> > > +	if (pdata) {
> > > +		memcpy(&priv->data, pdata, sizeof(priv->data));
> > > +	} else {
> > > +#ifdef CONFIG_OF
> > > +		int ret;
> > > +
> > > +		ret = ads1000_get_config_of(priv);
> > > +		if (ret)
> > > +			return ret;
> > > +#endif
> > > +	}
> > > +
> > > +	if (!priv->data.vdd) {
> > > +		dev_err(&client->dev, "No VDD regulator\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (priv->data.pga > 4) {
> > > +		dev_err(&client->dev, "Invalid gain, using default\n");
> > > +		priv->data.pga = ADS1000_DEFAULT_PGA;
> > > +	}
> > > +
> > > +	if (priv->data.data_rate > 4) {
> > > +		dev_err(&client->dev, "Invalid datarate, using default\n");
> > > +		priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > > +	}
> > > +
> > > +	if (priv->id == ads1000 && priv->data.data_rate != 0) {
> > > +		dev_warn(&client->dev, "ADC data rate can be 128SPS only\n");
> > > +		priv->data.data_rate = 0;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ads1000_set_config(struct ads1000 *priv)
> > > +{
> > > +	u8 data = 0;
> > > +	int ret;
> > > +
> > > +	/* disable continuous conversion */
> > > +	data |= (1 << 4);
> > > +	data |= priv->data.pga;
> > > +	data |= priv->data.data_rate << 2;
> > > +
> > > +	ret = i2c_master_send(priv->client, &data, 1);
> > > +
> > > +	return ret < 0 ? ret : 0;
> > > +}
> > > +
> > > +static int ads1000_probe(struct i2c_client *client,
> > > +			 const struct i2c_device_id *id)
> > > +{
> > > +	struct ads1000 *priv;
> > > +	int ret;
> > > +
> > > +	priv = ads1000_create_priv(client, id);
> > > +	if (IS_ERR(priv))
> > > +		return PTR_ERR(priv);
> > > +
> > > +	ret = ads1000_get_config(priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ads1000_enable_vdd(priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ads1000_set_config(priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	priv->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
> > > +				client->name, priv, ads1000_groups);
> > > +	if (IS_ERR(priv->hwmon_dev))
> > > +		return PTR_ERR(priv->hwmon_dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ads1000_id[] = {
> > > +	{ "ads1000",  ads1000},
> > > +	{ "ads1100",  ads1100},
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ads1000_id);
> > > +
> > > +static const struct of_device_id ads1000_of_match[] = {
> > > +	{
> > > +		.compatible = "ti,ads1000",
> > > +		.data = (void *)ads1000
> > > +	},
> > > +	{
> > > +		.compatible = "ti,ads1100",
> > > +		.data = (void *)ads1100
> > > +	},
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ads1000_of_match);
> > > +
> > > +static struct i2c_driver ads1000_driver = {
> > > +	.driver = {
> > > +		.name = "ads1000",
> > > +		.of_match_table = of_match_ptr(ads1000_of_match),
> > > +	},
> > > +	.probe = ads1000_probe,
> > > +	.id_table = ads1000_id,
> > > +};
> > > +module_i2c_driver(ads1000_driver);
> > > +
> > > +MODULE_AUTHOR("Serge Semin <fancer.lancer@gmail.com>");
> > > +MODULE_DESCRIPTION("ADS1000 driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/linux/platform_data/ads1000.h b/include/linux/platform_data/ads1000.h
> > > new file mode 100644
> > > index 000000000000..979670483537
> > > --- /dev/null
> > > +++ b/include/linux/platform_data/ads1000.h
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Platform Data for ADS1000/ADS1100 12-16-bit ADC
> > > + *
> > > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > > + */
> > > +
> > > +#ifndef LINUX_ADS1000_H
> > > +#define LINUX_ADS1000_H
> > > +
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +struct ads1000_platform_data {
> > > +	unsigned int pga;
> > > +	unsigned int data_rate;
> > > +	struct regulator *vdd;
> > > +	unsigned int divider[2];
> > > +};
> > > +
> > > +#endif /* LINUX_ADS1000_H */  
> 
> 

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Serge Semin]

Hello folks

On Wed, Jun 05, 2019 at 01:55:56PM -0700, Guenter Roeck wrote:
> On Mon, Jun 03, 2019 at 12:11:17PM +0100, Jonathan Cameron wrote:
> > On Thu, 30 May 2019 05:55:10 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > > Hi,
> > > 
> > > On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > > > These are simple Texas Instruments ADC working over i2c-interface with
> > > > just one differential input and with configurable 12-16 bits resolution.
> > > > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > > > ads1100. Vdd value reference value must be supplied so to properly
> > > > translate the sampled code to the real voltage. All of these configs are
> > > > implemented in the device drivers for hwmon subsystem. The next dts
> > > > properties should be specified to comply the device platform setup:
> > > >  - vdd-supply - voltage regulator connected to the Vdd pin of the device
> > > >  - ti,gain - programmable gain amplifier
> > > >  - ti,datarate - converter data rate
> > > >  - ti,voltage-divider - possible resistors-base external divider
> > > > See bindings documentation file for details.
> > > > 
> > > > Even though these devices seem more like ads1015 series, they
> > > > in fact pretty much different. First of all ads1000/ads1100 got less
> > > > capabilities: just one port, no configurations of digital comparator, no
> > > > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > > > In addition they haven't got internal voltage reference, but instead
> > > > are created to use Vdd pin voltage. Finally the output code value is
> > > > provided in different format. As a result it was much easier for
> > > > development and for future support to create a separate driver.
> > > >   
> > > 
> > > This chicp doesn't have any real hardware monitoring characteristics
> > > (no limit registers). It seems to be better suited to be implemented
> > > as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> > > should work just fine.
> > > 
> > > Jonathan, what do you think ?
> > Sorry for slow response, was on vacation.
> > 
> > Agreed, this looks like a standard multipurpose ADC so probably more suited
> > to IIO. Whether you bother with a buffered /chardev interface or not given it
> > is a fairly slow device is a separate question (can always be added later
> > when someone wants it).
> > 
> > Note the voltage-divider in the DT properties is something that should
> > have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
> > for that, in this case using the voltage divider binding.
> > 
> > gain and datarate are both characteristics that should be controlled from
> > userspace rather than via a binding.
> > 
> 
> In summary: Serge, please re-implement the driver as iio adc driver.
> 

Thanks for the comments. I see your point, but since you are asking of a pretty
much serious code redevelopment, I want to make sure it is fully justified.

I made my decision of creating the hwmon driver following the next logic.
Before I started this driver development, I searched the kernel for either a
ready-to-use code or for a similar device driver to add the ads1000 ADC support.
I found the ads1015 driver, which is created for TI ADC1015 ADCs. These devices
are similar to the ads1000 series, but are more complex. Due to the complexity
I decided to create a separate driver for ads1000s, and of course since the similar
device driver lived in hwmon, I chose it to be home of my new driver.

But now you are asking me to move it to IIO, while the driver of more complex
ads1015 device exists in the hwmon subsystem of the kernel. Moreover the ads1000
device is utilized on our board to monitor system itself (voltage on the input
DC-DC). Could you please tell me why the driver should really be in IIO instead
of hwmon and how do you select which subsystem one or another driver is supposed
to live in?

Regards,
-Sergey

> Thanks,
> Guenter
> 
> > Thanks,
> > 
> > Jonathan
> > > 
> > > Thanks,
> > > Guenter
> > > 
> > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > > ---
> > > >  MAINTAINERS                           |   8 +
> > > >  drivers/hwmon/Kconfig                 |  10 +
> > > >  drivers/hwmon/Makefile                |   1 +
> > > >  drivers/hwmon/ads1000.c               | 320 ++++++++++++++++++++++++++
> > > >  include/linux/platform_data/ads1000.h |  20 ++
> > > >  5 files changed, 359 insertions(+)
> > > >  create mode 100644 drivers/hwmon/ads1000.c
> > > >  create mode 100644 include/linux/platform_data/ads1000.h
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index ce573aaa04df..5c3a8107ef1a 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -517,6 +517,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
> > > >  S:	Supported
> > > >  F:	drivers/video/backlight/adp8860_bl.c
> > > >  
> > > > +ADS1000 HARDWARE MONITOR DRIVER
> > > > +M:	Serge Semin <fancer.lancer@gmail.com>
> > > > +L:	linux-hwmon@vger.kernel.org
> > > > +S:	Maintained
> > > > +F:	Documentation/hwmon/ads1000.rst
> > > > +F:	drivers/hwmon/ads1000.c
> > > > +F:	include/linux/platform_data/ads1000.h
> > > > +
> > > >  ADS1015 HARDWARE MONITOR DRIVER
> > > >  M:	Dirk Eibach <eibach@gdsys.de>
> > > >  L:	linux-hwmon@vger.kernel.org
> > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > > index 1915a18b537b..a1220cc48f2f 100644
> > > > --- a/drivers/hwmon/Kconfig
> > > > +++ b/drivers/hwmon/Kconfig
> > > > @@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
> > > >  	  This driver can also be built as a module. If so, the module
> > > >  	  will be called adc128d818.
> > > >  
> > > > +config SENSORS_ADS1000
> > > > +	tristate "Texas Instruments ADS1000"
> > > > +	depends on I2C
> > > > +	help
> > > > +	  If you say yes here you get support for Texas Instruments
> > > > +	  ADS1000/ADS1100 12-16-bit single channel ADC device.
> > > > +
> > > > +	  This driver can also be built as a module.  If so, the module
> > > > +	  will be called ads1000.
> > > > +
> > > >  config SENSORS_ADS1015
> > > >  	tristate "Texas Instruments ADS1015"
> > > >  	depends on I2C
> > > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > > index 8db472ea04f0..2cd82f6c651e 100644
> > > > --- a/drivers/hwmon/Makefile
> > > > +++ b/drivers/hwmon/Makefile
> > > > @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
> > > >  obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
> > > >  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
> > > >  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
> > > > +obj-$(CONFIG_SENSORS_ADS1000)	+= ads1000.o
> > > >  obj-$(CONFIG_SENSORS_ADS1015)	+= ads1015.o
> > > >  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
> > > >  obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
> > > > diff --git a/drivers/hwmon/ads1000.c b/drivers/hwmon/ads1000.c
> > > > new file mode 100644
> > > > index 000000000000..a88b738f56bd
> > > > --- /dev/null
> > > > +++ b/drivers/hwmon/ads1000.c
> > > > @@ -0,0 +1,320 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Driver for ADS1000/ADS1100 12-16-bit ADC
> > > > + *
> > > > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > > > + *
> > > > + * Based on the ads1015 driver by Dirk Eibach.
> > > > + *
> > > > + * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads1000.pdf
> > > > + */
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/hwmon.h>
> > > > +#include <linux/hwmon-sysfs.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/platform_data/ads1000.h>
> > > > +
> > > > +/* Data rates scale table */
> > > > +static const unsigned int scale_table[4] = {
> > > > +	2048, 8192, 16384, 32768
> > > > +};
> > > > +
> > > > +/* Minimal data rates in samples per second */
> > > > +static const unsigned int data_rate_table[4] = {
> > > > +	100, 25, 12, 5
> > > > +};
> > > > +
> > > > +#define ADS1000_DEFAULT_PGA 0
> > > > +#define ADS1000_DEFAULT_DATA_RATE 0
> > > > +#define ADS1000_DEFAULT_R1_DIVIDER 0
> > > > +#define ADS1000_DEFAULT_R2_DIVIDER 0
> > > > +
> > > > +enum ads1000_chips {
> > > > +	ads1000,
> > > > +	ads1100,
> > > > +};
> > > > +
> > > > +struct ads1000 {
> > > > +	struct device *hwmon_dev;
> > > > +	struct mutex update_lock;
> > > > +	struct i2c_client *client;
> > > > +	struct ads1000_platform_data data;
> > > > +	enum ads1000_chips id;
> > > > +};
> > > > +
> > > > +static inline int ads1000_enable_vdd(struct ads1000 *priv)
> > > > +{
> > > > +	return regulator_enable(priv->data.vdd);
> > > > +}
> > > > +
> > > > +static inline int ads1000_get_vdd(struct ads1000 *priv)
> > > > +{
> > > > +	return regulator_get_voltage(priv->data.vdd);
> > > > +}
> > > > +
> > > > +static int ads1000_read_adc(struct ads1000 *priv)
> > > > +{
> > > > +	struct i2c_client *client = priv->client;
> > > > +	unsigned int delay_ms;
> > > > +	u8 data[3] = {0};
> > > > +	int res;
> > > > +
> > > > +	mutex_lock(&priv->update_lock);
> > > > +
> > > > +	delay_ms = DIV_ROUND_UP(1000, data_rate_table[priv->data.data_rate]);
> > > > +
> > > > +	/* setup and start single conversion */
> > > > +	data[2] |= (1 << 7) | (1 << 4);
> > > > +	data[2] |= priv->data.pga;
> > > > +	data[2] |= priv->data.data_rate << 2;
> > > > +
> > > > +	res = i2c_master_send(client, &data[2], 1);
> > > > +	if (res < 0)
> > > > +		goto err_unlock;
> > > > +
> > > > +	/* wait until conversion finished */
> > > > +	msleep(delay_ms);
> > > > +	res = i2c_master_recv(client, data, 3);
> > > > +	if (res < 0)
> > > > +		goto err_unlock;
> > > > +
> > > > +	if (data[2] & (1 << 7)) {
> > > > +		res = -EIO;
> > > > +		goto err_unlock;
> > > > +	}
> > > > +
> > > > +	res = ((u16)data[0] << 8) | data[1];
> > > > +
> > > > +err_unlock:
> > > > +	mutex_unlock(&priv->update_lock);
> > > > +
> > > > +	return res;
> > > > +}
> > > > +
> > > > +static int ads1000_reg_to_mv(struct ads1000 *priv, s16 reg)
> > > > +{
> > > > +	unsigned int *divider = priv->data.divider;
> > > > +	int voltage = ads1000_get_vdd(priv);
> > > > +	int gain = 1 << priv->data.pga;
> > > > +	int c = 0;
> > > > +
> > > > +	voltage = reg*DIV_ROUND_CLOSEST(voltage, 1000);
> > > > +	gain = gain*scale_table[priv->data.data_rate];
> > > > +	voltage = DIV_ROUND_CLOSEST(voltage, gain);
> > > > +
> > > > +	if (divider[0] && divider[1]) {
> > > > +		c = divider[0]*voltage;
> > > > +		c = DIV_ROUND_CLOSEST(c, (int)divider[1]);
> > > > +	}
> > > > +
> > > > +	return voltage + c;
> > > > +}
> > > > +
> > > > +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> > > > +		       char *buf)
> > > > +{
> > > > +	struct ads1000 *priv = dev_get_drvdata(dev);
> > > > +	int res;
> > > > +
> > > > +	res = ads1000_read_adc(priv);
> > > > +	if (res < 0)
> > > > +		return res;
> > > > +
> > > > +	return sprintf(buf, "%d\n", ads1000_reg_to_mv(priv, res));
> > > > +}
> > > > +
> > > > +static SENSOR_DEVICE_ATTR(in0_input, 0444, show_in, NULL, 0);
> > > > +
> > > > +static struct attribute *ads1000_attrs[] = {
> > > > +	&sensor_dev_attr_in0_input.dev_attr.attr,
> > > > +	NULL
> > > > +};
> > > > +ATTRIBUTE_GROUPS(ads1000);
> > > > +
> > > > +static struct ads1000 *ads1000_create_priv(struct i2c_client *client,
> > > > +					   const struct i2c_device_id *id)
> > > > +{
> > > > +	struct ads1000 *priv;
> > > > +
> > > > +	priv = devm_kzalloc(&client->dev, sizeof(struct ads1000),
> > > > +			    GFP_KERNEL);
> > > > +	if (!priv)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	if (client->dev.of_node)
> > > > +		priv->id = (enum ads1000_chips)
> > > > +			of_device_get_match_data(&client->dev);
> > > > +	else
> > > > +		priv->id = id->driver_data;
> > > > +
> > > > +	i2c_set_clientdata(client, priv);
> > > > +	priv->client = client;
> > > > +	mutex_init(&priv->update_lock);
> > > > +
> > > > +	return priv;
> > > > +}
> > > > +
> > > > +#ifdef CONFIG_OF
> > > > +static int ads1000_get_config_of(struct ads1000 *priv)
> > > > +{
> > > > +	struct i2c_client *client = priv->client;
> > > > +	struct device_node *node = client->dev.of_node;
> > > > +	u32 divider[2];
> > > > +	u32 val;
> > > > +
> > > > +	if (!node)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!of_property_read_u32(node, "ti,gain", &val))
> > > > +		priv->data.pga = val;
> > > > +
> > > > +	if (!of_property_read_u32(node, "ti,datarate", &val))
> > > > +		priv->data.data_rate = val;
> > > > +
> > > > +	if (!of_property_read_u32_array(node, "ti,voltage-divider",
> > > > +					divider, 2)) {
> > > > +		priv->data.divider[0] = divider[0];
> > > > +		priv->data.divider[1] = divider[1];
> > > > +	}
> > > > +
> > > > +	priv->data.vdd = devm_regulator_get(&client->dev, "vdd");
> > > > +	if (IS_ERR(priv->data.vdd))
> > > > +		return PTR_ERR(priv->data.vdd);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > > +static int ads1000_get_config(struct ads1000 *priv)
> > > > +{
> > > > +	struct i2c_client *client = priv->client;
> > > > +	struct ads1000_platform_data *pdata = dev_get_platdata(&client->dev);
> > > > +
> > > > +	priv->data.pga = ADS1000_DEFAULT_PGA;
> > > > +	priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > > > +	priv->data.divider[0] = ADS1000_DEFAULT_R1_DIVIDER;
> > > > +	priv->data.divider[1] = ADS1000_DEFAULT_R2_DIVIDER;
> > > > +
> > > > +	/* prefer platform data */
> > > > +	if (pdata) {
> > > > +		memcpy(&priv->data, pdata, sizeof(priv->data));
> > > > +	} else {
> > > > +#ifdef CONFIG_OF
> > > > +		int ret;
> > > > +
> > > > +		ret = ads1000_get_config_of(priv);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +#endif
> > > > +	}
> > > > +
> > > > +	if (!priv->data.vdd) {
> > > > +		dev_err(&client->dev, "No VDD regulator\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (priv->data.pga > 4) {
> > > > +		dev_err(&client->dev, "Invalid gain, using default\n");
> > > > +		priv->data.pga = ADS1000_DEFAULT_PGA;
> > > > +	}
> > > > +
> > > > +	if (priv->data.data_rate > 4) {
> > > > +		dev_err(&client->dev, "Invalid datarate, using default\n");
> > > > +		priv->data.data_rate = ADS1000_DEFAULT_DATA_RATE;
> > > > +	}
> > > > +
> > > > +	if (priv->id == ads1000 && priv->data.data_rate != 0) {
> > > > +		dev_warn(&client->dev, "ADC data rate can be 128SPS only\n");
> > > > +		priv->data.data_rate = 0;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int ads1000_set_config(struct ads1000 *priv)
> > > > +{
> > > > +	u8 data = 0;
> > > > +	int ret;
> > > > +
> > > > +	/* disable continuous conversion */
> > > > +	data |= (1 << 4);
> > > > +	data |= priv->data.pga;
> > > > +	data |= priv->data.data_rate << 2;
> > > > +
> > > > +	ret = i2c_master_send(priv->client, &data, 1);
> > > > +
> > > > +	return ret < 0 ? ret : 0;
> > > > +}
> > > > +
> > > > +static int ads1000_probe(struct i2c_client *client,
> > > > +			 const struct i2c_device_id *id)
> > > > +{
> > > > +	struct ads1000 *priv;
> > > > +	int ret;
> > > > +
> > > > +	priv = ads1000_create_priv(client, id);
> > > > +	if (IS_ERR(priv))
> > > > +		return PTR_ERR(priv);
> > > > +
> > > > +	ret = ads1000_get_config(priv);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = ads1000_enable_vdd(priv);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = ads1000_set_config(priv);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	priv->hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
> > > > +				client->name, priv, ads1000_groups);
> > > > +	if (IS_ERR(priv->hwmon_dev))
> > > > +		return PTR_ERR(priv->hwmon_dev);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct i2c_device_id ads1000_id[] = {
> > > > +	{ "ads1000",  ads1000},
> > > > +	{ "ads1100",  ads1100},
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, ads1000_id);
> > > > +
> > > > +static const struct of_device_id ads1000_of_match[] = {
> > > > +	{
> > > > +		.compatible = "ti,ads1000",
> > > > +		.data = (void *)ads1000
> > > > +	},
> > > > +	{
> > > > +		.compatible = "ti,ads1100",
> > > > +		.data = (void *)ads1100
> > > > +	},
> > > > +	{ },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, ads1000_of_match);
> > > > +
> > > > +static struct i2c_driver ads1000_driver = {
> > > > +	.driver = {
> > > > +		.name = "ads1000",
> > > > +		.of_match_table = of_match_ptr(ads1000_of_match),
> > > > +	},
> > > > +	.probe = ads1000_probe,
> > > > +	.id_table = ads1000_id,
> > > > +};
> > > > +module_i2c_driver(ads1000_driver);
> > > > +
> > > > +MODULE_AUTHOR("Serge Semin <fancer.lancer@gmail.com>");
> > > > +MODULE_DESCRIPTION("ADS1000 driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > diff --git a/include/linux/platform_data/ads1000.h b/include/linux/platform_data/ads1000.h
> > > > new file mode 100644
> > > > index 000000000000..979670483537
> > > > --- /dev/null
> > > > +++ b/include/linux/platform_data/ads1000.h
> > > > @@ -0,0 +1,20 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Platform Data for ADS1000/ADS1100 12-16-bit ADC
> > > > + *
> > > > + * Copyright (C) 2019 T-platforms JSC (fancer.lancer@gmail.com)
> > > > + */
> > > > +
> > > > +#ifndef LINUX_ADS1000_H
> > > > +#define LINUX_ADS1000_H
> > > > +
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +struct ads1000_platform_data {
> > > > +	unsigned int pga;
> > > > +	unsigned int data_rate;
> > > > +	struct regulator *vdd;
> > > > +	unsigned int divider[2];
> > > > +};
> > > > +
> > > > +#endif /* LINUX_ADS1000_H */  
> > 
> > 

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Guenter Roeck]

On 6/7/19 4:01 PM, Serge Semin wrote:
> Hello folks
> 
> On Wed, Jun 05, 2019 at 01:55:56PM -0700, Guenter Roeck wrote:
>> On Mon, Jun 03, 2019 at 12:11:17PM +0100, Jonathan Cameron wrote:
>>> On Thu, 30 May 2019 05:55:10 -0700
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
>>>>> These are simple Texas Instruments ADC working over i2c-interface with
>>>>> just one differential input and with configurable 12-16 bits resolution.
>>>>> Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
>>>>> ads1100. Vdd value reference value must be supplied so to properly
>>>>> translate the sampled code to the real voltage. All of these configs are
>>>>> implemented in the device drivers for hwmon subsystem. The next dts
>>>>> properties should be specified to comply the device platform setup:
>>>>>   - vdd-supply - voltage regulator connected to the Vdd pin of the device
>>>>>   - ti,gain - programmable gain amplifier
>>>>>   - ti,datarate - converter data rate
>>>>>   - ti,voltage-divider - possible resistors-base external divider
>>>>> See bindings documentation file for details.
>>>>>
>>>>> Even though these devices seem more like ads1015 series, they
>>>>> in fact pretty much different. First of all ads1000/ads1100 got less
>>>>> capabilities: just one port, no configurations of digital comparator, no
>>>>> input multi-channel multiplexer, smaller PGA and data-rate ranges.
>>>>> In addition they haven't got internal voltage reference, but instead
>>>>> are created to use Vdd pin voltage. Finally the output code value is
>>>>> provided in different format. As a result it was much easier for
>>>>> development and for future support to create a separate driver.
>>>>>    
>>>>
>>>> This chicp doesn't have any real hardware monitoring characteristics
>>>> (no limit registers). It seems to be better suited to be implemented
>>>> as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
>>>> should work just fine.
>>>>
>>>> Jonathan, what do you think ?
>>> Sorry for slow response, was on vacation.
>>>
>>> Agreed, this looks like a standard multipurpose ADC so probably more suited
>>> to IIO. Whether you bother with a buffered /chardev interface or not given it
>>> is a fairly slow device is a separate question (can always be added later
>>> when someone wants it).
>>>
>>> Note the voltage-divider in the DT properties is something that should
>>> have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
>>> for that, in this case using the voltage divider binding.
>>>
>>> gain and datarate are both characteristics that should be controlled from
>>> userspace rather than via a binding.
>>>
>>
>> In summary: Serge, please re-implement the driver as iio adc driver.
>>
> 
> Thanks for the comments. I see your point, but since you are asking of a pretty
> much serious code redevelopment, I want to make sure it is fully justified.
> 
> I made my decision of creating the hwmon driver following the next logic.
> Before I started this driver development, I searched the kernel for either a
> ready-to-use code or for a similar device driver to add the ads1000 ADC support.
> I found the ads1015 driver, which is created for TI ADC1015 ADCs. These devices
> are similar to the ads1000 series, but are more complex. Due to the complexity
> I decided to create a separate driver for ads1000s, and of course since the similar
> device driver lived in hwmon, I chose it to be home of my new driver.
> 
> But now you are asking me to move it to IIO, while the driver of more complex
> ads1015 device exists in the hwmon subsystem of the kernel. Moreover the ads1000

A driver for ADS1015 also exists in drivers/iio/adc/ti-ads1015.c, meaning there
are already two drivers for that chip. Accepting the driver for ads1000 into
hwmon would ultimately mean that we would end up with another duplicate driver,
as soon as someone needs iio support for this chip. From hwmon perspective,
that driver would have zero additional functionality.

Users would then have to choose between the hwmon ads1000 driver and the iio
ads1000 driver plus iio->hwmon bridge. The kernel maintainers would have to
maintain two drivers instead of one, for no good reason. We would therefore
at that time remove hwmon driver from the kernel because it doesn't make sense
to keep two drivers for the same chip if both drivers provide exactly the same
functionality. This just doesn't make sense.

On top of that, the ads1000 has zero characteristics of a typical hardware
monitoring chip. It doesn't have any limit or alarm status registers.

> device is utilized on our board to monitor system itself (voltage on the input
> DC-DC). Could you please tell me why the driver should really be in IIO instead
> of hwmon and how do you select which subsystem one or another driver is supposed
> to live in?
> 
If a chip has typical hardware monitoring characteristics such as slow but accurate
conversion rates and limit/alarm registers, we are happy to accept it into the
hardware monitoring subsystem. If the chip has no such characteristics,
it should be implemented as iio driver.

Actually, we should remove the ads1015 driver from the hwmon subsystem.
I'll start a separate thread to discuss that.

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Serge Semin]

Hello Guenter

On Fri, Jun 07, 2019 at 06:02:48PM -0700, Guenter Roeck wrote:
> On 6/7/19 4:01 PM, Serge Semin wrote:
> > Hello folks
> > 
> > On Wed, Jun 05, 2019 at 01:55:56PM -0700, Guenter Roeck wrote:
> > > On Mon, Jun 03, 2019 at 12:11:17PM +0100, Jonathan Cameron wrote:
> > > > On Thu, 30 May 2019 05:55:10 -0700
> > > > Guenter Roeck <linux@roeck-us.net> wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > > > > > These are simple Texas Instruments ADC working over i2c-interface with
> > > > > > just one differential input and with configurable 12-16 bits resolution.
> > > > > > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > > > > > ads1100. Vdd value reference value must be supplied so to properly
> > > > > > translate the sampled code to the real voltage. All of these configs are
> > > > > > implemented in the device drivers for hwmon subsystem. The next dts
> > > > > > properties should be specified to comply the device platform setup:
> > > > > >   - vdd-supply - voltage regulator connected to the Vdd pin of the device
> > > > > >   - ti,gain - programmable gain amplifier
> > > > > >   - ti,datarate - converter data rate
> > > > > >   - ti,voltage-divider - possible resistors-base external divider
> > > > > > See bindings documentation file for details.
> > > > > > 
> > > > > > Even though these devices seem more like ads1015 series, they
> > > > > > in fact pretty much different. First of all ads1000/ads1100 got less
> > > > > > capabilities: just one port, no configurations of digital comparator, no
> > > > > > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > > > > > In addition they haven't got internal voltage reference, but instead
> > > > > > are created to use Vdd pin voltage. Finally the output code value is
> > > > > > provided in different format. As a result it was much easier for
> > > > > > development and for future support to create a separate driver.
> > > > > 
> > > > > This chicp doesn't have any real hardware monitoring characteristics
> > > > > (no limit registers). It seems to be better suited to be implemented
> > > > > as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> > > > > should work just fine.
> > > > > 
> > > > > Jonathan, what do you think ?
> > > > Sorry for slow response, was on vacation.
> > > > 
> > > > Agreed, this looks like a standard multipurpose ADC so probably more suited
> > > > to IIO. Whether you bother with a buffered /chardev interface or not given it
> > > > is a fairly slow device is a separate question (can always be added later
> > > > when someone wants it).
> > > > 
> > > > Note the voltage-divider in the DT properties is something that should
> > > > have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
> > > > for that, in this case using the voltage divider binding.
> > > > 
> > > > gain and datarate are both characteristics that should be controlled from
> > > > userspace rather than via a binding.
> > > > 
> > > 
> > > In summary: Serge, please re-implement the driver as iio adc driver.
> > > 
> > 
> > Thanks for the comments. I see your point, but since you are asking of a pretty
> > much serious code redevelopment, I want to make sure it is fully justified.
> > 
> > I made my decision of creating the hwmon driver following the next logic.
> > Before I started this driver development, I searched the kernel for either a
> > ready-to-use code or for a similar device driver to add the ads1000 ADC support.
> > I found the ads1015 driver, which is created for TI ADC1015 ADCs. These devices
> > are similar to the ads1000 series, but are more complex. Due to the complexity
> > I decided to create a separate driver for ads1000s, and of course since the similar
> > device driver lived in hwmon, I chose it to be home of my new driver.
> > 
> > But now you are asking me to move it to IIO, while the driver of more complex
> > ads1015 device exists in the hwmon subsystem of the kernel. Moreover the ads1000
> 
> A driver for ADS1015 also exists in drivers/iio/adc/ti-ads1015.c, meaning there
> are already two drivers for that chip. Accepting the driver for ads1000 into
> hwmon would ultimately mean that we would end up with another duplicate driver,
> as soon as someone needs iio support for this chip. From hwmon perspective,
> that driver would have zero additional functionality.
> 
> Users would then have to choose between the hwmon ads1000 driver and the iio
> ads1000 driver plus iio->hwmon bridge. The kernel maintainers would have to
> maintain two drivers instead of one, for no good reason. We would therefore
> at that time remove hwmon driver from the kernel because it doesn't make sense
> to keep two drivers for the same chip if both drivers provide exactly the same
> functionality. This just doesn't make sense.
> 
> On top of that, the ads1000 has zero characteristics of a typical hardware
> monitoring chip. It doesn't have any limit or alarm status registers.
> 
> > device is utilized on our board to monitor system itself (voltage on the input
> > DC-DC). Could you please tell me why the driver should really be in IIO instead
> > of hwmon and how do you select which subsystem one or another driver is supposed
> > to live in?
> > 
> If a chip has typical hardware monitoring characteristics such as slow but accurate
> conversion rates and limit/alarm registers, we are happy to accept it into the
> hardware monitoring subsystem. If the chip has no such characteristics,
> it should be implemented as iio driver.
> 
> Actually, we should remove the ads1015 driver from the hwmon subsystem.
> I'll start a separate thread to discuss that.
> 

Thank you very much for the detailed explanation. I can't believe I missed that
iio-version of ads1015. I'll take a look at that. If it's possible to add
ads1000 support right into the ads1015 drive, I'll do this. Otherwise I'll
port the ads1000 to the IIO subsystem as you suggested.

Regards,
-Sergey

> Thanks,
> Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: Add DT bindings for TI ads1000/ads1100 ADCs

[Rob Herring]

On Wed, May 15, 2019 at 01:58:08AM +0300, Serge Semin wrote:
> Add dt-binding documentation for the Texas Instruments ads1000/ads1100 ADCs
> driver.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  .../devicetree/bindings/hwmon/ads1000.txt     | 61 ++++++++++++++++

Bindings should be separate patch.

>  Documentation/hwmon/ads1000.rst               | 72 +++++++++++++++++++
>  2 files changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1000.txt
>  create mode 100644 Documentation/hwmon/ads1000.rst
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1000.txt b/Documentation/devicetree/bindings/hwmon/ads1000.txt
> new file mode 100644
> index 000000000000..3907b7da9b33
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1000.txt
> @@ -0,0 +1,61 @@
> +ADS1000/ADS1100 (I2C)
> +
> +This device is a 12-16 bit A-D converter with 1 input.

ADC's should be in bindings/iio/adc/

> +
> +The inputs can be used either as a differential pair of Vin+ Vin- or as a single
> +ended sensor for Vin+ GND. The inputs mode is platform-dependent and isn't
> +configured by software in any case.
> +
> +Device A-D converter sensitivity can be configured using two parameters:
> + - pga is the programmable gain amplifier
> +    0: x1 (default) 
> +    1: x2
> +    2: x4
> +    3: x8
> + - data_rate in samples per second also affecting the output code accuracy
> +    0: 128SPS - +/- Vdd*0.488mV (default, ads1000 accepts this rate only)
> +    1: 32SPS  - +/- Vdd*0.122mV
> +    2: 16SPS  - +/- Vdd*0.061mV
> +    3: 8SPS   - +/- Vdd*0.030mV
> +   Since this parameter also affects the output accuracy, be aware the greater
> +   SPS the worse accuracy.
> +
> +As a result the output value is calculated by the next formulae:
> +dVin = Cod * Vdd / (PGA * max(|Cod|)), where
> +max(|Cod|) - maximum possible value of the output code, which depends on the SPS
> +setting from the table above.
> +
> +The ADS1000/ADS1100 dts-node:
> +
> +  Required properties:
> +   - compatible : must be "ti,ads1000" or "ti,ads1100"
> +   - reg : I2C bus address of the device
> +   - #address-cells : must be <1>
> +   - #size-cells : must be <0>
> +   - vdd-supply : regulator for reference supply voltage (usually fixed)
> +
> +  Optional properties:
> +   - ti,gain : the programmable gain amplifier setting
> +   - ti,datarate : the converter data rate

IIRC, we have standard properties for these.

> +   - ti,voltage-divider : <R1 R2> Ohms inbound voltage dividers,
> +     so dVin = (R1 + R2)/R2 * dVin
> +
> +Example:
> +
> +vdd_5v0: fixedregulator@0 {
> +	compatible = "regulator-fixed";
> +	regulator-name = "vdd-ref";
> +	regulator-min-microvolt = <5000000>;
> +	regulator-max-microvolt = <5000000>;
> +	regulator-always-on;
> +};
> +
> +tiadc: ads1000@48 {

adc@48

> +	compatible = "ti,ads1000";
> +	reg = <0x48>;
> +
> +	vdd-supply = <&vdd_5v0>;
> +	ti,gain = <0>;
> +	ti,voltage-divider = <31600 3600>;
> +};
> +

Re: [PATCH 1/2] dt-bindings: hwmon: Add DT bindings for TI ads1000/ads1100 ADCs

[Serge Semin]

Hello Rob

On Thu, Jun 13, 2019 at 02:33:13PM -0600, Rob Herring wrote:
> On Wed, May 15, 2019 at 01:58:08AM +0300, Serge Semin wrote:
> > Add dt-binding documentation for the Texas Instruments ads1000/ads1100 ADCs
> > driver.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  .../devicetree/bindings/hwmon/ads1000.txt     | 61 ++++++++++++++++
> 
> Bindings should be separate patch.
> 
> >  Documentation/hwmon/ads1000.rst               | 72 +++++++++++++++++++
> >  2 files changed, 133 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1000.txt
> >  create mode 100644 Documentation/hwmon/ads1000.rst
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/ads1000.txt b/Documentation/devicetree/bindings/hwmon/ads1000.txt
> > new file mode 100644
> > index 000000000000..3907b7da9b33
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/ads1000.txt
> > @@ -0,0 +1,61 @@
> > +ADS1000/ADS1100 (I2C)
> > +
> > +This device is a 12-16 bit A-D converter with 1 input.
> 
> ADC's should be in bindings/iio/adc/
> 
> > +
> > +The inputs can be used either as a differential pair of Vin+ Vin- or as a single
> > +ended sensor for Vin+ GND. The inputs mode is platform-dependent and isn't
> > +configured by software in any case.
> > +
> > +Device A-D converter sensitivity can be configured using two parameters:
> > + - pga is the programmable gain amplifier
> > +    0: x1 (default) 
> > +    1: x2
> > +    2: x4
> > +    3: x8
> > + - data_rate in samples per second also affecting the output code accuracy
> > +    0: 128SPS - +/- Vdd*0.488mV (default, ads1000 accepts this rate only)
> > +    1: 32SPS  - +/- Vdd*0.122mV
> > +    2: 16SPS  - +/- Vdd*0.061mV
> > +    3: 8SPS   - +/- Vdd*0.030mV
> > +   Since this parameter also affects the output accuracy, be aware the greater
> > +   SPS the worse accuracy.
> > +
> > +As a result the output value is calculated by the next formulae:
> > +dVin = Cod * Vdd / (PGA * max(|Cod|)), where
> > +max(|Cod|) - maximum possible value of the output code, which depends on the SPS
> > +setting from the table above.
> > +
> > +The ADS1000/ADS1100 dts-node:
> > +
> > +  Required properties:
> > +   - compatible : must be "ti,ads1000" or "ti,ads1100"
> > +   - reg : I2C bus address of the device
> > +   - #address-cells : must be <1>
> > +   - #size-cells : must be <0>
> > +   - vdd-supply : regulator for reference supply voltage (usually fixed)
> > +
> > +  Optional properties:
> > +   - ti,gain : the programmable gain amplifier setting
> > +   - ti,datarate : the converter data rate
> 
> IIRC, we have standard properties for these.
> 
> > +   - ti,voltage-divider : <R1 R2> Ohms inbound voltage dividers,
> > +     so dVin = (R1 + R2)/R2 * dVin
> > +
> > +Example:
> > +
> > +vdd_5v0: fixedregulator@0 {
> > +	compatible = "regulator-fixed";
> > +	regulator-name = "vdd-ref";
> > +	regulator-min-microvolt = <5000000>;
> > +	regulator-max-microvolt = <5000000>;
> > +	regulator-always-on;
> > +};
> > +
> > +tiadc: ads1000@48 {
> 
> adc@48
> 
> > +	compatible = "ti,ads1000";
> > +	reg = <0x48>;
> > +
> > +	vdd-supply = <&vdd_5v0>;
> > +	ti,gain = <0>;
> > +	ti,voltage-divider = <31600 3600>;
> > +};
> > +

Thanks for this patch review. We had a conversation with Guenter Roeck
and Jonathan Cameron regarding the Linux subsystem to place this patchset
to, and decided to port the driver into the iio subsystem. So if this code
is ported as a separate driver I'll certainly take your comments into
account, while if I manage to integrate it into the existing ads1015 driver,
then this patch will be dropped since ads1015 already have the dedicated
bindings. In the last case sorry for wasting your time in advance.

Regards,
-Sergey