| Message ID | 104b5ef63c2ad4771503d9e6618bf427721042c3.1581597365.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support ROHM BD99954 charger IC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Fri, 14 Feb 2020 09:36:47 +0200, Matti Vaittinen wrote: > The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-Ion > secondary battery. Intended to be used in space-constraint equipment such > as Low profile Notebook PC, Tablets and other applications. BD99954 > provides a Dual-source Battery Charger, two port BC1.2 detection and a > Battery Monitor. > > Document the DT bindings for BD99954 > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > > It would probably be nice if the charger DT binding yaml could somehow > be listing and evaluating properties that it can use from static battery > nodes - and perhaps some warning could be emitted if unsupported > properties are given from battery nodes(?) Just some thinking here. > What if the charger ignores for example the current limits from battery > node (I am not sure but I think a few may ignore) - I guess it would be > nice to give a nudge to a person who added those properties in his DT > if they won't have any impact? Any thoughts? > > .../bindings/power/supply/rohm,bd9995x.yaml | 167 ++++++++++++++++++ > 1 file changed, 167 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > My bot found errors running 'make dt_binding_check' on your patch: warning: no schema found in file: Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml: ignoring, error parsing file Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml: while scanning a simple key in "<unicode string>", line 29, column 3 could not find expected ':' in "<unicode string>", line 30, column 1 Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/power/supply/rohm,bd9995x.example.dts' failed make[1]: *** [Documentation/devicetree/bindings/power/supply/rohm,bd9995x.example.dts] Error 1 Makefile:1263: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1237902 Please check and re-submit.
Morning Rob, On Tue, 2020-02-18 at 14:21 -0600, Rob Herring wrote: > On Fri, 14 Feb 2020 09:36:47 +0200, Matti Vaittinen wrote: > > The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium- > > Ion > > secondary battery. Intended to be used in space-constraint > > equipment such > > as Low profile Notebook PC, Tablets and other applications. BD99954 > > provides a Dual-source Battery Charger, two port BC1.2 detection > > and a > > Battery Monitor. > > > > Document the DT bindings for BD99954 > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > > > It would probably be nice if the charger DT binding yaml could > > somehow > > be listing and evaluating properties that it can use from static > > battery > > nodes - and perhaps some warning could be emitted if unsupported > > properties are given from battery nodes(?) Just some thinking here. > > What if the charger ignores for example the current limits from > > battery > > node (I am not sure but I think a few may ignore) - I guess it > > would be > > nice to give a nudge to a person who added those properties in his > > DT > > if they won't have any impact? Any thoughts? > > > > .../bindings/power/supply/rohm,bd9995x.yaml | 167 > > ++++++++++++++++++ > > 1 file changed, 167 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > > > > My bot found errors running 'make dt_binding_check' on your patch: Ouch ... sorry. There is some leftover block from another text based binding document which I used as an example while writing out the battery parameters BD99954 uses. There's two other hiccups when I try running make dt_binding_check. I assume they are false positives. <SIDE NOTE> Although... Back in my Nokia days I joined in a trainer-training. I had excellent British coach - Graham if I remember correctly - who told us never to assume. He explained where word ass-u-me comes from. I can still hear his very British accent: "It makes an ass out of u and me". I still do so though. I'm not learning easily it seems. </SIDE NOTE> 1. It seems to me the multipleOf: is not recognized. I guess it should(?) I will comment it out in v3 though until I get confirmation it should work. 2. schema validation for: rohm,vsys-regulation-microvolt: description: system specific lower limit for system voltage. minimum: 2560000 maximum: 19200000 #multipleOf: 64000 fails. But when I change this to rohm,vsys-regulation-microvolts: (plural) it seems to be passing the validation. A git grep under Documentation/devicetree/bindings reveals that both plural and singular are used - but the singular seems to be far more popular than plural. It also looks like the 'core bindings' like regulators use singular. Hence I'll leave this to singular for v3 even though it fails the validation - please let me know if this was wrong choice or if you spot any other oddities there. I can't see what else it could be but for some reason I still find this yaml terribly hard :( Hmm.. I wonder if I have some old checker tools installed and used on my PC? I also get validation failures for the example schemas :/ > warning: no schema found in file: > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > /builds/robherring/linux-dt- > review/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.ya > ml: ignoring, error parsing file > Documentation/devicetree/bindings/display/simple- > framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): > /example-0/chosen: chosen node must be at root node > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml: wh > ile scanning a simple key > in "<unicode string>", line 29, column 3 > could not find expected ':' > in "<unicode string>", line 30, column 1 > Documentation/devicetree/bindings/Makefile:12: recipe for target > 'Documentation/devicetree/bindings/power/supply/rohm,bd9995x.example. > dts' failed > make[1]: *** > [Documentation/devicetree/bindings/power/supply/rohm,bd9995x.example. > dts] Error 1 > Makefile:1263: recipe for target 'dt_binding_check' failed > make: *** [dt_binding_check] Error 2 > > See https://patchwork.ozlabs.org/patch/1237902 > Please check and re-submit. I have the RFC v3 almost finished. Hope to find the time to finish and submit it still today :) Thanks and regards Matti
On Wed, Feb 19, 2020 at 2:05 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Morning Rob, > > On Tue, 2020-02-18 at 14:21 -0600, Rob Herring wrote: > > On Fri, 14 Feb 2020 09:36:47 +0200, Matti Vaittinen wrote: > > > The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium- > > > Ion > > > secondary battery. Intended to be used in space-constraint > > > equipment such > > > as Low profile Notebook PC, Tablets and other applications. BD99954 > > > provides a Dual-source Battery Charger, two port BC1.2 detection > > > and a > > > Battery Monitor. > > > > > > Document the DT bindings for BD99954 > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > --- > > > > > > It would probably be nice if the charger DT binding yaml could > > > somehow > > > be listing and evaluating properties that it can use from static > > > battery > > > nodes - and perhaps some warning could be emitted if unsupported > > > properties are given from battery nodes(?) Just some thinking here. > > > What if the charger ignores for example the current limits from > > > battery > > > node (I am not sure but I think a few may ignore) - I guess it > > > would be > > > nice to give a nudge to a person who added those properties in his > > > DT > > > if they won't have any impact? Any thoughts? > > > > > > .../bindings/power/supply/rohm,bd9995x.yaml | 167 > > > ++++++++++++++++++ > > > 1 file changed, 167 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > > > > > > > My bot found errors running 'make dt_binding_check' on your patch: > > Ouch ... sorry. There is some leftover block from another text based > binding document which I used as an example while writing out the > battery parameters BD99954 uses. > > There's two other hiccups when I try running make dt_binding_check. I > assume they are false positives. > > <SIDE NOTE> > Although... Back in my Nokia days I joined in a trainer-training. I had > excellent British coach - Graham if I remember correctly - who told us > never to assume. He explained where word ass-u-me comes from. I can > still hear his very British accent: "It makes an ass out of u and me". > I still do so though. I'm not learning easily it seems. > </SIDE NOTE> > > 1. It seems to me the multipleOf: is not recognized. I guess it > should(?) I will comment it out in v3 though until I get confirmation > it should work. Yes, it should work. I reworked allowed keywords recently. Is dtschema up to date? > > 2. schema validation for: > > rohm,vsys-regulation-microvolt: > description: system specific lower limit for system voltage. > minimum: 2560000 > maximum: 19200000 > #multipleOf: 64000 > > fails. But when I change this to > rohm,vsys-regulation-microvolts: (plural) > it seems to be passing the validation. A git grep under > Documentation/devicetree/bindings reveals that both plural and singular > are used - but the singular seems to be far more popular than plural. Only in one case that got it wrong. > It also looks like the 'core bindings' like regulators use singular. > Hence I'll leave this to singular for v3 even though it fails the > validation - please let me know if this was wrong choice or if you spot > any other oddities there. I can't see what else it could be but for > some reason I still find this yaml terribly hard :( > > Hmm.. I wonder if I have some old checker tools installed and used on > my PC? I also get validation failures for the example schemas :/ You do have to stay up to date. > > warning: no schema found in file: > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > > /builds/robherring/linux-dt- > > review/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.ya > > ml: ignoring, error parsing file > > Documentation/devicetree/bindings/display/simple- > > framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): > > /example-0/chosen: chosen node must be at root node > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml: wh > > ile scanning a simple key > > in "<unicode string>", line 29, column 3 > > could not find expected ':' > > in "<unicode string>", line 30, column 1 Though this is just incorrect YAML and the tool version shouldn't matter. Rob
Morning again Rob, On Thu, 2020-02-20 at 14:18 -0600, Rob Herring wrote: > On Wed, Feb 19, 2020 at 2:05 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > Morning Rob, > > > > On Tue, 2020-02-18 at 14:21 -0600, Rob Herring wrote: > > > On Fri, 14 Feb 2020 09:36:47 +0200, Matti Vaittinen wrote: > > > > The ROHM BD99954 is a Battery Management LSI for 1-4 cell > > > > Lithium- > > > > Ion > > > > secondary battery. Intended to be used in space-constraint > > > > equipment such > > > > as Low profile Notebook PC, Tablets and other applications. > > > > BD99954 > > > > provides a Dual-source Battery Charger, two port BC1.2 > > > > detection > > > > and a > > > > Battery Monitor. > > > > > > > > Document the DT bindings for BD99954 > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaittinen@fi.rohmeurope.com> > > > > --- > > > > > > It also looks like the 'core bindings' like regulators use > > singular. > > Hence I'll leave this to singular for v3 even though it fails the > > validation - please let me know if this was wrong choice or if you > > spot > > any other oddities there. I can't see what else it could be but for > > some reason I still find this yaml terribly hard :( > > > > Hmm.. I wonder if I have some old checker tools installed and used > > on > > my PC? I also get validation failures for the example schemas :/ > > You do have to stay up to date. Uh. So it seems. I updated the multipleOf was recognized now. I also got nice warning about missing #size-cells and #address-cells for the i2c bus. It kind of is out of the scope of the charger binding (which should just sit in the bus) but the warning was clear enough so I understood what it was about. Nice job, thanks. I just wonder if it would be big task to add version query (like dt- doc-validate --version) to the toolings and include "required version" in kernel makefiles so that the 'dt_binding_check' make target could warn if one uses too old version? (Ignorant c-coders like me may not know the dt-schema tooling is not included in kernel scripts and needs to be updated on host PC.) Just a suggestion which might reduce errors before patch submissions. > > > warning: no schema found in file: > > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml > > > /builds/robherring/linux-dt- > > > review/Documentation/devicetree/bindings/power/supply/rohm,bd9995 > > > x.ya > > > ml: ignoring, error parsing file > > > Documentation/devicetree/bindings/display/simple- > > > framebuffer.example.dts:21.16-37.11: Warning > > > (chosen_node_is_root): > > > /example-0/chosen: chosen node must be at root node > > > Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml: > > > wh > > > ile scanning a simple key > > > in "<unicode string>", line 29, column 3 > > > could not find expected ':' > > > in "<unicode string>", line 30, column 1 > > Though this is just incorrect YAML and the tool version shouldn't > matter. Yes. That should be fixed now. And v4 will also have the multipleOf uncommented and #size-cells and #address-cells for the i2c bus added. I'll wait for a while for others to give feedback from the series before sending it out though. Thanks for the help! --Matti
diff --git a/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml b/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml new file mode 100644 index 000000000000..bd1e37ee644d --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml @@ -0,0 +1,167 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/rohm,bd9995x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ROHM BD99954 Battery charger driver + +maintainers: + - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> + - Markus Laine <markus.laine@fi.rohmeurope.com> + - Mikko Mutanen <mikko.mutanen@fi.rohmeurope.com> + +description: | + The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-Ion + secondary battery intended to be used in space-constraint equipment such + as Low profile Notebook PC, Tablets and other applications. BD99954 + provides a Dual-source Battery Charger, two port BC1.2 detection and a + Battery Monitor. + +Optional properties: +- monitored-battery: phandle of battery characteristics devicetree node + The charger uses the following battery properties: + + precharge-current-microamp: maximum charge current during precharge + phase (typically 20% of battery capacity). + + charge-term-current-microamp: a charge cycle terminates when the + battery voltage is above recharge threshold, and the current is below + this setting (typically 10% of battery capacity). + See also Documentation/devicetree/bindings/power/supply/battery.txt +- ti,system-minimum-microvolt: when power is connected and the battery is below + minimum system voltage, the system will be regulated above this setting. + +properties: + compatible: + const: rohm,bd9995x-charger +# +# The battery charging profile of BD99954. +# +# Curve (1) represents charging current. +# Curve (2) represents battery voltage. +# +# The BD99954 data sheet divides charging to three phases. +# a) Trickle-charge with constant current (8). +# b) pre-charge with constant current (6) +# c) fast-charge with: +# First a constant current (5) phase (CC) +# Then constant voltage (CV) phase (after the battery voltage has reached +# target level - until charging current has dropped to termination +# level (7) +# +# V ^ ^ I +# . . +# . . +# (4)- -.- - - - - - - - - - - - - - +++++++++++++++++++++++++++. +# . / . +# . ++++++/++ - - - - - - - - - - - - -.- - (5) +# . + / + . +# . + - -- . +# . + - + . +# . +.- -: . +# . .+ +` . +# . .- + | `/ . +# . .." + .: . +# . -" + -- . +# . (2) ..." + | :- . +# . ..."" + -: . +# (3)- -.-.""- - - - -+++++++++ - - - - - - -.:- - - - - - - - - .- - (6) +# . + `:. . +# . + | -: . +# . + -: . +# . + .. . +# . (1) + | "+++- - - -.- - (7) +# -++++++++++++++- - - - - - - - - - - - - - - - - + - - - .- - (8) +# . + - +# -------------------------------------------------+++++++++--> +# | | | CC | CV | +# | --trickle-- | -pre- | ---------fast----------- | +# +# The charger uses the following battery properties +# - tricklecharge-current-microamp: +# Current used at trickle-charge phase (8 in above chart) +# minimum: 64000 +# maximum: 1024000 +# multipleOf: 64000 +# - precharge-current-microamp: +# Current used at pre-charge phase (6 in above chart) +# minimum: 64000 +# maximum: 1024000 +# multipleOf: 64000 +# - constant-charge-current-max-microamp +# Current used at fast charge constant current phase (5 in above chart) +# minimum: 64000 +# maximum: 1024000 +# multipleOf: 64000 +# - constant-charge-voltage-max-microvolt +# The constant voltage used in fast charging phase (4 in above chart) +# minimum: 2560000 +# maximum: 19200000 +# multipleOf: 16000 +# - precharge-upper-limit-microvolt +# charging mode is changed from trickle charging to pre-charging +# when battery voltage exceeds this limit voltage (3 in above chart) +# minimum: 2048000 +# maximum: 19200000 +# multipleOf: 64000 +# - re-charge-voltage-microvolt +# minimum: 2560000 +# maximum: 19200000 +# multipleOf: 16000 +# re-charging is automatically started when battry has been discharging +# to the point where the battery voltage drops below this limit +# - over-voltage-threshold-microvolt +# battery is expected to be faulty if battery voltage exceeds this limit. +# Charger will then enter to a "battery faulty" -state +# minimum: 2560000 +# maximum: 19200000 +# multipleOf: 16000 +# - charge-term-current-microamp +# minimum: 0 +# maximum: 1024000 +# multipleOf: 64000 +# a charge cycle terminates when the battery voltage is above recharge +# threshold, and the current is below this setting (7 in above chart) +# See also Documentation/devicetree/bindings/power/supply/battery.txt + + monitored-battery: + description: + phandle of battery characteristics devicetree node + + rohm,vbus-input-current-limit-microamp: + description: + system specific VBUS input current limit (in microamps). + minimum: 32000 + maximum: 16352000 + multipleOf: 32000 + + rohm,vcc-input-current-limit-microamp: + description: + system specific VCC/VACP input current limit (in microamps). + minimum: 32000 + maximum: 16352000 + multipleOf: 32000 + + rohm,vsys-regulation-microvolt: + description: + system specific lower limit for system voltage. + minimum: 2560000 + maximum: 19200000 + multipleOf: 64000 + +required: + - compatible + +examples: + - | + i2c { + charger@9 { + compatible = "rohm,bd9995x-charger"; + monitored-battery = <&battery>; + reg = <0x9>; + interrupt-parent = <&gpio1>; + interrupts = <29 8>; + rohm,vsys-regulation-microvolt = <8960000>; + rohm,vbus-input-current-limit-microamp = <1472000>; + rohm,vcc-input-current-limit-microamp = <1472000>; + }; + };
The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-Ion secondary battery. Intended to be used in space-constraint equipment such as Low profile Notebook PC, Tablets and other applications. BD99954 provides a Dual-source Battery Charger, two port BC1.2 detection and a Battery Monitor. Document the DT bindings for BD99954 Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- It would probably be nice if the charger DT binding yaml could somehow be listing and evaluating properties that it can use from static battery nodes - and perhaps some warning could be emitted if unsupported properties are given from battery nodes(?) Just some thinking here. What if the charger ignores for example the current limits from battery node (I am not sure but I think a few may ignore) - I guess it would be nice to give a nudge to a person who added those properties in his DT if they won't have any impact? Any thoughts? .../bindings/power/supply/rohm,bd9995x.yaml | 167 ++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/rohm,bd9995x.yaml