Re: [PATCH 01/17] thermal: add thermal_zone_device_toggle() helper

[Eduardo Valentin]

On Tue, Apr 10, 2018 at 02:41:55PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Add thermal_zone_device_toggle() helper. Then update core code and
> drivers to use it.

Cool, but I think it would be good to have some sort of rational here
at the commit message telling why this helper is being added, 
don't you agree?

> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> ---
>  drivers/thermal/hisi_thermal.c     | 14 ++------------
>  drivers/thermal/of-thermal.c       |  2 +-
>  drivers/thermal/rockchip_thermal.c | 22 +++++-----------------
>  drivers/thermal/thermal_helpers.c  | 15 +++++++++++++++
>  drivers/thermal/thermal_sysfs.c    |  8 +++++---
>  include/linux/thermal.h            |  1 +
>  6 files changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index 761d055..9428499 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -515,15 +515,6 @@ static int hisi_thermal_register_sensor(struct platform_device *pdev,
>  };
>  MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>  
> -static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> -				       bool on)
> -{
> -	struct thermal_zone_device *tzd = sensor->tzd;
> -
> -	tzd->ops->set_mode(tzd,
> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> -}
> -
>  static int hisi_thermal_probe(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data;
> @@ -571,7 +562,7 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	hisi_thermal_toggle_sensor(&data->sensor, true);
> +	thermal_zone_device_toggle((&data->sensor)->tzd, true);
>  
>  	return 0;
>  }
> @@ -579,9 +570,8 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  static int hisi_thermal_remove(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
> -	struct hisi_thermal_sensor *sensor = &data->sensor;
>  
> -	hisi_thermal_toggle_sensor(sensor, false);
> +	thermal_zone_device_toggle((&data->sensor)->tzd, false);
>  
>  	data->disable_sensor(data);
>  
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index e09f035..f138b78 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -510,7 +510,7 @@ struct thermal_zone_device *
>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>  							 data, ops);
>  			if (!IS_ERR(tzd))
> -				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
> +				thermal_zone_device_toggle(tzd, true);
>  
>  			of_node_put(sensor_specs.np);
>  			of_node_put(child);
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index f36375d..c191e41 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -1022,15 +1022,6 @@ static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
>  };
>  MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
>  
> -static void
> -rockchip_thermal_toggle_sensor(struct rockchip_thermal_sensor *sensor, bool on)
> -{
> -	struct thermal_zone_device *tzd = sensor->tzd;
> -
> -	tzd->ops->set_mode(tzd,
> -		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> -}
> -
>  static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
>  {
>  	struct rockchip_thermal_data *thermal = dev;
> @@ -1292,7 +1283,7 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>  	thermal->chip->control(thermal->regs, true);
>  
>  	for (i = 0; i < thermal->chip->chn_num; i++)
> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, true);
>  
>  	platform_set_drvdata(pdev, thermal);
>  
> @@ -1311,11 +1302,8 @@ static int rockchip_thermal_remove(struct platform_device *pdev)
>  	struct rockchip_thermal_data *thermal = platform_get_drvdata(pdev);
>  	int i;
>  
> -	for (i = 0; i < thermal->chip->chn_num; i++) {
> -		struct rockchip_thermal_sensor *sensor = &thermal->sensors[i];
> -
> -		rockchip_thermal_toggle_sensor(sensor, false);
> -	}
> +	for (i = 0; i < thermal->chip->chn_num; i++)
> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, false);
>  
>  	thermal->chip->control(thermal->regs, false);
>  
> @@ -1332,7 +1320,7 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
>  	int i;
>  
>  	for (i = 0; i < thermal->chip->chn_num; i++)
> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], false);
> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, false);
>  
>  	thermal->chip->control(thermal->regs, false);
>  
> @@ -1383,7 +1371,7 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>  	thermal->chip->control(thermal->regs, true);
>  
>  	for (i = 0; i < thermal->chip->chn_num; i++)
> -		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
> +		thermal_zone_device_toggle((&thermal->sensors[i])->tzd, true);
>  
>  	pinctrl_pm_select_default_state(dev);
>  
> diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> index eb03d7e..d5db101 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -227,3 +227,18 @@ int thermal_zone_get_offset(struct thermal_zone_device *tz)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_offset);
> +
> +/**
> + * thermal_zone_device_toggle() - enables/disables thermal zone device
> + * @tz: a valid pointer to a struct thermal_zone_device
> + *
> + * Enables/Disables thermal zone device.
> + *
> + * Return: On success returns 0, an error code otherwise.
> + */
> +int thermal_zone_device_toggle(struct thermal_zone_device *tz, bool on)
> +{
> +	return tz->ops->set_mode(tz,
> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);

why not simply getting a mode type (enum thermal_device_mode)?

> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_device_toggle);

I know naming is always the hardest part :-), but to me, this function
is more a thermal_zone_set_mode() than device_toggle(). At least, based
on what I see in the code, we are not really toggling device per si,
just setting a mode.

> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 23b5e0a..343f52b 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -72,17 +72,19 @@
>  {
>  	struct thermal_zone_device *tz = to_thermal_zone(dev);
>  	int result;
> +	bool on;
>  
>  	if (!tz->ops->set_mode)
>  		return -EPERM;
>  
>  	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
> +		on = true;
>  	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
> -		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
> +		on = false;
>  	else
> -		result = -EINVAL;
> +		return -EINVAL;
>  
> +	result = thermal_zone_device_toggle(tz, on);
>  	if (result)
>  		return result;
>  
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 7834be6..4fbbabe 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -467,6 +467,7 @@ struct thermal_cooling_device *
>  int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>  int thermal_zone_get_slope(struct thermal_zone_device *tz);
>  int thermal_zone_get_offset(struct thermal_zone_device *tz);
> +int thermal_zone_device_toggle(struct thermal_zone_device *tz, bool on);
>  

You missed the #else section of the #if IS_ENABLED(CONFIG_THERMAL),
didn't you?

We want to have a stub when config thermal is not set.

>  int get_tz_trend(struct thermal_zone_device *, int);
>  struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
> -- 
> 1.9.1
>