| Message ID | 20081021185844.GA31070@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 691de57679e3f05b708b98ca2ab27657c768843f |
| Delegated to: | Paul Mackerras |
| Headers | show |
2008/10/21 Anton Vorontsov <avorontsov@ru.mvista.com>: > We don't want to encourage the device_type usage. It isn't used in the > code, so we can simply remove it from the dts files. > > Suggested-by: Scott Wood <scottwood@freescale.com> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Acked-by: Grant Likely <grant.likely@secretlab.ca> > --- > arch/powerpc/boot/dts/kuroboxHD.dts | 1 - > arch/powerpc/boot/dts/kuroboxHG.dts | 1 - > arch/powerpc/boot/dts/lite5200.dts | 1 - > arch/powerpc/boot/dts/lite5200b.dts | 1 - > arch/powerpc/boot/dts/motionpro.dts | 1 - > arch/powerpc/boot/dts/mpc8315erdb.dts | 1 - > arch/powerpc/boot/dts/mpc8349emitx.dts | 1 - > arch/powerpc/boot/dts/mpc8349emitxgp.dts | 1 - > arch/powerpc/boot/dts/mpc8377_rdb.dts | 1 - > arch/powerpc/boot/dts/mpc8378_rdb.dts | 1 - > arch/powerpc/boot/dts/mpc8379_rdb.dts | 1 - > arch/powerpc/boot/dts/pcm030.dts | 2 -- > arch/powerpc/boot/dts/tqm5200.dts | 1 - > 13 files changed, 0 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/boot/dts/kuroboxHD.dts b/arch/powerpc/boot/dts/kuroboxHD.dts > index 2e5a1a1..8d725d1 100644 > --- a/arch/powerpc/boot/dts/kuroboxHD.dts > +++ b/arch/powerpc/boot/dts/kuroboxHD.dts > @@ -76,7 +76,6 @@ XXXX add flash parts, rtc, ?? > interrupt-parent = <&mpic>; > > rtc@32 { > - device_type = "rtc"; > compatible = "ricoh,rs5c372a"; > reg = <0x32>; > }; > diff --git a/arch/powerpc/boot/dts/kuroboxHG.dts b/arch/powerpc/boot/dts/kuroboxHG.dts > index e4916e6..b13a11e 100644 > --- a/arch/powerpc/boot/dts/kuroboxHG.dts > +++ b/arch/powerpc/boot/dts/kuroboxHG.dts > @@ -76,7 +76,6 @@ XXXX add flash parts, rtc, ?? > interrupt-parent = <&mpic>; > > rtc@32 { > - device_type = "rtc"; > compatible = "ricoh,rs5c372a"; > reg = <0x32>; > }; > diff --git a/arch/powerpc/boot/dts/lite5200.dts b/arch/powerpc/boot/dts/lite5200.dts > index 2cf9a87..3f7a5dc 100644 > --- a/arch/powerpc/boot/dts/lite5200.dts > +++ b/arch/powerpc/boot/dts/lite5200.dts > @@ -130,7 +130,6 @@ > > rtc@800 { // Real time clock > compatible = "fsl,mpc5200-rtc"; > - device_type = "rtc"; > reg = <0x800 0x100>; > interrupts = <1 5 0 1 6 0>; > interrupt-parent = <&mpc5200_pic>; > diff --git a/arch/powerpc/boot/dts/lite5200b.dts b/arch/powerpc/boot/dts/lite5200b.dts > index 7bd5b9c..63e3bb4 100644 > --- a/arch/powerpc/boot/dts/lite5200b.dts > +++ b/arch/powerpc/boot/dts/lite5200b.dts > @@ -130,7 +130,6 @@ > > rtc@800 { // Real time clock > compatible = "fsl,mpc5200b-rtc","fsl,mpc5200-rtc"; > - device_type = "rtc"; > reg = <0x800 0x100>; > interrupts = <1 5 0 1 6 0>; > interrupt-parent = <&mpc5200_pic>; > diff --git a/arch/powerpc/boot/dts/motionpro.dts b/arch/powerpc/boot/dts/motionpro.dts > index 9e3c921..52ba6f9 100644 > --- a/arch/powerpc/boot/dts/motionpro.dts > +++ b/arch/powerpc/boot/dts/motionpro.dts > @@ -248,7 +248,6 @@ > fsl5200-clocking; > > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > }; > diff --git a/arch/powerpc/boot/dts/mpc8315erdb.dts b/arch/powerpc/boot/dts/mpc8315erdb.dts > index 6b85067..d3d3097 100644 > --- a/arch/powerpc/boot/dts/mpc8315erdb.dts > +++ b/arch/powerpc/boot/dts/mpc8315erdb.dts > @@ -117,7 +117,6 @@ > interrupt-parent = <&ipic>; > dfsrr; > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > }; > diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts > index 2c9d54a..d86c6a3 100644 > --- a/arch/powerpc/boot/dts/mpc8349emitx.dts > +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts > @@ -85,7 +85,6 @@ > dfsrr; > > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > interrupts = <18 0x8>; > diff --git a/arch/powerpc/boot/dts/mpc8349emitxgp.dts b/arch/powerpc/boot/dts/mpc8349emitxgp.dts > index fa40647..fd4bbc4 100644 > --- a/arch/powerpc/boot/dts/mpc8349emitxgp.dts > +++ b/arch/powerpc/boot/dts/mpc8349emitxgp.dts > @@ -83,7 +83,6 @@ > dfsrr; > > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > interrupts = <18 0x8>; > diff --git a/arch/powerpc/boot/dts/mpc8377_rdb.dts b/arch/powerpc/boot/dts/mpc8377_rdb.dts > index 435ef3d..9fe8e4c 100644 > --- a/arch/powerpc/boot/dts/mpc8377_rdb.dts > +++ b/arch/powerpc/boot/dts/mpc8377_rdb.dts > @@ -117,7 +117,6 @@ > interrupt-parent = <&ipic>; > dfsrr; > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > }; > diff --git a/arch/powerpc/boot/dts/mpc8378_rdb.dts b/arch/powerpc/boot/dts/mpc8378_rdb.dts > index b11e68f..3a6d528 100644 > --- a/arch/powerpc/boot/dts/mpc8378_rdb.dts > +++ b/arch/powerpc/boot/dts/mpc8378_rdb.dts > @@ -117,7 +117,6 @@ > interrupt-parent = <&ipic>; > dfsrr; > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > }; > diff --git a/arch/powerpc/boot/dts/mpc8379_rdb.dts b/arch/powerpc/boot/dts/mpc8379_rdb.dts > index 337af6e..ee64def 100644 > --- a/arch/powerpc/boot/dts/mpc8379_rdb.dts > +++ b/arch/powerpc/boot/dts/mpc8379_rdb.dts > @@ -117,7 +117,6 @@ > interrupt-parent = <&ipic>; > dfsrr; > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1339"; > reg = <0x68>; > }; > diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts > index 7c1bb95..be2c11c 100644 > --- a/arch/powerpc/boot/dts/pcm030.dts > +++ b/arch/powerpc/boot/dts/pcm030.dts > @@ -143,7 +143,6 @@ > > rtc@800 { // Real time clock > compatible = "fsl,mpc5200b-rtc","fsl,mpc5200-rtc"; > - device_type = "rtc"; > reg = <0x800 0x100>; > interrupts = <0x1 0x5 0x0 0x1 0x6 0x0>; > interrupt-parent = <&mpc5200_pic>; > @@ -301,7 +300,6 @@ > interrupt-parent = <&mpc5200_pic>; > fsl5200-clocking; > rtc@51 { > - device_type = "rtc"; > compatible = "nxp,pcf8563"; > reg = <0x51>; > }; > diff --git a/arch/powerpc/boot/dts/tqm5200.dts b/arch/powerpc/boot/dts/tqm5200.dts > index 3008bf8..906302e 100644 > --- a/arch/powerpc/boot/dts/tqm5200.dts > +++ b/arch/powerpc/boot/dts/tqm5200.dts > @@ -181,7 +181,6 @@ > fsl5200-clocking; > > rtc@68 { > - device_type = "rtc"; > compatible = "dallas,ds1307"; > reg = <0x68>; > }; > -- > 1.5.6.3 >
On Tue, Oct 21, 2008 at 10:58:44PM +0400, Anton Vorontsov wrote: > We don't want to encourage the device_type usage. It isn't used in the > code, so we can simply remove it from the dts files. > > Suggested-by: Scott Wood <scottwood@freescale.com> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Acked-by: David Gibson <david@gibson.dropbear.id.au>
I'm extremely troubled that it is "not used in the code", surely device_type is checked as a legacy for Open Firmware (otherwise a lot of devices may never be detected!), or does device tree parsing/checking follow a different path for FDT? (absolutely fine with it being removed from new DTS but, just concerned about your comment and it's impact...)
On Wed, Oct 22, 2008 at 08:38:03AM -0500, Matt Sealey wrote: > I'm extremely troubled that it is "not used in the code", surely > device_type is checked as a legacy for Open Firmware (otherwise a lot of > devices may never be detected!), Checked where? Can you point out a code snippet? (Except for CHRP platforms, the CHRP is running real OF, so it is irrelevant if it checks for device_type = "rtc" or not.) > or does device tree parsing/checking > follow a different path for FDT? > > (absolutely fine with it being removed from new DTS but, just concerned > about your comment and it's impact...)
Anton Vorontsov wrote: > On Wed, Oct 22, 2008 at 08:38:03AM -0500, Matt Sealey wrote: >> I'm extremely troubled that it is "not used in the code", surely >> device_type is checked as a legacy for Open Firmware (otherwise a lot of >> devices may never be detected!), > > Checked where? of_find_device_by_type() goes through it and of_find_compatible_node() - while not directly, checks the type field of the node which is filled in from device_type BEFORE it checks the compatible property. This isn't specific CHRP/FDT platform code it's in the generic tree scanning functions. > platforms, the CHRP is running real OF, so it is irrelevant if it > checks for device_type = "rtc" or not.) Except when there is no compatible property because the device is adequately identified by device_type (for instance if you are looking for every serial port on a system, device_type is your only hope without checking for every different KIND of serial port you may ever possibly encounter.. or if you are checking for what kind of console OF attached you to (be it display/keyboard combination or serial or something else). It's a useful property that should be kept around, you never know when it might be needed :D but of course since it's not in ePAPR, no point in shoving it in when "compatible" describes your board very very accurately anyway (except when you have no need to be very very accurate and are just looking for serial ports... :)
On Wed, Oct 22, 2008 at 01:40:50PM -0500, Matt Sealey wrote: > > > Anton Vorontsov wrote: >> On Wed, Oct 22, 2008 at 08:38:03AM -0500, Matt Sealey wrote: >>> I'm extremely troubled that it is "not used in the code", surely >>> device_type is checked as a legacy for Open Firmware (otherwise a lot >>> of devices may never be detected!), >> >> Checked where? > > of_find_device_by_type() goes through it and of_find_compatible_node() - > while not directly, checks the type field of the node which is filled in > from device_type BEFORE it checks the compatible property. Yes, I know this. > This isn't specific CHRP/FDT platform code it's in the generic tree > scanning functions. I think I got it. ;-) You think that I'm not aware of that we _can_ use the device_type for matching the nodes. Well, I'm aware of it, sure we can. ;-) But we don't use it for the rtc nodes, and we don't want to encourage the usage for the flat trees. And that's the point of this patch. Thanks,
Anton Vorontsov wrote: > On Wed, Oct 22, 2008 at 01:40:50PM -0500, Matt Sealey wrote: > > I think I got it. ;-) You think that I'm not aware of that we _can_ > use the device_type for matching the nodes. Well, I'm aware of it, > sure we can. ;-) I'm sure you are aware, I am just a little jumpy regarding this as the whole ePAPR-is-official thing and the direction Linux is taking with regards to redefining part of the device tree specs, that this could have been something a little more serious :) > But we don't use it for the rtc nodes, and we don't want to encourage > the usage for the flat trees. And that's the point of this patch. Would it not be prudent to, while not actively encouraging it, at least mention device_type in any specifications as a legacy item (for real Open Firmware only) and for if a device should be in the tree as a generic, IEEE 1275-style device (i.e. there would be a set of well-defined client interface methods for it in a real OF)? My basic concerns are for input/output as reported by /chosen - in case it is important exactly what is being used, there is at least one out-of-driver code snippet which checks if stdin and stdout are of type "serial" (or "failsafe") and auto-directs console to that - it would be nice to keep this clean and not dump a million serial-device-compatibles in another list here if someone wants to automatically choose between console output on the DIU or PSC for MPC5121e/MPC8610 for example, or wants to restrict the amount of fancy stuff it does on a terminal if it's a slow serial device, or perhaps even automatically invoke netconsole if it's set to "network"? I know U-Boot doesn't have the intelligence to output to anything but a serial port these days on those devices, but as they say, there is no fate but what we make .. we should make sure it doesn't turn up that code is never suggested or attempted because supporting it in Linux would be too big a jump or too messy a patch :)
On Wed, Oct 22, 2008 at 02:09:01PM -0500, Matt Sealey wrote: > Anton Vorontsov wrote: >> On Wed, Oct 22, 2008 at 01:40:50PM -0500, Matt Sealey wrote: >> >> I think I got it. ;-) You think that I'm not aware of that we _can_ >> use the device_type for matching the nodes. Well, I'm aware of it, >> sure we can. ;-) > > I'm sure you are aware, I am just a little jumpy regarding this as the > whole ePAPR-is-official thing and the direction Linux is taking with > regards to redefining part of the device tree specs, that this could have > been something a little more serious :) > >> But we don't use it for the rtc nodes, and we don't want to encourage >> the usage for the flat trees. And that's the point of this patch. > > Would it not be prudent to, while not actively encouraging it, at least > mention device_type in any specifications as a legacy item (for real Open > Firmware only) and for if a device should be in the tree as a generic, > IEEE 1275-style device (i.e. there would be a set of well-defined client > interface methods for it in a real OF)? > > My basic concerns are for input/output as reported by /chosen - in case > it is important exactly what is being used, there is at least one > out-of-driver code snippet which checks if stdin and stdout are of type > "serial" (or "failsafe") and auto-directs console to that - it would be > nice to keep this clean and not dump a million serial-device-compatibles > in another list here if someone wants to automatically choose between > console output on the DIU or PSC for MPC5121e/MPC8610 for example, or > wants to restrict the amount of fancy stuff it does on a terminal if it's > a slow serial device, or perhaps even automatically invoke netconsole if > it's set to "network"? > > I know U-Boot doesn't have the intelligence to output to anything but a > serial port these days on those devices, but as they say, there is no > fate but what we make .. we should make sure it doesn't turn up that code > is never suggested or attempted because supporting it in Linux would be > too big a jump or too messy a patch :) I don't feel competent to comment on "embedded-OF"/FDT design decisions... Let's Cc devicetree-discuss@ozlabs.org ?
diff --git a/arch/powerpc/boot/dts/kuroboxHD.dts b/arch/powerpc/boot/dts/kuroboxHD.dts index 2e5a1a1..8d725d1 100644 --- a/arch/powerpc/boot/dts/kuroboxHD.dts +++ b/arch/powerpc/boot/dts/kuroboxHD.dts @@ -76,7 +76,6 @@ XXXX add flash parts, rtc, ?? interrupt-parent = <&mpic>; rtc@32 { - device_type = "rtc"; compatible = "ricoh,rs5c372a"; reg = <0x32>; }; diff --git a/arch/powerpc/boot/dts/kuroboxHG.dts b/arch/powerpc/boot/dts/kuroboxHG.dts index e4916e6..b13a11e 100644 --- a/arch/powerpc/boot/dts/kuroboxHG.dts +++ b/arch/powerpc/boot/dts/kuroboxHG.dts @@ -76,7 +76,6 @@ XXXX add flash parts, rtc, ?? interrupt-parent = <&mpic>; rtc@32 { - device_type = "rtc"; compatible = "ricoh,rs5c372a"; reg = <0x32>; }; diff --git a/arch/powerpc/boot/dts/lite5200.dts b/arch/powerpc/boot/dts/lite5200.dts index 2cf9a87..3f7a5dc 100644 --- a/arch/powerpc/boot/dts/lite5200.dts +++ b/arch/powerpc/boot/dts/lite5200.dts @@ -130,7 +130,6 @@ rtc@800 { // Real time clock compatible = "fsl,mpc5200-rtc"; - device_type = "rtc"; reg = <0x800 0x100>; interrupts = <1 5 0 1 6 0>; interrupt-parent = <&mpc5200_pic>; diff --git a/arch/powerpc/boot/dts/lite5200b.dts b/arch/powerpc/boot/dts/lite5200b.dts index 7bd5b9c..63e3bb4 100644 --- a/arch/powerpc/boot/dts/lite5200b.dts +++ b/arch/powerpc/boot/dts/lite5200b.dts @@ -130,7 +130,6 @@ rtc@800 { // Real time clock compatible = "fsl,mpc5200b-rtc","fsl,mpc5200-rtc"; - device_type = "rtc"; reg = <0x800 0x100>; interrupts = <1 5 0 1 6 0>; interrupt-parent = <&mpc5200_pic>; diff --git a/arch/powerpc/boot/dts/motionpro.dts b/arch/powerpc/boot/dts/motionpro.dts index 9e3c921..52ba6f9 100644 --- a/arch/powerpc/boot/dts/motionpro.dts +++ b/arch/powerpc/boot/dts/motionpro.dts @@ -248,7 +248,6 @@ fsl5200-clocking; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; }; diff --git a/arch/powerpc/boot/dts/mpc8315erdb.dts b/arch/powerpc/boot/dts/mpc8315erdb.dts index 6b85067..d3d3097 100644 --- a/arch/powerpc/boot/dts/mpc8315erdb.dts +++ b/arch/powerpc/boot/dts/mpc8315erdb.dts @@ -117,7 +117,6 @@ interrupt-parent = <&ipic>; dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; }; diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts index 2c9d54a..d86c6a3 100644 --- a/arch/powerpc/boot/dts/mpc8349emitx.dts +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts @@ -85,7 +85,6 @@ dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; interrupts = <18 0x8>; diff --git a/arch/powerpc/boot/dts/mpc8349emitxgp.dts b/arch/powerpc/boot/dts/mpc8349emitxgp.dts index fa40647..fd4bbc4 100644 --- a/arch/powerpc/boot/dts/mpc8349emitxgp.dts +++ b/arch/powerpc/boot/dts/mpc8349emitxgp.dts @@ -83,7 +83,6 @@ dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; interrupts = <18 0x8>; diff --git a/arch/powerpc/boot/dts/mpc8377_rdb.dts b/arch/powerpc/boot/dts/mpc8377_rdb.dts index 435ef3d..9fe8e4c 100644 --- a/arch/powerpc/boot/dts/mpc8377_rdb.dts +++ b/arch/powerpc/boot/dts/mpc8377_rdb.dts @@ -117,7 +117,6 @@ interrupt-parent = <&ipic>; dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; }; diff --git a/arch/powerpc/boot/dts/mpc8378_rdb.dts b/arch/powerpc/boot/dts/mpc8378_rdb.dts index b11e68f..3a6d528 100644 --- a/arch/powerpc/boot/dts/mpc8378_rdb.dts +++ b/arch/powerpc/boot/dts/mpc8378_rdb.dts @@ -117,7 +117,6 @@ interrupt-parent = <&ipic>; dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; }; diff --git a/arch/powerpc/boot/dts/mpc8379_rdb.dts b/arch/powerpc/boot/dts/mpc8379_rdb.dts index 337af6e..ee64def 100644 --- a/arch/powerpc/boot/dts/mpc8379_rdb.dts +++ b/arch/powerpc/boot/dts/mpc8379_rdb.dts @@ -117,7 +117,6 @@ interrupt-parent = <&ipic>; dfsrr; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1339"; reg = <0x68>; }; diff --git a/arch/powerpc/boot/dts/pcm030.dts b/arch/powerpc/boot/dts/pcm030.dts index 7c1bb95..be2c11c 100644 --- a/arch/powerpc/boot/dts/pcm030.dts +++ b/arch/powerpc/boot/dts/pcm030.dts @@ -143,7 +143,6 @@ rtc@800 { // Real time clock compatible = "fsl,mpc5200b-rtc","fsl,mpc5200-rtc"; - device_type = "rtc"; reg = <0x800 0x100>; interrupts = <0x1 0x5 0x0 0x1 0x6 0x0>; interrupt-parent = <&mpc5200_pic>; @@ -301,7 +300,6 @@ interrupt-parent = <&mpc5200_pic>; fsl5200-clocking; rtc@51 { - device_type = "rtc"; compatible = "nxp,pcf8563"; reg = <0x51>; }; diff --git a/arch/powerpc/boot/dts/tqm5200.dts b/arch/powerpc/boot/dts/tqm5200.dts index 3008bf8..906302e 100644 --- a/arch/powerpc/boot/dts/tqm5200.dts +++ b/arch/powerpc/boot/dts/tqm5200.dts @@ -181,7 +181,6 @@ fsl5200-clocking; rtc@68 { - device_type = "rtc"; compatible = "dallas,ds1307"; reg = <0x68>; };
We don't want to encourage the device_type usage. It isn't used in the code, so we can simply remove it from the dts files. Suggested-by: Scott Wood <scottwood@freescale.com> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- arch/powerpc/boot/dts/kuroboxHD.dts | 1 - arch/powerpc/boot/dts/kuroboxHG.dts | 1 - arch/powerpc/boot/dts/lite5200.dts | 1 - arch/powerpc/boot/dts/lite5200b.dts | 1 - arch/powerpc/boot/dts/motionpro.dts | 1 - arch/powerpc/boot/dts/mpc8315erdb.dts | 1 - arch/powerpc/boot/dts/mpc8349emitx.dts | 1 - arch/powerpc/boot/dts/mpc8349emitxgp.dts | 1 - arch/powerpc/boot/dts/mpc8377_rdb.dts | 1 - arch/powerpc/boot/dts/mpc8378_rdb.dts | 1 - arch/powerpc/boot/dts/mpc8379_rdb.dts | 1 - arch/powerpc/boot/dts/pcm030.dts | 2 -- arch/powerpc/boot/dts/tqm5200.dts | 1 - 13 files changed, 0 insertions(+), 14 deletions(-)