[PATCH v3 0/4] hwmon: adc128d818: Support missing operation modes

[PATCH v3 0/4] hwmon: adc128d818: Support missing operation modes

[Alexander Koch]

The ADC128D818 offers four different chip operation modes which vary in the
number and measurement types of the available input signals (see datasheet
sec. 8.4.1).

The current version of the driver only supports the default chip operation
mode (mode 0), providing seven analog values and a temperature reading.

This patch series adds support for operation modes 1-3, selectable through
the device tree attribute 'ti,mode':

        adc1: adc128d818@1d {
                compatible = "ti,adc128d818";
                reg = <0x1d>;
                mode = <1>;
        };

The changes are transparent as the driver defaults to keeping the currently
active operation mode if no mode is specified via device tree (which is
mode 0 on chip initialization).


Changes from v2:
 - Omit device attribute refactoring (for checkpatch.pl), as requested by
   maintainer
 - Add vendor prefix 'ti,' for mode property in device tree
 - Drop size indication for mode property in device tree
 - Preserve chip operation mode if none specified in devicetree
 - Fix missing '\n' in dev_err() calls

Changes from v1:
 - Add bindings document as first patch
 - Preserve logical atomicity of code changes
 - Improve sysfs device node handling (use is_visible() instead of
   duplicate attribute list)
 - Add trivial code refactoring stage for checkpatch.pl to succeed


Alexander Koch (4):
  devicetree: hwmon: Add bindings for ADC128D818
  hwmon: adc128d818: Implement mode selection via dt
  hwmon: adc128d818: Support operation modes 1-3
  hwmon: adc128d818: Preserve operation mode

 .../devicetree/bindings/hwmon/adc128d818.txt       |  39 ++++++
 drivers/hwmon/adc128d818.c                         | 147 +++++++++++++++------
 2 files changed, 149 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adc128d818.txt

-- 
2.11.0

[PATCH v3 1/4] devicetree: hwmon: Add bindings for ADC128D818

[Alexander Koch]

Add bindings documentation for the ADC128D818 driver, featuring default I2C
properties along with the optional 'mode' property for chip operation mode
selection (see datasheet, sec. 8.4.1).

Signed-off-by: Alexander Koch <mail-y2PnNNZjvYd4VEKF+Mn3m16hYfS7NtTn@public.gmane.org>
Acked-by: Michael Hornung <mhornung.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/hwmon/adc128d818.txt       | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adc128d818.txt

diff --git a/Documentation/devicetree/bindings/hwmon/adc128d818.txt b/Documentation/devicetree/bindings/hwmon/adc128d818.txt
new file mode 100644
index 000000000000..db213dcc1391
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adc128d818.txt
@@ -0,0 +1,39 @@
+TI ADC128D818 ADC System Monitor With Temperature Sensor
+--------------------------------------------------------
+
+Operation modes:
+
+ - Mode 0:  7 single-ended voltage readings (IN0-IN6),
+            1 temperature reading (internal)
+ - Mode 1:  8 single-ended voltage readings (IN0-IN7),
+            no temperature
+ - Mode 2:  4 pseudo-differential voltage readings
+              (IN0-IN1, IN3-IN2, IN4-IN5, IN7-IN6),
+            1 temperature reading (internal)
+ - Mode 3:  4 single-ended voltage readings (IN0-IN3),
+            2 pseudo-differential voltage readings
+              (IN4-IN5, IN7-IN6),
+            1 temperature reading (internal)
+
+If no operation mode is configured via device tree, the driver keeps the
+currently active chip operation mode (default is mode 0).
+
+
+Required node properties:
+
+ - compatible:  must be set to "ti,adc128d818"
+ - reg:         I2C address of the device
+
+Optional node properties:
+
+ - ti,mode:     Operation mode (see above).
+
+
+Example (operation mode 2):
+
+	adc128d818@1d {
+		compatible = "ti,adc128d818";
+		reg = <0x1d>;
+		ti,mode = <2>;
+	};
+
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] devicetree: hwmon: Add bindings for ADC128D818

[Rob Herring]

On Fri, Jan 06, 2017 at 11:38:14AM +0100, Alexander Koch wrote:
> Add bindings documentation for the ADC128D818 driver, featuring default I2C
> properties along with the optional 'mode' property for chip operation mode
> selection (see datasheet, sec. 8.4.1).
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Acked-by: Michael Hornung <mhornung.linux@gmail.com>
> ---
>  .../devicetree/bindings/hwmon/adc128d818.txt       | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adc128d818.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v3 2/4] hwmon: adc128d818: Implement mode selection via dt

[Alexander Koch]

Implement operation mode selection using the optional 'ti,mode' devicetree
property (see [1]). The ADC128D818 supports four operation modes differing
in the number and type of input readings (see datasheet, sec. 8.4.1), of
which mode 0 is the default.

We only add handling of the 'ti,mode' property here, the driver still
supports nothing else than the default mode 0.

[1] Documentation/devicetree/bindings/hwmon/adc128d818.txt

Signed-off-by: Alexander Koch <mail-y2PnNNZjvYd4VEKF+Mn3m16hYfS7NtTn@public.gmane.org>
Acked-by: Michael Hornung <mhornung.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/hwmon/adc128d818.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index ad2b47e40345..2b61936c32ff 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -28,6 +28,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/mutex.h>
 #include <linux/bitops.h>
+#include <linux/of.h>
 
 /* Addresses to scan
  * The chip also supports addresses 0x35..0x37. Don't scan those addresses
@@ -63,6 +64,7 @@ struct adc128_data {
 	struct regulator *regulator;
 	int vref;		/* Reference voltage in mV */
 	struct mutex update_lock;
+	u8 mode;		/* Operation mode */
 	bool valid;		/* true if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
@@ -387,6 +389,15 @@ static int adc128_init_client(struct adc128_data *data)
 	if (err)
 		return err;
 
+	/* Set operation mode, if non-default */
+	if (data->mode != 0) {
+		err = i2c_smbus_write_byte_data(client,
+						ADC128_REG_CONFIG_ADV,
+						data->mode << 1);
+		if (err)
+			return err;
+	}
+
 	/* Start monitoring */
 	err = i2c_smbus_write_byte_data(client, ADC128_REG_CONFIG, 0x01);
 	if (err)
@@ -433,6 +444,19 @@ static int adc128_probe(struct i2c_client *client,
 		data->vref = 2560;	/* 2.56V, in mV */
 	}
 
+	/* Operation mode is optional and defaults to mode 0 */
+	if (of_property_read_u8(dev->of_node, "ti,mode", &data->mode) == 0) {
+		/* Currently only mode 0 supported */
+		if (data->mode != 0) {
+			dev_err(dev, "unsupported operation mode %d\n",
+				data->mode);
+			err = -EINVAL;
+			goto error;
+		}
+	} else {
+		data->mode = 0;
+	}
+
 	data->client = client;
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/4] hwmon: adc128d818: Support missing operation modes

[Guenter Roeck]

On Fri, Jan 06, 2017 at 11:38:13AM +0100, Alexander Koch wrote:
> The ADC128D818 offers four different chip operation modes which vary in the
> number and measurement types of the available input signals (see datasheet
> sec. 8.4.1).
> 
> The current version of the driver only supports the default chip operation
> mode (mode 0), providing seven analog values and a temperature reading.
> 
> This patch series adds support for operation modes 1-3, selectable through
> the device tree attribute 'ti,mode':
> 
>         adc1: adc128d818@1d {
>                 compatible = "ti,adc128d818";
>                 reg = <0x1d>;
>                 mode = <1>;
>         };
> 
> The changes are transparent as the driver defaults to keeping the currently
> active operation mode if no mode is specified via device tree (which is
> mode 0 on chip initialization).
> 
> 
> Changes from v2:
>  - Omit device attribute refactoring (for checkpatch.pl), as requested by
>    maintainer
>  - Add vendor prefix 'ti,' for mode property in device tree
>  - Drop size indication for mode property in device tree
>  - Preserve chip operation mode if none specified in devicetree
>  - Fix missing '\n' in dev_err() calls
> 
> Changes from v1:
>  - Add bindings document as first patch
>  - Preserve logical atomicity of code changes
>  - Improve sysfs device node handling (use is_visible() instead of
>    duplicate attribute list)
>  - Add trivial code refactoring stage for checkpatch.pl to succeed
> 
> 
> Alexander Koch (4):
>   devicetree: hwmon: Add bindings for ADC128D818
>   hwmon: adc128d818: Implement mode selection via dt
>   hwmon: adc128d818: Support operation modes 1-3
>   hwmon: adc128d818: Preserve operation mode
> 
>  .../devicetree/bindings/hwmon/adc128d818.txt       |  39 ++++++
>  drivers/hwmon/adc128d818.c                         | 147 +++++++++++++++------
>  2 files changed, 149 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adc128d818.txt
> 
Series applied to -next.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/4] hwmon: adc128d818: Support operation modes 1-3

[Alexander Koch]

Add support for operation modes 1-3 of the ADC128D818 (see datasheet sec.
8.4.1). These differ in the number and type of the available input signals,
requiring the driver to selectively hide sysfs nodes according to the
operation mode configured via devicetree.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
Acked-by: Michael Hornung <mhornung.linux@gmail.com>
---
 drivers/hwmon/adc128d818.c | 126 +++++++++++++++++++++++++++++++--------------
 1 file changed, 86 insertions(+), 40 deletions(-)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 2b61936c32ff..0502af963f73 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -59,6 +59,12 @@ static const unsigned short normal_i2c[] = {
 #define ADC128_REG_MAN_ID		0x3e
 #define ADC128_REG_DEV_ID		0x3f
 
+/* No. of voltage entries in adc128_attrs */
+#define ADC128_ATTR_NUM_VOLT		(8 * 4)
+
+/* Voltage inputs visible per operation mode */
+static const u8 num_inputs[] = { 7, 8, 4, 6 };
+
 struct adc128_data {
 	struct i2c_client *client;
 	struct regulator *regulator;
@@ -68,7 +74,7 @@ struct adc128_data {
 	bool valid;		/* true if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 
-	u16 in[3][7];		/* Register value, normalized to 12 bit
+	u16 in[3][8];		/* Register value, normalized to 12 bit
 				 * 0: input voltage
 				 * 1: min limit
 				 * 2: max limit
@@ -89,7 +95,7 @@ static struct adc128_data *adc128_update_device(struct device *dev)
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		for (i = 0; i < 7; i++) {
+		for (i = 0; i < num_inputs[data->mode]; i++) {
 			rv = i2c_smbus_read_word_swapped(client,
 							 ADC128_REG_IN(i));
 			if (rv < 0)
@@ -109,20 +115,25 @@ static struct adc128_data *adc128_update_device(struct device *dev)
 			data->in[2][i] = rv << 4;
 		}
 
-		rv = i2c_smbus_read_word_swapped(client, ADC128_REG_TEMP);
-		if (rv < 0)
-			goto abort;
-		data->temp[0] = rv >> 7;
+		if (data->mode != 1) {
+			rv = i2c_smbus_read_word_swapped(client,
+							 ADC128_REG_TEMP);
+			if (rv < 0)
+				goto abort;
+			data->temp[0] = rv >> 7;
 
-		rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_MAX);
-		if (rv < 0)
-			goto abort;
-		data->temp[1] = rv << 1;
+			rv = i2c_smbus_read_byte_data(client,
+						      ADC128_REG_TEMP_MAX);
+			if (rv < 0)
+				goto abort;
+			data->temp[1] = rv << 1;
 
-		rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_HYST);
-		if (rv < 0)
-			goto abort;
-		data->temp[2] = rv << 1;
+			rv = i2c_smbus_read_byte_data(client,
+						      ADC128_REG_TEMP_HYST);
+			if (rv < 0)
+				goto abort;
+			data->temp[2] = rv << 1;
+		}
 
 		rv = i2c_smbus_read_byte_data(client, ADC128_REG_ALARM);
 		if (rv < 0)
@@ -242,6 +253,25 @@ static ssize_t adc128_show_alarm(struct device *dev,
 	return sprintf(buf, "%u\n", !!(alarms & mask));
 }
 
+static umode_t adc128_is_visible(struct kobject *kobj,
+				 struct attribute *attr, int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct adc128_data *data = dev_get_drvdata(dev);
+
+	if (index < ADC128_ATTR_NUM_VOLT) {
+		/* Voltage, visible according to num_inputs[] */
+		if (index >= num_inputs[data->mode] * 4)
+			return 0;
+	} else {
+		/* Temperature, visible if not in mode 1 */
+		if (data->mode == 1)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
 static SENSOR_DEVICE_ATTR_2(in0_input, S_IRUGO,
 			    adc128_show_in, NULL, 0, 0);
 static SENSOR_DEVICE_ATTR_2(in0_min, S_IWUSR | S_IRUGO,
@@ -291,6 +321,13 @@ static SENSOR_DEVICE_ATTR_2(in6_min, S_IWUSR | S_IRUGO,
 static SENSOR_DEVICE_ATTR_2(in6_max, S_IWUSR | S_IRUGO,
 			    adc128_show_in, adc128_set_in, 6, 2);
 
+static SENSOR_DEVICE_ATTR_2(in7_input, S_IRUGO,
+			    adc128_show_in, NULL, 7, 0);
+static SENSOR_DEVICE_ATTR_2(in7_min, S_IWUSR | S_IRUGO,
+			    adc128_show_in, adc128_set_in, 7, 1);
+static SENSOR_DEVICE_ATTR_2(in7_max, S_IWUSR | S_IRUGO,
+			    adc128_show_in, adc128_set_in, 7, 2);
+
 static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adc128_show_temp, NULL, 0);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
 			  adc128_show_temp, adc128_set_temp, 1);
@@ -304,44 +341,54 @@ static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, adc128_show_alarm, NULL, 3);
 static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, adc128_show_alarm, NULL, 4);
 static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5);
 static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_alarm, S_IRUGO, adc128_show_alarm, NULL, 7);
 static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7);
 
 static struct attribute *adc128_attrs[] = {
-	&sensor_dev_attr_in0_min.dev_attr.attr,
-	&sensor_dev_attr_in1_min.dev_attr.attr,
-	&sensor_dev_attr_in2_min.dev_attr.attr,
-	&sensor_dev_attr_in3_min.dev_attr.attr,
-	&sensor_dev_attr_in4_min.dev_attr.attr,
-	&sensor_dev_attr_in5_min.dev_attr.attr,
-	&sensor_dev_attr_in6_min.dev_attr.attr,
-	&sensor_dev_attr_in0_max.dev_attr.attr,
-	&sensor_dev_attr_in1_max.dev_attr.attr,
-	&sensor_dev_attr_in2_max.dev_attr.attr,
-	&sensor_dev_attr_in3_max.dev_attr.attr,
-	&sensor_dev_attr_in4_max.dev_attr.attr,
-	&sensor_dev_attr_in5_max.dev_attr.attr,
-	&sensor_dev_attr_in6_max.dev_attr.attr,
+	&sensor_dev_attr_in0_alarm.dev_attr.attr,
 	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in0_max.dev_attr.attr,
+	&sensor_dev_attr_in0_min.dev_attr.attr,
+	&sensor_dev_attr_in1_alarm.dev_attr.attr,
 	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_max.dev_attr.attr,
+	&sensor_dev_attr_in1_min.dev_attr.attr,
+	&sensor_dev_attr_in2_alarm.dev_attr.attr,
 	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_max.dev_attr.attr,
+	&sensor_dev_attr_in2_min.dev_attr.attr,
+	&sensor_dev_attr_in3_alarm.dev_attr.attr,
 	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_max.dev_attr.attr,
+	&sensor_dev_attr_in3_min.dev_attr.attr,
+	&sensor_dev_attr_in4_alarm.dev_attr.attr,
 	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in4_max.dev_attr.attr,
+	&sensor_dev_attr_in4_min.dev_attr.attr,
+	&sensor_dev_attr_in5_alarm.dev_attr.attr,
 	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_in5_max.dev_attr.attr,
+	&sensor_dev_attr_in5_min.dev_attr.attr,
+	&sensor_dev_attr_in6_alarm.dev_attr.attr,
 	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_in6_max.dev_attr.attr,
+	&sensor_dev_attr_in6_min.dev_attr.attr,
+	&sensor_dev_attr_in7_alarm.dev_attr.attr,
+	&sensor_dev_attr_in7_input.dev_attr.attr,
+	&sensor_dev_attr_in7_max.dev_attr.attr,
+	&sensor_dev_attr_in7_min.dev_attr.attr,
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_in0_alarm.dev_attr.attr,
-	&sensor_dev_attr_in1_alarm.dev_attr.attr,
-	&sensor_dev_attr_in2_alarm.dev_attr.attr,
-	&sensor_dev_attr_in3_alarm.dev_attr.attr,
-	&sensor_dev_attr_in4_alarm.dev_attr.attr,
-	&sensor_dev_attr_in5_alarm.dev_attr.attr,
-	&sensor_dev_attr_in6_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(adc128);
+
+static struct attribute_group adc128_group = {
+	.attrs = adc128_attrs,
+	.is_visible = adc128_is_visible,
+};
+__ATTRIBUTE_GROUPS(adc128);
 
 static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
@@ -446,9 +493,8 @@ static int adc128_probe(struct i2c_client *client,
 
 	/* Operation mode is optional and defaults to mode 0 */
 	if (of_property_read_u8(dev->of_node, "ti,mode", &data->mode) == 0) {
-		/* Currently only mode 0 supported */
-		if (data->mode != 0) {
-			dev_err(dev, "unsupported operation mode %d\n",
+		if (data->mode > 3) {
+			dev_err(dev, "invalid operation mode %d\n",
 				data->mode);
 			err = -EINVAL;
 			goto error;
-- 
2.11.0

[PATCH v3 4/4] hwmon: adc128d818: Preserve operation mode

[Alexander Koch]

Preserve chip operation mode if no mode is specified via devicetree. This
enables operation when chip configuration is done by BIOS/ROMMON.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
Acked-by: Michael Hornung <mhornung.linux@gmail.com>
---
 drivers/hwmon/adc128d818.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 0502af963f73..bbe3a5c5b3f5 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -491,7 +491,7 @@ static int adc128_probe(struct i2c_client *client,
 		data->vref = 2560;	/* 2.56V, in mV */
 	}
 
-	/* Operation mode is optional and defaults to mode 0 */
+	/* Operation mode is optional. If unspecified, keep current mode */
 	if (of_property_read_u8(dev->of_node, "ti,mode", &data->mode) == 0) {
 		if (data->mode > 3) {
 			dev_err(dev, "invalid operation mode %d\n",
@@ -500,7 +500,10 @@ static int adc128_probe(struct i2c_client *client,
 			goto error;
 		}
 	} else {
-		data->mode = 0;
+		err = i2c_smbus_read_byte_data(client, ADC128_REG_CONFIG_ADV);
+		if (err < 0)
+			goto error;
+		data->mode = (err >> 1) & ADC128_REG_MASK;
 	}
 
 	data->client = client;
-- 
2.11.0