Date:	Wed, 20 Jun 2012 09:56:22 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
CC:	Stephen Warren <swarren@...dotorg.org>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"arnd@...db.de" <arnd@...db.de>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"lrg@...com" <lrg@...com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 3/3] ARM: dts: db8500: add node property "regulator-compatible"
 regulator node

On 20/06/12 09:19, Laxman Dewangan wrote:
> On Wednesday 20 June 2012 01:31 PM, Lee Jones wrote:
>> On 20/06/12 08:39, Laxman Dewangan wrote:
>>> On Wednesday 20 June 2012 12:39 PM, Lee Jones wrote:
>>>> On 19/06/12 18:32, Stephen Warren wrote:
>>>>> On 06/19/2012 10:13 AM, Lee Jones wrote:
>>>>>>> On 19/06/12 15:28, Laxman Dewangan wrote:
>>>>>>>>> Device's regulator matches their hardware counterparts with the
>>>>>>>>> property "regulator-compatible" of each child regulator node in
>>>>>>>>> place of the child node.
>>>>>>>>> Add the property "regulator-compatible" for each regulator with
>>>>>>>>> their name.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Laxman Dewangan<ldewangan@...dia.com>
>>>>>>>>> ---
>>>>>>>>> Changes from V1:
>>>>>>>>> - This is new change in V2.
>>>>>>>>>
>>>>>>>>> arch/arm/boot/dts/db8500.dtsi | 128
>>>>>>>>> +++++++++++++++++++++++++++++++----------
>>>>>>>>> 1 files changed, 97 insertions(+), 31 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> index 4ad5160..9548f80 100644
>>>>>>>>> --- a/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> +++ b/arch/arm/boot/dts/db8500.dtsi
>>>>>>>>> @@ -203,107 +203,149 @@
>>>>>>>>>
>>>>>>>>> db8500-prcmu-regulators {
>>>>>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>>>>> + #address-cells =<1>;
>>>>>>>>> + #size-cells =<0>;
>>>>>>> Why are these and the reg properties required?
>>>>> DT nodes should be named after the type of object they describe (e.g.
>>>>> "regulator") rather than the name of the object they're describing
>>>>> (e.g.
>>>>> "vape").
>>>>>
>>>>> Once you've made that change, you end up with many nodes with the same
>>>>> name in the same parent, so you need to make their names unique.
>>>>> You do
>>>>> this by adding a "unit address" to each of them - "@0", "@1", ... But,
>>>>> in order to be "allowed" to use such a unit address, you need a reg
>>>>> property that matches the unit address, and #address-cells/#size-cells
>>>>> in the parent node.
>>>> I don't like it. By doing this you are preventing any regulator from
>>>> being registered by of_platform_populate(). Also, the nodes are already
>>>> placed under an identifying node "db8500-prcmu-regulators", so we know
>>>> they are regulators, making the regulator@x, the reg property and the
>>>> *-cells properties unnecessary cruft.
>>>>
>>>> I'd prefer to have the second label removed and just to call the
>>>> regulators by their correct name. The property names become
>>>> functionally
>>>> redundant after the previous patch has been applied in any case.
>>>>
>>>> Something like this:
>>>>
>>>>> db8500-prcmu-regulators {
>>>>> compatible = "stericsson,db8500-prcmu-regulator";
>>>>>
>>>>> // DB8500_REGULATOR_VAPE
>>>>> - db8500_vape_reg: db8500_vape {
>>>>> + db8500_vape {
>>>>> + regulator-compatible = "db8500_vape";
>>>>> regulator-name = "db8500-vape";
>>>>> regulator-always-on;
>>>>> };
>>>
>>> You will require a label so that it can refer by the consumer.
>> Don't they both act as labels? Thus if you removed the second one, the
>> phandle will be taken from the remaining label? It's not something I've
>> tried, so I'm happy to be wrong here.
>>
>> If I'm wrong about that, can't we just omit the reg and *-size
>> properties? They are meaningless and restrictive.
>>
>
> We need to have the label. The name can not act as label. I tried
> following and got compilation error for dts file.
> Tried following way
>
> reg_vdd1 {
> regulator-compatible = "vdd1";
> :::::::::::::::::
> };
>
> reg_vdd2: regulator@1 {
> regulator-compatible = "vdd2";
> :::::::::::::::::
> };
>
> reg_vddctrl: regulator@2 {
> regulator-compatible = "vddctrl";
> :::::::::::::::::
> vin-supply = <&reg_vdd1>;
> };
>
> And got build error as
> **********
> DTC: dts->dtb on file "arch/arm/boot/dts/tegra30-cardhu.dts"
> ERROR (phandle_references): Reference to non-existent node or label
> "reg_vdd1"
>
> ERROR: Input tree has errors, aborting (use -f to force output)
> *******

Yes, I just did the same experiment. Shame. :(

> Are you OK to have the first patch with adding property
> "regulator-compatible" on each of child node so that I can go ahead with
> this patch and regulator core/documentation patch along with changes in
> my board to enable regulators.
> Once we will conclude on the child name either like vdd1 or regulator@0,
> we can have modification accordingly.

Yes I'm fine with it.

It's only the unnecessary reg and *-size properties I'm opposed to.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists