fsl_soc / mpc8349emitx patches

PATCH In review Peter Korsgaard 5 participants 18 messages Sep 20, 2007 – Sep 28, 2007

Subsystems (2)

Device tree bindings PowerPC

lore

Share & tools

lore

Peter Korsgaard

author

2007-09-20 10:43 UTC #0 lore raw

cover fsl_soc / mpc8349emitx patches

Various small fsl_soc / mpc8349emitx patches.

Signature · 1 line

Bye, Peter Korsgaard

Peter Korsgaard

author

2007-09-20 10:43 UTC #1 lore raw

PATCH 1/3 fsl_soc: Fix trivial printk typo.

Fix a trivial printk typo in fsl_soc.

Cc G. Liakhovetski <[email protected]>

Signed-off-by Peter Korsgaard <[email protected]>

---
 arch/powerpc/sysdev/fsl_soc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/sysdev/fsl_soc.c +1 -1

Index: 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;
 		}

Signature · 1 line

Bye, Peter Korsgaard

Peter Korsgaard

author

2007-09-20 10:43 UTC #2 lore raw

PATCH 2/3 fsl_soc: rtc-ds1307 support

In reply to Peter Korsgaard

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

Cc G. Liakhovetski <[email protected]>

Signed-off-by Peter Korsgaard <[email protected]>

---
 arch/powerpc/sysdev/fsl_soc.c |    6 ++++++
 1 file changed, 6 insertions(+)

arch/powerpc/sysdev/fsl_soc.c +6 -0

Index: 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)

Signature · 1 line

Bye, Peter Korsgaard

Peter Korsgaard

author

2007-09-20 10:43 UTC #3 lore raw

PATCH 3/3 mpc8349emitx.dts: Add ds1339 RTC

In reply to Peter Korsgaard

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

Cc Timur Tabi <[email protected]>

Signed-off-by Peter Korsgaard <[email protected]>

---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    7 +++++++
 1 file changed, 7 insertions(+)

arch/powerpc/boot/dts/mpc8349emitx.dts +7 -0

Index: 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 {

Signature · 1 line

Bye, Peter Korsgaard

Scott Wood

2007-09-20 13:35 UTC #4 lore raw

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

Quoted 5 lines · Peter Korsgaard

Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
===================================================================

arch/powerpc/boot/dts/mpc8349emitx.dts line 76

Quoted 9 lines

arch/powerpc/boot/dts/mpc8349emitx.dts +6 -0

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

Peter Korsgaard

author

2007-09-21 07:35 UTC #5 lore raw

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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Scott" == Scott Wood <[email protected]> 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 <[email protected]>

---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
 1 file changed, 9 insertions(+)

arch/powerpc/boot/dts/mpc8349emitx.dts +9 -0

Index: 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 {

Signature · 1 line

Bye, Peter Korsgaard

David Gibson

2007-09-24 05:07 UTC #6 lore raw

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

Quoted 8 lines · Peter Korsgaard

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Scott" == Scott Wood <[email protected]> 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.

Quoted 17 lines

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 <[email protected]>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
 1 file changed, 9 insertions(+)
Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
===================================================================

arch/powerpc/boot/dts/mpc8349emitx.dts line 67

Quoted 6 lines

arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0

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

arch/powerpc/boot/dts/mpc8349emitx.dts line 78

Quoted 11 lines

arch/powerpc/boot/dts/mpc8349emitx.dts +6 -0

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

Quoted 5 lines

arch/powerpc/boot/dts/mpc8349emitx.dts +1 -0
+
 		};
 
 		spi@7000 {

Signature · 4 lines

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

Peter Korsgaard

author

2007-09-24 05:52 UTC #7 lore raw

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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"David" == David Gibson <[email protected]> writes:

Quoted 11 lines

arch/powerpc/boot/dts/mpc8349emitx.dts line 78
Quoted 11 lines
compatible = "fsl-i2c";
reg = <3100 100>;
interrupts = <f 8>;
interrupt-parent = < &ipic >;
dfsrr;
arch/powerpc/boot/dts/mpc8349emitx.dts +6 -0
+
+			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.

Signature · 1 line

Bye, Peter Korsgaard

David Gibson

2007-09-25 02:13 UTC #8 lore raw

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

Quoted 34 lines · Peter Korsgaard

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"David" == David Gibson <[email protected]> writes:
Hi
Quoted 11 lines
arch/powerpc/boot/dts/mpc8349emitx.dts line 78
Quoted 11 lines
compatible = "fsl-i2c";
reg = <3100 100>;
interrupts = <f 8>;
interrupt-parent = < &ipic >;
dfsrr;
arch/powerpc/boot/dts/mpc8349emitx.dts +6 -0
+
+			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".

Quoted 6 lines

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.

Signature · 4 lines

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

Peter Korsgaard

author

2007-09-25 05:33 UTC #9 lore raw

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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"David" == David Gibson <[email protected]> writes:

Hi,

Quoted 2 lines

Quoted 2 lines
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.

Signature · 1 line

Bye, Peter Korsgaard

David Gibson

2007-09-25 05:47 UTC #10 lore raw

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

Quoted 13 lines · Peter Korsgaard

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"David" == David Gibson <[email protected]> writes:
Hi,
Quoted 2 lines
Quoted 2 lines
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.

Quoted 5 lines

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.

Signature · 4 lines

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

Kumar Gala

2007-09-24 06:13 UTC #11 lore raw

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

In reply to David Gibson

Quoted 32 lines · David Gibson

Quoted 9 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Scott" == Scott Wood <[email protected]> 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.
Quoted 19 lines
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 <[email protected]>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts |    9 +++++++++
 1 file changed, 9 insertions(+)
Index: linux/arch/powerpc/boot/dts/mpc8349emitx.dts
===================================================================
arch/powerpc/boot/dts/mpc8349emitx.dts +0 -0
@@ -62,12 +62,21 @@
 		};
 

Quoted 7 lines

arch/powerpc/boot/dts/mpc8349emitx.dts line 67
Quoted 4 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0
 		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

Quoted 6 lines

arch/powerpc/boot/dts/mpc8349emitx.dts +0 -0

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

Scott Wood

2007-09-24 14:53 UTC #12 lore raw

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

In reply to David Gibson

Quoted 7 lines · David Gibson

arch/powerpc/boot/dts/mpc8349emitx.dts line 67
Quoted 4 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0
 		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. :-)

Quoted 3 lines

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

David Gibson

2007-09-25 02:04 UTC #13 lore raw

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

Quoted 11 lines · Scott Wood

Quoted 7 lines · David Gibson
arch/powerpc/boot/dts/mpc8349emitx.dts line 67
Quoted 4 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0
 		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.

Quoted 6 lines

Quoted 3 lines
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.

Signature · 4 lines

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

Segher Boessenkool

2007-09-24 21:11 UTC #14 lore raw

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

In reply to David Gibson

Quoted 7 lines

Quoted 5 lines
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).

Quoted 7 lines

arch/powerpc/boot/dts/mpc8349emitx.dts line 67
Quoted 4 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0
 		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!

Quoted 9 lines

arch/powerpc/boot/dts/mpc8349emitx.dts line 78
Quoted 5 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +5 -0
+			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).

Quoted 3 lines

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

David Gibson

2007-09-25 02:11 UTC #15 lore raw

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

Quoted 11 lines · Segher Boessenkool

Quoted 7 lines
Quoted 5 lines
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.

Quoted 11 lines

Quoted 7 lines
arch/powerpc/boot/dts/mpc8349emitx.dts line 67
Quoted 4 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +2 -0
 		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.

Quoted 4 lines

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.

Quoted 10 lines

Quoted 9 lines
arch/powerpc/boot/dts/mpc8349emitx.dts line 78
Quoted 5 lines
arch/powerpc/boot/dts/mpc8349emitx.dts +5 -0
+			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.

Quoted 4 lines

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

Quoted 5 lines

Quoted 3 lines
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.

Quoted 3 lines

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.

Signature · 4 lines

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

Segher Boessenkool

2007-09-25 20:33 UTC #16 lore raw

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

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
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".

Quoted 11 lines

Quoted 8 lines
Quoted 3 lines
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).

Quoted 1 line

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

Yes, same "device support extensions" document.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 4 lines

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:

Quoted 6 lines

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

David Gibson

2007-09-28 02:45 UTC #17 lore raw

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

Quoted 7 lines · Segher Boessenkool

Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Quoted 4 lines
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).

Quoted 8 lines

Quoted 2 lines
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.

Quoted 3 lines

Quoted 1 line
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?

Quoted 5 lines

Quoted 3 lines
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]

Quoted 9 lines

Quoted 7 lines
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?

Signature · 4 lines

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