Received-SPF: pass (google.com: domain of linux-kernel+bounces-25969-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <7417c498-2439-485d-9f78-fbb22f9ce393@linaro.org>
Date: Mon, 15 Jan 2024 13:39:57 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 01/26] thermal: Introduce
 thermal_zone_device_register() and params structure
Content-Language: en-US
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: rafael@kernel.org, rui.zhang@intel.com, lukasz.luba@arm.com,
 linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@collabora.com
References: <20231221124825.149141-1-angelogioacchino.delregno@collabora.com>
 <20231221124825.149141-2-angelogioacchino.delregno@collabora.com>
From: Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20231221124825.149141-2-angelogioacchino.delregno@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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)
>   { }
>   

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog