diff mbox

[1/8] dt-bindings: pmic: Document Hi655x pmic driver

Message ID 1443611111-3196-2-git-send-email-w.f@huawei.com
State New, archived
Headers show

Commit Message

Wangfei (William, Euler) Sept. 30, 2015, 11:05 a.m. UTC
Document the new compatible for Hisilicon Hi655x pmic driver.

Signed-off-by: Fei Wang <w.f@huawei.com>
---
 .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |   80 ++++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt

Comments

Mark Brown Sept. 30, 2015, 5:39 p.m. UTC | #1
On Wed, Sep 30, 2015 at 07:05:04PM +0800, Fei Wang wrote:

> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
> +
> +hi655x consists of a large and varied group of sub-devices:
> +
> +Device			 Supply Names	 Description
> +------			 ------------	 -----------
> +hi655x-powerkey		:		: Powerkey
> +hi655x-regulator-pmic	:		: Regulators
> +hi655x-usbvbus		:		: USB plug detection
> +hi655x-pmu-rtc		:		: RTC
> +hi655x-coul		:		: Coulomb

...counter?

There's no documentation of the bindings for any of the above devices or
how things are structured, you need to provide binding documentation
which is understandable standalone.  None of the properties are
documented, nor is the set of regulators supported or how this is
structured.

> +                coul: coul@1 {
> +                        compatible = "hisilicon,hi655x-coul";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
> +                        battery_product_index = <0>;

For example, the "battery_product_index" property here is undocumented.
Lee Jones Oct. 1, 2015, 7:56 a.m. UTC | #2
On Wed, 30 Sep 2015, Fei Wang wrote:

> Document the new compatible for Hisilicon Hi655x pmic driver.

s/pmic/PMIC/

> Signed-off-by: Fei Wang <w.f@huawei.com>
> ---
>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |   80 ++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> new file mode 100644
> index 0000000..17bd8ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> @@ -0,0 +1,80 @@
> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
> +
> +hi655x consists of a large and varied group of sub-devices:
> +
> +Device			 Supply Names	 Description
> +------			 ------------	 -----------
> +hi655x-powerkey		:		: Powerkey
> +hi655x-regulator-pmic	:		: Regulators
> +hi655x-usbvbus		:		: USB plug detection
> +hi655x-pmu-rtc		:		: RTC
> +hi655x-coul		:		: Coulomb

As Mark said, these all need documentation.

> +Required properties:
> +- compatible : Should be "hisilicon,hi655x-pmic-driver"
> +- reg: Base address of PMIC on hi6220 soc
> +- #interrupt-cells: Should be 2, is the local IRQ number for hi655x.
> +- interrupt-controller: hi655x has internal IRQs (has own IRQ domain).
> +- pmu_irq_gpio: should be &gpio_pmu_irq_n, is the IRQ gpio of hi655x.

Better if you tab these out, like:

- compatible		: Should be "hisilicon,hi655x-pmic-driver"
- reg			: Base address of PMIC on hi6220 soc

Etc ...

> +Example:
> +	pmic: pmic@F8000000 {

s/F/f/

> +                compatible = "hisilicon,hi655x-pmic-driver";
> +                reg = <0x0 0xF8000000 0x0 0x1000>;

Small a => f please.

> +                #interrupt-cells = <2>;
> +                interrupt-controller;
> +                pmu_irq_gpio = <&gpio_pmu_irq_n>;

This should be <name>-gpios.

No underscores please.

> +                status = "ok";

Use "okay" instead.

> +                ponkey:ponkey@b1{

White space issues here.

Where is the 'reg' property?

> +                        compatible = "hisilicon,hi655x-powerkey";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <6 0>, <5 0>, <4 0>;

Use #defines (same goes for all below).

> +                        interrupt-names = "down", "up", "hold 1s";

White space in a name seems wrong to me.

> +                };
> +
> +                coul: coul@1 {

What is @1?

Is this label used?

> +                        compatible = "hisilicon,hi655x-coul";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
> +                        battery_product_index = <0>;

Documentation?  It doesn't look like a generic propriety either, so
should be have a vendor prefix.

> +                        status = "ok";

"okay"

> +                };
> +
> +                rtc: rtc@1 {

Is this label used?

> +                        compatible = "hisilicon,hi655x-pmu-rtc";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <20 0>;
> +                        interrupt-names = "hi655x_pmu_rtc";
> +                        board_id = <1>;

What's this?  No IDs in DT please.

> +                };
> +
> +                usbvbus:usbvbus@b2{

Whitespace.

'reg'?

> +                        compatible = "hisilicon,hi655x-usbvbus";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <9 0>, <8 0>;
> +                        interrupt-names = "connect", "disconnect";
> +                };
> +
> +		ldo2: regulator@a21 {

Tabbing seems wrong here.

> +                        compatible = "hisilicon,hi655x-regulator-pmic";
> +                        regulator-name = "ldo2";
> +                        regulator-min-microvolt = <2500000>;
> +                        regulator-max-microvolt = <3200000>;
> +                        hisilicon,valid-modes-mask = <0x02>;
> +                        hisilicon,valid-ops-mask = <0x01d>;
> +                        hisilicon,initial-mode = <0x02>;
> +                        hisilicon,regulator-type = <0x01>;
> +
> +                        hisilicon,off-on-delay = <120>;
> +                        hisilicon,ctrl-regs = <0x029 0x02a 0x02b>;
> +                        hisilicon,ctrl-data = <0x1 0x1>;
> +                        hisilicon,vset-regs = <0x072>;
> +                        hisilicon,vset-data = <0 0x3>;
> +                        hisilicon,regulator-n-vol = <8>;
> +                        hisilicon,vset-table = <2500000>,<2600000>,<2700000>,<2800000>,<2900000>,<3000000>,<3100000>,<3200000>;

Whitespace.

> +                        hisilicon,num_consumer_supplies = <1>;
> +                        hisilicon,consumer-supplies = "sensor_analog";


All of these vendor properties need documenting and signing of by Mark
(the Regulator Maintainer).

> +                };
> +	};
Lee Jones Oct. 1, 2015, 7:58 a.m. UTC | #3
I just noticed that you Cc'ed all of the Core ARM people too.  Please
do not do that.  I'm sure they have enough of their own mail to trawl
though.

Only mail relevant parties i.e. those who show up when you use:

  ./scripts/get_maintainer.pl.

> Document the new compatible for Hisilicon Hi655x pmic driver.
> 
> Signed-off-by: Fei Wang <w.f@huawei.com>
> ---
>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |   80 ++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> new file mode 100644
> index 0000000..17bd8ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> @@ -0,0 +1,80 @@
> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
> +
> +hi655x consists of a large and varied group of sub-devices:
> +
> +Device			 Supply Names	 Description
> +------			 ------------	 -----------
> +hi655x-powerkey		:		: Powerkey
> +hi655x-regulator-pmic	:		: Regulators
> +hi655x-usbvbus		:		: USB plug detection
> +hi655x-pmu-rtc		:		: RTC
> +hi655x-coul		:		: Coulomb
> +
> +Required properties:
> +- compatible : Should be "hisilicon,hi655x-pmic-driver"
> +- reg: Base address of PMIC on hi6220 soc
> +- #interrupt-cells: Should be 2, is the local IRQ number for hi655x.
> +- interrupt-controller: hi655x has internal IRQs (has own IRQ domain).
> +- pmu_irq_gpio: should be &gpio_pmu_irq_n, is the IRQ gpio of hi655x.
> +
> +Example:
> +	pmic: pmic@F8000000 {
> +                compatible = "hisilicon,hi655x-pmic-driver";
> +                reg = <0x0 0xF8000000 0x0 0x1000>;
> +                #interrupt-cells = <2>;
> +                interrupt-controller;
> +                pmu_irq_gpio = <&gpio_pmu_irq_n>;
> +                status = "ok";
> +
> +                ponkey:ponkey@b1{
> +                        compatible = "hisilicon,hi655x-powerkey";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <6 0>, <5 0>, <4 0>;
> +                        interrupt-names = "down", "up", "hold 1s";
> +                };
> +
> +                coul: coul@1 {
> +                        compatible = "hisilicon,hi655x-coul";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
> +                        battery_product_index = <0>;
> +                        status = "ok";
> +                };
> +
> +                rtc: rtc@1 {
> +                        compatible = "hisilicon,hi655x-pmu-rtc";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <20 0>;
> +                        interrupt-names = "hi655x_pmu_rtc";
> +                        board_id = <1>;
> +                };
> +
> +                usbvbus:usbvbus@b2{
> +                        compatible = "hisilicon,hi655x-usbvbus";
> +                        interrupt-parent = <&pmic>;
> +                        interrupts = <9 0>, <8 0>;
> +                        interrupt-names = "connect", "disconnect";
> +                };
> +
> +		ldo2: regulator@a21 {
> +                        compatible = "hisilicon,hi655x-regulator-pmic";
> +                        regulator-name = "ldo2";
> +                        regulator-min-microvolt = <2500000>;
> +                        regulator-max-microvolt = <3200000>;
> +                        hisilicon,valid-modes-mask = <0x02>;
> +                        hisilicon,valid-ops-mask = <0x01d>;
> +                        hisilicon,initial-mode = <0x02>;
> +                        hisilicon,regulator-type = <0x01>;
> +
> +                        hisilicon,off-on-delay = <120>;
> +                        hisilicon,ctrl-regs = <0x029 0x02a 0x02b>;
> +                        hisilicon,ctrl-data = <0x1 0x1>;
> +                        hisilicon,vset-regs = <0x072>;
> +                        hisilicon,vset-data = <0 0x3>;
> +                        hisilicon,regulator-n-vol = <8>;
> +                        hisilicon,vset-table = <2500000>,<2600000>,<2700000>,<2800000>,<2900000>,<3000000>,<3100000>,<3200000>;
> +                        hisilicon,num_consumer_supplies = <1>;
> +                        hisilicon,consumer-supplies = "sensor_analog";
> +                };
> +	};
Wangfei (William, Euler) Oct. 8, 2015, 6:33 a.m. UTC | #4
On 2015/10/1 1:39, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 07:05:04PM +0800, Fei Wang wrote:
> 
>> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
>> +
>> +hi655x consists of a large and varied group of sub-devices:
>> +
>> +Device			 Supply Names	 Description
>> +------			 ------------	 -----------
>> +hi655x-powerkey		:		: Powerkey
>> +hi655x-regulator-pmic	:		: Regulators
>> +hi655x-usbvbus		:		: USB plug detection
>> +hi655x-pmu-rtc		:		: RTC
>> +hi655x-coul		:		: Coulomb
> 
> ...counter?
> 
> There's no documentation of the bindings for any of the above devices or
> how things are structured, you need to provide binding documentation
> which is understandable standalone.  None of the properties are
> documented, nor is the set of regulators supported or how this is
> structured.

OK,i will add them.
> 
>> +                coul: coul@1 {
>> +                        compatible = "hisilicon,hi655x-coul";
>> +                        interrupt-parent = <&pmic>;
>> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
>> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
>> +                        battery_product_index = <0>;
> 
> For example, the "battery_product_index" property here is undocumented.

now on hikey, we only enable hi655x-regulator-pmic function, and regulator is child node of pmic.do i need to document regulator dt-binding in this file or create a new regulator dt-binding file?
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wangfei (William, Euler) Oct. 8, 2015, 7:03 a.m. UTC | #5
On 2015/10/1 15:58, Lee Jones wrote:
> I just noticed that you Cc'ed all of the Core ARM people too.  Please
> do not do that.  I'm sure they have enough of their own mail to trawl
> though.
> 
> Only mail relevant parties i.e. those who show up when you use:
> 
>   ./scripts/get_maintainer.pl.
> 
OK,i see,thank you!
>> Document the new compatible for Hisilicon Hi655x pmic driver.
>>
>> Signed-off-by: Fei Wang <w.f@huawei.com>
>> ---
>>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |   80 ++++++++++++++++++++
>>  1 file changed, 80 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>> new file mode 100644
>> index 0000000..17bd8ca
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>> @@ -0,0 +1,80 @@
>> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
>> +
>> +hi655x consists of a large and varied group of sub-devices:
>> +
>> +Device			 Supply Names	 Description
>> +------			 ------------	 -----------
>> +hi655x-powerkey		:		: Powerkey
>> +hi655x-regulator-pmic	:		: Regulators
>> +hi655x-usbvbus		:		: USB plug detection
>> +hi655x-pmu-rtc		:		: RTC
>> +hi655x-coul		:		: Coulomb
>> +
>> +Required properties:
>> +- compatible : Should be "hisilicon,hi655x-pmic-driver"
>> +- reg: Base address of PMIC on hi6220 soc
>> +- #interrupt-cells: Should be 2, is the local IRQ number for hi655x.
>> +- interrupt-controller: hi655x has internal IRQs (has own IRQ domain).
>> +- pmu_irq_gpio: should be &gpio_pmu_irq_n, is the IRQ gpio of hi655x.
>> +
>> +Example:
>> +	pmic: pmic@F8000000 {
>> +                compatible = "hisilicon,hi655x-pmic-driver";
>> +                reg = <0x0 0xF8000000 0x0 0x1000>;
>> +                #interrupt-cells = <2>;
>> +                interrupt-controller;
>> +                pmu_irq_gpio = <&gpio_pmu_irq_n>;
>> +                status = "ok";
>> +
>> +                ponkey:ponkey@b1{
>> +                        compatible = "hisilicon,hi655x-powerkey";
>> +                        interrupt-parent = <&pmic>;
>> +                        interrupts = <6 0>, <5 0>, <4 0>;
>> +                        interrupt-names = "down", "up", "hold 1s";
>> +                };
>> +
>> +                coul: coul@1 {
>> +                        compatible = "hisilicon,hi655x-coul";
>> +                        interrupt-parent = <&pmic>;
>> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
>> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
>> +                        battery_product_index = <0>;
>> +                        status = "ok";
>> +                };
>> +
>> +                rtc: rtc@1 {
>> +                        compatible = "hisilicon,hi655x-pmu-rtc";
>> +                        interrupt-parent = <&pmic>;
>> +                        interrupts = <20 0>;
>> +                        interrupt-names = "hi655x_pmu_rtc";
>> +                        board_id = <1>;
>> +                };
>> +
>> +                usbvbus:usbvbus@b2{
>> +                        compatible = "hisilicon,hi655x-usbvbus";
>> +                        interrupt-parent = <&pmic>;
>> +                        interrupts = <9 0>, <8 0>;
>> +                        interrupt-names = "connect", "disconnect";
>> +                };
>> +
>> +		ldo2: regulator@a21 {
>> +                        compatible = "hisilicon,hi655x-regulator-pmic";
>> +                        regulator-name = "ldo2";
>> +                        regulator-min-microvolt = <2500000>;
>> +                        regulator-max-microvolt = <3200000>;
>> +                        hisilicon,valid-modes-mask = <0x02>;
>> +                        hisilicon,valid-ops-mask = <0x01d>;
>> +                        hisilicon,initial-mode = <0x02>;
>> +                        hisilicon,regulator-type = <0x01>;
>> +
>> +                        hisilicon,off-on-delay = <120>;
>> +                        hisilicon,ctrl-regs = <0x029 0x02a 0x02b>;
>> +                        hisilicon,ctrl-data = <0x1 0x1>;
>> +                        hisilicon,vset-regs = <0x072>;
>> +                        hisilicon,vset-data = <0 0x3>;
>> +                        hisilicon,regulator-n-vol = <8>;
>> +                        hisilicon,vset-table = <2500000>,<2600000>,<2700000>,<2800000>,<2900000>,<3000000>,<3100000>,<3200000>;
>> +                        hisilicon,num_consumer_supplies = <1>;
>> +                        hisilicon,consumer-supplies = "sensor_analog";
>> +                };
>> +	};
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Oct. 12, 2015, 4:49 p.m. UTC | #6
On Thu, Oct 08, 2015 at 02:33:06PM +0800, wangfei wrote:
> On 2015/10/1 1:39, Mark Brown wrote:
> > On Wed, Sep 30, 2015 at 07:05:04PM +0800, Fei Wang wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> >> +                coul: coul@1 {
> >> +                        compatible = "hisilicon,hi655x-coul";
> >> +                        interrupt-parent = <&pmic>;
> >> +                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
> >> +                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
> >> +                        battery_product_index = <0>;

> > For example, the "battery_product_index" property here is undocumented.

> now on hikey, we only enable hi655x-regulator-pmic function, and
> regulator is child node of pmic.do i need to document regulator
> dt-binding in this file or create a new regulator dt-binding file?

Either is possible, though it is common to split the DT documentation
into per subsystem files for ease of reference and to avoid cross tree
merge issues.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
new file mode 100644
index 0000000..17bd8ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
@@ -0,0 +1,80 @@ 
+Hisilicon hi655x Power Management Integrated Circuit (PMIC)
+
+hi655x consists of a large and varied group of sub-devices:
+
+Device			 Supply Names	 Description
+------			 ------------	 -----------
+hi655x-powerkey		:		: Powerkey
+hi655x-regulator-pmic	:		: Regulators
+hi655x-usbvbus		:		: USB plug detection
+hi655x-pmu-rtc		:		: RTC
+hi655x-coul		:		: Coulomb
+
+Required properties:
+- compatible : Should be "hisilicon,hi655x-pmic-driver"
+- reg: Base address of PMIC on hi6220 soc
+- #interrupt-cells: Should be 2, is the local IRQ number for hi655x.
+- interrupt-controller: hi655x has internal IRQs (has own IRQ domain).
+- pmu_irq_gpio: should be &gpio_pmu_irq_n, is the IRQ gpio of hi655x.
+
+Example:
+	pmic: pmic@F8000000 {
+                compatible = "hisilicon,hi655x-pmic-driver";
+                reg = <0x0 0xF8000000 0x0 0x1000>;
+                #interrupt-cells = <2>;
+                interrupt-controller;
+                pmu_irq_gpio = <&gpio_pmu_irq_n>;
+                status = "ok";
+
+                ponkey:ponkey@b1{
+                        compatible = "hisilicon,hi655x-powerkey";
+                        interrupt-parent = <&pmic>;
+                        interrupts = <6 0>, <5 0>, <4 0>;
+                        interrupt-names = "down", "up", "hold 1s";
+                };
+
+                coul: coul@1 {
+                        compatible = "hisilicon,hi655x-coul";
+                        interrupt-parent = <&pmic>;
+                        interrupts = <24 0>, <25 0>, <26 0>, <27 0>;
+                        interrupt-names = "cl_int_i", "cl_out_i", "cl_in_i", "vbat_int_i";
+                        battery_product_index = <0>;
+                        status = "ok";
+                };
+
+                rtc: rtc@1 {
+                        compatible = "hisilicon,hi655x-pmu-rtc";
+                        interrupt-parent = <&pmic>;
+                        interrupts = <20 0>;
+                        interrupt-names = "hi655x_pmu_rtc";
+                        board_id = <1>;
+                };
+
+                usbvbus:usbvbus@b2{
+                        compatible = "hisilicon,hi655x-usbvbus";
+                        interrupt-parent = <&pmic>;
+                        interrupts = <9 0>, <8 0>;
+                        interrupt-names = "connect", "disconnect";
+                };
+
+		ldo2: regulator@a21 {
+                        compatible = "hisilicon,hi655x-regulator-pmic";
+                        regulator-name = "ldo2";
+                        regulator-min-microvolt = <2500000>;
+                        regulator-max-microvolt = <3200000>;
+                        hisilicon,valid-modes-mask = <0x02>;
+                        hisilicon,valid-ops-mask = <0x01d>;
+                        hisilicon,initial-mode = <0x02>;
+                        hisilicon,regulator-type = <0x01>;
+
+                        hisilicon,off-on-delay = <120>;
+                        hisilicon,ctrl-regs = <0x029 0x02a 0x02b>;
+                        hisilicon,ctrl-data = <0x1 0x1>;
+                        hisilicon,vset-regs = <0x072>;
+                        hisilicon,vset-data = <0 0x3>;
+                        hisilicon,regulator-n-vol = <8>;
+                        hisilicon,vset-table = <2500000>,<2600000>,<2700000>,<2800000>,<2900000>,<3000000>,<3100000>,<3200000>;
+                        hisilicon,num_consumer_supplies = <1>;
+                        hisilicon,consumer-supplies = "sensor_analog";
+                };
+	};