[patch 0/3] fsl_soc / mpc8349emitx patches

[patch 0/3] fsl_soc / mpc8349emitx patches

[Peter Korsgaard]

Various small fsl_soc / mpc8349emitx patches.

-- 
Bye, Peter Korsgaard

[patch 1/3] fsl_soc: Fix trivial printk typo.

[Peter Korsgaard]

Fix a trivial printk typo in fsl_soc.

Cc: G. Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 arch/powerpc/sysdev/fsl_soc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/arch/powerpc/sysdev/fsl_soc.c
===================================================================
--- linux.orig/arch/powerpc/sysdev/fsl_soc.c
+++ linux/arch/powerpc/sysdev/fsl_soc.c
@@ -346,7 +346,7 @@
 
 		addr = of_get_property(node, "reg", &len);
 		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
-			printk(KERN_WARNING "fsl_ioc.c: invalid i2c device entry\n");
+			printk(KERN_WARNING "fsl_soc.c: invalid i2c device entry\n");
 			continue;
 		}
 

-- 
Bye, Peter Korsgaard

[patch 2/3] fsl_soc: rtc-ds1307 support

[Peter Korsgaard]

Add support for the I2C devices handled by the rtc-ds1307 driver to
of_register_i2c_devices.

Cc: G. Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 arch/powerpc/sysdev/fsl_soc.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux/arch/powerpc/sysdev/fsl_soc.c
===================================================================
--- linux.orig/arch/powerpc/sysdev/fsl_soc.c
+++ linux/arch/powerpc/sysdev/fsl_soc.c
@@ -319,6 +319,12 @@
 	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
 	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
 	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+	{"dallas,ds1307",  "rtc-ds1307",  "ds1307",},
+	{"dallas,ds1337",  "rtc-ds1307",  "ds1337",},
+	{"dallas,ds1338",  "rtc-ds1307",  "ds1338",},
+	{"dallas,ds1339",  "rtc-ds1307",  "ds1339",},
+	{"dallas,ds1340",  "rtc-ds1307",  "ds1340",},
+	{"stm,m41t00",     "rtc-ds1307",  "m41t00"},
 };
 
 static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)

-- 
Bye, Peter Korsgaard

[patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Peter Korsgaard]

Add ds1339 I2C RTC chip as child of 2nd I2C controller.

Cc: Timur Tabi <timur@freescale.com>
Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
===================================================================
--- linux.orig/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ linux/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -68,6 +68,13 @@
 			interrupts = <f 8>;
 			interrupt-parent = < &ipic >;
 			dfsrr;
+
+			rtc@68 {
+				device_type = "rtc";
+				compatible = "dallas,ds1339";
+				reg = <68 4>;
+			};
+
 		};
 
 		spi@7000 {

-- 
Bye, Peter Korsgaard

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Scott Wood]

On Thu, Sep 20, 2007 at 12:42:14PM +0200, Peter Korsgaard wrote:
> Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
> ===================================================================
> --- linux.orig/arch/powerpc/boot/dts/mpc8349emitx.dts
> +++ linux/arch/powerpc/boot/dts/mpc8349emitx.dts
> @@ -68,6 +68,13 @@
>  			interrupts = <f 8>;
>  			interrupt-parent = < &ipic >;
>  			dfsrr;
> +
> +			rtc@68 {
> +				device_type = "rtc";
> +				compatible = "dallas,ds1339";
> +				reg = <68 4>;
> +			};

#size-cells is zero on i2c, so it should just be reg = <68>.

You'll probably need to add #address-cells and #size-cells to the
controller node, as well.

-Scott

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Peter Korsgaard]

>>>>> "Scott" == Scott Wood <scottwood@freescale.com> writes:

Hi,

 Scott> #size-cells is zero on i2c, so it should just be reg = <68>.

 Scott> You'll probably need to add #address-cells and #size-cells to the
 Scott> controller node, as well.

Ahh - Thanks. This should be better.
---

[PATCH] mpc8349emitx.dts: Add ds1339 RTC

Add ds1339 I2C RTC chip as child of 2nd I2C controller.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
===================================================================
--- linux.orig/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ linux/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -62,12 +62,21 @@
 		};
 
 		i2c@3100 {
+			#address-cells = <1>;
+			#size-cells = <0>;
 			device_type = "i2c";
 			compatible = "fsl-i2c";
 			reg = <3100 100>;
 			interrupts = <f 8>;
 			interrupt-parent = < &ipic >;
 			dfsrr;
+
+			rtc@68 {
+				device_type = "rtc";
+				compatible = "dallas,ds1339";
+				reg = <68>;
+			};
+
 		};
 
 		spi@7000 {

-- 
Bye, Peter Korsgaard

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Fri, Sep 21, 2007 at 09:35:03AM +0200, Peter Korsgaard wrote:
> >>>>> "Scott" == Scott Wood <scottwood@freescale.com> writes:
> 
> Hi,
> 
>  Scott> #size-cells is zero on i2c, so it should just be reg = <68>.
> 
>  Scott> You'll probably need to add #address-cells and #size-cells to the
>  Scott> controller node, as well.

Uh.. yes.. i2c interfaces should really always have #a and #s.

> Ahh - Thanks. This should be better.
> ---
> 
> [PATCH] mpc8349emitx.dts: Add ds1339 RTC
> 
> Add ds1339 I2C RTC chip as child of 2nd I2C controller.
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
> ===================================================================
> --- linux.orig/arch/powerpc/boot/dts/mpc8349emitx.dts
> +++ linux/arch/powerpc/boot/dts/mpc8349emitx.dts
> @@ -62,12 +62,21 @@
>  		};
>  
>  		i2c@3100 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
>  			device_type = "i2c";

Hrm... we probably want an "i2c" device_type class, but I don't think
we've actually defined one, which is a problem

>  			compatible = "fsl-i2c";
>  			reg = <3100 100>;
>  			interrupts = <f 8>;
>  			interrupt-parent = < &ipic >;
>  			dfsrr;
> +
> +			rtc@68 {
> +				device_type = "rtc";
> +				compatible = "dallas,ds1339";
> +				reg = <68>;
> +			};

I think we want to think a bit more carefully about how to do bindings
for RTC devices.  No "rtc" device_type is defined, but again we might
want to.

I did find one real OF binding for a different Dallas RTC (and NVRAM),
see:

http://playground.sun.com/1275/proposals/Closed/Remanded/Accepted/346-it.txt

It's a little different from the example above.

The fact that NVRAM+RTC chips are so common is a bit of an issue from
the point of view of defining a device class binding - a device can't
have type "rtc" and "nvram".

> +
>  		};
>  
>  		spi@7000 {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Peter Korsgaard]

>>>>> "David" == David Gibson <david@gibson.dropbear.id.au> writes:

Hi

 >> compatible = "fsl-i2c";
 >> reg = <3100 100>;
 >> interrupts = <f 8>;
 >> interrupt-parent = < &ipic >;
 >> dfsrr;
 >> +
 >> +			rtc@68 {
 >> +				device_type = "rtc";
 >> +				compatible = "dallas,ds1339";
 >> +				reg = <68>;
 >> +			};

 David> I think we want to think a bit more carefully about how to do bindings
 David> for RTC devices.  No "rtc" device_type is defined, but again we might
 David> want to.

Could be. I've simply done it like kuroboxHD.dts already does and
fsl_soc.c expects.

 David> I did find one real OF binding for a different Dallas RTC (and NVRAM),
 David> see:

 David> http://playground.sun.com/1275/proposals/Closed/Remanded/Accepted/346-it.txt

 David> It's a little different from the example above.

 David> The fact that NVRAM+RTC chips are so common is a bit of an issue from
 David> the point of view of defining a device class binding - a device can't
 David> have type "rtc" and "nvram".

True. I think we should primarily focus on the RTC part rather than
NVRAM as that's the "main" functionality and leave a NVRAM class for
I2C EEPROMs.

The Linux driver for the chip (rtc-1307.c) doesn't expose the NVRAM
bytes either.

But I'm open for suggestions.

-- 
Bye, Peter Korsgaard

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Mon, Sep 24, 2007 at 07:52:22AM +0200, Peter Korsgaard wrote:
> >>>>> "David" == David Gibson <david@gibson.dropbear.id.au> writes:
> 
> Hi
> 
>  >> compatible = "fsl-i2c";
>  >> reg = <3100 100>;
>  >> interrupts = <f 8>;
>  >> interrupt-parent = < &ipic >;
>  >> dfsrr;
>  >> +
>  >> +			rtc@68 {
>  >> +				device_type = "rtc";
>  >> +				compatible = "dallas,ds1339";
>  >> +				reg = <68>;
>  >> +			};
> 
>  David> I think we want to think a bit more carefully about how to do bindings
>  David> for RTC devices.  No "rtc" device_type is defined, but again we might
>  David> want to.
> 
> Could be. I've simply done it like kuroboxHD.dts already does and
> fsl_soc.c expects.
> 
>  David> I did find one real OF binding for a different Dallas RTC (and NVRAM),
>  David> see:
> 
>  David> http://playground.sun.com/1275/proposals/Closed/Remanded/Accepted/346-it.txt
> 
>  David> It's a little different from the example above.
> 
>  David> The fact that NVRAM+RTC chips are so common is a bit of an issue from
>  David> the point of view of defining a device class binding - a device can't
>  David> have type "rtc" and "nvram".
> 
> True. I think we should primarily focus on the RTC part rather than
> NVRAM as that's the "main" functionality and leave a NVRAM class for
> I2C EEPROMs.
> 
> The Linux driver for the chip (rtc-1307.c) doesn't expose the NVRAM
> bytes either.

Incidentally how are you planning on instantiating the driver?  AFAIK
all the rtc-* drivers are platform drivers rather than of_platform
drivers.  I had been thinking of an rtc helper function that would go
through the tree instantiating platform devices for any RTCs based on
a compatible -> platform device name table.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Peter Korsgaard]

>>>>> "David" == David Gibson <david@gibson.dropbear.id.au> writes:

Hi,

 >> The Linux driver for the chip (rtc-1307.c) doesn't expose the NVRAM
 >> bytes either.

 David> Incidentally how are you planning on instantiating the driver?  AFAIK
 David> all the rtc-* drivers are platform drivers rather than of_platform
 David> drivers.  I had been thinking of an rtc helper function that would go
 David> through the tree instantiating platform devices for any RTCs based on
 David> a compatible -> platform device name table.

Please see patch #2 in the series:

http://ozlabs.org/pipermail/linuxppc-dev/2007-September/042896.html

That helper function more or less already exists in fsl_soc.c.

-- 
Bye, Peter Korsgaard

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Tue, Sep 25, 2007 at 07:33:15AM +0200, Peter Korsgaard wrote:
> >>>>> "David" == David Gibson <david@gibson.dropbear.id.au> writes:
> 
> Hi,
> 
>  >> The Linux driver for the chip (rtc-1307.c) doesn't expose the NVRAM
>  >> bytes either.
> 
>  David> Incidentally how are you planning on instantiating the driver?  AFAIK
>  David> all the rtc-* drivers are platform drivers rather than of_platform
>  David> drivers.  I had been thinking of an rtc helper function that would go
>  David> through the tree instantiating platform devices for any RTCs based on
>  David> a compatible -> platform device name table.
> 
> Please see patch #2 in the series:
> 
> http://ozlabs.org/pipermail/linuxppc-dev/2007-September/042896.html
> 
> That helper function more or less already exists in fsl_soc.c.

Ah, I see.  Well... it exists for i2c devices (possibly including
RTCs).  Whereas I was thinking of a version for RTCs (possibly
including i2c devices).  Actually that won't quite work - looks like
the i2c RTC class drivers are probed differently from the RTC drivers
I was looking at which are pure platform devices.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Kumar Gala]

On Sep 24, 2007, at 12:07 AM, David Gibson wrote:

> On Fri, Sep 21, 2007 at 09:35:03AM +0200, Peter Korsgaard wrote:
>>>>>>> "Scott" == Scott Wood <scottwood@freescale.com> writes:
>>
>> Hi,
>>
>>  Scott> #size-cells is zero on i2c, so it should just be reg = <68>.
>>
>>  Scott> You'll probably need to add #address-cells and #size-cells  
>> to the
>>  Scott> controller node, as well.
>
> Uh.. yes.. i2c interfaces should really always have #a and #s.
>
>> Ahh - Thanks. This should be better.
>> ---
>>
>> [PATCH] mpc8349emitx.dts: Add ds1339 RTC
>>
>> Add ds1339 I2C RTC chip as child of 2nd I2C controller.
>>
>> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
>> ---
>>  arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
>> ===================================================================
>> --- linux.orig/arch/powerpc/boot/dts/mpc8349emitx.dts
>> +++ linux/arch/powerpc/boot/dts/mpc8349emitx.dts
>> @@ -62,12 +62,21 @@
>>  		};
>>
>>  		i2c@3100 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>>  			device_type = "i2c";
>
> Hrm... we probably want an "i2c" device_type class, but I don't think
> we've actually defined one, which is a prob

suggestions on how to handle this?  We've described that fsl-i2c  
nodes in booting-without-of.txt should have it. (which may have been  
wrong at the time).

- k

>
>>  			compatible = "fsl-i2c";
>>  			reg = <3100 100>;
>>  			interrupts = <f 8>;
>>  			interrupt-parent = < &ipic >;
>>  			dfsrr;

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Scott Wood]

David Gibson wrote:
>>  		i2c@3100 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>>  			device_type = "i2c";
> 
> Hrm... we probably want an "i2c" device_type class, but I don't think
> we've actually defined one, which is a problem

Right... but we need to get the kernel to stop expecting the device type 
to be there before we yell at people for including it. :-)

> The fact that NVRAM+RTC chips are so common is a bit of an issue from
> the point of view of defining a device class binding - a device can't
> have type "rtc" and "nvram".

This is one of the reasons that I'd prefer to use compatible for such 
things.

-Scott

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Mon, Sep 24, 2007 at 09:52:31AM -0500, Scott Wood wrote:
> David Gibson wrote:
> >>  		i2c@3100 {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >>  			device_type = "i2c";
> > 
> > Hrm... we probably want an "i2c" device_type class, but I don't think
> > we've actually defined one, which is a problem
> 
> Right... but we need to get the kernel to stop expecting the device type 
> to be there before we yell at people for including it. :-)

Obviously.  We should make sure all the corresponding compatibles are
specific enough, change the drivers, then think about getting rid of
it.

> > The fact that NVRAM+RTC chips are so common is a bit of an issue from
> > the point of view of defining a device class binding - a device can't
> > have type "rtc" and "nvram".
> 
> This is one of the reasons that I'd prefer to use compatible for such 
> things.

Yeah, fair enough.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Segher Boessenkool]

>>  Scott> #size-cells is zero on i2c, so it should just be reg = <68>.
>>
>>  Scott> You'll probably need to add #address-cells and #size-cells to 
>> the
>>  Scott> controller node, as well.
>
> Uh.. yes.. i2c interfaces should really always have #a and #s.

More generally, every node that defines a bus needs it (unless the
defaults of 2 resp. 1 are correct for this bus, but even then you
might want it because it makes things more explicit).

>>  		i2c@3100 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>>  			device_type = "i2c";
>
> Hrm... we probably want an "i2c" device_type class, but I don't think
> we've actually defined one, which is a problem

By defining new device_type's, or new semantics for device_type,
you open the door to (accidentally) becoming incompatible with
"real" OF.

And you don't need to: "real" OF has a mechanism for specifying
the "generic device class" already, if you use the "generic names"
recommended practice (and you do, for both this node and the rtc
node): it's the generic name itself!

>> +			rtc@68 {
>> +				device_type = "rtc";
>> +				compatible = "dallas,ds1339";
>> +				reg = <68>;
>> +			};
>
> I think we want to think a bit more carefully about how to do bindings
> for RTC devices.  No "rtc" device_type is defined, but again we might
> want to.

Actually, "device_type" = "rtc" _is_ defined (in the "device support
extensions" recommended practice), and there is no useful way a flat
device tree can implement it (it merely defines get-time and set-time
methods).

> The fact that NVRAM+RTC chips are so common is a bit of an issue from
> the point of view of defining a device class binding - a device can't
> have type "rtc" and "nvram".

You should match OS drivers on "compatible" only anyway.

Those cases where OS drivers don't nicely 1-1 match device nodes are a
bit of a headache; for RTC/NVRAM devices, these problems are nicely
side-stepped by handling this from platform code.


Segher

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Mon, Sep 24, 2007 at 11:11:28PM +0200, Segher Boessenkool wrote:
> >>  Scott> #size-cells is zero on i2c, so it should just be reg = <68>.
> >>
> >>  Scott> You'll probably need to add #address-cells and #size-cells to 
> >> the
> >>  Scott> controller node, as well.
> >
> > Uh.. yes.. i2c interfaces should really always have #a and #s.
> 
> More generally, every node that defines a bus needs it (unless the
> defaults of 2 resp. 1 are correct for this bus, but even then you
> might want it because it makes things more explicit).

Yes.  Actually I think we should make explicit #a and #s at least
strongly recommended in the flat tree documentation.

> >>  		i2c@3100 {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >>  			device_type = "i2c";
> >
> > Hrm... we probably want an "i2c" device_type class, but I don't think
> > we've actually defined one, which is a problem
> 
> By defining new device_type's, or new semantics for device_type,
> you open the door to (accidentally) becoming incompatible with
> "real" OF.

Hrm... perhaps.  But is it a realistic danger - I'll have to think
more about that.

> And you don't need to: "real" OF has a mechanism for specifying
> the "generic device class" already, if you use the "generic names"
> recommended practice (and you do, for both this node and the rtc
> node): it's the generic name itself!

Hmm, I guess.

> >> +			rtc@68 {
> >> +				device_type = "rtc";
> >> +				compatible = "dallas,ds1339";
> >> +				reg = <68>;
> >> +			};
> >
> > I think we want to think a bit more carefully about how to do bindings
> > for RTC devices.  No "rtc" device_type is defined, but again we might
> > want to.
> 
> Actually, "device_type" = "rtc" _is_ defined (in the "device support
> extensions" recommended practice), and there is no useful way a flat
> device tree can implement it (it merely defines get-time and set-time
> methods).

Ah.. right.  That changes things a bit, in that we might want to
include device_type purely for similarity with real OF tree.

Real OF has a device_type == "nvram" too, doesn't it?  AFAICT the real
OF systems I have (which I think all have ISA-like CMOS RTC/NVRAM
chips) the RTC is labelled as "nvram" rather than "rtc".

> > The fact that NVRAM+RTC chips are so common is a bit of an issue from
> > the point of view of defining a device class binding - a device can't
> > have type "rtc" and "nvram".
> 
> You should match OS drivers on "compatible" only anyway.

Absolutely.  I was only thinking of defining "device classes" where
for some reason it is useful to examine them without needing to pick a
particular driver.

> Those cases where OS drivers don't nicely 1-1 match device nodes are a
> bit of a headache; for RTC/NVRAM devices, these problems are nicely
> side-stepped by handling this from platform code.

Not necessarily.  The new RTC class drivers are just drivers like
anything other and are not especially instantiated from the platform
code.


And drat.  I was only really mentioning stuff about device_type in
passing, but it's the only thing anyone's responded to.  I was also
mostly suggesting changing the format of compatible, for greater
similarity with the existing ds1385 binding.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Segher Boessenkool]

>>> Hrm... we probably want an "i2c" device_type class, but I don't think
>>> we've actually defined one, which is a problem
>>
>> By defining new device_type's, or new semantics for device_type,
>> you open the door to (accidentally) becoming incompatible with
>> "real" OF.
>
> Hrm... perhaps.  But is it a realistic danger - I'll have to think
> more about that.

It is trivial to avoid these dangers completely by not overloading
the meaning of "device_type".

>>> I think we want to think a bit more carefully about how to do  
>>> bindings
>>> for RTC devices.  No "rtc" device_type is defined, but again we might
>>> want to.
>>
>> Actually, "device_type" = "rtc" _is_ defined (in the "device support
>> extensions" recommended practice), and there is no useful way a flat
>> device tree can implement it (it merely defines get-time and set-time
>> methods).
>
> Ah.. right.  That changes things a bit, in that we might want to
> include device_type purely for similarity with real OF tree.

You should include the device_type only if you implement its binding,
and a flat device tree does not, and cannot.  (In almost all cases,
a flat device tree cannot implement device_type's semantics -- this
means that pretty much the only case where a flat tree should use
device_type is to have it as a workaround for bad kernel requirements).

> Real OF has a device_type == "nvram" too, doesn't it?

Yes, same "device support extensions" document.

> AFAICT the real
> OF systems I have (which I think all have ISA-like CMOS RTC/NVRAM
> chips) the RTC is labelled as "nvram" rather than "rtc".

Sounds buggy.

>>> The fact that NVRAM+RTC chips are so common is a bit of an issue from
>>> the point of view of defining a device class binding - a device can't
>>> have type "rtc" and "nvram".
>>
>> You should match OS drivers on "compatible" only anyway.
>
> Absolutely.  I was only thinking of defining "device classes" where
> for some reason it is useful to examine them without needing to pick a
> particular driver.

Yeah I understand.  In what situations would this be useful?
Answering that question will make the requirements for this more
clear; or maybe it will show we do not need this at all.

>> Those cases where OS drivers don't nicely 1-1 match device nodes are a
>> bit of a headache; for RTC/NVRAM devices, these problems are nicely
>> side-stepped by handling this from platform code.
>
> Not necessarily.  The new RTC class drivers are just drivers like
> anything other and are not especially instantiated from the platform
> code.

I meant "can be nicely side-stepped", or "usually are ..." :-)

Obviously, when you cannot avoid the problem, you have a problem.

> And drat.  I was only really mentioning stuff about device_type in
> passing, but it's the only thing anyone's responded to.  I was also
> mostly suggesting changing the format of compatible, for greater
> similarity with the existing ds1385 binding.

Okay, quoting from your earlier message:

> I did find one real OF binding for a different Dallas RTC (and NVRAM),
> see:
>
> http://playground.sun.com/1275/proposals/Closed/Remanded/Accepted/346- 
> it.txt
>
> It's a little different from the example above.

That is a binding for the nvram part only, not for the RTC.


Segher

Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[David Gibson]

On Tue, Sep 25, 2007 at 10:33:58PM +0200, Segher Boessenkool wrote:
> >>> Hrm... we probably want an "i2c" device_type class, but I don't think
> >>> we've actually defined one, which is a problem
> >>
> >> By defining new device_type's, or new semantics for device_type,
> >> you open the door to (accidentally) becoming incompatible with
> >> "real" OF.
> >
> > Hrm... perhaps.  But is it a realistic danger - I'll have to think
> > more about that.
> 
> It is trivial to avoid these dangers completely by not overloading
> the meaning of "device_type".

Hrm.  Perhaps.

> >>> I think we want to think a bit more carefully about how to do  
> >>> bindings
> >>> for RTC devices.  No "rtc" device_type is defined, but again we might
> >>> want to.
> >>
> >> Actually, "device_type" = "rtc" _is_ defined (in the "device support
> >> extensions" recommended practice), and there is no useful way a flat
> >> device tree can implement it (it merely defines get-time and set-time
> >> methods).
> >
> > Ah.. right.  That changes things a bit, in that we might want to
> > include device_type purely for similarity with real OF tree.
> 
> You should include the device_type only if you implement its binding,
> and a flat device tree does not, and cannot.  (In almost all cases,
> a flat device tree cannot implement device_type's semantics -- this
> means that pretty much the only case where a flat tree should use
> device_type is to have it as a workaround for bad kernel requirements).

I really don't think there's an ambiguity here.  A flat-tree can
clearly never implement runtime binding features.  This is also true
for a flat tree derived from a real OF, and so full of device_type all
over the place.

> > Real OF has a device_type == "nvram" too, doesn't it?
> 
> Yes, same "device support extensions" document.

Erm.. I've lost track amongst our various threads.  Which same
document?

> > AFAICT the real
> > OF systems I have (which I think all have ISA-like CMOS RTC/NVRAM
> > chips) the RTC is labelled as "nvram" rather than "rtc".
> 
> Sounds buggy.

Why?

[snip]
> > I did find one real OF binding for a different Dallas RTC (and NVRAM),
> > see:
> >
> > http://playground.sun.com/1275/proposals/Closed/Remanded/Accepted/346- 
> > it.txt
> >
> > It's a little different from the example above.
> 
> That is a binding for the nvram part only, not for the RTC.

Hrm.  So how do you suggest we do bindings for combined devices?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson