| Message ID | 7cbb10853a343c9d488346596f604909ab0668b9.1392212059.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thanks, Guenter
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 ? What is your definition of "wrong" and "must have" ? How do you expect anyone to know that omitting those "optional" properties is by some definition "wrong" ? Guenter
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. Best regards Alejandro 50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964 http://cujae.edu.cu
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. Similar, the driver choses defaults for the other values. If that is incorrect, it should not do it. Really, something is very fishy here. Either the properties are optional or they are not. If they are optional, the driver should accept the fact silently. If they are mandatory, the driver should act accordingly and bail out if the attributes are not there. You can not have it both ways. I am close to recommending that Wim should not accept this driver until that is sorted out. Guenter
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
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 ? Regards Alejandro 50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964 http://cujae.edu.cu
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. Making these properties as mandatory and warn where any of them are not in device tree may be helpful. Alejandro 50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964 http://cujae.edu.cu
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@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. > 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. Guenter
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@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. 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. Alejandro 50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964 http://cujae.edu.cu
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@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. >> 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
On 02/23/2014 08:00 PM, Alejandro Cabrera wrote: > 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@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. >>> 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 :) > You have reached to completely different discussion. We should talk just about the code I have sent. I have checked what I have done and the intention was just to have better driver. I even didn't change any logic about DT and probe. DT binding just describes what it is written in the driver, nothing more nothing else. Please keep this in your mind. The driver with this binding is in the kernel for a while and if binding is wrong let's change it but it should be in separate patch which can fix binding warn/error messages. Wim: If you agree, I have no problem to send this follow up patch which can be applied on the top of this series. We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties and clock-frequence can go away and we can use CCF. Thanks, Michal
On 02/24/2014 08:25 PM, Wim Van Sebroeck wrote: > Hi Michal, > >> On 02/23/2014 08:00 PM, Alejandro Cabrera wrote: >>> 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@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. >>>>> 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 :) >>> >> >> You have reached to completely different discussion. >> We should talk just about the code I have sent. >> I have checked what I have done and the intention was just to have better driver. >> I even didn't change any logic about DT and probe. DT binding just describes >> what it is written in the driver, nothing more nothing else. >> >> Please keep this in your mind. >> >> The driver with this binding is in the kernel for a while and if binding is wrong >> let's change it but it should be in separate patch which can fix binding >> warn/error messages. >> >> Wim: If you agree, I have no problem to send this follow up patch >> which can be applied on the top of this series. >> We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties >> and clock-frequence can go away and we can use CCF. > > As said: a follow up patch would be welcome. So yes I agree. > And I also think that the discussion was interesting because it pointed out that a review of the bindings could also be usefull (what is required versus what os optional). Definitely it is not the first time where binding doc is useful. Also from binding we can see which features are common for all drivers in the same kind. Thanks, Michal
diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c index c229cc4..7deea21 100644 --- a/drivers/watchdog/of_xilinx_wdt.c +++ b/drivers/watchdog/of_xilinx_wdt.c @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev) static int xwdt_probe(struct platform_device *pdev) { int rc; - u32 *tmptr; - u32 *pfreq; + u32 pfreq, enable_once = 0; struct resource *res; struct xwdt_device *xdev; bool no_timeout = false; @@ -168,32 +167,28 @@ static int xwdt_probe(struct platform_device *pdev) if (IS_ERR(xdev->base)) return PTR_ERR(xdev->base); - 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); - } + + watchdog_set_nowayout(xilinx_wdt_wdd, enable_once); /* * Twice of the 2^wdt_interval / freq because the first wdt overflow is @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev) */ if (!no_timeout) xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) / - *pfreq); + pfreq); spin_lock_init(&xdev->spinlock); watchdog_set_drvdata(xilinx_wdt_wdd, xdev);