[PATCH v7 0/2] Add thermal control driver for Sunplus SP7021 SoC

[PATCH v7 0/2] Add thermal control driver for Sunplus SP7021 SoC

[Li-hao Kuo]

This is a patch series for thermal driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Li-hao Kuo (2):
  thermal: Add thermal driver for Sunplus SP7021
  dt-bindings:thermal: Add Sunplus SP7021 schema

 .../bindings/thermal/sunplus-thermal.yaml          |  43 +++++++
 MAINTAINERS                                        |   7 ++
 drivers/thermal/Kconfig                            |  10 ++
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/sunplus_thermal.c                  | 139 +++++++++++++++++++++
 5 files changed, 200 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml
 create mode 100644 drivers/thermal/sunplus_thermal.c

-- 
2.7.4

[PATCH v7 1/2] thermal: Add thermal driver for Sunplus SP7021

[Li-hao Kuo]

Add thermal driver for Sunplus SP7021.

Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>
---
Changes in v7:
 - Modify yaml file.

 MAINTAINERS                       |   6 ++
 drivers/thermal/Kconfig           |  10 +++
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/sunplus_thermal.c | 139 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+)
 create mode 100644 drivers/thermal/sunplus_thermal.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 61d9f11..307570c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18885,6 +18885,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 F:	drivers/spi/spi-sunplus-sp7021.c
 
+SUNPLUS THERMAL DRIVER
+M:	Li-hao Kuo <lhjeff911@gmail.com>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	drivers/thermal/sunplus_thermal.c
+
 SUNPLUS UART DRIVER
 M:	Hammer Hsieh <hammerh0314@gmail.com>
 S:	Maintained
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index e37691e..66316c3 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -502,4 +502,14 @@ config KHADAS_MCU_FAN_THERMAL
 	  If you say yes here you get support for the FAN controlled
 	  by the Microcontroller found on the Khadas VIM boards.
 
+config SUNPLUS_THERMAL
+	tristate "Sunplus thermal drivers"
+	depends on SOC_SP7021 || COMPILE_TEST
+	help
+	  This the Sunplus SP7021 thermal driver, which supports the primitive
+	  temperature sensor embedded in Sunplus SP7021 SoC.
+
+	  If you have a Sunplus SP7021 platform say Y here and enable this option
+	  to have support for thermal management
+
 endif
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index f0c36a1..38a76f9 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -61,3 +61,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
 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_SUNPLUS_THERMAL)	+= sunplus_thermal.o
diff --git a/drivers/thermal/sunplus_thermal.c b/drivers/thermal/sunplus_thermal.c
new file mode 100644
index 0000000..9a9b348
--- /dev/null
+++ b/drivers/thermal/sunplus_thermal.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Sunplus Inc.
+ * Author: Li-hao Kuo <lhjeff911@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/thermal.h>
+
+#define DISABLE_THERMAL		(BIT(31) | BIT(15))
+#define ENABLE_THERMAL		BIT(31)
+#define SP_THERMAL_MASK		GENMASK(10, 0)
+#define SP_TCODE_HIGH_MASK	GENMASK(10, 8)
+#define SP_TCODE_LOW_MASK	GENMASK(7, 0)
+
+#define TEMP_RATE		608
+#define TEMP_BASE		3500
+#define TEMP_OTP_BASE		1518
+
+#define SP_THERMAL_CTL0_REG	0x0000
+#define SP_THERMAL_STS0_REG	0x0030
+
+/* common data structures */
+struct sp_thermal_data {
+	struct thermal_zone_device *pcb_tz;
+	struct platform_device *pdev;
+	enum thermal_device_mode mode;
+	void __iomem *regs;
+	int otp_temp0;
+	u32 id;
+};
+
+static int sp7021_get_otp_temp_coef(struct sp_thermal_data *sp_data, struct device *dev)
+{
+	struct nvmem_cell *cell;
+	ssize_t otp_l;
+	char *otp_v;
+
+	cell = nvmem_cell_get(dev, "calib");
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	otp_v = nvmem_cell_read(cell, &otp_l);
+	nvmem_cell_put(cell);
+
+	if (otp_l < 3)
+		return -EINVAL;
+	sp_data->otp_temp0 = FIELD_PREP(SP_TCODE_LOW_MASK, otp_v[0]) |
+			     FIELD_PREP(SP_TCODE_HIGH_MASK, otp_v[1]);
+	if (sp_data->otp_temp0 == 0)
+		sp_data->otp_temp0 = TEMP_OTP_BASE;
+	return 0;
+}
+
+/*
+ *When remanufacturing, the 35 degree T_CODE will be read and stored in nvcell.
+ *TEMP_RATE is the SP7021 temperature slope.
+ *T_CODE : 11 digits in total
+ */
+
+static int sp_thermal_get_sensor_temp(void *data, int *temp)
+{
+	struct sp_thermal_data *sp_data = data;
+	int t_code;
+
+	t_code = readl(sp_data->regs + SP_THERMAL_STS0_REG);
+	t_code = FIELD_GET(SP_THERMAL_MASK, t_code);
+	*temp = ((sp_data->otp_temp0 - t_code) * 10000 / TEMP_RATE) + TEMP_BASE;
+	*temp *= 10;
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops sp_of_thermal_ops = {
+	.get_temp = sp_thermal_get_sensor_temp,
+};
+
+static int sp_thermal_register_sensor(struct platform_device *pdev,
+				      struct sp_thermal_data *data, int index)
+{
+	data->id = index;
+	data->pcb_tz = devm_thermal_zone_of_sensor_register(&pdev->dev,
+							    data->id,
+							    data, &sp_of_thermal_ops);
+	if (IS_ERR_OR_NULL(data->pcb_tz))
+		return PTR_ERR(data->pcb_tz);
+	return 0;
+}
+
+static int sp7021_thermal_probe(struct platform_device *pdev)
+{
+	struct sp_thermal_data *sp_data;
+	struct resource *res;
+	int ret;
+
+	sp_data = devm_kzalloc(&pdev->dev, sizeof(*sp_data), GFP_KERNEL);
+	if (!sp_data)
+		return -ENOMEM;
+
+	sp_data->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sp_data->regs)) {
+		dev_err(&pdev->dev, "resource get fail\n");
+		return PTR_ERR(sp_data->regs);
+	}
+
+	writel(ENABLE_THERMAL, sp_data->regs + SP_THERMAL_CTL0_REG);
+
+	platform_set_drvdata(pdev, sp_data);
+	ret = sp7021_get_otp_temp_coef(sp_data, &pdev->dev);
+	if (ret)
+		return ret;
+	ret = sp_thermal_register_sensor(pdev, sp_data, 0);
+	return ret;
+}
+
+static const struct of_device_id of_sp7021_thermal_ids[] = {
+	{ .compatible = "sunplus,sp7021-thermal" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_sp7021_thermal_ids);
+
+static struct platform_driver sp7021_thermal_driver = {
+	.probe	= sp7021_thermal_probe,
+	.driver	= {
+		.name	= "sp7021-thermal",
+		.of_match_table = of_sp7021_thermal_ids,
+		},
+};
+module_platform_driver(sp7021_thermal_driver);
+
+MODULE_AUTHOR("Li-hao Kuo <lhjeff911@gmail.com>");
+MODULE_DESCRIPTION("Thermal driver for SP7021 SoC");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH v7 2/2] dt-bindings:thermal: Add Sunplus SP7021 schema

[Li-hao Kuo]

Add bindings for Sunplus SP7021 thermal driver

Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>
---
Changes in v7:
 - Modify yaml file.
 - Change the filename sunplus_thermal to sunplus-thermal.
 - Change the nvmem-cell-names thermal_calib to calib.

 .../bindings/thermal/sunplus-thermal.yaml          | 43 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml

diff --git a/Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml b/Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml
new file mode 100644
index 0000000..b9d69b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/sunplus-thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus Thermal controller
+
+maintainers:
+  - Li-hao Kuo <lhjeff911@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-thermal
+
+  reg:
+    maxItems: 1
+
+  nvmem-cells:
+    maxItems: 1
+
+  nvmem-cell-names:
+    const: calib
+
+required:
+  - compatible
+  - reg
+  - nvmem-cells
+  - nvmem-cell-names
+
+additionalProperties: false
+
+examples:
+  - |
+    thermal@9c000280 {
+        compatible = "sunplus,sp7021-thermal";
+        reg = <0x9c000280 0xc>;
+        nvmem-cells = <&calib>;
+        nvmem-cell-names = "calib";
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 307570c..0eb22a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18889,6 +18889,7 @@ SUNPLUS THERMAL DRIVER
 M:	Li-hao Kuo <lhjeff911@gmail.com>
 L:	linux-pm@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/thermal/sunplus_thermal.yaml
 F:	drivers/thermal/sunplus_thermal.c
 
 SUNPLUS UART DRIVER
-- 
2.7.4

Re: [PATCH v7 2/2] dt-bindings:thermal: Add Sunplus SP7021 schema

[Krzysztof Kozlowski]

On 11/04/2022 10:52, Li-hao Kuo wrote:
> Add bindings for Sunplus SP7021 thermal driver
> 
> Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>

Thank you for your patch. There is something to discuss/improve.

> ---
> Changes in v7:
>  - Modify yaml file.
>  - Change the filename sunplus_thermal to sunplus-thermal.
>  - Change the nvmem-cell-names thermal_calib to calib.
> 
>  .../bindings/thermal/sunplus-thermal.yaml          | 43 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/sunplus-thermal.yaml
> 

I commented with v5 [1] how the naming should look like. That's the
Devicetree convention.

You sent v6 without implementing the changes. I pointed out that you did
not follow what I asked for.
Now you sent v7 also without implementing these changes, again.

You also did not discuss it with me, did not come with counter
arguments, other proposals. Therefore it looks like either you
misunderstood me or you ignored my comments.

Let's assume first case, so I will repeat. Name should be one of:
1. sunplus,thermal.yaml
2. sunplus,sp7021-thermal.yaml

Not other names. No other characters, no undescores, no hyphens after
vendor name. If this is unclear, please respond instead of ignoring.

Without implementing the changes:
NAK

[1]
https://lore.kernel.org/linux-devicetree/fe67c7e7-957b-3abf-a929-5ee346657bcf@canonical.com/

Best regards,
Krzysztof

RE: [PATCH v7 2/2] dt-bindings:thermal: Add Sunplus SP7021 schema

[Lh Kuo 郭力豪]

Hi Mr. Krzysztof

I'm sorry for that. I misunderstood at the beginning

I will change the name to sunplus,thermal.yaml in the next commit.

Thanks for your comment.

Best regards,
Li Hao Kuo

> You sent v6 without implementing the changes. I pointed out that you did not follow what I asked for.
> Now you sent v7 also without implementing these changes, again.
> 
> You also did not discuss it with me, did not come with counter arguments, other proposals. Therefore it
> looks like either you misunderstood me or you ignored my comments.
> 
> Let's assume first case, so I will repeat. Name should be one of:
> 1. sunplus,thermal.yaml
> 2. sunplus,sp7021-thermal.yaml

Re: [PATCH v7 1/2] thermal: Add thermal driver for Sunplus SP7021

[Daniel Lezcano]

On 11/04/2022 10:52, Li-hao Kuo wrote:
> Add thermal driver for Sunplus SP7021.

Already asked previously : please give a more detailed description of 
the sensor

I've commented again this patch. There are some comments which are not 
taken into account from my previous review on v4


> Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>
> ---
> Changes in v7:
>   - Modify yaml file.
> 
>   MAINTAINERS                       |   6 ++
>   drivers/thermal/Kconfig           |  10 +++
>   drivers/thermal/Makefile          |   1 +
>   drivers/thermal/sunplus_thermal.c | 139 ++++++++++++++++++++++++++++++++++++++
>   4 files changed, 156 insertions(+)
>   create mode 100644 drivers/thermal/sunplus_thermal.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 61d9f11..307570c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18885,6 +18885,12 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
>   F:	drivers/spi/spi-sunplus-sp7021.c
>   
> +SUNPLUS THERMAL DRIVER
> +M:	Li-hao Kuo <lhjeff911@gmail.com>
> +L:	linux-pm@vger.kernel.org
> +S:	Maintained
> +F:	drivers/thermal/sunplus_thermal.c
> +
>   SUNPLUS UART DRIVER
>   M:	Hammer Hsieh <hammerh0314@gmail.com>
>   S:	Maintained
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e37691e..66316c3 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -502,4 +502,14 @@ config KHADAS_MCU_FAN_THERMAL
>   	  If you say yes here you get support for the FAN controlled
>   	  by the Microcontroller found on the Khadas VIM boards.
>   
> +config SUNPLUS_THERMAL
> +	tristate "Sunplus thermal drivers"
> +	depends on SOC_SP7021 || COMPILE_TEST
> +	help
> +	  This the Sunplus SP7021 thermal driver, which supports the primitive
> +	  temperature sensor embedded in Sunplus SP7021 SoC.
> +
> +	  If you have a Sunplus SP7021 platform say Y here and enable this option
> +	  to have support for thermal management
> +
>   endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index f0c36a1..38a76f9 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -61,3 +61,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>   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_SUNPLUS_THERMAL)	+= sunplus_thermal.o
> diff --git a/drivers/thermal/sunplus_thermal.c b/drivers/thermal/sunplus_thermal.c
> new file mode 100644
> index 0000000..9a9b348
> --- /dev/null
> +++ b/drivers/thermal/sunplus_thermal.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Sunplus Inc.
> + * Author: Li-hao Kuo <lhjeff911@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/thermal.h>

Already commented, check the headers above.

> +#define DISABLE_THERMAL		(BIT(31) | BIT(15))
> +#define ENABLE_THERMAL		BIT(31)
> +#define SP_THERMAL_MASK		GENMASK(10, 0)
> +#define SP_TCODE_HIGH_MASK	GENMASK(10, 8)
> +#define SP_TCODE_LOW_MASK	GENMASK(7, 0)
> +
> +#define TEMP_RATE		608
> +#define TEMP_BASE		3500
> +#define TEMP_OTP_BASE		1518
> +
> +#define SP_THERMAL_CTL0_REG	0x0000
> +#define SP_THERMAL_STS0_REG	0x0030
> +
> +/* common data structures */
> +struct sp_thermal_data {
> +	struct thermal_zone_device *pcb_tz;

field not used outside of the function checking the return code of 
sensor register

> +	struct platform_device *pdev;

field not used

> +	enum thermal_device_mode mode;

field not used

> +	void __iomem *regs;
> +	int otp_temp0;
> +	u32 id;
> +};
> +
> +static int sp7021_get_otp_temp_coef(struct sp_thermal_data *sp_data, struct device *dev)
> +{
> +	struct nvmem_cell *cell;
> +	ssize_t otp_l;
> +	char *otp_v;
> +
> +	cell = nvmem_cell_get(dev, "calib");
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	otp_v = nvmem_cell_read(cell, &otp_l);
> +	nvmem_cell_put(cell);

See my previous comments in v4, this is wrong. Move the nvmem_cell_put() 
after FIELD_PREP() ... below where otp_v is no longer used.

> +	if (otp_l < 3)
> +		return -EINVAL;

Please replace '3' by a macro

Why this check is needed by the way ? Sounds like only 0 and 1 indexes 
are used here.

> +	sp_data->otp_temp0 = FIELD_PREP(SP_TCODE_LOW_MASK, otp_v[0]) |
> +			     FIELD_PREP(SP_TCODE_HIGH_MASK, otp_v[1]);
> +	if (sp_data->otp_temp0 == 0)
> +		sp_data->otp_temp0 = TEMP_OTP_BASE;
> +	return 0;
> +}
> +
> +/*
> + *When remanufacturing, the 35 degree T_CODE will be read and stored in nvcell.
> + *TEMP_RATE is the SP7021 temperature slope.
> + *T_CODE : 11 digits in total
> + */

nit: space after '*'

Please elaborate the comment, it is still unclear

> +
> +static int sp_thermal_get_sensor_temp(void *data, int *temp)
> +{
> +	struct sp_thermal_data *sp_data = data;
> +	int t_code;
> +
> +	t_code = readl(sp_data->regs + SP_THERMAL_STS0_REG);
> +	t_code = FIELD_GET(SP_THERMAL_MASK, t_code);
> +	*temp = ((sp_data->otp_temp0 - t_code) * 10000 / TEMP_RATE) + TEMP_BASE;
> +	*temp *= 10;
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops sp_of_thermal_ops = {
> +	.get_temp = sp_thermal_get_sensor_temp,
> +};
> +
> +static int sp_thermal_register_sensor(struct platform_device *pdev,
> +				      struct sp_thermal_data *data, int index)

Adding a function to wrap another function is pointless in this case. It 
is simpler to directly call devm_thermal_zone_of_sensor_register() from 
the probe function

> +{
> +	data->id = index;
> +	data->pcb_tz = devm_thermal_zone_of_sensor_register(&pdev->dev,
> +							    data->id,
> +							    data, &sp_of_thermal_ops);
> +	if (IS_ERR_OR_NULL(data->pcb_tz))
> +		return PTR_ERR(data->pcb_tz);
> +	return 0;
> +}
> +
> +static int sp7021_thermal_probe(struct platform_device *pdev)
> +{
> +	struct sp_thermal_data *sp_data;
> +	struct resource *res;
> +	int ret;
> +
> +	sp_data = devm_kzalloc(&pdev->dev, sizeof(*sp_data), GFP_KERNEL);
> +	if (!sp_data)
> +		return -ENOMEM;
> +
> +	sp_data->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sp_data->regs)) {
> +		dev_err(&pdev->dev, "resource get fail\n");
> +		return PTR_ERR(sp_data->regs);
> +	}
> +
> +	writel(ENABLE_THERMAL, sp_data->regs + SP_THERMAL_CTL0_REG);
> +
> +	platform_set_drvdata(pdev, sp_data);
> +	ret = sp7021_get_otp_temp_coef(sp_data, &pdev->dev);
> +	if (ret)
> +		return ret;

Add some space to let the code easier to read

> +	ret = sp_thermal_register_sensor(pdev, sp_data, 0);
> +	return ret;
> +}
> +
> +static const struct of_device_id of_sp7021_thermal_ids[] = {
> +	{ .compatible = "sunplus,sp7021-thermal" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_sp7021_thermal_ids);
> +
> +static struct platform_driver sp7021_thermal_driver = {
> +	.probe	= sp7021_thermal_probe,

and .remove ?

> +	.driver	= {
> +		.name	= "sp7021-thermal",
> +		.of_match_table = of_sp7021_thermal_ids,
> +		},
> +};
> +module_platform_driver(sp7021_thermal_driver);
> +
> +MODULE_AUTHOR("Li-hao Kuo <lhjeff911@gmail.com>");
> +MODULE_DESCRIPTION("Thermal driver for SP7021 SoC");
> +MODULE_LICENSE("GPL");


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v7 1/2] thermal: Add thermal driver for Sunplus SP7021

[郭力豪]

Hi

>> +
>> +     cell = nvmem_cell_get(dev, "calib");
>> +     if (IS_ERR(cell))
>> +             return PTR_ERR(cell);
>> +
>> +     otp_v = nvmem_cell_read(cell, &otp_l);
>> +     nvmem_cell_put(cell);

>See my previous comments in v4, this is wrong. Move the nvmem_cell_put()
>after FIELD_PREP() ... below where otp_v is no longer used.

> +     if (otp_l < 3)
> +             return -EINVAL;

>Please replace '3' by a macro

>Why this check is needed by the way ? Sounds like only 0 and 1 indexes
>are used here.

you mean the following?

 sp_data->otp_temp0 = nvmem_cell_read(cell, &otp_l);
nvmem_cell_put(cell);

> +static const struct of_device_id of_sp7021_thermal_ids[] = {
> +     { .compatible = "sunplus,sp7021-thermal" },
> +     { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_sp7021_thermal_ids);
> +
> +static struct platform_driver sp7021_thermal_driver = {
> +     .probe  = sp7021_thermal_probe,

>and .remove ?

If the remove function just returns 0; must it still exist?

Li-hao Kuo <lhjeff911@gmail.com> 於 2022年4月11日 週一 下午4:52寫道：
>
> Add thermal driver for Sunplus SP7021.
>
> Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>
> ---
> Changes in v7:
>  - Modify yaml file.
>
>  MAINTAINERS                       |   6 ++
>  drivers/thermal/Kconfig           |  10 +++
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/sunplus_thermal.c | 139 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 156 insertions(+)
>  create mode 100644 drivers/thermal/sunplus_thermal.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 61d9f11..307570c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18885,6 +18885,12 @@ S:     Maintained
>  F:     Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
>  F:     drivers/spi/spi-sunplus-sp7021.c
>
> +SUNPLUS THERMAL DRIVER
> +M:     Li-hao Kuo <lhjeff911@gmail.com>
> +L:     linux-pm@vger.kernel.org
> +S:     Maintained
> +F:     drivers/thermal/sunplus_thermal.c
> +
>  SUNPLUS UART DRIVER
>  M:     Hammer Hsieh <hammerh0314@gmail.com>
>  S:     Maintained
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index e37691e..66316c3 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -502,4 +502,14 @@ config KHADAS_MCU_FAN_THERMAL
>           If you say yes here you get support for the FAN controlled
>           by the Microcontroller found on the Khadas VIM boards.
>
> +config SUNPLUS_THERMAL
> +       tristate "Sunplus thermal drivers"
> +       depends on SOC_SP7021 || COMPILE_TEST
> +       help
> +         This the Sunplus SP7021 thermal driver, which supports the primitive
> +         temperature sensor embedded in Sunplus SP7021 SoC.
> +
> +         If you have a Sunplus SP7021 platform say Y here and enable this option
> +         to have support for thermal management
> +
>  endif
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index f0c36a1..38a76f9 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -61,3 +61,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)        += uniphier_thermal.o
>  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_SUNPLUS_THERMAL)  += sunplus_thermal.o
> diff --git a/drivers/thermal/sunplus_thermal.c b/drivers/thermal/sunplus_thermal.c
> new file mode 100644
> index 0000000..9a9b348
> --- /dev/null
> +++ b/drivers/thermal/sunplus_thermal.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Sunplus Inc.
> + * Author: Li-hao Kuo <lhjeff911@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/thermal.h>
> +
> +#define DISABLE_THERMAL                (BIT(31) | BIT(15))
> +#define ENABLE_THERMAL         BIT(31)
> +#define SP_THERMAL_MASK                GENMASK(10, 0)
> +#define SP_TCODE_HIGH_MASK     GENMASK(10, 8)
> +#define SP_TCODE_LOW_MASK      GENMASK(7, 0)
> +
> +#define TEMP_RATE              608
> +#define TEMP_BASE              3500
> +#define TEMP_OTP_BASE          1518
> +
> +#define SP_THERMAL_CTL0_REG    0x0000
> +#define SP_THERMAL_STS0_REG    0x0030
> +
> +/* common data structures */
> +struct sp_thermal_data {
> +       struct thermal_zone_device *pcb_tz;
> +       struct platform_device *pdev;
> +       enum thermal_device_mode mode;
> +       void __iomem *regs;
> +       int otp_temp0;
> +       u32 id;
> +};
> +
> +static int sp7021_get_otp_temp_coef(struct sp_thermal_data *sp_data, struct device *dev)
> +{
> +       struct nvmem_cell *cell;
> +       ssize_t otp_l;
> +       char *otp_v;
> +
> +       cell = nvmem_cell_get(dev, "calib");
> +       if (IS_ERR(cell))
> +               return PTR_ERR(cell);
> +
> +       otp_v = nvmem_cell_read(cell, &otp_l);
> +       nvmem_cell_put(cell);
> +
> +       if (otp_l < 3)
> +               return -EINVAL;
> +       sp_data->otp_temp0 = FIELD_PREP(SP_TCODE_LOW_MASK, otp_v[0]) |
> +                            FIELD_PREP(SP_TCODE_HIGH_MASK, otp_v[1]);
> +       if (sp_data->otp_temp0 == 0)
> +               sp_data->otp_temp0 = TEMP_OTP_BASE;
> +       return 0;
> +}
> +
> +/*
> + *When remanufacturing, the 35 degree T_CODE will be read and stored in nvcell.
> + *TEMP_RATE is the SP7021 temperature slope.
> + *T_CODE : 11 digits in total
> + */
> +
> +static int sp_thermal_get_sensor_temp(void *data, int *temp)
> +{
> +       struct sp_thermal_data *sp_data = data;
> +       int t_code;
> +
> +       t_code = readl(sp_data->regs + SP_THERMAL_STS0_REG);
> +       t_code = FIELD_GET(SP_THERMAL_MASK, t_code);
> +       *temp = ((sp_data->otp_temp0 - t_code) * 10000 / TEMP_RATE) + TEMP_BASE;
> +       *temp *= 10;
> +       return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops sp_of_thermal_ops = {
> +       .get_temp = sp_thermal_get_sensor_temp,
> +};
> +
> +static int sp_thermal_register_sensor(struct platform_device *pdev,
> +                                     struct sp_thermal_data *data, int index)
> +{
> +       data->id = index;
> +       data->pcb_tz = devm_thermal_zone_of_sensor_register(&pdev->dev,
> +                                                           data->id,
> +                                                           data, &sp_of_thermal_ops);
> +       if (IS_ERR_OR_NULL(data->pcb_tz))
> +               return PTR_ERR(data->pcb_tz);
> +       return 0;
> +}
> +
> +static int sp7021_thermal_probe(struct platform_device *pdev)
> +{
> +       struct sp_thermal_data *sp_data;
> +       struct resource *res;
> +       int ret;
> +
> +       sp_data = devm_kzalloc(&pdev->dev, sizeof(*sp_data), GFP_KERNEL);
> +       if (!sp_data)
> +               return -ENOMEM;
> +
> +       sp_data->regs = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(sp_data->regs)) {
> +               dev_err(&pdev->dev, "resource get fail\n");
> +               return PTR_ERR(sp_data->regs);
> +       }
> +
> +       writel(ENABLE_THERMAL, sp_data->regs + SP_THERMAL_CTL0_REG);
> +
> +       platform_set_drvdata(pdev, sp_data);
> +       ret = sp7021_get_otp_temp_coef(sp_data, &pdev->dev);
> +       if (ret)
> +               return ret;
> +       ret = sp_thermal_register_sensor(pdev, sp_data, 0);
> +       return ret;
> +}
> +
> +static const struct of_device_id of_sp7021_thermal_ids[] = {
> +       { .compatible = "sunplus,sp7021-thermal" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_sp7021_thermal_ids);
> +
> +static struct platform_driver sp7021_thermal_driver = {
> +       .probe  = sp7021_thermal_probe,
> +       .driver = {
> +               .name   = "sp7021-thermal",
> +               .of_match_table = of_sp7021_thermal_ids,
> +               },
> +};
> +module_platform_driver(sp7021_thermal_driver);
> +
> +MODULE_AUTHOR("Li-hao Kuo <lhjeff911@gmail.com>");
> +MODULE_DESCRIPTION("Thermal driver for SP7021 SoC");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>