Re: [PATCH V2 3/3] ARM: dts: db8500: add node property "regulator-compatible"regulator node

[Laxman Dewangan]

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@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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)
*******

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.

This will also need to avoid bi-sect issue as Stephen's suggested
patch 1: Add regulator-property on each child node of db8500.
patch 2: modify the regulator/core and documentation.

Patch3 and onwards: Based on discussion, name the child node.

Patch1 and 2 will not break anything and with this I can enable 
regulator on my boards.


Thanks,
Laxman


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/