[PATCH v3 0/3] riscv: sophgo: add thermal sensor support for cv180x/sg200x SoCs

[PATCH v3 0/3] riscv: sophgo: add thermal sensor support for cv180x/sg200x SoCs

[Haylen Chu]

This series implements driver for Sophgo cv180x/sg200x on-chip thermal
sensor and adds thermal zones for CV1800B SoCs.

Changed from v2:
1. style and code improvements
2. use human-readable value for sensor parameters

Changed from v1:
1. style and code improvements
2. make sample parameters configurable
3. generalize document temperature calculating formula

Haylen Chu (3):
  dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal
  riscv: dts: sophgo: cv18xx: Add sensor device and thermal zone
  thermal: cv180x: Add cv180x thermal driver support

 .../thermal/sophgo,cv1800-thermal.yaml        |  74 +++++
 arch/riscv/boot/dts/sophgo/cv1800b.dtsi       |  30 ++
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi        |   8 +
 drivers/thermal/Kconfig                       |   6 +
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/cv180x_thermal.c              | 281 ++++++++++++++++++
 6 files changed, 400 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
 create mode 100644 drivers/thermal/cv180x_thermal.c

-- 
2.45.2

[PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Haylen Chu]

Add devicetree binding documentation for thermal sensors integrated in
Sophgo CV180X SoCs.

Signed-off-by: Haylen Chu <heylenay@outlook.com>
---
 .../thermal/sophgo,cv1800-thermal.yaml        | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml

diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
new file mode 100644
index 000000000000..016299822c16
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV1800 on-SoC Thermal Sensor
+
+maintainers:
+  - Haylen Chu <heylenay@outlook.com>
+
+description: Binding for Sophgo CV1800 on-SoC thermal sensor
+
+properties:
+  compatible:
+    enum:
+      - sophgo,cv1800-thermal
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: The thermal sensor clock
+
+  interrupts:
+    maxItems: 1
+
+  accumulation-period:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Accumulation period for a sample
+    enum:
+      - 512
+      - 1024
+      - 2048
+      - 4096
+    default: 2048
+
+  chop-period:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: ADC chop period
+    enum:
+      - 128
+      - 256
+      - 512
+      - 1024
+    default: 1024
+
+  sample-cycle-us:
+    description: Period between samples. Should be greater than 524us.
+    default: 1000000
+
+  '#thermal-sensor-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/clock/sophgo,cv1800.h>
+        #include <dt-bindings/interrupt-controller/irq.h>
+        thermal-sensor@30e0000 {
+            compatible = "sophgo,cv1800-thermal";
+            reg = <0x30e0000 0x100>;
+            clocks = <&clk CLK_TEMPSEN>;
+            interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
+            #thermal-sensor-cells = <0>;
+        };
+...
-- 
2.45.2

[PATCH v3 2/3] riscv: dts: sophgo: cv18xx: Add sensor device and thermal zone

[Haylen Chu]

Add common sensor device Sophgo CV18xx SoCs and thermal zone for
CV1800b SoCs.

Signed-off-by: Haylen Chu <heylenay@outlook.com>
---
 arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 30 +++++++++++++++++++++++++
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi  |  8 +++++++
 2 files changed, 38 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
index ec9530972ae2..0b5c7bc94b05 100644
--- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
@@ -12,6 +12,34 @@ memory@80000000 {
 		device_type = "memory";
 		reg = <0x80000000 0x4000000>;
 	};
+
+	thermal-zones {
+		soc-thermal {
+			polling-delay-passive   = <1000>;
+			polling-delay           = <1000>;
+			thermal-sensors         = <&soc_temp>;
+
+			trips {
+				soc_passive: soc-passive {
+					temperature     = <75000>;
+					hysteresis      = <5000>;
+					type            = "passive";
+				};
+
+				soc_hot: soc-hot {
+					temperature     = <85000>;
+					hysteresis      = <5000>;
+					type            = "hot";
+				};
+
+				soc_critical: soc-critical {
+					temperature     = <100000>;
+					hysteresis      = <0>;
+					type            = "critical";
+				};
+			};
+		};
+	};
 };
 
 &plic {
@@ -25,3 +53,5 @@ &clint {
 &clk {
 	compatible = "sophgo,cv1800-clk";
 };
+
+
diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
index 891932ae470f..76b02cc279aa 100644
--- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
@@ -310,5 +310,13 @@ clint: timer@74000000 {
 			reg = <0x74000000 0x10000>;
 			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
 		};
+
+		soc_temp: thermal-sensor@30e0000 {
+			compatible = "sophgo,cv1800-thermal";
+			reg = <0x30e0000 0x100>;
+			clocks = <&clk CLK_TEMPSEN>;
+			interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
+			#thermal-sensor-cells = <0>;
+		};
 	};
 };
-- 
2.45.2

[PATCH v3 3/3] thermal: cv180x: Add cv180x thermal driver support

[Haylen Chu]

Add support for cv180x SoCs integrated thermal sensors.

Signed-off-by: Haylen Chu <heylenay@outlook.com>
---
 drivers/thermal/Kconfig          |   6 +
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/cv180x_thermal.c | 281 +++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/thermal/cv180x_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 204ed89a3ec9..f53c973a361d 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -514,4 +514,10 @@ config LOONGSON2_THERMAL
 	  is higher than the high temperature threshold or lower than the low
 	  temperature threshold, the interrupt will occur.
 
+config CV180X_THERMAL
+	tristate "Temperature sensor driver for Sophgo CV180X"
+	help
+	  If you say yes here you get support for thermal sensor integrated in
+	  Sophgo CV180X SoCs.
+
 endif
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 5cdf7d68687f..5b59bde8a579 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -65,3 +65,4 @@ obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
 obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
 obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
 obj-$(CONFIG_LOONGSON2_THERMAL)	+= loongson2_thermal.o
+obj-$(CONFIG_CV180X_THERMAL)	+= cv180x_thermal.o
diff --git a/drivers/thermal/cv180x_thermal.c b/drivers/thermal/cv180x_thermal.c
new file mode 100644
index 000000000000..ab8f0ba8113d
--- /dev/null
+++ b/drivers/thermal/cv180x_thermal.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Sophgo Inc.
+ * Copyright (C) 2024 Haylen Chu <heylenay@outlook.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define TEMPSEN_VERSION					0x0
+#define TEMPSEN_CTRL					0x4
+#define  TEMPSEN_CTRL_EN				BIT(0)
+#define  TEMPSEN_CTRL_SEL_MASK				GENMASK(7, 4)
+#define  TEMPSEN_CTRL_SEL_OFFSET			4
+#define TEMPSEN_STATUS					0x8
+#define TEMPSEN_SET					0xc
+#define  TEMPSEN_SET_CHOPSEL_MASK			GENMASK(5, 4)
+#define  TEMPSEN_SET_CHOPSEL_OFFSET			4
+#define  TEMPSEN_SET_CHOPSEL_128T			0
+#define  TEMPSEN_SET_CHOPSEL_256T			1
+#define  TEMPSEN_SET_CHOPSEL_512T			2
+#define  TEMPSEN_SET_CHOPSEL_1024T			3
+#define  TEMPSEN_SET_ACCSEL_MASK			GENMASK(7, 6)
+#define  TEMPSEN_SET_ACCSEL_OFFSET			6
+#define  TEMPSEN_SET_ACCSEL_512T			0
+#define  TEMPSEN_SET_ACCSEL_1024T			1
+#define  TEMPSEN_SET_ACCSEL_2048T			2
+#define  TEMPSEN_SET_ACCSEL_4096T			3
+#define  TEMPSEN_SET_CYC_CLKDIV_MASK			GENMASK(15, 8)
+#define  TEMPSEN_SET_CYC_CLKDIV_OFFSET			8
+#define TEMPSEN_INTR_EN					0x10
+#define TEMPSEN_INTR_CLR				0x14
+#define TEMPSEN_INTR_STA				0x18
+#define TEMPSEN_INTR_RAW				0x1c
+#define TEMPSEN_RESULT(n)				(0x20 + (n) * 4)
+#define  TEMPSEN_RESULT_RESULT_MASK			GENMASK(12, 0)
+#define  TEMPSEN_RESULT_MAX_RESULT_MASK			GENMASK(28, 16)
+#define  TEMPSEN_RESULT_CLR_MAX_RESULT			BIT(31)
+#define TEMPSEN_AUTO_PERIOD				0x64
+#define  TEMPSEN_AUTO_PERIOD_AUTO_CYCLE_MASK		GENMASK(23, 0)
+#define  TEMPSEN_AUTO_PERIOD_AUTO_CYCLE_OFFSET		0
+
+struct cv180x_thermal_zone {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *clk_tempsen;
+	u32 chop_period;
+	u32 accum_period;
+	u32 sample_cycle;
+};
+
+static void cv180x_thermal_init(struct cv180x_thermal_zone *ctz)
+{
+	void __iomem *base = ctz->base;
+	u32 regval;
+
+	writel(readl(base + TEMPSEN_INTR_RAW), base + TEMPSEN_INTR_CLR);
+	writel(TEMPSEN_RESULT_CLR_MAX_RESULT, base + TEMPSEN_RESULT(0));
+
+	regval = readl(base + TEMPSEN_SET);
+	regval &= ~TEMPSEN_SET_CHOPSEL_MASK;
+	regval &= ~TEMPSEN_SET_ACCSEL_MASK;
+	regval &= ~TEMPSEN_SET_CYC_CLKDIV_MASK;
+	regval |= ctz->chop_period << TEMPSEN_SET_CHOPSEL_OFFSET;
+	regval |= ctz->accum_period << TEMPSEN_SET_ACCSEL_OFFSET;
+	regval |= 0x31 << TEMPSEN_SET_CYC_CLKDIV_OFFSET;
+	writel(regval, base + TEMPSEN_SET);
+
+	regval = readl(base + TEMPSEN_AUTO_PERIOD);
+	regval &= ~TEMPSEN_AUTO_PERIOD_AUTO_CYCLE_MASK;
+	regval |= ctz->sample_cycle << TEMPSEN_AUTO_PERIOD_AUTO_CYCLE_OFFSET;
+	writel(regval, base + TEMPSEN_AUTO_PERIOD);
+
+	regval = readl(base + TEMPSEN_CTRL);
+	regval &= ~TEMPSEN_CTRL_SEL_MASK;
+	regval |= 1 << TEMPSEN_CTRL_SEL_OFFSET;
+	regval |= TEMPSEN_CTRL_EN;
+	writel(regval, base + TEMPSEN_CTRL);
+}
+
+static void cv180x_thermal_deinit(struct cv180x_thermal_zone *ct)
+{
+	void __iomem *base = ct->base;
+	u32 regval;
+
+	regval = readl(base + TEMPSEN_CTRL);
+	regval &= ~(TEMPSEN_CTRL_SEL_MASK | TEMPSEN_CTRL_EN);
+	writel(regval, base + TEMPSEN_CTRL);
+
+	writel(readl(base + TEMPSEN_INTR_RAW), base + TEMPSEN_INTR_CLR);
+}
+
+/*
+ *	Raw register value to temperature (mC) formula:
+ *
+ *		       read_val * 1000 * 716
+ *	Temperature = ----------------------- - 273000
+ *				divider
+ *
+ *	where divider should be ticks number of accumulation period,
+ *	e.g. 2048 for TEMPSEN_CTRL_ACCSEL_2048T
+ */
+static int cv180x_calc_temp(struct cv180x_thermal_zone *ctz, u32 result)
+{
+	u32 divider = (u32)(512 * int_pow(2, ctz->accum_period));
+
+	return (result * 1000) * 716 / divider - 273000;
+}
+
+static int cv180x_get_temp(struct thermal_zone_device *tdev, int *temperature)
+{
+	struct cv180x_thermal_zone *ctz = thermal_zone_device_priv(tdev);
+	void __iomem *base = ctz->base;
+	u32 result;
+
+	result = readl(base + TEMPSEN_RESULT(0)) & TEMPSEN_RESULT_RESULT_MASK;
+	*temperature = cv180x_calc_temp(ctz, result);
+
+	return 0;
+}
+
+static const struct thermal_zone_device_ops cv180x_thermal_ops = {
+	.get_temp = cv180x_get_temp,
+};
+
+static const struct of_device_id cv180x_thermal_of_match[] = {
+	{ .compatible = "sophgo,cv1800-thermal" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cv180x_thermal_of_match);
+
+static int
+cv180x_parse_dt(struct cv180x_thermal_zone *ctz)
+{
+	struct device_node *np = ctz->dev->of_node;
+	u32 tmp;
+
+	if (of_property_read_u32(np, "accumulation-period", &tmp)) {
+		ctz->accum_period = TEMPSEN_SET_ACCSEL_2048T;
+	} else {
+		switch (tmp) {
+		case 512:
+			ctz->accum_period = TEMPSEN_SET_ACCSEL_512T;
+			break;
+		case 1024:
+			ctz->accum_period = TEMPSEN_SET_ACCSEL_1024T;
+			break;
+		case 2048:
+			ctz->accum_period = TEMPSEN_SET_ACCSEL_2048T;
+			break;
+		case 4096:
+			ctz->accum_period = TEMPSEN_SET_ACCSEL_4096T;
+			break;
+		default:
+			dev_err(ctz->dev, "invalid accumulation period %d\n",
+				tmp);
+			return -EINVAL;
+		}
+	}
+
+	if (of_property_read_u32(np, "chop-period", &tmp)) {
+		ctz->chop_period = TEMPSEN_SET_CHOPSEL_1024T;
+	} else {
+		switch (tmp) {
+		case 128:
+			ctz->chop_period = TEMPSEN_SET_CHOPSEL_128T;
+			break;
+		case 256:
+			ctz->chop_period = TEMPSEN_SET_CHOPSEL_256T;
+			break;
+		case 512:
+			ctz->chop_period = TEMPSEN_SET_CHOPSEL_512T;
+			break;
+		case 1024:
+			ctz->chop_period = TEMPSEN_SET_CHOPSEL_1024T;
+			break;
+		default:
+			dev_err(ctz->dev, "invalid chop period %d\n", tmp);
+			return -EINVAL;
+		}
+	}
+
+	if (of_property_read_u32(np, "sample-cycle-us", &ctz->sample_cycle))
+		ctz->sample_cycle = 1000000;
+
+	return 0;
+}
+
+static int cv180x_thermal_probe(struct platform_device *pdev)
+{
+	struct cv180x_thermal_zone *ctz;
+	struct thermal_zone_device *tz;
+	struct resource *res;
+	int ret;
+
+	ctz = devm_kzalloc(&pdev->dev, sizeof(*ctz), GFP_KERNEL);
+	if (!ctz)
+		return -ENOMEM;
+
+	ctz->dev = &pdev->dev;
+
+	ret = cv180x_parse_dt(ctz);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to parse dt\n");
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ctz->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ctz->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(ctz->base),
+				     "failed to map tempsen registers\n");
+
+	ctz->clk_tempsen = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(ctz->clk_tempsen))
+		return dev_err_probe(&pdev->dev, PTR_ERR(ctz->clk_tempsen),
+				     "failed to get clk_tempsen\n");
+
+	cv180x_thermal_init(ctz);
+
+	tz = devm_thermal_of_zone_register(&pdev->dev, 0, ctz,
+					   &cv180x_thermal_ops);
+	if (IS_ERR(tz))
+		return dev_err_probe(&pdev->dev, PTR_ERR(tz),
+				     "failed to register thermal zone\n");
+
+	platform_set_drvdata(pdev, ctz);
+
+	return 0;
+}
+
+static int cv180x_thermal_remove(struct platform_device *pdev)
+{
+	struct cv180x_thermal_zone *ctz = platform_get_drvdata(pdev);
+
+	cv180x_thermal_deinit(ctz);
+
+	return 0;
+}
+
+static int __maybe_unused cv180x_thermal_suspend(struct device *dev)
+{
+	struct cv180x_thermal_zone *ctz = dev_get_drvdata(dev);
+
+	cv180x_thermal_deinit(ctz);
+	clk_disable_unprepare(ctz->clk_tempsen);
+
+	return 0;
+}
+
+static int __maybe_unused cv180x_thermal_resume(struct device *dev)
+{
+	struct cv180x_thermal_zone *ctz = dev_get_drvdata(dev);
+
+	clk_prepare_enable(ctz->clk_tempsen);
+	cv180x_thermal_init(ctz);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cv180x_thermal_pm_ops,
+			 cv180x_thermal_suspend, cv180x_thermal_resume);
+
+static struct platform_driver cv180x_thermal_driver = {
+	.probe = cv180x_thermal_probe,
+	.remove = cv180x_thermal_remove,
+	.driver = {
+		.name = "cv180x-thermal",
+		.pm = &cv180x_thermal_pm_ops,
+		.of_match_table = cv180x_thermal_of_match,
+	},
+};
+
+module_platform_driver(cv180x_thermal_driver);
+
+MODULE_DESCRIPTION("Sophgo CV180x thermal driver");
+MODULE_AUTHOR("Haylen Chu <heylenay@outlook.com>");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

Rob/Krzysztof, Haylen,

On Tue, Jul 02, 2024 at 09:30:24AM +0000, Haylen Chu wrote:
> Add devicetree binding documentation for thermal sensors integrated in
> Sophgo CV180X SoCs.
> 
> Signed-off-by: Haylen Chu <heylenay@outlook.com>
> ---
>  .../thermal/sophgo,cv1800-thermal.yaml        | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> 
> diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> new file mode 100644
> index 000000000000..016299822c16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo CV1800 on-SoC Thermal Sensor
> +
> +maintainers:
> +  - Haylen Chu <heylenay@outlook.com>
> +
> +description: Binding for Sophgo CV1800 on-SoC thermal sensor
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sophgo,cv1800-thermal
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: The thermal sensor clock
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  accumulation-period:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Accumulation period for a sample
> +    enum:
> +      - 512
> +      - 1024
> +      - 2048
> +      - 4096
> +    default: 2048
> +
> +  chop-period:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: ADC chop period
> +    enum:
> +      - 128
> +      - 256
> +      - 512
> +      - 1024
> +    default: 1024
> +
> +  sample-cycle-us:

the more common term btw would be "sample-rate" rather than
"sample-cycle".

> +    description: Period between samples. Should be greater than 524us.

The constraint here should be "minimum: 524". What's the upper limit?

> +    default: 1000000

Rob/Krzysztof, could you comment on the suitability of the three custom
properties here? I know if this was an IIO device, these kinds of things
would be controllable from userspace, and not in the binding. I
mentioned this on the previous version, but I'm not really sure if
thermal devices are somehow different:
https://lore.kernel.org/all/SEYPR01MB4221A739D0645EF0255336EBD7CE2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/

Cheers,
Conor.

> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/clock/sophgo,cv1800.h>
> +        #include <dt-bindings/interrupt-controller/irq.h>
> +        thermal-sensor@30e0000 {
> +            compatible = "sophgo,cv1800-thermal";
> +            reg = <0x30e0000 0x100>;
> +            clocks = <&clk CLK_TEMPSEN>;
> +            interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> +            #thermal-sensor-cells = <0>;
> +        };
> +...
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Krzysztof Kozlowski]

On 02/07/2024 17:00, Conor Dooley wrote:
> Rob/Krzysztof, Haylen,
>> +
>> +maintainers:
>> +  - Haylen Chu <heylenay@outlook.com>
>> +
>> +description: Binding for Sophgo CV1800 on-SoC thermal sensor

Drop "Binding"

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - sophgo,cv1800-thermal
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    description: The thermal sensor clock
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  accumulation-period:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Accumulation period for a sample
>> +    enum:
>> +      - 512
>> +      - 1024
>> +      - 2048
>> +      - 4096
>> +    default: 2048
>> +
>> +  chop-period:

period in what sort of units? Sounds like time to me, so this would
require proper unit suffix.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: ADC chop period
>> +    enum:
>> +      - 128
>> +      - 256
>> +      - 512
>> +      - 1024
>> +    default: 1024
>> +
>> +  sample-cycle-us:
> 
> the more common term btw would be "sample-rate" rather than
> "sample-cycle".

yeah, sample-rate-hz

> 
>> +    description: Period between samples. Should be greater than 524us.
> 
> The constraint here should be "minimum: 524". What's the upper limit?
> 
>> +    default: 1000000
> 
> Rob/Krzysztof, could you comment on the suitability of the three custom
> properties here? I know if this was an IIO device, these kinds of things
> would be controllable from userspace, and not in the binding. I
> mentioned this on the previous version, but I'm not really sure if
> thermal devices are somehow different:
> https://lore.kernel.org/all/SEYPR01MB4221A739D0645EF0255336EBD7CE2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> 

Why would different boards have different values here? Does it affect
accuracy? If so, how much?

I doubt there are any boards with different values, thus it sounds like
unnecessary tuning parameter.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Haylen Chu]

On Tue, Jul 02, 2024 at 05:09:35PM +0200, Krzysztof Kozlowski wrote:
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - sophgo,cv1800-thermal
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    description: The thermal sensor clock
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  accumulation-period:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description: Accumulation period for a sample
> >> +    enum:
> >> +      - 512
> >> +      - 1024
> >> +      - 2048
> >> +      - 4096
> >> +    default: 2048
> >> +
> >> +  chop-period:
> 
> period in what sort of units? Sounds like time to me, so this would
> require proper unit suffix.

In clock ticks.

When setting to 1024, a time of sample takes (1024 + 2 + 64) clock
ticks. The clock runs at (25MHz / divider) and the divider is
configurable.

> > 
> >> +    description: Period between samples. Should be greater than 524us.
> > 
> > The constraint here should be "minimum: 524". What's the upper limit?
> > 
> >> +    default: 1000000
> > 
> > Rob/Krzysztof, could you comment on the suitability of the three custom
> > properties here? I know if this was an IIO device, these kinds of things
> > would be controllable from userspace, and not in the binding. I
> > mentioned this on the previous version, but I'm not really sure if
> > thermal devices are somehow different:
> > https://lore.kernel.org/all/SEYPR01MB4221A739D0645EF0255336EBD7CE2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
> > 
> 
> Why would different boards have different values here? Does it affect
> accuracy? If so, how much?
> 
> I doubt there are any boards with different values, thus it sounds like
> unnecessary tuning parameter.

Theses values affect accuracy in a minor way (about 1 Celsius degree in
my test) and could be shared between CV18xx/SG20xx SoCs as they have the
same design.

In the first revision, fixed values are used, and I was asked to add
support for all possible configuration[1]. Now I think this introduces
extra unnecessary complexity and should be avoided, since this is a
simple thermal sensor, tuning seems to be useless.

I suggest renaming "sample-cycle-us" to "sample-rate-hz" and dropping
other parameters for simplicity.

Best regards,
Haylen Chu

[1]: https://lore.kernel.org/all/IA1PR20MB49533177BEFC431FC16D1AB8BBF32@IA1PR20MB4953.namprd20.prod.outlook.com/

Re: [PATCH v3 1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

[Krzysztof Kozlowski]

On 04/07/2024 10:25, Haylen Chu wrote:
> On Tue, Jul 02, 2024 at 05:09:35PM +0200, Krzysztof Kozlowski wrote:
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - sophgo,cv1800-thermal
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    description: The thermal sensor clock
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  accumulation-period:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: Accumulation period for a sample
>>>> +    enum:
>>>> +      - 512
>>>> +      - 1024
>>>> +      - 2048
>>>> +      - 4096
>>>> +    default: 2048
>>>> +
>>>> +  chop-period:
>>
>> period in what sort of units? Sounds like time to me, so this would
>> require proper unit suffix.
> 
> In clock ticks.

Then please mention it in the property description.

> 
> When setting to 1024, a time of sample takes (1024 + 2 + 64) clock
> ticks. The clock runs at (25MHz / divider) and the divider is
> configurable.

> 
>>>
>>>> +    description: Period between samples. Should be greater than 524us.
>>>
>>> The constraint here should be "minimum: 524". What's the upper limit?
>>>
>>>> +    default: 1000000
>>>
>>> Rob/Krzysztof, could you comment on the suitability of the three custom
>>> properties here? I know if this was an IIO device, these kinds of things
>>> would be controllable from userspace, and not in the binding. I
>>> mentioned this on the previous version, but I'm not really sure if
>>> thermal devices are somehow different:
>>> https://lore.kernel.org/all/SEYPR01MB4221A739D0645EF0255336EBD7CE2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com/
>>>
>>
>> Why would different boards have different values here? Does it affect
>> accuracy? If so, how much?
>>
>> I doubt there are any boards with different values, thus it sounds like
>> unnecessary tuning parameter.
> 
> Theses values affect accuracy in a minor way (about 1 Celsius degree in
> my test) and could be shared between CV18xx/SG20xx SoCs as they have the
> same design.
> 
> In the first revision, fixed values are used, and I was asked to add
> support for all possible configuration[1]. Now I think this introduces
> extra unnecessary complexity and should be avoided, since this is a
> simple thermal sensor, tuning seems to be useless.
> 
> I suggest renaming "sample-cycle-us" to "sample-rate-hz" and dropping
> other parameters for simplicity.

Ack for me.

Best regards,
Krzysztof