From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Subject: Re: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name Date: Sun, 29 Jul 2018 21:27:44 +0200 Message-ID: <20180729212744.31cf0747@xps13> References: <20180716144206.30985-1-miquel.raynal@bootlin.com> <20180716144206.30985-2-miquel.raynal@bootlin.com> <84f97022-a470-f314-ee75-e5afb733bea5@linaro.org> <20180727135230.3c7102c7@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Daniel Lezcano Cc: Mark Rutland , Andrew Lunn , Jason Cooper , Nadav Haklai , devicetree@vger.kernel.org, Antoine Tenart , Catalin Marinas , Gregory Clement , linux-pm@vger.kernel.org, Will Deacon , Maxime Chevallier , Eduardo Valentin , David Sniatkiwicz , Rob Herring , Thomas Petazzoni , Zhang Rui , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: linux-pm@vger.kernel.org SGkgRGFuaWVsLAoKRGFuaWVsIExlemNhbm8gPGRhbmllbC5sZXpjYW5vQGxpbmFyby5vcmc+IHdy b3RlIG9uIEZyaSwgMjcgSnVsIDIwMTgKMTc6Mjk6NTIgKzAyMDA6Cgo+IE9uIDI3LzA3LzIwMTgg MTM6NTIsIE1pcXVlbCBSYXluYWwgd3JvdGU6Cj4gPiBIaSBEYW5pZWwsCj4gPiAKPiA+IERhbmll bCBMZXpjYW5vIDxkYW5pZWwubGV6Y2Fub0BsaW5hcm8ub3JnPiB3cm90ZSBvbiBGcmksIDI3IEp1 bCAyMDE4Cj4gPiAxMzozNDoxOSArMDIwMDoKPiA+ICAgCj4gPj4gT24gMTYvMDcvMjAxOCAxNjo0 MSwgTWlxdWVsIFJheW5hbCB3cm90ZTogIAo+ID4+PiBUaGVybWFsIHpvbmUgbmFtZXMgbXVzdCBm b2xsb3cgY2VydGFpbiBydWxlcyBpbXBvc2VkIGJ5IHRoZSBmcmFtZXdvcmsuCj4gPj4+IFRoZXkg YXJlIGxpbWl0ZWQgaW4gbGVuZ3RoIGFuZCBzaGFsbCBub3QgaGF2ZSBhbnkgaHlwaGVuICctJy4K PiA+Pj4KPiA+Pj4gVGhpcyBpcyBkb25lIGluIGEgc2VwYXJhdGUgZnVuY3Rpb24gZm9yIGZ1dHVy ZSB1c2UgaW4gYW5vdGhlciBsb2NhdGlvbi4KPiA+Pj4KPiA+Pj4gU2lnbmVkLW9mZi1ieTogTWlx dWVsIFJheW5hbCA8bWlxdWVsLnJheW5hbEBib290bGluLmNvbT4gICAgCj4gPj4KPiA+PiBXaHkg ZG8geW91IGhhdmUgdG8gcHJvdmlkZSBhIGZ1bmN0aW9uIHRvIHRlc3QgdGhhdD8KPiA+Pgo+ID4+ IExvZ2ljYWxseSwgdGhlIG9uZSB3aG8gZGlkIHRoZSBjaGFuZ2UgdG8gYWRkIGEgdGhlcm1hbCBu YW1lLCBzaG91bGQKPiA+PiBjaGVjayBpdHMgY29kZSB3b3Jrcy4gV2l0aG91dCBhIHByb3BlciBu YW1lIHRoYXQgd29uJ3Qgd29yay4gIAo+ID4gCj4gPiBXaGF0IGRvIHlvdSBtZWFuICJ0aGUgb25l IHdobyBkaWQgdGhlIGNoYW5nZSI/Cj4gPiBJIHRoaW5rIHRoZSB0aGVybWFsIGNvcmUgc2hvdWxk IG5vdCBjYXJlIHRoYXQgbXVjaCB0byB3aGF0IGlzIGdpdmVuIGFzCj4gPiBuYW1lIGFuZCBzaG91 bGQgcHJvYmFibHkgbm90IGJlIHNvIHN0cmljdC4KPiA+IAo+ID4gQWxzbywgSSBkb24ndCBjaG9v c2Ugd2hhdCBkZXZfbmFtZSgpIHJldHVybnMsIGl0J3MgaW4gdGhlIGRldmljZSB0cmVlCj4gPiBh bmQgdGhlIGRldmljZSB0cmVlIGRvIG5vdCBjYXJlIGFib3V0IHRoZSBpbXBsZW1lbnRhdGlvbiwg aXQncyBqdXN0IGEKPiA+IGRlc2NyaXB0aXZlIGZpbGUuCj4gPiAgIAo+ID4+Cj4gPj4gU28gdGhp cyBmdW5jdGlvbiBpcyB0ZXN0aW5nIHNvbWV0aGluZyB3aGljaCBzaG91bGQgYmUgYWxyZWFkeSB0 ZXN0ZWQsIG5vPyAgCj4gPiAKPiA+IEkgZG9uJ3QgdGhpbmsgaXQgaXMuIFdpdGhvdXQgdGhpcyBm dW5jdGlvbiB0aGUgcHJvYmUgd2lsbCBzaW1wbHkgZmFpbC4KPiA+IAo+ID4gVGhlIGV4cGxhbmF0 aW9uIG9mIHdoYXQgZmFpbHMgaXMgaW4gdGhlIGNvZGU6Cj4gPiAgIAo+ID4+PiArCQkvKgo+ID4+ PiArCQkgKiBXaGVuIGluc2lkZSBhIHN5c3RlbSBjb250cm9sbGVyLCB0aGUgZGV2aWNlIG5hbWUg aGFzIHRoZQo+ID4+PiArCQkgKiBmb3JtOiBmMDZmODAwMC5zeXN0ZW0tY29udHJvbGxlcjphcC10 aGVybWFsIHNvIHN0cmlwcGluZwo+ID4+PiArCQkgKiBhZnRlciB0aGUgJzonIHNob3VsZCBnaXZl IHVzIGEgc2hvcnRlciBidXQgbWVhbmluZ2Z1bCBuYW1lLgo+ID4+PiArCQkgKi8KPiA+Pj4gKwkJ bmFtZSA9IHN0cnJjaHIobmFtZSwgJzonKTsKPiA+Pj4gKwkJaWYgKCFuYW1lKQo+ID4+PiArCQkJ bmFtZSA9ICJhcm1hZGFfdGhlcm1hbCI7Cj4gPj4+ICsJCWVsc2UKPiA+Pj4gKwkJCW5hbWUrKzsg IAo+ID4gCj4gPiBbLi4uXSAgCj4gCj4gSSdtIG5vdCBpbiBmYXZvciBvZiB0aGlzLCBwb3RlbnRp YWxseSBpdCBjYW4gaW50cm9kdWNlIGEgYnJlYWsgd2hpY2ggY2FuCj4gYmUgZXhwbG9pdGVkIGxh dGVyLgoKSSBrbm93LCBJIGRvbid0IGxpa2UgdGhpcyBuZWl0aGVyIGJ1dCB0aGF0IHdhcyB0aGUg YmVzdCB3YXkgSU1ITyB0bwpoYW5kbGUgdGhlIGNvcmUncyBsaW1pdGF0aW9ucy4KCj4gCj4gWW91 IGFyZSBjcmVhdGluZyB0aGUgdGhlcm1hbCB6b25lcyBtYW51YWxseSBmcm9tIHRoZSBkcml2ZXIg YnV0IHdhbnQgdG8KPiByZWx5IG9uIHRoZSB0ZW1wZXJhdHVyZSBjb250cm9sbGVyIG5hbWUgdG8g Z2l2ZSBhIG5hbWUgdG8gdGhlIHRoZXJtYWwgem9uZS4KPiAKPiBXaHkgbm90IGNyZWF0ZSB0aGUg dGhlcm1hbCB6b25lIGluIHRoZSBEVCB3aXRoIHRoZSBjb3JyZWN0IG5hbWUgYW5kIHVzZQo+IGRl dm1fdGhlcm1hbF96b25lX29mX3NlbnNvcl9yZWdpc3RlciA/CgpUaGF0J3MgZXhhY3RseSB3aGF0 IGlzIGRvbmUgbGF0ZXIgaW4gdGhlIHNlcmllcyBhY3R1YWxseSwgYnV0IGZvciBEVApiYWNrd2Fy ZCBjb21wYXRpYmlsaXR5IGFuZCBpbiBvcmRlciB0byBjb250aW51ZSBhZGRpbmcgZmVhdHVyZSB0 byB0aGlzCmRyaXZlciwgd2UgbXVzdCBlbnN1cmUgdGhlcmUgd29uJ3QgYmUgYW55IGRyYXdiYWNr IGZvciBwZW9wbGUgdXNpbmcgb2xkCkRUIHdpdGggZGVwcmVjYXRlZCB0aGVybWFsIHJlcHJlc2Vu dGF0aW9uLgoKClRoYW5rcyBmb3IgcmV2aWV3aW5nLCBpZiB5b3UgZG9uJ3QgbWluZCBJIGNvdWxk IGNvcHkgeW91IGluIGZ1dHVyZQpjaGFuZ2VzPyBVbmZvcnR1bmF0ZWx5IGZvciB0aGlzIHNlcmll cyBpdCdzIGFscmVhZHkgYmVlbiBhcHBsaWVkIGFuZCBJCndvdWxkIGxpa2Ugbm90IHRvIGRvIGFu eSBkZWVwIGNoYW5nZXMgYmVmb3JlIGl0J3MgbWVyZ2VkLCBidXQgZm9yIHN1cmUKdGhpcyBmdW5j dGlvbiBjb3VsZCBiZSBlbmhhbmNlZC4KCktpbmQgcmVnYXJkcywKTWlxdcOobAoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBt YWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9s aXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: miquel.raynal@bootlin.com (Miquel Raynal) Date: Sun, 29 Jul 2018 21:27:44 +0200 Subject: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name In-Reply-To: References: <20180716144206.30985-1-miquel.raynal@bootlin.com> <20180716144206.30985-2-miquel.raynal@bootlin.com> <84f97022-a470-f314-ee75-e5afb733bea5@linaro.org> <20180727135230.3c7102c7@xps13> Message-ID: <20180729212744.31cf0747@xps13> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Daniel, Daniel Lezcano wrote on Fri, 27 Jul 2018 17:29:52 +0200: > On 27/07/2018 13:52, Miquel Raynal wrote: > > Hi Daniel, > > > > Daniel Lezcano 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 > >> > >> 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++; > > > > [...] > > I'm not in favor of this, potentially it can introduce a break which can > be exploited later. I know, I don't like this neither but that was the best way IMHO to handle the core's limitations. > > 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 ? That's exactly what is done later in the series actually, but for DT backward compatibility and in order to continue adding feature to this driver, we must ensure there won't be any drawback for people using old DT with deprecated thermal representation. Thanks for reviewing, if you don't mind I could copy you in future changes? Unfortunately for this series it's already been applied and I would like not to do any deep changes before it's merged, but for sure this function could be enhanced. Kind regards, Miqu?l