diff mbox series

[2/5] dt-bindings: thermal: qcom: Add MBG thermal monitor bindings

Message ID 20240712-mbg-tm-support-v1-2-7d78bec920ca@quicinc.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for MBG Thermal monitoring device | expand

Commit Message

Satya Priya Kakitapalli July 12, 2024, 12:43 p.m. UTC
Add bindings support for the MBG Temp alarm peripheral found on
pm8775 pmics.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
 .../bindings/thermal/qcom-spmi-mbg-tm.yaml         | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Rob Herring (Arm) July 12, 2024, 2:34 p.m. UTC | #1
On Fri, 12 Jul 2024 18:13:29 +0530, Satya Priya Kakitapalli wrote:
> Add bindings support for the MBG Temp alarm peripheral found on
> pm8775 pmics.
> 
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>  .../bindings/thermal/qcom-spmi-mbg-tm.yaml         | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
In file included from Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.example.dts:25:
./scripts/dtc/include-prefixes/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8775.h:9:10: fatal error: dt-bindings/iio/adc/qcom,spmi-vadc.h: No such file or directory
    9 | #include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240712-mbg-tm-support-v1-2-7d78bec920ca@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski July 12, 2024, 5:25 p.m. UTC | #2
On 12/07/2024 14:43, Satya Priya Kakitapalli wrote:
> Add bindings support for the MBG Temp alarm peripheral found on
> pm8775 pmics.
> 
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>  .../bindings/thermal/qcom-spmi-mbg-tm.yaml         | 63 ++++++++++++++++++++++

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

>  1 file changed, 63 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
> new file mode 100644
> index 000000000000..9b6d1bc34a11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/qcom-spmi-mbg-tm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. SPMI PMIC MBG Thermal Monitoring
> +
> +maintainers:
> +  - Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Qualcomm's thermal driver for the MBG thermal monitoring device.

Driver as Linux driver? Instead please describe the hardware.

Missing $ref to thermal-sensor.

> +
> +properties:
> +  compatible:
> +    const: qcom,spmi-mbg-tm

Instead use SoC specific compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  io-channels:

Missing constraints. Use items with description.
> +    description:
> +      IIO channel specifier for the ADC channel, which reports

And drop redundant part - "IIO channel specifier for". This cannot be
anything else.

> +      chip die temperature.
> +
> +  io-channel-names:
> +    const: thermal
> +
> +  "#thermal-sensor-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - io-channels
> +  - io-channel-names
> +  - "#thermal-sensor-cells"

And this won't be needed.

> +
> +additionalProperties: false

unevaluatedProperties instead

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8775.h>
> +    spmi_bus {

Eh... No. Is this really directly on SPMI bus? Anyway, use correct node
names.

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      pm8775_sail_1_tz: qcom,mbg-tm@d700 {

Oh no, please don't bring downstream crap.

Do you see any node called like this?

Also, drop unused label.

> +        compatible = "qcom,spmi-mbg-tm";
> +        reg = <0xd700>;
> +        interrupts = <0x1 0xd7 0x0 IRQ_TYPE_EDGE_RISING>;

This suggests it is not on SPMI bus but part of PMIC. Why doing
something entirely different then entire Linux kernel? Do not use
downstream as template, that's a no go.

Best regards,
Krzysztof
Konrad Dybcio July 12, 2024, 5:37 p.m. UTC | #3
On 12.07.2024 7:25 PM, Krzysztof Kozlowski wrote:
> On 12/07/2024 14:43, Satya Priya Kakitapalli wrote:
>> Add bindings support for the MBG Temp alarm peripheral found on
>> pm8775 pmics.
>>
>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>> ---

Please also describe what MBG stands for and how it differs from the
currently supported temp alarm that's been in use for the past 10 years
on various PMICs

Konrad
Dmitry Baryshkov July 13, 2024, 4:14 p.m. UTC | #4
On Fri, Jul 12, 2024 at 06:13:29PM GMT, Satya Priya Kakitapalli wrote:
> Add bindings support for the MBG Temp alarm peripheral found on
> pm8775 pmics.
> 
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>  .../bindings/thermal/qcom-spmi-mbg-tm.yaml         | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
> new file mode 100644
> index 000000000000..9b6d1bc34a11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/qcom-spmi-mbg-tm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. SPMI PMIC MBG Thermal Monitoring
> +
> +maintainers:
> +  - Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> +
> +description: |
> +  Qualcomm's thermal driver for the MBG thermal monitoring device.

I was hoping for the binding to tell me, what is MBG. But they don't.
Could you please fix that?

> +
> +properties:
> +  compatible:
> +    const: qcom,spmi-mbg-tm
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
new file mode 100644
index 000000000000..9b6d1bc34a11
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-mbg-tm.yaml
@@ -0,0 +1,63 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/qcom-spmi-mbg-tm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. SPMI PMIC MBG Thermal Monitoring
+
+maintainers:
+  - Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
+
+description: |
+  Qualcomm's thermal driver for the MBG thermal monitoring device.
+
+properties:
+  compatible:
+    const: qcom,spmi-mbg-tm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  io-channels:
+    description:
+      IIO channel specifier for the ADC channel, which reports
+      chip die temperature.
+
+  io-channel-names:
+    const: thermal
+
+  "#thermal-sensor-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - io-channels
+  - io-channel-names
+  - "#thermal-sensor-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8775.h>
+    spmi_bus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pm8775_sail_1_tz: qcom,mbg-tm@d700 {
+        compatible = "qcom,spmi-mbg-tm";
+        reg = <0xd700>;
+        interrupts = <0x1 0xd7 0x0 IRQ_TYPE_EDGE_RISING>;
+        io-channels = <&pm8775_1_adc PM8775_ADC5_GEN3_DIE_TEMP(1)>;
+        io-channel-names = "thermal";
+        #thermal-sensor-cells = <0>;
+      };
+    };
+...