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

[RFC PATCH v2 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 'mode':

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

The changes are transparent as 'mode' defaults to mode 0 which yields the
previous behaviour.


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: Trivial code style fixup
  hwmon: adc128d818: Support operation modes 1-3

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

-- 
2.11.0

[RFC PATCH v2 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@alexanderkoch.net>
---
 .../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..5e14aa36a696
--- /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 defaults
+to Mode 0.
+
+
+Required node properties:
+
+ - compatible:  must be set to "ti,adc128d818"
+ - reg:         I2C address of the device
+
+Optional node properties:
+
+ - mode:        Operation mode (see above).
+
+
+Example (operation mode 2):
+
+	adc128d818@1d {
+		compatible = "ti,adc128d818";
+		reg = <0x1d>;
+		mode = /bits/ 8 <2>;
+	};
+
-- 
2.11.0

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

[Rob Herring]

On Fri, Dec 23, 2016 at 11:12:02PM +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>
> ---
>  .../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..5e14aa36a696
> --- /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 defaults
> +to Mode 0.
> +
> +
> +Required node properties:
> +
> + - compatible:  must be set to "ti,adc128d818"
> + - reg:         I2C address of the device
> +
> +Optional node properties:
> +
> + - mode:        Operation mode (see above).

Needs a vendor prefix.

> +
> +
> +Example (operation mode 2):
> +
> +	adc128d818@1d {
> +		compatible = "ti,adc128d818";
> +		reg = <0x1d>;
> +		mode = /bits/ 8 <2>;

Need to specify above this is an 8-bit value. Or just drop the size 
here as there's little gain to use a byte here.

> +	};
> +
> -- 
> 2.11.0
> 

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

[Alexander Koch]

Implement operation mode selection using the optional '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 '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@alexanderkoch.net>
---
 drivers/hwmon/adc128d818.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index ad2b47e40345..8667f454ea11 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, "mode", &data->mode) == 0) {
+		/* Currently only mode 0 supported */
+		if (data->mode != 0) {
+			dev_err(dev, "unsupported operation mode %d",
+				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

[RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Alexander Koch]

Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
permissions. This fixes checkpatch.pl warnings.

Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
---
 drivers/hwmon/adc128d818.c | 99 ++++++++++++++++++----------------------------
 1 file changed, 39 insertions(+), 60 deletions(-)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 8667f454ea11..cbb3bc5e5229 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -242,69 +242,48 @@ static ssize_t adc128_show_alarm(struct device *dev,
 	return sprintf(buf, "%u\n", !!(alarms & mask));
 }
 
-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,
-			    adc128_show_in, adc128_set_in, 0, 1);
-static SENSOR_DEVICE_ATTR_2(in0_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 0, 2);
-
-static SENSOR_DEVICE_ATTR_2(in1_input, S_IRUGO,
-			    adc128_show_in, NULL, 1, 0);
-static SENSOR_DEVICE_ATTR_2(in1_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 1, 1);
-static SENSOR_DEVICE_ATTR_2(in1_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 1, 2);
-
-static SENSOR_DEVICE_ATTR_2(in2_input, S_IRUGO,
-			    adc128_show_in, NULL, 2, 0);
-static SENSOR_DEVICE_ATTR_2(in2_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 2, 1);
-static SENSOR_DEVICE_ATTR_2(in2_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 2, 2);
-
-static SENSOR_DEVICE_ATTR_2(in3_input, S_IRUGO,
-			    adc128_show_in, NULL, 3, 0);
-static SENSOR_DEVICE_ATTR_2(in3_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 3, 1);
-static SENSOR_DEVICE_ATTR_2(in3_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 3, 2);
-
-static SENSOR_DEVICE_ATTR_2(in4_input, S_IRUGO,
-			    adc128_show_in, NULL, 4, 0);
-static SENSOR_DEVICE_ATTR_2(in4_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 4, 1);
-static SENSOR_DEVICE_ATTR_2(in4_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 4, 2);
-
-static SENSOR_DEVICE_ATTR_2(in5_input, S_IRUGO,
-			    adc128_show_in, NULL, 5, 0);
-static SENSOR_DEVICE_ATTR_2(in5_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 5, 1);
-static SENSOR_DEVICE_ATTR_2(in5_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 5, 2);
-
-static SENSOR_DEVICE_ATTR_2(in6_input, S_IRUGO,
-			    adc128_show_in, NULL, 6, 0);
-static SENSOR_DEVICE_ATTR_2(in6_min, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 6, 1);
-static SENSOR_DEVICE_ATTR_2(in6_max, S_IWUSR | S_IRUGO,
-			    adc128_show_in, adc128_set_in, 6, 2);
-
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adc128_show_temp, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
+static SENSOR_DEVICE_ATTR_2(in0_input, 0444, adc128_show_in, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(in0_min, 0644, adc128_show_in, adc128_set_in, 0, 1);
+static SENSOR_DEVICE_ATTR_2(in0_max, 0644, adc128_show_in, adc128_set_in, 0, 2);
+
+static SENSOR_DEVICE_ATTR_2(in1_input, 0444, adc128_show_in, NULL, 1, 0);
+static SENSOR_DEVICE_ATTR_2(in1_min, 0644, adc128_show_in, adc128_set_in, 1, 1);
+static SENSOR_DEVICE_ATTR_2(in1_max, 0644, adc128_show_in, adc128_set_in, 1, 2);
+
+static SENSOR_DEVICE_ATTR_2(in2_input, 0444, adc128_show_in, NULL, 2, 0);
+static SENSOR_DEVICE_ATTR_2(in2_min, 0644, adc128_show_in, adc128_set_in, 2, 1);
+static SENSOR_DEVICE_ATTR_2(in2_max, 0644, adc128_show_in, adc128_set_in, 2, 2);
+
+static SENSOR_DEVICE_ATTR_2(in3_input, 0444, adc128_show_in, NULL, 3, 0);
+static SENSOR_DEVICE_ATTR_2(in3_min, 0644, adc128_show_in, adc128_set_in, 3, 1);
+static SENSOR_DEVICE_ATTR_2(in3_max, 0644, adc128_show_in, adc128_set_in, 3, 2);
+
+static SENSOR_DEVICE_ATTR_2(in4_input, 0444, adc128_show_in, NULL, 4, 0);
+static SENSOR_DEVICE_ATTR_2(in4_min, 0644, adc128_show_in, adc128_set_in, 4, 1);
+static SENSOR_DEVICE_ATTR_2(in4_max, 0644, adc128_show_in, adc128_set_in, 4, 2);
+
+static SENSOR_DEVICE_ATTR_2(in5_input, 0444, adc128_show_in, NULL, 5, 0);
+static SENSOR_DEVICE_ATTR_2(in5_min, 0644, adc128_show_in, adc128_set_in, 5, 1);
+static SENSOR_DEVICE_ATTR_2(in5_max, 0644, adc128_show_in, adc128_set_in, 5, 2);
+
+static SENSOR_DEVICE_ATTR_2(in6_input, 0444, adc128_show_in, NULL, 6, 0);
+static SENSOR_DEVICE_ATTR_2(in6_min, 0644, adc128_show_in, adc128_set_in, 6, 1);
+static SENSOR_DEVICE_ATTR_2(in6_max, 0644, adc128_show_in, adc128_set_in, 6, 2);
+
+static SENSOR_DEVICE_ATTR(temp1_input, 0444, adc128_show_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, 0644,
 			  adc128_show_temp, adc128_set_temp, 1);
-static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
+static SENSOR_DEVICE_ATTR(temp1_max_hyst, 0644,
 			  adc128_show_temp, adc128_set_temp, 2);
 
-static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, adc128_show_alarm, NULL, 0);
-static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, adc128_show_alarm, NULL, 1);
-static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, adc128_show_alarm, NULL, 2);
-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(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7);
+static SENSOR_DEVICE_ATTR(in0_alarm, 0444, adc128_show_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_alarm, 0444, adc128_show_alarm, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_alarm, 0444, adc128_show_alarm, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_alarm, 0444, adc128_show_alarm, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_alarm, 0444, adc128_show_alarm, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_alarm, 0444, adc128_show_alarm, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_alarm, 0444, adc128_show_alarm, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, adc128_show_alarm, NULL, 7);
 
 static struct attribute *adc128_attrs[] = {
 	&sensor_dev_attr_in0_min.dev_attr.attr,
-- 
2.11.0

Re: [RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Guenter Roeck]

On 12/23/2016 02:12 PM, Alexander Koch wrote:
> Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
> permissions. This fixes checkpatch.pl warnings.
>
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>

Please do not bother with those warnings and ignore checkpatch.
We are in the process of doing an automated conversion.

Thanks,
Guenter

> ---
>  drivers/hwmon/adc128d818.c | 99 ++++++++++++++++++----------------------------
>  1 file changed, 39 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
> index 8667f454ea11..cbb3bc5e5229 100644
> --- a/drivers/hwmon/adc128d818.c
> +++ b/drivers/hwmon/adc128d818.c
> @@ -242,69 +242,48 @@ static ssize_t adc128_show_alarm(struct device *dev,
>  	return sprintf(buf, "%u\n", !!(alarms & mask));
>  }
>
> -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,
> -			    adc128_show_in, adc128_set_in, 0, 1);
> -static SENSOR_DEVICE_ATTR_2(in0_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 0, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in1_input, S_IRUGO,
> -			    adc128_show_in, NULL, 1, 0);
> -static SENSOR_DEVICE_ATTR_2(in1_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 1, 1);
> -static SENSOR_DEVICE_ATTR_2(in1_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 1, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in2_input, S_IRUGO,
> -			    adc128_show_in, NULL, 2, 0);
> -static SENSOR_DEVICE_ATTR_2(in2_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 2, 1);
> -static SENSOR_DEVICE_ATTR_2(in2_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 2, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in3_input, S_IRUGO,
> -			    adc128_show_in, NULL, 3, 0);
> -static SENSOR_DEVICE_ATTR_2(in3_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 3, 1);
> -static SENSOR_DEVICE_ATTR_2(in3_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 3, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in4_input, S_IRUGO,
> -			    adc128_show_in, NULL, 4, 0);
> -static SENSOR_DEVICE_ATTR_2(in4_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 4, 1);
> -static SENSOR_DEVICE_ATTR_2(in4_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 4, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in5_input, S_IRUGO,
> -			    adc128_show_in, NULL, 5, 0);
> -static SENSOR_DEVICE_ATTR_2(in5_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 5, 1);
> -static SENSOR_DEVICE_ATTR_2(in5_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 5, 2);
> -
> -static SENSOR_DEVICE_ATTR_2(in6_input, S_IRUGO,
> -			    adc128_show_in, NULL, 6, 0);
> -static SENSOR_DEVICE_ATTR_2(in6_min, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 6, 1);
> -static SENSOR_DEVICE_ATTR_2(in6_max, S_IWUSR | S_IRUGO,
> -			    adc128_show_in, adc128_set_in, 6, 2);
> -
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adc128_show_temp, NULL, 0);
> -static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
> +static SENSOR_DEVICE_ATTR_2(in0_input, 0444, adc128_show_in, NULL, 0, 0);
> +static SENSOR_DEVICE_ATTR_2(in0_min, 0644, adc128_show_in, adc128_set_in, 0, 1);
> +static SENSOR_DEVICE_ATTR_2(in0_max, 0644, adc128_show_in, adc128_set_in, 0, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in1_input, 0444, adc128_show_in, NULL, 1, 0);
> +static SENSOR_DEVICE_ATTR_2(in1_min, 0644, adc128_show_in, adc128_set_in, 1, 1);
> +static SENSOR_DEVICE_ATTR_2(in1_max, 0644, adc128_show_in, adc128_set_in, 1, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in2_input, 0444, adc128_show_in, NULL, 2, 0);
> +static SENSOR_DEVICE_ATTR_2(in2_min, 0644, adc128_show_in, adc128_set_in, 2, 1);
> +static SENSOR_DEVICE_ATTR_2(in2_max, 0644, adc128_show_in, adc128_set_in, 2, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in3_input, 0444, adc128_show_in, NULL, 3, 0);
> +static SENSOR_DEVICE_ATTR_2(in3_min, 0644, adc128_show_in, adc128_set_in, 3, 1);
> +static SENSOR_DEVICE_ATTR_2(in3_max, 0644, adc128_show_in, adc128_set_in, 3, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in4_input, 0444, adc128_show_in, NULL, 4, 0);
> +static SENSOR_DEVICE_ATTR_2(in4_min, 0644, adc128_show_in, adc128_set_in, 4, 1);
> +static SENSOR_DEVICE_ATTR_2(in4_max, 0644, adc128_show_in, adc128_set_in, 4, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in5_input, 0444, adc128_show_in, NULL, 5, 0);
> +static SENSOR_DEVICE_ATTR_2(in5_min, 0644, adc128_show_in, adc128_set_in, 5, 1);
> +static SENSOR_DEVICE_ATTR_2(in5_max, 0644, adc128_show_in, adc128_set_in, 5, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(in6_input, 0444, adc128_show_in, NULL, 6, 0);
> +static SENSOR_DEVICE_ATTR_2(in6_min, 0644, adc128_show_in, adc128_set_in, 6, 1);
> +static SENSOR_DEVICE_ATTR_2(in6_max, 0644, adc128_show_in, adc128_set_in, 6, 2);
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, adc128_show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, 0644,
>  			  adc128_show_temp, adc128_set_temp, 1);
> -static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, 0644,
>  			  adc128_show_temp, adc128_set_temp, 2);
>
> -static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, adc128_show_alarm, NULL, 0);
> -static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, adc128_show_alarm, NULL, 1);
> -static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, adc128_show_alarm, NULL, 2);
> -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(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7);
> +static SENSOR_DEVICE_ATTR(in0_alarm, 0444, adc128_show_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_alarm, 0444, adc128_show_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_alarm, 0444, adc128_show_alarm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_alarm, 0444, adc128_show_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_alarm, 0444, adc128_show_alarm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_alarm, 0444, adc128_show_alarm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_alarm, 0444, adc128_show_alarm, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, adc128_show_alarm, NULL, 7);
>
>  static struct attribute *adc128_attrs[] = {
>  	&sensor_dev_attr_in0_min.dev_attr.attr,
>

Re: [RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Alexander Koch]

On 12/26/2016 11:47 AM, Guenter Roeck wrote:
> On 12/23/2016 02:12 PM, Alexander Koch wrote:
>> Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
>> permissions. This fixes checkpatch.pl warnings.
>>
>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>
> Please do not bother with those warnings and ignore checkpatch.
> We are in the process of doing an automated conversion.

Okay, so I shall create v3 of the patchset, without these changes. I've
found a typo in the 4th patch so I thought about going v3 anyways.

I've tested operation modes 1-3 on real hardware today and found no
issues so far, so I hope v3 will have good chances of getting accepted.


Cheers,

Alex

Re: [RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Guenter Roeck]

On Thu, Dec 29, 2016 at 07:22:12PM +0100, Alexander Koch wrote:
> On 12/26/2016 11:47 AM, Guenter Roeck wrote:
> > On 12/23/2016 02:12 PM, Alexander Koch wrote:
> >> Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
> >> permissions. This fixes checkpatch.pl warnings.
> >>
> >> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> >
> > Please do not bother with those warnings and ignore checkpatch.
> > We are in the process of doing an automated conversion.
> 
> Okay, so I shall create v3 of the patchset, without these changes. I've
> found a typo in the 4th patch so I thought about going v3 anyways.
> 
> I've tested operation modes 1-3 on real hardware today and found no
> issues so far, so I hope v3 will have good chances of getting accepted.
> 
Pretty much. One request, though: If there is no configuration data
from DT, I would like the driver to read the mode from the chip - if for
nothing else, this will let me test all modes, but it also supports the
case where the chip is configured by BIOS/ROMMON.

Thanks,
Guenter

Re: [RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Alexander Koch]

On 12/29/2016 08:46 PM, Guenter Roeck wrote:
> On Thu, Dec 29, 2016 at 07:22:12PM +0100, Alexander Koch wrote:
>> On 12/26/2016 11:47 AM, Guenter Roeck wrote:
>>> On 12/23/2016 02:12 PM, Alexander Koch wrote:
>>>> Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
>>>> permissions. This fixes checkpatch.pl warnings.
>>>>
>>>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>>> Please do not bother with those warnings and ignore checkpatch.
>>> We are in the process of doing an automated conversion.
>> Okay, so I shall create v3 of the patchset, without these changes. I've
>> found a typo in the 4th patch so I thought about going v3 anyways.
>>
>> I've tested operation modes 1-3 on real hardware today and found no
>> issues so far, so I hope v3 will have good chances of getting accepted.
>>
> Pretty much. One request, though: If there is no configuration data
> from DT, I would like the driver to read the mode from the chip - if for
> nothing else, this will let me test all modes, but it also supports the
> case where the chip is configured by BIOS/ROMMON.

Aye, will add this as new fourth patch then. I assume the chip reset in
adc128_init_client() clears the chip mode as well, so I will read it in
the operation mode block in adc128_probe().

Just out of interest: how does this help you test the modes? Do you
configure the chip externally and test it on a platform without
devicetree support?


Cheers,

Alex

Re: [RFC PATCH v2 3/4] hwmon: adc128d818: Trivial code style fixup

[Guenter Roeck]

On 12/29/2016 12:30 PM, Alexander Koch wrote:
> On 12/29/2016 08:46 PM, Guenter Roeck wrote:
>> On Thu, Dec 29, 2016 at 07:22:12PM +0100, Alexander Koch wrote:
>>> On 12/26/2016 11:47 AM, Guenter Roeck wrote:
>>>> On 12/23/2016 02:12 PM, Alexander Koch wrote:
>>>>> Replace sysfs symbolic file permissions, e.g. 'S_IRUGO', by octal
>>>>> permissions. This fixes checkpatch.pl warnings.
>>>>>
>>>>> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
>>>> Please do not bother with those warnings and ignore checkpatch.
>>>> We are in the process of doing an automated conversion.
>>> Okay, so I shall create v3 of the patchset, without these changes. I've
>>> found a typo in the 4th patch so I thought about going v3 anyways.
>>>
>>> I've tested operation modes 1-3 on real hardware today and found no
>>> issues so far, so I hope v3 will have good chances of getting accepted.
>>>
>> Pretty much. One request, though: If there is no configuration data
>> from DT, I would like the driver to read the mode from the chip - if for
>> nothing else, this will let me test all modes, but it also supports the
>> case where the chip is configured by BIOS/ROMMON.
>
> Aye, will add this as new fourth patch then. I assume the chip reset in
> adc128_init_client() clears the chip mode as well, so I will read it in
> the operation mode block in adc128_probe().
>
> Just out of interest: how does this help you test the modes? Do you
> configure the chip externally and test it on a platform without
> devicetree support?
>

I use the i2c test driver in the kernel and a module test script, on x86.
See https://github.com/groeck/module-tests. That only works if the driver
does not overwrite the register values on probe.

Not really sure if resetting the chip during probe is such a good idea
in the first place. It is quite unusual.

Guenter

[RFC PATCH v2 4/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>
---
 drivers/hwmon/adc128d818.c | 120 ++++++++++++++++++++++++++++++---------------
 1 file changed, 80 insertions(+), 40 deletions(-)

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index cbb3bc5e5229..46c9fc9f1be2 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -59,6 +59,9 @@ static const unsigned short normal_i2c[] = {
 #define ADC128_REG_MAN_ID		0x3e
 #define ADC128_REG_DEV_ID		0x3f
 
+/* 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 +71,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 +92,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 +112,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 +250,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 < 8 * 4) {
+		/* 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, 0444, adc128_show_in, NULL, 0, 0);
 static SENSOR_DEVICE_ATTR_2(in0_min, 0644, adc128_show_in, adc128_set_in, 0, 1);
 static SENSOR_DEVICE_ATTR_2(in0_max, 0644, adc128_show_in, adc128_set_in, 0, 2);
@@ -270,6 +297,10 @@ static SENSOR_DEVICE_ATTR_2(in6_input, 0444, adc128_show_in, NULL, 6, 0);
 static SENSOR_DEVICE_ATTR_2(in6_min, 0644, adc128_show_in, adc128_set_in, 6, 1);
 static SENSOR_DEVICE_ATTR_2(in6_max, 0644, adc128_show_in, adc128_set_in, 6, 2);
 
+static SENSOR_DEVICE_ATTR_2(in7_input, 0444, adc128_show_in, NULL, 7, 0);
+static SENSOR_DEVICE_ATTR_2(in7_min, 0644, adc128_show_in, adc128_set_in, 7, 1);
+static SENSOR_DEVICE_ATTR_2(in7_max, 0644, adc128_show_in, adc128_set_in, 7, 2);
+
 static SENSOR_DEVICE_ATTR(temp1_input, 0444, adc128_show_temp, NULL, 0);
 static SENSOR_DEVICE_ATTR(temp1_max, 0644,
 			  adc128_show_temp, adc128_set_temp, 1);
@@ -283,44 +314,54 @@ static SENSOR_DEVICE_ATTR(in3_alarm, 0444, adc128_show_alarm, NULL, 3);
 static SENSOR_DEVICE_ATTR(in4_alarm, 0444, adc128_show_alarm, NULL, 4);
 static SENSOR_DEVICE_ATTR(in5_alarm, 0444, adc128_show_alarm, NULL, 5);
 static SENSOR_DEVICE_ATTR(in6_alarm, 0444, adc128_show_alarm, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_alarm, 0444, adc128_show_alarm, NULL, 7);
 static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, 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)
 {
@@ -425,9 +466,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, "mode", &data->mode) == 0) {
-		/* Currently only mode 0 supported */
-		if (data->mode != 0) {
-			dev_err(dev, "unsupported operation mode %d",
+		if (data->mode > 3) {
+			dev_err(dev, "invalid operation mode %d",
 				data->mode);
 			err = -EINVAL;
 			goto error;
-- 
2.11.0