| Message ID | 1378982427-5802-3-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mark, On Thursday 12 of September 2013 11:40:27 Mark Brown wrote: > From: Mark Brown <broonie@linaro.org> > > Ensure that unused I2C controllers are not activated, causing problems > due to inappropriate pinmuxing or similar, by marking the controllers as > disabled by default and requiring boards to explicitly enable those that > are in use. > > Signed-off-by: Mark Brown <broonie@linaro.org> > --- > arch/arm/boot/dts/exynos5250-arndale.dts | 31 > ++----------------------------- > arch/arm/boot/dts/exynos5250-smdk5250.dts | 25 +++++-------------------- > arch/arm/boot/dts/exynos5250-snow.dts | 4 ++++ > arch/arm/boot/dts/exynos5250.dtsi | 10 ++++++++++ > 4 files changed, 21 insertions(+), 49 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts > b/arch/arm/boot/dts/exynos5250-arndale.dts index 4687fa0..4c10901 100644 > --- a/arch/arm/boot/dts/exynos5250-arndale.dts > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts > @@ -31,6 +31,7 @@ > }; > > i2c@12C60000 { > + status = "okay"; I would keep the status properties at the end of all properties, indicating that at this point the description is complete and also to be consistent with other dts files around. > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <20000>; > samsung,i2c-slave-addr = <0x66>; > @@ -319,16 +320,8 @@ > }; > }; > > - i2c@12C70000 { > - status = "disabled"; > - }; > - > - i2c@12C80000 { > - status = "disabled"; > - }; > - > i2c@12C90000 { > - wm1811a@1a { > + status = "okay"; Hmm, I sense something wrong going on here. > compatible = "wlf,wm1811"; > reg = <0x1a>; > > @@ -346,26 +339,6 @@ > }; > }; > > - i2c@12CA0000 { > - status = "disabled"; > - }; > - > - i2c@12CB0000 { > - status = "disabled"; > - }; > - > - i2c@12CC0000 { > - status = "disabled"; > - }; > - > - i2c@12CD0000 { > - status = "disabled"; > - }; > - > - i2c@121D0000 { > - status = "disabled"; > - }; > - > dwmmc_0: dwmmc0@12200000 { > num-slots = <1>; > supports-highspeed; > diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts > b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 67d3c43..90127e3 > 100644 > --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts > +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts > @@ -28,6 +28,7 @@ > }; > > i2c@12C60000 { > + status = "okay"; > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <20000>; > > @@ -62,6 +63,7 @@ > }; > > i2c@12C70000 { > + status = "okay"; > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <20000>; > > @@ -86,6 +88,7 @@ > }; > > i2c@121D0000 { > + status = "okay"; > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <40000>; > samsung,i2c-slave-addr = <0x38>; > @@ -101,6 +104,7 @@ > }; > > i2c@12C80000 { > + status = "okay"; > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <66000>; > > @@ -110,27 +114,8 @@ > }; > }; > > - i2c@12C90000 { > - status = "disabled"; > - }; > - > - i2c@12CA0000 { > - status = "disabled"; > - }; > - > - i2c@12CB0000 { > - status = "disabled"; > - }; > - > - i2c@12CC0000 { > - status = "disabled"; > - }; > - > - i2c@12CD0000 { > - status = "disabled"; > - }; > - > i2c@12CE0000 { > + status = "okay"; > samsung,i2c-sda-delay = <100>; > samsung,i2c-max-bus-freq = <66000>; > > diff --git a/arch/arm/boot/dts/exynos5250-snow.dts > b/arch/arm/boot/dts/exynos5250-snow.dts index fd711e2..b7674d5 100644 > --- a/arch/arm/boot/dts/exynos5250-snow.dts > +++ b/arch/arm/boot/dts/exynos5250-snow.dts > @@ -48,6 +48,10 @@ > }; > }; > > + i2c@12CA0000 { > + status = "okay"; Hmm, no other properties needed here? > + }; > + > i2c-arbitrator { > compatible = "i2c-arb-gpio-challenge"; > #address-cells = <1>; > diff --git a/arch/arm/boot/dts/exynos5250.dtsi > b/arch/arm/boot/dts/exynos5250.dtsi index a886ee4..4d6312e 100644 > --- a/arch/arm/boot/dts/exynos5250.dtsi > +++ b/arch/arm/boot/dts/exynos5250.dtsi > @@ -229,6 +229,7 @@ > > i2c_0: i2c@12C60000 { > compatible = "samsung,s3c2440-i2c"; > + status = "disabled"; For the sake of consistency, I would place the status property as last here as well. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 12, 2013 at 02:58:59PM +0200, Tomasz Figa wrote: > On Thursday 12 of September 2013 11:40:27 Mark Brown wrote: Please delete unneeded context from your mails, it makes it much easier to find the new content. > > + i2c@12CA0000 { > > + status = "okay"; > Hmm, no other properties needed here? Apparently not, it looks like the binding for the aribtrator doesn't sit as a child of the bus it's arbitrating - this is the controller referenced from the arbitrator node.
On 09/12/2013 06:58 AM, Tomasz Figa wrote: > Hi Mark, > > On Thursday 12 of September 2013 11:40:27 Mark Brown wrote: >> From: Mark Brown <broonie@linaro.org> >> >> Ensure that unused I2C controllers are not activated, causing problems >> due to inappropriate pinmuxing or similar, by marking the controllers as >> disabled by default and requiring boards to explicitly enable those that >> are in use. >> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts >> i2c@12C60000 { >> + status = "okay"; > > I would keep the status properties at the end of all properties, indicating > that at this point the description is complete and also to be consistent > with other dts files around. DT doesn't define any kind of ordering for the properties AFAIK, so the order shouldn't matter in practice; it conveys no semantic representation as far as the parsing code is concerned even if a human may be influenced otherwise:-) Just as an FYI, the rule I've been trying to follow in Tegra DT files is that properties that are overridden from any included .dtsi file come first, followed by any new properties. Still, there's no particular reason anyone else has to follow that layout. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 12 of September 2013 14:52:33 Mark Brown wrote: > On Thu, Sep 12, 2013 at 02:58:59PM +0200, Tomasz Figa wrote: > > On Thursday 12 of September 2013 11:40:27 Mark Brown wrote: > Please delete unneeded context from your mails, it makes it much easier > to find the new content. I always snip unnecessary parts of quoted message if its really long, but if its length is in acceptable range then I try to keep the whole message just for reference. > > > + i2c@12CA0000 { > > > + status = "okay"; > > > > Hmm, no other properties needed here? > > Apparently not, it looks like the binding for the aribtrator doesn't sit > as a child of the bus it's arbitrating - this is the controller > referenced from the arbitrator node. i2c@12CA0000 does look like a normal hardware I2C controller, which needs some extra properties like samsung,i2c-sda-delay or samsung,i2c-max-bus- freq. However they are specified in cros5250-common.dtsi, so I'd say that this might be better place for the status override as well. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 12 of September 2013 09:03:23 Stephen Warren wrote: > On 09/12/2013 06:58 AM, Tomasz Figa wrote: > > Hi Mark, > > > > On Thursday 12 of September 2013 11:40:27 Mark Brown wrote: > >> From: Mark Brown <broonie@linaro.org> > >> > >> Ensure that unused I2C controllers are not activated, causing problems > >> due to inappropriate pinmuxing or similar, by marking the controllers > >> as > >> disabled by default and requiring boards to explicitly enable those > >> that > >> are in use. > >> > >> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts > >> > >> i2c@12C60000 { > >> > >> + status = "okay"; > > > > I would keep the status properties at the end of all properties, > > indicating that at this point the description is complete and also to > > be consistent with other dts files around. > > DT doesn't define any kind of ordering for the properties AFAIK, so the > order shouldn't matter in practice; it conveys no semantic > representation as far as the parsing code is concerned even if a human > may be influenced otherwise:-) Well, this is somehow similar to code, maybe not exactly in case of ordering, but coding style and conventions. Unreadable code can do exactly the same thing as readable code, while we somehow prefer the readable one. > Just as an FYI, the rule I've been trying to follow in Tegra DT files is > that properties that are overridden from any included .dtsi file come > first, followed by any new properties. Still, there's no particular > reason anyone else has to follow that layout. On Exynos I've been trying to keep status as the last property, compatible as first, followed by reg and without any specific order of any other properties. Again, this is simply to keep some consistence across the dtses making them easier to read. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 12, 2013 at 05:55:59PM +0200, Tomasz Figa wrote: > On Thursday 12 of September 2013 14:52:33 Mark Brown wrote: > > Please delete unneeded context from your mails, it makes it much easier > > to find the new content. > I always snip unnecessary parts of quoted message if its really long, but > if its length is in acceptable range then I try to keep the whole message > just for reference. I tend to say that when I find myself paging through entire screens of quote and can't see the relevance to any content I eventually find. > i2c@12CA0000 does look like a normal hardware I2C controller, which needs > some extra properties like samsung,i2c-sda-delay or samsung,i2c-max-bus- Shouldn't these have sane defaults? > freq. However they are specified in cros5250-common.dtsi, so I'd say that > this might be better place for the status override as well. Can we rename that file to something like exynos5250-cros-common.dtsi so that it's obvious it's relevant to exynos? I had completely failed to notice that it was anything to do with Exynos when I was searching for affected DTS files. Especially given that it's included in exactly one DTS at the minute...
On Thursday 12 of September 2013 21:00:13 Mark Brown wrote: > On Thu, Sep 12, 2013 at 05:55:59PM +0200, Tomasz Figa wrote: > > On Thursday 12 of September 2013 14:52:33 Mark Brown wrote: > > > Please delete unneeded context from your mails, it makes it much > > > easier > > > to find the new content. > > > > I always snip unnecessary parts of quoted message if its really long, > > but if its length is in acceptable range then I try to keep the whole > > message just for reference. > > I tend to say that when I find myself paging through entire screens of > quote and can't see the relevance to any content I eventually find. Hmm, looking at it here from my desktop, I could have dropped some of the context between second and third reply indeed. > > i2c@12CA0000 does look like a normal hardware I2C controller, which > > needs some extra properties like samsung,i2c-sda-delay or > > samsung,i2c-max-bus- > Shouldn't these have sane defaults? AFAIK those parameters are highly board specific so it would be hard to set any reasonable default for them. > > freq. However they are specified in cros5250-common.dtsi, so I'd say > > that this might be better place for the status override as well. > > Can we rename that file to something like exynos5250-cros-common.dtsi > so that it's obvious it's relevant to exynos? I had completely failed > to notice that it was anything to do with Exynos when I was searching > for affected DTS files. > > Especially given that it's included in exactly one DTS at the minute... Yes, it would surely make sense to do so, unless there is another cros board that could use this dtsi and is based on another SoC, which I doubt, because there are multiple SoC specific values specified inside it. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 13, 2013 at 01:02:11AM +0200, Tomasz Figa wrote: > On Thursday 12 of September 2013 21:00:13 Mark Brown wrote: > > > i2c@12CA0000 does look like a normal hardware I2C controller, which > > > needs some extra properties like samsung,i2c-sda-delay or > > > samsung,i2c-max-bus- > > Shouldn't these have sane defaults? > AFAIK those parameters are highly board specific so it would be hard to > set any reasonable default for them. At least the maximum bus frequency should be able to be set to 100Hz by default, that's the minimum from the I2C spec though pretty much everything does at least 400Hz. sda-delay should really be able to default to either zero or something per-SoC, I don't remember having to tune it when using board files and I2C is generally pretty robust so it'd be surprising if it were that sensitive to system design.
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts index 4687fa0..4c10901 100644 --- a/arch/arm/boot/dts/exynos5250-arndale.dts +++ b/arch/arm/boot/dts/exynos5250-arndale.dts @@ -31,6 +31,7 @@ }; i2c@12C60000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <20000>; samsung,i2c-slave-addr = <0x66>; @@ -319,16 +320,8 @@ }; }; - i2c@12C70000 { - status = "disabled"; - }; - - i2c@12C80000 { - status = "disabled"; - }; - i2c@12C90000 { - wm1811a@1a { + status = "okay"; compatible = "wlf,wm1811"; reg = <0x1a>; @@ -346,26 +339,6 @@ }; }; - i2c@12CA0000 { - status = "disabled"; - }; - - i2c@12CB0000 { - status = "disabled"; - }; - - i2c@12CC0000 { - status = "disabled"; - }; - - i2c@12CD0000 { - status = "disabled"; - }; - - i2c@121D0000 { - status = "disabled"; - }; - dwmmc_0: dwmmc0@12200000 { num-slots = <1>; supports-highspeed; diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts index 67d3c43..90127e3 100644 --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts @@ -28,6 +28,7 @@ }; i2c@12C60000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <20000>; @@ -62,6 +63,7 @@ }; i2c@12C70000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <20000>; @@ -86,6 +88,7 @@ }; i2c@121D0000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <40000>; samsung,i2c-slave-addr = <0x38>; @@ -101,6 +104,7 @@ }; i2c@12C80000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <66000>; @@ -110,27 +114,8 @@ }; }; - i2c@12C90000 { - status = "disabled"; - }; - - i2c@12CA0000 { - status = "disabled"; - }; - - i2c@12CB0000 { - status = "disabled"; - }; - - i2c@12CC0000 { - status = "disabled"; - }; - - i2c@12CD0000 { - status = "disabled"; - }; - i2c@12CE0000 { + status = "okay"; samsung,i2c-sda-delay = <100>; samsung,i2c-max-bus-freq = <66000>; diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts index fd711e2..b7674d5 100644 --- a/arch/arm/boot/dts/exynos5250-snow.dts +++ b/arch/arm/boot/dts/exynos5250-snow.dts @@ -48,6 +48,10 @@ }; }; + i2c@12CA0000 { + status = "okay"; + }; + i2c-arbitrator { compatible = "i2c-arb-gpio-challenge"; #address-cells = <1>; diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi index a886ee4..4d6312e 100644 --- a/arch/arm/boot/dts/exynos5250.dtsi +++ b/arch/arm/boot/dts/exynos5250.dtsi @@ -229,6 +229,7 @@ i2c_0: i2c@12C60000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12C60000 0x100>; interrupts = <0 56 0>; #address-cells = <1>; @@ -241,6 +242,7 @@ i2c_1: i2c@12C70000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12C70000 0x100>; interrupts = <0 57 0>; #address-cells = <1>; @@ -253,6 +255,7 @@ i2c_2: i2c@12C80000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12C80000 0x100>; interrupts = <0 58 0>; #address-cells = <1>; @@ -265,6 +268,7 @@ i2c_3: i2c@12C90000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12C90000 0x100>; interrupts = <0 59 0>; #address-cells = <1>; @@ -277,6 +281,7 @@ i2c_4: i2c@12CA0000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12CA0000 0x100>; interrupts = <0 60 0>; #address-cells = <1>; @@ -289,6 +294,7 @@ i2c_5: i2c@12CB0000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12CB0000 0x100>; interrupts = <0 61 0>; #address-cells = <1>; @@ -301,6 +307,7 @@ i2c_6: i2c@12CC0000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12CC0000 0x100>; interrupts = <0 62 0>; #address-cells = <1>; @@ -313,6 +320,7 @@ i2c_7: i2c@12CD0000 { compatible = "samsung,s3c2440-i2c"; + status = "disabled"; reg = <0x12CD0000 0x100>; interrupts = <0 63 0>; #address-cells = <1>; @@ -325,6 +333,7 @@ i2c_8: i2c@12CE0000 { compatible = "samsung,s3c2440-hdmiphy-i2c"; + status = "disabled"; reg = <0x12CE0000 0x1000>; interrupts = <0 64 0>; #address-cells = <1>; @@ -335,6 +344,7 @@ i2c@121D0000 { compatible = "samsung,exynos5-sata-phy-i2c"; + status = "disabled"; reg = <0x121D0000 0x100>; #address-cells = <1>; #size-cells = <0>;