diff mbox series

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

Message ID 20230131140439.600164-1-bchihi@baylibre.com
State New
Headers
Series [v3] dt-bindings: thermal: mediatek: Add LVTS thermal controllers |

Commit Message

Balsam CHIHI Jan. 31, 2023, 2:04 p.m. UTC 
  From: Balsam CHIHI <bchihi@baylibre.com>

Add LVTS thermal controllers dt-binding definition for mt8195.

Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
---
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
  v2:
     - Fixed subject prefix
     - Fixed licenses GPL-2.0+ to GPL-2.0
     - Added dual licenses
---
---
 .../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

Comments

Krzysztof Kozlowski Feb. 1, 2023, 7:46 a.m. UTC | #1 
On 31/01/2023 15:04, bchihi@baylibre.com wrote:
> From: Balsam CHIHI <bchihi@baylibre.com>
> 
> Add LVTS thermal controllers dt-binding definition for mt8195.
> 
> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> ---
> 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@baylibre.com)
    + Link:
https://lore.kernel.org/r/20230131153816.21709-1-bchihi@baylibre.com
  ✓ [PATCH v12 2/6] dt-bindings: thermal: mediatek: Add LVTS thermal
controllers dt-binding definition
    ✓ Signed: DKIM/baylibre-com.20210112.gappssmtp.com (From:
bchihi@baylibre.com)
    + Link:
https://lore.kernel.org/r/20230126161048.94089-1-bchihi@baylibre.com
  ERROR: missing [3/1]!
  ERROR: missing [4/1]!
  ERROR: missing [5/1]!
  ERROR: missing [6/1]!

--------

b4 diff '<20230131140439.600164-1-bchihi@baylibre.com>'
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@baylibre.com>
> +
> +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
Balsam CHIHI Feb. 1, 2023, 1:34 p.m. UTC | #2 
Hi Krzysztof

On Wed, Feb 1, 2023 at 8:46 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 31/01/2023 15:04, bchihi@baylibre.com wrote:
> > From: Balsam CHIHI <bchihi@baylibre.com>
> >
> > Add LVTS thermal controllers dt-binding definition for mt8195.
> >
> > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> > ---
> > 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.

sorry, I'll be careful next time.
and I will take into account the new change requests in the next full
version of the series.
I apologize for the mess.

>
> >   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?

sorry again.

>
> ------------
>
> 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@baylibre.com)
>     + Link:
> https://lore.kernel.org/r/20230131153816.21709-1-bchihi@baylibre.com
>   ✓ [PATCH v12 2/6] dt-bindings: thermal: mediatek: Add LVTS thermal
> controllers dt-binding definition
>     ✓ Signed: DKIM/baylibre-com.20210112.gappssmtp.com (From:
> bchihi@baylibre.com)
>     + Link:
> https://lore.kernel.org/r/20230126161048.94089-1-bchihi@baylibre.com
>   ERROR: missing [3/1]!
>   ERROR: missing [4/1]!
>   ERROR: missing [5/1]!
>   ERROR: missing [6/1]!
>
> --------
>
> b4 diff '<20230131140439.600164-1-bchihi@baylibre.com>'
> 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?

I will put it back "GPL-2.0-only OR BSD-2-Clause" in the binding and
do the same for the header.

>
> > +%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@baylibre.com>
> > +
> > +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?

Yes, there is another MTK SoC mt8192 that supports LVTS,
I was asked in v10 of the series to remove the unimplemented SoC.
It will be added later with the driver that supports it.
just let me know if you still want to add mt8192 bindings in the next
version without the driver.
(LVTS support for mt8192 will be sent in a different series).

>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 1, 2023, 1:37 p.m. UTC | #3 
On 01/02/2023 14:34, Balsam CHIHI wrote:
>>> +
>>> +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?
> 
> Yes, there is another MTK SoC mt8192 that supports LVTS,
> I was asked in v10 of the series to remove the unimplemented SoC.
> It will be added later with the driver that supports it.
> just let me know if you still want to add mt8192 bindings in the next
> version without the driver.

The binding should be complete, if that's possible, so if you had mt8192
already there, it could stay. Anyway it's fine then.

Best regards,
Krzysztof
Balsam CHIHI Feb. 1, 2023, 1:56 p.m. UTC | #4 
Hi Krzysztof,

On Wed, Feb 1, 2023 at 2:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 01/02/2023 14:34, Balsam CHIHI wrote:
> >>> +
> >>> +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?
> >
> > Yes, there is another MTK SoC mt8192 that supports LVTS,
> > I was asked in v10 of the series to remove the unimplemented SoC.
> > It will be added later with the driver that supports it.
> > just let me know if you still want to add mt8192 bindings in the next
> > version without the driver.
>
> The binding should be complete, if that's possible, so if you had mt8192
> already there, it could stay. Anyway it's fine then.

OK, I will put back mt8192 dt-bindings.
this is the link to the v10 patch
"https://patchwork.kernel.org/project/linux-pm/patch/20230112152855.216072-3-bchihi@baylibre.com/",
it will be the same in next version (v13) of series.
it would be great if you review it in advance, so I could take into
account the new changes if needed.
is it possible to resend this patch under v13, to simplify the review
and avoid breaking the series again?

>
> Best regards,
> Krzysztof
>

Best regards,
Balsam
diff mbox series

Patch

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)
+%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@baylibre.com>
+
+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
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+    description: LVTS reset for clearing temporary data on AP/MCU.
+
+  nvmem-cells:
+    minItems: 1
+    items:
+      - description: Calibration eFuse data 1 for LVTS
+      - description: Calibration eFuse data 2 for LVTS
+
+  nvmem-cell-names:
+    minItems: 1
+    items:
+      - const: lvts-calib-data-1
+      - const: lvts-calib-data-2
+
+  "#thermal-sensor-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - resets
+  - nvmem-cells
+  - nvmem-cell-names
+  - "#thermal-sensor-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/mt8195-clk.h>
+    #include <dt-bindings/reset/mt8195-resets.h>
+    #include <dt-bindings/thermal/mediatek,lvts-thermal.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      lvts_mcu: thermal-sensor@11278000 {
+        compatible = "mediatek,mt8195-lvts-mcu";
+        reg = <0 0x11278000 0 0x1000>;
+        interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH 0>;
+        clocks = <&infracfg_ao CLK_INFRA_AO_THERM>;
+        resets = <&infracfg_ao MT8195_INFRA_RST4_THERM_CTRL_MCU_SWRST>;
+        nvmem-cells = <&lvts_efuse_data1 &lvts_efuse_data2>;
+        nvmem-cell-names = "lvts-calib-data-1", "lvts-calib-data-2";
+        #thermal-sensor-cells = <1>;
+      };
+    };
+
+    thermal_zones: thermal-zones {
+      cpu0-thermal {
+        polling-delay = <1000>;
+        polling-delay-passive = <250>;
+        thermal-sensors = <&lvts_mcu MT8195_MCU_LITTLE_CPU0>;
+
+        trips {
+          cpu0_alert: trip-alert {
+            temperature = <85000>;
+            hysteresis = <2000>;
+            type = "passive";
+          };
+
+          cpu0_crit: trip-crit {
+            temperature = <100000>;
+            hysteresis = <2000>;
+            type = "critical";
+          };
+        };
+      };
+    };
diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
new file mode 100644
index 000000000000..ca1ef29a8fee
--- /dev/null
+++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2023 MediaTek Inc.
+ * Author: Balsam CHIHI <bchihi@baylibre.com>
+ */
+
+#ifndef __MEDIATEK_LVTS_DT_H
+#define __MEDIATEK_LVTS_DT_H
+
+#define MT8195_MCU_BIG_CPU0		0
+#define MT8195_MCU_BIG_CPU1		1
+#define MT8195_MCU_BIG_CPU2		2
+#define MT8195_MCU_BIG_CPU3		3
+#define MT8195_MCU_LITTLE_CPU0	4
+#define MT8195_MCU_LITTLE_CPU1	5
+#define MT8195_MCU_LITTLE_CPU2	6
+#define MT8195_MCU_LITTLE_CPU3	7
+
+#endif /* __MEDIATEK_LVTS_DT_H */