[RFC] adding hwmon support to sequoia device tree

[RFC] adding hwmon support to sequoia device tree

[Matthias Fuchs]

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

Re: [RFC] adding hwmon support to sequoia device tree

[Josh Boyer]

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?

>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.

>+                                       compatible = "analog,ad7414";

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

josh

Re: [RFC] adding hwmon support to sequoia device tree

[Stefan Roese]

On Thursday 04 September 2008, Josh Boyer wrote:
> >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

> >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.
>
> >+                                       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

Re: [RFC] adding hwmon support to sequoia device tree

[Matthias Fuchs]

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:
> >Hi,
> >
> >I added support for the Sequoia on-board I2C temperature sensor to the d=
evice.
> >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,
> >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.
>=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
> >diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/s=
equoia.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 =3D <1>;
> >+                               #size-cells =3D <0>;
> >                                compatible =3D "ibm,iic-440epx", "ibm,ii=
c";
> >                                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
> >+                                       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

[PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Matthias Fuchs]

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

Signed-off-by: Matthias Fuchs <matthias.fuchs@esd-electronics.com>
---
 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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Stefan Roese]

On Thursday 04 September 2008, Matthias Fuchs wrote:
> This patch adds support for the AD7414 temperature sensor
> on Sequoia PPC440EPx board.
>
> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd-electronics.com>
> ---
>  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 {

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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Matthias Fuchs]

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:
> > 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.

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Scott Wood]

On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:
> > +
> > +				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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Sean MacLennan]

On Fri, 5 Sep 2008 11:00:18 -0500
"Scott Wood" <scottwood@freescale.com> wrote:

> On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:
> > > +
> > > +				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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Josh Boyer]

On Fri, Sep 05, 2008 at 08:17:03PM -0400, Sean MacLennan wrote:
>On Fri, 5 Sep 2008 11:00:18 -0500
>"Scott Wood" <scottwood@freescale.com> wrote:
>
>> On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:
>> > > +
>> > > +				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

[PATCH V2] powerpc/44x: Add hwmon support to Sequoia device tree

[Matthias Fuchs]

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

Signed-off-by: Matthias Fuchs <matthias.fuchs@esd-electronics.com>
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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Matthias Fuchs]

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" <scottwood@freescale.com> wrote:
> > On Fri, Sep 05, 2008 at 12:19:43PM +0200, Stefan Roese wrote:
> > > > +
> > > > +				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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Sean MacLennan]

On Sat, 6 Sep 2008 14:40:41 +0200
"Matthias Fuchs" <matthias.fuchs@esd-electronics.com> 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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Wolfgang Grandegger]

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.

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Scott Wood]

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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Matthias Fuchs]

On Monday 08 September 2008 18:03, Scott Wood wrote:
> 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

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

Re: [PATCH] powerpc/44x: Add hwmon support to Sequoia device tree

[Josh Boyer]

On Tue, Sep 09, 2008 at 01:52:29PM +0200, Matthias Fuchs wrote:
>On Monday 08 September 2008 18:03, Scott Wood wrote:
>> 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
>
>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

RE: [RFC] adding hwmon support to sequoia device tree

[Sean MacLennan]

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:	linuxppc-dev-bounces+smaclennan=3Dpikatech.com@ozlabs.org on =
behalf of Matthias Fuchs
Sent:	Thu 9/4/2008 10:28 AM
To:	Josh Boyer
Cc:	linuxppc-dev@ozlabs.org
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:
> >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.
>=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
> >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 =3D <1>;
> >+                               #size-cells =3D <0>;
> >                                compatible =3D "ibm,iic-440epx", =
"ibm,iic";
> >                                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
> >+                                       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