Re: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>, Andrew Lunn <andrew@lunn.ch>,
	Jason Cooper <jason@lakedaemon.net>,
	Nadav Haklai <nadavh@marvell.com>,
	devicetree@vger.kernel.org,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	linux-pm@vger.kernel.org, Will Deacon <will.deacon@arm.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	Eduardo Valentin <edubezval@gmail.com>,
	David Sniatkiwicz <davidsn@marvell.com>,
	Rob Herring <robh+dt@kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Zhang Rui <rui.zhang@intel.com>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name
Date: Fri, 27 Jul 2018 13:52:30 +0200	[thread overview]
Message-ID: <20180727135230.3c7102c7@xps13> (raw)
In-Reply-To: <84f97022-a470-f314-ee75-e5afb733bea5@linaro.org>

Hi Daniel,

Daniel Lezcano <daniel.lezcano@linaro.org> wrote on Fri, 27 Jul 2018
13:34:19 +0200:

> On 16/07/2018 16:41, Miquel Raynal wrote:
> > 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.

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

> > +		/*
> > +		 * 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++;

[...]

> > +
> > +	/* Then check there are no '-' or hwmon core will complain */

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel