[RFC] adding hwmon support to sequoia device tree

Hi,

I added support for the Sequoia on-board I2C temperature sensor to the device.
I am not sure if there is any node naming convention for such devices.
The needed I2C driver can be found under drivers/hwmon in the kernel sources,
so I found "hwmon" suitable for the node.

Do we want to add this to the sequoia dts file? If this is how it should be done,
I will commit a proper patch.

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/sequoia.dts
index 72d15f0..82fdfdf 100644
--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,23 @@
                        };

                        IIC0: i2c@ef600700 {
+                               #address-cells = <1>;
+                               #size-cells = <0>;
                                compatible = "ibm,iic-440epx", "ibm,iic";
                                reg = <0xef600700 0x00000014>;
                                interrupt-parent = <&UIC0>;
                                interrupts = <0x2 0x4>;
+
+                               hwmon@48 {
+                                       device_type = "hwmon";
+                                       compatible = "analog,ad7414";
+                                       reg = <0x48>;
+                               };
                        };

                        IIC1: i2c@ef600800 {
+                               #address-cells = <1>;
+                               #size-cells = <0>;

(prepare for adding devices on the 2nd I2C also)

                                compatible = "ibm,iic-440epx", "ibm,iic";
                                reg = <0xef600800 0x00000014>;
                                interrupt-parent = <&UIC0>;


Matthias

On Thu, Sep 04, 2008 at 11:55:02AM +0200, Matthias Fuchs wrote:

Hi,

I added support for the Sequoia on-board I2C temperature sensor to the device.
I am not sure if there is any node naming convention for such devices.
The needed I2C driver can be found under drivers/hwmon in the kernel sources,
so I found "hwmon" suitable for the node.

Do we want to add this to the sequoia dts file? If this is how it should be done,
I will commit a proper patch.

See comments below.  Out of curiosity, do you have a working driver and
setup with this?

We don't need device_type.  Particularly not a new one like this.

+                                       compatible = "analog,ad7414";

Perhaps 'compatible = "analog,ad7417", "amcc,hwmon-440epx";'

josh

On Thursday 04 September 2008, Josh Boyer wrote:

quoted

Do we want to add this to the sequoia dts file? If this is how it should
be done, I will commit a proper patch.

See comments below.  Out of curiosity, do you have a working driver and
setup with this?

Sure, it did hit mainline with 2.6.27:

drivers/hwmon/ad7414.c

quoted

diff --git a/arch/powerpc/boot/dts/sequoia.dts

b/arch/powerpc/boot/dts/sequoia.dts index 72d15f0..82fdfdf 100644

--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,23 @@
                       };

                       IIC0: i2c@ef600700 {
+                               #address-cells = <1>;
+                               #size-cells = <0>;
                               compatible = "ibm,iic-440epx", "ibm,iic";
                               reg = <0xef600700 0x00000014>;
                               interrupt-parent = <&UIC0>;
                               interrupts = <0x2 0x4>;
+
+                               hwmon@48 {
+                                       device_type = "hwmon";

We don't need device_type.  Particularly not a new one like this.

quoted

+                                       compatible = "analog,ad7414";

Perhaps 'compatible = "analog,ad7417", "amcc,hwmon-440epx";'

I don't think we need any amcc and/or 440epx here. Its a common chip from 
Analog.

Best regards,
Stefan

Hi,=20

On Thursday 04 September 2008 14:25, Josh Boyer wrote:

On Thu, Sep 04, 2008 at 11:55:02AM +0200, Matthias Fuchs wrote:

quoted

Hi,

I added support for the Sequoia on-board I2C temperature sensor to the d=

evice.

quoted

I am not sure if there is any node naming convention for such devices.
The needed I2C driver can be found under drivers/hwmon in the kernel sou=

rces,

quoted

so I found "hwmon" suitable for the node.

Do we want to add this to the sequoia dts file? If this is how it should=

 be done,

quoted

I will commit a proper patch.

=20
See comments below.  Out of curiosity, do you have a working driver and
setup with this?

Of course.=20

You need
	CONFIG_HWMON=3Dy
	CONFIG_SENSORS_AD7414=3Dy

Via sysfs you get:
sequoia:~# cat /sys/bus/i2c/devices/0-0048/temp1_input
34750
sequoia:~# cat /sys/class/hwmon/hwmon0/device/temp1_input
34750
sequoia:~#=20

34750 =3D 34.75=B0C

=20

quoted

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/s=

equoia.dts

quoted

index 72d15f0..82fdfdf 100644

--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,23 @@
                       };

                       IIC0: i2c@ef600700 {
+                               #address-cells =3D <1>;
+                               #size-cells =3D <0>;
                               compatible =3D "ibm,iic-440epx", "ibm,ii=

c";

quoted

                               reg =3D <0xef600700 0x00000014>;
                               interrupt-parent =3D <&UIC0>;
                               interrupts =3D <0x2 0x4>;
+
+                               hwmon@48 {
+                                       device_type =3D "hwmon";

=20
We don't need device_type.  Particularly not a new one like this.

I will remove that.

=20

quoted

+                                       compatible =3D "analog,ad7414";

=20
Perhaps 'compatible =3D "analog,ad7417", "amcc,hwmon-440epx";'

It has nothing to do with either AMCC or 440EPx.

Patch is on the way.

Matthias

This patch adds support for the AD7414 temperature sensor
on Sequoia PPC440EPx board.

Signed-off-by: Matthias Fuchs <redacted>
---
 arch/powerpc/boot/dts/sequoia.dts |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/sequoia.dts
index 72d15f0..9ba5def 100644
--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,22 @@
 	   		};
 
 			IIC0: i2c@ef600700 {
+				#address-cells = <1>;
+				#size-cells = <0>;
 				compatible = "ibm,iic-440epx", "ibm,iic";
 				reg = <0xef600700 0x00000014>;
 				interrupt-parent = <&UIC0>;
 				interrupts = <0x2 0x4>;
+
+				hwmon@48 {
+					compatible = "analog,ad7414";
+					reg = <0x48>;
+				};
 			};
 
 			IIC1: i2c@ef600800 {
+				#address-cells = <1>;
+				#size-cells = <0>;
 				compatible = "ibm,iic-440epx", "ibm,iic";
 				reg = <0xef600800 0x00000014>;
 				interrupt-parent = <&UIC0>;

-- 
1.5.3

On Thursday 04 September 2008, Matthias Fuchs wrote:

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

Other than this:

Acked-by: Stefan Roese <sr@denx.de>

Best regards,
Stefan

Yes, the ad7414 driver was finally accepted into the kernel. The ad7414 =
is a generic temperature chip.

FWIW, the warp.dts uses ad7414@4c, not hwmon.

Cheers,
   Sean

-----Original Message-----
From:	<redacted> on =
behalf of Matthias Fuchs
Sent:	Thu 9/4/2008 10:28 AM
To:	Josh Boyer
Cc:	<redacted>
Subject:	Re: [RFC] adding hwmon support to sequoia device tree

Hi,=20

On Thursday 04 September 2008 14:25, Josh Boyer wrote:

On Thu, Sep 04, 2008 at 11:55:02AM +0200, Matthias Fuchs wrote:

quoted

Hi,

I added support for the Sequoia on-board I2C temperature sensor to =

the device.

quoted

I am not sure if there is any node naming convention for such =

devices.

quoted

The needed I2C driver can be found under drivers/hwmon in the kernel =

sources,

quoted

so I found "hwmon" suitable for the node.

Do we want to add this to the sequoia dts file? If this is how it =

should be done,

quoted

I will commit a proper patch.

=20
See comments below.  Out of curiosity, do you have a working driver =

and

setup with this?

Of course.=20

You need
	CONFIG_HWMON=3Dy
	CONFIG_SENSORS_AD7414=3Dy

Via sysfs you get:
sequoia:~# cat /sys/bus/i2c/devices/0-0048/temp1_input
34750
sequoia:~# cat /sys/class/hwmon/hwmon0/device/temp1_input
34750
sequoia:~#=20

34750 =3D 34.75=B0C

=20

quoted

diff --git a/arch/powerpc/boot/dts/sequoia.dts =

b/arch/powerpc/boot/dts/sequoia.dts

quoted

index 72d15f0..82fdfdf 100644

--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,23 @@
                       };

                       IIC0: i2c@ef600700 {
+                               #address-cells =3D <1>;
+                               #size-cells =3D <0>;
                               compatible =3D "ibm,iic-440epx", =

"ibm,iic";

quoted

                               reg =3D <0xef600700 0x00000014>;
                               interrupt-parent =3D <&UIC0>;
                               interrupts =3D <0x2 0x4>;
+
+                               hwmon@48 {
+                                       device_type =3D "hwmon";

=20
We don't need device_type.  Particularly not a new one like this.

I will remove that.

=20

quoted

+                                       compatible =3D =

"analog,ad7414";

=20
Perhaps 'compatible =3D "analog,ad7417", "amcc,hwmon-440epx";'

It has nothing to do with either AMCC or 440EPx.

Patch is on the way.

Matthias
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Hi,

I was inspired by some I2C RTC node (tqm5200.dts). Those nodes
are named rtc@... But to be in common I can change it. Also
the vendor is differnet in warp's dts file. adi (analog device inc.) against
"analog".

So I will update my patch to be compatible with warp.dts.

I keep your ack, Stefan.

Matthias

On Friday 05 September 2008 12:19, Stefan Roese wrote:

quoted

diff --git a/arch/powerpc/boot/dts/sequoia.dts

b/arch/powerpc/boot/dts/sequoia.dts index 72d15f0..9ba5def 100644

--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,22 @@
 	   		};

 			IIC0: i2c@ef600700 {
+				#address-cells = <1>;
+				#size-cells = <0>;
 				compatible = "ibm,iic-440epx", "ibm,iic";
 				reg = <0xef600700 0x00000014>;
 				interrupt-parent = <&UIC0>;
 				interrupts = <0x2 0x4>;
+
+				hwmon@48 {

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:

quoted

+
+				hwmon@48 {

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

We shouldn't.  Node names are supposed to be generic:
http://playground.sun.com/1275/practice/gnames/gnamv14a.html

-Scott

On Fri, 5 Sep 2008 11:00:18 -0500
"Scott Wood" [off-list ref] wrote:

On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:

quoted

quoted

+
+				hwmon@48 {

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

We shouldn't.  Node names are supposed to be generic:
http://playground.sun.com/1275/practice/gnames/gnamv14a.html

Damn. Where were you a year ago when I first introduced this? ;)

And if it is really supposed to be generic, would temp@48 be a
better name since this is basically a generic temperature chip?

Now that the i2c driver is a full of platform driver, I think I
can change the name with no repercussions. So I can live with whatever
decision is made. Can't do anything about the systems that are out in
the field though....

Cheers,
   Sean

On Fri, Sep 05, 2008 at 08:17:03PM -0400, Sean MacLennan wrote:

On Fri, 5 Sep 2008 11:00:18 -0500
"Scott Wood" [off-list ref] wrote:

quoted

On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:

quoted

quoted

+
+				hwmon@48 {

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

We shouldn't.  Node names are supposed to be generic:
http://playground.sun.com/1275/practice/gnames/gnamv14a.html

Damn. Where were you a year ago when I first introduced this? ;)

And if it is really supposed to be generic, would temp@48 be a
better name since this is basically a generic temperature chip?

Now that the i2c driver is a full of platform driver, I think I
can change the name with no repercussions. So I can live with whatever
decision is made. Can't do anything about the systems that are out in
the field though....

It's just the unit-name that should be generic, not the compatible.  The
only thing unit-name is used for is path lookup and distinguishing between
similar hardware units.  I don't think either is a problem in this case.

josh

Hi,

I have chosen "hwmon" because those drivers are placed in the kernel's
drivers/hwmon subdirectory. "temp" sounds to much like "temporary" for
me :-)

We can name it "sensor" - inspired by the lm-sensors projects that 
takes care of such sensors. Because these type of devices are not always
used to monitor the whatever hardware  ... sensor could even be better.

So my favorites are sensor@ or hwmon@.

Sean, I can take your "adi" instead of "analog" for this special sensor.

Matthias

On Saturday 06 September 2008 02:17:03 Sean MacLennan wrote:

On Fri, 5 Sep 2008 11:00:18 -0500

"Scott Wood" [off-list ref] wrote:

quoted

On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:

quoted

quoted

+
+				hwmon@48 {

Not sure if we shouldn't use

                                ad7414@48 {

here. This is the way it is already done in warp.dts.

We shouldn't.  Node names are supposed to be generic:
http://playground.sun.com/1275/practice/gnames/gnamv14a.html

Damn. Where were you a year ago when I first introduced this? ;)

And if it is really supposed to be generic, would temp@48 be a
better name since this is basically a generic temperature chip?

Now that the i2c driver is a full of platform driver, I think I
can change the name with no repercussions. So I can live with whatever
decision is made. Can't do anything about the systems that are out in
the field though....

Cheers,
   Sean
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

On Sat, 6 Sep 2008 14:40:41 +0200
"Matthias Fuchs" [off-list ref] wrote:

Sean, I can take your "adi" instead of "analog" for this special
sensor.

I think I was told to use adi because it is the stock market symbol for
analog devices.

Cheers,
   Sean

Matthias Fuchs wrote:

Hi,

I have chosen "hwmon" because those drivers are placed in the kernel's
drivers/hwmon subdirectory. "temp" sounds to much like "temporary" for
me :-)

How about "dtt"? That's what we use in U-Boot for digital temperature
sensors as well.

We can name it "sensor" - inspired by the lm-sensors projects that 
takes care of such sensors. Because these type of devices are not always
used to monitor the whatever hardware  ... sensor could even be better.

So my favorites are sensor@ or hwmon@.

Sean, I can take your "adi" instead of "analog" for this special sensor.

"adi" sounds more reasonable, indeed and as the OF software ignores the
vendor field, changing the name should not harm.

Wolfgang.

This patch adds support for the AD7414 temperature sensor
on Sequoia PPC440EPx board.

Signed-off-by: Matthias Fuchs <redacted>
Acked-by: Stefan Roese <sr@denx.de>
---
 arch/powerpc/boot/dts/sequoia.dts |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/sequoia.dts
index 72d15f0..3b295e8 100644
--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts

@@ -246,13 +246,22 @@
 	   		};
 
 			IIC0: i2c@ef600700 {
+				#address-cells = <1>;
+				#size-cells = <0>;
 				compatible = "ibm,iic-440epx", "ibm,iic";
 				reg = <0xef600700 0x00000014>;
 				interrupt-parent = <&UIC0>;
 				interrupts = <0x2 0x4>;
+
+				hwmon@48 {
+					compatible = "adi,ad7414";
+					reg = <0x48>;
+				};
 			};
 
 			IIC1: i2c@ef600800 {
+				#address-cells = <1>;
+				#size-cells = <0>;
 				compatible = "ibm,iic-440epx", "ibm,iic";
 				reg = <0xef600800 0x00000014>;
 				interrupt-parent = <&UIC0>;

-- 
1.5.3

On Sat, Sep 06, 2008 at 05:07:15PM +0200, Wolfgang Grandegger wrote:

How about "dtt"? That's what we use in U-Boot for digital temperature
sensors as well.

I vote against any acronyms that are not very well known.  Even knowing
that it's supposed to be "digital temperature <something>", I can't
figure out what the <something> is.  Not having the context that it
relates to temperature, I wouldn't have a clue what "dtt" is.

hwmon is concise and fairly obvious, and unlike "temp" could be
meaningfully used with chips that also monitor voltages, fan speeds, etc.

-Scott

On Monday 08 September 2008 18:03, Scott Wood wrote:

On Sat, Sep 06, 2008 at 05:07:15PM +0200, Wolfgang Grandegger wrote:

quoted

How about "dtt"? That's what we use in U-Boot for digital temperature
sensors as well.

I vote against any acronyms that are not very well known.  Even knowing
that it's supposed to be "digital temperature <something>", I can't
figure out what the <something> is.  Not having the context that it
relates to temperature, I wouldn't have a clue what "dtt" is.

hwmon is concise and fairly obvious, and unlike "temp" could be
meaningfully used with chips that also monitor voltages, fan speeds, etc.

-Scott

Uh, thanks Scott. Now we got it. I am fully with you and my "V2" patch
should make everybody happy.

So, let's apply this patch.

Matthias

On Tue, Sep 09, 2008 at 01:52:29PM +0200, Matthias Fuchs wrote:

On Monday 08 September 2008 18:03, Scott Wood wrote:

quoted

On Sat, Sep 06, 2008 at 05:07:15PM +0200, Wolfgang Grandegger wrote:

quoted

How about "dtt"? That's what we use in U-Boot for digital temperature
sensors as well.

I vote against any acronyms that are not very well known.  Even knowing
that it's supposed to be "digital temperature <something>", I can't
figure out what the <something> is.  Not having the context that it
relates to temperature, I wouldn't have a clue what "dtt" is.

hwmon is concise and fairly obvious, and unlike "temp" could be
meaningfully used with chips that also monitor voltages, fan speeds, etc.

-Scott

Uh, thanks Scott. Now we got it. I am fully with you and my "V2" patch
should make everybody happy.

So, let's apply this patch.

I already have it queued up.

josh