On Thu 29 Jul 06:48 PDT 2021, AngeloGioacchino Del Regno wrote:

Il 29/07/21 13:14, Thara Gopinath ha scritto:

quoted

On 7/29/21 6:55 AM, Konrad Dybcio wrote:

quoted

On 29.07.2021 12:54, Thara Gopinath wrote:

quoted

On 7/29/21 6:52 AM, Konrad Dybcio wrote:

quoted

On 29.07.2021 12:50, Thara Gopinath wrote:

quoted

Hi Konrad,

On 7/28/21 6:25 PM, Konrad Dybcio wrote:

quoted

This will enable temperature reporting for various SoC
components.

Signed-off-by: AngeloGioacchino Del Regno
[off-list ref]
Signed-off-by: Konrad Dybcio <redacted>
---
    .../devicetree/bindings/thermal/qcom-tsens.yaml       |  1 +
    arch/arm64/boot/dts/qcom/sdm630.dtsi                  | 11 +++++++++++
    2 files changed, 12 insertions(+)

diff --git
a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 4a2eaf28e3fd..d3b9e9b600a2 100644

--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml

@@ -48,6 +48,7 @@ properties:

                  - qcom,sc7180-tsens
                  - qcom,sc7280-tsens
                  - qcom,sc8180x-tsens
+              - qcom,sdm630-tsens
                  - qcom,sdm845-tsens
                  - qcom,sm8150-tsens
                  - qcom,sm8250-tsens

diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi

b/arch/arm64/boot/dts/qcom/sdm630.dtsi
index 1e54828817d5..7e9c80e35fba 100644

--- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi

@@ -627,6 +627,17 @@ mnoc: interconnect@1745000 {

                     <&mmcc AHB_CLK_SRC>;
            };
    +        tsens: thermal-sensor@10ae000 {
+            compatible = "qcom,sdm630-tsens", "qcom,tsens-v2";
+            reg = <0x010ae000 0x1000>, /* TM */
+                  <0x010ad000 0x1000>; /* SROT */
+            #qcom,sensors = <12>;

Are all 12 sensors used ? I see that in a later patch
"arm64: dts: qcom: sdm630: Add thermal-zones
configuration" only 9 are used.

Hi,

if I recall correctly, they all give output but not all of
the mappings were documented in the downstream sources and
we have no documentation whatsoever :(

Right. In that case, why not change #qcom,sensors to 9 and add
rest of the sensors if and when needed ?

I don't think it makes sense to describe the hardware incorrectly,
even if some of it is unused.

My thinking was more along the lines of don't expose unused h/w bits.

You're right about not exposing unused HW bits, but even PC x86 motherboards
(I mean the smbus/i2c drivers for the big holy management/sensors chips) do
have such a "base" configuration, where some lines are read as 0 because they
are effectively not connected by hardware.

In order to avoid confusion to other developers, in my personal opinion, it would
be good go for the current value of 12 (which isn't incorrect, as that's what the
SoC supports)... I don't think that anyone would be confused by seeing zero
readings on some sensors (if their device don't support such sensor), as I think
that everyone is used to that anyway, even if that's in other circumstances...

In any case, luckily that's also safe, because there's no firmware that restricts
the readings to a subset of sensors in this domain (nobody is going to get a
hypervisor fault for that).

I would also, in case, propose to see how things go: I would expect other
developers to push device trees for many SDM630/636/660 devices, including but
not limited to smartphones and SBCs.. so perhaps if we find out that really
nobody uses the 12 sensors, or if the very vast majority uses a different amount,
perhaps we may just transfer the value to device-specific configurations in one
go, as to avoid unnecessary noise... I think :)))

If the SoC has 12 sensors I think it makes sense to define that, similar
to how a SoC might have 200 GPIOs, even though only a handful is
actually used.

Regards,
Bjorn