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@xxxxxxx>
> > ---
> > 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@xxxxxxx>");
> > +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> > +controller"); MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4