Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1366560 > unrolled thread

[PATCH 0/2] hwmon: adc128d818: Support operation mode 1

Started byAlexander Koch <mail@alexanderkoch.net>
First post2016-03-29 21:20 +0200
Last post2016-03-29 22:10 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Alexander Koch <mail@alexanderkoch.net> - 2016-03-29 21:20 +0200
    [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Alexander Koch <mail@alexanderkoch.net> - 2016-03-29 21:20 +0200
      Re: [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Guenter Roeck <linux@roeck-us.net> - 2016-03-29 22:10 +0200
    [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch <mail@alexanderkoch.net> - 2016-03-29 21:20 +0200
      Re: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Guenter Roeck <linux@roeck-us.net> - 2016-03-29 22:00 +0200
    Re: [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Guenter Roeck <linux@roeck-us.net> - 2016-03-29 22:10 +0200

#1366560 — [PATCH 0/2] hwmon: adc128d818: Support operation mode 1

FromAlexander Koch <mail@alexanderkoch.net>
Date2016-03-29 21:20 +0200
Subject[PATCH 0/2] hwmon: adc128d818: Support operation mode 1
Message-ID<ri7lg-2gu-11@gated-at.bofh.it>
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 mode 1 (eight analog inputs but no
temperature), 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. Code has been tested on real hardware using an evaluation
board.

Alexander Koch (2):
  hwmon: adc128d818: Implement chip mode selection
  hwmon: adc128d818: Implement operation mode 1

 drivers/hwmon/adc128d818.c | 113 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 95 insertions(+), 18 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1366561 — [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1

FromAlexander Koch <mail@alexanderkoch.net>
Date2016-03-29 21:20 +0200
Subject[PATCH 2/2] hwmon: adc128d818: Implement operation mode 1
Message-ID<ri7lg-2gu-9@gated-at.bofh.it>
In reply to#1366560
Implement chip operation mode 1 (see datasheet sec. 8.4.1) which offers an
additional analog input signal on channel 7 but no temperature reading.

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

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index 7505d4e..d29ba36 100644
--- a/drivers/hwmon/adc128d818.c
+++ b/drivers/hwmon/adc128d818.c
@@ -298,6 +298,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);
@@ -311,6 +318,7 @@ 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 *adc128m0_attrs[] = {
@@ -349,6 +357,42 @@ static struct attribute *adc128m0_attrs[] = {
 	NULL
 };
 ATTRIBUTE_GROUPS(adc128m0);
+static struct attribute *adc128m1_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_in7_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_in7_max.dev_attr.attr,
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_in7_input.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_in7_alarm.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(adc128m1);
 
 static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
@@ -456,7 +500,9 @@ static int adc128_probe(struct i2c_client *client,
 	data->mode = 0;
 	groups = adc128m0_groups;
 	if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) {
-		if (data->mode != 0) {
+		if (data->mode == 1) {
+			groups = adc128m1_groups;
+		} else if (data->mode != 0) {
 			dev_err(dev, "unsupported operation mode %d",
 				data->mode);
 			err = -EINVAL;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1366614 — Re: [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1

FromGuenter Roeck <linux@roeck-us.net>
Date2016-03-29 22:10 +0200
SubjectRe: [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1
Message-ID<ri87E-2Ps-15@gated-at.bofh.it>
In reply to#1366561
On Tue, Mar 29, 2016 at 09:03:39PM +0200, Alexander Koch wrote:
> Implement chip operation mode 1 (see datasheet sec. 8.4.1) which offers an
> additional analog input signal on channel 7 but no temperature reading.
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Michael Hornung <mhornung.linux@gmail.com>
> ---
>  drivers/hwmon/adc128d818.c | 48 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
> index 7505d4e..d29ba36 100644
> --- a/drivers/hwmon/adc128d818.c
> +++ b/drivers/hwmon/adc128d818.c
> @@ -298,6 +298,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);
> @@ -311,6 +318,7 @@ 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 *adc128m0_attrs[] = {
> @@ -349,6 +357,42 @@ static struct attribute *adc128m0_attrs[] = {
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(adc128m0);
> +static struct attribute *adc128m1_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_in7_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_in7_max.dev_attr.attr,
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_in6_input.dev_attr.attr,
> +	&sensor_dev_attr_in7_input.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_in7_alarm.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(adc128m1);
>  
No, this is not how to do it. You have two options:

- define multiple groups, and add groups as needed. 
  For example, you would have one group for in0..in3, one for in4..in5, one
  for in6, and one for in7, plus one for temp1. This would cover all
  operational modes.

  Then use a bitmap for supported voltages in a given mode, and use that bit map
  to determine if a voltage sensor is supported or not. For temperature you can
  use a boolean, or declare that the temperature sensor is available if in7
  is not available. This way you don't always have to check the operational mode
  when updating sensor values.

- Use a single group and is_visible() to determine is a sensor is visible or
  not (in combination with the bit map suggested above). This would probably
  be the simpler solution.

Guenter

>  static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
>  {
> @@ -456,7 +500,9 @@ static int adc128_probe(struct i2c_client *client,
>  	data->mode = 0;
>  	groups = adc128m0_groups;
>  	if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) {
> -		if (data->mode != 0) {
> +		if (data->mode == 1) {
> +			groups = adc128m1_groups;
> +		} else if (data->mode != 0) {
>  			dev_err(dev, "unsupported operation mode %d",
>  				data->mode);
>  			err = -EINVAL;
> -- 
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1366564 — [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection

FromAlexander Koch <mail@alexanderkoch.net>
Date2016-03-29 21:20 +0200
Subject[PATCH 1/2] hwmon: adc128d818: Implement chip mode selection
Message-ID<ri7lg-2gu-21@gated-at.bofh.it>
In reply to#1366560
The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1)
which vary in the number of available input signals and their types
(differential vs. absolute).

The current version of this driver only implements mode 0, this patch
prepares implementation of the others.

 * Introduce device tree property 'mode' for selection of chip operation
   mode, default to mode 0

 * Extend device data structure to cover all eight input channels

Example of 'mode' usage:

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

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

diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
index ad2b47e..7505d4e 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,10 +64,11 @@ 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 */
 
-	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
@@ -82,12 +84,14 @@ static struct adc128_data *adc128_update_device(struct device *dev)
 	struct adc128_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
 	struct adc128_data *ret = data;
-	int i, rv;
+	int i, rv, no_in;
+
+	no_in = (data->mode == 1 || data->mode == 2) ? 8 : 7;
 
 	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 < no_in; i++) {
 			rv = i2c_smbus_read_word_swapped(client,
 							 ADC128_REG_IN(i));
 			if (rv < 0)
@@ -107,20 +111,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)
@@ -304,7 +313,7 @@ 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 struct attribute *adc128_attrs[] = {
+static struct attribute *adc128m0_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,
@@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = {
 	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(adc128);
+ATTRIBUTE_GROUPS(adc128m0);
 
 static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
@@ -387,6 +396,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)
@@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client,
 	struct regulator *regulator;
 	struct device *hwmon_dev;
 	struct adc128_data *data;
+	const struct attribute_group **groups;
 	int err, vref;
 
 	data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
@@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client,
 		data->vref = 2560;	/* 2.56V, in mV */
 	}
 
+	/* Operation mode is optional and defaults to mode 0 */
+	data->mode = 0;
+	groups = adc128m0_groups;
+	if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) {
+		if (data->mode != 0) {
+			dev_err(dev, "unsupported operation mode %d",
+				data->mode);
+			err = -EINVAL;
+			goto error;
+		}
+	}
+
 	data->client = client;
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
@@ -443,7 +474,7 @@ static int adc128_probe(struct i2c_client *client,
 		goto error;
 
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data, adc128_groups);
+							   data, groups);
 	if (IS_ERR(hwmon_dev)) {
 		err = PTR_ERR(hwmon_dev);
 		goto error;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1366606 — Re: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection

FromGuenter Roeck <linux@roeck-us.net>
Date2016-03-29 22:00 +0200
SubjectRe: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection
Message-ID<ri7XZ-2vZ-31@gated-at.bofh.it>
In reply to#1366564
On Tue, Mar 29, 2016 at 09:03:38PM +0200, Alexander Koch wrote:
> The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1)
> which vary in the number of available input signals and their types
> (differential vs. absolute).
> 
> The current version of this driver only implements mode 0, this patch
> prepares implementation of the others.
> 
>  * Introduce device tree property 'mode' for selection of chip operation
>    mode, default to mode 0
> 
>  * Extend device data structure to cover all eight input channels
> 
> Example of 'mode' usage:
> 
>         adc1: adc128d818@1d {
>                 compatible = "ti,adc128d818";
>                 reg = <0x1d>;
>                 mode = /bits/ 8 <0>;

This will require devicetree maintainer review (and bindings document).
The property will probably require a ti, prefix. The bindings document also
needs to list the valid modes (all of them, not only the ones supported
by the driver).

>         };
> 
> Signed-off-by: Alexander Koch <mail@alexanderkoch.net>
> Acked-by: Michael Hornung <mhornung.linux@gmail.com>
> ---
>  drivers/hwmon/adc128d818.c | 67 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
> index ad2b47e..7505d4e 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,10 +64,11 @@ 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 */
>  
> -	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
> @@ -82,12 +84,14 @@ static struct adc128_data *adc128_update_device(struct device *dev)
>  	struct adc128_data *data = dev_get_drvdata(dev);
>  	struct i2c_client *client = data->client;
>  	struct adc128_data *ret = data;
> -	int i, rv;
> +	int i, rv, no_in;
> +
> +	no_in = (data->mode == 1 || data->mode == 2) ? 8 : 7;
>  
mode=2 has four voltages plus temperature sensor.

>  	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 < no_in; i++) {
>  			rv = i2c_smbus_read_word_swapped(client,
>  							 ADC128_REG_IN(i));
>  			if (rv < 0)
> @@ -107,20 +111,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)
> @@ -304,7 +313,7 @@ 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 struct attribute *adc128_attrs[] = {
> +static struct attribute *adc128m0_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,
> @@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = {
>  	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
>  	NULL
>  };
> -ATTRIBUTE_GROUPS(adc128);
> +ATTRIBUTE_GROUPS(adc128m0);
>  
>  static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info)
>  {
> @@ -387,6 +396,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)
> @@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client,
>  	struct regulator *regulator;
>  	struct device *hwmon_dev;
>  	struct adc128_data *data;
> +	const struct attribute_group **groups;
>  	int err, vref;
>  
>  	data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL);
> @@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client,
>  		data->vref = 2560;	/* 2.56V, in mV */
>  	}
>  
> +	/* Operation mode is optional and defaults to mode 0 */
> +	data->mode = 0;
> +	groups = adc128m0_groups;
> +	if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) {
> +		if (data->mode != 0) {
> +			dev_err(dev, "unsupported operation mode %d",
> +				data->mode);
> +			err = -EINVAL;
> +			goto error;
> +		}

I am sure you are goint to introduce mode support with the next patch, but you also
added some mode support in this patch. Patches should be logically consistent,
which is not the case here. Most of your changes are not even testable because
mode !=0 is rejected here, and some of the changes in this patch don't make
sense without the next patch.

It might make more sense to introduce the bindings document in the first patch,
and the code changes in the second patch.

> +	}
> +
>  	data->client = client;
>  	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
> @@ -443,7 +474,7 @@ static int adc128_probe(struct i2c_client *client,
>  		goto error;
>  
>  	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> -							   data, adc128_groups);
> +							   data, groups);
>  	if (IS_ERR(hwmon_dev)) {
>  		err = PTR_ERR(hwmon_dev);
>  		goto error;
> -- 
> 2.7.4
>

[toc] | [prev] | [next] | [standalone]

#1366622

FromGuenter Roeck <linux@roeck-us.net>
Date2016-03-29 22:10 +0200
Message-ID<ri87F-2Ps-49@gated-at.bofh.it>
In reply to#1366560
On Tue, Mar 29, 2016 at 09:03:37PM +0200, 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 mode 1 (eight analog inputs but no
> temperature), 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. Code has been tested on real hardware using an evaluation
> board.
> 
> Alexander Koch (2):
>   hwmon: adc128d818: Implement chip mode selection
>   hwmon: adc128d818: Implement operation mode 1
> 
>  drivers/hwmon/adc128d818.c | 113 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 95 insertions(+), 18 deletions(-)
> 

Please send follow-up patches to linux-hwmon@vger.kernel.org.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web