| Message ID | 1443611111-3196-2-git-send-email-w.f@huawei.com |
|---|---|
| State | Under Review, archived |
| Headers | show |
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.
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). > + }; > + };
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"; > + }; > + };
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
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
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 --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"; + }; + };
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