Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220817080757.352021-1-bchihi@baylibre.com> <20220817080757.352021-3-bchihi@baylibre.com>
 <716d6471-f0e2-489e-5f9e-9e38e9e7953a@collabora.com>
In-Reply-To: <716d6471-f0e2-489e-5f9e-9e38e9e7953a@collabora.com>
From:   Balsam CHIHI <bchihi@baylibre.com>
Date:   Wed, 14 Sep 2022 16:01:22 +0200
Message-ID: <CAGuA+orxfcycwcUMpLe+dkjnXPQkELQsz0vBggGKTQ04XRGc+g@mail.gmail.com>
Subject: Re: [PATCH v9,2/7] dt-bindings: thermal: Add dt-binding document for
 LVTS thermal controllers
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     rafael@kernel.org, rui.zhang@intel.com, daniel.lezcano@linaro.org,
        amitk@kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, khilman@baylibre.com,
        mka@chromium.org, robh+dt@kernel.org, krzk+dt@kernel.org,
        matthias.bgg@gmail.com, p.zabel@pengutronix.de,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, james.lo@mediatek.com,
        fan.chen@mediatek.com, louis.yu@mediatek.com,
        rex-bc.chen@mediatek.com, abailon@baylibre.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Angelo,

I've got the following errors after implementing these changes :
[...]
  nvmem-cells:
    minItems: 1
    description: Calibration eFuse data for LVTS

  nvmem-cell-names:
    minItems: 1
    items:
      pattern: 'lvts-calib-data[0-9]+$'

  "#thermal-sensor-cells":
    const: 1

allOf:
  - $ref: thermal-sensor.yaml#

  - if:
      properties:
        compatible:
          contains:
            enum:
              - mediatek,mt8192-lvts-ap
              - mediatek,mt8192-lvts-mcu
    then:
      properties:
        nvmem-cells:
          maxItems: 1

        nvmem-cell-names:
          maxItems: 1

  - if:
      properties:
        compatible:
          contains:
            enum:
              - mediatek,mt8195-lvts-ap
              - mediatek,mt8195-lvts-mcu
    then:
      properties:
        nvmem-cells:
          maxItems: 2

        nvmem-cell-names:
          maxItems: 2
[...]

$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
/home/balsam/src/linux-mtk-lvts-newThermalOF/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml:
properties:nvmem-cell-names:items: {'pattern':
'lvts-calib-data[0-9]+$'} is not of type 'array'
from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/balsam/src/linux-mtk-lvts-newThermalOF/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml:
ignoring, error in schema: properties: nvmem-cell-names: items
  DTEX    Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.example.dts
  DTC     Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.example.dtb
  CHECK   Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.example.dtb
Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.example.dtb:0:0:
/example-0/soc/thermal-sensor@1100b000: failed to match any schema
with compatible: ['mediatek,mt8192-lvts-ap']

am I missing something?

Best regards,
Balsam

On Wed, Sep 14, 2022 at 2:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 17/08/22 10:07, bchihi@baylibre.com ha scritto:
> > From: Alexandre Bailon <abailon@baylibre.com>
> >
> > Add dt-binding document for mt8192 and mt8195 LVTS thermal controllers.
> >
> > Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> > Co-developed-by: Balsam CHIHI <bchihi@baylibre.com>
> > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> > ---
> >   .../thermal/mediatek,lvts-thermal.yaml        | 152 ++++++++++++++++++
> >   1 file changed, 152 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> > new file mode 100644
> > index 000000000000..31d9e220513a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> > @@ -0,0 +1,152 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/thermal/mediatek,lvts-thermal.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek SoC LVTS thermal controller
>
> title: MediaTek SoC Low Voltage Thermal Sensor (LVTS)
>
> > +
> > +maintainers:
> > +  - Yu-Chia Chang <ethan.chang@mediatek.com>
> > +  - Ben Tseng <ben.tseng@mediatek.com>
> > +
> > +description: |
>
> description:
>    LVTS is a thermal management architecture composed of three subsystems,
>    a Sensing device - Thermal Sensing Micro Circuit Unit (TSMCU),
>    a Convertor - Low Voltage Thermal Sensor convertor (LVTS), and
>    a Digital controller (LVTS_CTRL).
>
> > +  LVTS (Low Voltage Thermal Sensor).
> > +  The architecture will be first used on mt8192 and mt8195.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8192-lvts-ap
> > +      - mediatek,mt8192-lvts-mcu
> > +      - mediatek,mt8195-lvts-ap
> > +      - mediatek,mt8195-lvts-mcu
> > +
> > +  "#thermal-sensor-cells":
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: LVTS instance registers.
>
> This description looks obvious, as it doesn't really say anything "new"...
> I would rather drop it.
>
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +    description: LVTS instance interrupts.
>
> Same here
>
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description: LVTS instance clock.
>
> and here.
>
> > +
> > +  resets:
> > +    maxItems: 1
> > +    description: |
> > +      LVTS instance SW reset for HW AP/MCU domain to clean temporary data
> > +      on HW initialization/resume.
>
> What about something like...
>
>    resets:
>      items:
>        - description: LVTS reset for clearing temporary data on AP/MCU
>
> > +
> > +  nvmem-cells:
> > +    minItems: 1
> > +    maxItems: 2
> > +    description: Calibration efuse data for LVTS
>
>    nvmem-cells:
>      minItems: 1
>      items:
>        - description: Calibration eFuse data for LVTS
>        - description: Additional eFuse data (?)
>
>
> > +
> > +  nvmem-cell-names:
> > +    minItems: 1
> > +    maxItems: 2
> > +    description: Calibration efuse cell names for LVTS
>
> Actually, maxItems is not really two, but it depends on how many
> eFuse arrays / nvmem cells we have for each SoC, so I was thinking...
>
> ...what about doing something like
>
>    nvmem-cell-names:
>      minItems: 1
>      items:
>        pattern: 'lvts-calib-data[0-9]+$'
>
> and then,
>    if:
>      properties:
>        compatible:
>          contains:
>            enum:
>              - mediatek,blahblah-something
>    then:
>      properties:
>        nvmem-cell-names:
>          maxItems: 2 (or 3, 4, 5...)
>
> P.S.: I haven't tried any binding check on the proposed lines.
>
> Krzysztof, any opinions on that?
>
> Regards,
> Angelo
>
>
>