[v3,09/12] thermal/core: Register with the trip points

As we added the thermal trip points structure in the thermal zone,
let's extend the thermal zone register function to have the thermal
trip structures as a parameter and store it in the 'trips' field of
the thermal zone structure.

Cc: Alexandre Bailon <abailon@baylibre.com>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc; Eduardo Valentin <eduval@amazon.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org>
---
 drivers/thermal/thermal_core.c | 22 +++++++++++++++++-----
 drivers/thermal/thermal_core.h |  4 ++--
 include/linux/thermal.h        |  6 ++++++
 3 files changed, 25 insertions(+), 7 deletions(-)

On 7/3/22 19:30, Daniel Lezcano wrote:
> As we added the thermal trip points structure in the thermal zone,
> let's extend the thermal zone register function to have the thermal
> trip structures as a parameter and store it in the 'trips' field of
> the thermal zone structure.
> 
> Cc: Alexandre Bailon <abailon@baylibre.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc; Eduardo Valentin <eduval@amazon.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org>
> ---
>   drivers/thermal/thermal_core.c | 22 +++++++++++++++++-----
>   drivers/thermal/thermal_core.h |  4 ++--
>   include/linux/thermal.h        |  6 ++++++
>   3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 434a675da245..e865c41d2320 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1167,8 +1167,9 @@ static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
>   }
>   
>   /**
> - * thermal_zone_device_register() - register a new thermal zone device
> + * thermal_zone_device_register_with_trips() - register a new thermal zone device
>    * @type:	the thermal zone device type
> + * @trips:	a pointer to an array of thermal trips
>    * @ntrips:	the number of trip points the thermal zone support
>    * @mask:	a bit string indicating the writeablility of trip points
>    * @devdata:	private device data
> @@ -1191,10 +1192,10 @@ static void thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
>    * IS_ERR*() helpers.
>    */
>   struct thermal_zone_device *
> -thermal_zone_device_register(const char *type, int ntrips, int mask,
> -			     void *devdata, struct thermal_zone_device_ops *ops,
> -			     struct thermal_zone_params *tzp, int passive_delay,
> -			     int polling_delay)
> +thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int ntrips, int mask,
> +					void *devdata, struct thermal_zone_device_ops *ops,
> +					struct thermal_zone_params *tzp, int passive_delay,
> +					int polling_delay)
>   {
>   	struct thermal_zone_device *tz;
>   	enum thermal_trip_type trip_type;
> @@ -1256,6 +1257,7 @@ thermal_zone_device_register(const char *type, int ntrips, int mask,
>   	tz->device.class = &thermal_class;
>   	tz->devdata = devdata;
>   	tz->ntrips = ntrips;
> +	tz->trips = trips;
>   
>   	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>   	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> @@ -1331,6 +1333,16 @@ thermal_zone_device_register(const char *type, int ntrips, int mask,
>   	kfree(tz);
>   	return ERR_PTR(result);
>   }
> +
> +struct thermal_zone_device *thermal_zone_device_register(const char *type, int ntrips, int mask,
> +							 void *devdata, struct thermal_zone_device_ops *ops,
> +							 struct thermal_zone_params *tzp, int passive_delay,
> +							 int polling_delay)
> +{
> +	return thermal_zone_device_register_with_trips(type, NULL, ntrips, mask,
> +						       devdata, ops, tzp,
> +						       passive_delay, polling_delay);
> +}
>   EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>   
>   /**
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 84e341c1e0fc..bbe3ec26d12e 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -113,12 +113,12 @@ int thermal_build_list_of_policies(char *buf);
>   /* Helpers */
>   void thermal_zone_set_trips(struct thermal_zone_device *tz);
>   
> -static inline struct thermal_trip *thermal_zone_get_trips(struct thermal_zone *tz)
> +static inline struct thermal_trip *thermal_zone_get_trips(struct thermal_zone_device *tz)
>   {
>   	return tz->trips;
>   }
>   
> -static inline int thermal_zone_get_ntrips(struct thermal_zone *tz)
> +static inline int thermal_zone_get_ntrips(struct thermal_zone_device *tz)
>   {
>   	return tz->ntrips;
>   }
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 3733e23b6359..8cbe237a92d0 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -379,8 +379,14 @@ void devm_thermal_zone_of_sensor_unregister(struct device *dev,
>   struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
>   		void *, struct thermal_zone_device_ops *,
>   		struct thermal_zone_params *, int, int);
> +
>   void thermal_zone_device_unregister(struct thermal_zone_device *);
>   
> +struct thermal_zone_device *
> +thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int, int,
> +					void *, struct thermal_zone_device_ops *,
> +					struct thermal_zone_params *, int, int);
> +
>   int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
>   				     struct thermal_cooling_device *,
>   				     unsigned long, unsigned long,

Apart from 'ntrips', which might be 'num_trips', this looks OK.

On Sun, 2022-07-03 at 20:30 +0200, Daniel Lezcano wrote:
> As we added the thermal trip points structure in the thermal zone,
> let's extend the thermal zone register function to have the thermal
> trip structures as a parameter and store it in the 'trips' field of
> the thermal zone structure.

Just FYI.

I proposed a small topic for this year' LPC about the
thermal_zone_device_register() parameters.
We have more and more parameters introduced, IMO, it's better to use a
unified structure for registration phase configurations, or reuse
struct thermal_zone_params.
In this way, when a new parameter is needed, we only need to introduce
a new field in the structure, rather than update every caller of this
API, or introduce new wrapper functions like we did in this patch.

thanks,
rui
> 
> Cc: Alexandre Bailon <abailon@baylibre.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc; Eduardo Valentin <eduval@amazon.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org>
> ---
>  drivers/thermal/thermal_core.c | 22 +++++++++++++++++-----
>  drivers/thermal/thermal_core.h |  4 ++--
>  include/linux/thermal.h        |  6 ++++++
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index 434a675da245..e865c41d2320 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1167,8 +1167,9 @@ static void thermal_set_delay_jiffies(unsigned
> long *delay_jiffies, int delay_ms
>  }
>  
>  /**
> - * thermal_zone_device_register() - register a new thermal zone
> device
> + * thermal_zone_device_register_with_trips() - register a new
> thermal zone device
>   * @type:	the thermal zone device type
> + * @trips:	a pointer to an array of thermal trips
>   * @ntrips:	the number of trip points the thermal zone support
>   * @mask:	a bit string indicating the writeablility of trip
> points
>   * @devdata:	private device data
> @@ -1191,10 +1192,10 @@ static void
> thermal_set_delay_jiffies(unsigned long *delay_jiffies, int delay_ms
>   * IS_ERR*() helpers.
>   */
>  struct thermal_zone_device *
> -thermal_zone_device_register(const char *type, int ntrips, int mask,
> -			     void *devdata, struct
> thermal_zone_device_ops *ops,
> -			     struct thermal_zone_params *tzp, int
> passive_delay,
> -			     int polling_delay)
> +thermal_zone_device_register_with_trips(const char *type, struct
> thermal_trip *trips, int ntrips, int mask,
> +					void *devdata, struct
> thermal_zone_device_ops *ops,
> +					struct thermal_zone_params
> *tzp, int passive_delay,
> +					int polling_delay)
>  {
>  	struct thermal_zone_device *tz;
>  	enum thermal_trip_type trip_type;
> @@ -1256,6 +1257,7 @@ thermal_zone_device_register(const char *type,
> int ntrips, int mask,
>  	tz->device.class = &thermal_class;
>  	tz->devdata = devdata;
>  	tz->ntrips = ntrips;
> +	tz->trips = trips;
>  
>  	thermal_set_delay_jiffies(&tz->passive_delay_jiffies,
> passive_delay);
>  	thermal_set_delay_jiffies(&tz->polling_delay_jiffies,
> polling_delay);
> @@ -1331,6 +1333,16 @@ thermal_zone_device_register(const char *type,
> int ntrips, int mask,
>  	kfree(tz);
>  	return ERR_PTR(result);
>  }
> +
> +struct thermal_zone_device *thermal_zone_device_register(const char
> *type, int ntrips, int mask,
> +							 void *devdata,
> struct thermal_zone_device_ops *ops,
> +							 struct
> thermal_zone_params *tzp, int passive_delay,
> +							 int
> polling_delay)
> +{
> +	return thermal_zone_device_register_with_trips(type, NULL,
> ntrips, mask,
> +						       devdata, ops,
> tzp,
> +						       passive_delay,
> polling_delay);
> +}
>  EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>  
>  /**
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 84e341c1e0fc..bbe3ec26d12e 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -113,12 +113,12 @@ int thermal_build_list_of_policies(char *buf);
>  /* Helpers */
>  void thermal_zone_set_trips(struct thermal_zone_device *tz);
>  
> -static inline struct thermal_trip *thermal_zone_get_trips(struct
> thermal_zone *tz)
> +static inline struct thermal_trip *thermal_zone_get_trips(struct
> thermal_zone_device *tz)
>  {
>  	return tz->trips;
>  }
>  
> -static inline int thermal_zone_get_ntrips(struct thermal_zone *tz)
> +static inline int thermal_zone_get_ntrips(struct thermal_zone_device
> *tz)
>  {
>  	return tz->ntrips;
>  }
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 3733e23b6359..8cbe237a92d0 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -379,8 +379,14 @@ void
> devm_thermal_zone_of_sensor_unregister(struct device *dev,
>  struct thermal_zone_device *thermal_zone_device_register(const char
> *, int, int,
>  		void *, struct thermal_zone_device_ops *,
>  		struct thermal_zone_params *, int, int);
> +
>  void thermal_zone_device_unregister(struct thermal_zone_device *);
>  
> +struct thermal_zone_device *
> +thermal_zone_device_register_with_trips(const char *, struct
> thermal_trip *, int, int,
> +					void *, struct
> thermal_zone_device_ops *,
> +					struct thermal_zone_params *,
> int, int);
> +
>  int thermal_zone_bind_cooling_device(struct thermal_zone_device *,
> int,
>  				     struct thermal_cooling_device *,
>  				     unsigned long, unsigned long,

On Tue, Jul 5, 2022 at 4:03 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Sun, 2022-07-03 at 20:30 +0200, Daniel Lezcano wrote:
> > As we added the thermal trip points structure in the thermal zone,
> > let's extend the thermal zone register function to have the thermal
> > trip structures as a parameter and store it in the 'trips' field of
> > the thermal zone structure.
>
> Just FYI.
>
> I proposed a small topic for this year' LPC about the
> thermal_zone_device_register() parameters.
> We have more and more parameters introduced, IMO, it's better to use a
> unified structure for registration phase configurations, or reuse
> struct thermal_zone_params.
> In this way, when a new parameter is needed, we only need to introduce
> a new field in the structure, rather than update every caller of this
> API, or introduce new wrapper functions like we did in this patch.

Sounds reasonable to me.