[PATCH] hwmon: (ads1015) Add devicetree documentation

[PATCH] hwmon: (ads1015) Add devicetree documentation

[Dirk Eibach]

Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
 Documentation/devicetree/bindings/i2c/ads1015.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt

diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
new file mode 100644
index 0000000..3a7d67a
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/ads1015.txt
@@ -0,0 +1,15 @@
+ADS1015 (I2C)
+
+Optional properties:
+
+ - exported-channels : exported_channels is a bitmask that specifies which
+		       inputs should be exported to sysfs.
+
+Example:
+ads1015@49 {
+	compatible = "ti,ads1015";
+	reg = <0x49>;
+	exported-channels = <0x14>;
+};
+
+In this example only in2_input and in4_input would be created.
-- 
1.5.6.5

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Thu, Mar 03, 2011 at 10:16:45AM +0100, Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
>  Documentation/devicetree/bindings/i2c/ads1015.txt |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> new file mode 100644
> index 0000000..3a7d67a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/ads1015.txt
> @@ -0,0 +1,15 @@
> +ADS1015 (I2C)
> +
> +Optional properties:
> +
> + - exported-channels : exported_channels is a bitmask that specifies which
> +		       inputs should be exported to sysfs.

Hmm, device tree bindings should be OS-neutral, sysfs is not. Maybe
"active-channels" would be better; then the OS could decide what to do
with the active channels. Then again, what is the drawback of exporting
all channels? Is there another hwmon-driver doing so (couldn't find
one)?

Regards,

   Wolfram

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

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

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Jean Delvare]

Hi Wolfram,

On Thu, 3 Mar 2011 12:51:51 +0100, Wolfram Sang wrote:
> On Thu, Mar 03, 2011 at 10:16:45AM +0100, Dirk Eibach wrote:
> > Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> > ---
> >  Documentation/devicetree/bindings/i2c/ads1015.txt |   15 +++++++++++++++
> >  1 files changed, 15 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> > new file mode 100644
> > index 0000000..3a7d67a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/ads1015.txt
> > @@ -0,0 +1,15 @@
> > +ADS1015 (I2C)
> > +
> > +Optional properties:
> > +
> > + - exported-channels : exported_channels is a bitmask that specifies which
> > +		       inputs should be exported to sysfs.
> 
> Hmm, device tree bindings should be OS-neutral, sysfs is not.

Why do we document this in the Linux kernel tree then?

> Maybe
> "active-channels" would be better; then the OS could decide what to do
> with the active channels. Then again, what is the drawback of exporting
> all channels?

Performance and user-friendliness. libsensors-based applications will
read all available attributes by default, and each reading takes time.
Letting the platform declare how the inputs are used allows for a sane
output for "sensors" and other similar tools out of the box, without
the user having to tinker with ignore statements in configuration files
to discard the nonsensical values.

> Is there another hwmon-driver doing so (couldn't find one)?

If "doing so" means "letting the user define how the ADC inputs are
used", then yes, the pcf8591 driver does something similar, except that
it uses a module parameter for the setting, for historical reasons.
Platform-provided, per-device data is better in my opinion.

-- 
Jean Delvare

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

> > Hmm, device tree bindings should be OS-neutral, sysfs is not.
> 
> Why do we document this in the Linux kernel tree then?

To describe which bindings Linux supports.

> > with the active channels. Then again, what is the drawback of exporting
> > all channels?
> 
> Performance and user-friendliness. libsensors-based applications will
> read all available attributes by default, and each reading takes time.
> Letting the platform declare how the inputs are used allows for a sane
> output for "sensors" and other similar tools out of the box, without
> the user having to tinker with ignore statements in configuration files
> to discard the nonsensical values.

OK, that's fine I'd say.

> > Is there another hwmon-driver doing so (couldn't find one)?
> 
> If "doing so" means "letting the user define how the ADC inputs are
> used", then yes, the pcf8591 driver does something similar, except that
> it uses a module parameter for the setting, for historical reasons.
> Platform-provided, per-device data is better in my opinion.

OK. The thing is you can't map platform_data 1:1 to bindings, because
most are very specific to the Linux-driver. Do you think something like
"active-channels" would be sufficent for those other hwmon devices, too?
(I still do not like "exported-channels", because there is no need to
export the channels for the OS. The devicetree is primarily a hardware
description language) Or maybe we go specific and say "ads1015,channel1
= 1"? Maybe somebody knows of a similar chips as a reference?

Regards,

   Wolfram

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

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

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Grant Likely]

On Thu, Mar 03, 2011 at 02:25:49PM +0100, Wolfram Sang wrote:
> > > Hmm, device tree bindings should be OS-neutral, sysfs is not.
> > 
> > Why do we document this in the Linux kernel tree then?
> 
> To describe which bindings Linux supports.
> 
> > > with the active channels. Then again, what is the drawback of exporting
> > > all channels?
> > 
> > Performance and user-friendliness. libsensors-based applications will
> > read all available attributes by default, and each reading takes time.
> > Letting the platform declare how the inputs are used allows for a sane
> > output for "sensors" and other similar tools out of the box, without
> > the user having to tinker with ignore statements in configuration files
> > to discard the nonsensical values.
> 
> OK, that's fine I'd say.
> 
> > > Is there another hwmon-driver doing so (couldn't find one)?
> > 
> > If "doing so" means "letting the user define how the ADC inputs are
> > used", then yes, the pcf8591 driver does something similar, except that
> > it uses a module parameter for the setting, for historical reasons.
> > Platform-provided, per-device data is better in my opinion.
> 
> OK. The thing is you can't map platform_data 1:1 to bindings, because
> most are very specific to the Linux-driver. Do you think something like
> "active-channels" would be sufficent for those other hwmon devices, too?
> (I still do not like "exported-channels", because there is no need to
> export the channels for the OS. The devicetree is primarily a hardware
> description language) Or maybe we go specific and say "ads1015,channel1
> = 1"? Maybe somebody knows of a similar chips as a reference?

Yes, Wolfram's correct.  The focus should be on what the connections
actually are instead of what the OS should attempt to do with them.
ie. give the active channels actual names for what they do, or use a
phandle to reference them from another node.  The driver can then make
a decision based on whether or not a channel has a configuration
provided.

>From the little information I have, I'd recommend something like:

sensor@49 {
	compatible = "ti,ads1015"
	reg = <0x49>;

	// Each child node (one node per channel) has an address with
	// no range
	#address-cells = <1>;
	#size-cells = <0>;

	adc@2 {
		ti,measurement = "cpu voltage";
		reg = <2>;
	};

	adc@4 {
		ti,measurement = "base voltage";
		reg = <4>;
	};
};

However, after taking a little look at the data sheet, this binding
might end up being a little naive for what the part can do.  It might
make more sense to do something like:

sensor@49 {
	compatible = "ti,ads1015"
	reg = <0x49>;

	// Each child node (one node per channel) has an address with
	// no range
	#address-cells = <1>;
	#size-cells = <0>;

	measurement@0 {
		reg = <0>;	// This reg value no longer reflects
				// the chip, it's just a handle for
				// logical measurement channels
		ti,measurement = "cpu voltage";
		ti,multiplexer = <2>; // AIN0 vs. AIN3
		ti,gain = <5>; // +/-0.256V
		ti,inverse-polarity;
	};

	measurement@1 {
		reg = <1>;
		ti,measurement = "base voltage";
		ti,multiplexer = <4>; // AIN0 vs. GND
		ti,gain = <2>; // +/-2.048V
	};
};

...which splits it up by logical measurement configurations instead of
only the multiplexer setting.

It's more verbose than a single exported-channels property, but it is
flexible enough that it can be easily extended with extra
configuration data per channel.

g.

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Jean Delvare]

On Thu, 3 Mar 2011 10:26:53 -0700, Grant Likely wrote:
> On Thu, Mar 03, 2011 at 02:25:49PM +0100, Wolfram Sang wrote:
> > OK. The thing is you can't map platform_data 1:1 to bindings, because
> > most are very specific to the Linux-driver. Do you think something like
> > "active-channels" would be sufficent for those other hwmon devices, too?
> > (I still do not like "exported-channels", because there is no need to
> > export the channels for the OS. The devicetree is primarily a hardware
> > description language) Or maybe we go specific and say "ads1015,channel1
> > = 1"? Maybe somebody knows of a similar chips as a reference?
> 
> Yes, Wolfram's correct.  The focus should be on what the connections
> actually are instead of what the OS should attempt to do with them.
> ie. give the active channels actual names for what they do, or use a
> phandle to reference them from another node.  The driver can then make
> a decision based on whether or not a channel has a configuration
> provided.
> 
> From the little information I have, I'd recommend something like:
> 
> sensor@49 {
> 	compatible = "ti,ads1015"
> 	reg = <0x49>;
> 
> 	// Each child node (one node per channel) has an address with
> 	// no range
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	adc@2 {
> 		ti,measurement = "cpu voltage";
> 		reg = <2>;
> 	};
> 
> 	adc@4 {
> 		ti,measurement = "base voltage";
> 		reg = <4>;
> 	};
> };
> 
> However, after taking a little look at the data sheet, this binding
> might end up being a little naive for what the part can do.  It might
> make more sense to do something like:
> 
> sensor@49 {
> 	compatible = "ti,ads1015"
> 	reg = <0x49>;
> 
> 	// Each child node (one node per channel) has an address with
> 	// no range
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	measurement@0 {
> 		reg = <0>;	// This reg value no longer reflects
> 				// the chip, it's just a handle for
> 				// logical measurement channels

If there any hard requirement for this? From the driver's perspective,
the multiplexer setting can be used as the channel ID, and it's easier
this way than using arbitrary identifiers.

> 		ti,measurement = "cpu voltage";
> 		ti,multiplexer = <2>; // AIN0 vs. AIN3
> 		ti,gain = <5>; // +/-0.256V
> 		ti,inverse-polarity;
> 	};
> 
> 	measurement@1 {
> 		reg = <1>;
> 		ti,measurement = "base voltage";
> 		ti,multiplexer = <4>; // AIN0 vs. GND
> 		ti,gain = <2>; // +/-2.048V
> 	};
> };
> 
> ...which splits it up by logical measurement configurations instead of
> only the multiplexer setting.

I wouldn't disagree, as I did suggest making the gain a per-channel
setting while reviewing Dirk's driver. I had not thought of labelling,
but the hwmon sysfs API supports this too, so it would fit nicely
(although it can also be handled in user-space if needed.)

> It's more verbose than a single exported-channels property, but it is
> flexible enough that it can be easily extended with extra
> configuration data per channel.

I will leave it up to Dirk to decide how much time he wants to spend on
this. He has already been very patient with the driver review process
and I do not want to abuse his patience.

-- 
Jean Delvare

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Grant Likely]

On Thu, Mar 3, 2011 at 3:02 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Thu, 3 Mar 2011 10:26:53 -0700, Grant Likely wrote:
>> On Thu, Mar 03, 2011 at 02:25:49PM +0100, Wolfram Sang wrote:
>> > OK. The thing is you can't map platform_data 1:1 to bindings, because
>> > most are very specific to the Linux-driver. Do you think something like
>> > "active-channels" would be sufficent for those other hwmon devices, too?
>> > (I still do not like "exported-channels", because there is no need to
>> > export the channels for the OS. The devicetree is primarily a hardware
>> > description language) Or maybe we go specific and say "ads1015,channel1
>> > = 1"? Maybe somebody knows of a similar chips as a reference?
>>
>> Yes, Wolfram's correct.  The focus should be on what the connections
>> actually are instead of what the OS should attempt to do with them.
>> ie. give the active channels actual names for what they do, or use a
>> phandle to reference them from another node.  The driver can then make
>> a decision based on whether or not a channel has a configuration
>> provided.
>>
>> From the little information I have, I'd recommend something like:
>>
>> sensor@49 {
>>       compatible = "ti,ads1015"
>>       reg = <0x49>;
>>
>>       // Each child node (one node per channel) has an address with
>>       // no range
>>       #address-cells = <1>;
>>       #size-cells = <0>;
>>
>>       adc@2 {
>>               ti,measurement = "cpu voltage";
>>               reg = <2>;
>>       };
>>
>>       adc@4 {
>>               ti,measurement = "base voltage";
>>               reg = <4>;
>>       };
>> };
>>
>> However, after taking a little look at the data sheet, this binding
>> might end up being a little naive for what the part can do.  It might
>> make more sense to do something like:
>>
>> sensor@49 {
>>       compatible = "ti,ads1015"
>>       reg = <0x49>;
>>
>>       // Each child node (one node per channel) has an address with
>>       // no range
>>       #address-cells = <1>;
>>       #size-cells = <0>;
>>
>>       measurement@0 {
>>               reg = <0>;      // This reg value no longer reflects
>>                               // the chip, it's just a handle for
>>                               // logical measurement channels
>
> If there any hard requirement for this? From the driver's perspective,
> the multiplexer setting can be used as the channel ID, and it's easier
> this way than using arbitrary identifiers.

Nope, not a hard requirement.  Just a suggestion.  It would be
valuable if (and only if) there was ever reason to have more than one
configuration for a particular multiplexer setting.

>> It's more verbose than a single exported-channels property, but it is
>> flexible enough that it can be easily extended with extra
>> configuration data per channel.
>
> I will leave it up to Dirk to decide how much time he wants to spend on
> this. He has already been very patient with the driver review process
> and I do not want to abuse his patience.

BTW Dirk, I'm happy to help show how to write the parser code for the
sub-node approach if you need it.

g.

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Jean Delvare]

On Thu, 3 Mar 2011 15:09:28 -0700, Grant Likely wrote:
> On Thu, Mar 3, 2011 at 3:02 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > I will leave it up to Dirk to decide how much time he wants to spend on
> > this. He has already been very patient with the driver review process
> > and I do not want to abuse his patience.
> 
> BTW Dirk, I'm happy to help show how to write the parser code for the
> sub-node approach if you need it.

Grant, Wolfram, how are such patches committed? Documentation/devicetree/
isn't listed in MAINTAINERS. Is it OK for me to commit it once Dirk gets
your approval? Or do you prefer to commit it yourself?

-- 
Jean Delvare

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Grant Likely]

On Thu, Mar 17, 2011 at 11:25:30AM +0100, Jean Delvare wrote:
> On Thu, 3 Mar 2011 15:09:28 -0700, Grant Likely wrote:
> > On Thu, Mar 3, 2011 at 3:02 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > > I will leave it up to Dirk to decide how much time he wants to spend on
> > > this. He has already been very patient with the driver review process
> > > and I do not want to abuse his patience.
> > 
> > BTW Dirk, I'm happy to help show how to write the parser code for the
> > sub-node approach if you need it.
> 
> Grant, Wolfram, how are such patches committed? Documentation/devicetree/
> isn't listed in MAINTAINERS. Is it OK for me to commit it once Dirk gets
> your approval? Or do you prefer to commit it yourself?

Yup.  It makes sense for the documentation to be committed along side
the driver changes.  I have no problem with you taking this patch.

g.

Re: [PATCH] hwmon: (ads1015) Add devicetree documentation

[Grant Likely]

On Thu, Mar 03, 2011 at 01:20:25PM +0100, Jean Delvare wrote:
> Hi Wolfram,
> 
> On Thu, 3 Mar 2011 12:51:51 +0100, Wolfram Sang wrote:
> > On Thu, Mar 03, 2011 at 10:16:45AM +0100, Dirk Eibach wrote:
> > > Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> > > ---
> > >  Documentation/devicetree/bindings/i2c/ads1015.txt |   15 +++++++++++++++
> > >  1 files changed, 15 insertions(+), 0 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
> > > new file mode 100644
> > > index 0000000..3a7d67a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/i2c/ads1015.txt
> > > @@ -0,0 +1,15 @@
> > > +ADS1015 (I2C)
> > > +
> > > +Optional properties:
> > > +
> > > + - exported-channels : exported_channels is a bitmask that specifies which
> > > +		       inputs should be exported to sysfs.
> > 
> > Hmm, device tree bindings should be OS-neutral, sysfs is not.
> 
> Why do we document this in the Linux kernel tree then?

Lack of a better place and process for documenting and reviewing
bindings.  I've been toying with moving device tree bindings out to
devicetree.org, but I don't have a good review model for that.  I
might end up splitting it out into a separate git repository.

g.

[PATCH v3] hwmon: (ads1015) Add devicetree documentation

[Dirk Eibach]

Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
Changes since v1:
- removed sysfs from description
- explain channels 

Changes since v2:
- Replaced 'accessable' by 'accessible'.

 Documentation/devicetree/bindings/i2c/ads1015.txt |   29 +++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/ads1015.txt

diff --git a/Documentation/devicetree/bindings/i2c/ads1015.txt b/Documentation/devicetree/bindings/i2c/ads1015.txt
new file mode 100644
index 0000000..985e24d
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/ads1015.txt
@@ -0,0 +1,29 @@
+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.
-- 
1.5.6.5