From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <53A982ED.3070709@nsn.com> Date: Tue, 24 Jun 2014 15:53:49 +0200 From: Alexander Sverdlin MIME-Version: 1.0 Subject: Re: [PATCH 5/6] OF: Utility helper functions for dynamic nodes References: <1403430039-15085-1-git-send-email-pantelis.antoniou@konsulko.com> <1403430039-15085-6-git-send-email-pantelis.antoniou@konsulko.com> <53A85549.7040809@nsn.com> <6E91A461-4361-4A18-BE32-CECDD789C114@konsulko.com> <20140623183343.GA10389@heimdall> <78ACBAF6-A73E-4272-8D3A-258C4B10858C@konsulko.com> <53A8848A.3000803@roeck-us.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit To: ext Rob Herring , Pantelis Antoniou Cc: Guenter Roeck , Ioan Nicu , Grant Likely , Stephen Warren , Matt Porter , Koen Kooi , Greg Kroah-Hartman , Alison Chaiken , Dinh Nguyen , Jan Lubbe , Michael Stickel , Dirk Behme , Alan Tull , Sascha Hauer , Michael Bohan , Michal Simek , Matt Ranostay , Joel Becker , "devicetree@vger.kernel.org" , Wolfram Sang , "linux-i2c@vger.kernel.org" , Mark Brown , "linux-spi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Pete Popov , Dan Malek , Georgi Vlaev List-ID: Hi! On 24/06/14 15:46, ext Rob Herring wrote: [...] >>>> If that's the case, the code in irq.c is wrong. >>>> >>>> interrupt-controller is a bool property; the correct call to use is of_property_read_bool() >>>> which returns true or false when the value is defined. >>>> >>>> The use of of_get_property is a bug here. It is perfectly valid for a property to have a >>>> NULL value when length = 0. >>>> >>> >>> That usage is spread throughout the code though. There are three or four similar checks >>> for interrupt-controller in the code, and many others using of_get_property() to check >>> for booleans. >>> >>> Some examples: >>> s5m8767,pmic-buck2-ramp-enable >>> s5m8767,pmic-buck3-ramp-enable >>> s5m8767,pmic-buck4-ramp-enable >>> d7s-flipped? >>> atmel,use-dma-rx >>> linux,rs485-enabled-at-boot-time >>> marvell,enable-port1 (and many others) >>> linux,bootx-noscreen >>> linux,opened >>> >>> and many many others. >>> >>> Maybe people meant to use of_find_property() ? >>> >> >> I bet... I see a lot of users doing if (of_get_property()). >> >> Which is no good. > > I don't see what the issue is. The irq.c code is correct. The code is > checking for existence of a property or not and of_get_property > adequately does that. of_find_property is equivalent in this case. Now The code is correct and the functions are equal as long as we declare that void property has value != NULL. Which must not be actually and the patch from Pantelis actually creates this situation first time. So let's first agree, how should the empty property look like. length should be 0, it's clear. But what about the value? > we could make the code stricter that properties defined to be empty > are in fact empty. My opinion is that it is not the kernel's job to > validate DTs. This can and should be done at dtb build time. > > Boolean and empty properties are not the same. Booleans can have a > value of true or false in addition to being absent (false) or present > with no value (true). of_get_property predates of_property_read_bool > and other helpers by many years, so no doubt there are many callers of > of_get_property which could be converted. Patches welcome. :) -- Best regards, Alexander Sverdlin.