All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vasily Khoruzhick <anarsoul@gmail.com>
To: Maxime Ripard <mripard@kernel.org>
Cc: "Yangtao Li" <tiny.windzz@gmail.com>,
	"Zhang Rui" <rui.zhang@intel.com>,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	"Amit Kucheria" <amit.kucheria@verdurent.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Linux PM" <linux-pm@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	arm-linux <linux-arm-kernel@lists.infradead.org>,
	"Ondřej Jirman" <megous@megous.com>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 2/7] dt-bindings: thermal: add YAML schema for sun8i-thermal driver bindings
Date: Wed, 18 Dec 2019 14:27:00 -0800	[thread overview]
Message-ID: <CA+E=qVdfV5LKBEar8eT286+ADrpygEkbe5OX1GVRw+khatrJhA@mail.gmail.com> (raw)
In-Reply-To: <20191218220037.4g6pzdvrhroaj4qu@gilmour.lan>

On Wed, Dec 18, 2019 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Tue, Dec 17, 2019 at 08:21:16PM -0800, Vasily Khoruzhick wrote:
> > From: Yangtao Li <tiny.windzz@gmail.com>
> >
> > 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@gmail.com>
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  .../thermal/allwinner,sun8i-a83t-ths.yaml     | 146 ++++++++++++++++++
> >  1 file changed, 146 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..8768c2450633
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> > @@ -0,0 +1,146 @@
> > +# 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@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - allwinner,sun8i-a83t-ths
> > +      - allwinner,sun8i-h3-ths
> > +      - allwinner,sun8i-r40-ths
> > +      - allwinner,sun50i-a64-ths
> > +      - allwinner,sun50i-h5-ths
> > +      - allwinner,sun50i-h6-ths
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  nvmem-cells:
> > +    maxItems: 1
> > +    description: Calibration data for thermal sensors
> > +
> > +  nvmem-cell-names:
> > +    const: calibration
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: allwinner,sun50i-h6-ths
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 1
>
> When minItems and maxItems are equal, you can only set one, the other
> will be filled automatically.

Is it documented anywhere? I have a feeling like I'm shooting in the
dark. So far I've read Documentation/devicetree/writing-schema.rst,
Documentation/devicetree/bindings/example-schema.yaml and few other
schemas for inspiration but yet I don't have solid understanding how
it's supposed to be written. Examples are pretty scarce and figuring
out why certain construction doesn't work is pretty tricky.

> > +
> > +        clock-names:
> > +          minItems: 1
> > +          maxItems: 1
> > +          items:
> > +            - const: bus
>
> And this can even be just
>
> clock-names:
>   const: bus

OK

> > +
> > +    else:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 2
> > +
> > +        clock-names:
> > +          minItems: 1
> > +          maxItems: 2
> > +          items:
> > +            - const: bus
> > +            - const: mod
>
> I'm not sure why you need the minItems set to 1 here though?
>
> it's always 2 for the !H6 case, right?
>
> if so, then we should even do something like:
>
> properties:
>   ...
>
>   # This is needed because we will need to check both the H6 and !H6
>   # case, and it must validate. So we make sure we match against the
>   # union of both cases.
>   clocks:
>     minItems: 1
>     maxItems: 2
>     items:
>       - description: Bus Clock
>       - description: Module Clock
>
>   # Same story here
>   clock-names:
>     minItems: 1
>     maxItems: 2
>     items:
>       - const: bus
>       - const: mod
>
> allOf:
>   - if:
>     properties:
>       compatible:
>         contains:
>           const: allwinner,sun50i-h6-ths
>
>     # Here we validate in the H6 case we only have one clock
>     then:
>       properties:
>         clocks:
>           maxItems: 1
>
>         clock-names:
>           maxItems: 1
>
>     # and here that in the other case we have two clocks, the names
>     # being validated by the schema above
>     else:
>       properties:
>         clocks:
>           maxItems: 2
>
>         clock-names:
>           maxItems: 2
>
> # And now we can set this since all our properties will have been
> # expressed in the upper level schema
> additionalProperties: false

OK

> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: allwinner,sun8i-h3-ths
> > +
> > +    then:
> > +      properties:
> > +        "#thermal-sensor-cells":
> > +          const: 0
> > +
> > +    else:
> > +      properties:
> > +        "#thermal-sensor-cells":
> > +          const: 1
>
> Same thing here, you should have an enum accepting both values in the
> upper schema, the condition here only making further checks. Also, in
> the case where #thermal-sensor-cells is one, then you need to document
> what that argument is.

OK

>
> Thanks!
> Maxime

WARNING: multiple messages have this Message-ID (diff)
From: Vasily Khoruzhick <anarsoul@gmail.com>
To: Maxime Ripard <mripard@kernel.org>
Cc: "Mark Rutland" <mark.rutland@arm.com>,
	devicetree <devicetree@vger.kernel.org>,
	"Amit Kucheria" <amit.kucheria@verdurent.com>,
	"Linux PM" <linux-pm@vger.kernel.org>,
	"Yangtao Li" <tiny.windzz@gmail.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
	"Zhang Rui" <rui.zhang@intel.com>,
	"Ondřej Jirman" <megous@megous.com>,
	"David S. Miller" <davem@davemloft.net>,
	arm-linux <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v7 2/7] dt-bindings: thermal: add YAML schema for sun8i-thermal driver bindings
Date: Wed, 18 Dec 2019 14:27:00 -0800	[thread overview]
Message-ID: <CA+E=qVdfV5LKBEar8eT286+ADrpygEkbe5OX1GVRw+khatrJhA@mail.gmail.com> (raw)
In-Reply-To: <20191218220037.4g6pzdvrhroaj4qu@gilmour.lan>

On Wed, Dec 18, 2019 at 2:00 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Tue, Dec 17, 2019 at 08:21:16PM -0800, Vasily Khoruzhick wrote:
> > From: Yangtao Li <tiny.windzz@gmail.com>
> >
> > 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@gmail.com>
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  .../thermal/allwinner,sun8i-a83t-ths.yaml     | 146 ++++++++++++++++++
> >  1 file changed, 146 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..8768c2450633
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> > @@ -0,0 +1,146 @@
> > +# 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@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - allwinner,sun8i-a83t-ths
> > +      - allwinner,sun8i-h3-ths
> > +      - allwinner,sun8i-r40-ths
> > +      - allwinner,sun50i-a64-ths
> > +      - allwinner,sun50i-h5-ths
> > +      - allwinner,sun50i-h6-ths
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  nvmem-cells:
> > +    maxItems: 1
> > +    description: Calibration data for thermal sensors
> > +
> > +  nvmem-cell-names:
> > +    const: calibration
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: allwinner,sun50i-h6-ths
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 1
>
> When minItems and maxItems are equal, you can only set one, the other
> will be filled automatically.

Is it documented anywhere? I have a feeling like I'm shooting in the
dark. So far I've read Documentation/devicetree/writing-schema.rst,
Documentation/devicetree/bindings/example-schema.yaml and few other
schemas for inspiration but yet I don't have solid understanding how
it's supposed to be written. Examples are pretty scarce and figuring
out why certain construction doesn't work is pretty tricky.

> > +
> > +        clock-names:
> > +          minItems: 1
> > +          maxItems: 1
> > +          items:
> > +            - const: bus
>
> And this can even be just
>
> clock-names:
>   const: bus

OK

> > +
> > +    else:
> > +      properties:
> > +        clocks:
> > +          minItems: 1
> > +          maxItems: 2
> > +
> > +        clock-names:
> > +          minItems: 1
> > +          maxItems: 2
> > +          items:
> > +            - const: bus
> > +            - const: mod
>
> I'm not sure why you need the minItems set to 1 here though?
>
> it's always 2 for the !H6 case, right?
>
> if so, then we should even do something like:
>
> properties:
>   ...
>
>   # This is needed because we will need to check both the H6 and !H6
>   # case, and it must validate. So we make sure we match against the
>   # union of both cases.
>   clocks:
>     minItems: 1
>     maxItems: 2
>     items:
>       - description: Bus Clock
>       - description: Module Clock
>
>   # Same story here
>   clock-names:
>     minItems: 1
>     maxItems: 2
>     items:
>       - const: bus
>       - const: mod
>
> allOf:
>   - if:
>     properties:
>       compatible:
>         contains:
>           const: allwinner,sun50i-h6-ths
>
>     # Here we validate in the H6 case we only have one clock
>     then:
>       properties:
>         clocks:
>           maxItems: 1
>
>         clock-names:
>           maxItems: 1
>
>     # and here that in the other case we have two clocks, the names
>     # being validated by the schema above
>     else:
>       properties:
>         clocks:
>           maxItems: 2
>
>         clock-names:
>           maxItems: 2
>
> # And now we can set this since all our properties will have been
> # expressed in the upper level schema
> additionalProperties: false

OK

> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: allwinner,sun8i-h3-ths
> > +
> > +    then:
> > +      properties:
> > +        "#thermal-sensor-cells":
> > +          const: 0
> > +
> > +    else:
> > +      properties:
> > +        "#thermal-sensor-cells":
> > +          const: 1
>
> Same thing here, you should have an enum accepting both values in the
> upper schema, the condition here only making further checks. Also, in
> the case where #thermal-sensor-cells is one, then you need to document
> what that argument is.

OK

>
> Thanks!
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2019-12-18 22:27 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-18  4:21 [PATCH v7 0/7] add thermal sensor driver for A64, A83T, H3, H5, H6, R40 Vasily Khoruzhick
2019-12-18  4:21 ` Vasily Khoruzhick
2019-12-18  4:21 ` [PATCH v7 1/7] thermal: sun8i: add thermal driver for H6/H5/H3/A64/A83T/R40 Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18  4:21 ` [PATCH v7 2/7] dt-bindings: thermal: add YAML schema for sun8i-thermal driver bindings Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18 22:00   ` Maxime Ripard
2019-12-18 22:00     ` Maxime Ripard
2019-12-18 22:27     ` Vasily Khoruzhick [this message]
2019-12-18 22:27       ` Vasily Khoruzhick
2019-12-19  9:12       ` Maxime Ripard
2019-12-19  9:12         ` Maxime Ripard
2019-12-18  4:21 ` [PATCH v7 3/7] ARM: dts: sun8i-a83t: Add thermal sensor and thermal zones Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18  4:21 ` [PATCH v7 4/7] ARM: dts: sun8i-h3: " Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18  4:21 ` [PATCH v7 5/7] arm64: dts: allwinner: h5: " Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18  4:21 ` [PATCH v7 6/7] arm64: dts: allwinner: h6: " Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick
2019-12-18  4:32   ` Chen-Yu Tsai
2019-12-18  4:32     ` Chen-Yu Tsai
2019-12-18 23:18     ` Vasily Khoruzhick
2019-12-18 23:18       ` Vasily Khoruzhick
2019-12-19  1:03       ` Ondřej Jirman
2019-12-19  1:03         ` Ondřej Jirman
2019-12-19  1:42         ` Vasily Khoruzhick
2019-12-19  1:42           ` Vasily Khoruzhick
2019-12-19  2:59           ` Chen-Yu Tsai
2019-12-19  2:59             ` Chen-Yu Tsai
2019-12-18  4:21 ` [PATCH v7 7/7] arm64: dts: allwinner: a64: Add thermal sensors " Vasily Khoruzhick
2019-12-18  4:21   ` Vasily Khoruzhick

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CA+E=qVdfV5LKBEar8eT286+ADrpygEkbe5OX1GVRw+khatrJhA@mail.gmail.com' \
    --to=anarsoul@gmail.com \
    --cc=amit.kucheria@verdurent.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=megous@megous.com \
    --cc=mripard@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=rui.zhang@intel.com \
    --cc=tiny.windzz@gmail.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.