[PATCH v3 0/6] Add support for H616 Thermal system

[PATCH v3 0/6] Add support for H616 Thermal system

[Andre Przywara]

Hi,

this is v3 of the series by Martin, now complemented by patches to fix
the problem with way-too-high temperatures reported on some boards. This
seems to be related to the firmware being run, because the vendor U-Boot
contains a hack avoiding this problem. Some investigation and digging
in BSP code later we identified that bit 16 in register 0x3000000
(SYS_CFG) needs to be cleared for the raw temperature register values to
contain reasonable values.
To achieve this, patch 1/6 exports this very register from the already
existing syscon device. Patch 4/6 then adds code to the thermal driver
to find the syscon device via a new DT property, and query its regmap to
clear bit 16 in there.

I am not fully convinced this is the best solution, but it works for me.
What leaves a bit of a bitter taste is that the SRAM driver (the one
exporting the regmap) also uses this register, to switch some SRAM C
region to the video engine (VE). Experiments show that only bit 0 in
this register is doing this job, so the current mask covering the 31
LSBs should probably be amended to only cover bit 0.
Another solution could be to model this bit as an SRAM switch, and let
the THS driver claim some (dummy?) SRAM region from the syscon/SRAM driver
directly. While this sounds cleaner to some degree, I don't think there
is really such a THS SRAM region, so it's not fully correct either.

I would appreciate any feedback on this, happy to implement the other
approach, if that's desired.

The rest of the patches is mostly unchanged from Martin's v2, just
updated and massaged the commit messages a bit. I also added patch 3/6
to document some so-far unknown register value.

Please have a look!

Cheers,
Andre

Changelog v2 .. v3:
- rebase on top of v6.7-rc3
- add patches to clear bit 16 in SYS_CFG register 0x3000000
- add syscon to the binding documentation
- add patch explaining the unknown control register value

Changelog v1 .. v2:
- Fix typos
- Remove h616 calc and init functions
- Use TEMP_CALIB_MASK insteaf of 0xffff
- Adjust calibration function to new offset and scale
- Add proper comment to bindings patch
- Split delta calculations to 2 lines
- Add parentheses around caldata[2|3] >> 12
- Negate bindings if for clocks

Andre Przywara (3):
  soc: sunxi: sram: export register 0 for THS on H616
  thermal: sun8i: explain unknown H6 register value
  thermal: sun8i: add syscon register access code

Martin Botka (3):
  dt-bindings: thermal: sun8i: Add H616 THS controller
  thermal: sun8i: add support for H616 THS controller
  arm64: dts: allwinner: h616: Add thermal sensor and zones

 .../thermal/allwinner,sun8i-a83t-ths.yaml     |  30 ++--
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  88 ++++++++++
 drivers/soc/sunxi/sunxi_sram.c                |   5 +
 drivers/thermal/sun8i_thermal.c               | 152 ++++++++++++++++--
 4 files changed, 252 insertions(+), 23 deletions(-)

-- 
2.35.8

[PATCH v3 1/6] soc: sunxi: sram: export register 0 for THS on H616

[Andre Przywara]

The Allwinner H616 SoC contains a mysterious bit at register offset 0x0
in the SRAM control block. If bit 16 is set (the reset value), the
temperature readings of the THS are way off, leading to reports about
200C, at normal ambient temperatures. Clearing this bits brings the
reported values down to reasonable ranges.
The BSP code clears this bit in firmware (U-Boot), and has an explicit
comment about this, but offers no real explanation.

Since we should not rely on firmware settings, allow other code (the THS
driver) to access this register, by exporting it through the already
existing syscon regmap. This mimics what we already do for the LDO
control and the EMAC register.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/soc/sunxi/sunxi_sram.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
index 4458b2e0562b0..24eba9ebf9f5a 100644
--- a/drivers/soc/sunxi/sunxi_sram.c
+++ b/drivers/soc/sunxi/sunxi_sram.c
@@ -287,6 +287,7 @@ EXPORT_SYMBOL(sunxi_sram_release);
 struct sunxi_sramc_variant {
 	int num_emac_clocks;
 	bool has_ldo_ctrl;
+	bool has_ths_offset;
 };
 
 static const struct sunxi_sramc_variant sun4i_a10_sramc_variant = {
@@ -308,8 +309,10 @@ static const struct sunxi_sramc_variant sun50i_a64_sramc_variant = {
 
 static const struct sunxi_sramc_variant sun50i_h616_sramc_variant = {
 	.num_emac_clocks = 2,
+	.has_ths_offset = true,
 };
 
+#define SUNXI_SRAM_THS_OFFSET_REG	0x0
 #define SUNXI_SRAM_EMAC_CLOCK_REG	0x30
 #define SUNXI_SYS_LDO_CTRL_REG		0x150
 
@@ -318,6 +321,8 @@ static bool sunxi_sram_regmap_accessible_reg(struct device *dev,
 {
 	const struct sunxi_sramc_variant *variant = dev_get_drvdata(dev);
 
+	if (reg == SUNXI_SRAM_THS_OFFSET_REG && variant->has_ths_offset)
+		return true;
 	if (reg >= SUNXI_SRAM_EMAC_CLOCK_REG &&
 	    reg <  SUNXI_SRAM_EMAC_CLOCK_REG + variant->num_emac_clocks * 4)
 		return true;
-- 
2.35.8

[PATCH v3 2/6] dt-bindings: thermal: sun8i: Add H616 THS controller

[Andre Przywara]

From: Martin Botka <martin.botka@somainline.org>

This controller is similar to the H6, but covers four sensors and uses
slightly different calibration methods.
Also the H616 requires to poke a bit in the SYS_CFG register range for
correct operation, so add a "syscon" phandle property to point there.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../thermal/allwinner,sun8i-a83t-ths.yaml     | 30 ++++++++++++-------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
index fbd4212285e28..95a6ab9a5889b 100644
--- a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
+++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
@@ -20,6 +20,7 @@ properties:
       - allwinner,sun50i-a100-ths
       - allwinner,sun50i-h5-ths
       - allwinner,sun50i-h6-ths
+      - allwinner,sun50i-h616-ths
 
   clocks:
     minItems: 1
@@ -63,6 +64,7 @@ allOf:
             enum:
               - allwinner,sun50i-a100-ths
               - allwinner,sun50i-h6-ths
+              - allwinner,sun50i-h616-ths
 
     then:
       properties:
@@ -80,6 +82,18 @@ allOf:
         clock-names:
           minItems: 2
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun50i-h616-ths
+
+    then:
+      properties:
+        syscon:
+          maxItems: 1
+          description: phandle to syscon device allowing access to SYS_CFG registers
+
   - if:
       properties:
         compatible:
@@ -97,16 +111,12 @@ allOf:
           const: 1
 
   - if:
-      properties:
-        compatible:
-          contains:
-            enum:
-              - allwinner,sun8i-h3-ths
-              - allwinner,sun8i-r40-ths
-              - allwinner,sun50i-a64-ths
-              - allwinner,sun50i-a100-ths
-              - allwinner,sun50i-h5-ths
-              - allwinner,sun50i-h6-ths
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - allwinner,sun8i-a83t-ths
 
     then:
       required:
-- 
2.35.8

Re: [PATCH v3 2/6] dt-bindings: thermal: sun8i: Add H616 THS controller

[Krzysztof Kozlowski]

On 28/11/2023 01:58, Andre Przywara wrote:
> From: Martin Botka <martin.botka@somainline.org>
> 
> This controller is similar to the H6, but covers four sensors and uses
> slightly different calibration methods.
> Also the H616 requires to poke a bit in the SYS_CFG register range for
> correct operation, so add a "syscon" phandle property to point there.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../thermal/allwinner,sun8i-a83t-ths.yaml     | 30 ++++++++++++-------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> index fbd4212285e28..95a6ab9a5889b 100644
> --- a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> +++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> @@ -20,6 +20,7 @@ properties:
>        - allwinner,sun50i-a100-ths
>        - allwinner,sun50i-h5-ths
>        - allwinner,sun50i-h6-ths
> +      - allwinner,sun50i-h616-ths
>  
>    clocks:
>      minItems: 1
> @@ -63,6 +64,7 @@ allOf:
>              enum:
>                - allwinner,sun50i-a100-ths
>                - allwinner,sun50i-h6-ths
> +              - allwinner,sun50i-h616-ths
>  
>      then:
>        properties:
> @@ -80,6 +82,18 @@ allOf:
>          clock-names:
>            minItems: 2
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: allwinner,sun50i-h616-ths
> +
> +    then:
> +      properties:
> +        syscon:

Nope, there is no such property. First of all, properties must be
defined in top level. Second of all - this is exactly the example I used
for my talk. Two times.

> +          maxItems: 1
> +          description: phandle to syscon device allowing access to SYS_CFG registers

You must also say what is the purpose. Drop redundant parts like
"phandle to syscon device allowing access" and explain what is the hardware.



Best regards,
Krzysztof

[PATCH v3 3/6] thermal: sun8i: explain unknown H6 register value

[Andre Przywara]

So far we were ORing in some "unknown" value into the THS control
register on the Allwinner H6. This part of the register is not explained
in the H6 manual, but the H616 manual details those bits, and on closer
inspection the THS IP blocks in both SoCs seem very close:
- The BSP code for both SoCs writes the same values into THS_CTRL.
- The reset values of at least the first three registers are the same.

Replace the "unknown" value with its proper meaning: "acquire time",
most probably the sample part of the sample & hold circuit of the ADC,
according to its explanation in the H616 manual.

No functional change, just a macro rename and adjustment.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/thermal/sun8i_thermal.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index f989b55a8aa8e..44554c3efc96c 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -50,7 +50,8 @@
 #define SUN8I_THS_CTRL2_T_ACQ1(x)		((GENMASK(15, 0) & (x)) << 16)
 #define SUN8I_THS_DATA_IRQ_STS(x)		BIT(x + 8)
 
-#define SUN50I_THS_CTRL0_T_ACQ(x)		((GENMASK(15, 0) & (x)) << 16)
+#define SUN50I_THS_CTRL0_T_ACQ(x)		(GENMASK(15, 0) & ((x) - 1))
+#define SUN50I_THS_CTRL0_T_SAMPLE_PER(x)	((GENMASK(15, 0) & ((x) - 1)) << 16)
 #define SUN50I_THS_FILTER_EN			BIT(2)
 #define SUN50I_THS_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
 #define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
@@ -410,25 +411,27 @@ static int sun8i_h3_thermal_init(struct ths_device *tmdev)
 	return 0;
 }
 
-/*
- * Without this undocumented value, the returned temperatures would
- * be higher than real ones by about 20C.
- */
-#define SUN50I_H6_CTRL0_UNK 0x0000002f
-
 static int sun50i_h6_thermal_init(struct ths_device *tmdev)
 {
 	int val;
 
 	/*
-	 * T_acq = 20us
-	 * clkin = 24MHz
-	 *
-	 * x = T_acq * clkin - 1
-	 *   = 479
+	 * The manual recommends an overall sample frequency of 50 KHz (20us,
+	 * 480 cycles at 24 MHz), which provides plenty of time for both the
+	 * acquisition time (>24 cycles) and the actual conversion time
+	 * (>14 cycles).
+	 * The lower half of the CTRL register holds the "acquire time", in
+	 * clock cycles, which the manual recommends to be 2us:
+	 * 24MHz * 2us = 48 cycles.
+	 * The high half of THS_CTRL encodes the sample frequency, in clock
+	 * cycles: 24MHz * 20us = 480 cycles.
+	 * This is explained in the H616 manual, but apparently wrongly
+	 * described in the H6 manual, although the BSP code does the same
+	 * for both SoCs.
 	 */
 	regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
-		     SUN50I_H6_CTRL0_UNK | SUN50I_THS_CTRL0_T_ACQ(479));
+		     SUN50I_THS_CTRL0_T_ACQ(48) |
+		     SUN50I_THS_CTRL0_T_SAMPLE_PER(480));
 	/* average over 4 samples */
 	regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
 		     SUN50I_THS_FILTER_EN |
-- 
2.35.8

[PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Andre Przywara]

The Allwinner H616 SoC needs to clear a bit in one register in the SRAM
controller (exported as a syscon), to report reasonable temperature
values. On reset, bit 16 in register 0x3000000 is set, which leads to the
driver reporting temperatures around 200C. Clearing this bit brings the
values down to the expected range. The BSP code does a one-time write in
U-Boot, with a comment just mentioning the effect on the THS, but offering
no further explanation.

To not rely on firmware to set things up for us, add code that queries
the syscon device via a DT phandle link, then clear just this single
bit.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/thermal/sun8i_thermal.c | 50 +++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index 44554c3efc96c..920e419ce7343 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -66,6 +67,7 @@ struct tsensor {
 struct ths_thermal_chip {
 	bool            has_mod_clk;
 	bool            has_bus_clk_reset;
+	bool		needs_syscon;
 	int		sensor_num;
 	int		offset;
 	int		scale;
@@ -83,6 +85,7 @@ struct ths_device {
 	const struct ths_thermal_chip		*chip;
 	struct device				*dev;
 	struct regmap				*regmap;
+	struct regmap_field			*syscon_regmap_field;
 	struct reset_control			*reset;
 	struct clk				*bus_clk;
 	struct clk                              *mod_clk;
@@ -325,6 +328,34 @@ static void sun8i_ths_reset_control_assert(void *data)
 	reset_control_assert(data);
 }
 
+static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
+{
+	struct device_node *syscon_node;
+	struct platform_device *syscon_pdev;
+	struct regmap *regmap = NULL;
+
+	syscon_node = of_parse_phandle(node, "syscon", 0);
+	if (!syscon_node)
+		return ERR_PTR(-ENODEV);
+
+	syscon_pdev = of_find_device_by_node(syscon_node);
+	if (!syscon_pdev) {
+		/* platform device might not be probed yet */
+		regmap = ERR_PTR(-EPROBE_DEFER);
+		goto out_put_node;
+	}
+
+	/* If no regmap is found then the other device driver is at fault */
+	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
+	if (!regmap)
+		regmap = ERR_PTR(-EINVAL);
+
+	platform_device_put(syscon_pdev);
+out_put_node:
+	of_node_put(syscon_node);
+	return regmap;
+}
+
 static int sun8i_ths_resource_init(struct ths_device *tmdev)
 {
 	struct device *dev = tmdev->dev;
@@ -369,6 +400,21 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
 	if (ret)
 		return ret;
 
+	if (tmdev->chip->needs_syscon) {
+		const struct reg_field sun8i_syscon_reg_field =
+			REG_FIELD(0x0, 16, 16);
+		struct regmap *regmap;
+
+		regmap = sun8i_ths_get_syscon_regmap(dev->of_node);
+		if (IS_ERR(regmap))
+			return PTR_ERR(regmap);
+		tmdev->syscon_regmap_field = devm_regmap_field_alloc(dev,
+						      regmap,
+						      sun8i_syscon_reg_field);
+		if (IS_ERR(tmdev->syscon_regmap_field))
+			return PTR_ERR(tmdev->syscon_regmap_field);
+	}
+
 	ret = sun8i_ths_calibrate(tmdev);
 	if (ret)
 		return ret;
@@ -415,6 +461,10 @@ static int sun50i_h6_thermal_init(struct ths_device *tmdev)
 {
 	int val;
 
+	/* The H616 needs to have a bit in the SRAM control register cleared. */
+	if (tmdev->syscon_regmap_field)
+		regmap_field_write(tmdev->syscon_regmap_field, 0);
+
 	/*
 	 * The manual recommends an overall sample frequency of 50 KHz (20us,
 	 * 480 cycles at 24 MHz), which provides plenty of time for both the
-- 
2.35.8

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Krzysztof Kozlowski]

On 28/11/2023 01:58, Andre Przywara wrote:
>  
> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> +{
> +	struct device_node *syscon_node;
> +	struct platform_device *syscon_pdev;
> +	struct regmap *regmap = NULL;
> +
> +	syscon_node = of_parse_phandle(node, "syscon", 0);

Nope. For the 100th time, this cannot be generic.

> +	if (!syscon_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	syscon_pdev = of_find_device_by_node(syscon_node);
> +	if (!syscon_pdev) {
> +		/* platform device might not be probed yet */
> +		regmap = ERR_PTR(-EPROBE_DEFER);
> +		goto out_put_node;
> +	}
> +
> +	/* If no regmap is found then the other device driver is at fault */
> +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> +	if (!regmap)
> +		regmap = ERR_PTR(-EINVAL);

Aren't you open-coding existing API to get regmap from syscon?


Best regards,
Krzysztof

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/11/2023 01:58, Andre Przywara wrote:
> >
> > +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > +{
> > +     struct device_node *syscon_node;
> > +     struct platform_device *syscon_pdev;
> > +     struct regmap *regmap = NULL;
> > +
> > +     syscon_node = of_parse_phandle(node, "syscon", 0);
>
> Nope. For the 100th time, this cannot be generic.
>
> > +     if (!syscon_node)
> > +             return ERR_PTR(-ENODEV);
> > +
> > +     syscon_pdev = of_find_device_by_node(syscon_node);
> > +     if (!syscon_pdev) {
> > +             /* platform device might not be probed yet */
> > +             regmap = ERR_PTR(-EPROBE_DEFER);
> > +             goto out_put_node;
> > +     }
> > +
> > +     /* If no regmap is found then the other device driver is at fault */
> > +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> > +     if (!regmap)
> > +             regmap = ERR_PTR(-EINVAL);
>
> Aren't you open-coding existing API to get regmap from syscon?

Not really. This is to get a regmap exported by the device. Syscon's regmap
is not tied to the device at all.

With this scheme a device to could export just enough registers for the
consumer to use, instead of the whole address range.

We do this in the R40 clock controller as well, which has some bits that
tweak the ethernet controllers RGMII delay...


ChenYu

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Krzysztof Kozlowski]

On 28/11/2023 08:50, Chen-Yu Tsai wrote:
> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>
>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>> +{
>>> +     struct device_node *syscon_node;
>>> +     struct platform_device *syscon_pdev;
>>> +     struct regmap *regmap = NULL;
>>> +
>>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
>>
>> Nope. For the 100th time, this cannot be generic.
>>
>>> +     if (!syscon_node)
>>> +             return ERR_PTR(-ENODEV);
>>> +
>>> +     syscon_pdev = of_find_device_by_node(syscon_node);
>>> +     if (!syscon_pdev) {
>>> +             /* platform device might not be probed yet */
>>> +             regmap = ERR_PTR(-EPROBE_DEFER);
>>> +             goto out_put_node;
>>> +     }
>>> +
>>> +     /* If no regmap is found then the other device driver is at fault */
>>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
>>> +     if (!regmap)
>>> +             regmap = ERR_PTR(-EINVAL);
>>
>> Aren't you open-coding existing API to get regmap from syscon?
> 
> Not really. This is to get a regmap exported by the device. Syscon's regmap
> is not tied to the device at all.

I am talking about open-coding existing API. Look at syscon.h.

> 
> With this scheme a device to could export just enough registers for the
> consumer to use, instead of the whole address range.
> 
> We do this in the R40 clock controller as well, which has some bits that
> tweak the ethernet controllers RGMII delay...

Not related.

> 
> 
> ChenYu

Best regards,
Krzysztof

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/11/2023 08:50, Chen-Yu Tsai wrote:
> > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 28/11/2023 01:58, Andre Przywara wrote:
> >>>
> >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> >>> +{
> >>> +     struct device_node *syscon_node;
> >>> +     struct platform_device *syscon_pdev;
> >>> +     struct regmap *regmap = NULL;
> >>> +
> >>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
> >>
> >> Nope. For the 100th time, this cannot be generic.
> >>
> >>> +     if (!syscon_node)
> >>> +             return ERR_PTR(-ENODEV);
> >>> +
> >>> +     syscon_pdev = of_find_device_by_node(syscon_node);
> >>> +     if (!syscon_pdev) {
> >>> +             /* platform device might not be probed yet */
> >>> +             regmap = ERR_PTR(-EPROBE_DEFER);
> >>> +             goto out_put_node;
> >>> +     }
> >>> +
> >>> +     /* If no regmap is found then the other device driver is at fault */
> >>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> >>> +     if (!regmap)
> >>> +             regmap = ERR_PTR(-EINVAL);
> >>
> >> Aren't you open-coding existing API to get regmap from syscon?
> >
> > Not really. This is to get a regmap exported by the device. Syscon's regmap
> > is not tied to the device at all.
>
> I am talking about open-coding existing API. Look at syscon.h.

The underlying implementation is different.

syscon maintains its own mapping of device nodes to regmaps, and creates
regmaps when none exist. The regmap is not tied to any struct device.
syscon basically exists outside of the driver model and one has no control
over what is exposed because it is meant for blocks that are a collection
of random stuff.

Here the provider device registers the (limited) regmap it wants to provide,
tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
call. The provider has a main function and isn't exposing that part of its
register map to the outside; only the random bits that were stuffed in are.

> > With this scheme a device to could export just enough registers for the
> > consumer to use, instead of the whole address range.
> >
> > We do this in the R40 clock controller as well, which has some bits that
> > tweak the ethernet controllers RGMII delay...
>
> Not related.

Related as in that is possibly what this code was based on, commit
49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
from external device").


ChenYu

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 28/11/2023 08:50, Chen-Yu Tsai wrote:
> > > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 28/11/2023 01:58, Andre Przywara wrote:
> > >>>
> > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > >>> +{
> > >>> +     struct device_node *syscon_node;
> > >>> +     struct platform_device *syscon_pdev;
> > >>> +     struct regmap *regmap = NULL;
> > >>> +
> > >>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
> > >>
> > >> Nope. For the 100th time, this cannot be generic.
> > >>
> > >>> +     if (!syscon_node)
> > >>> +             return ERR_PTR(-ENODEV);
> > >>> +
> > >>> +     syscon_pdev = of_find_device_by_node(syscon_node);
> > >>> +     if (!syscon_pdev) {
> > >>> +             /* platform device might not be probed yet */
> > >>> +             regmap = ERR_PTR(-EPROBE_DEFER);
> > >>> +             goto out_put_node;
> > >>> +     }
> > >>> +
> > >>> +     /* If no regmap is found then the other device driver is at fault */
> > >>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> > >>> +     if (!regmap)
> > >>> +             regmap = ERR_PTR(-EINVAL);
> > >>
> > >> Aren't you open-coding existing API to get regmap from syscon?
> > >
> > > Not really. This is to get a regmap exported by the device. Syscon's regmap
> > > is not tied to the device at all.
> >
> > I am talking about open-coding existing API. Look at syscon.h.
>
> The underlying implementation is different.
>
> syscon maintains its own mapping of device nodes to regmaps, and creates
> regmaps when none exist. The regmap is not tied to any struct device.
> syscon basically exists outside of the driver model and one has no control
> over what is exposed because it is meant for blocks that are a collection
> of random stuff.

My bad. I failed to realize there is a platform device driver for syscon,
in addition to the existing "no struct device" implementation.


ChenYu

> Here the provider device registers the (limited) regmap it wants to provide,
> tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
> call. The provider has a main function and isn't exposing that part of its
> register map to the outside; only the random bits that were stuffed in are.
>
> > > With this scheme a device to could export just enough registers for the
> > > consumer to use, instead of the whole address range.
> > >
> > > We do this in the R40 clock controller as well, which has some bits that
> > > tweak the ethernet controllers RGMII delay...
> >
> > Not related.
>
> Related as in that is possibly what this code was based on, commit
> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
> from external device").
>
>
> ChenYu

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote:
>
> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote:
> >
> > On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > On 28/11/2023 08:50, Chen-Yu Tsai wrote:
> > > > On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >>
> > > >> On 28/11/2023 01:58, Andre Przywara wrote:
> > > >>>
> > > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > > >>> +{
> > > >>> +     struct device_node *syscon_node;
> > > >>> +     struct platform_device *syscon_pdev;
> > > >>> +     struct regmap *regmap = NULL;
> > > >>> +
> > > >>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
> > > >>
> > > >> Nope. For the 100th time, this cannot be generic.
> > > >>
> > > >>> +     if (!syscon_node)
> > > >>> +             return ERR_PTR(-ENODEV);
> > > >>> +
> > > >>> +     syscon_pdev = of_find_device_by_node(syscon_node);
> > > >>> +     if (!syscon_pdev) {
> > > >>> +             /* platform device might not be probed yet */
> > > >>> +             regmap = ERR_PTR(-EPROBE_DEFER);
> > > >>> +             goto out_put_node;
> > > >>> +     }
> > > >>> +
> > > >>> +     /* If no regmap is found then the other device driver is at fault */
> > > >>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> > > >>> +     if (!regmap)
> > > >>> +             regmap = ERR_PTR(-EINVAL);
> > > >>
> > > >> Aren't you open-coding existing API to get regmap from syscon?
> > > >
> > > > Not really. This is to get a regmap exported by the device. Syscon's regmap
> > > > is not tied to the device at all.
> > >
> > > I am talking about open-coding existing API. Look at syscon.h.
> >
> > The underlying implementation is different.
> >
> > syscon maintains its own mapping of device nodes to regmaps, and creates
> > regmaps when none exist. The regmap is not tied to any struct device.
> > syscon basically exists outside of the driver model and one has no control
> > over what is exposed because it is meant for blocks that are a collection
> > of random stuff.
>
> My bad. I failed to realize there is a platform device driver for syscon,
> in addition to the existing "no struct device" implementation.

Actually that doesn't do anything on DT platforms as of commit bdb0066df96e
("mfd: syscon: Decouple syscon interface from platform devices"). All the
regmaps are, as I previously stated, not tied to any struct device.

> > Here the provider device registers the (limited) regmap it wants to provide,
> > tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
> > call. The provider has a main function and isn't exposing that part of its
> > register map to the outside; only the random bits that were stuffed in are.
> >
> > > > With this scheme a device to could export just enough registers for the
> > > > consumer to use, instead of the whole address range.
> > > >
> > > > We do this in the R40 clock controller as well, which has some bits that
> > > > tweak the ethernet controllers RGMII delay...
> > >
> > > Not related.
> >
> > Related as in that is possibly what this code was based on, commit
> > 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
> > from external device").
> >
> >
> > ChenYu

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Krzysztof Kozlowski]

On 28/11/2023 10:09, Chen-Yu Tsai wrote:
> On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>
>> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>>
>>> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 28/11/2023 08:50, Chen-Yu Tsai wrote:
>>>>> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>
>>>>>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>>>>>
>>>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>>>>>> +{
>>>>>>> +     struct device_node *syscon_node;
>>>>>>> +     struct platform_device *syscon_pdev;
>>>>>>> +     struct regmap *regmap = NULL;
>>>>>>> +
>>>>>>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
>>>>>>
>>>>>> Nope. For the 100th time, this cannot be generic.
>>>>>>
>>>>>>> +     if (!syscon_node)
>>>>>>> +             return ERR_PTR(-ENODEV);
>>>>>>> +
>>>>>>> +     syscon_pdev = of_find_device_by_node(syscon_node);
>>>>>>> +     if (!syscon_pdev) {
>>>>>>> +             /* platform device might not be probed yet */
>>>>>>> +             regmap = ERR_PTR(-EPROBE_DEFER);
>>>>>>> +             goto out_put_node;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     /* If no regmap is found then the other device driver is at fault */
>>>>>>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
>>>>>>> +     if (!regmap)
>>>>>>> +             regmap = ERR_PTR(-EINVAL);
>>>>>>
>>>>>> Aren't you open-coding existing API to get regmap from syscon?
>>>>>
>>>>> Not really. This is to get a regmap exported by the device. Syscon's regmap
>>>>> is not tied to the device at all.
>>>>
>>>> I am talking about open-coding existing API. Look at syscon.h.
>>>
>>> The underlying implementation is different.
>>>
>>> syscon maintains its own mapping of device nodes to regmaps, and creates
>>> regmaps when none exist. The regmap is not tied to any struct device.
>>> syscon basically exists outside of the driver model and one has no control
>>> over what is exposed because it is meant for blocks that are a collection
>>> of random stuff.
>>
>> My bad. I failed to realize there is a platform device driver for syscon,
>> in addition to the existing "no struct device" implementation.
> 
> Actually that doesn't do anything on DT platforms as of commit bdb0066df96e
> ("mfd: syscon: Decouple syscon interface from platform devices"). All the
> regmaps are, as I previously stated, not tied to any struct device.


Sorry, it's your third reply, so I don't know what exactly you want to
discuss.

This code open-codes existing API. Fix it.

> 
>>> Here the provider device registers the (limited) regmap it wants to provide,
>>> tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
>>> call. The provider has a main function and isn't exposing that part of its
>>> register map to the outside; only the random bits that were stuffed in are.
>>>
>>>>> With this scheme a device to could export just enough registers for the
>>>>> consumer to use, instead of the whole address range.
>>>>>
>>>>> We do this in the R40 clock controller as well, which has some bits that
>>>>> tweak the ethernet controllers RGMII delay...
>>>>
>>>> Not related.
>>>
>>> Related as in that is possibly what this code was based on, commit
>>> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
>>> from external device").


How duplicating a code is related to R40 controller? Duplicating code is
generic problem, not specific and not related to your hardware.

Best regards,
Krzysztof

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Krzysztof Kozlowski]

On 28/11/2023 10:13, Krzysztof Kozlowski wrote:
> On 28/11/2023 10:09, Chen-Yu Tsai wrote:
>> On Tue, Nov 28, 2023 at 5:02 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>>
>>> On Tue, Nov 28, 2023 at 4:59 PM Chen-Yu Tsai <wens@csie.org> wrote:
>>>>
>>>> On Tue, Nov 28, 2023 at 4:30 PM Krzysztof Kozlowski
>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>> On 28/11/2023 08:50, Chen-Yu Tsai wrote:
>>>>>> On Tue, Nov 28, 2023 at 3:43 PM Krzysztof Kozlowski
>>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>>
>>>>>>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>>>>>>
>>>>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>>>>>>> +{
>>>>>>>> +     struct device_node *syscon_node;
>>>>>>>> +     struct platform_device *syscon_pdev;
>>>>>>>> +     struct regmap *regmap = NULL;
>>>>>>>> +
>>>>>>>> +     syscon_node = of_parse_phandle(node, "syscon", 0);
>>>>>>>
>>>>>>> Nope. For the 100th time, this cannot be generic.
>>>>>>>
>>>>>>>> +     if (!syscon_node)
>>>>>>>> +             return ERR_PTR(-ENODEV);
>>>>>>>> +
>>>>>>>> +     syscon_pdev = of_find_device_by_node(syscon_node);
>>>>>>>> +     if (!syscon_pdev) {
>>>>>>>> +             /* platform device might not be probed yet */
>>>>>>>> +             regmap = ERR_PTR(-EPROBE_DEFER);
>>>>>>>> +             goto out_put_node;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     /* If no regmap is found then the other device driver is at fault */
>>>>>>>> +     regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
>>>>>>>> +     if (!regmap)
>>>>>>>> +             regmap = ERR_PTR(-EINVAL);
>>>>>>>
>>>>>>> Aren't you open-coding existing API to get regmap from syscon?
>>>>>>
>>>>>> Not really. This is to get a regmap exported by the device. Syscon's regmap
>>>>>> is not tied to the device at all.
>>>>>
>>>>> I am talking about open-coding existing API. Look at syscon.h.
>>>>
>>>> The underlying implementation is different.
>>>>
>>>> syscon maintains its own mapping of device nodes to regmaps, and creates
>>>> regmaps when none exist. The regmap is not tied to any struct device.
>>>> syscon basically exists outside of the driver model and one has no control
>>>> over what is exposed because it is meant for blocks that are a collection
>>>> of random stuff.
>>>
>>> My bad. I failed to realize there is a platform device driver for syscon,
>>> in addition to the existing "no struct device" implementation.
>>
>> Actually that doesn't do anything on DT platforms as of commit bdb0066df96e
>> ("mfd: syscon: Decouple syscon interface from platform devices"). All the
>> regmaps are, as I previously stated, not tied to any struct device.
> 
> 
> Sorry, it's your third reply, so I don't know what exactly you want to
> discuss.
> 
> This code open-codes existing API. Fix it.
> 
>>
>>>> Here the provider device registers the (limited) regmap it wants to provide,
>>>> tying the new regmap to itself. The consumer gets it via the dev_get_regmap()
>>>> call. The provider has a main function and isn't exposing that part of its
>>>> register map to the outside; only the random bits that were stuffed in are.
>>>>
>>>>>> With this scheme a device to could export just enough registers for the
>>>>>> consumer to use, instead of the whole address range.
>>>>>>
>>>>>> We do this in the R40 clock controller as well, which has some bits that
>>>>>> tweak the ethernet controllers RGMII delay...
>>>>>
>>>>> Not related.
>>>>
>>>> Related as in that is possibly what this code was based on, commit
>>>> 49a06cae6e7c ("net: stmmac: dwmac-sun8i: Allow getting syscon regmap
>>>> from external device").
> 
> 
> How duplicating a code is related to R40 controller? Duplicating code is
> generic problem, not specific and not related to your hardware.

I think I understand now what you wanted to say - the "syscon" property
is pointing not to a syscon.

That's the mistake in the bindings - you claim it is a syscon, but it is
not and has nothing to do with syscon. Neither in the bindings nor in
the Linux drivers. This should be fixed.

Best regards,
Krzysztof

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Andre Przywara]

On Tue, 28 Nov 2023 08:43:32 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi,

> On 28/11/2023 01:58, Andre Przywara wrote:
> >  
> > +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > +{
> > +	struct device_node *syscon_node;
> > +	struct platform_device *syscon_pdev;
> > +	struct regmap *regmap = NULL;
> > +
> > +	syscon_node = of_parse_phandle(node, "syscon", 0);  
> 
> Nope. For the 100th time, this cannot be generic.

OK. Shall this name refer to the required functionality (temperature
offset fix) or to the target syscon node (like allwinner,misc-syscon).
The problem is that this is really a syscon, as in: "random collection of
bits that we didn't know where else to put in", so "syscon" alone actually
says it all.


And btw: it would have been about the same effort (and more helpful!) to
type:

"This cannot be generic, please check writing-bindings.rst."    ;-)

> 
> > +	if (!syscon_node)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	syscon_pdev = of_find_device_by_node(syscon_node);
> > +	if (!syscon_pdev) {
> > +		/* platform device might not be probed yet */
> > +		regmap = ERR_PTR(-EPROBE_DEFER);
> > +		goto out_put_node;
> > +	}
> > +
> > +	/* If no regmap is found then the other device driver is at fault */
> > +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> > +	if (!regmap)
> > +		regmap = ERR_PTR(-EINVAL);  
> 
> Aren't you open-coding existing API to get regmap from syscon?

That's a good point, I lifted that code from sun8i-emac.c, where we have
the exact same problem. 
Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
node to have "syscon" in its compatible string list, which we
don't have. We actually explicitly dropped this for the A64 (with
1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
I guess we could add it back, and it would work for this case here (tested
that), but then cannot replace the sun8i-emac.c code, because that would
break older DTs.
So is there any chance we can drop the requirement for "syscon" in the
compatible string list, in the implementation of
syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
prototype? Or is there another existing API that does this already?

Cheers,
Andre

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Krzysztof Kozlowski]

On 28/11/2023 15:33, Andre Przywara wrote:
> On Tue, 28 Nov 2023 08:43:32 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> Hi,
> 
>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>  
>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>> +{
>>> +	struct device_node *syscon_node;
>>> +	struct platform_device *syscon_pdev;
>>> +	struct regmap *regmap = NULL;
>>> +
>>> +	syscon_node = of_parse_phandle(node, "syscon", 0);  
>>
>> Nope. For the 100th time, this cannot be generic.
> 
> OK. Shall this name refer to the required functionality (temperature
> offset fix) or to the target syscon node (like allwinner,misc-syscon).
> The problem is that this is really a syscon, as in: "random collection of
> bits that we didn't know where else to put in", so "syscon" alone actually
> says it all.

Every syscon is a "random collection of bits...", but not every "random
collection of bits..." is a syscon.

Your target device does not implement syscon nodes. Your Linux
implementation does not use it as syscon. Therefore if something does
not look like syscon and does not behave like syscon, it is not a syscon.

I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
something entirely different and we have a binding for it: "sram", I think.

> 
> 
> And btw: it would have been about the same effort (and more helpful!) to
> type:
> 
> "This cannot be generic, please check writing-bindings.rst."    ;-)
> 
>>
>>> +	if (!syscon_node)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	syscon_pdev = of_find_device_by_node(syscon_node);
>>> +	if (!syscon_pdev) {
>>> +		/* platform device might not be probed yet */
>>> +		regmap = ERR_PTR(-EPROBE_DEFER);
>>> +		goto out_put_node;
>>> +	}
>>> +
>>> +	/* If no regmap is found then the other device driver is at fault */
>>> +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
>>> +	if (!regmap)
>>> +		regmap = ERR_PTR(-EINVAL);  
>>
>> Aren't you open-coding existing API to get regmap from syscon?
> 
> That's a good point, I lifted that code from sun8i-emac.c, where we have
> the exact same problem. 
> Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
> node to have "syscon" in its compatible string list, which we
> don't have. We actually explicitly dropped this for the A64 (with
> 1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
> I guess we could add it back, and it would work for this case here (tested
> that), but then cannot replace the sun8i-emac.c code, because that would
> break older DTs.
> So is there any chance we can drop the requirement for "syscon" in the
> compatible string list, in the implementation of
> syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
> prototype? Or is there another existing API that does this already?

I must correct myself: I was wrong. You are not open-coding, because as
pointed out, this is not a phandle to syscon (even if you call it like
"syscon").

The code is fine, maybe except missing links (needs double checking,
because maybe regmap creates links?). The DT binding and DTS needs
fixing, because it is not a syscon.

Best regards,
Krzysztof

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Andre Przywara]

On Tue, 28 Nov 2023 15:48:18 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi,

(adding Maxime for the syscon question below)

> On 28/11/2023 15:33, Andre Przywara wrote:
> > On Tue, 28 Nov 2023 08:43:32 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> > Hi,
> >   
> >> On 28/11/2023 01:58, Andre Przywara wrote:  
> >>>  
> >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> >>> +{
> >>> +	struct device_node *syscon_node;
> >>> +	struct platform_device *syscon_pdev;
> >>> +	struct regmap *regmap = NULL;
> >>> +
> >>> +	syscon_node = of_parse_phandle(node, "syscon", 0);    
> >>
> >> Nope. For the 100th time, this cannot be generic.  
> > 
> > OK. Shall this name refer to the required functionality (temperature
> > offset fix) or to the target syscon node (like allwinner,misc-syscon).
> > The problem is that this is really a syscon, as in: "random collection of
> > bits that we didn't know where else to put in", so "syscon" alone actually
> > says it all.  
> 
> Every syscon is a "random collection of bits...", but not every "random
> collection of bits..." is a syscon.
> 
> Your target device does not implement syscon nodes. Your Linux
> implementation does not use it as syscon. Therefore if something does
> not look like syscon and does not behave like syscon, it is not a syscon.
> 
> I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
> something entirely different and we have a binding for it: "sram", I think.

Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
it can switch the control of certain SRAM regions between CPU access and
peripheral access (for the video and the display engine). But then it's
also a syscon, because on top of that, it also controls those random bits,
for instance the EMAC clock register, and this ominous THS bit.
I guess in hindsight we should have never dropped that "syscon" string
then, but I am not sure if adding it back has side effects?

And as I mentioned in the cover letter: modelling this as some SRAM
region, as you suggest, might be an alternative, but it doesn't sound right
either, as I don't think it really is one: I just tried in U-Boot, and I
can write and read the whole SRAM C region just fine, with and without the
bit set. And SRAM content is preserved, even with the thermal sensor
running and the bit cleared (or set).

So adding the "syscon" to the compatible would fix most things, but then
we need to keep the open coded lookup code in dwmac-sun8i.c (because older
DTs would break otherwise).

What do people think about this?
Samuel, does this affect the D1 LDO driver as well?

Cheers,
Andre

> 
> > 
> > 
> > And btw: it would have been about the same effort (and more helpful!) to
> > type:
> > 
> > "This cannot be generic, please check writing-bindings.rst."    ;-)
> >   
> >>  
> >>> +	if (!syscon_node)
> >>> +		return ERR_PTR(-ENODEV);
> >>> +
> >>> +	syscon_pdev = of_find_device_by_node(syscon_node);
> >>> +	if (!syscon_pdev) {
> >>> +		/* platform device might not be probed yet */
> >>> +		regmap = ERR_PTR(-EPROBE_DEFER);
> >>> +		goto out_put_node;
> >>> +	}
> >>> +
> >>> +	/* If no regmap is found then the other device driver is at fault */
> >>> +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> >>> +	if (!regmap)
> >>> +		regmap = ERR_PTR(-EINVAL);    
> >>
> >> Aren't you open-coding existing API to get regmap from syscon?  
> > 
> > That's a good point, I lifted that code from sun8i-emac.c, where we have
> > the exact same problem. 
> > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
> > node to have "syscon" in its compatible string list, which we
> > don't have. We actually explicitly dropped this for the A64 (with
> > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
> > I guess we could add it back, and it would work for this case here (tested
> > that), but then cannot replace the sun8i-emac.c code, because that would
> > break older DTs.
> > So is there any chance we can drop the requirement for "syscon" in the
> > compatible string list, in the implementation of
> > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
> > prototype? Or is there another existing API that does this already?  
> 
> I must correct myself: I was wrong. You are not open-coding, because as
> pointed out, this is not a phandle to syscon (even if you call it like
> "syscon").
> 
> The code is fine, maybe except missing links (needs double checking,
> because maybe regmap creates links?). The DT binding and DTS needs
> fixing, because it is not a syscon.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Wed, Nov 29, 2023 at 12:10 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 28 Nov 2023 15:48:18 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
> Hi,
>
> (adding Maxime for the syscon question below)
>
> > On 28/11/2023 15:33, Andre Przywara wrote:
> > > On Tue, 28 Nov 2023 08:43:32 +0100
> > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > >> On 28/11/2023 01:58, Andre Przywara wrote:
> > >>>
> > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > >>> +{
> > >>> + struct device_node *syscon_node;
> > >>> + struct platform_device *syscon_pdev;
> > >>> + struct regmap *regmap = NULL;
> > >>> +
> > >>> + syscon_node = of_parse_phandle(node, "syscon", 0);
> > >>
> > >> Nope. For the 100th time, this cannot be generic.
> > >
> > > OK. Shall this name refer to the required functionality (temperature
> > > offset fix) or to the target syscon node (like allwinner,misc-syscon).
> > > The problem is that this is really a syscon, as in: "random collection of
> > > bits that we didn't know where else to put in", so "syscon" alone actually
> > > says it all.
> >
> > Every syscon is a "random collection of bits...", but not every "random
> > collection of bits..." is a syscon.
> >
> > Your target device does not implement syscon nodes. Your Linux
> > implementation does not use it as syscon. Therefore if something does
> > not look like syscon and does not behave like syscon, it is not a syscon.
> >
> > I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
> > something entirely different and we have a binding for it: "sram", I think.
>
> Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
> it can switch the control of certain SRAM regions between CPU access and
> peripheral access (for the video and the display engine). But then it's
> also a syscon, because on top of that, it also controls those random bits,
> for instance the EMAC clock register, and this ominous THS bit.
> I guess in hindsight we should have never dropped that "syscon" string
> then, but I am not sure if adding it back has side effects?

Either way you would need to add locking around the register accesses,
or you could, however unlikely, end up with two simultaneous read-update-write
accesses by both consumers (THS and claiming C1).

If you add the syscon string back, then you'd have to convert the SRAM
controller driver to use syscon as well, as there is no way to provide
a custom spinlock for the syscon regmap. Another reason why a driver
would want to create its own regmap.

> And as I mentioned in the cover letter: modelling this as some SRAM
> region, as you suggest, might be an alternative, but it doesn't sound right
> either, as I don't think it really is one: I just tried in U-Boot, and I
> can write and read the whole SRAM C region just fine, with and without the
> bit set. And SRAM content is preserved, even with the thermal sensor
> running and the bit cleared (or set).
>
> So adding the "syscon" to the compatible would fix most things, but then
> we need to keep the open coded lookup code in dwmac-sun8i.c (because older
> DTs would break otherwise).

dwmac-sun8i already falls back to syscon_regmap_lookup_by_phandle() because
of even older DTs. I'm the one that added the open coded stuff, mostly
because the R40 had the bits embedded in the clock controller, not the
system control, and it seemed error prone and hard to debug for some
other device to have full access.

So you'd just be reverting the driver to the old ways.

ChenYu


> What do people think about this?
> Samuel, does this affect the D1 LDO driver as well?
>
> Cheers,
> Andre
>
> >
> > >
> > >
> > > And btw: it would have been about the same effort (and more helpful!) to
> > > type:
> > >
> > > "This cannot be generic, please check writing-bindings.rst."    ;-)
> > >
> > >>
> > >>> + if (!syscon_node)
> > >>> +         return ERR_PTR(-ENODEV);
> > >>> +
> > >>> + syscon_pdev = of_find_device_by_node(syscon_node);
> > >>> + if (!syscon_pdev) {
> > >>> +         /* platform device might not be probed yet */
> > >>> +         regmap = ERR_PTR(-EPROBE_DEFER);
> > >>> +         goto out_put_node;
> > >>> + }
> > >>> +
> > >>> + /* If no regmap is found then the other device driver is at fault */
> > >>> + regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> > >>> + if (!regmap)
> > >>> +         regmap = ERR_PTR(-EINVAL);
> > >>
> > >> Aren't you open-coding existing API to get regmap from syscon?
> > >
> > > That's a good point, I lifted that code from sun8i-emac.c, where we have
> > > the exact same problem.
> > > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
> > > node to have "syscon" in its compatible string list, which we
> > > don't have. We actually explicitly dropped this for the A64 (with
> > > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
> > > I guess we could add it back, and it would work for this case here (tested
> > > that), but then cannot replace the sun8i-emac.c code, because that would
> > > break older DTs.
> > > So is there any chance we can drop the requirement for "syscon" in the
> > > compatible string list, in the implementation of
> > > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
> > > prototype? Or is there another existing API that does this already?
> >
> > I must correct myself: I was wrong. You are not open-coding, because as
> > pointed out, this is not a phandle to syscon (even if you call it like
> > "syscon").
> >
> > The code is fine, maybe except missing links (needs double checking,
> > because maybe regmap creates links?). The DT binding and DTS needs
> > fixing, because it is not a syscon.
> >
> > Best regards,
> > Krzysztof
> >
>

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Rob Herring]

On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 28 Nov 2023 15:48:18 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
> Hi,
>
> (adding Maxime for the syscon question below)
>
> > On 28/11/2023 15:33, Andre Przywara wrote:
> > > On Tue, 28 Nov 2023 08:43:32 +0100
> > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > >> On 28/11/2023 01:58, Andre Przywara wrote:
> > >>>
> > >>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> > >>> +{
> > >>> + struct device_node *syscon_node;
> > >>> + struct platform_device *syscon_pdev;
> > >>> + struct regmap *regmap = NULL;
> > >>> +
> > >>> + syscon_node = of_parse_phandle(node, "syscon", 0);
> > >>
> > >> Nope. For the 100th time, this cannot be generic.

Unless it is the 100th time for the submitter, please just point to
the documentation.

Can we simply ban "syscon" as a property name? It looks like we have
65 cases in upstream dts files. Maybe that's doable. This is where we
need levels of warnings with okay for existing vs. don't use in new
designs.

> > > OK. Shall this name refer to the required functionality (temperature
> > > offset fix) or to the target syscon node (like allwinner,misc-syscon).
> > > The problem is that this is really a syscon, as in: "random collection of
> > > bits that we didn't know where else to put in", so "syscon" alone actually
> > > says it all.
> >
> > Every syscon is a "random collection of bits...", but not every "random
> > collection of bits..." is a syscon.
> >
> > Your target device does not implement syscon nodes. Your Linux
> > implementation does not use it as syscon. Therefore if something does
> > not look like syscon and does not behave like syscon, it is not a syscon.
> >
> > I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
> > something entirely different and we have a binding for it: "sram", I think.
>
> Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
> it can switch the control of certain SRAM regions between CPU access and
> peripheral access (for the video and the display engine). But then it's
> also a syscon, because on top of that, it also controls those random bits,
> for instance the EMAC clock register, and this ominous THS bit.
> I guess in hindsight we should have never dropped that "syscon" string
> then, but I am not sure if adding it back has side effects?
>
> And as I mentioned in the cover letter: modelling this as some SRAM
> region, as you suggest, might be an alternative, but it doesn't sound right
> either, as I don't think it really is one: I just tried in U-Boot, and I
> can write and read the whole SRAM C region just fine, with and without the
> bit set. And SRAM content is preserved, even with the thermal sensor
> running and the bit cleared (or set).
>
> So adding the "syscon" to the compatible would fix most things, but then
> we need to keep the open coded lookup code in dwmac-sun8i.c (because older
> DTs would break otherwise).

Really, I'd like to get rid of the "syscon" compatible. It is nothing
more than a flag for Linux to create a regmap.

Not a fully baked idea, but perhaps what is needed is drivers that
request a regmap for a node simply get one regardless. That kind of
throws out the Linux driver model though. Alternatively with no
"syscon" compatible, we'd have to have table(s) of 100s of compatibles
in the kernel.

Rob

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Andre Przywara]

Hi,

On 28/11/2023 16:50, Rob Herring wrote:
> On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> On Tue, 28 Nov 2023 15:48:18 +0100
>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>> Hi,
>>
>> (adding Maxime for the syscon question below)
>>
>>> On 28/11/2023 15:33, Andre Przywara wrote:
>>>> On Tue, 28 Nov 2023 08:43:32 +0100
>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>>> On 28/11/2023 01:58, Andre Przywara wrote:
>>>>>>
>>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
>>>>>> +{
>>>>>> + struct device_node *syscon_node;
>>>>>> + struct platform_device *syscon_pdev;
>>>>>> + struct regmap *regmap = NULL;
>>>>>> +
>>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0);
>>>>>
>>>>> Nope. For the 100th time, this cannot be generic.
> 
> Unless it is the 100th time for the submitter, please just point to
> the documentation.
> 
> Can we simply ban "syscon" as a property name? It looks like we have
> 65 cases in upstream dts files. Maybe that's doable. This is where we
> need levels of warnings with okay for existing vs. don't use in new
> designs.
> 
>>>> OK. Shall this name refer to the required functionality (temperature
>>>> offset fix) or to the target syscon node (like allwinner,misc-syscon).
>>>> The problem is that this is really a syscon, as in: "random collection of
>>>> bits that we didn't know where else to put in", so "syscon" alone actually
>>>> says it all.
>>>
>>> Every syscon is a "random collection of bits...", but not every "random
>>> collection of bits..." is a syscon.
>>>
>>> Your target device does not implement syscon nodes. Your Linux
>>> implementation does not use it as syscon. Therefore if something does
>>> not look like syscon and does not behave like syscon, it is not a syscon.
>>>
>>> I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
>>> something entirely different and we have a binding for it: "sram", I think.
>>
>> Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
>> it can switch the control of certain SRAM regions between CPU access and
>> peripheral access (for the video and the display engine). But then it's
>> also a syscon, because on top of that, it also controls those random bits,
>> for instance the EMAC clock register, and this ominous THS bit.
>> I guess in hindsight we should have never dropped that "syscon" string
>> then, but I am not sure if adding it back has side effects?
>>
>> And as I mentioned in the cover letter: modelling this as some SRAM
>> region, as you suggest, might be an alternative, but it doesn't sound right
>> either, as I don't think it really is one: I just tried in U-Boot, and I
>> can write and read the whole SRAM C region just fine, with and without the
>> bit set. And SRAM content is preserved, even with the thermal sensor
>> running and the bit cleared (or set).
>>
>> So adding the "syscon" to the compatible would fix most things, but then
>> we need to keep the open coded lookup code in dwmac-sun8i.c (because older
>> DTs would break otherwise).
> 
> Really, I'd like to get rid of the "syscon" compatible. It is nothing
> more than a flag for Linux to create a regmap.

Yeah, so thinking about it indeed feels a bit like we are changing the 
DT here to cater for some Linux implementation detail. After all we 
already access the regmap successfully in dwmac-sun8i.c, is that 
approach frowned upon (because: driver model) and just tolerated because 
it's already in the code base?

> Not a fully baked idea, but perhaps what is needed is drivers that
> request a regmap for a node simply get one regardless. That kind of > throws out the Linux driver model though. Alternatively with no
> "syscon" compatible, we'd have to have table(s) of 100s of compatibles
> in the kernel.

So do you mean to either just remove the explicit syscon compatible 
check in syscon_node_to_regmap(), or replace it with a check against a 
list of allowed devices?
Wouldn't it be sufficient to leave that check to the (syscon-like) 
devices, by them exporting a regmap in the first place or not? And we 
can do filtering of accesses there, like we do in sunxi_sram.c?

Cheers,
Andre


> 
> Rob

Re: [PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Chen-Yu Tsai]

On Thu, Nov 30, 2023 at 1:03 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi,
>
> On 28/11/2023 16:50, Rob Herring wrote:
> > On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >>
> >> On Tue, 28 Nov 2023 15:48:18 +0100
> >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> Hi,
> >>
> >> (adding Maxime for the syscon question below)
> >>
> >>> On 28/11/2023 15:33, Andre Przywara wrote:
> >>>> On Tue, 28 Nov 2023 08:43:32 +0100
> >>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On 28/11/2023 01:58, Andre Przywara wrote:
> >>>>>>
> >>>>>> +static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
> >>>>>> +{
> >>>>>> + struct device_node *syscon_node;
> >>>>>> + struct platform_device *syscon_pdev;
> >>>>>> + struct regmap *regmap = NULL;
> >>>>>> +
> >>>>>> + syscon_node = of_parse_phandle(node, "syscon", 0);
> >>>>>
> >>>>> Nope. For the 100th time, this cannot be generic.
> >
> > Unless it is the 100th time for the submitter, please just point to
> > the documentation.
> >
> > Can we simply ban "syscon" as a property name? It looks like we have
> > 65 cases in upstream dts files. Maybe that's doable. This is where we
> > need levels of warnings with okay for existing vs. don't use in new
> > designs.
> >
> >>>> OK. Shall this name refer to the required functionality (temperature
> >>>> offset fix) or to the target syscon node (like allwinner,misc-syscon).
> >>>> The problem is that this is really a syscon, as in: "random collection of
> >>>> bits that we didn't know where else to put in", so "syscon" alone actually
> >>>> says it all.
> >>>
> >>> Every syscon is a "random collection of bits...", but not every "random
> >>> collection of bits..." is a syscon.
> >>>
> >>> Your target device does not implement syscon nodes. Your Linux
> >>> implementation does not use it as syscon. Therefore if something does
> >>> not look like syscon and does not behave like syscon, it is not a syscon.
> >>>
> >>> I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
> >>> something entirely different and we have a binding for it: "sram", I think.
> >>
> >> Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
> >> it can switch the control of certain SRAM regions between CPU access and
> >> peripheral access (for the video and the display engine). But then it's
> >> also a syscon, because on top of that, it also controls those random bits,
> >> for instance the EMAC clock register, and this ominous THS bit.
> >> I guess in hindsight we should have never dropped that "syscon" string
> >> then, but I am not sure if adding it back has side effects?
> >>
> >> And as I mentioned in the cover letter: modelling this as some SRAM
> >> region, as you suggest, might be an alternative, but it doesn't sound right
> >> either, as I don't think it really is one: I just tried in U-Boot, and I
> >> can write and read the whole SRAM C region just fine, with and without the
> >> bit set. And SRAM content is preserved, even with the thermal sensor
> >> running and the bit cleared (or set).
> >>
> >> So adding the "syscon" to the compatible would fix most things, but then
> >> we need to keep the open coded lookup code in dwmac-sun8i.c (because older
> >> DTs would break otherwise).
> >
> > Really, I'd like to get rid of the "syscon" compatible. It is nothing
> > more than a flag for Linux to create a regmap.
>
> Yeah, so thinking about it indeed feels a bit like we are changing the
> DT here to cater for some Linux implementation detail. After all we
> already access the regmap successfully in dwmac-sun8i.c, is that
> approach frowned upon (because: driver model) and just tolerated because
> it's already in the code base?
>
> > Not a fully baked idea, but perhaps what is needed is drivers that
> > request a regmap for a node simply get one regardless. That kind of > throws out the Linux driver model though. Alternatively with no
> > "syscon" compatible, we'd have to have table(s) of 100s of compatibles
> > in the kernel.
>
> So do you mean to either just remove the explicit syscon compatible
> check in syscon_node_to_regmap(), or replace it with a check against a
> list of allowed devices?

There is already device_node_to_regmap() which skips the check. It still
bypasses the driver model though.

> Wouldn't it be sufficient to leave that check to the (syscon-like)
> devices, by them exporting a regmap in the first place or not? And we
> can do filtering of accesses there, like we do in sunxi_sram.c?
>
> Cheers,
> Andre
>
>
> >
> > Rob

[PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

From: Martin Botka <martin.botka@somainline.org>

Add support for the thermal sensor found in H616 SoCs, which slightly
resembles the H6 thermal sensor controller, with a few changes like
four sensors.
Also the registers readings are wrong, unless a bit in the first SYS_CFG
register cleared, so set needs_syscon to trigger that code

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/thermal/sun8i_thermal.c | 73 +++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index 920e419ce7343..9a404fa9d76a9 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -280,6 +280,64 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
 	return 0;
 }
 
+static int sun50i_h616_ths_calibrate(struct ths_device *tmdev,
+				     u16 *caldata, int callen)
+{
+	struct device *dev = tmdev->dev;
+	int i, ft_temp;
+
+	if (!caldata[0])
+		return -EINVAL;
+
+	/*
+	 * h616 efuse THS calibration data layout:
+	 *
+	 * 0      11  16     27   32     43   48    57
+	 * +----------+-----------+-----------+-----------+
+	 * |  temp |  |sensor0|   |sensor1|   |sensor2|   |
+	 * +----------+-----------+-----------+-----------+
+	 *                      ^           ^           ^
+	 *                      |           |           |
+	 *                      |           |           sensor3[11:8]
+	 *                      |           sensor3[7:4]
+	 *                      sensor3[3:0]
+	 *
+	 * The calibration data on the H616 is the ambient temperature and
+	 * sensor values that are filled during the factory test stage.
+	 *
+	 * The unit of stored FT temperature is 0.1 degree celsius.
+	 */
+	ft_temp = caldata[0] & FT_TEMP_MASK;
+
+	for (i = 0; i < tmdev->chip->sensor_num; i++) {
+		int delta, cdata, offset, reg, temp;
+
+		if (i == 3)
+			reg = (caldata[1] >> 12)
+			      | ((caldata[2] >> 12) << 4)
+			      | ((caldata[3] >> 12) << 8);
+		else
+			reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
+
+		temp = tmdev->chip->calc_temp(tmdev, i, reg);
+		delta = ((temp - ft_temp * 100) * 10) / tmdev->chip->scale;
+		cdata = CALIBRATE_DEFAULT - delta;
+		if (cdata & ~TEMP_CALIB_MASK) {
+			dev_warn(dev, "sensor%d is not calibrated.\n", i);
+
+			continue;
+		}
+
+		offset = (i % 2) * 16;
+		regmap_update_bits(tmdev->regmap,
+				   SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
+				   TEMP_CALIB_MASK << offset,
+				   cdata << offset);
+	}
+
+	return 0;
+}
+
 static int sun8i_ths_calibrate(struct ths_device *tmdev)
 {
 	struct nvmem_cell *calcell;
@@ -659,6 +717,20 @@ static const struct ths_thermal_chip sun50i_h6_ths = {
 	.calc_temp = sun8i_ths_calc_temp,
 };
 
+static const struct ths_thermal_chip sun50i_h616_ths = {
+	.sensor_num = 4,
+	.has_bus_clk_reset = true,
+	.needs_syscon = true,
+	.ft_deviation = 8000,
+	.offset = 263655,
+	.scale = 810,
+	.temp_data_base = SUN50I_H6_THS_TEMP_DATA,
+	.calibrate = sun50i_h616_ths_calibrate,
+	.init = sun50i_h6_thermal_init,
+	.irq_ack = sun50i_h6_irq_ack,
+	.calc_temp = sun8i_ths_calc_temp,
+};
+
 static const struct of_device_id of_ths_match[] = {
 	{ .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
 	{ .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
@@ -667,6 +739,7 @@ static const struct of_device_id of_ths_match[] = {
 	{ .compatible = "allwinner,sun50i-a100-ths", .data = &sun50i_a100_ths },
 	{ .compatible = "allwinner,sun50i-h5-ths", .data = &sun50i_h5_ths },
 	{ .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
+	{ .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, of_ths_match);
-- 
2.35.8

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maksim Kiselev]

Hi Martin, Andre.

May I inquire? Why do we need a separate sun50i_h616_ths_calibrate() 
function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?

At my glance the calculations in both functions are the same. We just 
need to handle a special case for the 4th sensor.

Best regards,
Maksim

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Sat, 9 Dec 2023 13:44:34 +0300
Maksim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

> Hi Martin, Andre.
> 
> May I inquire? Why do we need a separate sun50i_h616_ths_calibrate() 
> function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> 
> At my glance the calculations in both functions are the same. We just 
> need to handle a special case for the 4th sensor.

You seem to be right, they are indeed the same, just written slightly
differently. Do you already have any patches that unify that?
I don't know if Martin or I find time to do it this week, but we could
also optimise this later.

Cheers,
Andre

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maxim Kiselev]

пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
>
> On Sat, 9 Dec 2023 13:44:34 +0300
> Maksim Kiselev <bigunclemax@gmail.com> wrote:
>
> Hi Maksim,
>
> > Hi Martin, Andre.
> >
> > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> >
> > At my glance the calculations in both functions are the same. We just
> > need to handle a special case for the 4th sensor.
>
> You seem to be right, they are indeed the same, just written slightly
> differently. Do you already have any patches that unify that?

No, I don't have any patches for that yet. But I can do it if you told
me where to
send the patch. Should I put it here as a reply?

> I don't know if Martin or I find time to do it this week, but we could
> also optimise this later.
>
> Cheers,
> Andre

Best regards,
Maksim

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Tue, 12 Dec 2023 21:09:45 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

sorry for the delay.

> пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
> >
> > On Sat, 9 Dec 2023 13:44:34 +0300
> > Maksim Kiselev <bigunclemax@gmail.com> wrote:
> >
> > Hi Maksim,
> >  
> > > Hi Martin, Andre.
> > >
> > > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> > >
> > > At my glance the calculations in both functions are the same. We just
> > > need to handle a special case for the 4th sensor.  
> >
> > You seem to be right, they are indeed the same, just written slightly
> > differently. Do you already have any patches that unify that?  
> 
> No, I don't have any patches for that yet. But I can do it if you told
> me where to
> send the patch. Should I put it here as a reply?

Can you make one patch on top of mainline, that prepares the existing
sun50i_h6_ths_calibrate() function to deal with four sensors? I would then
include this patch of yours in the next submission, and put Martin's H616
patch (now probably just the struct ths_thermal_chip) on top then.

Please send this patch just to the list (CC:ing people like Martin and
me), starting a new thread.

Many thanks,
Andre

> > I don't know if Martin or I find time to do it this week, but we could
> > also optimise this later.
> >
> > Cheers,
> > Andre  
> 
> Best regards,
> Maksim

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maxim Kiselev]

Hi Andre,

чт, 14 дек. 2023 г. в 13:59, Andre Przywara <andre.przywara@arm.com>:
>
> On Tue, 12 Dec 2023 21:09:45 +0300
> Maxim Kiselev <bigunclemax@gmail.com> wrote:
>
> Hi Maksim,
>
> sorry for the delay.

Now it's my turn to apologize for the delay :)

>
> > пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
> > >
> > > On Sat, 9 Dec 2023 13:44:34 +0300
> > > Maksim Kiselev <bigunclemax@gmail.com> wrote:
> > >
> > > Hi Maksim,
> > >
> > > > Hi Martin, Andre.
> > > >
> > > > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > > > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> > > >
> > > > At my glance the calculations in both functions are the same. We just
> > > > need to handle a special case for the 4th sensor.
> > >
> > > You seem to be right, they are indeed the same, just written slightly
> > > differently. Do you already have any patches that unify that?
> >
> > No, I don't have any patches for that yet. But I can do it if you told
> > me where to
> > send the patch. Should I put it here as a reply?
>
> Can you make one patch on top of mainline, that prepares the existing
> sun50i_h6_ths_calibrate() function to deal with four sensors? I would then
> include this patch of yours in the next submission, and put Martin's H616
> patch (now probably just the struct ths_thermal_chip) on top then.
>
> Please send this patch just to the list (CC:ing people like Martin and
> me), starting a new thread.

You can find my patch here:
https://lore.kernel.org/all/20231217133637.54773-1-bigunclemax@gmail.com/

Best regards,
Maksim

> Many thanks,
> Andre
>
> > > I don't know if Martin or I find time to do it this week, but we could
> > > also optimise this later.
> > >
> > > Cheers,
> > > Andre
> >
> > Best regards,
> > Maksim
>

[PATCH v3 6/6] arm64: dts: allwinner: h616: Add thermal sensor and zones

[Andre Przywara]

From: Martin Botka <martin.botka@somainline.org>

There are four thermal sensors:
- CPU
- GPU
- VE
- DRAM

Add the thermal sensor configuration and the thermal zones.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index d549d277d9729..94764f2bd375b 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -9,6 +9,7 @@
 #include <dt-bindings/clock/sun6i-rtc.h>
 #include <dt-bindings/reset/sun50i-h616-ccu.h>
 #include <dt-bindings/reset/sun50i-h6-r-ccu.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	interrupt-parent = <&gic>;
@@ -138,6 +139,10 @@ sid: efuse@3006000 {
 			reg = <0x03006000 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			ths_calibration: thermal-sensor-calibration@14 {
+				reg = <0x14 0x8>;
+			};
 		};
 
 		watchdog: watchdog@30090a0 {
@@ -511,6 +516,19 @@ mdio0: mdio {
 			};
 		};
 
+		ths: thermal-sensor@5070400 {
+			compatible = "allwinner,sun50i-h616-ths";
+			reg = <0x05070400 0x400>;
+			interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_THS>;
+			clock-names = "bus";
+			resets = <&ccu RST_BUS_THS>;
+			nvmem-cells = <&ths_calibration>;
+			nvmem-cell-names = "calibration";
+			syscon = <&syscon>;
+			#thermal-sensor-cells = <1>;
+		};
+
 		usbotg: usb@5100000 {
 			compatible = "allwinner,sun50i-h616-musb",
 				     "allwinner,sun8i-h3-musb";
@@ -755,4 +773,74 @@ r_rsb: rsb@7083000 {
 			#size-cells = <0>;
 		};
 	};
+
+	thermal-zones {
+		cpu-thermal {
+			polling-delay-passive = <500>;
+			polling-delay = <1000>;
+			thermal-sensors = <&ths 2>;
+			sustainable-power = <1000>;
+
+			trips {
+				cpu_threshold: cpu-trip-0 {
+					temperature = <60000>;
+					type = "passive";
+					hysteresis = <0>;
+				};
+				cpu_target: cpu-trip-1 {
+					temperature = <70000>;
+					type = "passive";
+					hysteresis = <0>;
+				};
+				cpu_critical: cpu-trip-2 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		gpu-thermal {
+			polling-delay-passive = <500>;
+			polling-delay = <1000>;
+			thermal-sensors = <&ths 0>;
+			sustainable-power = <1100>;
+
+			trips {
+				gpu_temp_critical: gpu-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		ve-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&ths 1>;
+
+			trips {
+				ve_temp_critical: ve-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&ths 3>;
+
+			trips {
+				ddr_temp_critical: ddr-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+	};
 };
-- 
2.35.8

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maxim Kiselev]

Hello, again!

On 28.11.2023 03:58, Andre Przywara wrote:
> From: Martin Botka <martin.botka@somainline.org>
>
> Add support for the thermal sensor found in H616 SoCs, which slightly
> resembles the H6 thermal sensor controller, with a few changes like
> four sensors.
> Also the registers readings are wrong, unless a bit in the first SYS_CFG
> register cleared, so set needs_syscon to trigger that code
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> drivers/thermal/sun8i_thermal.c | 73 +++++++++++++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 920e419ce7343..9a404fa9d76a9 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -280,6 +280,64 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
> return 0;
> }
>
> +static int sun50i_h616_ths_calibrate(struct ths_device *tmdev,
> + u16 *caldata, int callen)
> +{
> + struct device *dev = tmdev->dev;
> + int i, ft_temp;
> +
> + if (!caldata[0])
> + return -EINVAL;
> +
> + /*
> + * h616 efuse THS calibration data layout:
> + *
> + * 0 11 16 27 32 43 48 57
> + * +----------+-----------+-----------+-----------+
> + * | temp | |sensor0| |sensor1| |sensor2| |
> + * +----------+-----------+-----------+-----------+
> + * ^ ^ ^
> + * | | |
> + * | | sensor3[11:8]
> + * | sensor3[7:4]
> + * sensor3[3:0]
> + *
> + * The calibration data on the H616 is the ambient temperature and
> + * sensor values that are filled during the factory test stage.
> + *
> + * The unit of stored FT temperature is 0.1 degree celsius.
> + */
> + ft_temp = caldata[0] & FT_TEMP_MASK;
> +
> + for (i = 0; i < tmdev->chip->sensor_num; i++) {
> + int delta, cdata, offset, reg, temp;
> +
> + if (i == 3)
> + reg = (caldata[1] >> 12)
> + | ((caldata[2] >> 12) << 4)
> + | ((caldata[3] >> 12) << 8);
> + else
> + reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
> +
> + temp = tmdev->chip->calc_temp(tmdev, i, reg);
> + delta = ((temp - ft_temp * 100) * 10) / tmdev->chip->scale;
> + cdata = CALIBRATE_DEFAULT - delta;
> + if (cdata & ~TEMP_CALIB_MASK) {
> + dev_warn(dev, "sensor%d is not calibrated.\n", i);
> +
> + continue;
> + }
> +
> + offset = (i % 2) * 16;
> + regmap_update_bits(tmdev->regmap,
> + SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
> + TEMP_CALIB_MASK << offset,
> + cdata << offset);
> + }
> +
> + return 0;
> +}
> +
> static int sun8i_ths_calibrate(struct ths_device *tmdev)
> {
> struct nvmem_cell *calcell;
> @@ -659,6 +717,20 @@ static const struct ths_thermal_chip sun50i_h6_ths = {
> .calc_temp = sun8i_ths_calc_temp,
> };
>
> +static const struct ths_thermal_chip sun50i_h616_ths = {
> + .sensor_num = 4,
> + .has_bus_clk_reset = true,
> + .needs_syscon = true,
> + .ft_deviation = 8000,
> + .offset = 263655,
> + .scale = 810,

Can I ask you, how did you get the offset and the scale values?

In the H616 user manual (3.10.3.4. THS Temperature Conversion Formula)
we can find this formula:

T = (sensor_data - 3255)/(-12.401)

I calculated offset and scale values, and this is what I got:

scale = 1 / 12.401 = 806,38658173 ~ 806
offset = 3255 / 12.401 = 262478,832352 ~ 262479

Let me assume that you got offset and scale values from Tina Linux
Here for example
https://github.com/Tina-Linux/linux-5.4/blob/ff6b1361f7041be75d4486559141e13134d164ef/drivers/thermal/sunxi_thermal.c#L373

In Tina Linux the scale is rounded to 81 (that equals to 810 in
mainline driver).
And looks like you used this rounded value to recalculate the offset:
3255 * 81 = 263655

I would be glad to know your thoughts about this.

> + .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
> + .calibrate = sun50i_h616_ths_calibrate,
> + .init = sun50i_h6_thermal_init,
> + .irq_ack = sun50i_h6_irq_ack,
> + .calc_temp = sun8i_ths_calc_temp,
> +};
> +
> static const struct of_device_id of_ths_match[] = {
> { .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
> { .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
> @@ -667,6 +739,7 @@ static const struct of_device_id of_ths_match[] = {
> { .compatible = "allwinner,sun50i-a100-ths", .data = &sun50i_a100_ths },
> { .compatible = "allwinner,sun50i-h5-ths", .data = &sun50i_h5_ths },
> { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> + { .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, of_ths_match);

Best regards,
Maksim

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Wed, 13 Dec 2023 12:41:29 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

> Hello, again!
> 
> On 28.11.2023 03:58, Andre Przywara wrote:
> > From: Martin Botka <martin.botka@somainline.org>
> >
> > Add support for the thermal sensor found in H616 SoCs, which slightly
> > resembles the H6 thermal sensor controller, with a few changes like
> > four sensors.
> > Also the registers readings are wrong, unless a bit in the first SYS_CFG
> > register cleared, so set needs_syscon to trigger that code
> >
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > drivers/thermal/sun8i_thermal.c | 73 +++++++++++++++++++++++++++++++++
> > 1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > index 920e419ce7343..9a404fa9d76a9 100644
> > --- a/drivers/thermal/sun8i_thermal.c
> > +++ b/drivers/thermal/sun8i_thermal.c
> > @@ -280,6 +280,64 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
> > return 0;
> > }
> >
> > +static int sun50i_h616_ths_calibrate(struct ths_device *tmdev,
> > + u16 *caldata, int callen)
> > +{
> > + struct device *dev = tmdev->dev;
> > + int i, ft_temp;
> > +
> > + if (!caldata[0])
> > + return -EINVAL;
> > +
> > + /*
> > + * h616 efuse THS calibration data layout:
> > + *
> > + * 0 11 16 27 32 43 48 57
> > + * +----------+-----------+-----------+-----------+
> > + * | temp | |sensor0| |sensor1| |sensor2| |
> > + * +----------+-----------+-----------+-----------+
> > + * ^ ^ ^
> > + * | | |
> > + * | | sensor3[11:8]
> > + * | sensor3[7:4]
> > + * sensor3[3:0]
> > + *
> > + * The calibration data on the H616 is the ambient temperature and
> > + * sensor values that are filled during the factory test stage.
> > + *
> > + * The unit of stored FT temperature is 0.1 degree celsius.
> > + */
> > + ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > + for (i = 0; i < tmdev->chip->sensor_num; i++) {
> > + int delta, cdata, offset, reg, temp;
> > +
> > + if (i == 3)
> > + reg = (caldata[1] >> 12)
> > + | ((caldata[2] >> 12) << 4)
> > + | ((caldata[3] >> 12) << 8);
> > + else
> > + reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
> > +
> > + temp = tmdev->chip->calc_temp(tmdev, i, reg);
> > + delta = ((temp - ft_temp * 100) * 10) / tmdev->chip->scale;
> > + cdata = CALIBRATE_DEFAULT - delta;
> > + if (cdata & ~TEMP_CALIB_MASK) {
> > + dev_warn(dev, "sensor%d is not calibrated.\n", i);
> > +
> > + continue;
> > + }
> > +
> > + offset = (i % 2) * 16;
> > + regmap_update_bits(tmdev->regmap,
> > + SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
> > + TEMP_CALIB_MASK << offset,
> > + cdata << offset);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int sun8i_ths_calibrate(struct ths_device *tmdev)
> > {
> > struct nvmem_cell *calcell;
> > @@ -659,6 +717,20 @@ static const struct ths_thermal_chip sun50i_h6_ths = {
> > .calc_temp = sun8i_ths_calc_temp,
> > };
> >
> > +static const struct ths_thermal_chip sun50i_h616_ths = {
> > + .sensor_num = 4,
> > + .has_bus_clk_reset = true,
> > + .needs_syscon = true,
> > + .ft_deviation = 8000,
> > + .offset = 263655,
> > + .scale = 810,  
> 
> Can I ask you, how did you get the offset and the scale values?
> 
> In the H616 user manual (3.10.3.4. THS Temperature Conversion Formula)
> we can find this formula:
> 
> T = (sensor_data - 3255)/(-12.401)
> 
> I calculated offset and scale values, and this is what I got:
> 
> scale = 1 / 12.401 = 806,38658173 ~ 806
> offset = 3255 / 12.401 = 262478,832352 ~ 262479
> 
> Let me assume that you got offset and scale values from Tina Linux
> Here for example
> https://github.com/Tina-Linux/linux-5.4/blob/ff6b1361f7041be75d4486559141e13134d164ef/drivers/thermal/sunxi_thermal.c#L373
> 
> In Tina Linux the scale is rounded to 81 (that equals to 810 in
> mainline driver).
> And looks like you used this rounded value to recalculate the offset:
> 3255 * 81 = 263655
> 
> I would be glad to know your thoughts about this.

Yes, this submission was based on some BSP code, from there we calculated
the values to match the formula used in mainline, so this would include
the rounding error, as your rightly figured.

I don't know if the manual is any more accurate than the BSP data, but
your calculations above make sense, and would explain the difference.

I will use your new values in the next version.

Thanks,
Andre

> > + .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
> > + .calibrate = sun50i_h616_ths_calibrate,
> > + .init = sun50i_h6_thermal_init,
> > + .irq_ack = sun50i_h6_irq_ack,
> > + .calc_temp = sun8i_ths_calc_temp,
> > +};
> > +
> > static const struct of_device_id of_ths_match[] = {
> > { .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
> > { .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
> > @@ -667,6 +739,7 @@ static const struct of_device_id of_ths_match[] = {
> > { .compatible = "allwinner,sun50i-a100-ths", .data = &sun50i_a100_ths },
> > { .compatible = "allwinner,sun50i-h5-ths", .data = &sun50i_h5_ths },
> > { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
> > + { .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
> > { /* sentinel */ },
> > };
> > MODULE_DEVICE_TABLE(of, of_ths_match);  
> 
> Best regards,
> Maksim
>