Re: [PATCH v4 4/6] thermal: tsens: Add support for SDM845

On Thu, Jul 5, 2018 at 11:13 PM Amit Kucheria [off-list ref] wrote:

On Fri, Jul 6, 2018 at 2:07 AM Rob Herring [off-list ref] wrote:

quoted

On Wed, Jul 04, 2018 at 10:56:26PM +0530, Amit Kucheria wrote:

quoted

On Tue, Jul 3, 2018 at 9:56 PM, Rob Herring [off-list ref] wrote:

quoted

On Mon, Jul 02, 2018 at 06:14:07PM +0530, Amit Kucheria wrote:

quoted

SDM845 uses v2.4.0 of the TSENS IP block but the get_temp() function
appears to be identical across v2.x.y in code seen so far. We use the
generic get_temp() function.

Signed-off-by: Amit Kucheria <redacted>
---
 Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 2 ++
 drivers/thermal/qcom/tsens-v2.c                          | 6 +++++-
 drivers/thermal/qcom/tsens.c                             | 6 ++++++
 drivers/thermal/qcom/tsens.h                             | 5 ++++-
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
index 06195e8..075182e 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt

@@ -5,6 +5,8 @@ Required properties:
  - "qcom,msm8916-tsens" : For 8916 Family of SoCs
  - "qcom,msm8974-tsens" : For 8974 Family of SoCs
  - "qcom,msm8996-tsens" : For 8996 Family of SoCs
+ - "qcom,tsens-v2.4.0"  : For SDM845 Family of SoCs
+ - "qcom,tsens-v2"      : Generic fallback binding for any Soc using 2.x.y version of the tsens IP

You need to show what are valid combinations of compatibles. Does v2
apply to 8996? One valid combination per line.

I've restructured qcom-tsens.txt to look like this:

-----%<-------

* QCOM SoC Temperature Sensor (TSENS)

Required properties:
- compatible: must be one of the following:
    - "qcom,msm8916-tsens" (MSM8916)
    - "qcom,msm8974-tsens" (MSM8974)
    - "qcom,msm8996-tsens" (MSM8996)
    - "qcom,tsens-<ip_version>", "qcom,tsens-v2" (TSENS IP version and a
       generic v2 property as fallback except for MSM8996)

  Examples with ip_version are:
    - "qcom,tsens-v2.4.0", "qcom,tsens-v2" (SDM845)
    - "qcom,tsens-v2.2.1", "qcom,tsens-v2" (MSM8998)

-----%<-------

8996 would end up being something like this if needed, though we're
stuck with "qcom,msm8996-tsens":
"qcom,msm8996-tsens", "qcom,tsens-v2.1.0", "qcom,tsens-v2" (MSM8996)

3 versions here for 3 SoCs. I'm not getting that convinced version
numbers really are better. I would assume that other QCom IP blocks

Yeah, it is a bit unfortunate that the 3-4 SoCs we're focusing on
getting supported upstream have different versions of the TSENS IP.
The other goal of this work was to make the upstream driver
feature-complete so we can make a case to switch to it in the
downstream trees, even on platforms that aren't being active
upstreamed. They'll still need to keep around those SoC-specific
one-liner patches in the downstream trees.

Why? They can just use the "v2" fallback (which was sufficient for
your original version). That's not my recommendation, but what do I
care for downstream. Only upstream needs the specific strings to
appease the annoying DT maintainers.

Plus, if it's not upstream, it doesn't exist. :)

quoted

have versions too, but pretty much *every* *other* binding uses SoC names.
Why is this one special?

I'm not an expert on all QC IPs, but this one _seems_ to be reused a
lot more than others.

quoted

The other problem with versions is the mapping
of versions to SoCs most likely can't be validated outside of QCom
unless there's a version register.

There is in fact a HW version register that I was hoping to add
support for later.

Yes, you should rely on that as much as possible. But I have seen h/w
designers forget to update revision registers or there can be
integration differences even if the IP version is the same.

quoted

So, sorry to go in circles, but can you go back to qcom,<soc>-tsens. You
can keep qcom,tsens-v2 as a fallback.

OK.

quoted

Yes, it's annoying to have to update bindings for new SoCs. But it's
trivial one line patches. Look at Renesas bindings. Maybe adding new
ones will be scriptable once we move to json-schema binding docs.

I did look at the Renesas RCar bindings when restructuring the
documentation. They seem to have settled upon a 3-level fallback:
"soc-specific", "generic-no TZ", "generic-TZ". But
drivers/thermal/rcar_thermal.c seems to be compatible with only one
soc-specific property (thermal-r8a77995) and all other SoCs seem to be
just relying the fallbacks.

Anyways, I'll respin.

Thanks.

Regards,
Amit