[RFC,01/26] thermal: Introduce thermal_zone_device_register() and params structure

  In preparation for extending the thermal zone devices to actually have
a name and disambiguation of thermal zone types/names, introduce a new
thermal_zone_device_params structure which holds all of the parameters
that are necessary to register a thermal zone device, then add a new
function thermal_zone_device_register().

The latter takes as parameter the newly introduced structure and is
made to eventually replace all usages of the now deprecated function
thermal_zone_device_register_with_trips() and of
thermal_tripless_zone_device_register().

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
 include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
  

On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
> In preparation for extending the thermal zone devices to actually have
> a name and disambiguation of thermal zone types/names, introduce a new
> thermal_zone_device_params structure which holds all of the parameters
> that are necessary to register a thermal zone device, then add a new
> function thermal_zone_device_register().
> 
> The latter takes as parameter the newly introduced structure and is
> made to eventually replace all usages of the now deprecated function
> thermal_zone_device_register_with_trips() and of
> thermal_tripless_zone_device_register().
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>   2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index e5434cdbf23b..6be508eb2d72 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>    *		   whether trip points have been crossed (0 for interrupt
>    *		   driven systems)
>    *
> + * This function is deprecated. See thermal_zone_device_register().
> + *
>    * This interface function adds a new thermal zone device (sensor) to
>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>    * thermal cooling devices registered at the same time.
> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *t
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>   
> +/* This function is deprecated. See thermal_zone_device_register(). */
>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>   					const char *type,
>   					void *devdata,
> @@ -1420,6 +1423,30 @@ struct thermal_zone_device *thermal_tripless_zone_device_register(
>   }
>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>   
> +/**
> + * thermal_zone_device_register() - register a new thermal zone device
> + * @tzdp:	Parameters of the new thermal zone device
> + *		See struct thermal_zone_device_register.
> + *
> + * This interface function adds a new thermal zone device (sensor) to
> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
> + * thermal cooling devices registered at the same time.
> + * thermal_zone_device_unregister() must be called when the device is no
> + * longer needed. The passive cooling depends on the .get_trend() return value.
> + *
> + * Return: a pointer to the created struct thermal_zone_device or an
> + * in case of error, an ERR_PTR. Caller must check return value with
> + * IS_ERR*() helpers.
> + */
> +struct thermal_zone_device *thermal_zone_device_register(struct thermal_zone_device_params *tzdp)
> +{
> +	return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, tzdp->num_trips,
> +						       tzdp->mask, tzdp->devdata, tzdp->ops,
> +						       &tzdp->tzp, tzdp->passive_delay,
> +						       tzdp->polling_delay);
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
> +
>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>   {
>   	return tzd->devdata;
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 98957bae08ff..c6ed33a7e468 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>   	int offset;
>   };
>   
> +/**
> + * struct thermal_zone_device_params - parameters for a thermal zone device
> + * @type:		the thermal zone device type
> + * @tzp:		thermal zone platform parameters
> + * @ops:		standard thermal zone device callbacks
> + * @devdata:		private device data
> + * @trips:		a pointer to an array of thermal trips, if any
> + * @num_trips:		the number of trip points the thermal zone support
> + * @mask:		a bit string indicating the writeablility of trip points
> + * @passive_delay:	number of milliseconds to wait between polls when
> + *			performing passive cooling
> + * @polling_delay:	number of milliseconds to wait between polls when checking
> + *			whether trip points have been crossed (0 for interrupt
> + *			driven systems)
> + */
> +struct thermal_zone_device_params {
> +	const char *type;
> +	struct thermal_zone_params tzp;
> +	struct thermal_zone_device_ops *ops;
> +	void *devdata;
> +	struct thermal_trip *trips;
> +	int num_trips;
> +	int mask;
> +	int passive_delay;
> +	int polling_delay;
> +};

 From my POV, this "struct thermal_zone_params" has been always a 
inadequate and catch-all structure. It will confuse with 
thermal_zone_device_params

I suggest to cleanup a bit that by sorting the parameters in the right 
structures where the result could be something like:

eg.

struct thermal_zone_params {

	const char *type;
	struct thermal_zone_device_ops *ops;
	struct thermal_trip *trips;
	int num_trips;

	int passive_delay;
	int polling_delay;
	
	void *devdata;
         bool no_hwmon;
};

struct thermal_governor_ipa_params {
         u32 sustainable_power;
         s32 k_po;
         s32 k_pu;
         s32 k_i;
         s32 k_d;
         s32 integral_cutoff;
         int slope;
         int offset;
};

struct thermal_governor_params {
	char governor_name[THERMAL_NAME_LENGTH];
	union {
		struct thermal_governor_ipa_params ipa_params;
	};
};

struct thermal_zone_device_params {
	struct thermal_zone_params *tzp;
	struct thermal_governor_params *tgp;
}

No functional changes just code reorg, being a series to be submitted 
before the rest on these RFC changes (2->26)

>   /* Function declarations */
>   #ifdef CONFIG_THERMAL_OF
>   struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
> @@ -310,6 +337,8 @@ struct thermal_zone_device *thermal_tripless_zone_device_register(
>   					struct thermal_zone_device_ops *ops,
>   					const struct thermal_zone_params *tzp);
>   
> +struct thermal_zone_device *thermal_zone_device_register(struct thermal_zone_device_params *tzdp);
> +
>   void thermal_zone_device_unregister(struct thermal_zone_device *tz);
>   
>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> @@ -372,6 +401,10 @@ static inline struct thermal_zone_device *thermal_tripless_zone_device_register(
>   					const struct thermal_zone_params *tzp)
>   { return ERR_PTR(-ENODEV); }
>   
> +static inline struct thermal_zone_device *thermal_zone_device_register(
> +					struct thermal_zone_device_params *tzdp)
> +{ return ERR_PTR(-ENODEV); }
> +
>   static inline void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>   { }
>
  

Il 15/01/24 13:39, Daniel Lezcano ha scritto:
> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>> In preparation for extending the thermal zone devices to actually have
>> a name and disambiguation of thermal zone types/names, introduce a new
>> thermal_zone_device_params structure which holds all of the parameters
>> that are necessary to register a thermal zone device, then add a new
>> function thermal_zone_device_register().
>>
>> The latter takes as parameter the newly introduced structure and is
>> made to eventually replace all usages of the now deprecated function
>> thermal_zone_device_register_with_trips() and of
>> thermal_tripless_zone_device_register().
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index e5434cdbf23b..6be508eb2d72 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>    *           whether trip points have been crossed (0 for interrupt
>>    *           driven systems)
>>    *
>> + * This function is deprecated. See thermal_zone_device_register().
>> + *
>>    * This interface function adds a new thermal zone device (sensor) to
>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>    * thermal cooling devices registered at the same time.
>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char *type, 
>> struct thermal_trip *t
>>   }
>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>                       const char *type,
>>                       void *devdata,
>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>> *thermal_tripless_zone_device_register(
>>   }
>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>> +/**
>> + * thermal_zone_device_register() - register a new thermal zone device
>> + * @tzdp:    Parameters of the new thermal zone device
>> + *        See struct thermal_zone_device_register.
>> + *
>> + * This interface function adds a new thermal zone device (sensor) to
>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>> + * thermal cooling devices registered at the same time.
>> + * thermal_zone_device_unregister() must be called when the device is no
>> + * longer needed. The passive cooling depends on the .get_trend() return value.
>> + *
>> + * Return: a pointer to the created struct thermal_zone_device or an
>> + * in case of error, an ERR_PTR. Caller must check return value with
>> + * IS_ERR*() helpers.
>> + */
>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>> thermal_zone_device_params *tzdp)
>> +{
>> +    return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, 
>> tzdp->num_trips,
>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>> +                               &tzdp->tzp, tzdp->passive_delay,
>> +                               tzdp->polling_delay);
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>> +
>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>   {
>>       return tzd->devdata;
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 98957bae08ff..c6ed33a7e468 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>       int offset;
>>   };
>> +/**
>> + * struct thermal_zone_device_params - parameters for a thermal zone device
>> + * @type:        the thermal zone device type
>> + * @tzp:        thermal zone platform parameters
>> + * @ops:        standard thermal zone device callbacks
>> + * @devdata:        private device data
>> + * @trips:        a pointer to an array of thermal trips, if any
>> + * @num_trips:        the number of trip points the thermal zone support
>> + * @mask:        a bit string indicating the writeablility of trip points
>> + * @passive_delay:    number of milliseconds to wait between polls when
>> + *            performing passive cooling
>> + * @polling_delay:    number of milliseconds to wait between polls when checking
>> + *            whether trip points have been crossed (0 for interrupt
>> + *            driven systems)
>> + */
>> +struct thermal_zone_device_params {
>> +    const char *type;
>> +    struct thermal_zone_params tzp;
>> +    struct thermal_zone_device_ops *ops;
>> +    void *devdata;
>> +    struct thermal_trip *trips;
>> +    int num_trips;
>> +    int mask;
>> +    int passive_delay;
>> +    int polling_delay;
>> +};
> 
>  From my POV, this "struct thermal_zone_params" has been always a inadequate and 
> catch-all structure. It will confuse with thermal_zone_device_params
> 
> I suggest to cleanup a bit that by sorting the parameters in the right structures 
> where the result could be something like:
> 
> eg.
> 
> struct thermal_zone_params {
> 
>      const char *type;
>      struct thermal_zone_device_ops *ops;
>      struct thermal_trip *trips;
>      int num_trips;
> 
>      int passive_delay;
>      int polling_delay;
> 
>      void *devdata;
>          bool no_hwmon;
> };
> 
> struct thermal_governor_ipa_params {
>          u32 sustainable_power;
>          s32 k_po;
>          s32 k_pu;
>          s32 k_i;
>          s32 k_d;
>          s32 integral_cutoff;
>          int slope;
>          int offset;
> };
> 
> struct thermal_governor_params {
>      char governor_name[THERMAL_NAME_LENGTH];
>      union {
>          struct thermal_governor_ipa_params ipa_params;
>      };
> };
> 
> struct thermal_zone_device_params {
>      struct thermal_zone_params *tzp;
>      struct thermal_governor_params *tgp;
> }
> 
> No functional changes just code reorg, being a series to be submitted before the 
> rest on these RFC changes (2->26)
> 

Could work. It's true that thermal_zone_params is a catch-all structure, and it's
not really the best... but I also haven't checked how complex and/or how much time
would your proposed change take.

Shouldn't take much as far as I can foresee, but I really have to check a bit.
If I'm right as in it's not something huge, the next series will directly have
this stuff sorted - if not, I'll reach to you.

Cheers,
Angelo

>>   /* Function declarations */
>>   #ifdef CONFIG_THERMAL_OF
>>   struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, 
>> int id, void *data,
>> @@ -310,6 +337,8 @@ struct thermal_zone_device 
>> *thermal_tripless_zone_device_register(
>>                       struct thermal_zone_device_ops *ops,
>>                       const struct thermal_zone_params *tzp);
>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>> thermal_zone_device_params *tzdp);
>> +
>>   void thermal_zone_device_unregister(struct thermal_zone_device *tz);
>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
>> @@ -372,6 +401,10 @@ static inline struct thermal_zone_device 
>> *thermal_tripless_zone_device_register(
>>                       const struct thermal_zone_params *tzp)
>>   { return ERR_PTR(-ENODEV); }
>> +static inline struct thermal_zone_device *thermal_zone_device_register(
>> +                    struct thermal_zone_device_params *tzdp)
>> +{ return ERR_PTR(-ENODEV); }
>> +
>>   static inline void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>>   { }
>
  

Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>> In preparation for extending the thermal zone devices to actually have
>>> a name and disambiguation of thermal zone types/names, introduce a new
>>> thermal_zone_device_params structure which holds all of the parameters
>>> that are necessary to register a thermal zone device, then add a new
>>> function thermal_zone_device_register().
>>>
>>> The latter takes as parameter the newly introduced structure and is
>>> made to eventually replace all usages of the now deprecated function
>>> thermal_zone_device_register_with_trips() and of
>>> thermal_tripless_zone_device_register().
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>>   2 files changed, 60 insertions(+)
>>>
>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>> index e5434cdbf23b..6be508eb2d72 100644
>>> --- a/drivers/thermal/thermal_core.c
>>> +++ b/drivers/thermal/thermal_core.c
>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>    *           whether trip points have been crossed (0 for interrupt
>>>    *           driven systems)
>>>    *
>>> + * This function is deprecated. See thermal_zone_device_register().
>>> + *
>>>    * This interface function adds a new thermal zone device (sensor) to
>>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>>    * thermal cooling devices registered at the same time.
>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char *type, 
>>> struct thermal_trip *t
>>>   }
>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>                       const char *type,
>>>                       void *devdata,
>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>>> *thermal_tripless_zone_device_register(
>>>   }
>>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>> +/**
>>> + * thermal_zone_device_register() - register a new thermal zone device
>>> + * @tzdp:    Parameters of the new thermal zone device
>>> + *        See struct thermal_zone_device_register.
>>> + *
>>> + * This interface function adds a new thermal zone device (sensor) to
>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>> + * thermal cooling devices registered at the same time.
>>> + * thermal_zone_device_unregister() must be called when the device is no
>>> + * longer needed. The passive cooling depends on the .get_trend() return value.
>>> + *
>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>> + * IS_ERR*() helpers.
>>> + */
>>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>>> thermal_zone_device_params *tzdp)
>>> +{
>>> +    return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, 
>>> tzdp->num_trips,
>>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>>> +                               &tzdp->tzp, tzdp->passive_delay,
>>> +                               tzdp->polling_delay);
>>> +}
>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>> +
>>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>   {
>>>       return tzd->devdata;
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index 98957bae08ff..c6ed33a7e468 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>       int offset;
>>>   };
>>> +/**
>>> + * struct thermal_zone_device_params - parameters for a thermal zone device
>>> + * @type:        the thermal zone device type
>>> + * @tzp:        thermal zone platform parameters
>>> + * @ops:        standard thermal zone device callbacks
>>> + * @devdata:        private device data
>>> + * @trips:        a pointer to an array of thermal trips, if any
>>> + * @num_trips:        the number of trip points the thermal zone support
>>> + * @mask:        a bit string indicating the writeablility of trip points
>>> + * @passive_delay:    number of milliseconds to wait between polls when
>>> + *            performing passive cooling
>>> + * @polling_delay:    number of milliseconds to wait between polls when checking
>>> + *            whether trip points have been crossed (0 for interrupt
>>> + *            driven systems)
>>> + */
>>> +struct thermal_zone_device_params {
>>> +    const char *type;
>>> +    struct thermal_zone_params tzp;
>>> +    struct thermal_zone_device_ops *ops;
>>> +    void *devdata;
>>> +    struct thermal_trip *trips;
>>> +    int num_trips;
>>> +    int mask;
>>> +    int passive_delay;
>>> +    int polling_delay;
>>> +};
>>
>>  From my POV, this "struct thermal_zone_params" has been always a inadequate and 
>> catch-all structure. It will confuse with thermal_zone_device_params
>>
>> I suggest to cleanup a bit that by sorting the parameters in the right structures 
>> where the result could be something like:
>>
>> eg.
>>
>> struct thermal_zone_params {
>>
>>      const char *type;
>>      struct thermal_zone_device_ops *ops;
>>      struct thermal_trip *trips;
>>      int num_trips;
>>
>>      int passive_delay;
>>      int polling_delay;
>>
>>      void *devdata;
>>          bool no_hwmon;
>> };
>>
>> struct thermal_governor_ipa_params {
>>          u32 sustainable_power;
>>          s32 k_po;
>>          s32 k_pu;
>>          s32 k_i;
>>          s32 k_d;
>>          s32 integral_cutoff;
>>          int slope;
>>          int offset;
>> };
>>
>> struct thermal_governor_params {
>>      char governor_name[THERMAL_NAME_LENGTH];
>>      union {
>>          struct thermal_governor_ipa_params ipa_params;
>>      };
>> };
>>
>> struct thermal_zone_device_params {
>>      struct thermal_zone_params *tzp;
>>      struct thermal_governor_params *tgp;
>> }
>>
>> No functional changes just code reorg, being a series to be submitted before the 
>> rest on these RFC changes (2->26)
>>
> 
> Could work. It's true that thermal_zone_params is a catch-all structure, and it's
> not really the best... but I also haven't checked how complex and/or how much time
> would your proposed change take.
> 
> Shouldn't take much as far as I can foresee, but I really have to check a bit.
> If I'm right as in it's not something huge, the next series will directly have
> this stuff sorted - if not, I'll reach to you.
> 

So... I've checked the situation and coded some.

I came to the conclusion that doing it as suggested is possible but realistically
suboptimal, because that will need multiple immutable branches to actually end up
in upstream as changes would otherwise break kernel build.

Then, here I am suggesting a different way forward, while still performing this
much needed cleanup and reorganization:

First, we introduce thermal_zone_device_register() and params structure with
this commit, which will have

/* Structure to define Thermal Zone parameters */
struct thermal_zone_params {
	/* Scheduled for removal - see struct thermal_zone_governor_params. */
	char governor_name[THERMAL_NAME_LENGTH];

	/* Scheduled for removal - see struct thermal_zone_governor_params. */
	bool no_hwmon;

	/*
	 * Sustainable power (heat) that this thermal zone can dissipate in
	 * mW
	 */
	u32 sustainable_power;

	/*
	 * Proportional parameter of the PID controller when
	 * overshooting (i.e., when temperature is below the target)
	 */
	s32 k_po;

	/*
	 * Proportional parameter of the PID controller when
	 * undershooting
	 */
	s32 k_pu;

	/* Integral parameter of the PID controller */
	s32 k_i;

	/* Derivative parameter of the PID controller */
	s32 k_d;

	/* threshold below which the error is no longer accumulated */
	s32 integral_cutoff;

	/*
	 * @slope:	slope of a linear temperature adjustment curve.
	 * 		Used by thermal zone drivers.
	 */
	int slope;
	/*
	 * @offset:	offset of a linear temperature adjustment curve.
	 * 		Used by thermal zone drivers (default 0).
	 */
	int offset;
};

struct thermal_governor_params {
	char governor_name[THERMAL_NAME_LENGTH];
	struct thermal_zone_params ipa_params;
};

struct thermal_zone_platform_params {
	const char *type;
	struct thermal_zone_device_ops *ops;
	struct thermal_trip *trips;
	int num_trips;
	int mask;

	int passive_delay;
	int polling_delay;

	void *devdata;
	bool no_hwmon;
};


struct thermal_zone_device_params {
	struct thermal_zone_platform_params tzp;
	struct thermal_governor_params *tgp;
};

(Note that the `no_hwmon` and `governor_name` are *temporarily* duplicated, but
there are good reasons to do that!)

Drivers will follow with the migration to `thermal_zone_device_register()`,
and that will be done *strictly* like so:

struct thermal_zone_device_params tzdp = {
	.tzp = {
		.type = "acerhdf",
		.tzp = { .governor_name = "bang_bang" },
		.ops = &acerhdf_dev_ops,
		.trips = trips,
		.num_trips = ARRAY_SIZE(trips),
		.polling_delay = kernelmode ? interval * 1000 : 0,
		/* devdata, no_hwmon go here later in the code */
	},
	.tgp = { .governor_name = "bang_bang" }
};

Notice how in this case we're never *ever* referencing to any struct name,
apart from struct thermal_zone_device_params (meaning, I'm never creating
vars/pointers to struct thermal_zone_platform_params).

If we follow this style, at least temporarily and at least during this cleanup,
we will end up with a plan such that:

1. In the first merge window:
    - Drivers get migrated to thermal_zone_device_register()
    - Functions register_with_trips()/tripless get deprecated but not yet removed

2. In the next merge window (expecting all users updated from the first window):
    - Functions register_with_trips/tripless get removed (<- no more external refs
      to struct thermal_zone_params, which can be then safely renamed!)
    - Duplicated members governor_name and no_hwmon get removed from
      struct thermal_zone_params
    - Some structures get renamed to give them the proposed better names (which
      I also genuinely like)
    - Only Thermal API internal changes, as users (all the ones that are not in
      drivers/thermal/) won't need to get updated at all!
      ... and that's why I said "strictly like so" in the example up there.

I think that this is the best strategy, for both ease of merging the changes and
internal reorganization.

Sure in the first merge window we'll be wasting a byte or two, but I am confident
in that this is a very low price to pay - and even better, only temporarily - for
other bigger benefits.

What do you think?

Cheers!
Angelo
  

Il 18/01/24 10:39, AngeloGioacchino Del Regno ha scritto:
> Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
>> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>>> In preparation for extending the thermal zone devices to actually have
>>>> a name and disambiguation of thermal zone types/names, introduce a new
>>>> thermal_zone_device_params structure which holds all of the parameters
>>>> that are necessary to register a thermal zone device, then add a new
>>>> function thermal_zone_device_register().
>>>>
>>>> The latter takes as parameter the newly introduced structure and is
>>>> made to eventually replace all usages of the now deprecated function
>>>> thermal_zone_device_register_with_trips() and of
>>>> thermal_tripless_zone_device_register().
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>>>   2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index e5434cdbf23b..6be508eb2d72 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>>    *           whether trip points have been crossed (0 for interrupt
>>>>    *           driven systems)
>>>>    *
>>>> + * This function is deprecated. See thermal_zone_device_register().
>>>> + *
>>>>    * This interface function adds a new thermal zone device (sensor) to
>>>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>>>    * thermal cooling devices registered at the same time.
>>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char *type, 
>>>> struct thermal_trip *t
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>>                       const char *type,
>>>>                       void *devdata,
>>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>>>> *thermal_tripless_zone_device_register(
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>>> +/**
>>>> + * thermal_zone_device_register() - register a new thermal zone device
>>>> + * @tzdp:    Parameters of the new thermal zone device
>>>> + *        See struct thermal_zone_device_register.
>>>> + *
>>>> + * This interface function adds a new thermal zone device (sensor) to
>>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>>> + * thermal cooling devices registered at the same time.
>>>> + * thermal_zone_device_unregister() must be called when the device is no
>>>> + * longer needed. The passive cooling depends on the .get_trend() return value.
>>>> + *
>>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>>> + * IS_ERR*() helpers.
>>>> + */
>>>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>>>> thermal_zone_device_params *tzdp)
>>>> +{
>>>> +    return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, 
>>>> tzdp->num_trips,
>>>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>>>> +                               &tzdp->tzp, tzdp->passive_delay,
>>>> +                               tzdp->polling_delay);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>>> +
>>>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>>   {
>>>>       return tzd->devdata;
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 98957bae08ff..c6ed33a7e468 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>>       int offset;
>>>>   };
>>>> +/**
>>>> + * struct thermal_zone_device_params - parameters for a thermal zone device
>>>> + * @type:        the thermal zone device type
>>>> + * @tzp:        thermal zone platform parameters
>>>> + * @ops:        standard thermal zone device callbacks
>>>> + * @devdata:        private device data
>>>> + * @trips:        a pointer to an array of thermal trips, if any
>>>> + * @num_trips:        the number of trip points the thermal zone support
>>>> + * @mask:        a bit string indicating the writeablility of trip points
>>>> + * @passive_delay:    number of milliseconds to wait between polls when
>>>> + *            performing passive cooling
>>>> + * @polling_delay:    number of milliseconds to wait between polls when checking
>>>> + *            whether trip points have been crossed (0 for interrupt
>>>> + *            driven systems)
>>>> + */
>>>> +struct thermal_zone_device_params {
>>>> +    const char *type;
>>>> +    struct thermal_zone_params tzp;
>>>> +    struct thermal_zone_device_ops *ops;
>>>> +    void *devdata;
>>>> +    struct thermal_trip *trips;
>>>> +    int num_trips;
>>>> +    int mask;
>>>> +    int passive_delay;
>>>> +    int polling_delay;
>>>> +};
>>>
>>>  From my POV, this "struct thermal_zone_params" has been always a inadequate and 
>>> catch-all structure. It will confuse with thermal_zone_device_params
>>>
>>> I suggest to cleanup a bit that by sorting the parameters in the right 
>>> structures where the result could be something like:
>>>
>>> eg.
>>>
>>> struct thermal_zone_params {
>>>
>>>      const char *type;
>>>      struct thermal_zone_device_ops *ops;
>>>      struct thermal_trip *trips;
>>>      int num_trips;
>>>
>>>      int passive_delay;
>>>      int polling_delay;
>>>
>>>      void *devdata;
>>>          bool no_hwmon;
>>> };
>>>
>>> struct thermal_governor_ipa_params {
>>>          u32 sustainable_power;
>>>          s32 k_po;
>>>          s32 k_pu;
>>>          s32 k_i;
>>>          s32 k_d;
>>>          s32 integral_cutoff;
>>>          int slope;
>>>          int offset;
>>> };
>>>
>>> struct thermal_governor_params {
>>>      char governor_name[THERMAL_NAME_LENGTH];
>>>      union {
>>>          struct thermal_governor_ipa_params ipa_params;
>>>      };
>>> };
>>>
>>> struct thermal_zone_device_params {
>>>      struct thermal_zone_params *tzp;
>>>      struct thermal_governor_params *tgp;
>>> }
>>>
>>> No functional changes just code reorg, being a series to be submitted before the 
>>> rest on these RFC changes (2->26)
>>>
>>
>> Could work. It's true that thermal_zone_params is a catch-all structure, and it's
>> not really the best... but I also haven't checked how complex and/or how much time
>> would your proposed change take.
>>
>> Shouldn't take much as far as I can foresee, but I really have to check a bit.
>> If I'm right as in it's not something huge, the next series will directly have
>> this stuff sorted - if not, I'll reach to you.
>>
> 
> So... I've checked the situation and coded some.
> 
> I came to the conclusion that doing it as suggested is possible but realistically
> suboptimal, because that will need multiple immutable branches to actually end up
> in upstream as changes would otherwise break kernel build.
> 
> Then, here I am suggesting a different way forward, while still performing this
> much needed cleanup and reorganization:
> 
> First, we introduce thermal_zone_device_register() and params structure with
> this commit, which will have
> 
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      char governor_name[THERMAL_NAME_LENGTH];
> 
>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      bool no_hwmon;
> 
>      /*
>       * Sustainable power (heat) that this thermal zone can dissipate in
>       * mW
>       */
>      u32 sustainable_power;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * overshooting (i.e., when temperature is below the target)
>       */
>      s32 k_po;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * undershooting
>       */
>      s32 k_pu;
> 
>      /* Integral parameter of the PID controller */
>      s32 k_i;
> 
>      /* Derivative parameter of the PID controller */
>      s32 k_d;
> 
>      /* threshold below which the error is no longer accumulated */
>      s32 integral_cutoff;
> 
>      /*
>       * @slope:    slope of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers.
>       */
>      int slope;
>      /*
>       * @offset:    offset of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers (default 0).
>       */
>      int offset;
> };
> 
> struct thermal_governor_params {
>      char governor_name[THERMAL_NAME_LENGTH];
>      struct thermal_zone_params ipa_params;
> };
> 
> struct thermal_zone_platform_params {
>      const char *type;
>      struct thermal_zone_device_ops *ops;
>      struct thermal_trip *trips;
>      int num_trips;
>      int mask;
> 
>      int passive_delay;
>      int polling_delay;
> 
>      void *devdata;
>      bool no_hwmon;
> };
> 
> 
> struct thermal_zone_device_params {
>      struct thermal_zone_platform_params tzp;
>      struct thermal_governor_params *tgp;
> };
> 
> (Note that the `no_hwmon` and `governor_name` are *temporarily* duplicated, but
> there are good reasons to do that!)
> 
> Drivers will follow with the migration to `thermal_zone_device_register()`,
> and that will be done *strictly* like so:
> 
> struct thermal_zone_device_params tzdp = {
>      .tzp = {
>          .type = "acerhdf",
>          .tzp = { .governor_name = "bang_bang" },

Whoops, sorry, this .tzp should've been removed, my bad!




>          .ops = &acerhdf_dev_ops,
>          .trips = trips,
>          .num_trips = ARRAY_SIZE(trips),
>          .polling_delay = kernelmode ? interval * 1000 : 0,
>          /* devdata, no_hwmon go here later in the code */
>      },
>      .tgp = { .governor_name = "bang_bang" }
> };

Looks like this instead:

  struct thermal_zone_device_params tzdp = {
       .tzp = {
           .type = "acerhdf",
           .ops = &acerhdf_dev_ops,
           .trips = trips,
           .num_trips = ARRAY_SIZE(trips),
           .polling_delay = kernelmode ? interval * 1000 : 0,
       },
       .tgp = { .governor_name = "bang_bang" }
};

> 
> Notice how in this case we're never *ever* referencing to any struct name,
> apart from struct thermal_zone_device_params (meaning, I'm never creating
> vars/pointers to struct thermal_zone_platform_params).
> 
> If we follow this style, at least temporarily and at least during this cleanup,
> we will end up with a plan such that:
> 
> 1. In the first merge window:
>     - Drivers get migrated to thermal_zone_device_register()
>     - Functions register_with_trips()/tripless get deprecated but not yet removed
> 
> 2. In the next merge window (expecting all users updated from the first window):
>     - Functions register_with_trips/tripless get removed (<- no more external refs
>       to struct thermal_zone_params, which can be then safely renamed!)
>     - Duplicated members governor_name and no_hwmon get removed from
>       struct thermal_zone_params
>     - Some structures get renamed to give them the proposed better names (which
>       I also genuinely like)
>     - Only Thermal API internal changes, as users (all the ones that are not in
>       drivers/thermal/) won't need to get updated at all!
>       ... and that's why I said "strictly like so" in the example up there.
> 
> I think that this is the best strategy, for both ease of merging the changes and
> internal reorganization.
> 
> Sure in the first merge window we'll be wasting a byte or two, but I am confident
> in that this is a very low price to pay - and even better, only temporarily - for
> other bigger benefits.
> 
> What do you think?
> 
> Cheers!
> Angelo
  

On 18/01/2024 10:39, AngeloGioacchino Del Regno wrote:
> Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
>> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>>> In preparation for extending the thermal zone devices to actually have
>>>> a name and disambiguation of thermal zone types/names, introduce a new
>>>> thermal_zone_device_params structure which holds all of the parameters
>>>> that are necessary to register a thermal zone device, then add a new
>>>> function thermal_zone_device_register().
>>>>
>>>> The latter takes as parameter the newly introduced structure and is
>>>> made to eventually replace all usages of the now deprecated function
>>>> thermal_zone_device_register_with_trips() and of
>>>> thermal_tripless_zone_device_register().
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>>>   2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c 
>>>> b/drivers/thermal/thermal_core.c
>>>> index e5434cdbf23b..6be508eb2d72 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>>    *           whether trip points have been crossed (0 for interrupt
>>>>    *           driven systems)
>>>>    *
>>>> + * This function is deprecated. See thermal_zone_device_register().
>>>> + *
>>>>    * This interface function adds a new thermal zone device (sensor) to
>>>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to 
>>>> bind all the
>>>>    * thermal cooling devices registered at the same time.
>>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const 
>>>> char *type, struct thermal_trip *t
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>>                       const char *type,
>>>>                       void *devdata,
>>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>>>> *thermal_tripless_zone_device_register(
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>>> +/**
>>>> + * thermal_zone_device_register() - register a new thermal zone device
>>>> + * @tzdp:    Parameters of the new thermal zone device
>>>> + *        See struct thermal_zone_device_register.
>>>> + *
>>>> + * This interface function adds a new thermal zone device (sensor) to
>>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind 
>>>> all the
>>>> + * thermal cooling devices registered at the same time.
>>>> + * thermal_zone_device_unregister() must be called when the device 
>>>> is no
>>>> + * longer needed. The passive cooling depends on the .get_trend() 
>>>> return value.
>>>> + *
>>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>>> + * IS_ERR*() helpers.
>>>> + */
>>>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>>>> thermal_zone_device_params *tzdp)
>>>> +{
>>>> +    return thermal_zone_device_register_with_trips(tzdp->type, 
>>>> tzdp->trips, tzdp->num_trips,
>>>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>>>> +                               &tzdp->tzp, tzdp->passive_delay,
>>>> +                               tzdp->polling_delay);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>>> +
>>>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>>   {
>>>>       return tzd->devdata;
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 98957bae08ff..c6ed33a7e468 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>>       int offset;
>>>>   };
>>>> +/**
>>>> + * struct thermal_zone_device_params - parameters for a thermal 
>>>> zone device
>>>> + * @type:        the thermal zone device type
>>>> + * @tzp:        thermal zone platform parameters
>>>> + * @ops:        standard thermal zone device callbacks
>>>> + * @devdata:        private device data
>>>> + * @trips:        a pointer to an array of thermal trips, if any
>>>> + * @num_trips:        the number of trip points the thermal zone 
>>>> support
>>>> + * @mask:        a bit string indicating the writeablility of trip 
>>>> points
>>>> + * @passive_delay:    number of milliseconds to wait between polls 
>>>> when
>>>> + *            performing passive cooling
>>>> + * @polling_delay:    number of milliseconds to wait between polls 
>>>> when checking
>>>> + *            whether trip points have been crossed (0 for interrupt
>>>> + *            driven systems)
>>>> + */
>>>> +struct thermal_zone_device_params {
>>>> +    const char *type;
>>>> +    struct thermal_zone_params tzp;
>>>> +    struct thermal_zone_device_ops *ops;
>>>> +    void *devdata;
>>>> +    struct thermal_trip *trips;
>>>> +    int num_trips;
>>>> +    int mask;
>>>> +    int passive_delay;
>>>> +    int polling_delay;
>>>> +};
>>>
>>>  From my POV, this "struct thermal_zone_params" has been always a 
>>> inadequate and catch-all structure. It will confuse with 
>>> thermal_zone_device_params
>>>
>>> I suggest to cleanup a bit that by sorting the parameters in the 
>>> right structures where the result could be something like:
>>>
>>> eg.
>>>
>>> struct thermal_zone_params {
>>>
>>>      const char *type;
>>>      struct thermal_zone_device_ops *ops;
>>>      struct thermal_trip *trips;
>>>      int num_trips;
>>>
>>>      int passive_delay;
>>>      int polling_delay;
>>>
>>>      void *devdata;
>>>          bool no_hwmon;
>>> };
>>>
>>> struct thermal_governor_ipa_params {
>>>          u32 sustainable_power;
>>>          s32 k_po;
>>>          s32 k_pu;
>>>          s32 k_i;
>>>          s32 k_d;
>>>          s32 integral_cutoff;
>>>          int slope;
>>>          int offset;
>>> };
>>>
>>> struct thermal_governor_params {
>>>      char governor_name[THERMAL_NAME_LENGTH];
>>>      union {
>>>          struct thermal_governor_ipa_params ipa_params;
>>>      };
>>> };
>>>
>>> struct thermal_zone_device_params {
>>>      struct thermal_zone_params *tzp;
>>>      struct thermal_governor_params *tgp;
>>> }
>>>
>>> No functional changes just code reorg, being a series to be submitted 
>>> before the rest on these RFC changes (2->26)
>>>
>>
>> Could work. It's true that thermal_zone_params is a catch-all 
>> structure, and it's
>> not really the best... but I also haven't checked how complex and/or 
>> how much time
>> would your proposed change take.
>>
>> Shouldn't take much as far as I can foresee, but I really have to 
>> check a bit.
>> If I'm right as in it's not something huge, the next series will 
>> directly have
>> this stuff sorted - if not, I'll reach to you.
>>
> 
> So... I've checked the situation and coded some.
> 
> I came to the conclusion that doing it as suggested is possible but 
> realistically
> suboptimal, because that will need multiple immutable branches to 
> actually end up
> in upstream as changes would otherwise break kernel build.
> 
> Then, here I am suggesting a different way forward, while still 
> performing this
> much needed cleanup and reorganization:
> 
> First, we introduce thermal_zone_device_register() and params structure 
> with
> this commit, which will have
> 
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      char governor_name[THERMAL_NAME_LENGTH];

Take the opportunity to introduce a patch before doing a change to:

	const char *governor_name;

AFAICS, there is no place in the kernel where it is not a const char * 
assignation which is by the way wrong:

	char governor_name[THERMAL_NAME_LENGTH];
	governor_name = "step_wise";

	:)

>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      bool no_hwmon;
> 
>      /*
>       * Sustainable power (heat) that this thermal zone can dissipate in
>       * mW
>       */
>      u32 sustainable_power;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * overshooting (i.e., when temperature is below the target)
>       */
>      s32 k_po;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * undershooting
>       */
>      s32 k_pu;
> 
>      /* Integral parameter of the PID controller */
>      s32 k_i;
> 
>      /* Derivative parameter of the PID controller */
>      s32 k_d;
> 
>      /* threshold below which the error is no longer accumulated */
>      s32 integral_cutoff;
> 
>      /*
>       * @slope:    slope of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers.
>       */
>      int slope;
>      /*
>       * @offset:    offset of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers (default 0).
>       */
>      int offset;
> };
> 
> struct thermal_governor_params {
>      char governor_name[THERMAL_NAME_LENGTH];

	const char *governor_name;

>      struct thermal_zone_params ipa_params;
> };
> 
> struct thermal_zone_platform_params {

The name sounds inadequate.

May be just thermal_zone_params ?

>      const char *type;
>      struct thermal_zone_device_ops *ops;

Move the ops in the thermal_zone_device_params

>      struct thermal_trip *trips;
>      int num_trips;
>      int mask;
> 
>      int passive_delay;
>      int polling_delay;
> 
>      void *devdata;

And devdata also in the thermal_zone_device_params

>      bool no_hwmon;
> };
> 
> 
> struct thermal_zone_device_params {
>      struct thermal_zone_platform_params tzp;
>      struct thermal_governor_params *tgp;
> };
> 
> (Note that the `no_hwmon` and `governor_name` are *temporarily* 
> duplicated, but
> there are good reasons to do that!)
> 
> Drivers will follow with the migration to `thermal_zone_device_register()`,
> and that will be done *strictly* like so:
> 
> struct thermal_zone_device_params tzdp = {
>      .tzp = {
>          .type = "acerhdf",
>          .tzp = { .governor_name = "bang_bang" },
>          .ops = &acerhdf_dev_ops,
>          .trips = trips,
>          .num_trips = ARRAY_SIZE(trips),
>          .polling_delay = kernelmode ? interval * 1000 : 0,
>          /* devdata, no_hwmon go here later in the code */
>      },
>      .tgp = { .governor_name = "bang_bang" }
> };
> 
> Notice how in this case we're never *ever* referencing to any struct name,
> apart from struct thermal_zone_device_params (meaning, I'm never creating
> vars/pointers to struct thermal_zone_platform_params).
> 
> If we follow this style, at least temporarily and at least during this 
> cleanup,
> we will end up with a plan such that:
> 
> 1. In the first merge window:
>     - Drivers get migrated to thermal_zone_device_register()
>     - Functions register_with_trips()/tripless get deprecated but not 
> yet removed
 >
> 2. In the next merge window (expecting all users updated from the first 
> window):
>     - Functions register_with_trips/tripless get removed (<- no more 
> external refs
>       to struct thermal_zone_params, which can be then safely renamed!)
>     - Duplicated members governor_name and no_hwmon get removed from
>       struct thermal_zone_params
>     - Some structures get renamed to give them the proposed better names 
> (which
>       I also genuinely like)
>     - Only Thermal API internal changes, as users (all the ones that are 
> not in
>       drivers/thermal/) won't need to get updated at all!
>       ... and that's why I said "strictly like so" in the example up there.
> 
> I think that this is the best strategy, for both ease of merging the 
> changes and
> internal reorganization.
> 
> Sure in the first merge window we'll be wasting a byte or two, but I am 
> confident
> in that this is a very low price to pay - and even better, only 
> temporarily - for
> other bigger benefits.
> 
> What do you think?

Sounds like a plan :)
  

On 18/01/2024 10:39, AngeloGioacchino Del Regno wrote:
> Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
>> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>>> In preparation for extending the thermal zone devices to actually have
>>>> a name and disambiguation of thermal zone types/names, introduce a new
>>>> thermal_zone_device_params structure which holds all of the parameters
>>>> that are necessary to register a thermal zone device, then add a new
>>>> function thermal_zone_device_register().
>>>>
>>>> The latter takes as parameter the newly introduced structure and is
>>>> made to eventually replace all usages of the now deprecated function
>>>> thermal_zone_device_register_with_trips() and of
>>>> thermal_tripless_zone_device_register().
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>> <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>>>   2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c 
>>>> b/drivers/thermal/thermal_core.c
>>>> index e5434cdbf23b..6be508eb2d72 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>>    *           whether trip points have been crossed (0 for interrupt
>>>>    *           driven systems)
>>>>    *
>>>> + * This function is deprecated. See thermal_zone_device_register().
>>>> + *
>>>>    * This interface function adds a new thermal zone device (sensor) to
>>>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to 
>>>> bind all the
>>>>    * thermal cooling devices registered at the same time.
>>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const 
>>>> char *type, struct thermal_trip *t
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>>                       const char *type,
>>>>                       void *devdata,
>>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>>>> *thermal_tripless_zone_device_register(
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>>> +/**
>>>> + * thermal_zone_device_register() - register a new thermal zone device
>>>> + * @tzdp:    Parameters of the new thermal zone device
>>>> + *        See struct thermal_zone_device_register.
>>>> + *
>>>> + * This interface function adds a new thermal zone device (sensor) to
>>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind 
>>>> all the
>>>> + * thermal cooling devices registered at the same time.
>>>> + * thermal_zone_device_unregister() must be called when the device 
>>>> is no
>>>> + * longer needed. The passive cooling depends on the .get_trend() 
>>>> return value.
>>>> + *
>>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>>> + * IS_ERR*() helpers.
>>>> + */
>>>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>>>> thermal_zone_device_params *tzdp)
>>>> +{
>>>> +    return thermal_zone_device_register_with_trips(tzdp->type, 
>>>> tzdp->trips, tzdp->num_trips,
>>>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>>>> +                               &tzdp->tzp, tzdp->passive_delay,
>>>> +                               tzdp->polling_delay);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>>> +
>>>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>>   {
>>>>       return tzd->devdata;
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 98957bae08ff..c6ed33a7e468 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>>       int offset;
>>>>   };
>>>> +/**
>>>> + * struct thermal_zone_device_params - parameters for a thermal 
>>>> zone device
>>>> + * @type:        the thermal zone device type
>>>> + * @tzp:        thermal zone platform parameters
>>>> + * @ops:        standard thermal zone device callbacks
>>>> + * @devdata:        private device data
>>>> + * @trips:        a pointer to an array of thermal trips, if any
>>>> + * @num_trips:        the number of trip points the thermal zone 
>>>> support
>>>> + * @mask:        a bit string indicating the writeablility of trip 
>>>> points
>>>> + * @passive_delay:    number of milliseconds to wait between polls 
>>>> when
>>>> + *            performing passive cooling
>>>> + * @polling_delay:    number of milliseconds to wait between polls 
>>>> when checking
>>>> + *            whether trip points have been crossed (0 for interrupt
>>>> + *            driven systems)
>>>> + */
>>>> +struct thermal_zone_device_params {
>>>> +    const char *type;
>>>> +    struct thermal_zone_params tzp;
>>>> +    struct thermal_zone_device_ops *ops;
>>>> +    void *devdata;
>>>> +    struct thermal_trip *trips;
>>>> +    int num_trips;
>>>> +    int mask;
>>>> +    int passive_delay;
>>>> +    int polling_delay;
>>>> +};
>>>
>>>  From my POV, this "struct thermal_zone_params" has been always a 
>>> inadequate and catch-all structure. It will confuse with 
>>> thermal_zone_device_params
>>>
>>> I suggest to cleanup a bit that by sorting the parameters in the 
>>> right structures where the result could be something like:
>>>
>>> eg.
>>>
>>> struct thermal_zone_params {
>>>
>>>      const char *type;
>>>      struct thermal_zone_device_ops *ops;
>>>      struct thermal_trip *trips;
>>>      int num_trips;
>>>
>>>      int passive_delay;
>>>      int polling_delay;
>>>
>>>      void *devdata;
>>>          bool no_hwmon;
>>> };
>>>
>>> struct thermal_governor_ipa_params {
>>>          u32 sustainable_power;
>>>          s32 k_po;
>>>          s32 k_pu;
>>>          s32 k_i;
>>>          s32 k_d;
>>>          s32 integral_cutoff;
>>>          int slope;
>>>          int offset;
>>> };
>>>
>>> struct thermal_governor_params {
>>>      char governor_name[THERMAL_NAME_LENGTH];
>>>      union {
>>>          struct thermal_governor_ipa_params ipa_params;
>>>      };
>>> };
>>>
>>> struct thermal_zone_device_params {
>>>      struct thermal_zone_params *tzp;
>>>      struct thermal_governor_params *tgp;
>>> }
>>>
>>> No functional changes just code reorg, being a series to be submitted 
>>> before the rest on these RFC changes (2->26)
>>>
>>
>> Could work. It's true that thermal_zone_params is a catch-all 
>> structure, and it's
>> not really the best... but I also haven't checked how complex and/or 
>> how much time
>> would your proposed change take.
>>
>> Shouldn't take much as far as I can foresee, but I really have to 
>> check a bit.
>> If I'm right as in it's not something huge, the next series will 
>> directly have
>> this stuff sorted - if not, I'll reach to you.
>>
> 
> So... I've checked the situation and coded some.
> 
> I came to the conclusion that doing it as suggested is possible but 
> realistically
> suboptimal, because that will need multiple immutable branches to 
> actually end up
> in upstream as changes would otherwise break kernel build.
> 
> Then, here I am suggesting a different way forward, while still 
> performing this
> much needed cleanup and reorganization:
> 
> First, we introduce thermal_zone_device_register() and params structure 
> with
> this commit, which will have
> 
> /* Structure to define Thermal Zone parameters */
> struct thermal_zone_params {
>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      char governor_name[THERMAL_NAME_LENGTH];

Take the opportunity to introduce a patch before doing a change to:

	const char *governor_name;

AFAICS, there is no place in the kernel where it is not a const char * 
assignation which is by the way wrong:

	char governor_name[THERMAL_NAME_LENGTH];
	governor_name = "step_wise";

	:)

>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>      bool no_hwmon;
> 
>      /*
>       * Sustainable power (heat) that this thermal zone can dissipate in
>       * mW
>       */
>      u32 sustainable_power;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * overshooting (i.e., when temperature is below the target)
>       */
>      s32 k_po;
> 
>      /*
>       * Proportional parameter of the PID controller when
>       * undershooting
>       */
>      s32 k_pu;
> 
>      /* Integral parameter of the PID controller */
>      s32 k_i;
> 
>      /* Derivative parameter of the PID controller */
>      s32 k_d;
> 
>      /* threshold below which the error is no longer accumulated */
>      s32 integral_cutoff;
> 
>      /*
>       * @slope:    slope of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers.
>       */
>      int slope;
>      /*
>       * @offset:    offset of a linear temperature adjustment curve.
>       *         Used by thermal zone drivers (default 0).
>       */
>      int offset;
> };
> 
> struct thermal_governor_params {
>      char governor_name[THERMAL_NAME_LENGTH];

	const char *governor_name;

>      struct thermal_zone_params ipa_params;
> };
> 
> struct thermal_zone_platform_params {

The name sounds inadequate.

May be just thermal_zone_params ?

>      const char *type;
>      struct thermal_zone_device_ops *ops;

Move the ops in the thermal_zone_device_params

>      struct thermal_trip *trips;
>      int num_trips;
>      int mask;
> 
>      int passive_delay;
>      int polling_delay;
> 
>      void *devdata;

And devdata also in the thermal_zone_device_params

>      bool no_hwmon;
> };
> 
> 
> struct thermal_zone_device_params {
>      struct thermal_zone_platform_params tzp;
>      struct thermal_governor_params *tgp;
> };
> 
> (Note that the `no_hwmon` and `governor_name` are *temporarily* 
> duplicated, but
> there are good reasons to do that!)
> 
> Drivers will follow with the migration to `thermal_zone_device_register()`,
> and that will be done *strictly* like so:
> 
> struct thermal_zone_device_params tzdp = {
>      .tzp = {
>          .type = "acerhdf",
>          .tzp = { .governor_name = "bang_bang" },
>          .ops = &acerhdf_dev_ops,
>          .trips = trips,
>          .num_trips = ARRAY_SIZE(trips),
>          .polling_delay = kernelmode ? interval * 1000 : 0,
>          /* devdata, no_hwmon go here later in the code */
>      },
>      .tgp = { .governor_name = "bang_bang" }
> };
> 
> Notice how in this case we're never *ever* referencing to any struct name,
> apart from struct thermal_zone_device_params (meaning, I'm never creating
> vars/pointers to struct thermal_zone_platform_params).
> 
> If we follow this style, at least temporarily and at least during this 
> cleanup,
> we will end up with a plan such that:
> 
> 1. In the first merge window:
>     - Drivers get migrated to thermal_zone_device_register()
>     - Functions register_with_trips()/tripless get deprecated but not 
> yet removed
 >
> 2. In the next merge window (expecting all users updated from the first 
> window):
>     - Functions register_with_trips/tripless get removed (<- no more 
> external refs
>       to struct thermal_zone_params, which can be then safely renamed!)
>     - Duplicated members governor_name and no_hwmon get removed from
>       struct thermal_zone_params
>     - Some structures get renamed to give them the proposed better names 
> (which
>       I also genuinely like)
>     - Only Thermal API internal changes, as users (all the ones that are 
> not in
>       drivers/thermal/) won't need to get updated at all!
>       ... and that's why I said "strictly like so" in the example up there.
> 
> I think that this is the best strategy, for both ease of merging the 
> changes and
> internal reorganization.
> 
> Sure in the first merge window we'll be wasting a byte or two, but I am 
> confident
> in that this is a very low price to pay - and even better, only 
> temporarily - for
> other bigger benefits.
> 
> What do you think?

Sounds like a plan :)
  

Il 22/01/24 20:19, Daniel Lezcano ha scritto:
> On 18/01/2024 10:39, AngeloGioacchino Del Regno wrote:
>> Il 16/01/24 10:58, AngeloGioacchino Del Regno ha scritto:
>>> Il 15/01/24 13:39, Daniel Lezcano ha scritto:
>>>> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
>>>>> In preparation for extending the thermal zone devices to actually have
>>>>> a name and disambiguation of thermal zone types/names, introduce a new
>>>>> thermal_zone_device_params structure which holds all of the parameters
>>>>> that are necessary to register a thermal zone device, then add a new
>>>>> function thermal_zone_device_register().
>>>>>
>>>>> The latter takes as parameter the newly introduced structure and is
>>>>> made to eventually replace all usages of the now deprecated function
>>>>> thermal_zone_device_register_with_trips() and of
>>>>> thermal_tripless_zone_device_register().
>>>>>
>>>>> Signed-off-by: AngeloGioacchino Del Regno 
>>>>> <angelogioacchino.delregno@collabora.com>
>>>>> ---
>>>>>   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
>>>>>   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>>> index e5434cdbf23b..6be508eb2d72 100644
>>>>> --- a/drivers/thermal/thermal_core.c
>>>>> +++ b/drivers/thermal/thermal_core.c
>>>>> @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>>>>>    *           whether trip points have been crossed (0 for interrupt
>>>>>    *           driven systems)
>>>>>    *
>>>>> + * This function is deprecated. See thermal_zone_device_register().
>>>>> + *
>>>>>    * This interface function adds a new thermal zone device (sensor) to
>>>>>    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>>>>    * thermal cooling devices registered at the same time.
>>>>> @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char 
>>>>> *type, struct thermal_trip *t
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>>>> +/* This function is deprecated. See thermal_zone_device_register(). */
>>>>>   struct thermal_zone_device *thermal_tripless_zone_device_register(
>>>>>                       const char *type,
>>>>>                       void *devdata,
>>>>> @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
>>>>> *thermal_tripless_zone_device_register(
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
>>>>> +/**
>>>>> + * thermal_zone_device_register() - register a new thermal zone device
>>>>> + * @tzdp:    Parameters of the new thermal zone device
>>>>> + *        See struct thermal_zone_device_register.
>>>>> + *
>>>>> + * This interface function adds a new thermal zone device (sensor) to
>>>>> + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
>>>>> + * thermal cooling devices registered at the same time.
>>>>> + * thermal_zone_device_unregister() must be called when the device is no
>>>>> + * longer needed. The passive cooling depends on the .get_trend() return value.
>>>>> + *
>>>>> + * Return: a pointer to the created struct thermal_zone_device or an
>>>>> + * in case of error, an ERR_PTR. Caller must check return value with
>>>>> + * IS_ERR*() helpers.
>>>>> + */
>>>>> +struct thermal_zone_device *thermal_zone_device_register(struct 
>>>>> thermal_zone_device_params *tzdp)
>>>>> +{
>>>>> +    return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, 
>>>>> tzdp->num_trips,
>>>>> +                               tzdp->mask, tzdp->devdata, tzdp->ops,
>>>>> +                               &tzdp->tzp, tzdp->passive_delay,
>>>>> +                               tzdp->polling_delay);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
>>>>> +
>>>>>   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
>>>>>   {
>>>>>       return tzd->devdata;
>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>> index 98957bae08ff..c6ed33a7e468 100644
>>>>> --- a/include/linux/thermal.h
>>>>> +++ b/include/linux/thermal.h
>>>>> @@ -258,6 +258,33 @@ struct thermal_zone_params {
>>>>>       int offset;
>>>>>   };
>>>>> +/**
>>>>> + * struct thermal_zone_device_params - parameters for a thermal zone device
>>>>> + * @type:        the thermal zone device type
>>>>> + * @tzp:        thermal zone platform parameters
>>>>> + * @ops:        standard thermal zone device callbacks
>>>>> + * @devdata:        private device data
>>>>> + * @trips:        a pointer to an array of thermal trips, if any
>>>>> + * @num_trips:        the number of trip points the thermal zone support
>>>>> + * @mask:        a bit string indicating the writeablility of trip points
>>>>> + * @passive_delay:    number of milliseconds to wait between polls when
>>>>> + *            performing passive cooling
>>>>> + * @polling_delay:    number of milliseconds to wait between polls when checking
>>>>> + *            whether trip points have been crossed (0 for interrupt
>>>>> + *            driven systems)
>>>>> + */
>>>>> +struct thermal_zone_device_params {
>>>>> +    const char *type;
>>>>> +    struct thermal_zone_params tzp;
>>>>> +    struct thermal_zone_device_ops *ops;
>>>>> +    void *devdata;
>>>>> +    struct thermal_trip *trips;
>>>>> +    int num_trips;
>>>>> +    int mask;
>>>>> +    int passive_delay;
>>>>> +    int polling_delay;
>>>>> +};
>>>>
>>>>  From my POV, this "struct thermal_zone_params" has been always a inadequate 
>>>> and catch-all structure. It will confuse with thermal_zone_device_params
>>>>
>>>> I suggest to cleanup a bit that by sorting the parameters in the right 
>>>> structures where the result could be something like:
>>>>
>>>> eg.
>>>>
>>>> struct thermal_zone_params {
>>>>
>>>>      const char *type;
>>>>      struct thermal_zone_device_ops *ops;
>>>>      struct thermal_trip *trips;
>>>>      int num_trips;
>>>>
>>>>      int passive_delay;
>>>>      int polling_delay;
>>>>
>>>>      void *devdata;
>>>>          bool no_hwmon;
>>>> };
>>>>
>>>> struct thermal_governor_ipa_params {
>>>>          u32 sustainable_power;
>>>>          s32 k_po;
>>>>          s32 k_pu;
>>>>          s32 k_i;
>>>>          s32 k_d;
>>>>          s32 integral_cutoff;
>>>>          int slope;
>>>>          int offset;
>>>> };
>>>>
>>>> struct thermal_governor_params {
>>>>      char governor_name[THERMAL_NAME_LENGTH];
>>>>      union {
>>>>          struct thermal_governor_ipa_params ipa_params;
>>>>      };
>>>> };
>>>>
>>>> struct thermal_zone_device_params {
>>>>      struct thermal_zone_params *tzp;
>>>>      struct thermal_governor_params *tgp;
>>>> }
>>>>
>>>> No functional changes just code reorg, being a series to be submitted before 
>>>> the rest on these RFC changes (2->26)
>>>>
>>>
>>> Could work. It's true that thermal_zone_params is a catch-all structure, and it's
>>> not really the best... but I also haven't checked how complex and/or how much time
>>> would your proposed change take.
>>>
>>> Shouldn't take much as far as I can foresee, but I really have to check a bit.
>>> If I'm right as in it's not something huge, the next series will directly have
>>> this stuff sorted - if not, I'll reach to you.
>>>
>>
>> So... I've checked the situation and coded some.
>>
>> I came to the conclusion that doing it as suggested is possible but realistically
>> suboptimal, because that will need multiple immutable branches to actually end up
>> in upstream as changes would otherwise break kernel build.
>>
>> Then, here I am suggesting a different way forward, while still performing this
>> much needed cleanup and reorganization:
>>
>> First, we introduce thermal_zone_device_register() and params structure with
>> this commit, which will have
>>
>> /* Structure to define Thermal Zone parameters */
>> struct thermal_zone_params {
>>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>>      char governor_name[THERMAL_NAME_LENGTH];
> 
> Take the opportunity to introduce a patch before doing a change to:
> 
>      const char *governor_name;
> 

Agreed!

> AFAICS, there is no place in the kernel where it is not a const char * assignation 
> which is by the way wrong:
> 
>      char governor_name[THERMAL_NAME_LENGTH];
>      governor_name = "step_wise";
> 
>      :)
> 
>>      /* Scheduled for removal - see struct thermal_zone_governor_params. */
>>      bool no_hwmon;
>>
>>      /*
>>       * Sustainable power (heat) that this thermal zone can dissipate in
>>       * mW
>>       */
>>      u32 sustainable_power;
>>
>>      /*
>>       * Proportional parameter of the PID controller when
>>       * overshooting (i.e., when temperature is below the target)
>>       */
>>      s32 k_po;
>>
>>      /*
>>       * Proportional parameter of the PID controller when
>>       * undershooting
>>       */
>>      s32 k_pu;
>>
>>      /* Integral parameter of the PID controller */
>>      s32 k_i;
>>
>>      /* Derivative parameter of the PID controller */
>>      s32 k_d;
>>
>>      /* threshold below which the error is no longer accumulated */
>>      s32 integral_cutoff;
>>
>>      /*
>>       * @slope:    slope of a linear temperature adjustment curve.
>>       *         Used by thermal zone drivers.
>>       */
>>      int slope;
>>      /*
>>       * @offset:    offset of a linear temperature adjustment curve.
>>       *         Used by thermal zone drivers (default 0).
>>       */
>>      int offset;
>> };
>>
>> struct thermal_governor_params {
>>      char governor_name[THERMAL_NAME_LENGTH];
> 
>      const char *governor_name;
> 
>>      struct thermal_zone_params ipa_params;
>> };
>>
>> struct thermal_zone_platform_params {
> 
> The name sounds inadequate.
> 
> May be just thermal_zone_params ?
> 

It's not the best, but the plan is to change the struct name in the second cycle,
as keeping the thermal_zone_params struct named as it is right now is essential
to avoid immutable branches.

window 1: Reorganization with temporary struct names -> no immutable branches
window 2: Cleanup and rename -> no immutable branches

Any change from the window 2 part brought to window 1 would need immutable
branches all around... so I really can't call it "thermal_zone_params" in
window 1.

Perhaps I can change the _platform_ name to something else, but still needs
to be different from "thermal_zone_params"...

>>      const char *type;
>>      struct thermal_zone_device_ops *ops;
> 
> Move the ops in the thermal_zone_device_params
> 
>>      struct thermal_trip *trips;
>>      int num_trips;
>>      int mask;
>>
>>      int passive_delay;
>>      int polling_delay;
>>
>>      void *devdata;
> 
> And devdata also in the thermal_zone_device_params
> 

Ok! :-)

>>      bool no_hwmon;
>> };
>>
>>
>> struct thermal_zone_device_params {
>>      struct thermal_zone_platform_params tzp;
>>      struct thermal_governor_params *tgp;
>> };
>>
>> (Note that the `no_hwmon` and `governor_name` are *temporarily* duplicated, but
>> there are good reasons to do that!)
>>
>> Drivers will follow with the migration to `thermal_zone_device_register()`,
>> and that will be done *strictly* like so:
>>
>> struct thermal_zone_device_params tzdp = {
>>      .tzp = {
>>          .type = "acerhdf",
>>          .tzp = { .governor_name = "bang_bang" },
>>          .ops = &acerhdf_dev_ops,
>>          .trips = trips,
>>          .num_trips = ARRAY_SIZE(trips),
>>          .polling_delay = kernelmode ? interval * 1000 : 0,
>>          /* devdata, no_hwmon go here later in the code */
>>      },
>>      .tgp = { .governor_name = "bang_bang" }
>> };
>>
>> Notice how in this case we're never *ever* referencing to any struct name,
>> apart from struct thermal_zone_device_params (meaning, I'm never creating
>> vars/pointers to struct thermal_zone_platform_params).
>>
>> If we follow this style, at least temporarily and at least during this cleanup,
>> we will end up with a plan such that:
>>
>> 1. In the first merge window:
>>     - Drivers get migrated to thermal_zone_device_register()
>>     - Functions register_with_trips()/tripless get deprecated but not yet removed
>  >
>> 2. In the next merge window (expecting all users updated from the first window):
>>     - Functions register_with_trips/tripless get removed (<- no more external refs
>>       to struct thermal_zone_params, which can be then safely renamed!)
>>     - Duplicated members governor_name and no_hwmon get removed from
>>       struct thermal_zone_params
>>     - Some structures get renamed to give them the proposed better names (which
>>       I also genuinely like)
>>     - Only Thermal API internal changes, as users (all the ones that are not in
>>       drivers/thermal/) won't need to get updated at all!
>>       ... and that's why I said "strictly like so" in the example up there.
>>
>> I think that this is the best strategy, for both ease of merging the changes and
>> internal reorganization.
>>
>> Sure in the first merge window we'll be wasting a byte or two, but I am confident
>> in that this is a very low price to pay - and even better, only temporarily - for
>> other bigger benefits.
>>
>> What do you think?
> 
> Sounds like a plan :)
> 
> 

Cool. It's time to write a good non-RFC series then!

Cheers,
Angelo