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.zolnier...@samsung.com>
> ---
> 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
>
