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

[Keerthy <a0393675@ti.com>]

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