| Message ID | 20181105165134.28963-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
| Series | thermal: imx: register as OF sensor | expand |
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); >
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 >
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 > >
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 > > > > >
Hi Eduardo, Am Dienstag, den 12.02.2019, 11:19 +0100 schrieb Lucas Stach: > 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. Please let me know how we can move forward with this. This is blocking other patches and has been in limbo for quite a long time. 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 > > > > > > > > > >
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);
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(-)