Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck <linux@roeck-us.net>]

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@xilinx.com>
>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-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. Following your logic, every driver
would need to warn about everything, just to be sure.

Guenter