On 14/04/2020 20:01, Andrzej Pietrasiewicz wrote:
> All the drivers which provide ->get_mode()/->set_mode() methods store their
> mode in a thermal_device_mode enum, so keep this information in struct
> thermal_zone_device rather than scattered all over the place.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
> ---
> drivers/thermal/thermal_core.c | 28 +++++++++++++++++++
> drivers/thermal/thermal_sysfs.c | 9 +++----
> include/linux/thermal.h | 48 +++++++++++++++++++++++++++++++++
> 3 files changed, 79 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 9a321dc548c8..cb0ff47f0dbe 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -469,6 +469,34 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz)
> thermal_zone_device_init(tz);
> }
>
> +int thermal_zone_device_get_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode *mode)
> +{
> + if (tz->ops->get_mode)
> + return tz->ops->get_mode(tz, mode);
I think we can get rid of the get_mode here.
locks missing.
and mode = tz->mode must be always set.
> + *mode = tz->mode;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_device_get_mode);
> +
> +int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode mode)
> +{
> + if (mode != THERMAL_DEVICE_DISABLED &&
> + mode != THERMAL_DEVICE_ENABLED)
> + return -EINVAL;
I'm not sure this is useful as 'mode' is an enum and this condition will
be always correct.
locks missing.
> + if (tz->ops->set_mode)
> + return tz->ops->set_mode(tz, mode);
> + tz->mode = mode;
It should be like:
int ret = 0;
mutex_lock(&tz->lock);
if (tz->ops->set_mode)
ret = tz->ops->set_mode(tz, mode);
*mode = tz->mode;
mutex_unlock(&tz->lock);
return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode);
> +
> void thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> {
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb4dff7..66d9691b8bd6 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -52,10 +52,7 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> enum thermal_device_mode mode;
> int result;
>
> - if (!tz->ops->get_mode)
> - return -EPERM;
> -
> - result = tz->ops->get_mode(tz, &mode);
> + result = thermal_zone_device_get_mode(tz, &mode);
> if (result)
> return result;
>
> @@ -74,9 +71,9 @@ mode_store(struct device *dev, struct device_attribute *attr,
> return -EPERM;
>
> if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
> - result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
> + result = thermal_zone_device_enable(tz);
> else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
> - result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
> + result = thermal_zone_device_disable(tz);
> else
> result = -EINVAL;
>
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index c91b1e344d56..9ff8542b7e7d 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -143,6 +143,7 @@ struct thermal_attr {
> * @trip_temp_attrs: attributes for trip points for sysfs: trip temperature
> * @trip_type_attrs: attributes for trip points for sysfs: trip type
> * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis
> + * @mode: current mode of this thermal zone
> * @devdata: private pointer for device private data
> * @trips: number of trip points the thermal zone supports
> * @trips_disabled; bitmap for disabled trips
> @@ -185,6 +186,7 @@ struct thermal_zone_device {
> struct thermal_attr *trip_temp_attrs;
> struct thermal_attr *trip_type_attrs;
> struct thermal_attr *trip_hyst_attrs;
> + enum thermal_device_mode mode;
> void *devdata;
> int trips;
> unsigned long trips_disabled; /* bitmap for disabled trips */
> @@ -437,6 +439,19 @@ int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
> unsigned int);
> int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
> struct thermal_cooling_device *);
> +
> +int thermal_zone_device_get_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode *mode);
> +int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode mode);
> +
> +static inline void
> +thermal_zone_device_store_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode mode)
> +{
> + tz->mode = mode;
> +}
> +
Please remove this store_mode function, it is not needed.
Just:
thermal_zone_device_get_mode()
thermal_zone_device_set_mode()
thermal_zone_device_disable()
thermal_zone_device_enable()
And all of them in drivers/thermal/thermal_core.h
> void thermal_zone_device_update(struct thermal_zone_device *,
> enum thermal_notify_event);
> void thermal_zone_set_trips(struct thermal_zone_device *);
> @@ -494,6 +509,17 @@ static inline int thermal_zone_unbind_cooling_device(
> struct thermal_zone_device *tz, int trip,
> struct thermal_cooling_device *cdev)
> { return -ENODEV; }
> +static inline int thermal_zone_device_get_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode *mode)
> +{ return -ENODEV; }
> +static inline int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode mode)
> +{ return -ENODEV; }
> +static inline void
> +thermal_zone_device_store_mode(struct thermal_zone_device *tz,
> + enum thermal_device_mode mode)
> +{ }
> +
> static inline void thermal_zone_device_update(struct thermal_zone_device *tz,
> enum thermal_notify_event event)
> { }
> @@ -543,4 +569,26 @@ static inline void thermal_notify_framework(struct thermal_zone_device *tz,
> { }
> #endif /* CONFIG_THERMAL */
>
> +static inline int thermal_zone_device_enable(struct thermal_zone_device *tz)
> +{
> + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED);
> +}
> +
> +static inline int thermal_zone_device_disable(struct thermal_zone_device *tz)
> +{
> + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED);
> +}
> +
> +static inline void
> +thermal_zone_device_store_enabled(struct thermal_zone_device *tz)
> +{
> + thermal_zone_device_store_mode(tz, THERMAL_DEVICE_ENABLED);
> +}
> +
> +static inline void
> +thermal_zone_device_store_disabled(struct thermal_zone_device *tz)
> +{
> + thermal_zone_device_store_mode(tz, THERMAL_DEVICE_DISABLED);
> +}
> +
> #endif /* __THERMAL_H__ */
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
