Re: [PATCH v3] dt-bindings: thermal: mediatek: Add LVTS thermal controllers

[Krzysztof Kozlowski]

On 31/01/2023 15:04, bchihi@xxxxxxxxxxxx wrote:
> From: Balsam CHIHI <bchihi@xxxxxxxxxxxx>
> 
> Add LVTS thermal controllers dt-binding definition for mt8195.
> 
> Signed-off-by: Balsam CHIHI <bchihi@xxxxxxxxxxxx>
> ---
> Changelog:
>   v3:
>      - Fixed subject prefix
>      - Fixed licenses GPL-2.0-only OR BSD-2-Clause
>        to GPL-2.0 OR MIT (to match DT)
>      - Fixed matching dt-binding file names

If this is v3 with only one patch, where is the driver or DTS using
these bindings? Please link it. It's unusual to see only bindings,
without the users.

>   v2:
>      - Fixed subject prefix
>      - Fixed licenses GPL-2.0+ to GPL-2.0
>      - Added dual licenses

Is there a reason to make our review more difficult and keep versions
broken, threads attached to some other threads?

------------

Grabbing thread from
lore.kernel.org/all/20230131140439.600164-1-bchihi%40baylibre.com/t.mbox.gz
Checking for newer revisions on https://lore.kernel.org/all/
Analyzing 38 messages in the thread
Will use the latest revision: v12
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v12] thermal: drivers: mediatek: Add the Low Voltage Thermal
Sensor driver
    ✓ Signed: DKIM/baylibre-com.20210112.gappssmtp.com (From:
bchihi@xxxxxxxxxxxx)
    + Link:
https://lore.kernel.org/r/20230131153816.21709-1-bchihi@xxxxxxxxxxxx
  ✓ [PATCH v12 2/6] dt-bindings: thermal: mediatek: Add LVTS thermal
controllers dt-binding definition
    ✓ Signed: DKIM/baylibre-com.20210112.gappssmtp.com (From:
bchihi@xxxxxxxxxxxx)
    + Link:
https://lore.kernel.org/r/20230126161048.94089-1-bchihi@xxxxxxxxxxxx
  ERROR: missing [3/1]!
  ERROR: missing [4/1]!
  ERROR: missing [5/1]!
  ERROR: missing [6/1]!

--------

b4 diff '<20230131140439.600164-1-bchihi@xxxxxxxxxxxx>'
Checking for older revisions on https://lore.kernel.org/all/
---
Analyzing 38 messages in the thread
Assuming new revision: v4 ([PATCH v12] thermal: drivers: mediatek: Add
the Low Voltage Thermal Sensor driver)
Preparing fake-am for v3: dt-bindings: thermal: mediatek: Add LVTS
thermal controllers
  range: 291580cde5f6..de7fe5e0293a
Preparing fake-am for v12: arm64: dts: mediatek: mt8195: Add thermal
zones and thermal nodes
  ERROR: Could not find matching blob for
arch/arm64/boot/dts/mediatek/mt8195.dtsi (09df105f4606)
         If you know on which tree this patchset is based,
         add it as a remote and perform "git remote update"
         in order to fetch the missing objects.
---
Could not create fake-am range for upper series v12


> ---
> ---
>  .../thermal/mediatek,lvts-thermal.yaml        | 107 ++++++++++++++++++
>  .../thermal/mediatek,lvts-thermal.h           |  19 ++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>  create mode 100644 include/dt-bindings/thermal/mediatek,lvts-thermal.h
> 
> 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..5fa5c7a1a417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR MIT)

WARNING: DT binding documents should be licensed (GPL-2.0-only OR
BSD-2-Clause)
#24: FILE:
Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml:1:
+# SPDX-License-Identifier: (GPL-2.0 OR MIT)

I asked you to use the binding license for header file. Then you changed
binding license... why? Why do you need other SPDX text? Why do you need
MIT?

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/mediatek,lvts-thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek SoC Low Voltage Thermal Sensor (LVTS)
> +
> +maintainers:
> +  - Balsam CHIHI <bchihi@xxxxxxxxxxxx>
> +
> +description: |
> +  LVTS is a thermal management architecture composed of three subsystems,
> +  a Sensing device - Thermal Sensing Micro Circuit Unit (TSMCU),
> +  a Converter - Low Voltage Thermal Sensor converter (LVTS), and
> +  a Digital controller (LVTS_CTRL).
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8195-lvts-ap
> +      - mediatek,mt8195-lvts-mcu

What about other devices? You called the file name as generic for all
Mediatek SoCs, so why only one SoC is here? Is there going to be more?
If yes, why they cannot be added now?

Best regards,
Krzysztof