Re: [PATCH 1/4] mfd: DT bindings for TPS65218 PMIC

[Keerthy]

On Tuesday 03 December 2013 03:06 PM, Lee Jones wrote:
> > Add DT bindings for TPS65218 PMIC.
> >
> > Signed-off-by: Keerthy <j-keerthy@xxxxxx>
> > ---
> >   Documentation/devicetree/bindings/mfd/tps65218.txt |   27 ++++++++++++++++++++
> >   .../devicetree/bindings/regulator/tps65218.txt     |   22 ++++++++++++++++
> >   2 files changed, 49 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/mfd/tps65218.txt
> >   create mode 100644 Documentation/devicetree/bindings/regulator/tps65218.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/tps65218.txt b/Documentation/devicetree/bindings/mfd/tps65218.txt
> > new file mode 100644
> > index 0000000..87cb7a8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/tps65218.txt
> > @@ -0,0 +1,27 @@
> > +The TPS65218 Integrated Power Management Chips.
> > +These chips are connected to an i2c bus.
>                                     I2C

Ok.

> > +Required properties:
> > +- compatible : Must be "ti,tps65218";
> > +- interrupts : This i2c device has an IRQ line connected to the main SoC
>                         I2C

Ok.

> > +- interrupt-controller : Since the tps65218 support several interrupts
>                                                 support(s) (hosts?)

Yes Supports multiple interrupts from the sub-modules.

> > +  internally, it is considered as an interrupt controller cascaded to the SoC.
> > +- #interrupt-cells = <2>;
> > +- interrupt-parent : The parent interrupt controller.
>             Phandle to ...
>
> > +Optional node:
>              node(s):

Ok.

> > +- Child nodes contain in the tps65218.
> > +  It supports a number of features.
> Please re-phase the above two sentences to something decipherable.

Ok.

> > +  The children nodes will thus depend of the capability of the variant.
> Please, go on ...

Ok.

> > +Example:
> > +/*
> > + * Integrated Power Management Chip
> > + */
> > +tps@24 {
> > +    compatible = "ti,tps65218";
> > +    reg = <0x24>;
> > +    interrupt-controller;
> > +    #interrupt-cells = <2>;
> > +    interrupt-parent = <&gic>;
> > +};
> Perhaps it would be better to centralise the documentation inclusive
> of the regulator contingent. Or at least provide a _full_ example in
> each document.

Ok.

> > diff --git a/Documentation/devicetree/bindings/regulator/tps65218.txt b/Documentation/devicetree/bindings/regulator/tps65218.txt
> > new file mode 100644
> > index 0000000..1ccf170
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/tps65218.txt
> > @@ -0,0 +1,22 @@
> > +TPS65218 family of regulators
> > +
> > +Required properties:
> > +For tps65218 regulators/LDOs
> > +- compatible:
> > +  - "ti,tps65218-dcdc1" for DCDC1
> > +  - "ti,tps65218-dcdc2" for DCDC2
> > +  - "ti,tps65218-dcdc3" for DCDC3
> > +  - "ti,tps65218-dcdc4" for DCDC4
> > +  - "ti,tps65218-dcdc5" for DCDC5
> > +  - "ti,tps65218-dcdc6" for DCDC6
> > +  - "ti,tps65218-ldo1" for LDO1 LDO
> Why aren't you using 'regulator-compatible'?

I referred tps65217.txt the predecessors and used the compatible.
I also referred twl-regulator.txt.

> > +Optional properties:
> > +- Any optional property defined in bindings/regulator/regulator.txt
> > +
> > +Example:
> > +	xyz: regulator@0 {
> Genuine question: Is the @0 meaningful?

@0 is not the best example. @24 is what i use on the am43x.
I will change it to @24.

> > +		compatible = "ti,tps65218-dcdc1";
> > +		regulator-min-microvolt  = <1000000>;
> > +		regulator-max-microvolt  = <3000000>;
> > +	};

Thanks for reviewing.

Kind Regards,
Keerthy
--
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/