On 27/07/2018 13:52, Miquel Raynal wrote:

Hi Daniel,

Daniel Lezcano [off-list ref] wrote on Fri, 27 Jul 2018
13:34:19 +0200:

quoted

On 16/07/2018 16:41, Miquel Raynal wrote:

quoted

Thermal zone names must follow certain rules imposed by the framework.
They are limited in length and shall not have any hyphen '-'.

This is done in a separate function for future use in another location.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  

Why do you have to provide a function to test that?

Logically, the one who did the change to add a thermal name, should
check its code works. Without a proper name that won't work.

What do you mean "the one who did the change"?
I think the thermal core should not care that much to what is given as
name and should probably not be so strict.

Also, I don't choose what dev_name() returns, it's in the device tree
and the device tree do not care about the implementation, it's just a
descriptive file.

quoted

So this function is testing something which should be already tested, no?

I don't think it is. Without this function the probe will simply fail.

The explanation of what fails is in the code:

quoted

quoted

+		/*
+		 * When inside a system controller, the device name has the
+		 * form: f06f8000.system-controller:ap-thermal so stripping
+		 * after the ':' should give us a shorter but meaningful name.
+		 */
+		name = strrchr(name, ':');
+		if (!name)
+			name = "armada_thermal";
+		else
+			name++;

[...]

I'm not in favor of this, potentially it can introduce a break which can
be exploited later.

You are creating the thermal zones manually from the driver but want to
rely on the temperature controller name to give a name to the thermal zone.

Why not create the thermal zone in the DT with the correct name and use
devm_thermal_zone_of_sensor_register ?

Or alternatively hardcode the thermal zone name ?







-- 
 <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