Re: [PATCH 2/3] thermal/drivers/devfreq_cooling: Use device name instead of auto-numbering

[Lukasz Luba]

On 3/10/21 11:45 AM, Daniel Lezcano wrote:
> Currently the naming of a cooling device is just a cooling technique
> followed by a number. When there are multiple cooling devices using
> the same technique, it is impossible to clearly identify the related
> device as this one is just a number.
>
> For instance:
>
>   thermal-devfreq-0
>   thermal-devfreq-1
>   etc ...
>
> The 'thermal' prefix is redundant with the subsystem namespace. This
> patch removes the 'thermal prefix and changes the number by the device

missing ' after 'thermal

> name. So the naming above becomes:
>
>   devfreq-5000000.gpu
>   devfreq-1d84000.ufshc
>   etc ...
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
>   drivers/thermal/devfreq_cooling.c | 21 ++++-----------------
>   1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> index fed3121ff2a1..62abcffeb422 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c

same here, you can now remove the idr.h header

> @@ -25,11 +25,8 @@
>   #define HZ_PER_KHZ		1000
>   #define SCALE_ERROR_MITIGATION	100
>   
> -static DEFINE_IDA(devfreq_ida);
> -
>   /**
>    * struct devfreq_cooling_device - Devfreq cooling device
> - * @id:		unique integer value corresponding to each
>    *		devfreq_cooling_device registered.
>    * @cdev:	Pointer to associated thermal cooling device.
>    * @devfreq:	Pointer to associated devfreq device.
> @@ -51,7 +48,6 @@ static DEFINE_IDA(devfreq_ida);
>    * @em_pd:		Energy Model for the associated Devfreq device
>    */
>   struct devfreq_cooling_device {
> -	int id;
>   	struct thermal_cooling_device *cdev;
>   	struct devfreq *devfreq;
>   	unsigned long cooling_state;
> @@ -363,7 +359,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>   	struct thermal_cooling_device *cdev;
>   	struct device *dev = df->dev.parent;
>   	struct devfreq_cooling_device *dfc;
> -	char dev_name[THERMAL_NAME_LENGTH];
> +	char name[THERMAL_NAME_LENGTH];

This is probably too short (20 char) array. For example in my phone's
devfreq dir, there are really long names:

---------------------------------------------------------
redfin:/sys/class/devfreq # for f in `ls ./` ; do echo $f; cat $f/name | 
wc -m ; done 

18321000.qcom,devfreq-l3:qcom,cdsp-cdsp-l3-lat
47
18321000.qcom,devfreq-l3:qcom,cpu0-cpu-l3-lat
46
18321000.qcom,devfreq-l3:qcom,cpu6-cpu-l3-lat
46
18321000.qcom,devfreq-l3:qcom,cpu7-cpu-l3-lat
46
3d00000.qcom,kgsl-3d0
22
soc:qcom,cpu-cpu-llcc-bw
25
soc:qcom,cpu-llcc-ddr-bw
25
soc:qcom,cpu0-cpu-ddr-latfloor
31
soc:qcom,cpu0-cpu-llcc-lat
27
soc:qcom,cpu0-llcc-ddr-lat
27
soc:qcom,cpu6-cpu-ddr-latfloor
31
soc:qcom,cpu6-cpu-llcc-lat
27
soc:qcom,cpu6-llcc-ddr-lat
27
soc:qcom,cpu7-cpu-ddr-latfloor
31
soc:qcom,gpubw
15
soc:qcom,kgsl-busmon
21
soc:qcom,npu-llcc-ddr-bw
25
soc:qcom,npu-npu-llcc-bw
25
soc:qcom,npudsp-npu-ddr-bw
27
soc:qcom,snoc_cnoc_keepalive
29
---------------------------------------------------------

We should allocate tmp buffer for it, to not loose the meaningful part
of that string name or end up with only the same prefix, like for the
first 3 from top:

devfreq-18321000.qco

or for the GPU:
devfreq-3d00000.qcom

This is tricky area and vendors might put any non-meaningful prefix.

The rest of the code looks OK, only this name construction part.