Message-ID: <530A453E.4070608@udio.cujae.edu.cu>
Date:	Sun, 23 Feb 2014 11:00:14 -0800
From:	Alejandro Cabrera <acabrera@...o.cujae.edu.cu>
To:	Guenter Roeck <linux@...ck-us.net>
CC:	Wim Van Sebroeck <wim@...ana.be>,
	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, monstr@...str.eu,
	linux-watchdog@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

On 23/2/2014 6:43 AM, Guenter Roeck wrote:
> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>>> Hi Michal,
>>>>>>>>>>
>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@...inx.com>
>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@...ck-us.net>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>
>>>>>>>>>> Looks good.
>>>>>>>>>>
>>>>>>>>>> Another comment/remark.
>>>>>>>>>>
>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>> -
>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "clock-frequency",&pfreq);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>>>>>> obtained\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-interval",
>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>> -    } else {
>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-enable-once",
>>>>>>>>>>> + &enable_once);
>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not 
>>>>>>>>>>> found\n");
>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>> -    }
>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>
>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>> a separate patch.
>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>>>>>> patch is thus welcome.
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> I support Michal intention, I think it is a warning because 
>>>>>>>> device tree blob must have the "xlnx,wdt-enable-once" property 
>>>>>>>> specified in order to allow the system to be sure of the real 
>>>>>>>> value of this property. In addition to, this warning can be 
>>>>>>>> helpful to detect a wrong device tree specification.
>>>>>>>>
>>>>>>>
>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>
>>>>>>> Optional properties:
>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> 
>>>>>>> clock cycles,
>>>>>>> <val> is integer from 8 to 31.
>>>>>>>
>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>> Hi Guenter
>>>>>>
>>>>>> I didn't know that these properties was classified as optional...
>>>>>> I think that they should not be, because all xilinx watchog 
>>>>>> devices (at least for microblaze processor)
>>>>>> have these properties defined in theirs MPD files and theirs 
>>>>>> values can be obtained during the
>>>>>> hardware specification to device tree conversion.
>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>> for all xilinx watchdog devices they must be present in the 
>>>>>> device tree because they allows
>>>>>> the system (linux/user) to know exactly how a watchdog device is 
>>>>>> configured.
>>>>>> Because these properties always can be obtained from hardware 
>>>>>> design there is no
>>>>>> reason to leave them out from the device tree. This is why I 
>>>>>> consider that a device tree without
>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>> How do you expect anyone to know that omitting those
>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>> I'm agree with you.
>>>>>> Maybe these properties shouldn't be optional.
>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>>>>>> device tree,
>>>>>> when a watchdog daemon ask for this property what is the obtained 
>>>>>> value ?
>>>>>> Independently of this value, why do not warn the user about this 
>>>>>> missing property
>>>>>> when it can always be in the device tree ?
>>>>>>
>>>>>
>>>>> Really, this line of argument doesn't make any sense to me.
>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>> doesn't really add any value.
>>>>>
>>>>
>>>> If the device has been configured with wdt-enable-once=true
>>>> and the device tree doesn't have this property then a watchdog daemon
>>>> would see it as "false" because it is the default making the system 
>>>> to misbehave...
>>>> A warning during driver loading could help user to identify the 
>>>> problem.
>>>>
>>>
>>> All this would give you is a false sense of safety. The property could
>>> just as well be there and be wrong (eg be configured as = <0> when it
>>> should be 1, or with a wrong frequency.
>> These issues "cannot" be detected but the missing properties yes.
>>> Following your logic, every driver
>>> would need to warn about everything, just to be sure.
>> Every driver should warn about anything that it considers weird and 
>> let the user to decide if it matters or not.
>> I think that an example of weird could be the lack of an expected 
>> property.
>>
>
> I don't think it makes sense to continue this discussion.
> We have fundamental differences in opinion which we won't
> resolve by repeating our arguments over and over.
>
> Wim, I'll let you decide how to handle this. My recommendation
> is to request the author to decide if the properties are optional
> or not before accepting this patch set. Either the properties
> are optional, and there should be no warnings, or they are
> mandatory and the driver should bail out if they are missing.
>
I'm totally agreed with you :)




50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists