Re: [PATCH v6 2/7] dt-bindings: thermal: add YAML schema for sun8i-thermal driver bindings

[Vasily Khoruzhick]

On Wed, Nov 27, 2019 at 10:07 AM OndÅej Jirman <megous@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Nov 27, 2019 at 06:44:34PM +0100, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, Nov 26, 2019 at 09:29:30PM -0800, Vasily Khoruzhick wrote:
> > > From: Yangtao Li <tiny.windzz@xxxxxxxxx>
> > >
> > > sun8i-thermal driver supports thermal sensor in wide range of Allwinner
> > > SoCs. Add YAML schema for its bindings.
> > >
> > > Signed-off-by: Yangtao Li <tiny.windzz@xxxxxxxxx>
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@xxxxxxxxx>
> > > ---
> > >  .../thermal/allwinner,sun8i-a83t-ths.yaml     | 103 ++++++++++++++++++
> > >  1 file changed, 103 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> > > new file mode 100644
> > > index 000000000000..e622f0a4be90
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> > > @@ -0,0 +1,103 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/thermal/allwinner,sun8i-a83t-ths.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Allwinner SUN8I Thermal Controller Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Yangtao Li <tiny.windzz@xxxxxxxxx>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - const: allwinner,sun8i-a83t-ths
> > > +      - const: allwinner,sun8i-h3-ths
> > > +      - const: allwinner,sun8i-r40-ths
> > > +      - const: allwinner,sun50i-a64-ths
> > > +      - const: allwinner,sun50i-h5-ths
> > > +      - const: allwinner,sun50i-h6-ths
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    anyOf:
> > > +      - items:
> > > +        - const: bus
> > > +        - const: mod
> > > +      - items:
> > > +        - const: bus
> >
> > This can be:
> >
> > clock-names:
> >   minItems: 1
>
> Additionally, minItems should be 0, since A83T doesn't have bus clock/reset. And
> then there should be a special case for A83T too with min/maxItems = 0 for both
> resets and clocks.

That's why I removed clocks, clock-names and resets from required
properties. If they're present min/maxItems should be 1 and 2
accordingly.

> regards,
>         o.
>
> >   maxItems: 2
> >   items:
> >     - const: bus
> >     - const: mod
> >
> > And the length should be checked based on the compatible value, with
> > something like
> >
> > if:
> >   properties:
> >     compatible:
> >       contains:
> >         const: allwinner,sun50i-h6-ths
> >
> > then:
> >   properties:
> >     clocks:
> >       maxItems: 1
> >
> >     clock-names:
> >       maxItems: 1
> >
> > else:
> >   properties:
> >     clocks:
> >       maxItems: 2
> >
> >     clock-names:
> >       maxItems: 2
> >
> > > +
> > > +  '#thermal-sensor-cells':
> > > +    enum: [ 0, 1 ]
> > > +    description: |
> > > +      Definition depends on soc version:
> > > +
> > > +      For "allwinner,sun8i-h3-ths",
> > > +      value must be 0.
> > > +      For all other compatibles
> > > +      value must be 1.
> >
> > This should be checked using an if as well.
> >
> > > +
> > > +  nvmem-cells:
> > > +    maxItems: 1
> > > +    items:
> > > +      - description: Calibration data for thermal sensors
> >
> > You can drop the items and just move the description up one level,
> > under nvmem-cells
> >
> > > +
> > > +  nvmem-cell-names:
> > > +    items:
> > > +      - const: calibration
> >
> > Ditto for the const
> >
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - '#thermal-sensor-cells'
> >
> > Whether clocks, clock-names and resets are thereshould be check using
> > an if statement as well.
> >
> > > +
> > > +examples:
> > > +  - |
> > > +    ths_a83t: ths@1f04000 {
> >
> > You don't need the label at all, and the node name should be
> > temperature-sensor according to the DT spec, not ths. This applies to
> > all you examples.
> >
> > > +         compatible = "allwinner,sun8i-a83t-ths";
> > > +         reg = <0x01f04000 0x100>;
> > > +         interrupts = <0 31 0>;
> > > +         nvmem-cells = <&ths_calibration>;
> > > +         nvmem-cell-names = "calibration";
> > > +         #thermal-sensor-cells = <1>;
> > > +    };
> >
> > New line.
> >
> > Thanks!
> > Maxime
>
>