Re: [PATCH v4 3/6] thermal: tsens: Rename tsens-8996 to tsens-v2 for reuse

On Tue, Jul 3, 2018 at 12:23 AM Bjorn Andersson
[off-list ref] wrote:

On Mon 02 Jul 05:44 PDT 2018, Amit Kucheria wrote:

quoted

The TSENS block inside the 8996 is internally classified as version 2 of
the IP. Several other SoC families use this block and can share this code.

We rename get_temp() to reflect that it can be used across the v2 family.

Signed-off-by: Amit Kucheria <redacted>
---
 drivers/thermal/qcom/Makefile                     |  2 +-
 drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} | 26 ++++++++---------------
 2 files changed, 10 insertions(+), 18 deletions(-)
 rename drivers/thermal/qcom/{tsens-8996.c => tsens-v2.c} (66%)

diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 2cc2193..a821929 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile

@@ -1,2 +1,2 @@
 obj-$(CONFIG_QCOM_TSENS)     += qcom_tsens.o
-qcom_tsens-y                 += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o
+qcom_tsens-y                 += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o

diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-v2.c
similarity index 66%
rename from drivers/thermal/qcom/tsens-8996.c
rename to drivers/thermal/qcom/tsens-v2.c
index e1f7781..2eca7ff 100644
--- a/drivers/thermal/qcom/tsens-8996.c
+++ b/drivers/thermal/qcom/tsens-v2.c

@@ -1,27 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
+ * Copyright (c) 2018, Linaro Limited
  */

-#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include "tsens.h"

-#define STATUS_OFFSET        0x10a0
-#define LAST_TEMP_MASK       0xfff
+#define STATUS_OFFSET                0xa0

This is not backwards compatible with present day dts files, you need to
keep this effectively 0x10a0 when the memory region isn't split in two.

Argh!! Good catch.

Perhaps you can just offset the ioremap by 4k when there's only one
region?

That'll cause problems when we want to access some features exposed in
the other register bank through the common get_temp_tsens_v2 function.
The SW reset bit is in there that I'd like to add support for, for
example.

I can see a few other ways to detect this at runtime. None of them
look pretty but option 3 looks least ugly at least to me. Any
preferences?

1. Don't try to combine get_temp for 8996 with other v2 platforms.
This would mean retaining a separate ops_8996, pointing to a custom
get_temp(). We end up with two copies of the function.

2. In get_temp_tsens_v2(), check for number of regions but this has an
overhead for each temperature read.

     struct platform_device *op = of_find_device_by_node(tmdev->dev->of_node);
     if (op->num_resources > 1)
        status_offset = 0xa0;
     else
        status_offset = 0x10a0;
     Read temperature(status_offset);

3. Restrict to init time (ideal) by storing the status_offset to
struct tsens_device. This is then used directly in
get_temp_generic_v2() to determine the actual address. This should
work for 8996, 8916 and 8974 which all have a single memory region
iomapp'ed. The last two don't even need this value since they have
their own get_temp functions. See diff attached.

I have to ask: Are there really devices in the field that rely on the
existing *upstream* code and DT? If not, could we consider deprecating
the "qcom,msm8996-tsens" property immediately or at least mark it for
future removal?

Regards,
Amit