diff mbox series

[RFC,v2,4/9] thermal: core: Let thermal zone device's mode be stored in its struct

Message ID 20200414180105.20042-5-andrzej.p@collabora.com (mailing list archive)
State New, archived
Headers show
Series [RFC,v2,1/9] thermal: int3400_thermal: Statically initialize .get_mode()/.set_mode() ops | expand

Commit Message

Andrzej Pietrasiewicz April 14, 2020, 6:01 p.m. UTC 
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@collabora.com>
---
 drivers/thermal/thermal_core.c  | 28 +++++++++++++++++++
 drivers/thermal/thermal_sysfs.c |  9 +++----
 include/linux/thermal.h         | 48 +++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 6 deletions(-)

Comments

Daniel Lezcano April 14, 2020, 10:30 p.m. UTC | #1 
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@collabora.com>
> ---
>  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__ */
>
diff mbox series

Patch

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);
+
+	*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;
+
+	if (tz->ops->set_mode)
+		return tz->ops->set_mode(tz, mode);
+
+	tz->mode = mode;
+
+	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;
+}
+
 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__ */