Re: [RFC PATCH v2 2/5] dt_bindings: ROHM BD99954 Charger

[Rob Herring <robh@kernel.org>]

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