[PATCH v1 4/9] imx: thermal: Configure trip point from DT

[Marco Felsch]

Hi Francesco,

nice patch, only a few nits.

On 22-06-15, Francesco Dolcini wrote:
> Allow over-writing critical and passive trip point for each
> temperature grade from the device tree, by default the pre-existing
> hard-coded trip points are used.
> 
> This change enables configuring the system thermal characteristics into
> the system-specific device tree instead of relying on global hard-coded
> temperature thresholds that does not take into account the specific
> system thermal design.
> 
> Signed-off-by: Francesco Dolcini <francesco.dolcini at toradex.com>
> ---
>  drivers/thermal/imx_thermal.c | 49 +++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 16663373b682..ef3e152b5ee2 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -17,6 +17,8 @@
>  #include <linux/nvmem-consumer.h>
>  #include <linux/pm_runtime.h>
>  
> +#include "thermal_core.h"
> +
>  #define REG_SET		0x4
>  #define REG_CLR		0x8
>  #define REG_TOG		0xc
> @@ -479,36 +481,83 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  	return 0;
>  }
>  
> +static void imx_init_temp_from_of(struct platform_device *pdev, const char *name)
> +{
> +	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> +	struct device_node *thermal, *trips, *trip_point;
> +
> +	thermal = of_get_child_by_name(pdev->dev.of_node, name);

here I would do:

	if (!thermal)
		return;

since the thermal node is only available with your dt-changes in place.

> +	trips = of_get_child_by_name(thermal, "trips");
> +
> +	for_each_child_of_node(trips, trip_point) {
> +		struct thermal_trip t;
> +
> +		if (thermal_of_populate_trip(trip_point, &t))
> +			continue;
> +
> +		switch (t.type) {
> +		case THERMAL_TRIP_PASSIVE:
> +			data->temp_passive = t.temperature;
> +			break;
> +		case THERMAL_TRIP_CRITICAL:
> +			data->temp_critical = t.temperature;
> +			break;
> +		default:
> +			dev_dbg(&pdev->dev, "Ignoring trip type %d\n", t.type);
			  ^
Maybe it is worth to use dev_info() since this never should happen and
if it happen, it is a bug/misconfiguration/misusage.

> +			break;
> +		}
> +	};
> +
> +	of_node_put(trips);
> +	of_node_put(thermal);
> +
> +	if (data->temp_passive >= data->temp_critical) {
> +		dev_warn(&pdev->dev,
> +			 "passive trip point must be lower than critical, fixing it up\n");
> +		data->temp_passive = data->temp_critical - (1000 * 5);
								^
			Magic number? Maybe it would be worth a comment.

Regards,
  Marco

> +	}
> +}
> +
>  static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> +	const char *thermal_node_name;
>  
>  	/* The maximum die temp is specified by the Temperature Grade */
>  	switch ((ocotp_mem0 >> 6) & 0x3) {
>  	case 0: /* Commercial (0 to 95 °C) */
> +		thermal_node_name = "commercial-thermal";
>  		data->temp_grade = "Commercial";
>  		data->temp_max = 95000;
>  		break;
>  	case 1: /* Extended Commercial (-20 °C to 105 °C) */
> +		thermal_node_name = "extended-commercial-thermal";
>  		data->temp_grade = "Extended Commercial";
>  		data->temp_max = 105000;
>  		break;
>  	case 2: /* Industrial (-40 °C to 105 °C) */
> +		thermal_node_name = "industrial-thermal";
>  		data->temp_grade = "Industrial";
>  		data->temp_max = 105000;
>  		break;
>  	case 3: /* Automotive (-40 °C to 125 °C) */
> +		thermal_node_name = "automotive-thermal";
>  		data->temp_grade = "Automotive";
>  		data->temp_max = 125000;
>  		break;
>  	}
>  
>  	/*
> +	 * Set defaults trips
> +	 *
>  	 * Set the critical trip point at 5 °C under max
>  	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
>  	 */
>  	data->temp_critical = data->temp_max - (1000 * 5);
>  	data->temp_passive = data->temp_max - (1000 * 10);
> +
> +	/* Override critical/passive temperature from devicetree */
> +	imx_init_temp_from_of(pdev, thermal_node_name);
>  }
>  
>  static int imx_init_from_tempmon_data(struct platform_device *pdev)
> -- 
> 2.25.1
> 
> 
>