thermal: imx: register as OF sensor
diff mbox series

Message ID 20181105165134.28963-1-l.stach@pengutronix.de
State New
Delegated to: Eduardo Valentin
Headers show
Series
  • thermal: imx: register as OF sensor
Related show

Commit Message

Lucas Stach Nov. 5, 2018, 4:51 p.m. UTC
To make the internal sensor usable with a thermal zone description
provided via DT, also register our device as a OF sensor.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Daniel Lezcano Nov. 5, 2018, 5:53 p.m. UTC | #1
On 05/11/2018 17:51, Lucas Stach wrote:
> To make the internal sensor usable with a thermal zone description
> provided via DT, also register our device as a OF sensor.

Can you elaborate ? Is the imx DT based *only* ?

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/thermal/imx_thermal.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index aa452acb60b6..32f406099479 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -202,7 +202,7 @@ static struct thermal_soc_data thermal_imx7d_data = {
>  
>  struct imx_thermal_data {
>  	struct cpufreq_policy *policy;
> -	struct thermal_zone_device *tz;
> +	struct thermal_zone_device *tz, *sensor;
>  	struct thermal_cooling_device *cdev;
>  	enum thermal_device_mode mode;
>  	struct regmap *tempmon;
> @@ -338,6 +338,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
>  	return 0;
>  }
>  
> +static int imx_of_sensor_get_temp(void *data, int *temp)
> +{
> +	struct imx_thermal_data *thermal_data = data;
> +
> +	return imx_get_temp(thermal_data->tz, temp);
> +}

Why not pass the tz directly to thermal_zone_of_sensor_register()
instead of data ?

>  static int imx_get_mode(struct thermal_zone_device *tz,
>  			enum thermal_device_mode *mode)
>  {
> @@ -482,6 +489,10 @@ static struct thermal_zone_device_ops imx_tz_ops = {
>  	.set_trip_temp = imx_set_trip_temp,
>  };
>  
> +static const struct thermal_zone_of_device_ops imx_tz_of_ops = {
> +	.get_temp = imx_of_sensor_get_temp,
> +};
> +
>  static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> @@ -798,6 +809,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	data->sensor = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> +						       &imx_tz_of_ops);
>
>  	dev_info(&pdev->dev, "%s CPU temperature grade - max:%dC"
>  		 " critical:%dC passive:%dC\n", data->temp_grade,
>  		 data->temp_max / 1000, data->temp_critical / 1000,
> @@ -848,6 +862,7 @@ static int imx_thermal_remove(struct platform_device *pdev)
>  	if (!IS_ERR(data->thermal_clk))
>  		clk_disable_unprepare(data->thermal_clk);
>  
> +	thermal_zone_of_sensor_unregister(&pdev->dev, data->sensor);
>  	thermal_zone_device_unregister(data->tz);
>  	cpufreq_cooling_unregister(data->cdev);
>  	cpufreq_cpu_put(data->policy);
>
Eduardo Valentin Nov. 6, 2018, 12:11 a.m. UTC | #2
On Mon, Nov 05, 2018 at 05:51:34PM +0100, Lucas Stach wrote:
> To make the internal sensor usable with a thermal zone description
> provided via DT, also register our device as a OF sensor.

Not sure I understand your patch. I see you probably want to
have thermal zones described in DT that can make use of the
imx sensors, but here you may end up in a situation with double
registration of the same driver.

Typically drivers will have either mode. Some of them kept
both modes, legacy and of- based, but at run time one is picked,
not both at the same time.

> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/thermal/imx_thermal.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index aa452acb60b6..32f406099479 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -202,7 +202,7 @@ static struct thermal_soc_data thermal_imx7d_data = {
>  
>  struct imx_thermal_data {
>  	struct cpufreq_policy *policy;
> -	struct thermal_zone_device *tz;
> +	struct thermal_zone_device *tz, *sensor;
>  	struct thermal_cooling_device *cdev;
>  	enum thermal_device_mode mode;
>  	struct regmap *tempmon;
> @@ -338,6 +338,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
>  	return 0;
>  }
>  
> +static int imx_of_sensor_get_temp(void *data, int *temp)
> +{
> +	struct imx_thermal_data *thermal_data = data;
> +
> +	return imx_get_temp(thermal_data->tz, temp);
> +}
> +
>  static int imx_get_mode(struct thermal_zone_device *tz,
>  			enum thermal_device_mode *mode)
>  {
> @@ -482,6 +489,10 @@ static struct thermal_zone_device_ops imx_tz_ops = {
>  	.set_trip_temp = imx_set_trip_temp,
>  };
>  
> +static const struct thermal_zone_of_device_ops imx_tz_of_ops = {
> +	.get_temp = imx_of_sensor_get_temp,
> +};
> +
>  static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> @@ -798,6 +809,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	data->sensor = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> +						       &imx_tz_of_ops);
> +
>  	dev_info(&pdev->dev, "%s CPU temperature grade - max:%dC"
>  		 " critical:%dC passive:%dC\n", data->temp_grade,
>  		 data->temp_max / 1000, data->temp_critical / 1000,
> @@ -848,6 +862,7 @@ static int imx_thermal_remove(struct platform_device *pdev)
>  	if (!IS_ERR(data->thermal_clk))
>  		clk_disable_unprepare(data->thermal_clk);
>  
> +	thermal_zone_of_sensor_unregister(&pdev->dev, data->sensor);
>  	thermal_zone_device_unregister(data->tz);
>  	cpufreq_cooling_unregister(data->cdev);
>  	cpufreq_cpu_put(data->policy);
> -- 
> 2.19.1
>
Lucas Stach Nov. 6, 2018, 9:14 a.m. UTC | #3
Hi Eduardo,

Am Montag, den 05.11.2018, 16:11 -0800 schrieb Eduardo Valentin:
> On Mon, Nov 05, 2018 at 05:51:34PM +0100, Lucas Stach wrote:
> > To make the internal sensor usable with a thermal zone description
> > provided via DT, also register our device as a OF sensor.
> 
> Not sure I understand your patch. I see you probably want to
> have thermal zones described in DT that can make use of the
> imx sensors, but here you may end up in a situation with double
> registration of the same driver.
> 
> Typically drivers will have either mode. Some of them kept
> both modes, legacy and of- based, but at run time one is picked,
> not both at the same time.

So the thing here is that the thermal zone registered by the driver has
a value on its own, as it adds a critical trip-point for the maximum
SoC temperature, as determined by the fuses.

So I think there is some merit in keeping both the "legacy" thermal
zone and the OF sensor. I also don't see where this would be an issue,
but I admit not having looked too closely at all the details of the
thermal framework. Do you have any guidance here?

Regards,
Lucas

> > 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/thermal/imx_thermal.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index aa452acb60b6..32f406099479 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -202,7 +202,7 @@ static struct thermal_soc_data thermal_imx7d_data = {
> >  
> >  struct imx_thermal_data {
> > > >  	struct cpufreq_policy *policy;
> > > > -	struct thermal_zone_device *tz;
> > > > +	struct thermal_zone_device *tz, *sensor;
> > > >  	struct thermal_cooling_device *cdev;
> > > >  	enum thermal_device_mode mode;
> > > >  	struct regmap *tempmon;
> > @@ -338,6 +338,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
> > > >  	return 0;
> >  }
> >  
> > +static int imx_of_sensor_get_temp(void *data, int *temp)
> > +{
> > > > +	struct imx_thermal_data *thermal_data = data;
> > +
> > > > +	return imx_get_temp(thermal_data->tz, temp);
> > +}
> > +
> >  static int imx_get_mode(struct thermal_zone_device *tz,
> > > >  			enum thermal_device_mode *mode)
> >  {
> > @@ -482,6 +489,10 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> > > >  	.set_trip_temp = imx_set_trip_temp,
> >  };
> >  
> > +static const struct thermal_zone_of_device_ops imx_tz_of_ops = {
> > > > +	.get_temp = imx_of_sensor_get_temp,
> > +};
> > +
> >  static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
> >  {
> > > >  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> > @@ -798,6 +809,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
> > > >  		return ret;
> > > >  	}
> >  
> > > > +	data->sensor = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> > > > +						       &imx_tz_of_ops);
> > +
> > > >  	dev_info(&pdev->dev, "%s CPU temperature grade - max:%dC"
> > > >  		 " critical:%dC passive:%dC\n", data->temp_grade,
> > > >  		 data->temp_max / 1000, data->temp_critical / 1000,
> > @@ -848,6 +862,7 @@ static int imx_thermal_remove(struct platform_device *pdev)
> > > >  	if (!IS_ERR(data->thermal_clk))
> > > >  		clk_disable_unprepare(data->thermal_clk);
> >  
> > > > +	thermal_zone_of_sensor_unregister(&pdev->dev, data->sensor);
> > > >  	thermal_zone_device_unregister(data->tz);
> > > >  	cpufreq_cooling_unregister(data->cdev);
> > > >  	cpufreq_cpu_put(data->policy);
> > -- 
> > 2.19.1
> >
Lucas Stach Feb. 12, 2019, 10:19 a.m. UTC | #4
Hi all,

Am Dienstag, den 06.11.2018, 10:14 +0100 schrieb Lucas Stach:
> Hi Eduardo,
> 
> Am Montag, den 05.11.2018, 16:11 -0800 schrieb Eduardo Valentin:
> > On Mon, Nov 05, 2018 at 05:51:34PM +0100, Lucas Stach wrote:
> > > To make the internal sensor usable with a thermal zone description
> > > provided via DT, also register our device as a OF sensor.
> > 
> > Not sure I understand your patch. I see you probably want to
> > have thermal zones described in DT that can make use of the
> > imx sensors, but here you may end up in a situation with double
> > registration of the same driver.
> > 
> > Typically drivers will have either mode. Some of them kept
> > both modes, legacy and of- based, but at run time one is picked,
> > not both at the same time.
> 
> So the thing here is that the thermal zone registered by the driver has
> a value on its own, as it adds a critical trip-point for the maximum
> SoC temperature, as determined by the fuses.
> 
> So I think there is some merit in keeping both the "legacy" thermal
> zone and the OF sensor. I also don't see where this would be an issue,
> but I admit not having looked too closely at all the details of the
> thermal framework. Do you have any guidance here?

Any preference on how to move forward with this? This patch is blocking
the introduction of thermal policies to some mainline DTs.

Regards,
Lucas

> 
> > > 
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > 
> > > ---
> > >  drivers/thermal/imx_thermal.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > > index aa452acb60b6..32f406099479 100644
> > > --- a/drivers/thermal/imx_thermal.c
> > > +++ b/drivers/thermal/imx_thermal.c
> > > @@ -202,7 +202,7 @@ static struct thermal_soc_data thermal_imx7d_data = {
> > >  
> > >  struct imx_thermal_data {
> > > > > > > > > >  	struct cpufreq_policy *policy;
> > > > > > > > > > -	struct thermal_zone_device *tz;
> > > > > > > > > > +	struct thermal_zone_device *tz, *sensor;
> > > > > > > > > >  	struct thermal_cooling_device *cdev;
> > > > > > > > > >  	enum thermal_device_mode mode;
> > > > >  	struct regmap *tempmon;
> > > 
> > > @@ -338,6 +338,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
> > > > >  	return 0;
> > > 
> > >  }
> > >  
> > > +static int imx_of_sensor_get_temp(void *data, int *temp)
> > > +{
> > > > > +	struct imx_thermal_data *thermal_data = data;
> > > 
> > > +
> > > > > +	return imx_get_temp(thermal_data->tz, temp);
> > > 
> > > +}
> > > +
> > >  static int imx_get_mode(struct thermal_zone_device *tz,
> > > > >  			enum thermal_device_mode *mode)
> > > 
> > >  {
> > > @@ -482,6 +489,10 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> > > > >  	.set_trip_temp = imx_set_trip_temp,
> > > 
> > >  };
> > >  
> > > +static const struct thermal_zone_of_device_ops imx_tz_of_ops = {
> > > > > +	.get_temp = imx_of_sensor_get_temp,
> > > 
> > > +};
> > > +
> > >  static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
> > >  {
> > > > >  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> > > 
> > > @@ -798,6 +809,9 @@ static int imx_thermal_probe(struct platform_device *pdev)
> > > > > > > > > >  		return ret;
> > > > >  	}
> > > 
> > >  
> > > > > > > > > > +	data->sensor = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> > > > > +						       &imx_tz_of_ops);
> > > 
> > > +
> > > > > > > > > >  	dev_info(&pdev->dev, "%s CPU temperature grade - max:%dC"
> > > > > > > > > >  		 " critical:%dC passive:%dC\n", data->temp_grade,
> > > > >  		 data->temp_max / 1000, data->temp_critical / 1000,
> > > 
> > > @@ -848,6 +862,7 @@ static int imx_thermal_remove(struct platform_device *pdev)
> > > > > > > > > >  	if (!IS_ERR(data->thermal_clk))
> > > > >  		clk_disable_unprepare(data->thermal_clk);
> > > 
> > >  
> > > > > > > > > > +	thermal_zone_of_sensor_unregister(&pdev->dev, data->sensor);
> > > > > > > > > >  	thermal_zone_device_unregister(data->tz);
> > > > > > > > > >  	cpufreq_cooling_unregister(data->cdev);
> > > > >  	cpufreq_cpu_put(data->policy);
> > > 
> > > -- 
> > > 2.19.1
> > > 
> 
>

Patch
diff mbox series

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index aa452acb60b6..32f406099479 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -202,7 +202,7 @@  static struct thermal_soc_data thermal_imx7d_data = {
 
 struct imx_thermal_data {
 	struct cpufreq_policy *policy;
-	struct thermal_zone_device *tz;
+	struct thermal_zone_device *tz, *sensor;
 	struct thermal_cooling_device *cdev;
 	enum thermal_device_mode mode;
 	struct regmap *tempmon;
@@ -338,6 +338,13 @@  static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
 	return 0;
 }
 
+static int imx_of_sensor_get_temp(void *data, int *temp)
+{
+	struct imx_thermal_data *thermal_data = data;
+
+	return imx_get_temp(thermal_data->tz, temp);
+}
+
 static int imx_get_mode(struct thermal_zone_device *tz,
 			enum thermal_device_mode *mode)
 {
@@ -482,6 +489,10 @@  static struct thermal_zone_device_ops imx_tz_ops = {
 	.set_trip_temp = imx_set_trip_temp,
 };
 
+static const struct thermal_zone_of_device_ops imx_tz_of_ops = {
+	.get_temp = imx_of_sensor_get_temp,
+};
+
 static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
 {
 	struct imx_thermal_data *data = platform_get_drvdata(pdev);
@@ -798,6 +809,9 @@  static int imx_thermal_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	data->sensor = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
+						       &imx_tz_of_ops);
+
 	dev_info(&pdev->dev, "%s CPU temperature grade - max:%dC"
 		 " critical:%dC passive:%dC\n", data->temp_grade,
 		 data->temp_max / 1000, data->temp_critical / 1000,
@@ -848,6 +862,7 @@  static int imx_thermal_remove(struct platform_device *pdev)
 	if (!IS_ERR(data->thermal_clk))
 		clk_disable_unprepare(data->thermal_clk);
 
+	thermal_zone_of_sensor_unregister(&pdev->dev, data->sensor);
 	thermal_zone_device_unregister(data->tz);
 	cpufreq_cooling_unregister(data->cdev);
 	cpufreq_cpu_put(data->policy);