| Message ID | 20120109005603.GA1835@S2101-09.ap.freescale.net |
|---|---|
| State | New |
| Headers | show |
On 01/08/2012 06:56 PM, Shawn Guo wrote: > On Sun, Jan 08, 2012 at 12:55:05PM -0800, Mark Brown wrote: >> On Sun, Jan 08, 2012 at 10:52:56PM +0800, Shawn Guo wrote: >>> On Fri, Jan 06, 2012 at 11:25:41AM +0800, Richard Zhao wrote: >> >>>> + VDDA-supply = <®_2P5V>; >>>> + VDDIO-supply = <®_3P3V>; >> >>> I would prefer to have them named vdda-supply and vddio-supply. But >>> I just learnt that they do not work, because sgtl5000 driver >>> (sound/soc/codecs/sgtl5000.c) has the supply_names in upper case, while >>> unlike of_node_cmp() is strcasecmp(), of_prop_cmp() is just strcmp(). >> >>> But the convention on property name is really all using lower case, >>> and mixing cases there looks odd, so I'm thinking about the changes >>> below on of_get_regulator(). >> >>> snprintf(prop_name, 32, "%s-supply", supply); >>> + while (prop_name[i] && i < 32) { >>> + prop_name[i] = tolower(prop_name[i]); >>> + i++; >>> + } >> >> There's two big problems here. One is that we clearly shouldn't be >> open coding this here but adding a function for it. The other is that >> this is going to break any existing device tree which has upper cased >> supplies. If we were going to do something here I'd go with case >> insensitve matching though I'm not sure it's a real problem. > > Ok, let's test device tree maintainers. > > Grant, Rob, > > Could the problem we are seeing here be a good reason to make the > following change? > > diff --git a/include/linux/of.h b/include/linux/of.h > index a75a831..c26c20f 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size) > /* Default string compare functions, Allow arch asm/prom.h to override */ > #if !defined(of_compat_cmp) > #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2)) > -#define of_prop_cmp(s1, s2) strcmp((s1), (s2)) > +#define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) > #define of_node_cmp(s1, s2) strcasecmp((s1), (s2)) > #endif Device-trees are case sensitive, so I don't think we want to go globally changing that behavior. If you want lower case names, then change the sgtl5000 code to lower case names. Rob
>> /* Default string compare functions, Allow arch asm/prom.h to override */ >> #if !defined(of_compat_cmp) >> #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2)) >> -#define of_prop_cmp(s1, s2) strcmp((s1), (s2)) >> +#define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) >> #define of_node_cmp(s1, s2) strcasecmp((s1), (s2)) >> #endif > > Device-trees are case sensitive, so I don't think we want to go globally > changing that behavior. > > If you want lower case names, then change the sgtl5000 code to lower > case names. +1, I'd vote for consistent lower case names.
On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote: > >> /* Default string compare functions, Allow arch asm/prom.h to override */ > >> #if !defined(of_compat_cmp) > >> #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2)) > >> -#define of_prop_cmp(s1, s2) strcmp((s1), (s2)) > >> +#define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) > >> #define of_node_cmp(s1, s2) strcasecmp((s1), (s2)) > >> #endif > > > > Device-trees are case sensitive, so I don't think we want to go globally > > changing that behavior. > > > > If you want lower case names, then change the sgtl5000 code to lower > > case names. > > +1, I'd vote for consistent lower case names. sgtl5000 is used by many other platforms too. I doubt why we do the big change just because it's convention or looks nice. As I said in other mails, I don't feel good about the lower case convention: - hw spec or sch may use upper case, lower case don't reflect exact. - the lower case rule might have to extend to other subsystems, for example, regulator. I bet it's not the only case when we get more and more DT bindings. Thanks Richard > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote: > > If you want lower case names, then change the sgtl5000 code to lower > > case names. > +1, I'd vote for consistent lower case names. That would be non-idiomatic for the regulators - the idiom for the regulator API is to use upper case names as that's the idiom used to label supplies in datasheets and schematics.
On Mon, Jan 09, 2012 at 02:52:40PM +0800, Shawn Guo wrote: > On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote: > > That would be non-idiomatic for the regulators - the idiom for the > > regulator API is to use upper case names as that's the idiom used to > > label supplies in datasheets and schematics. > Hmm, the idiom seems not so strong. > drivers/mmc/host/sdhci.c: > host->vmmc = regulator_get(mmc_dev(mmc), "vmmc"); It's purely an idiom, there's nothing enforcing it.
On Sun, Jan 08, 2012 at 09:38:39PM -0600, Rob Herring wrote: > On 01/08/2012 06:56 PM, Shawn Guo wrote: > > diff --git a/include/linux/of.h b/include/linux/of.h > > index a75a831..c26c20f 100644 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size) > > /* Default string compare functions, Allow arch asm/prom.h to override */ > > #if !defined(of_compat_cmp) > > #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2)) > > -#define of_prop_cmp(s1, s2) strcmp((s1), (s2)) > > +#define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) > > #define of_node_cmp(s1, s2) strcasecmp((s1), (s2)) > > #endif > > Device-trees are case sensitive, I do not believe this is true, as of_compat_cmp() and of_node_cmp() are clearly defined as strcasecmp() above. > so I don't think we want to go globally > changing that behavior. > > If you want lower case names, then change the sgtl5000 code to lower > case names. > I do not have a strong opinion about which is the one we should change among OF, regulator and sgtl5000. But I think it's the convention that we should keep property name in dts all lower case. I'm actually even fine with this odd mixing case name, if you tell that it's okay to override the convention for some cases.
On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote: > On Mon, Jan 09, 2012 at 01:05:39PM +0800, Eric Miao wrote: > > > > If you want lower case names, then change the sgtl5000 code to lower > > > case names. > > > +1, I'd vote for consistent lower case names. > > That would be non-idiomatic for the regulators - the idiom for the > regulator API is to use upper case names as that's the idiom used to > label supplies in datasheets and schematics. Hmm, the idiom seems not so strong. drivers/mmc/host/sdhci.c: host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
On Mon, Jan 09, 2012 at 03:17:16PM +0800, Shawn Guo wrote: > On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote: > > It's purely an idiom, there's nothing enforcing it. > Actually, a rough calculation on 'git grep regulator_get drivers/' > output tells 80% callers are using lower case. Add in the ASoC stuff (which I actually actively review) and I suspect the number changes. Anyway, there is a good solid reason for wanting capitals in the code so...
On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote: > On Mon, Jan 09, 2012 at 02:52:40PM +0800, Shawn Guo wrote: > > On Sun, Jan 08, 2012 at 10:25:12PM -0800, Mark Brown wrote: > > > > That would be non-idiomatic for the regulators - the idiom for the > > > regulator API is to use upper case names as that's the idiom used to > > > label supplies in datasheets and schematics. > > > Hmm, the idiom seems not so strong. > > > drivers/mmc/host/sdhci.c: > > > host->vmmc = regulator_get(mmc_dev(mmc), "vmmc"); > > It's purely an idiom, there's nothing enforcing it. Actually, a rough calculation on 'git grep regulator_get drivers/' output tells 80% callers are using lower case.
On Sun, Jan 08, 2012 at 11:12:56PM -0800, Mark Brown wrote: > On Mon, Jan 09, 2012 at 03:17:16PM +0800, Shawn Guo wrote: > > On Sun, Jan 08, 2012 at 10:43:29PM -0800, Mark Brown wrote: > > > > It's purely an idiom, there's nothing enforcing it. > > > Actually, a rough calculation on 'git grep regulator_get drivers/' > > output tells 80% callers are using lower case. > > Add in the ASoC stuff (which I actually actively review) and I suspect > the number changes. Anyway, there is a good solid reason for wanting > capitals in the code so... Shawn and Mark, Could you get agreement on it? I incline to not forcing upper case property names. Thanks Richard > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed, Jan 11, 2012 at 08:57:54AM +0800, Richard Zhao wrote: > Could you get agreement on it? > I incline to not forcing upper case property names. Personally I see no prolbme with the status quo.
diff --git a/include/linux/of.h b/include/linux/of.h index a75a831..c26c20f 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -147,7 +147,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size) /* Default string compare functions, Allow arch asm/prom.h to override */ #if !defined(of_compat_cmp) #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2)) -#define of_prop_cmp(s1, s2) strcmp((s1), (s2)) +#define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) #define of_node_cmp(s1, s2) strcasecmp((s1), (s2)) #endif