On Thu, Apr 9, 2020 at 8:04 PM Anson Huang <anson.huang@xxxxxxx> wrote:
>
> Hi, Amit
>
> > Subject: Re: [PATCH] dt-bindings: thermal: Convert i.MX to json-schema
> >
> > Hi Anson,
> >
> > On Thu, Apr 9, 2020 at 10:29 AM Anson Huang <Anson.Huang@xxxxxxx>
> > wrote:
> > >
> > > Convert the i.MX thermal binding to DT schema format using json-schema
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > > ---
> > > .../devicetree/bindings/thermal/imx-thermal.txt | 61 --------------
> > > .../devicetree/bindings/thermal/imx-thermal.yaml | 97
> > ++++++++++++++++++++++
> > > 2 files changed, 97 insertions(+), 61 deletions(-) delete mode
> > > 100644 Documentation/devicetree/bindings/thermal/imx-thermal.txt
> > > create mode 100644
> > > Documentation/devicetree/bindings/thermal/imx-thermal.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> > > b/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> > > deleted file mode 100644
> > > index 823e417..0000000
> > > --- a/Documentation/devicetree/bindings/thermal/imx-thermal.txt
> > > +++ /dev/null
> > > @@ -1,61 +0,0 @@
> > > -* Temperature Monitor (TEMPMON) on Freescale i.MX SoCs
> > > -
> > > -Required properties:
> > > -- compatible : must be one of following:
> > > - - "fsl,imx6q-tempmon" for i.MX6Q,
> > > - - "fsl,imx6sx-tempmon" for i.MX6SX,
> > > - - "fsl,imx7d-tempmon" for i.MX7S/D.
> > > -- interrupts : the interrupt output of the controller:
> > > - i.MX6Q has one IRQ which will be triggered when temperature is
> > > higher than high threshold,
> > > - i.MX6SX and i.MX7S/D have two more IRQs than i.MX6Q, one is IRQ_LOW
> > > and the other is IRQ_PANIC,
> > > - when temperature is below than low threshold, IRQ_LOW will be
> > > triggered, when temperature
> > > - is higher than panic threshold, system will auto reboot by SRC module.
> > > -- fsl,tempmon : phandle pointer to system controller that contains
> > > TEMPMON
> > > - control registers, e.g. ANATOP on imx6q.
> > > -- nvmem-cells: A phandle to the calibration cells provided by ocotp.
> > > -- nvmem-cell-names: Should be "calib", "temp_grade".
> > > -
> > > -Deprecated properties:
> > > -- fsl,tempmon-data : phandle pointer to fuse controller that contains
> > > TEMPMON
> > > - calibration data, e.g. OCOTP on imx6q. The details about
> > > calibration data
> > > - can be found in SoC Reference Manual.
> > > -
> > > -Direct access to OCOTP via fsl,tempmon-data is incorrect on some
> > > newer chips -because it does not handle OCOTP clock requirements.
> > > -
> > > -Optional properties:
> > > -- clocks : thermal sensor's clock source.
> > > -
> > > -Example:
> > > -ocotp: ocotp@21bc000 {
> > > - #address-cells = <1>;
> > > - #size-cells = <1>;
> > > - compatible = "fsl,imx6sx-ocotp", "syscon";
> > > - reg = <0x021bc000 0x4000>;
> > > - clocks = <&clks IMX6SX_CLK_OCOTP>;
> > > -
> > > - tempmon_calib: calib@38 {
> > > - reg = <0x38 4>;
> > > - };
> > > -
> > > - tempmon_temp_grade: temp-grade@20 {
> > > - reg = <0x20 4>;
> > > - };
> > > -};
> > > -
> > > -tempmon: tempmon {
> > > - compatible = "fsl,imx6sx-tempmon", "fsl,imx6q-tempmon";
> > > - interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> > > - fsl,tempmon = <&anatop>;
> > > - nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
> > > - nvmem-cell-names = "calib", "temp_grade";
> > > - clocks = <&clks IMX6SX_CLK_PLL3_USB_OTG>;
> > > -};
> > > -
> > > -Legacy method (Deprecated):
> > > -tempmon {
> > > - compatible = "fsl,imx6q-tempmon";
> > > - fsl,tempmon = <&anatop>;
> > > - fsl,tempmon-data = <&ocotp>;
> > > - clocks = <&clks 172>;
> > > -};
> > > diff --git
> > > a/Documentation/devicetree/bindings/thermal/imx-thermal.yaml
> > > b/Documentation/devicetree/bindings/thermal/imx-thermal.yaml
> > > new file mode 100644
> > > index 0000000..ad12622
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/thermal/imx-thermal.yaml
> > > @@ -0,0 +1,97 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > >
> > +cetree.org%2Fschemas%2Fthermal%2Fimx-thermal.yaml%23&data=02%
> > 7C01
> > >
> > +%7Canson.huang%40nxp.com%7C408494436dca450abb5d08d7dc8cc727%7
> > C686ea1d
> > >
> > +3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637220369539115517&s
> > data=e3R5
> > > +q1YhIRLSOVdLYJWVlbOdqdjxYHNJMcXz%2Bxb1kjc%3D&reserved=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > >
> > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cans
> > on.hua
> > >
> > +ng%40nxp.com%7C408494436dca450abb5d08d7dc8cc727%7C686ea1d3bc
> > 2b4c6fa92
> > >
> > +cd99c5c301635%7C0%7C1%7C637220369539125508&sdata=VjZ%2FPJ
> > BVQc9jpc
> > > +H3Hvvc2fYy70B%2Fmun0mxdvSHHktEU%3D&reserved=0
> > > +
> > > +title: NXP i.MX Thermal Binding
> > > +
> > > +maintainers:
> > > + - Anson Huang <Anson.Huang@xxxxxxx>
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - fsl,imx6q-tempmon
> > > + - fsl,imx6sx-tempmon
> > > + - fsl,imx7d-tempmon
> > > +
> >
> > > + interrupts:
> > > + description: |
> > > + The interrupt output of the controller, the IRQ will be triggered
> > > + when temperature is higher than high threshold.
> > > + maxItems: 1
> > > +
> >
> > imx6sx and imx7d have 3 interrupts each. So you need an if clause to change
> > the interrupt number based on what compatible is provided. See
> > qcom-tsens.yaml for an example.
>
> I noticed this, yes, i.MX6SX/7D has 3 interrupts, but the driver actually ONLY uses
> the high irq, low and panic irq are NOT used. So for such scenario, binding doc should
> still mention all of them?
No, just describe what the driver implements. If the driver doesn't
implement that other two interrupts, leave it out.
I was only comparing it to the old binding where those 3 interrupts
are mentioned.
Regards,
Amit
> >
> > > + nvmem-cells:
> > > + description: |
> > > + Phandle to the calibration cells provided by ocotp for calibration
> > > + data and temperature grade.
> > > + maxItems: 2
> > > +
> > > + nvmem-cell-names:
> > > + maxItems: 2
> > > + items:
> > > + - const: calib
> > > + - const: temp_grade
> > > +
> > > + fsl,tempmon:
> > > + description: |
> > > + Phandle pointer to system controller that contains TEMPMON
> > control
> > > + registers, e.g. ANATOP on imx6q.
> > > + $ref: '/schemas/types.yaml#/definitions/phandle'
> >
> > Nit: move $ref line above description. IMO it makes the binding easier to read.
>
> OK.
>
> >
> > > +
> > > + fsl,tempmon-data:
> > > + description: |
> > > + Deprecated property, phandle pointer to fuse controller that
> > contains
> > > + TEMPMON calibration data, e.g. OCOTP on imx6q. The details about
> > > + calibration data can be found in SoC Reference Manual.
> > > + $ref: '/schemas/types.yaml#/definitions/phandle'
> >
> > Nit: move $ref line above description. IMO it makes the binding easier to read.
>
> OK.
>
> >
> > > +
> > > + clocks:
> > > + description: |
> > > + Thermal sensor's clock source.
> > > + maxItems: 1
> > > +
> > > +required:
> > > + - compatible
> > > + - interrupts
> > > + - fsl,tempmon
> > > + - clocks
> >
> > Clocks was an optional property before, are you sure?
>
> I misunderstand the 'optional', the clock is always necessary, ONLY for those SoCs have
> no thermal clock available, then it is unnecessary. Now I see, I will make it as optional.
>
> I will add example for #thermal-sensor-cells property you mentioned in the other mail.
>
> Thanks,
> Anson
>
> >
> > > + - nvmem-cells
> > > + - nvmem-cell-names
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/clock/imx6sx-clock.h>
> > > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +
> > > + ocotp: ocotp@21bc000 {
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + compatible = "fsl,imx6sx-ocotp", "syscon";
> > > + reg = <0x021bc000 0x4000>;
> > > + clocks = <&clks IMX6SX_CLK_OCOTP>;
> > > +
> > > + tempmon_calib: calib@38 {
> > > + reg = <0x38 4>;
> > > + };
> > > +
> > > + tempmon_temp_grade: temp-grade@20 {
> > > + reg = <0x20 4>;
> > > + };
> > > + };
> > > +
> > > + tempmon: tempmon {
> > > + compatible = "fsl,imx6sx-tempmon";
> > > + interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> > > + fsl,tempmon = <&anatop>;
> > > + nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
> > > + nvmem-cell-names = "calib", "temp_grade";
> > > + clocks = <&clks IMX6SX_CLK_PLL3_USB_OTG>;
> > > + };
> > > +
> > > +...
> > > --
> > > 2.7.4
> > >
