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

Keerthy
2013-12-03T10:52:50Z

On Tuesday 03 December 2013 03:06 PM, Lee Jones wrote:
>> Add DT bindings for TPS65218 PMIC.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>> 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