[PATCH V12 1/5] dt-bindings: fsl: scu: add thermal binding

[PATCH V12 1/5] dt-bindings: fsl: scu: add thermal binding

[Anson Huang]

NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and thermal sensors etc. management, Linux kernel
has to communicate with system controller via MU (message unit)
IPC to get temperature from thermal sensors, this patch adds
binding doc for i.MX system controller thermal driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 5d7dbab..f4fb6d5 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -133,6 +133,17 @@ RTC bindings based on SCU Message Protocol
 Required properties:
 - compatible: should be "fsl,imx8qxp-sc-rtc";
 
+Thermal bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible:			Should be :
+				  "fsl,imx8qxp-sc-thermal"
+				followed by "fsl,imx-sc-thermal";
+
+- #thermal-sensor-cells:	See Documentation/devicetree/bindings/thermal/thermal.txt
+				for a description.
+
 Example (imx8qxp):
 -------------
 aliases {
@@ -185,6 +196,11 @@ firmware {
 		rtc: rtc {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
+
+		tsens: thermal-sensor {
+			compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
+			#thermal-sensor-cells = <1>;
+		};
 	};
 };
 
-- 
2.7.4

[PATCH V12 2/5] thermal: of-thermal: add API for getting sensor ID from DT

[Anson Huang]

On some platforms like i.MX8QXP, the thermal driver needs a
real HW sensor ID from DT thermal zone, the HW sensor ID is
used to get temperature from SCU firmware, and the virtual
sensor ID starting from 0 to N is NOT used at all, this patch
adds new API thermal_zone_of_get_sensor_id() to provide the
feature of getting sensor ID from DT thermal zone's node.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
New patch.
---
 drivers/thermal/of-thermal.c | 53 ++++++++++++++++++++++++++++++++++----------
 include/linux/thermal.h      | 10 +++++++++
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 2df059c..2e4320c 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -446,6 +446,46 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 }
 
 /**
+ * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
+ * @tz_np: a valid thermal zone device node.
+ * @sensor_specs: pointer to output arguments structure will be passed back.
+ * @id: a sensor ID pointer will be passed back.
+ *
+ * This function will get sensor ID from a given thermal zone node, use
+ * "thermal-sensors" as list name, and get sensor ID from first phandle's
+ * argument.
+ *
+ * Return: 0 on success, proper error code otherwise.
+ */
+
+int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+				  struct of_phandle_args *sensor_specs,
+				  u32 *id)
+{
+	int ret;
+
+	ret = of_parse_phandle_with_args(tz_np,
+					 "thermal-sensors",
+					 "#thermal-sensor-cells",
+					 0,
+					 sensor_specs);
+	if (ret)
+		return ret;
+
+	if (sensor_specs->args_count >= 1) {
+		*id = sensor_specs->args[0];
+		WARN(sensor_specs->args_count > 1,
+		     "%pOFn: too many cells in sensor specifier %d\n",
+		     sensor_specs->np, sensor_specs->args_count);
+	} else {
+		*id = 0;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
+
+/**
  * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
  * @dev: a valid struct device pointer of a sensor device. Must contain
  *       a valid .of_node, for the sensor node.
@@ -500,21 +540,10 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 		int ret, id;
 
 		/* For now, thermal framework supports only 1 sensor per zone */
-		ret = of_parse_phandle_with_args(child, "thermal-sensors",
-						 "#thermal-sensor-cells",
-						 0, &sensor_specs);
+		ret = thermal_zone_of_get_sensor_id(child, &sensor_specs, &id);
 		if (ret)
 			continue;
 
-		if (sensor_specs.args_count >= 1) {
-			id = sensor_specs.args[0];
-			WARN(sensor_specs.args_count > 1,
-			     "%pOFn: too many cells in sensor specifier %d\n",
-			     sensor_specs.np, sensor_specs.args_count);
-		} else {
-			id = 0;
-		}
-
 		if (sensor_specs.np == sensor_np && id == sensor_id) {
 			tzd = thermal_zone_of_add_sensor(child, sensor_np,
 							 data, ops);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 5f4705f..970fb37 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -375,6 +375,9 @@ struct thermal_trip {
 
 /* Function declarations */
 #ifdef CONFIG_THERMAL_OF
+int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+				  struct of_phandle_args *sensor_specs,
+				  u32 *id);
 struct thermal_zone_device *
 thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
 				const struct thermal_zone_of_device_ops *ops);
@@ -386,6 +389,13 @@ struct thermal_zone_device *devm_thermal_zone_of_sensor_register(
 void devm_thermal_zone_of_sensor_unregister(struct device *dev,
 					    struct thermal_zone_device *tz);
 #else
+
+static int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+					 struct of_phandle_args *sensor_specs,
+					 u32 *id)
+{
+	return -ENOENT;
+}
 static inline struct thermal_zone_device *
 thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
 				const struct thermal_zone_of_device_ops *ops)
-- 
2.7.4

[PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Anson Huang]

i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
inside, the system controller is in charge of controlling power,
clock and thermal sensors etc..

This patch adds i.MX system controller thermal driver support,
Linux kernel has to communicate with system controller via MU
(message unit) IPC to get each thermal sensor's temperature,
it supports multiple sensors which are passed from device tree,
please see the binding doc for details.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V11:
	- move the API of getting thermal zone sensor ID to of-thermal.c as generic API;
	- remove unnecessary __packed.
---
 drivers/thermal/Kconfig          |  11 ++++
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/imx_sc_thermal.c | 137 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/thermal/imx_sc_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 653aa27..4e4fa7e 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -223,6 +223,17 @@ config IMX_THERMAL
 	  cpufreq is used as the cooling device to throttle CPUs when the
 	  passive trip is crossed.
 
+config IMX_SC_THERMAL
+	tristate "Temperature sensor driver for NXP i.MX SoCs with System Controller"
+	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST
+	depends on OF
+	help
+	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX SoCs with
+	  system controller inside, Linux kernel has to communicate with system
+	  controller via MU (message unit) IPC to get temperature from thermal
+	  sensor. It supports one critical trip point and one
+	  passive trip point for each thermal sensor.
+
 config MAX77620_THERMAL
 	tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
 	depends on MFD_MAX77620
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 486d682..4062627 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_DB8500_THERMAL)	+= db8500_thermal.o
 obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
 obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
 obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
+obj-$(CONFIG_IMX_SC_THERMAL)	+= imx_sc_thermal.o
 obj-$(CONFIG_MAX77620_THERMAL)	+= max77620_thermal.o
 obj-$(CONFIG_QORIQ_THERMAL)	+= qoriq_thermal.o
 obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
diff --git a/drivers/thermal/imx_sc_thermal.c b/drivers/thermal/imx_sc_thermal.c
new file mode 100644
index 0000000..dcf16fc
--- /dev/null
+++ b/drivers/thermal/imx_sc_thermal.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018-2019 NXP.
+ */
+
+#include <linux/err.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define IMX_SC_MISC_FUNC_GET_TEMP	13
+#define IMX_SC_C_TEMP			0
+
+static struct imx_sc_ipc *thermal_ipc_handle;
+
+struct imx_sc_sensor {
+	struct thermal_zone_device *tzd;
+	u32 resource_id;
+};
+
+struct req_get_temp {
+	u16 resource_id;
+	u8 type;
+} __packed;
+
+struct resp_get_temp {
+	u16 celsius;
+	u8 tenths;
+} __packed;
+
+struct imx_sc_msg_misc_get_temp {
+	struct imx_sc_rpc_msg hdr;
+	union {
+		struct req_get_temp req;
+		struct resp_get_temp resp;
+	} data;
+};
+
+static int imx_sc_thermal_get_temp(void *data, int *temp)
+{
+	struct imx_sc_msg_misc_get_temp msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	struct imx_sc_sensor *sensor = data;
+	int ret;
+
+	msg.data.req.resource_id = sensor->resource_id;
+	msg.data.req.type = IMX_SC_C_TEMP;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_MISC;
+	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
+	hdr->size = 2;
+
+	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
+	if (ret) {
+		pr_err("read temp sensor %d failed, ret %d\n",
+			sensor->resource_id, ret);
+		return ret;
+	}
+
+	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
+	.get_temp = imx_sc_thermal_get_temp,
+};
+
+static int imx_sc_thermal_probe(struct platform_device *pdev)
+{
+	struct device_node *np, *child;
+	int ret;
+
+	ret = imx_scu_get_handle(&thermal_ipc_handle);
+	if (ret)
+		return ret;
+
+	np = of_find_node_by_name(NULL, "thermal-zones");
+	if (!np)
+		return -ENODEV;
+
+	for_each_available_child_of_node(np, child) {
+		struct of_phandle_args sensor_specs;
+		struct imx_sc_sensor *sensor =
+			devm_kzalloc(&pdev->dev, sizeof(*sensor), GFP_KERNEL);
+		if (!sensor)
+			return -ENOMEM;
+
+		ret = thermal_zone_of_get_sensor_id(child,
+						    &sensor_specs,
+						    &sensor->resource_id);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"failed to get valid sensor resource id: %d\n",
+				ret);
+			break;
+		}
+
+		sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
+								   sensor->resource_id,
+								   sensor,
+								   &imx_sc_thermal_ops);
+		if (IS_ERR(sensor->tzd)) {
+			dev_err(&pdev->dev, "failed to register thermal zone\n");
+			ret = PTR_ERR(sensor->tzd);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static const struct of_device_id imx_sc_thermal_table[] = {
+	{ .compatible = "fsl,imx-sc-thermal", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
+
+static struct platform_driver imx_sc_thermal_driver = {
+		.probe = imx_sc_thermal_probe,
+		.driver = {
+			.name = "imx-sc-thermal",
+			.of_match_table = imx_sc_thermal_table,
+		},
+};
+module_platform_driver(imx_sc_thermal_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system controller");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH V12 4/5] defconfig: arm64: add i.MX system controller thermal support

[Anson Huang]

This patch enables CONFIG_IMX_SC_THERMAL as module.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 17daa97..fbe3be7 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -425,6 +425,7 @@ CONFIG_SENSORS_INA2XX=m
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
+CONFIG_IMX_SC_THERMAL=m
 CONFIG_ROCKCHIP_THERMAL=m
 CONFIG_RCAR_THERMAL=y
 CONFIG_RCAR_GEN3_THERMAL=y
-- 
2.7.4

[PATCH V12 5/5] arm64: dts: imx: add i.MX8QXP thermal support

[Anson Huang]

Add i.MX8QXP CPU thermal zone support.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V11:
	- add cooling device for all secondary CPUs.
---
 arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 0683ee2..5eeaddc 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/pads-imx8qxp.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	interrupt-parent = <&gic>;
@@ -149,6 +150,11 @@
 		rtc: rtc {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
+
+		tsens: thermal-sensor {
+			compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
+			#thermal-sensor-cells = <1>;
+		};
 	};
 
 	timer {
@@ -522,4 +528,35 @@
 		compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
 		timeout-sec = <60>;
 	};
+
+	thermal_zones: thermal-zones {
+		cpu-thermal0 {
+			polling-delay-passive = <250>;
+			polling-delay = <2000>;
+			thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
+			trips {
+				cpu_alert0: trip0 {
+					temperature = <107000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				cpu_crit0: trip1 {
+					temperature = <127000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+			cooling-maps {
+				map0 {
+					trip = <&cpu_alert0>;
+					cooling-device =
+						<&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&A35_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&A35_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+
+				};
+			};
+		};
+	};
 };
-- 
2.7.4

Re: [PATCH V12 5/5] arm64: dts: imx: add i.MX8QXP thermal support

[Daniel Lezcano]

On 16/04/2019 05:22, Anson Huang wrote:
> Add i.MX8QXP CPU thermal zone support.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
> Changes since V11:
> 	- add cooling device for all secondary CPUs.
> ---
>  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 ++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> index 0683ee2..5eeaddc 100644
> --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/pinctrl/pads-imx8qxp.h>
> +#include <dt-bindings/thermal/thermal.h>
>  
>  / {
>  	interrupt-parent = <&gic>;
> @@ -149,6 +150,11 @@
>  		rtc: rtc {
>  			compatible = "fsl,imx8qxp-sc-rtc";
>  		};
> +
> +		tsens: thermal-sensor {
> +			compatible = "fsl,imx8qxp-sc-thermal", "fsl,imx-sc-thermal";
> +			#thermal-sensor-cells = <1>;
> +		};
>  	};
>  
>  	timer {
> @@ -522,4 +528,35 @@
>  		compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
>  		timeout-sec = <60>;
>  	};
> +
> +	thermal_zones: thermal-zones {
> +		cpu-thermal0 {
> +			polling-delay-passive = <250>;
> +			polling-delay = <2000>;
> +			thermal-sensors = <&tsens IMX_SC_R_SYSTEM>;
> +			trips {
> +				cpu_alert0: trip0 {
> +					temperature = <107000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +				cpu_crit0: trip1 {
> +					temperature = <127000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu_alert0>;
> +					cooling-device =
> +						<&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						<&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						<&A35_2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						<&A35_3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +
> +				};
> +			};
> +		};
> +	};
>  };
> 


-- 
 <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 V12 1/5] dt-bindings: fsl: scu: add thermal binding

[Aisheng Dong]

> From: Anson Huang
> Sent: Tuesday, April 16, 2019 11:22 AM
> 
> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> controller, the system controller is in charge of system power, clock and
> thermal sensors etc. management, Linux kernel has to communicate with
> system controller via MU (message unit) IPC to get temperature from thermal
> sensors, this patch adds binding doc for i.MX system controller thermal driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> No changes.
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt        | 16
> ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> index 5d7dbab..f4fb6d5 100644
> --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> @@ -133,6 +133,17 @@ RTC bindings based on SCU Message Protocol
> Required properties:
>  - compatible: should be "fsl,imx8qxp-sc-rtc";
> 
> +Thermal bindings based on SCU Message Protocol
> +------------------------------------------------------------
> +
> +Required properties:
> +- compatible:			Should be :
> +				  "fsl,imx8qxp-sc-thermal"
> +				followed by "fsl,imx-sc-thermal";
> +
> +- #thermal-sensor-cells:	See
> Documentation/devicetree/bindings/thermal/thermal.txt
> +				for a description.

Better to have an explicit value here.
e.g.
Must be 1. See xxx for a description.

Otherwise:
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Dong Aisheng

RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Aisheng Dong]

> From: Anson Huang
> Sent: Tuesday, April 16, 2019 11:22 AM
> 
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller inside,
> the system controller is in charge of controlling power, clock and thermal
> sensors etc..
> 
> This patch adds i.MX system controller thermal driver support, Linux kernel has
> to communicate with system controller via MU (message unit) IPC to get each
> thermal sensor's temperature, it supports multiple sensors which are passed
> from device tree, please see the binding doc for details.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V11:
> 	- move the API of getting thermal zone sensor ID to of-thermal.c as
> generic API;
> 	- remove unnecessary __packed.
> ---
>  drivers/thermal/Kconfig          |  11 ++++
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/imx_sc_thermal.c | 137
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/thermal/imx_sc_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> 653aa27..4e4fa7e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -223,6 +223,17 @@ config IMX_THERMAL
>  	  cpufreq is used as the cooling device to throttle CPUs when the
>  	  passive trip is crossed.
> 
> +config IMX_SC_THERMAL
> +	tristate "Temperature sensor driver for NXP i.MX SoCs with System
> Controller"
> +	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST

COMPILE_TEST seems won't work and may cause build issue due to
the dependency of IMX_SCU.

> +	depends on OF
> +	help
> +	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX SoCs
> with
> +	  system controller inside, Linux kernel has to communicate with system
> +	  controller via MU (message unit) IPC to get temperature from thermal
> +	  sensor. It supports one critical trip point and one
> +	  passive trip point for each thermal sensor.
> +

[...]

> +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> +	struct imx_sc_msg_misc_get_temp msg;
> +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +	struct imx_sc_sensor *sensor = data;
> +	int ret;
> +
> +	msg.data.req.resource_id = sensor->resource_id;
> +	msg.data.req.type = IMX_SC_C_TEMP;

IMX_SC_C_TEMP duplicates with enum imx_sc_ctrl we already have.
include/linux/firmware/imx/types.h

But I guess you're not want to use a CTRL here.
So please fix the name convention.

> +
> +	hdr->ver = IMX_SC_RPC_VERSION;
> +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> +	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> +	hdr->size = 2;
> +
> +	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
> +	if (ret) {
> +		pr_err("read temp sensor %d failed, ret %d\n",
> +			sensor->resource_id, ret);

The error message is meaningless.
Can't convert to dev_err?

> +		return ret;
> +	}
> +
> +	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
> +	.get_temp = imx_sc_thermal_get_temp,
> +};
> +
> +static int imx_sc_thermal_probe(struct platform_device *pdev) {
> +	struct device_node *np, *child;
> +	int ret;
> +
> +	ret = imx_scu_get_handle(&thermal_ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	np = of_find_node_by_name(NULL, "thermal-zones");
> +	if (!np)
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct of_phandle_args sensor_specs;
> +		struct imx_sc_sensor *sensor =
> +			devm_kzalloc(&pdev->dev, sizeof(*sensor), GFP_KERNEL);

Usually we do not mix the complicated code in the declare line.
You can move the declarations into the top level to make code more clean.

> +		if (!sensor)
> +			return -ENOMEM;
> +
> +		ret = thermal_zone_of_get_sensor_id(child,
> +						    &sensor_specs,

It looks a bit strange why need sensor_specs as you even did not use it.

Regards
Dong Aisheng

> +						    &sensor->resource_id);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to get valid sensor resource id: %d\n",
> +				ret);
> +			break;
> +		}
> +
> +		sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
> +								   sensor->resource_id,
> +								   sensor,
> +								   &imx_sc_thermal_ops);
> +		if (IS_ERR(sensor->tzd)) {
> +			dev_err(&pdev->dev, "failed to register thermal zone\n");
> +			ret = PTR_ERR(sensor->tzd);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id imx_sc_thermal_table[] = {
> +	{ .compatible = "fsl,imx-sc-thermal", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> +
> +static struct platform_driver imx_sc_thermal_driver = {
> +		.probe = imx_sc_thermal_probe,
> +		.driver = {
> +			.name = "imx-sc-thermal",
> +			.of_match_table = imx_sc_thermal_table,
> +		},
> +};
> +module_platform_driver(imx_sc_thermal_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> +controller"); MODULE_LICENSE("GPL v2");
> --
> 2.7.4

RE: [PATCH V12 1/5] dt-bindings: fsl: scu: add thermal binding

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Aisheng Dong
> Sent: Wednesday, April 17, 2019 11:32 AM
> To: Anson Huang <anson.huang@nxp.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; catalin.marinas@arm.com;
> will.deacon@arm.com; rui.zhang@intel.com; edubezval@gmail.com;
> daniel.lezcano@linaro.org; ulf.hansson@linaro.org; Daniel Baluta
> <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> heiko@sntech.de; horms+renesas@verge.net.au; agross@kernel.org;
> olof@lixom.net; bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pm@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH V12 1/5] dt-bindings: fsl: scu: add thermal binding
> 
> > From: Anson Huang
> > Sent: Tuesday, April 16, 2019 11:22 AM
> >
> > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > controller, the system controller is in charge of system power, clock
> > and thermal sensors etc. management, Linux kernel has to communicate
> > with system controller via MU (message unit) IPC to get temperature
> > from thermal sensors, this patch adds binding doc for i.MX system
> controller thermal driver.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > No changes.
> > ---
> >  .../devicetree/bindings/arm/freescale/fsl,scu.txt        | 16
> > ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 5d7dbab..f4fb6d5 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -133,6 +133,17 @@ RTC bindings based on SCU Message Protocol
> > Required properties:
> >  - compatible: should be "fsl,imx8qxp-sc-rtc";
> >
> > +Thermal bindings based on SCU Message Protocol
> > +------------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible:			Should be :
> > +				  "fsl,imx8qxp-sc-thermal"
> > +				followed by "fsl,imx-sc-thermal";
> > +
> > +- #thermal-sensor-cells:	See
> > Documentation/devicetree/bindings/thermal/thermal.txt
> > +				for a description.
> 
> Better to have an explicit value here.
> e.g.
> Must be 1. See xxx for a description.

OK, will add it in next version.

Anson.

> 
> Otherwise:
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Regards
> Dong Aisheng

RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> > From: Anson Huang
> > Sent: Tuesday, April 16, 2019 11:22 AM
> >
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and thermal sensors etc..
> >
> > This patch adds i.MX system controller thermal driver support, Linux
> > kernel has to communicate with system controller via MU (message unit)
> > IPC to get each thermal sensor's temperature, it supports multiple
> > sensors which are passed from device tree, please see the binding doc for
> details.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V11:
> > 	- move the API of getting thermal zone sensor ID to of-thermal.c as
> > generic API;
> > 	- remove unnecessary __packed.
> > ---
> >  drivers/thermal/Kconfig          |  11 ++++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/imx_sc_thermal.c | 137
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 149 insertions(+)
> >  create mode 100644 drivers/thermal/imx_sc_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 653aa27..4e4fa7e 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -223,6 +223,17 @@ config IMX_THERMAL
> >  	  cpufreq is used as the cooling device to throttle CPUs when the
> >  	  passive trip is crossed.
> >
> > +config IMX_SC_THERMAL
> > +	tristate "Temperature sensor driver for NXP i.MX SoCs with System
> > Controller"
> > +	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST
> 
> COMPILE_TEST seems won't work and may cause build issue due to the
> dependency of IMX_SCU.

I forgot this one, will fix it in next version.

> 
> > +	depends on OF
> > +	help
> > +	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX
> SoCs
> > with
> > +	  system controller inside, Linux kernel has to communicate with
> system
> > +	  controller via MU (message unit) IPC to get temperature from
> thermal
> > +	  sensor. It supports one critical trip point and one
> > +	  passive trip point for each thermal sensor.
> > +
> 
> [...]
> 
> > +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> > +	struct imx_sc_msg_misc_get_temp msg;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	struct imx_sc_sensor *sensor = data;
> > +	int ret;
> > +
> > +	msg.data.req.resource_id = sensor->resource_id;
> > +	msg.data.req.type = IMX_SC_C_TEMP;
> 
> IMX_SC_C_TEMP duplicates with enum imx_sc_ctrl we already have.
> include/linux/firmware/imx/types.h
> 
> But I guess you're not want to use a CTRL here.
> So please fix the name convention.

Using the definition in types.h is correct, I will remove local definition.

> 
> > +
> > +	hdr->ver = IMX_SC_RPC_VERSION;
> > +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> > +	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> > +	hdr->size = 2;
> > +
> > +	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
> > +	if (ret) {
> > +		pr_err("read temp sensor %d failed, ret %d\n",
> > +			sensor->resource_id, ret);
> 
> The error message is meaningless.
> Can't convert to dev_err?

The error message is just to indicate whether the SCU call success or NOT, I can covert
to dev_err using below change:

dev_err(&sensor->tzd->device, ......);

> 
> > +		return ret;
> > +	}
> > +
> > +	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
> > +	.get_temp = imx_sc_thermal_get_temp, };
> > +
> > +static int imx_sc_thermal_probe(struct platform_device *pdev) {
> > +	struct device_node *np, *child;
> > +	int ret;
> > +
> > +	ret = imx_scu_get_handle(&thermal_ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	np = of_find_node_by_name(NULL, "thermal-zones");
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	for_each_available_child_of_node(np, child) {
> > +		struct of_phandle_args sensor_specs;
> > +		struct imx_sc_sensor *sensor =
> > +			devm_kzalloc(&pdev->dev, sizeof(*sensor),
> GFP_KERNEL);
> 
> Usually we do not mix the complicated code in the declare line.
> You can move the declarations into the top level to make code more clean.

OK. Maybe because I refer to the code drivers/thermal/of-thermal.c:

thermal_zone_of_sensor_register(),

I can move it to the beginning of function.

> 
> > +		if (!sensor)
> > +			return -ENOMEM;
> > +
> > +		ret = thermal_zone_of_get_sensor_id(child,
> > +						    &sensor_specs,
> 
> It looks a bit strange why need sensor_specs as you even did not use it.

It is because parsing the phandle arguments needs it, such as the thermal_zone_of_sensor_register()
function needs this pointer to be passed back to check the elements are valid or NOT, so the API does
NOT define it as local structure. The API NOT just return the sensor id, but also the of_phandle_args address,
although we do NOT need it at all here.

Thanks.
Anson. 

> 
> Regards
> Dong Aisheng
> 
> > +						    &sensor->resource_id);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev,
> > +				"failed to get valid sensor resource id: %d\n",
> > +				ret);
> > +			break;
> > +		}
> > +
> > +		sensor->tzd =
> devm_thermal_zone_of_sensor_register(&pdev->dev,
> > +								   sensor-
> >resource_id,
> > +								   sensor,
> > +
> &imx_sc_thermal_ops);
> > +		if (IS_ERR(sensor->tzd)) {
> > +			dev_err(&pdev->dev, "failed to register thermal
> zone\n");
> > +			ret = PTR_ERR(sensor->tzd);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id imx_sc_thermal_table[] = {
> > +	{ .compatible = "fsl,imx-sc-thermal", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> > +
> > +static struct platform_driver imx_sc_thermal_driver = {
> > +		.probe = imx_sc_thermal_probe,
> > +		.driver = {
> > +			.name = "imx-sc-thermal",
> > +			.of_match_table = imx_sc_thermal_table,
> > +		},
> > +};
> > +module_platform_driver(imx_sc_thermal_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> > +controller"); MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4

Re: [PATCH V12 1/5] dt-bindings: fsl: scu: add thermal binding

[Rob Herring]

On Tue, 16 Apr 2019 03:22:09 +0000, Anson Huang wrote:
> NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> system controller, the system controller is in charge of system
> power, clock and thermal sensors etc. management, Linux kernel
> has to communicate with system controller via MU (message unit)
> IPC to get temperature from thermal sensors, this patch adds
> binding doc for i.MX system controller thermal driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> No changes.
> ---
>  .../devicetree/bindings/arm/freescale/fsl,scu.txt        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Re: [PATCH V12 5/5] arm64: dts: imx: add i.MX8QXP thermal support

[Rob Herring]

On Tue, 16 Apr 2019 03:22:41 +0000, Anson Huang wrote:
> Add i.MX8QXP CPU thermal zone support.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V11:
> 	- add cooling device for all secondary CPUs.
> ---
>  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 37 ++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Anson Huang]

Ping...

> -----Original Message-----
> From: Anson Huang [mailto:Anson.Huang@nxp.com]
> Sent: Tuesday, April 16, 2019 11:22 AM
> To: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> catalin.marinas@arm.com; will.deacon@arm.com; rui.zhang@intel.com;
> edubezval@gmail.com; daniel.lezcano@linaro.org; Aisheng Dong
> <aisheng.dong@nxp.com>; ulf.hansson@linaro.org; Daniel Baluta
> <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> heiko@sntech.de; horms+renesas@verge.net.au; agross@kernel.org;
> olof@lixom.net; bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pm@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller
> thermal support
> 
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller inside,
> the system controller is in charge of controlling power, clock and thermal
> sensors etc..
> 
> This patch adds i.MX system controller thermal driver support, Linux kernel
> has to communicate with system controller via MU (message unit) IPC to get
> each thermal sensor's temperature, it supports multiple sensors which are
> passed from device tree, please see the binding doc for details.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V11:
> 	- move the API of getting thermal zone sensor ID to of-thermal.c as
> generic API;
> 	- remove unnecessary __packed.
> ---
>  drivers/thermal/Kconfig          |  11 ++++
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/imx_sc_thermal.c | 137
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/thermal/imx_sc_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> 653aa27..4e4fa7e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -223,6 +223,17 @@ config IMX_THERMAL
>  	  cpufreq is used as the cooling device to throttle CPUs when the
>  	  passive trip is crossed.
> 
> +config IMX_SC_THERMAL
> +	tristate "Temperature sensor driver for NXP i.MX SoCs with System
> Controller"
> +	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX
> SoCs with
> +	  system controller inside, Linux kernel has to communicate with
> system
> +	  controller via MU (message unit) IPC to get temperature from
> thermal
> +	  sensor. It supports one critical trip point and one
> +	  passive trip point for each thermal sensor.
> +
>  config MAX77620_THERMAL
>  	tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
>  	depends on MFD_MAX77620
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> 486d682..4062627 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_DB8500_THERMAL)	+=
> db8500_thermal.o
>  obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
>  obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
>  obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
> +obj-$(CONFIG_IMX_SC_THERMAL)	+= imx_sc_thermal.o
>  obj-$(CONFIG_MAX77620_THERMAL)	+= max77620_thermal.o
>  obj-$(CONFIG_QORIQ_THERMAL)	+= qoriq_thermal.o
>  obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
> diff --git a/drivers/thermal/imx_sc_thermal.c
> b/drivers/thermal/imx_sc_thermal.c
> new file mode 100644
> index 0000000..dcf16fc
> --- /dev/null
> +++ b/drivers/thermal/imx_sc_thermal.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#include "thermal_core.h"
> +
> +#define IMX_SC_MISC_FUNC_GET_TEMP	13
> +#define IMX_SC_C_TEMP			0
> +
> +static struct imx_sc_ipc *thermal_ipc_handle;
> +
> +struct imx_sc_sensor {
> +	struct thermal_zone_device *tzd;
> +	u32 resource_id;
> +};
> +
> +struct req_get_temp {
> +	u16 resource_id;
> +	u8 type;
> +} __packed;
> +
> +struct resp_get_temp {
> +	u16 celsius;
> +	u8 tenths;
> +} __packed;
> +
> +struct imx_sc_msg_misc_get_temp {
> +	struct imx_sc_rpc_msg hdr;
> +	union {
> +		struct req_get_temp req;
> +		struct resp_get_temp resp;
> +	} data;
> +};
> +
> +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> +	struct imx_sc_msg_misc_get_temp msg;
> +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +	struct imx_sc_sensor *sensor = data;
> +	int ret;
> +
> +	msg.data.req.resource_id = sensor->resource_id;
> +	msg.data.req.type = IMX_SC_C_TEMP;
> +
> +	hdr->ver = IMX_SC_RPC_VERSION;
> +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> +	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> +	hdr->size = 2;
> +
> +	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
> +	if (ret) {
> +		pr_err("read temp sensor %d failed, ret %d\n",
> +			sensor->resource_id, ret);
> +		return ret;
> +	}
> +
> +	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
> +	.get_temp = imx_sc_thermal_get_temp,
> +};
> +
> +static int imx_sc_thermal_probe(struct platform_device *pdev) {
> +	struct device_node *np, *child;
> +	int ret;
> +
> +	ret = imx_scu_get_handle(&thermal_ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	np = of_find_node_by_name(NULL, "thermal-zones");
> +	if (!np)
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct of_phandle_args sensor_specs;
> +		struct imx_sc_sensor *sensor =
> +			devm_kzalloc(&pdev->dev, sizeof(*sensor),
> GFP_KERNEL);
> +		if (!sensor)
> +			return -ENOMEM;
> +
> +		ret = thermal_zone_of_get_sensor_id(child,
> +						    &sensor_specs,
> +						    &sensor->resource_id);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to get valid sensor resource id: %d\n",
> +				ret);
> +			break;
> +		}
> +
> +		sensor->tzd =
> devm_thermal_zone_of_sensor_register(&pdev->dev,
> +								   sensor-
> >resource_id,
> +								   sensor,
> +
> &imx_sc_thermal_ops);
> +		if (IS_ERR(sensor->tzd)) {
> +			dev_err(&pdev->dev, "failed to register thermal
> zone\n");
> +			ret = PTR_ERR(sensor->tzd);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id imx_sc_thermal_table[] = {
> +	{ .compatible = "fsl,imx-sc-thermal", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> +
> +static struct platform_driver imx_sc_thermal_driver = {
> +		.probe = imx_sc_thermal_probe,
> +		.driver = {
> +			.name = "imx-sc-thermal",
> +			.of_match_table = imx_sc_thermal_table,
> +		},
> +};
> +module_platform_driver(imx_sc_thermal_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> +controller"); MODULE_LICENSE("GPL v2");
> --
> 2.7.4

RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Anson Huang]

Ping...

> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, May 9, 2019 9:44 AM
> To: 'Anson Huang' <Anson.Huang@nxp.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; catalin.marinas@arm.com;
> will.deacon@arm.com; rui.zhang@intel.com; edubezval@gmail.com;
> daniel.lezcano@linaro.org; Aisheng Dong <aisheng.dong@nxp.com>;
> ulf.hansson@linaro.org; Daniel Baluta <daniel.baluta@nxp.com>; Peng Fan
> <peng.fan@nxp.com>; heiko@sntech.de; horms+renesas@verge.net.au;
> agross@kernel.org; olof@lixom.net; bjorn.andersson@linaro.org;
> jagan@amarulasolutions.com; enric.balletbo@collabora.com;
> marc.w.gonzalez@free.fr; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> pm@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller
> thermal support
> 
> Ping...
> 
> > -----Original Message-----
> > From: Anson Huang [mailto:Anson.Huang@nxp.com]
> > Sent: Tuesday, April 16, 2019 11:22 AM
> > To: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > catalin.marinas@arm.com; will.deacon@arm.com; rui.zhang@intel.com;
> > edubezval@gmail.com; daniel.lezcano@linaro.org; Aisheng Dong
> > <aisheng.dong@nxp.com>; ulf.hansson@linaro.org; Daniel Baluta
> > <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> heiko@sntech.de;
> > horms+renesas@verge.net.au; agross@kernel.org; olof@lixom.net;
> > bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
> > enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-pm@vger.kernel.org
> > Cc: dl-linux-imx <linux-imx@nxp.com>
> > Subject: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller
> > thermal support
> >
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and thermal sensors etc..
> >
> > This patch adds i.MX system controller thermal driver support, Linux
> > kernel has to communicate with system controller via MU (message unit)
> > IPC to get each thermal sensor's temperature, it supports multiple
> > sensors which are passed from device tree, please see the binding doc for
> details.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V11:
> > 	- move the API of getting thermal zone sensor ID to of-thermal.c as
> > generic API;
> > 	- remove unnecessary __packed.
> > ---
> >  drivers/thermal/Kconfig          |  11 ++++
> >  drivers/thermal/Makefile         |   1 +
> >  drivers/thermal/imx_sc_thermal.c | 137
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 149 insertions(+)
> >  create mode 100644 drivers/thermal/imx_sc_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 653aa27..4e4fa7e 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -223,6 +223,17 @@ config IMX_THERMAL
> >  	  cpufreq is used as the cooling device to throttle CPUs when the
> >  	  passive trip is crossed.
> >
> > +config IMX_SC_THERMAL
> > +	tristate "Temperature sensor driver for NXP i.MX SoCs with System
> > Controller"
> > +	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST
> > +	depends on OF
> > +	help
> > +	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX
> > SoCs with
> > +	  system controller inside, Linux kernel has to communicate with
> > system
> > +	  controller via MU (message unit) IPC to get temperature from
> > thermal
> > +	  sensor. It supports one critical trip point and one
> > +	  passive trip point for each thermal sensor.
> > +
> >  config MAX77620_THERMAL
> >  	tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
> >  	depends on MFD_MAX77620
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > 486d682..4062627 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -40,6 +40,7 @@ obj-$(CONFIG_DB8500_THERMAL)	+=
> > db8500_thermal.o
> >  obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
> >  obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
> >  obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
> > +obj-$(CONFIG_IMX_SC_THERMAL)	+= imx_sc_thermal.o
> >  obj-$(CONFIG_MAX77620_THERMAL)	+= max77620_thermal.o
> >  obj-$(CONFIG_QORIQ_THERMAL)	+= qoriq_thermal.o
> >  obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
> > diff --git a/drivers/thermal/imx_sc_thermal.c
> > b/drivers/thermal/imx_sc_thermal.c
> > new file mode 100644
> > index 0000000..dcf16fc
> > --- /dev/null
> > +++ b/drivers/thermal/imx_sc_thermal.c
> > @@ -0,0 +1,137 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/firmware/imx/sci.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#include "thermal_core.h"
> > +
> > +#define IMX_SC_MISC_FUNC_GET_TEMP	13
> > +#define IMX_SC_C_TEMP			0
> > +
> > +static struct imx_sc_ipc *thermal_ipc_handle;
> > +
> > +struct imx_sc_sensor {
> > +	struct thermal_zone_device *tzd;
> > +	u32 resource_id;
> > +};
> > +
> > +struct req_get_temp {
> > +	u16 resource_id;
> > +	u8 type;
> > +} __packed;
> > +
> > +struct resp_get_temp {
> > +	u16 celsius;
> > +	u8 tenths;
> > +} __packed;
> > +
> > +struct imx_sc_msg_misc_get_temp {
> > +	struct imx_sc_rpc_msg hdr;
> > +	union {
> > +		struct req_get_temp req;
> > +		struct resp_get_temp resp;
> > +	} data;
> > +};
> > +
> > +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> > +	struct imx_sc_msg_misc_get_temp msg;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	struct imx_sc_sensor *sensor = data;
> > +	int ret;
> > +
> > +	msg.data.req.resource_id = sensor->resource_id;
> > +	msg.data.req.type = IMX_SC_C_TEMP;
> > +
> > +	hdr->ver = IMX_SC_RPC_VERSION;
> > +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> > +	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> > +	hdr->size = 2;
> > +
> > +	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
> > +	if (ret) {
> > +		pr_err("read temp sensor %d failed, ret %d\n",
> > +			sensor->resource_id, ret);
> > +		return ret;
> > +	}
> > +
> > +	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
> > +	.get_temp = imx_sc_thermal_get_temp, };
> > +
> > +static int imx_sc_thermal_probe(struct platform_device *pdev) {
> > +	struct device_node *np, *child;
> > +	int ret;
> > +
> > +	ret = imx_scu_get_handle(&thermal_ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	np = of_find_node_by_name(NULL, "thermal-zones");
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	for_each_available_child_of_node(np, child) {
> > +		struct of_phandle_args sensor_specs;
> > +		struct imx_sc_sensor *sensor =
> > +			devm_kzalloc(&pdev->dev, sizeof(*sensor),
> > GFP_KERNEL);
> > +		if (!sensor)
> > +			return -ENOMEM;
> > +
> > +		ret = thermal_zone_of_get_sensor_id(child,
> > +						    &sensor_specs,
> > +						    &sensor->resource_id);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev,
> > +				"failed to get valid sensor resource id: %d\n",
> > +				ret);
> > +			break;
> > +		}
> > +
> > +		sensor->tzd =
> > devm_thermal_zone_of_sensor_register(&pdev->dev,
> > +								   sensor-
> > >resource_id,
> > +								   sensor,
> > +
> > &imx_sc_thermal_ops);
> > +		if (IS_ERR(sensor->tzd)) {
> > +			dev_err(&pdev->dev, "failed to register thermal
> > zone\n");
> > +			ret = PTR_ERR(sensor->tzd);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id imx_sc_thermal_table[] = {
> > +	{ .compatible = "fsl,imx-sc-thermal", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> > +
> > +static struct platform_driver imx_sc_thermal_driver = {
> > +		.probe = imx_sc_thermal_probe,
> > +		.driver = {
> > +			.name = "imx-sc-thermal",
> > +			.of_match_table = imx_sc_thermal_table,
> > +		},
> > +};
> > +module_platform_driver(imx_sc_thermal_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> > +controller"); MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4

Re: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Daniel Lezcano]

On 16/04/2019 05:22, Anson Huang wrote:
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> inside, the system controller is in charge of controlling power,
> clock and thermal sensors etc..
> 
> This patch adds i.MX system controller thermal driver support,
> Linux kernel has to communicate with system controller via MU
> (message unit) IPC to get each thermal sensor's temperature,
> it supports multiple sensors which are passed from device tree,
> please see the binding doc for details.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V11:
> 	- move the API of getting thermal zone sensor ID to of-thermal.c as generic API;
> 	- remove unnecessary __packed.

The patch itself sounds good to me but the getting thermal zone by
sensor id patch seems to be controversial. It would be nice to rise a
discussion with this new function you proposed in 2/5.

> ---
>  drivers/thermal/Kconfig          |  11 ++++
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/imx_sc_thermal.c | 137 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/thermal/imx_sc_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 653aa27..4e4fa7e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -223,6 +223,17 @@ config IMX_THERMAL
>  	  cpufreq is used as the cooling device to throttle CPUs when the
>  	  passive trip is crossed.
>  
> +config IMX_SC_THERMAL
> +	tristate "Temperature sensor driver for NXP i.MX SoCs with System Controller"
> +	depends on (ARCH_MXC && IMX_SCU) || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Support for Temperature Monitor (TEMPMON) found on NXP i.MX SoCs with
> +	  system controller inside, Linux kernel has to communicate with system
> +	  controller via MU (message unit) IPC to get temperature from thermal
> +	  sensor. It supports one critical trip point and one
> +	  passive trip point for each thermal sensor.
> +
>  config MAX77620_THERMAL
>  	tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
>  	depends on MFD_MAX77620
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 486d682..4062627 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_DB8500_THERMAL)	+= db8500_thermal.o
>  obj-$(CONFIG_ARMADA_THERMAL)	+= armada_thermal.o
>  obj-$(CONFIG_TANGO_THERMAL)	+= tango_thermal.o
>  obj-$(CONFIG_IMX_THERMAL)	+= imx_thermal.o
> +obj-$(CONFIG_IMX_SC_THERMAL)	+= imx_sc_thermal.o
>  obj-$(CONFIG_MAX77620_THERMAL)	+= max77620_thermal.o
>  obj-$(CONFIG_QORIQ_THERMAL)	+= qoriq_thermal.o
>  obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
> diff --git a/drivers/thermal/imx_sc_thermal.c b/drivers/thermal/imx_sc_thermal.c
> new file mode 100644
> index 0000000..dcf16fc
> --- /dev/null
> +++ b/drivers/thermal/imx_sc_thermal.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +
> +#include "thermal_core.h"
> +
> +#define IMX_SC_MISC_FUNC_GET_TEMP	13
> +#define IMX_SC_C_TEMP			0
> +
> +static struct imx_sc_ipc *thermal_ipc_handle;
> +
> +struct imx_sc_sensor {
> +	struct thermal_zone_device *tzd;
> +	u32 resource_id;
> +};
> +
> +struct req_get_temp {
> +	u16 resource_id;
> +	u8 type;
> +} __packed;
> +
> +struct resp_get_temp {
> +	u16 celsius;
> +	u8 tenths;
> +} __packed;
> +
> +struct imx_sc_msg_misc_get_temp {
> +	struct imx_sc_rpc_msg hdr;
> +	union {
> +		struct req_get_temp req;
> +		struct resp_get_temp resp;
> +	} data;
> +};
> +
> +static int imx_sc_thermal_get_temp(void *data, int *temp)
> +{
> +	struct imx_sc_msg_misc_get_temp msg;
> +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +	struct imx_sc_sensor *sensor = data;
> +	int ret;
> +
> +	msg.data.req.resource_id = sensor->resource_id;
> +	msg.data.req.type = IMX_SC_C_TEMP;
> +
> +	hdr->ver = IMX_SC_RPC_VERSION;
> +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> +	hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> +	hdr->size = 2;
> +
> +	ret = imx_scu_call_rpc(thermal_ipc_handle, &msg, true);
> +	if (ret) {
> +		pr_err("read temp sensor %d failed, ret %d\n",
> +			sensor->resource_id, ret);
> +		return ret;
> +	}
> +
> +	*temp = msg.data.resp.celsius * 1000 + msg.data.resp.tenths * 100;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops imx_sc_thermal_ops = {
> +	.get_temp = imx_sc_thermal_get_temp,
> +};
> +
> +static int imx_sc_thermal_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np, *child;
> +	int ret;
> +
> +	ret = imx_scu_get_handle(&thermal_ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	np = of_find_node_by_name(NULL, "thermal-zones");
> +	if (!np)
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct of_phandle_args sensor_specs;
> +		struct imx_sc_sensor *sensor =
> +			devm_kzalloc(&pdev->dev, sizeof(*sensor), GFP_KERNEL);
> +		if (!sensor)
> +			return -ENOMEM;
> +
> +		ret = thermal_zone_of_get_sensor_id(child,
> +						    &sensor_specs,
> +						    &sensor->resource_id);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to get valid sensor resource id: %d\n",
> +				ret);
> +			break;
> +		}
> +
> +		sensor->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
> +								   sensor->resource_id,
> +								   sensor,
> +								   &imx_sc_thermal_ops);
> +		if (IS_ERR(sensor->tzd)) {
> +			dev_err(&pdev->dev, "failed to register thermal zone\n");
> +			ret = PTR_ERR(sensor->tzd);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id imx_sc_thermal_table[] = {
> +	{ .compatible = "fsl,imx-sc-thermal", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> +
> +static struct platform_driver imx_sc_thermal_driver = {
> +		.probe = imx_sc_thermal_probe,
> +		.driver = {
> +			.name = "imx-sc-thermal",
> +			.of_match_table = imx_sc_thermal_table,
> +		},
> +};
> +module_platform_driver(imx_sc_thermal_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system controller");
> +MODULE_LICENSE("GPL v2");
> 


-- 
 <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 V12 2/5] thermal: of-thermal: add API for getting sensor ID from DT

[Daniel Lezcano]

On 16/04/2019 05:22, Anson Huang wrote:
> On some platforms like i.MX8QXP, the thermal driver needs a
> real HW sensor ID from DT thermal zone, the HW sensor ID is
> used to get temperature from SCU firmware, and the virtual
> sensor ID starting from 0 to N is NOT used at all, this patch
> adds new API thermal_zone_of_get_sensor_id() to provide the
> feature of getting sensor ID from DT thermal zone's node.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Eduardo?

What do you think about this patch?

> ---
> New patch.
> ---
>  drivers/thermal/of-thermal.c | 53 ++++++++++++++++++++++++++++++++++----------
>  include/linux/thermal.h      | 10 +++++++++
>  2 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 2df059c..2e4320c 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -446,6 +446,46 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  }
>  
>  /**
> + * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
> + * @tz_np: a valid thermal zone device node.
> + * @sensor_specs: pointer to output arguments structure will be passed back.
> + * @id: a sensor ID pointer will be passed back.
> + *
> + * This function will get sensor ID from a given thermal zone node, use
> + * "thermal-sensors" as list name, and get sensor ID from first phandle's
> + * argument.
> + *
> + * Return: 0 on success, proper error code otherwise.
> + */
> +
> +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> +				  struct of_phandle_args *sensor_specs,
> +				  u32 *id)
> +{
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(tz_np,
> +					 "thermal-sensors",
> +					 "#thermal-sensor-cells",
> +					 0,
> +					 sensor_specs);
> +	if (ret)
> +		return ret;
> +
> +	if (sensor_specs->args_count >= 1) {
> +		*id = sensor_specs->args[0];
> +		WARN(sensor_specs->args_count > 1,
> +		     "%pOFn: too many cells in sensor specifier %d\n",
> +		     sensor_specs->np, sensor_specs->args_count);
> +	} else {
> +		*id = 0;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
> +
> +/**
>   * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
>   * @dev: a valid struct device pointer of a sensor device. Must contain
>   *       a valid .of_node, for the sensor node.
> @@ -500,21 +540,10 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
>  		int ret, id;
>  
>  		/* For now, thermal framework supports only 1 sensor per zone */
> -		ret = of_parse_phandle_with_args(child, "thermal-sensors",
> -						 "#thermal-sensor-cells",
> -						 0, &sensor_specs);
> +		ret = thermal_zone_of_get_sensor_id(child, &sensor_specs, &id);
>  		if (ret)
>  			continue;
>  
> -		if (sensor_specs.args_count >= 1) {
> -			id = sensor_specs.args[0];
> -			WARN(sensor_specs.args_count > 1,
> -			     "%pOFn: too many cells in sensor specifier %d\n",
> -			     sensor_specs.np, sensor_specs.args_count);
> -		} else {
> -			id = 0;
> -		}
> -
>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>  							 data, ops);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 5f4705f..970fb37 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -375,6 +375,9 @@ struct thermal_trip {
>  
>  /* Function declarations */
>  #ifdef CONFIG_THERMAL_OF
> +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> +				  struct of_phandle_args *sensor_specs,
> +				  u32 *id);
>  struct thermal_zone_device *
>  thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
>  				const struct thermal_zone_of_device_ops *ops);
> @@ -386,6 +389,13 @@ struct thermal_zone_device *devm_thermal_zone_of_sensor_register(
>  void devm_thermal_zone_of_sensor_unregister(struct device *dev,
>  					    struct thermal_zone_device *tz);
>  #else
> +
> +static int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> +					 struct of_phandle_args *sensor_specs,
> +					 u32 *id)
> +{
> +	return -ENOENT;
> +}
>  static inline struct thermal_zone_device *
>  thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
>  				const struct thermal_zone_of_device_ops *ops)
> 


-- 
 <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 V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Aisheng Dong]

[...]

> > > +		if (!sensor)
> > > +			return -ENOMEM;
> > > +
> > > +		ret = thermal_zone_of_get_sensor_id(child,
> > > +						    &sensor_specs,
> >
> > It looks a bit strange why need sensor_specs as you even did not use it.
> 
> It is because parsing the phandle arguments needs it, such as the
> thermal_zone_of_sensor_register() function needs this pointer to be passed
> back to check the elements are valid or NOT, so the API does NOT define it as
> local structure. The API NOT just return the sensor id, but also the
> of_phandle_args address, although we do NOT need it at all here.

The main problem is this one introduced in 2/5 which needs get ack from maintainer.
For me, I would suggest to make sensor_specs transparent to user if
it's really not needed by users.

Please try to make it either optional or hide to core users as well.
And I guess the later one is doable after a glance at the core code.

Regards
Dong Aisheng

RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller thermal support

[Anson Huang]

> -----Original Message-----
> From: Aisheng Dong
> Sent: Thursday, May 16, 2019 6:40 PM
> To: Anson Huang <anson.huang@nxp.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; catalin.marinas@arm.com;
> will.deacon@arm.com; rui.zhang@intel.com; edubezval@gmail.com;
> daniel.lezcano@linaro.org; ulf.hansson@linaro.org; Daniel Baluta
> <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> heiko@sntech.de; horms+renesas@verge.net.au; agross@kernel.org;
> olof@lixom.net; bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pm@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH V12 3/5] thermal: imx_sc: add i.MX system controller
> thermal support
> 
> [...]
> 
> > > > +		if (!sensor)
> > > > +			return -ENOMEM;
> > > > +
> > > > +		ret = thermal_zone_of_get_sensor_id(child,
> > > > +						    &sensor_specs,
> > >
> > > It looks a bit strange why need sensor_specs as you even did not use it.
> >
> > It is because parsing the phandle arguments needs it, such as the
> > thermal_zone_of_sensor_register() function needs this pointer to be
> > passed back to check the elements are valid or NOT, so the API does
> > NOT define it as local structure. The API NOT just return the sensor
> > id, but also the of_phandle_args address, although we do NOT need it at all
> here.
> 
> The main problem is this one introduced in 2/5 which needs get ack from
> maintainer.
> For me, I would suggest to make sensor_specs transparent to user if it's
> really not needed by users.
> 
> Please try to make it either optional or hide to core users as well.
> And I guess the later one is doable after a glance at the core code.

OK, I will hide it to user if no other different comments.

Anson.

> 
> Regards
> Dong Aisheng

RE: [PATCH V12 2/5] thermal: of-thermal: add API for getting sensor ID from DT

[Anson Huang]

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Thursday, May 16, 2019 5:39 PM
> To: Anson Huang <anson.huang@nxp.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; catalin.marinas@arm.com;
> will.deacon@arm.com; rui.zhang@intel.com; edubezval@gmail.com;
> Aisheng Dong <aisheng.dong@nxp.com>; ulf.hansson@linaro.org; Daniel
> Baluta <daniel.baluta@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> heiko@sntech.de; horms+renesas@verge.net.au; agross@kernel.org;
> olof@lixom.net; bjorn.andersson@linaro.org; jagan@amarulasolutions.com;
> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-pm@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V12 2/5] thermal: of-thermal: add API for getting sensor
> ID from DT
> 
> On 16/04/2019 05:22, Anson Huang wrote:
> > On some platforms like i.MX8QXP, the thermal driver needs a real HW
> > sensor ID from DT thermal zone, the HW sensor ID is used to get
> > temperature from SCU firmware, and the virtual sensor ID starting from
> > 0 to N is NOT used at all, this patch adds new API
> > thermal_zone_of_get_sensor_id() to provide the feature of getting
> > sensor ID from DT thermal zone's node.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> Eduardo?
> 
> What do you think about this patch?

I would like to hear Eduardo's opinion about this patch before sending out a new version
for review, thanks.

Anson.

> 
> > ---
> > New patch.
> > ---
> >  drivers/thermal/of-thermal.c | 53
> ++++++++++++++++++++++++++++++++++----------
> >  include/linux/thermal.h      | 10 +++++++++
> >  2 files changed, 51 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/thermal/of-thermal.c
> > b/drivers/thermal/of-thermal.c index 2df059c..2e4320c 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -446,6 +446,46 @@ thermal_zone_of_add_sensor(struct device_node
> > *zone,  }
> >
> >  /**
> > + * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal
> > + zone
> > + * @tz_np: a valid thermal zone device node.
> > + * @sensor_specs: pointer to output arguments structure will be passed
> back.
> > + * @id: a sensor ID pointer will be passed back.
> > + *
> > + * This function will get sensor ID from a given thermal zone node,
> > + use
> > + * "thermal-sensors" as list name, and get sensor ID from first
> > + phandle's
> > + * argument.
> > + *
> > + * Return: 0 on success, proper error code otherwise.
> > + */
> > +
> > +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> > +				  struct of_phandle_args *sensor_specs,
> > +				  u32 *id)
> > +{
> > +	int ret;
> > +
> > +	ret = of_parse_phandle_with_args(tz_np,
> > +					 "thermal-sensors",
> > +					 "#thermal-sensor-cells",
> > +					 0,
> > +					 sensor_specs);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (sensor_specs->args_count >= 1) {
> > +		*id = sensor_specs->args[0];
> > +		WARN(sensor_specs->args_count > 1,
> > +		     "%pOFn: too many cells in sensor specifier %d\n",
> > +		     sensor_specs->np, sensor_specs->args_count);
> > +	} else {
> > +		*id = 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
> > +
> > +/**
> >   * thermal_zone_of_sensor_register - registers a sensor to a DT thermal
> zone
> >   * @dev: a valid struct device pointer of a sensor device. Must contain
> >   *       a valid .of_node, for the sensor node.
> > @@ -500,21 +540,10 @@ thermal_zone_of_sensor_register(struct device
> *dev, int sensor_id, void *data,
> >  		int ret, id;
> >
> >  		/* For now, thermal framework supports only 1 sensor per
> zone */
> > -		ret = of_parse_phandle_with_args(child, "thermal-sensors",
> > -						 "#thermal-sensor-cells",
> > -						 0, &sensor_specs);
> > +		ret = thermal_zone_of_get_sensor_id(child, &sensor_specs,
> &id);
> >  		if (ret)
> >  			continue;
> >
> > -		if (sensor_specs.args_count >= 1) {
> > -			id = sensor_specs.args[0];
> > -			WARN(sensor_specs.args_count > 1,
> > -			     "%pOFn: too many cells in sensor specifier %d\n",
> > -			     sensor_specs.np, sensor_specs.args_count);
> > -		} else {
> > -			id = 0;
> > -		}
> > -
> >  		if (sensor_specs.np == sensor_np && id == sensor_id) {
> >  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
> >  							 data, ops);
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h index
> > 5f4705f..970fb37 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -375,6 +375,9 @@ struct thermal_trip {
> >
> >  /* Function declarations */
> >  #ifdef CONFIG_THERMAL_OF
> > +int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> > +				  struct of_phandle_args *sensor_specs,
> > +				  u32 *id);
> >  struct thermal_zone_device *
> >  thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
> >  				const struct thermal_zone_of_device_ops
> *ops); @@ -386,6 +389,13
> > @@ struct thermal_zone_device
> *devm_thermal_zone_of_sensor_register(
> >  void devm_thermal_zone_of_sensor_unregister(struct device *dev,
> >  					    struct thermal_zone_device *tz);
> #else
> > +
> > +static int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
> > +					 struct of_phandle_args
> *sensor_specs,
> > +					 u32 *id)
> > +{
> > +	return -ENOENT;
> > +}
> >  static inline struct thermal_zone_device *
> > thermal_zone_of_sensor_register(struct device *dev, int id, void *data,
> >  				const struct thermal_zone_of_device_ops
> *ops)
> >
> 
> 
> --
> 
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Ca89b31
> e8690d4fba3e8b08d6d9e25488%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636935963439948138&amp;sdata=gAYU5pi1Isj%2BhfREN8n6v03
> Lb%2F9eRzDzC2ocDdfe%2Bkc%3D&amp;reserved=0> Linaro.org │ Open
> source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Canson.huang%4
> 0nxp.com%7Ca89b31e8690d4fba3e8b08d6d9e25488%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C636935963439948138&amp;sdata=j%2Fo%2
> BFN8mALGBVhVQfKoYhijDw51FhcGjackCaUl%2BY%2FQ%3D&amp;reserved=
> 0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7Ca89b31e8690d4fba3e8b08d6d9e25488%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C636935963439948138&amp;sdata=O%2Bry%2Fdk
> UGuO22brYTMaz%2FCvGOUNt15WJF8uALLVgiwk%3D&amp;reserved=0>
> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Ca89b31e869
> 0d4fba3e8b08d6d9e25488%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C636935963439948138&amp;sdata=WLHahx%2BryvBprdoNUEvQ4kCnY
> FmUuqgA14xqHjS7WHU%3D&amp;reserved=0> Blog