[PATCH] hwmon: (ads1015) Make gain and datarate configurable

[PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Dirk Eibach]

Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
 .../devicetree/bindings/hwmon/ads1015.txt          |   73 ++++++++++++++
 Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 ------
 Documentation/hwmon/ads1015                        |   48 +++++++---
 drivers/hwmon/ads1015.c                            |  104 ++++++++++++++++----
 include/linux/i2c/ads1015.h                        |    8 ++
 5 files changed, 202 insertions(+), 60 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
 delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
new file mode 100644
index 0000000..fb6e139
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
@@ -0,0 +1,73 @@
+ADS1015 (I2C)
+
+This device is a 12-bit A-D converter with 4 inputs.
+
+The inputs can be used single ended or in certain differential combinations.
+
+For configuration all possible combinations are mapped to 8 channels:
+  0: Voltage over AIN0 and AIN1.
+  1: Voltage over AIN0 and AIN3.
+  2: Voltage over AIN1 and AIN3.
+  3: Voltage over AIN2 and AIN3.
+  4: Voltage over AIN0 and GND.
+  5: Voltage over AIN1 and GND.
+  6: Voltage over AIN2 and GND.
+  7: Voltage over AIN3 and GND.
+
+Each channel can be configured indvidually:
+ - pga ist the programmable gain amplifier
+    0: FS = +/- 6.144 V
+    1: FS = +/- 4.096 V
+    2: FS = +/- 2.048 V (default)
+    3: FS = +/- 1.024 V
+    4: FS = +/- 0.512 V
+    5: FS = +/- 0.256 V
+ - data_rate in samples per second
+    0: 128
+    1: 250
+    2: 490
+    3: 920
+    4: 1600
+    5: 2400
+    6: 3300
+
+1) The /ads1015 node
+
+  Required properties:
+
+   - compatible : must be "ti,ads1015"
+   - reg : I2C busaddress of the device
+   - #address-cells : must be <1>
+   - #size-cells : must be <0>
+
+  The node contains child nodes for each channel that the platform uses.
+
+  Example ADS1015 node:
+
+    ads1015@49 {
+	    compatible = "ti,ads1015";
+	    reg = <0x49>;
+	    #address-cells = <1>;
+	    #size-cells = <0>;
+
+	    [ child node definitions... ]
+    }
+
+2) channel nodes
+
+  Required properties:
+
+   - reg : the channel number
+
+  Optional properties:
+
+   - ti,gain : the programmable gain amplifier setting
+   - ti,datarate : the converter datarate
+
+  Example ADS1015 node:
+
+    channel@4 {
+	    reg = <4>;
+	    ti,gain = <3>;
+	    ti,datarate = <5>;
+    };
diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
deleted file mode 100644
index 985e24d..0000000
--- a/Documentation/devicetree/bindings/i2c/ads1015.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-ADS1015 (I2C)
-
-This device is a 12-bit A-D converter with 4 inputs.
-
-The inputs can be used single ended or in certain differential combinations.
-
-For configuration all possible combinations are mapped to 8 channels:
-0: Voltage over AIN0 and AIN1.
-1: Voltage over AIN0 and AIN3.
-2: Voltage over AIN1 and AIN3.
-3: Voltage over AIN2 and AIN3.
-4: Voltage over AIN0 and GND.
-5: Voltage over AIN1 and GND.
-6: Voltage over AIN2 and GND.
-7: Voltage over AIN3 and GND.
-
-Optional properties:
-
- - exported-channels : exported_channels is a bitmask that specifies which
-		       channels should be accessible by the user.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = <0x14>;
-};
-
-In this example only channel 2 and 4 would be accessible by the user.
diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
index 56ee797..30250ca 100644
--- a/Documentation/hwmon/ads1015
+++ b/Documentation/hwmon/ads1015
@@ -38,30 +38,52 @@ Platform Data
 
 In linux/i2c/ads1015.h platform data is defined as:
 
+struct ads1015_channel_data {
+	unsigned int pga;
+	unsigned int data_rate;
+};
+
 struct ads1015_platform_data {
 	unsigned int exported_channels;
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 exported_channels is a bitmask that specifies which inputs should be exported.
 
+channel_data contains configuration data for the exported inputs:
+- pga ist the programmable gain amplifier
+  0: FS = +/- 6.144 V
+  1: FS = +/- 4.096 V
+  2: FS = +/- 2.048 V (default)
+  3: FS = +/- 1.024 V
+  4: FS = +/- 0.512 V
+  5: FS = +/- 0.256 V
+- data_rate in samples per second
+  0: 128
+  1: 250
+  2: 490
+  3: 920
+  4: 1600
+  5: 2400
+  6: 3300
+
 Example:
 struct ads1015_platform_data data = {
-	.exported_channels = (1 << 2) | (1 << 4)
+	.exported_channels = (1 << 2) | (1 << 4),
+	.channel_data = {
+		{},
+		{},
+		{ .pga = 1, .data_rate = 0 },
+		{},
+		{ .pga = 4, .data_rate = 5},
+	}
 };
 
-In this case only in2_input and in4_input would be created.
+In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input
+(FS +/- 0.512 V, 2400 SPS) would be created.
 
 Devicetree
 ----------
 
-The ads1015 node may have an "exported-channels" property.
-exported_channels is a bitmask that specifies which inputs should be exported.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = < 0x14 >;
-};
-
-In this case only in2_input and in4_input would be created.
+Configuration is also possible via devicetree:
+Documentation/devicetree/bindings/hwmon/ads1015.txt
diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
index 8ef2b60..8657863 100644
--- a/drivers/hwmon/ads1015.c
+++ b/drivers/hwmon/ads1015.c
@@ -25,7 +25,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
-#include <linux/jiffies.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -45,12 +45,18 @@ enum {
 static const unsigned int fullscale_table[8] = {
 	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
 
-#define ADS1015_CONFIG_CHANNELS 8
+/* Data rates in samples per second */
+static const unsigned int data_rate_table[8] = {
+	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
+
 #define ADS1015_DEFAULT_CHANNELS 0xff
+#define ADS1015_DEFAULT_PGA 2
+#define ADS1015_DEFAULT_DATA_RATE 4
 
 struct ads1015_data {
 	struct device *hwmon_dev;
 	struct mutex update_lock; /* mutex protect updates */
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
@@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
 {
 	u16 config;
 	s16 conversion;
-	unsigned int pga;
+	struct ads1015_data *data = i2c_get_clientdata(client);
+	unsigned int pga = data->channel_data[channel].pga;
 	int fullscale;
+	unsigned int data_rate = data->channel_data[channel].data_rate;
+	unsigned int conversion_time_ms;
 	unsigned int k;
-	struct ads1015_data *data = i2c_get_clientdata(client);
 	int res;
 
 	mutex_lock(&data->update_lock);
 
-	/* get fullscale voltage */
+	/* get channel parameters */
 	res = ads1015_read_reg(client, ADS1015_CONFIG);
 	if (res < 0)
 		goto err_unlock;
 	config = res;
-	pga = (config >> 9) & 0x0007;
 	fullscale = fullscale_table[pga];
+	conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];
 
 	/* set channel and start single conversion */
-	config &= ~(0x0007 << 12);
-	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
+	config &= 0x001f;
+	config |= (1 << 15) | (1 << 8);
+	config |= (channel & 0x0007) << 12;
+	config |= (pga & 0x0007) << 9;
+	config |= (data_rate & 0x0007) << 5;
 
-	/* wait until conversion finished */
 	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
 	if (res < 0)
 		goto err_unlock;
+
+	/* wait until conversion finished */
 	for (k = 0; k < 5; ++k) {
-		schedule_timeout(msecs_to_jiffies(1));
+		msleep(k ? 1 : conversion_time_ms);
 		res = ads1015_read_reg(client, ADS1015_CONFIG);
 		if (res < 0)
 			goto err_unlock;
@@ -168,26 +180,82 @@ static int ads1015_remove(struct i2c_client *client)
 
 static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
 {
+	unsigned int k;
+	struct ads1015_data *data = i2c_get_clientdata(client);
 	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
 #ifdef CONFIG_OF
-	struct device_node *np = client->dev.of_node;
-	const __be32 *of_channels;
-	int of_channels_size;
+	unsigned int of_exported_channels = 0;
+	struct device_node *node;
 #endif
 
 	/* prefer platform data */
-	if (pdata)
+	if (pdata) {
+		memcpy(data->channel_data, pdata->channel_data,
+		       sizeof(data->channel_data));
 		return pdata->exported_channels;
+	}
 
 #ifdef CONFIG_OF
 	/* fallback on OF */
-	of_channels = of_get_property(np, "exported-channels",
-				      &of_channels_size);
-	if (of_channels && (of_channels_size == sizeof(*of_channels)))
-		return be32_to_cpup(of_channels);
+	if (!client->dev.of_node
+	    || !of_get_next_child(client->dev.of_node, NULL))
+		goto fallback_default;
+
+	for_each_child_of_node(client->dev.of_node, node) {
+		const __be32 *property;
+		int len;
+		unsigned int channel;
+		unsigned int pga = ADS1015_DEFAULT_PGA;
+		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
+
+		property = of_get_property(node, "reg", &len);
+		if (!property || (len < sizeof(int))) {
+			dev_err(&client->dev, "ads1015: invalid reg on %s\n",
+				node->full_name);
+			continue;
+		}
+
+		channel = be32_to_cpup(property);
+		if (channel > ADS1015_CONFIG_CHANNELS) {
+			dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
+				channel, node->full_name);
+			continue;
+		}
+
+		property = of_get_property(node, "ti,gain", &len);
+		if (property && (len == sizeof(int))) {
+			pga = be32_to_cpup(property);
+			if (pga > 7) {
+				dev_err(&client->dev,
+					"ads1015: invalid gain on %s\n",
+					node->full_name);
+			}
+		}
+
+		property = of_get_property(node, "ti,datarate", &len);
+		if (property && (len == sizeof(int))) {
+			data_rate = be32_to_cpup(property);
+			if (data_rate > 7)
+				dev_err(&client->dev,
+					"ads1015: invalid data_rate on %s\n",
+					node->full_name);
+		}
+
+		of_exported_channels |= (1 << channel);
+		data->channel_data[channel].pga = pga;
+		data->channel_data[channel].data_rate = data_rate;
+	}
+
+	return of_exported_channels;
 #endif
 
 	/* fallback on default configuration */
+fallback_default:
+	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
+		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
+		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
+	}
+
 	return ADS1015_DEFAULT_CHANNELS;
 }
 
diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
index 8541c6a..8f32040 100644
--- a/include/linux/i2c/ads1015.h
+++ b/include/linux/i2c/ads1015.h
@@ -21,8 +21,16 @@
 #ifndef LINUX_ADS1015_H
 #define LINUX_ADS1015_H
 
+#define ADS1015_CONFIG_CHANNELS 8
+
+struct ads1015_channel_data {
+	unsigned int pga;
+	unsigned int data_rate;
+};
+
 struct ads1015_platform_data {
 	unsigned int exported_channels;
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 #endif /* LINUX_ADS1015_H */
-- 
1.5.6.5

Re: [lm-sensors] [PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Guenter Roeck]

Hi Dirk,

On Wed, Mar 09, 2011 at 08:52:40AM -0500, Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   73 ++++++++++++++
>  Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 ------
>  Documentation/hwmon/ads1015                        |   48 +++++++---
>  drivers/hwmon/ads1015.c                            |  104 ++++++++++++++++----
>  include/linux/i2c/ads1015.h                        |    8 ++
>  5 files changed, 202 insertions(+), 60 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> new file mode 100644
> index 0000000..fb6e139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured indvidually:
> + - pga ist the programmable gain amplifier
> +    0: FS = +/- 6.144 V
> +    1: FS = +/- 4.096 V
> +    2: FS = +/- 2.048 V (default)
> +    3: FS = +/- 1.024 V
> +    4: FS = +/- 0.512 V
> +    5: FS = +/- 0.256 V
> + - data_rate in samples per second
> +    0: 128
> +    1: 250
> +    2: 490
> +    3: 920
> +    4: 1600
> +    5: 2400
> +    6: 3300
> +

I thought the data rate only applies for continuous mode, which you don't use.
What does it do in single shot mode ?

Thanks,
Guenter

RE: [lm-sensors] [PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Eibach, Dirk]

> Hi Dirk,

Hi Guenter,

> ...
> > +Each channel can be configured indvidually:
> > + - pga ist the programmable gain amplifier
> > +    0: FS = +/- 6.144 V
> > +    1: FS = +/- 4.096 V
> > +    2: FS = +/- 2.048 V (default)
> > +    3: FS = +/- 1.024 V
> > +    4: FS = +/- 0.512 V
> > +    5: FS = +/- 0.256 V
> > + - data_rate in samples per second
> > +    0: 128
> > +    1: 250
> > +    2: 490
> > +    3: 920
> > +    4: 1600
> > +    5: 2400
> > +    6: 3300
> > +
> 
> I thought the data rate only applies for continuous mode, 
> which you don't use.
> What does it do in single shot mode ?

It affects the sigma delta integration time and as a result the
conversion time.

> Thanks,
> Guenter

Cheers
Dirk

Re: [lm-sensors] [PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Guenter Roeck]

Hi Dirk,

On Wed, 2011-03-09 at 08:52 -0500, Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   73 ++++++++++++++
>  Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 ------
>  Documentation/hwmon/ads1015                        |   48 +++++++---
>  drivers/hwmon/ads1015.c                            |  104 ++++++++++++++++----
>  include/linux/i2c/ads1015.h                        |    8 ++
>  5 files changed, 202 insertions(+), 60 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> new file mode 100644
> index 0000000..fb6e139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured indvidually:
> + - pga ist the programmable gain amplifier
> +    0: FS = +/- 6.144 V
> +    1: FS = +/- 4.096 V
> +    2: FS = +/- 2.048 V (default)
> +    3: FS = +/- 1.024 V
> +    4: FS = +/- 0.512 V
> +    5: FS = +/- 0.256 V
> + - data_rate in samples per second
> +    0: 128
> +    1: 250
> +    2: 490
> +    3: 920
> +    4: 1600
> +    5: 2400
> +    6: 3300
> +
Just a thought ... if you specify the data rate (and possibly pga) this
way, you'll have to redefine its meaning for ads1115. If you just define
it as "samples per second" and then calculate the index from it in the
code (eg as closest match), you'd be done.

> +1) The /ads1015 node
> +
> +  Required properties:
> +
> +   - compatible : must be "ti,ads1015"
> +   - reg : I2C busaddress of the device
> +   - #address-cells : must be <1>
> +   - #size-cells : must be <0>
> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example ADS1015 node:
> +
> +    ads1015@49 {
> +           compatible = "ti,ads1015";
> +           reg = <0x49>;
> +           #address-cells = <1>;
> +           #size-cells = <0>;
> +
> +           [ child node definitions... ]
> +    }
> +
> +2) channel nodes
> +
> +  Required properties:
> +
> +   - reg : the channel number
> +
> +  Optional properties:
> +
> +   - ti,gain : the programmable gain amplifier setting
> +   - ti,datarate : the converter datarate
> +
> +  Example ADS1015 node:
> +
> +    channel@4 {
> +           reg = <4>;
> +           ti,gain = <3>;
> +           ti,datarate = <5>;
> +    };
> diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> deleted file mode 100644
> index 985e24d..0000000
> --- a/Documentation/devicetree/bindings/i2c/ads1015.txt
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -ADS1015 (I2C)
> -
> -This device is a 12-bit A-D converter with 4 inputs.
> -
> -The inputs can be used single ended or in certain differential combinations.
> -
> -For configuration all possible combinations are mapped to 8 channels:
> -0: Voltage over AIN0 and AIN1.
> -1: Voltage over AIN0 and AIN3.
> -2: Voltage over AIN1 and AIN3.
> -3: Voltage over AIN2 and AIN3.
> -4: Voltage over AIN0 and GND.
> -5: Voltage over AIN1 and GND.
> -6: Voltage over AIN2 and GND.
> -7: Voltage over AIN3 and GND.
> -
> -Optional properties:
> -
> - - exported-channels : exported_channels is a bitmask that specifies which
> -                      channels should be accessible by the user.
> -
> -Example:
> -ads1015@49 {
> -       compatible = "ti,ads1015";
> -       reg = <0x49>;
> -       exported-channels = <0x14>;
> -};
> -
> -In this example only channel 2 and 4 would be accessible by the user.
> diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
> index 56ee797..30250ca 100644
> --- a/Documentation/hwmon/ads1015
> +++ b/Documentation/hwmon/ads1015
> @@ -38,30 +38,52 @@ Platform Data
> 
>  In linux/i2c/ads1015.h platform data is defined as:
> 
> +struct ads1015_channel_data {
> +       unsigned int pga;
> +       unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>         unsigned int exported_channels;
> +       struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
> 
>  exported_channels is a bitmask that specifies which inputs should be exported.
> 
> +channel_data contains configuration data for the exported inputs:
> +- pga ist the programmable gain amplifier
> +  0: FS = +/- 6.144 V
> +  1: FS = +/- 4.096 V
> +  2: FS = +/- 2.048 V (default)
> +  3: FS = +/- 1.024 V
> +  4: FS = +/- 0.512 V
> +  5: FS = +/- 0.256 V
> +- data_rate in samples per second
> +  0: 128
> +  1: 250
> +  2: 490
> +  3: 920
> +  4: 1600
> +  5: 2400
> +  6: 3300
> +
>  Example:
>  struct ads1015_platform_data data = {
> -       .exported_channels = (1 << 2) | (1 << 4)
> +       .exported_channels = (1 << 2) | (1 << 4),
> +       .channel_data = {
> +               {},
> +               {},
> +               { .pga = 1, .data_rate = 0 },
> +               {},
> +               { .pga = 4, .data_rate = 5},

Should have a blank between 5 and } for consistency.

> +       }
>  };
> 
> -In this case only in2_input and in4_input would be created.
> +In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input
> +(FS +/- 0.512 V, 2400 SPS) would be created.
> 
>  Devicetree
>  ----------
> 
> -The ads1015 node may have an "exported-channels" property.
> -exported_channels is a bitmask that specifies which inputs should be exported.
> -
> -Example:
> -ads1015@49 {
> -       compatible = "ti,ads1015";
> -       reg = <0x49>;
> -       exported-channels = < 0x14 >;
> -};
> -
> -In this case only in2_input and in4_input would be created.
> +Configuration is also possible via devicetree:
> +Documentation/devicetree/bindings/hwmon/ads1015.txt
> diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
> index 8ef2b60..8657863 100644
> --- a/drivers/hwmon/ads1015.c
> +++ b/drivers/hwmon/ads1015.c
> @@ -25,7 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> -#include <linux/jiffies.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -45,12 +45,18 @@ enum {
>  static const unsigned int fullscale_table[8] = {
>         6144, 4096, 2048, 1024, 512, 256, 256, 256 };
> 
> -#define ADS1015_CONFIG_CHANNELS 8
> +/* Data rates in samples per second */
> +static const unsigned int data_rate_table[8] = {
> +       128, 250, 490, 920, 1600, 2400, 3300, 3300 };
> +
>  #define ADS1015_DEFAULT_CHANNELS 0xff
> +#define ADS1015_DEFAULT_PGA 2
> +#define ADS1015_DEFAULT_DATA_RATE 4
> 
>  struct ads1015_data {
>         struct device *hwmon_dev;
>         struct mutex update_lock; /* mutex protect updates */
> +       struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
> 
>  static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
> @@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
>  {
>         u16 config;
>         s16 conversion;
> -       unsigned int pga;
> +       struct ads1015_data *data = i2c_get_clientdata(client);
> +       unsigned int pga = data->channel_data[channel].pga;
>         int fullscale;
> +       unsigned int data_rate = data->channel_data[channel].data_rate;
> +       unsigned int conversion_time_ms;
>         unsigned int k;
> -       struct ads1015_data *data = i2c_get_clientdata(client);
>         int res;
> 
>         mutex_lock(&data->update_lock);
> 
> -       /* get fullscale voltage */
> +       /* get channel parameters */
>         res = ads1015_read_reg(client, ADS1015_CONFIG);
>         if (res < 0)
>                 goto err_unlock;
>         config = res;
> -       pga = (config >> 9) & 0x0007;
>         fullscale = fullscale_table[pga];
> +       conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];
> 
>         /* set channel and start single conversion */
> -       config &= ~(0x0007 << 12);
> -       config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
> +       config &= 0x001f;
> +       config |= (1 << 15) | (1 << 8);
> +       config |= (channel & 0x0007) << 12;
> +       config |= (pga & 0x0007) << 9;
> +       config |= (data_rate & 0x0007) << 5;
> 
> -       /* wait until conversion finished */
>         res = ads1015_write_reg(client, ADS1015_CONFIG, config);
>         if (res < 0)
>                 goto err_unlock;
> +
> +       /* wait until conversion finished */
>         for (k = 0; k < 5; ++k) {
> -               schedule_timeout(msecs_to_jiffies(1));
> +               msleep(k ? 1 : conversion_time_ms);
>                 res = ads1015_read_reg(client, ADS1015_CONFIG);
>                 if (res < 0)
>                         goto err_unlock;
> @@ -168,26 +180,82 @@ static int ads1015_remove(struct i2c_client *client)
> 
>  static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
>  {
> +       unsigned int k;
> +       struct ads1015_data *data = i2c_get_clientdata(client);
>         struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
>  #ifdef CONFIG_OF
> -       struct device_node *np = client->dev.of_node;
> -       const __be32 *of_channels;
> -       int of_channels_size;
> +       unsigned int of_exported_channels = 0;
> +       struct device_node *node;
>  #endif
> 
>         /* prefer platform data */
> -       if (pdata)
> +       if (pdata) {
> +               memcpy(data->channel_data, pdata->channel_data,
> +                      sizeof(data->channel_data));
>                 return pdata->exported_channels;
> +       }
> 
>  #ifdef CONFIG_OF
>         /* fallback on OF */
> -       of_channels = of_get_property(np, "exported-channels",
> -                                     &of_channels_size);
> -       if (of_channels && (of_channels_size == sizeof(*of_channels)))
> -               return be32_to_cpup(of_channels);
> +       if (!client->dev.of_node
> +           || !of_get_next_child(client->dev.of_node, NULL))
> +               goto fallback_default;
> +
> +       for_each_child_of_node(client->dev.of_node, node) {
> +               const __be32 *property;
> +               int len;
> +               unsigned int channel;
> +               unsigned int pga = ADS1015_DEFAULT_PGA;
> +               unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
> +
> +               property = of_get_property(node, "reg", &len);
> +               if (!property || (len < sizeof(int))) {
> +                       dev_err(&client->dev, "ads1015: invalid reg on %s\n",
> +                               node->full_name);
> +                       continue;
> +               }
> +
> +               channel = be32_to_cpup(property);
> +               if (channel > ADS1015_CONFIG_CHANNELS) {
> +                       dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
> +                               channel, node->full_name);
> +                       continue;
> +               }

Can channel be < 0 ?

> +
> +               property = of_get_property(node, "ti,gain", &len);
> +               if (property && (len == sizeof(int))) {
> +                       pga = be32_to_cpup(property);
> +                       if (pga > 7) {
> +                               dev_err(&client->dev,
> +                                       "ads1015: invalid gain on %s\n",
> +                                       node->full_name);
> +                       }
> +               }

Can pga be < 0 ? Also, no action if pga is bad ?

> +
> +               property = of_get_property(node, "ti,datarate", &len);
> +               if (property && (len == sizeof(int))) {
> +                       data_rate = be32_to_cpup(property);
> +                       if (data_rate > 7)
> +                               dev_err(&client->dev,
> +                                       "ads1015: invalid data_rate on %s\n",
> +                                       node->full_name);
> +               }

No action if data_rate is bad ? And can it be < 0 ?

> +
> +               of_exported_channels |= (1 << channel);
> +               data->channel_data[channel].pga = pga;
> +               data->channel_data[channel].data_rate = data_rate;
> +       }
> +
> +       return of_exported_channels;
>  #endif
> 
>         /* fallback on default configuration */
> +fallback_default:
> +       for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> +               data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
> +               data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
> +       }
> +
>         return ADS1015_DEFAULT_CHANNELS;
>  }
> 
> diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
> index 8541c6a..8f32040 100644
> --- a/include/linux/i2c/ads1015.h
> +++ b/include/linux/i2c/ads1015.h
> @@ -21,8 +21,16 @@
>  #ifndef LINUX_ADS1015_H
>  #define LINUX_ADS1015_H
> 
> +#define ADS1015_CONFIG_CHANNELS 8
> +
> +struct ads1015_channel_data {
> +       unsigned int pga;
> +       unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>         unsigned int exported_channels;
> +       struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
> 
>  #endif /* LINUX_ADS1015_H */
> --
> 1.5.6.5
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Grant Likely]

On Wed, Mar 09, 2011 at 02:52:40PM +0100, Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>

Hi Dirk,

Patches need a description.  Very seldom is the patch title
sufficient, and usually only for trivial stuff.  Tell us some details
about what the patch does and what testing has been performed on it.

> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   73 ++++++++++++++
>  Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 ------

Huh?  What tree is this patch committed against?  If this is a
follow-on to a previous patch, then the patch description should
detail that information.

>  Documentation/hwmon/ads1015                        |   48 +++++++---
>  drivers/hwmon/ads1015.c                            |  104 ++++++++++++++++----
>  include/linux/i2c/ads1015.h                        |    8 ++
>  5 files changed, 202 insertions(+), 60 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> new file mode 100644
> index 0000000..fb6e139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured indvidually:
> + - pga ist the programmable gain amplifier
> +    0: FS = +/- 6.144 V
> +    1: FS = +/- 4.096 V
> +    2: FS = +/- 2.048 V (default)
> +    3: FS = +/- 1.024 V
> +    4: FS = +/- 0.512 V
> +    5: FS = +/- 0.256 V
> + - data_rate in samples per second
> +    0: 128
> +    1: 250
> +    2: 490
> +    3: 920
> +    4: 1600
> +    5: 2400
> +    6: 3300
> +
> +1) The /ads1015 node
> +
> +  Required properties:
> +
> +   - compatible : must be "ti,ads1015"
> +   - reg : I2C busaddress of the device
> +   - #address-cells : must be <1>
> +   - #size-cells : must be <0>
> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example ADS1015 node:
> +
> +    ads1015@49 {
> +	    compatible = "ti,ads1015";
> +	    reg = <0x49>;
> +	    #address-cells = <1>;
> +	    #size-cells = <0>;
> +
> +	    [ child node definitions... ]
> +    }
> +
> +2) channel nodes
> +
> +  Required properties:
> +
> +   - reg : the channel number
> +
> +  Optional properties:
> +
> +   - ti,gain : the programmable gain amplifier setting
> +   - ti,datarate : the converter datarate
> +
> +  Example ADS1015 node:
> +
> +    channel@4 {
> +	    reg = <4>;
> +	    ti,gain = <3>;
> +	    ti,datarate = <5>;
> +    };

Binding looks good to me.

> diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> deleted file mode 100644
> index 985e24d..0000000
> --- a/Documentation/devicetree/bindings/i2c/ads1015.txt
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -ADS1015 (I2C)
> -
> -This device is a 12-bit A-D converter with 4 inputs.
> -
> -The inputs can be used single ended or in certain differential combinations.
> -
> -For configuration all possible combinations are mapped to 8 channels:
> -0: Voltage over AIN0 and AIN1.
> -1: Voltage over AIN0 and AIN3.
> -2: Voltage over AIN1 and AIN3.
> -3: Voltage over AIN2 and AIN3.
> -4: Voltage over AIN0 and GND.
> -5: Voltage over AIN1 and GND.
> -6: Voltage over AIN2 and GND.
> -7: Voltage over AIN3 and GND.
> -
> -Optional properties:
> -
> - - exported-channels : exported_channels is a bitmask that specifies which
> -		       channels should be accessible by the user.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = <0x14>;
> -};
> -
> -In this example only channel 2 and 4 would be accessible by the user.
> diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
> index 56ee797..30250ca 100644
> --- a/Documentation/hwmon/ads1015
> +++ b/Documentation/hwmon/ads1015
> @@ -38,30 +38,52 @@ Platform Data
>  
>  In linux/i2c/ads1015.h platform data is defined as:
>  
> +struct ads1015_channel_data {
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>  	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  exported_channels is a bitmask that specifies which inputs should be exported.
>  
> +channel_data contains configuration data for the exported inputs:
> +- pga ist the programmable gain amplifier
> +  0: FS = +/- 6.144 V
> +  1: FS = +/- 4.096 V
> +  2: FS = +/- 2.048 V (default)
> +  3: FS = +/- 1.024 V
> +  4: FS = +/- 0.512 V
> +  5: FS = +/- 0.256 V
> +- data_rate in samples per second
> +  0: 128
> +  1: 250
> +  2: 490
> +  3: 920
> +  4: 1600
> +  5: 2400
> +  6: 3300
> +
>  Example:
>  struct ads1015_platform_data data = {
> -	.exported_channels = (1 << 2) | (1 << 4)
> +	.exported_channels = (1 << 2) | (1 << 4),
> +	.channel_data = {
> +		{},
> +		{},
> +		{ .pga = 1, .data_rate = 0 },
> +		{},
> +		{ .pga = 4, .data_rate = 5},
> +	}

You can use explicit C99 array initializers here which would be more
concise; something like:

.channel_data = {
	[2] = { .pga = 1, .data_rate = 0 },
	[4] = { .pga = 4, .data_rate = 5 },
}

Also, would it not make sense to encode the exported_channels bit into
this array too.  Maybe something like this?

.channel_data = {
	[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
	[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
}



>  };
>  
> -In this case only in2_input and in4_input would be created.
> +In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input
> +(FS +/- 0.512 V, 2400 SPS) would be created.
>  
>  Devicetree
>  ----------
>  
> -The ads1015 node may have an "exported-channels" property.
> -exported_channels is a bitmask that specifies which inputs should be exported.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = < 0x14 >;
> -};
> -
> -In this case only in2_input and in4_input would be created.
> +Configuration is also possible via devicetree:
> +Documentation/devicetree/bindings/hwmon/ads1015.txt
> diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
> index 8ef2b60..8657863 100644
> --- a/drivers/hwmon/ads1015.c
> +++ b/drivers/hwmon/ads1015.c
> @@ -25,7 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> -#include <linux/jiffies.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -45,12 +45,18 @@ enum {
>  static const unsigned int fullscale_table[8] = {
>  	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
>  
> -#define ADS1015_CONFIG_CHANNELS 8
> +/* Data rates in samples per second */
> +static const unsigned int data_rate_table[8] = {
> +	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
> +
>  #define ADS1015_DEFAULT_CHANNELS 0xff
> +#define ADS1015_DEFAULT_PGA 2
> +#define ADS1015_DEFAULT_DATA_RATE 4
>  
>  struct ads1015_data {
>  	struct device *hwmon_dev;
>  	struct mutex update_lock; /* mutex protect updates */
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
> @@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
>  {
>  	u16 config;
>  	s16 conversion;
> -	unsigned int pga;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
> +	unsigned int pga = data->channel_data[channel].pga;
>  	int fullscale;
> +	unsigned int data_rate = data->channel_data[channel].data_rate;
> +	unsigned int conversion_time_ms;
>  	unsigned int k;
> -	struct ads1015_data *data = i2c_get_clientdata(client);
>  	int res;
>  
>  	mutex_lock(&data->update_lock);
>  
> -	/* get fullscale voltage */
> +	/* get channel parameters */
>  	res = ads1015_read_reg(client, ADS1015_CONFIG);
>  	if (res < 0)
>  		goto err_unlock;
>  	config = res;
> -	pga = (config >> 9) & 0x0007;
>  	fullscale = fullscale_table[pga];
> +	conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];
>  
>  	/* set channel and start single conversion */
> -	config &= ~(0x0007 << 12);
> -	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
> +	config &= 0x001f;
> +	config |= (1 << 15) | (1 << 8);
> +	config |= (channel & 0x0007) << 12;
> +	config |= (pga & 0x0007) << 9;
> +	config |= (data_rate & 0x0007) << 5;
>  
> -	/* wait until conversion finished */
>  	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
>  	if (res < 0)
>  		goto err_unlock;
> +
> +	/* wait until conversion finished */
>  	for (k = 0; k < 5; ++k) {
> -		schedule_timeout(msecs_to_jiffies(1));
> +		msleep(k ? 1 : conversion_time_ms);
>  		res = ads1015_read_reg(client, ADS1015_CONFIG);
>  		if (res < 0)
>  			goto err_unlock;
> @@ -168,26 +180,82 @@ static int ads1015_remove(struct i2c_client *client)
>  
>  static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
>  {
> +	unsigned int k;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
>  	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
>  #ifdef CONFIG_OF
> -	struct device_node *np = client->dev.of_node;
> -	const __be32 *of_channels;
> -	int of_channels_size;
> +	unsigned int of_exported_channels = 0;
> +	struct device_node *node;
>  #endif
>  
>  	/* prefer platform data */
> -	if (pdata)
> +	if (pdata) {
> +		memcpy(data->channel_data, pdata->channel_data,
> +		       sizeof(data->channel_data));
>  		return pdata->exported_channels;
> +	}
>  
>  #ifdef CONFIG_OF
>  	/* fallback on OF */
> -	of_channels = of_get_property(np, "exported-channels",
> -				      &of_channels_size);
> -	if (of_channels && (of_channels_size == sizeof(*of_channels)))
> -		return be32_to_cpup(of_channels);
> +	if (!client->dev.of_node
> +	    || !of_get_next_child(client->dev.of_node, NULL))
> +		goto fallback_default;
> +
> +	for_each_child_of_node(client->dev.of_node, node) {
> +		const __be32 *property;
> +		int len;
> +		unsigned int channel;
> +		unsigned int pga = ADS1015_DEFAULT_PGA;
> +		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
> +
> +		property = of_get_property(node, "reg", &len);
> +		if (!property || (len < sizeof(int))) {
> +			dev_err(&client->dev, "ads1015: invalid reg on %s\n",
> +				node->full_name);
> +			continue;
> +		}
> +
> +		channel = be32_to_cpup(property);
> +		if (channel > ADS1015_CONFIG_CHANNELS) {
> +			dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
> +				channel, node->full_name);
> +			continue;
> +		}
> +
> +		property = of_get_property(node, "ti,gain", &len);
> +		if (property && (len == sizeof(int))) {
> +			pga = be32_to_cpup(property);
> +			if (pga > 7) {
> +				dev_err(&client->dev,
> +					"ads1015: invalid gain on %s\n",
> +					node->full_name);
> +			}
> +		}
> +
> +		property = of_get_property(node, "ti,datarate", &len);
> +		if (property && (len == sizeof(int))) {
> +			data_rate = be32_to_cpup(property);
> +			if (data_rate > 7)
> +				dev_err(&client->dev,
> +					"ads1015: invalid data_rate on %s\n",
> +					node->full_name);
> +		}
> +
> +		of_exported_channels |= (1 << channel);
> +		data->channel_data[channel].pga = pga;
> +		data->channel_data[channel].data_rate = data_rate;
> +	}
> +
> +	return of_exported_channels;
>  #endif
>  
>  	/* fallback on default configuration */
> +fallback_default:
> +	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> +		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
> +		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
> +	}
> +
>  	return ADS1015_DEFAULT_CHANNELS;
>  }
>  
> diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
> index 8541c6a..8f32040 100644
> --- a/include/linux/i2c/ads1015.h
> +++ b/include/linux/i2c/ads1015.h
> @@ -21,8 +21,16 @@
>  #ifndef LINUX_ADS1015_H
>  #define LINUX_ADS1015_H
>  
> +#define ADS1015_CONFIG_CHANNELS 8
> +
> +struct ads1015_channel_data {
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>  	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  #endif /* LINUX_ADS1015_H */
> -- 
> 1.5.6.5
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Dirk Eibach]

Configuration for ads1015 gain and datarate is possible via
devicetree or platform data.

This is a followup patch to previous ads1015 patches on Jean Delvares
tree.

Changes since v1:
- replaced exported_channels bitmask with an enable entry per channel
- improved platform data example in Documentation
- improved patch description

Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
 .../devicetree/bindings/hwmon/ads1015.txt          |   73 +++++++++++++
 Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 -----
 Documentation/hwmon/ads1015                        |   44 ++++++---
 drivers/hwmon/ads1015.c                            |  113 +++++++++++++++----
 include/linux/i2c/ads1015.h                        |   10 ++-
 5 files changed, 202 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
 delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
new file mode 100644
index 0000000..fb6e139
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
@@ -0,0 +1,73 @@
+ADS1015 (I2C)
+
+This device is a 12-bit A-D converter with 4 inputs.
+
+The inputs can be used single ended or in certain differential combinations.
+
+For configuration all possible combinations are mapped to 8 channels:
+  0: Voltage over AIN0 and AIN1.
+  1: Voltage over AIN0 and AIN3.
+  2: Voltage over AIN1 and AIN3.
+  3: Voltage over AIN2 and AIN3.
+  4: Voltage over AIN0 and GND.
+  5: Voltage over AIN1 and GND.
+  6: Voltage over AIN2 and GND.
+  7: Voltage over AIN3 and GND.
+
+Each channel can be configured indvidually:
+ - pga ist the programmable gain amplifier
+    0: FS = +/- 6.144 V
+    1: FS = +/- 4.096 V
+    2: FS = +/- 2.048 V (default)
+    3: FS = +/- 1.024 V
+    4: FS = +/- 0.512 V
+    5: FS = +/- 0.256 V
+ - data_rate in samples per second
+    0: 128
+    1: 250
+    2: 490
+    3: 920
+    4: 1600
+    5: 2400
+    6: 3300
+
+1) The /ads1015 node
+
+  Required properties:
+
+   - compatible : must be "ti,ads1015"
+   - reg : I2C busaddress of the device
+   - #address-cells : must be <1>
+   - #size-cells : must be <0>
+
+  The node contains child nodes for each channel that the platform uses.
+
+  Example ADS1015 node:
+
+    ads1015@49 {
+	    compatible = "ti,ads1015";
+	    reg = <0x49>;
+	    #address-cells = <1>;
+	    #size-cells = <0>;
+
+	    [ child node definitions... ]
+    }
+
+2) channel nodes
+
+  Required properties:
+
+   - reg : the channel number
+
+  Optional properties:
+
+   - ti,gain : the programmable gain amplifier setting
+   - ti,datarate : the converter datarate
+
+  Example ADS1015 node:
+
+    channel@4 {
+	    reg = <4>;
+	    ti,gain = <3>;
+	    ti,datarate = <5>;
+    };
diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
deleted file mode 100644
index 985e24d..0000000
--- a/Documentation/devicetree/bindings/i2c/ads1015.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-ADS1015 (I2C)
-
-This device is a 12-bit A-D converter with 4 inputs.
-
-The inputs can be used single ended or in certain differential combinations.
-
-For configuration all possible combinations are mapped to 8 channels:
-0: Voltage over AIN0 and AIN1.
-1: Voltage over AIN0 and AIN3.
-2: Voltage over AIN1 and AIN3.
-3: Voltage over AIN2 and AIN3.
-4: Voltage over AIN0 and GND.
-5: Voltage over AIN1 and GND.
-6: Voltage over AIN2 and GND.
-7: Voltage over AIN3 and GND.
-
-Optional properties:
-
- - exported-channels : exported_channels is a bitmask that specifies which
-		       channels should be accessible by the user.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = <0x14>;
-};
-
-In this example only channel 2 and 4 would be accessible by the user.
diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
index 56ee797..18a8e00 100644
--- a/Documentation/hwmon/ads1015
+++ b/Documentation/hwmon/ads1015
@@ -38,30 +38,48 @@ Platform Data
 
 In linux/i2c/ads1015.h platform data is defined as:
 
+struct ads1015_channel_data {
+	unsigned int pga;
+	unsigned int data_rate;
+};
+
 struct ads1015_platform_data {
 	unsigned int exported_channels;
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 exported_channels is a bitmask that specifies which inputs should be exported.
 
+channel_data contains configuration data for the exported inputs:
+- pga ist the programmable gain amplifier
+  0: FS = +/- 6.144 V
+  1: FS = +/- 4.096 V
+  2: FS = +/- 2.048 V (default)
+  3: FS = +/- 1.024 V
+  4: FS = +/- 0.512 V
+  5: FS = +/- 0.256 V
+- data_rate in samples per second
+  0: 128
+  1: 250
+  2: 490
+  3: 920
+  4: 1600
+  5: 2400
+  6: 3300
+
 Example:
 struct ads1015_platform_data data = {
-	.exported_channels = (1 << 2) | (1 << 4)
+	.channel_data = {
+		[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
+		[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
+	}
 };
 
-In this case only in2_input and in4_input would be created.
+In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input
+(FS +/- 0.512 V, 2400 SPS) would be created.
 
 Devicetree
 ----------
 
-The ads1015 node may have an "exported-channels" property.
-exported_channels is a bitmask that specifies which inputs should be exported.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = < 0x14 >;
-};
-
-In this case only in2_input and in4_input would be created.
+Configuration is also possible via devicetree:
+Documentation/devicetree/bindings/hwmon/ads1015.txt
diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
index 8ef2b60..a7c524d 100644
--- a/drivers/hwmon/ads1015.c
+++ b/drivers/hwmon/ads1015.c
@@ -25,7 +25,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
-#include <linux/jiffies.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -45,12 +45,18 @@ enum {
 static const unsigned int fullscale_table[8] = {
 	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
 
-#define ADS1015_CONFIG_CHANNELS 8
+/* Data rates in samples per second */
+static const unsigned int data_rate_table[8] = {
+	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
+
 #define ADS1015_DEFAULT_CHANNELS 0xff
+#define ADS1015_DEFAULT_PGA 2
+#define ADS1015_DEFAULT_DATA_RATE 4
 
 struct ads1015_data {
 	struct device *hwmon_dev;
 	struct mutex update_lock; /* mutex protect updates */
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
@@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
 {
 	u16 config;
 	s16 conversion;
-	unsigned int pga;
+	struct ads1015_data *data = i2c_get_clientdata(client);
+	unsigned int pga = data->channel_data[channel].pga;
 	int fullscale;
+	unsigned int data_rate = data->channel_data[channel].data_rate;
+	unsigned int conversion_time_ms;
 	unsigned int k;
-	struct ads1015_data *data = i2c_get_clientdata(client);
 	int res;
 
 	mutex_lock(&data->update_lock);
 
-	/* get fullscale voltage */
+	/* get channel parameters */
 	res = ads1015_read_reg(client, ADS1015_CONFIG);
 	if (res < 0)
 		goto err_unlock;
 	config = res;
-	pga = (config >> 9) & 0x0007;
 	fullscale = fullscale_table[pga];
+	conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];
 
 	/* set channel and start single conversion */
-	config &= ~(0x0007 << 12);
-	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
+	config &= 0x001f;
+	config |= (1 << 15) | (1 << 8);
+	config |= (channel & 0x0007) << 12;
+	config |= (pga & 0x0007) << 9;
+	config |= (data_rate & 0x0007) << 5;
 
-	/* wait until conversion finished */
 	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
 	if (res < 0)
 		goto err_unlock;
+
+	/* wait until conversion finished */
 	for (k = 0; k < 5; ++k) {
-		schedule_timeout(msecs_to_jiffies(1));
+		msleep(k ? 1 : conversion_time_ms);
 		res = ads1015_read_reg(client, ADS1015_CONFIG);
 		if (res < 0)
 			goto err_unlock;
@@ -166,29 +178,83 @@ static int ads1015_remove(struct i2c_client *client)
 	return 0;
 }
 
-static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
+static void ads1015_get_exported_channels(struct i2c_client *client)
 {
+	unsigned int k;
+	struct ads1015_data *data = i2c_get_clientdata(client);
 	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
 #ifdef CONFIG_OF
-	struct device_node *np = client->dev.of_node;
-	const __be32 *of_channels;
-	int of_channels_size;
+	struct device_node *node;
 #endif
 
 	/* prefer platform data */
-	if (pdata)
-		return pdata->exported_channels;
+	if (pdata) {
+		memcpy(data->channel_data, pdata->channel_data,
+		       sizeof(data->channel_data));
+		return;
+	}
 
 #ifdef CONFIG_OF
 	/* fallback on OF */
-	of_channels = of_get_property(np, "exported-channels",
-				      &of_channels_size);
-	if (of_channels && (of_channels_size == sizeof(*of_channels)))
-		return be32_to_cpup(of_channels);
+	if (!client->dev.of_node
+	    || !of_get_next_child(client->dev.of_node, NULL))
+		goto fallback_default;
+
+	for_each_child_of_node(client->dev.of_node, node) {
+		const __be32 *property;
+		int len;
+		unsigned int channel;
+		unsigned int pga = ADS1015_DEFAULT_PGA;
+		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
+
+		property = of_get_property(node, "reg", &len);
+		if (!property || (len < sizeof(int))) {
+			dev_err(&client->dev, "ads1015: invalid reg on %s\n",
+				node->full_name);
+			continue;
+		}
+
+		channel = be32_to_cpup(property);
+		if (channel > ADS1015_CONFIG_CHANNELS) {
+			dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
+				channel, node->full_name);
+			continue;
+		}
+
+		property = of_get_property(node, "ti,gain", &len);
+		if (property && (len == sizeof(int))) {
+			pga = be32_to_cpup(property);
+			if (pga > 7) {
+				dev_err(&client->dev,
+					"ads1015: invalid gain on %s\n",
+					node->full_name);
+			}
+		}
+
+		property = of_get_property(node, "ti,datarate", &len);
+		if (property && (len == sizeof(int))) {
+			data_rate = be32_to_cpup(property);
+			if (data_rate > 7)
+				dev_err(&client->dev,
+					"ads1015: invalid data_rate on %s\n",
+					node->full_name);
+		}
+
+		data->channel_data[channel].enabled = true;
+		data->channel_data[channel].pga = pga;
+		data->channel_data[channel].data_rate = data_rate;
+	}
+
+	return;
 #endif
 
 	/* fallback on default configuration */
-	return ADS1015_DEFAULT_CHANNELS;
+fallback_default:
+	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
+		data->channel_data[k].enabled = true;
+		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
+		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
+	}
 }
 
 static int ads1015_probe(struct i2c_client *client,
@@ -196,7 +262,6 @@ static int ads1015_probe(struct i2c_client *client,
 {
 	struct ads1015_data *data;
 	int err;
-	unsigned int exported_channels;
 	unsigned int k;
 
 	data = kzalloc(sizeof(struct ads1015_data), GFP_KERNEL);
@@ -209,9 +274,9 @@ static int ads1015_probe(struct i2c_client *client,
 	mutex_init(&data->update_lock);
 
 	/* build sysfs attribute group */
-	exported_channels = ads1015_get_exported_channels(client);
+	ads1015_get_exported_channels(client);
 	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
-		if (!(exported_channels & (1<<k)))
+		if (!data->channel_data[k].enabled)
 			continue;
 		err = device_create_file(&client->dev, &ads1015_in[k].dev_attr);
 		if (err)
diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
index 8541c6a..0d9e746 100644
--- a/include/linux/i2c/ads1015.h
+++ b/include/linux/i2c/ads1015.h
@@ -21,8 +21,16 @@
 #ifndef LINUX_ADS1015_H
 #define LINUX_ADS1015_H
 
+#define ADS1015_CONFIG_CHANNELS 8
+
+struct ads1015_channel_data {
+	bool enabled;
+	unsigned int pga;
+	unsigned int data_rate;
+};
+
 struct ads1015_platform_data {
-	unsigned int exported_channels;
+	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
 };
 
 #endif /* LINUX_ADS1015_H */
-- 
1.5.6.5

Re: [PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Grant Likely]

On Wed, Mar 16, 2011 at 11:01:55AM +0100, Dirk Eibach wrote:
> Configuration for ads1015 gain and datarate is possible via
> devicetree or platform data.
> 
> This is a followup patch to previous ads1015 patches on Jean Delvares
> tree.
> 
> Changes since v1:
> - replaced exported_channels bitmask with an enable entry per channel
> - improved platform data example in Documentation
> - improved patch description
> 
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   73 +++++++++++++
>  Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 -----
>  Documentation/hwmon/ads1015                        |   44 ++++++---
>  drivers/hwmon/ads1015.c                            |  113 +++++++++++++++----
>  include/linux/i2c/ads1015.h                        |   10 ++-
>  5 files changed, 202 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> new file mode 100644
> index 0000000..fb6e139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured indvidually:
> + - pga ist the programmable gain amplifier
> +    0: FS = +/- 6.144 V
> +    1: FS = +/- 4.096 V
> +    2: FS = +/- 2.048 V (default)
> +    3: FS = +/- 1.024 V
> +    4: FS = +/- 0.512 V
> +    5: FS = +/- 0.256 V
> + - data_rate in samples per second
> +    0: 128
> +    1: 250
> +    2: 490
> +    3: 920
> +    4: 1600
> +    5: 2400
> +    6: 3300
> +
> +1) The /ads1015 node
> +
> +  Required properties:
> +
> +   - compatible : must be "ti,ads1015"
> +   - reg : I2C busaddress of the device
> +   - #address-cells : must be <1>
> +   - #size-cells : must be <0>
> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example ADS1015 node:
> +
> +    ads1015@49 {
> +	    compatible = "ti,ads1015";
> +	    reg = <0x49>;
> +	    #address-cells = <1>;
> +	    #size-cells = <0>;
> +
> +	    [ child node definitions... ]
> +    }
> +
> +2) channel nodes
> +
> +  Required properties:
> +
> +   - reg : the channel number
> +
> +  Optional properties:
> +
> +   - ti,gain : the programmable gain amplifier setting
> +   - ti,datarate : the converter datarate
> +
> +  Example ADS1015 node:
> +
> +    channel@4 {
> +	    reg = <4>;
> +	    ti,gain = <3>;
> +	    ti,datarate = <5>;
> +    };
> diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> deleted file mode 100644
> index 985e24d..0000000
> --- a/Documentation/devicetree/bindings/i2c/ads1015.txt
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -ADS1015 (I2C)
> -
> -This device is a 12-bit A-D converter with 4 inputs.
> -
> -The inputs can be used single ended or in certain differential combinations.
> -
> -For configuration all possible combinations are mapped to 8 channels:
> -0: Voltage over AIN0 and AIN1.
> -1: Voltage over AIN0 and AIN3.
> -2: Voltage over AIN1 and AIN3.
> -3: Voltage over AIN2 and AIN3.
> -4: Voltage over AIN0 and GND.
> -5: Voltage over AIN1 and GND.
> -6: Voltage over AIN2 and GND.
> -7: Voltage over AIN3 and GND.
> -
> -Optional properties:
> -
> - - exported-channels : exported_channels is a bitmask that specifies which
> -		       channels should be accessible by the user.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = <0x14>;
> -};
> -
> -In this example only channel 2 and 4 would be accessible by the user.
> diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
> index 56ee797..18a8e00 100644
> --- a/Documentation/hwmon/ads1015
> +++ b/Documentation/hwmon/ads1015
> @@ -38,30 +38,48 @@ Platform Data
>  
>  In linux/i2c/ads1015.h platform data is defined as:
>  
> +struct ads1015_channel_data {
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>  	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  exported_channels is a bitmask that specifies which inputs should be exported.
>  
> +channel_data contains configuration data for the exported inputs:
> +- pga ist the programmable gain amplifier
> +  0: FS = +/- 6.144 V
> +  1: FS = +/- 4.096 V
> +  2: FS = +/- 2.048 V (default)
> +  3: FS = +/- 1.024 V
> +  4: FS = +/- 0.512 V
> +  5: FS = +/- 0.256 V
> +- data_rate in samples per second
> +  0: 128
> +  1: 250
> +  2: 490
> +  3: 920
> +  4: 1600
> +  5: 2400
> +  6: 3300
> +
>  Example:
>  struct ads1015_platform_data data = {
> -	.exported_channels = (1 << 2) | (1 << 4)
> +	.channel_data = {
> +		[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
> +		[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
> +	}
>  };
>  
> -In this case only in2_input and in4_input would be created.
> +In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input
> +(FS +/- 0.512 V, 2400 SPS) would be created.
>  
>  Devicetree
>  ----------
>  
> -The ads1015 node may have an "exported-channels" property.
> -exported_channels is a bitmask that specifies which inputs should be exported.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = < 0x14 >;
> -};
> -
> -In this case only in2_input and in4_input would be created.
> +Configuration is also possible via devicetree:
> +Documentation/devicetree/bindings/hwmon/ads1015.txt
> diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
> index 8ef2b60..a7c524d 100644
> --- a/drivers/hwmon/ads1015.c
> +++ b/drivers/hwmon/ads1015.c
> @@ -25,7 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> -#include <linux/jiffies.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -45,12 +45,18 @@ enum {
>  static const unsigned int fullscale_table[8] = {
>  	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
>  
> -#define ADS1015_CONFIG_CHANNELS 8
> +/* Data rates in samples per second */
> +static const unsigned int data_rate_table[8] = {
> +	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
> +
>  #define ADS1015_DEFAULT_CHANNELS 0xff
> +#define ADS1015_DEFAULT_PGA 2
> +#define ADS1015_DEFAULT_DATA_RATE 4
>  
>  struct ads1015_data {
>  	struct device *hwmon_dev;
>  	struct mutex update_lock; /* mutex protect updates */
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
> @@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
>  {
>  	u16 config;
>  	s16 conversion;
> -	unsigned int pga;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
> +	unsigned int pga = data->channel_data[channel].pga;
>  	int fullscale;
> +	unsigned int data_rate = data->channel_data[channel].data_rate;
> +	unsigned int conversion_time_ms;
>  	unsigned int k;
> -	struct ads1015_data *data = i2c_get_clientdata(client);
>  	int res;
>  
>  	mutex_lock(&data->update_lock);
>  
> -	/* get fullscale voltage */
> +	/* get channel parameters */
>  	res = ads1015_read_reg(client, ADS1015_CONFIG);
>  	if (res < 0)
>  		goto err_unlock;
>  	config = res;
> -	pga = (config >> 9) & 0x0007;
>  	fullscale = fullscale_table[pga];
> +	conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];
>  
>  	/* set channel and start single conversion */
> -	config &= ~(0x0007 << 12);
> -	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
> +	config &= 0x001f;
> +	config |= (1 << 15) | (1 << 8);
> +	config |= (channel & 0x0007) << 12;
> +	config |= (pga & 0x0007) << 9;
> +	config |= (data_rate & 0x0007) << 5;
>  
> -	/* wait until conversion finished */
>  	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
>  	if (res < 0)
>  		goto err_unlock;
> +
> +	/* wait until conversion finished */
>  	for (k = 0; k < 5; ++k) {
> -		schedule_timeout(msecs_to_jiffies(1));
> +		msleep(k ? 1 : conversion_time_ms);
>  		res = ads1015_read_reg(client, ADS1015_CONFIG);
>  		if (res < 0)
>  			goto err_unlock;
> @@ -166,29 +178,83 @@ static int ads1015_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
> +static void ads1015_get_exported_channels(struct i2c_client *client)
>  {
> +	unsigned int k;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
>  	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
>  #ifdef CONFIG_OF
> -	struct device_node *np = client->dev.of_node;
> -	const __be32 *of_channels;
> -	int of_channels_size;
> +	struct device_node *node;
>  #endif
>  
>  	/* prefer platform data */
> -	if (pdata)
> -		return pdata->exported_channels;
> +	if (pdata) {
> +		memcpy(data->channel_data, pdata->channel_data,
> +		       sizeof(data->channel_data));
> +		return;
> +	}
>  
>  #ifdef CONFIG_OF
>  	/* fallback on OF */
> -	of_channels = of_get_property(np, "exported-channels",
> -				      &of_channels_size);
> -	if (of_channels && (of_channels_size == sizeof(*of_channels)))
> -		return be32_to_cpup(of_channels);
> +	if (!client->dev.of_node
> +	    || !of_get_next_child(client->dev.of_node, NULL))
> +		goto fallback_default;
> +
> +	for_each_child_of_node(client->dev.of_node, node) {
> +		const __be32 *property;
> +		int len;
> +		unsigned int channel;
> +		unsigned int pga = ADS1015_DEFAULT_PGA;
> +		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
> +
> +		property = of_get_property(node, "reg", &len);
> +		if (!property || (len < sizeof(int))) {
> +			dev_err(&client->dev, "ads1015: invalid reg on %s\n",
> +				node->full_name);
> +			continue;
> +		}
> +
> +		channel = be32_to_cpup(property);
> +		if (channel > ADS1015_CONFIG_CHANNELS) {
> +			dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
> +				channel, node->full_name);
> +			continue;
> +		}
> +
> +		property = of_get_property(node, "ti,gain", &len);
> +		if (property && (len == sizeof(int))) {
> +			pga = be32_to_cpup(property);
> +			if (pga > 7) {
> +				dev_err(&client->dev,
> +					"ads1015: invalid gain on %s\n",
> +					node->full_name);
> +			}
> +		}
> +
> +		property = of_get_property(node, "ti,datarate", &len);
> +		if (property && (len == sizeof(int))) {
> +			data_rate = be32_to_cpup(property);
> +			if (data_rate > 7)
> +				dev_err(&client->dev,
> +					"ads1015: invalid data_rate on %s\n",
> +					node->full_name);
> +		}
> +
> +		data->channel_data[channel].enabled = true;
> +		data->channel_data[channel].pga = pga;
> +		data->channel_data[channel].data_rate = data_rate;
> +	}
> +
> +	return;
>  #endif
>  
>  	/* fallback on default configuration */
> -	return ADS1015_DEFAULT_CHANNELS;
> +fallback_default:
> +	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> +		data->channel_data[k].enabled = true;
> +		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
> +		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
> +	}
>  }
>  
>  static int ads1015_probe(struct i2c_client *client,
> @@ -196,7 +262,6 @@ static int ads1015_probe(struct i2c_client *client,
>  {
>  	struct ads1015_data *data;
>  	int err;
> -	unsigned int exported_channels;
>  	unsigned int k;
>  
>  	data = kzalloc(sizeof(struct ads1015_data), GFP_KERNEL);
> @@ -209,9 +274,9 @@ static int ads1015_probe(struct i2c_client *client,
>  	mutex_init(&data->update_lock);
>  
>  	/* build sysfs attribute group */
> -	exported_channels = ads1015_get_exported_channels(client);
> +	ads1015_get_exported_channels(client);
>  	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> -		if (!(exported_channels & (1<<k)))
> +		if (!data->channel_data[k].enabled)
>  			continue;
>  		err = device_create_file(&client->dev, &ads1015_in[k].dev_attr);
>  		if (err)
> diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
> index 8541c6a..0d9e746 100644
> --- a/include/linux/i2c/ads1015.h
> +++ b/include/linux/i2c/ads1015.h
> @@ -21,8 +21,16 @@
>  #ifndef LINUX_ADS1015_H
>  #define LINUX_ADS1015_H
>  
> +#define ADS1015_CONFIG_CHANNELS 8
> +
> +struct ads1015_channel_data {
> +	bool enabled;
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
> -	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  #endif /* LINUX_ADS1015_H */
> -- 
> 1.5.6.5
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Jean Delvare]

Hi Dirk,

On Wed, 16 Mar 2011 11:01:55 +0100, Dirk Eibach wrote:
> Configuration for ads1015 gain and datarate is possible via
> devicetree or platform data.
> 
> This is a followup patch to previous ads1015 patches on Jean Delvares
> tree.
> 
> Changes since v1:
> - replaced exported_channels bitmask with an enable entry per channel
> - improved platform data example in Documentation
> - improved patch description
> 
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   73 +++++++++++++
>  Documentation/devicetree/bindings/i2c/ads1015.txt  |   29 -----
>  Documentation/hwmon/ads1015                        |   44 ++++++---
>  drivers/hwmon/ads1015.c                            |  113 +++++++++++++++----
>  include/linux/i2c/ads1015.h                        |   10 ++-
>  5 files changed, 202 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  delete mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt

Took me a little longer than I expected to review this, but here we go
finally:

> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> new file mode 100644
> index 0000000..fb6e139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured indvidually:

Spelling: individually.

> + - pga ist the programmable gain amplifier

"is", not "ist" ;)

> +    0: FS = +/- 6.144 V

Please include the term "full scale" in the description, otherwise it's
not clear what "FS" means.

> +    1: FS = +/- 4.096 V
> +    2: FS = +/- 2.048 V (default)
> +    3: FS = +/- 1.024 V
> +    4: FS = +/- 0.512 V
> +    5: FS = +/- 0.256 V
> + - data_rate in samples per second
> +    0: 128
> +    1: 250
> +    2: 490
> +    3: 920
> +    4: 1600
> +    5: 2400
> +    6: 3300

These sample rates make me wonder if hwmon is really the right
subsystem for this device. hwmon is more like 2 samples per second.

As you documented the default value for pga, you should do the same for
data_rate.

> +
> +1) The /ads1015 node
> +
> +  Required properties:
> +
> +   - compatible : must be "ti,ads1015"
> +   - reg : I2C busaddress of the device

Missing space between "bus" and "address".

> +   - #address-cells : must be <1>
> +   - #size-cells : must be <0>
> +
> +  The node contains child nodes for each channel that the platform uses.
> +
> +  Example ADS1015 node:
> +
> +    ads1015@49 {
> +	    compatible = "ti,ads1015";
> +	    reg = <0x49>;
> +	    #address-cells = <1>;
> +	    #size-cells = <0>;
> +
> +	    [ child node definitions... ]
> +    }
> +
> +2) channel nodes
> +
> +  Required properties:
> +
> +   - reg : the channel number
> +
> +  Optional properties:
> +
> +   - ti,gain : the programmable gain amplifier setting
> +   - ti,datarate : the converter datarate

Missing space between "data" and "rate" (the second time, of course.)
It's also confusing to have "datarate" for the property, but
"data_rate" as the struct member.

> +
> +  Example ADS1015 node:

This is an example channel node, not ADS1015 node.

> +
> +    channel@4 {
> +	    reg = <4>;
> +	    ti,gain = <3>;
> +	    ti,datarate = <5>;
> +    };
> diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> deleted file mode 100644
> index 985e24d..0000000
> --- a/Documentation/devicetree/bindings/i2c/ads1015.txt
> +++ /dev/null

For the next iteration, please consider that the file was originally
located in Documentation/devicetree/bindings/hwmon. This is where I put
it upon recommendation by Grant Likely. So this new patch should modify
the document in-place, rather than create a new one and delete the old
one.

> @@ -1,29 +0,0 @@
> -ADS1015 (I2C)
> -
> -This device is a 12-bit A-D converter with 4 inputs.
> -
> -The inputs can be used single ended or in certain differential combinations.
> -
> -For configuration all possible combinations are mapped to 8 channels:
> -0: Voltage over AIN0 and AIN1.
> -1: Voltage over AIN0 and AIN3.
> -2: Voltage over AIN1 and AIN3.
> -3: Voltage over AIN2 and AIN3.
> -4: Voltage over AIN0 and GND.
> -5: Voltage over AIN1 and GND.
> -6: Voltage over AIN2 and GND.
> -7: Voltage over AIN3 and GND.
> -
> -Optional properties:
> -
> - - exported-channels : exported_channels is a bitmask that specifies which
> -		       channels should be accessible by the user.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = <0x14>;
> -};
> -
> -In this example only channel 2 and 4 would be accessible by the user.
> diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
> index 56ee797..18a8e00 100644
> --- a/Documentation/hwmon/ads1015
> +++ b/Documentation/hwmon/ads1015
> @@ -38,30 +38,48 @@ Platform Data
>  
>  In linux/i2c/ads1015.h platform data is defined as:
>  
> +struct ads1015_channel_data {
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
>  	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };

These structure definitions do not match the actual ones in
<linux/i2c/ads1015.h>. This is a common problem when you duplicate
code... It doesn't stay magically in sync.

>  
>  exported_channels is a bitmask that specifies which inputs should be exported.

exported_channels no longer exists.

>  
> +channel_data contains configuration data for the exported inputs:

It was underlined before that the term "exported" should be avoided
here. "for the used input combinations" would sound better.

> +- pga ist the programmable gain amplifier

"is"

> +  0: FS = +/- 6.144 V
> +  1: FS = +/- 4.096 V
> +  2: FS = +/- 2.048 V (default)

I fail to see how this default is relevant when using platform data. If
the value isn't specified as part of the channel data, it'll be read as
0 by the driver, not 2.

> +  3: FS = +/- 1.024 V
> +  4: FS = +/- 0.512 V
> +  5: FS = +/- 0.256 V
> +- data_rate in samples per second
> +  0: 128
> +  1: 250
> +  2: 490
> +  3: 920
> +  4: 1600
> +  5: 2400
> +  6: 3300
> +

The duplicate documentation between Documentation/hwmon/ads1015 and
Documentation/devicetree/bindings/hwmon/ads1015.txt is somewhat
unpleasant. It will be difficult to keep things in sync over time. I
wonder if this could be avoided.

>  Example:
>  struct ads1015_platform_data data = {
> -	.exported_channels = (1 << 2) | (1 << 4)
> +	.channel_data = {
> +		[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
> +		[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
> +	}
>  };

This looks pretty good, I have to admit :)

>  
> -In this case only in2_input and in4_input would be created.
> +In this case only in2_input(FS +/- 4.096 V, 128 SPS) and in4_input

Space missing before opening parenthesis.

> +(FS +/- 0.512 V, 2400 SPS) would be created.
>  
>  Devicetree
>  ----------
>  
> -The ads1015 node may have an "exported-channels" property.
> -exported_channels is a bitmask that specifies which inputs should be exported.
> -
> -Example:
> -ads1015@49 {
> -	compatible = "ti,ads1015";
> -	reg = <0x49>;
> -	exported-channels = < 0x14 >;
> -};
> -
> -In this case only in2_input and in4_input would be created.
> +Configuration is also possible via devicetree:
> +Documentation/devicetree/bindings/hwmon/ads1015.txt
> diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
> index 8ef2b60..a7c524d 100644
> --- a/drivers/hwmon/ads1015.c
> +++ b/drivers/hwmon/ads1015.c
> @@ -25,7 +25,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> -#include <linux/jiffies.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -45,12 +45,18 @@ enum {
>  static const unsigned int fullscale_table[8] = {
>  	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
>  
> -#define ADS1015_CONFIG_CHANNELS 8
> +/* Data rates in samples per second */
> +static const unsigned int data_rate_table[8] = {
> +	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
> +
>  #define ADS1015_DEFAULT_CHANNELS 0xff
> +#define ADS1015_DEFAULT_PGA 2
> +#define ADS1015_DEFAULT_DATA_RATE 4

Does it really make sense to have a default configuration? The chip
needs proper configuration to be usable at all, I would think it makes
sense to make the platform or devicetree data mandatory. I am
particularly worried about the case where the PGA is set improperly: the
input voltage could be over what the chip supports?

Or asking the other way around: do you see any problem with making the
platform data or devicetree data mandatory?

>  
>  struct ads1015_data {
>  	struct device *hwmon_dev;
>  	struct mutex update_lock; /* mutex protect updates */
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
> @@ -71,32 +77,38 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
>  {
>  	u16 config;
>  	s16 conversion;
> -	unsigned int pga;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
> +	unsigned int pga = data->channel_data[channel].pga;
>  	int fullscale;
> +	unsigned int data_rate = data->channel_data[channel].data_rate;
> +	unsigned int conversion_time_ms;
>  	unsigned int k;
> -	struct ads1015_data *data = i2c_get_clientdata(client);
>  	int res;
>  
>  	mutex_lock(&data->update_lock);
>  
> -	/* get fullscale voltage */
> +	/* get channel parameters */
>  	res = ads1015_read_reg(client, ADS1015_CONFIG);
>  	if (res < 0)
>  		goto err_unlock;
>  	config = res;
> -	pga = (config >> 9) & 0x0007;
>  	fullscale = fullscale_table[pga];
> +	conversion_time_ms = 1 + 1000 / data_rate_table[data_rate];

Maybe a suitable use case for DIV_ROUND_UP()?

>  
>  	/* set channel and start single conversion */

You don't only set the channel and start the conversion. You also set
the PGA and data rate.

> -	config &= ~(0x0007 << 12);
> -	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
> +	config &= 0x001f;
> +	config |= (1 << 15) | (1 << 8);
> +	config |= (channel & 0x0007) << 12;
> +	config |= (pga & 0x0007) << 9;
> +	config |= (data_rate & 0x0007) << 5;
>  
> -	/* wait until conversion finished */
>  	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
>  	if (res < 0)
>  		goto err_unlock;
> +
> +	/* wait until conversion finished */
>  	for (k = 0; k < 5; ++k) {
> -		schedule_timeout(msecs_to_jiffies(1));
> +		msleep(k ? 1 : conversion_time_ms);
>  		res = ads1015_read_reg(client, ADS1015_CONFIG);
>  		if (res < 0)
>  			goto err_unlock;

This could be done more efficiently: msleep(conversion_time_ms) before
the loop, and msleep(1) inside the loop (after reading the register.)

That being said... As the conversion rate is known in advance, the
retry loop should never be needed, should it? Did you ever see it
trigger?

> @@ -166,29 +178,83 @@ static int ads1015_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
> +static void ads1015_get_exported_channels(struct i2c_client *client)
>  {
> +	unsigned int k;
> +	struct ads1015_data *data = i2c_get_clientdata(client);
>  	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
>  #ifdef CONFIG_OF
> -	struct device_node *np = client->dev.of_node;
> -	const __be32 *of_channels;
> -	int of_channels_size;
> +	struct device_node *node;
>  #endif
>  
>  	/* prefer platform data */
> -	if (pdata)
> -		return pdata->exported_channels;
> +	if (pdata) {
> +		memcpy(data->channel_data, pdata->channel_data,
> +		       sizeof(data->channel_data));
> +		return;
> +	}
>  
>  #ifdef CONFIG_OF
>  	/* fallback on OF */
> -	of_channels = of_get_property(np, "exported-channels",
> -				      &of_channels_size);
> -	if (of_channels && (of_channels_size == sizeof(*of_channels)))
> -		return be32_to_cpup(of_channels);
> +	if (!client->dev.of_node
> +	    || !of_get_next_child(client->dev.of_node, NULL))
> +		goto fallback_default;
> +
> +	for_each_child_of_node(client->dev.of_node, node) {
> +		const __be32 *property;
> +		int len;
> +		unsigned int channel;
> +		unsigned int pga = ADS1015_DEFAULT_PGA;
> +		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
> +
> +		property = of_get_property(node, "reg", &len);
> +		if (!property || (len < sizeof(int))) {

For consistency with the following tests, you should test for len !=
sizeof(int). Note that you can omit the parentheses around this test,
BTW, and same below.

> +			dev_err(&client->dev, "ads1015: invalid reg on %s\n",

The leading "ads1015:" in all error messages is redundant, as dev_err()
will include the client name already.

> +				node->full_name);
> +			continue;
> +		}
> +
> +		channel = be32_to_cpup(property);
> +		if (channel > ADS1015_CONFIG_CHANNELS) {
> +			dev_err(&client->dev, "ads1015: invalid addr=%x on %s\n",
> +				channel, node->full_name);

"addr" is misleading, it is a channel number (sort of, at least), not
an address. I'm sure you can come up with a more useful error message.

> +			continue;
> +		}
> +
> +		property = of_get_property(node, "ti,gain", &len);
> +		if (property && (len == sizeof(int))) {
> +			pga = be32_to_cpup(property);
> +			if (pga > 7) {
> +				dev_err(&client->dev,
> +					"ads1015: invalid gain on %s\n",
> +					node->full_name);
> +			}
> +		}
> +
> +		property = of_get_property(node, "ti,datarate", &len);
> +		if (property && (len == sizeof(int))) {
> +			data_rate = be32_to_cpup(property);
> +			if (data_rate > 7)

I count only 6 possible data rates.

> +				dev_err(&client->dev,
> +					"ads1015: invalid data_rate on %s\n",
> +					node->full_name);

Curly braces would be appreciated, for readability and consistency.

> +		}
> +
> +		data->channel_data[channel].enabled = true;
> +		data->channel_data[channel].pga = pga;
> +		data->channel_data[channel].data_rate = data_rate;
> +	}
> +
> +	return;
>  #endif
>  
>  	/* fallback on default configuration */
> -	return ADS1015_DEFAULT_CHANNELS;
> +fallback_default:

Use of goto and label is discouraged, except for error paths. If you
feel the need for it, it suggests that the devicetree parsing code
should be moved to a separate function. Maybe this would let you merge
the two #ifdef CONFIG_OF blocks, which would be nice, and also get rid
of the following warning I'm seeing:

drivers/hwmon/ads1015.c: In function ‘ads1015_get_exported_channels’:
drivers/hwmon/ads1015.c:252:1: warning: label ‘fallback_default’ defined but not used



> +	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> +		data->channel_data[k].enabled = true;
> +		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
> +		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
> +	}

As said earlier, I'm really not sure that this default makes any sense.

>  }  
>  static int ads1015_probe(struct i2c_client *client,
> @@ -196,7 +262,6 @@ static int ads1015_probe(struct i2c_client *client,
>  {
>  	struct ads1015_data *data;
>  	int err;
> -	unsigned int exported_channels;
>  	unsigned int k;
>  
>  	data = kzalloc(sizeof(struct ads1015_data), GFP_KERNEL);
> @@ -209,9 +274,9 @@ static int ads1015_probe(struct i2c_client *client,
>  	mutex_init(&data->update_lock);
>  
>  	/* build sysfs attribute group */
> -	exported_channels = ads1015_get_exported_channels(client);
> +	ads1015_get_exported_channels(client);

This function would rather be named ads1015_get_channels_config or
similar.

>  	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
> -		if (!(exported_channels & (1<<k)))
> +		if (!data->channel_data[k].enabled)
>  			continue;
>  		err = device_create_file(&client->dev, &ads1015_in[k].dev_attr);
>  		if (err)
> diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
> index 8541c6a..0d9e746 100644
> --- a/include/linux/i2c/ads1015.h
> +++ b/include/linux/i2c/ads1015.h
> @@ -21,8 +21,16 @@
>  #ifndef LINUX_ADS1015_H
>  #define LINUX_ADS1015_H
>  
> +#define ADS1015_CONFIG_CHANNELS 8

I can't make sense of the "CONFIG" part in the name.

> +
> +struct ads1015_channel_data {
> +	bool enabled;
> +	unsigned int pga;
> +	unsigned int data_rate;
> +};
> +
>  struct ads1015_platform_data {
> -	unsigned int exported_channels;
> +	struct ads1015_channel_data channel_data[ADS1015_CONFIG_CHANNELS];
>  };
>  
>  #endif /* LINUX_ADS1015_H */


-- 
Jean Delvare

RE: [PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Eibach, Dirk]

Hi Jean,

> Took me a little longer than I expected to review this, but here we go
> finally:

No problem, I appreciate your careful and detailed reviews very much.

> > +    1: FS = +/- 4.096 V
> > +    2: FS = +/- 2.048 V (default)
> > +    3: FS = +/- 1.024 V
> > +    4: FS = +/- 0.512 V
> > +    5: FS = +/- 0.256 V
> > + - data_rate in samples per second
> > +    0: 128
> > +    1: 250
> > +    2: 490
> > +    3: 920
> > +    4: 1600
> > +    5: 2400
> > +    6: 3300
> 
> These sample rates make me wonder if hwmon is really the 
> right subsystem for this device. hwmon is more like 2 samples 
> per second.

ADS1015 has a beautiful low energy single shot mode which makes it
pretty adorable for hwmon ;)
Sample rate adjustment is only for setting up the sigma delta
parameters.

> The duplicate documentation between 
> Documentation/hwmon/ads1015 and 
> Documentation/devicetree/bindings/hwmon/ads1015.txt is 
> somewhat unpleasant. It will be difficult to keep things in 
> sync over time. I wonder if this could be avoided.

Same here. But no ideas so far.

> >  Example:
> >  struct ads1015_platform_data data = {
> > -	.exported_channels = (1 << 2) | (1 << 4)
> > +	.channel_data = {
> > +		[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
> > +		[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
> > +	}
> >  };
> 
> This looks pretty good, I have to admit :)

Gorgeous ;)

> Does it really make sense to have a default configuration? 
> The chip needs proper configuration to be usable at all, I 
> would think it makes sense to make the platform or devicetree 
> data mandatory. I am particularly worried about the case 
> where the PGA is set improperly: the input voltage could be 
> over what the chip supports?

In fact it does. Default configuration reflects hardware defaults, which
hopefully reflect some default usecase.
The engineer that designed my board actually considered this, so no pga
adjustment is necessary.
Inappropriate PGA settingts can't damage the chip.

> Use of goto and label is discouraged, except for error paths. 

I am not proud of this, but I learned BASIC on a Commodore 116 ;)

Cheers
Dirk
--------------------------------------------------------------------------
Guntermann & Drunck GmbH Systementwicklung 
Dortmunder Str. 4a 
D-57234 Wilnsdorf - Germany 
Tel: +49 (0) 27 39 / 89 01 - 100  Fax: +49 (0) 27 39 / 89 01 - 120 
E-Mail: mailto:sales-dJ+jgKLZIg4@public.gmane.org Web: www.gdsys.de
--------------------------------------------------------------------------
Geschaeftsfuehrer: 
Udo Guntermann - Martin Drunck - Reiner Ruelmann - Klaus Tocke
HRB 2884, Amtsgericht Siegen - WEEE-Reg.-Nr. DE30763240
USt.-Id.-Nr. DE 126575222 - Steuer-Nr. 342 / 5835 / 1041
--------------------------------------------------------------------------
DQS-zertifiziert nach ISO 9001:2008
--------------------------------------------------------------------------

Re: [PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Jean Delvare]

On Mon, 21 Mar 2011 11:17:42 +0100, Eibach, Dirk wrote:
> > Use of goto and label is discouraged, except for error paths. 
> 
> I am not proud of this, but I learned BASIC on a Commodore 116 ;)

Oh dear, you must be even older than me then :p Locomotive BASIC on an
Amstrad CPC464 for me. I don't remember the exact date... must have
been in 1986.

-- 
Jean Delvare

Re: [PATCH v2] hwmon: (ads1015) Make gain and datarate configurable

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]


> > I am not proud of this, but I learned BASIC on a Commodore 116 ;)
> 
> Oh dear, you must be even older than me then :p Locomotive BASIC on an
> Amstrad CPC464 for me.

Both were released in 1984. (And the BASIC V3.5 of the 116 was quite
better than V2.0 of the Commodore 64 (released in 1982). Still BASIC,
though :)) SCNR, I love the 8-bit era...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Dirk Eibach]

Configuration for ads1015 gain and datarate is possible via
devicetree or platform data.

This is a followup patch to previous ads1015 patches on Jean Delvares
tree.

Changes since v1:
- replaced exported_channels bitmask with an enable entry per channel
- improved platform data example in Documentation
- improved patch description

Changes since v2:
- fixed spelling and whitespace in Documentation
- marked default data_rate
- replaced FS by full scale
- removed structure definition from Documentation
- used DIV_ROUND_UP()
- improved comments
- improved waiting for conversion
- moved devicetree stuff to ads1015_get_channels_config_of()
- renamed ads1015_get_exported_channels() to ads1015_get_channels_config()
- renamed ADS1015_CONFIG_CHANNELS to ADS1015_CHANNELS
- fixed some style inconsistencies

Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
 .../devicetree/bindings/hwmon/ads1015.txt          |   88 +++++++++---
 Documentation/hwmon/ads1015                        |   49 ++++---
 drivers/hwmon/ads1015.c                            |  151 ++++++++++++++------
 include/linux/i2c/ads1015.h                        |   10 ++-
 4 files changed, 212 insertions(+), 86 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt b/Documentation/devicetree/bindings/hwmon/ads1015.txt
index 985e24d..918a507 100644
--- a/Documentation/devicetree/bindings/hwmon/ads1015.txt
+++ b/Documentation/devicetree/bindings/hwmon/ads1015.txt
@@ -5,25 +5,69 @@ This device is a 12-bit A-D converter with 4 inputs.
 The inputs can be used single ended or in certain differential combinations.
 
 For configuration all possible combinations are mapped to 8 channels:
-0: Voltage over AIN0 and AIN1.
-1: Voltage over AIN0 and AIN3.
-2: Voltage over AIN1 and AIN3.
-3: Voltage over AIN2 and AIN3.
-4: Voltage over AIN0 and GND.
-5: Voltage over AIN1 and GND.
-6: Voltage over AIN2 and GND.
-7: Voltage over AIN3 and GND.
-
-Optional properties:
-
- - exported-channels : exported_channels is a bitmask that specifies which
-		       channels should be accessible by the user.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = <0x14>;
-};
-
-In this example only channel 2 and 4 would be accessible by the user.
+  0: Voltage over AIN0 and AIN1.
+  1: Voltage over AIN0 and AIN3.
+  2: Voltage over AIN1 and AIN3.
+  3: Voltage over AIN2 and AIN3.
+  4: Voltage over AIN0 and GND.
+  5: Voltage over AIN1 and GND.
+  6: Voltage over AIN2 and GND.
+  7: Voltage over AIN3 and GND.
+
+Each channel can be configured individually:
+ - pga is the programmable gain amplifier (values are full scale)
+    0: +/- 6.144 V
+    1: +/- 4.096 V
+    2: +/- 2.048 V (default)
+    3: +/- 1.024 V
+    4: +/- 0.512 V
+    5: +/- 0.256 V
+ - data_rate in samples per second
+    0: 128
+    1: 250
+    2: 490
+    3: 920
+    4: 1600 (default)
+    5: 2400
+    6: 3300
+
+1) The /ads1015 node
+
+  Required properties:
+
+   - compatible : must be "ti,ads1015"
+   - reg : I2C bus address of the device
+   - #address-cells : must be <1>
+   - #size-cells : must be <0>
+
+  The node contains child nodes for each channel that the platform uses.
+
+  Example ADS1015 node:
+
+    ads1015@49 {
+	    compatible = "ti,ads1015";
+	    reg = <0x49>;
+	    #address-cells = <1>;
+	    #size-cells = <0>;
+
+	    [ child node definitions... ]
+    }
+
+2) channel nodes
+
+  Required properties:
+
+   - reg : the channel number
+
+  Optional properties:
+
+   - ti,gain : the programmable gain amplifier setting
+   - ti,datarate : the converter data rate
+
+  Example ADS1015 channel node:
+
+    channel@4 {
+	    reg = <4>;
+	    ti,gain = <3>;
+	    ti,datarate = <5>;
+    };
diff --git a/Documentation/hwmon/ads1015 b/Documentation/hwmon/ads1015
index 56ee797..f6fe9c2 100644
--- a/Documentation/hwmon/ads1015
+++ b/Documentation/hwmon/ads1015
@@ -19,7 +19,7 @@ This device is a 12-bit A-D converter with 4 inputs.
 
 The inputs can be used single ended or in certain differential combinations.
 
-The inputs can be exported to 8 sysfs input files in0_input - in7_input:
+The inputs can be made available by 8 sysfs input files in0_input - in7_input:
 in0: Voltage over AIN0 and AIN1.
 in1: Voltage over AIN0 and AIN3.
 in2: Voltage over AIN1 and AIN3.
@@ -29,39 +29,44 @@ in5: Voltage over AIN1 and GND.
 in6: Voltage over AIN2 and GND.
 in7: Voltage over AIN3 and GND.
 
-Which inputs are exported can be configured using platform data or devicetree.
+Which inputs are available can be configured using platform data or devicetree.
 
 By default all inputs are exported.
 
 Platform Data
 -------------
 
-In linux/i2c/ads1015.h platform data is defined as:
-
-struct ads1015_platform_data {
-	unsigned int exported_channels;
-};
-
-exported_channels is a bitmask that specifies which inputs should be exported.
+In linux/i2c/ads1015.h platform data is defined, channel_data contains
+configuration data for the used input combinations:
+- pga is the programmable gain amplifier (values are full scale)
+  0: +/- 6.144 V
+  1: +/- 4.096 V
+  2: +/- 2.048 V
+  3: +/- 1.024 V
+  4: +/- 0.512 V
+  5: +/- 0.256 V
+- data_rate in samples per second
+  0: 128
+  1: 250
+  2: 490
+  3: 920
+  4: 1600
+  5: 2400
+  6: 3300
 
 Example:
 struct ads1015_platform_data data = {
-	.exported_channels = (1 << 2) | (1 << 4)
+	.channel_data = {
+		[2] = { .enabled = true, .pga = 1, .data_rate = 0 },
+		[4] = { .enabled = true, .pga = 4, .data_rate = 5 },
+	}
 };
 
-In this case only in2_input and in4_input would be created.
+In this case only in2_input (FS +/- 4.096 V, 128 SPS) and in4_input
+(FS +/- 0.512 V, 2400 SPS) would be created.
 
 Devicetree
 ----------
 
-The ads1015 node may have an "exported-channels" property.
-exported_channels is a bitmask that specifies which inputs should be exported.
-
-Example:
-ads1015@49 {
-	compatible = "ti,ads1015";
-	reg = <0x49>;
-	exported-channels = < 0x14 >;
-};
-
-In this case only in2_input and in4_input would be created.
+Configuration is also possible via devicetree:
+Documentation/devicetree/bindings/hwmon/ads1015.txt
diff --git a/drivers/hwmon/ads1015.c b/drivers/hwmon/ads1015.c
index 8ef2b60..e9beeda 100644
--- a/drivers/hwmon/ads1015.c
+++ b/drivers/hwmon/ads1015.c
@@ -25,7 +25,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
-#include <linux/jiffies.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -45,12 +45,18 @@ enum {
 static const unsigned int fullscale_table[8] = {
 	6144, 4096, 2048, 1024, 512, 256, 256, 256 };
 
-#define ADS1015_CONFIG_CHANNELS 8
+/* Data rates in samples per second */
+static const unsigned int data_rate_table[8] = {
+	128, 250, 490, 920, 1600, 2400, 3300, 3300 };
+
 #define ADS1015_DEFAULT_CHANNELS 0xff
+#define ADS1015_DEFAULT_PGA 2
+#define ADS1015_DEFAULT_DATA_RATE 4
 
 struct ads1015_data {
 	struct device *hwmon_dev;
 	struct mutex update_lock; /* mutex protect updates */
+	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 };
 
 static s32 ads1015_read_reg(struct i2c_client *client, unsigned int reg)
@@ -71,40 +77,42 @@ static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
 {
 	u16 config;
 	s16 conversion;
-	unsigned int pga;
-	int fullscale;
-	unsigned int k;
 	struct ads1015_data *data = i2c_get_clientdata(client);
+	unsigned int pga = data->channel_data[channel].pga;
+	int fullscale;
+	unsigned int data_rate = data->channel_data[channel].data_rate;
+	unsigned int conversion_time_ms;
 	int res;
 
 	mutex_lock(&data->update_lock);
 
-	/* get fullscale voltage */
+	/* get channel parameters */
 	res = ads1015_read_reg(client, ADS1015_CONFIG);
 	if (res < 0)
 		goto err_unlock;
 	config = res;
-	pga = (config >> 9) & 0x0007;
 	fullscale = fullscale_table[pga];
+	conversion_time_ms = DIV_ROUND_UP(1000, data_rate_table[data_rate]);
 
-	/* set channel and start single conversion */
-	config &= ~(0x0007 << 12);
-	config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
+	/* setup and start single conversion */
+	config &= 0x001f;
+	config |= (1 << 15) | (1 << 8);
+	config |= (channel & 0x0007) << 12;
+	config |= (pga & 0x0007) << 9;
+	config |= (data_rate & 0x0007) << 5;
 
-	/* wait until conversion finished */
 	res = ads1015_write_reg(client, ADS1015_CONFIG, config);
 	if (res < 0)
 		goto err_unlock;
-	for (k = 0; k < 5; ++k) {
-		schedule_timeout(msecs_to_jiffies(1));
-		res = ads1015_read_reg(client, ADS1015_CONFIG);
-		if (res < 0)
-			goto err_unlock;
-		config = res;
-		if (config & (1 << 15))
-			break;
-	}
-	if (k == 5) {
+
+	/* wait until conversion finished */
+	msleep(conversion_time_ms);
+	res = ads1015_read_reg(client, ADS1015_CONFIG);
+	if (res < 0)
+		goto err_unlock;
+	config = res;
+	if (!(config & (1 << 15))) {
+		/* conversion not finished in time */
 		res = -EIO;
 		goto err_unlock;
 	}
@@ -160,35 +168,97 @@ static int ads1015_remove(struct i2c_client *client)
 	int k;
 
 	hwmon_device_unregister(data->hwmon_dev);
-	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k)
+	for (k = 0; k < ADS1015_CHANNELS; ++k)
 		device_remove_file(&client->dev, &ads1015_in[k].dev_attr);
 	kfree(data);
 	return 0;
 }
 
-static unsigned int ads1015_get_exported_channels(struct i2c_client *client)
-{
-	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
 #ifdef CONFIG_OF
-	struct device_node *np = client->dev.of_node;
-	const __be32 *of_channels;
-	int of_channels_size;
+static int ads1015_get_channels_config_of(struct i2c_client *client)
+{
+	struct ads1015_data *data = i2c_get_clientdata(client);
+	struct device_node *node;
+
+	if (!client->dev.of_node
+	    || !of_get_next_child(client->dev.of_node, NULL))
+		return -EINVAL;
+
+	for_each_child_of_node(client->dev.of_node, node) {
+		const __be32 *property;
+		int len;
+		unsigned int channel;
+		unsigned int pga = ADS1015_DEFAULT_PGA;
+		unsigned int data_rate = ADS1015_DEFAULT_DATA_RATE;
+
+		property = of_get_property(node, "reg", &len);
+		if (!property || len != sizeof(int)) {
+			dev_err(&client->dev, "invalid reg on %s\n",
+				node->full_name);
+			continue;
+		}
+
+		channel = be32_to_cpup(property);
+		if (channel > ADS1015_CHANNELS) {
+			dev_err(&client->dev,
+				"invalid channel index %d on %s\n",
+				channel, node->full_name);
+			continue;
+		}
+
+		property = of_get_property(node, "ti,gain", &len);
+		if (property && len == sizeof(int)) {
+			pga = be32_to_cpup(property);
+			if (pga > 6) {
+				dev_err(&client->dev,
+					"invalid gain on %s\n",
+					node->full_name);
+			}
+		}
+
+		property = of_get_property(node, "ti,datarate", &len);
+		if (property && len == sizeof(int)) {
+			data_rate = be32_to_cpup(property);
+			if (data_rate > 7) {
+				dev_err(&client->dev,
+					"invalid data_rate on %s\n",
+					node->full_name);
+			}
+		}
+
+		data->channel_data[channel].enabled = true;
+		data->channel_data[channel].pga = pga;
+		data->channel_data[channel].data_rate = data_rate;
+	}
+
+	return 0;
+}
 #endif
 
+static void ads1015_get_channels_config(struct i2c_client *client)
+{
+	unsigned int k;
+	struct ads1015_data *data = i2c_get_clientdata(client);
+	struct ads1015_platform_data *pdata = dev_get_platdata(&client->dev);
+
 	/* prefer platform data */
-	if (pdata)
-		return pdata->exported_channels;
+	if (pdata) {
+		memcpy(data->channel_data, pdata->channel_data,
+		       sizeof(data->channel_data));
+		return;
+	}
 
 #ifdef CONFIG_OF
-	/* fallback on OF */
-	of_channels = of_get_property(np, "exported-channels",
-				      &of_channels_size);
-	if (of_channels && (of_channels_size == sizeof(*of_channels)))
-		return be32_to_cpup(of_channels);
+	if (!ads1015_get_channels_config_of(client))
+		return;
 #endif
 
 	/* fallback on default configuration */
-	return ADS1015_DEFAULT_CHANNELS;
+	for (k = 0; k < ADS1015_CHANNELS; ++k) {
+		data->channel_data[k].enabled = true;
+		data->channel_data[k].pga = ADS1015_DEFAULT_PGA;
+		data->channel_data[k].data_rate = ADS1015_DEFAULT_DATA_RATE;
+	}
 }
 
 static int ads1015_probe(struct i2c_client *client,
@@ -196,7 +266,6 @@ static int ads1015_probe(struct i2c_client *client,
 {
 	struct ads1015_data *data;
 	int err;
-	unsigned int exported_channels;
 	unsigned int k;
 
 	data = kzalloc(sizeof(struct ads1015_data), GFP_KERNEL);
@@ -209,9 +278,9 @@ static int ads1015_probe(struct i2c_client *client,
 	mutex_init(&data->update_lock);
 
 	/* build sysfs attribute group */
-	exported_channels = ads1015_get_exported_channels(client);
-	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k) {
-		if (!(exported_channels & (1<<k)))
+	ads1015_get_channels_config(client);
+	for (k = 0; k < ADS1015_CHANNELS; ++k) {
+		if (!data->channel_data[k].enabled)
 			continue;
 		err = device_create_file(&client->dev, &ads1015_in[k].dev_attr);
 		if (err)
@@ -227,7 +296,7 @@ static int ads1015_probe(struct i2c_client *client,
 	return 0;
 
 exit_remove:
-	for (k = 0; k < ADS1015_CONFIG_CHANNELS; ++k)
+	for (k = 0; k < ADS1015_CHANNELS; ++k)
 		device_remove_file(&client->dev, &ads1015_in[k].dev_attr);
 exit_free:
 	kfree(data);
diff --git a/include/linux/i2c/ads1015.h b/include/linux/i2c/ads1015.h
index 8541c6a..d5aa2a0 100644
--- a/include/linux/i2c/ads1015.h
+++ b/include/linux/i2c/ads1015.h
@@ -21,8 +21,16 @@
 #ifndef LINUX_ADS1015_H
 #define LINUX_ADS1015_H
 
+#define ADS1015_CHANNELS 8
+
+struct ads1015_channel_data {
+	bool enabled;
+	unsigned int pga;
+	unsigned int data_rate;
+};
+
 struct ads1015_platform_data {
-	unsigned int exported_channels;
+	struct ads1015_channel_data channel_data[ADS1015_CHANNELS];
 };
 
 #endif /* LINUX_ADS1015_H */
-- 
1.5.6.5

Re: [PATCH] hwmon: (ads1015) Make gain and datarate configurable

[Jean Delvare]

On Mon, 21 Mar 2011 11:37:45 +0100, Dirk Eibach wrote:
> Configuration for ads1015 gain and datarate is possible via
> devicetree or platform data.
> 
> This is a followup patch to previous ads1015 patches on Jean Delvares
> tree.
> 
> Changes since v1:
> - replaced exported_channels bitmask with an enable entry per channel
> - improved platform data example in Documentation
> - improved patch description
> 
> Changes since v2:
> - fixed spelling and whitespace in Documentation
> - marked default data_rate
> - replaced FS by full scale
> - removed structure definition from Documentation
> - used DIV_ROUND_UP()
> - improved comments
> - improved waiting for conversion
> - moved devicetree stuff to ads1015_get_channels_config_of()
> - renamed ads1015_get_exported_channels() to ads1015_get_channels_config()
> - renamed ADS1015_CONFIG_CHANNELS to ADS1015_CHANNELS
> - fixed some style inconsistencies
> 
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
>  .../devicetree/bindings/hwmon/ads1015.txt          |   88 +++++++++---
>  Documentation/hwmon/ads1015                        |   49 ++++---
>  drivers/hwmon/ads1015.c                            |  151 ++++++++++++++------
>  include/linux/i2c/ads1015.h                        |   10 ++-
>  4 files changed, 212 insertions(+), 86 deletions(-)

Applied, thanks.

I'll send all my hwmon patches to Linus later today.

-- 
Jean Delvare