[PATCH v6 3/5] watchdog: at91sam9_wdt: add device tree support

[Fabio Porcedda]

On Tue, Oct 2, 2012 at 9:00 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj at jcrosoft.com> wrote:
> On 17:04 Tue 02 Oct     , Fabio Porcedda wrote:
>> On Mon, Oct 1, 2012 at 3:06 PM, Andrew Lunn <andrew at lunn.ch> wrote:
>> > On Mon, Oct 01, 2012 at 02:54:55PM +0200, Fabio Porcedda wrote:
>> >> On Mon, Oct 1, 2012 at 2:48 PM, Fabio Porcedda <fabio.porcedda at gmail.com> wrote:
>> >> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew at lunn.ch> wrote:
>> >> >> On Mon, Oct 01, 2012 at 02:24:39PM +0200, Fabio Porcedda wrote:
>> >> >>> Tested on an at91sam9260 board (evk-pro3)
>> >> >>>
>> >> >>> Signed-off-by: Fabio Porcedda <fabio.porcedda at gmail.com>
>> >> >>> ---
>> >> >>>  .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
>> >> >>>  drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
>> >> >>>  2 files changed, 40 insertions(+)
>> >> >>> ...
>> >> >>
>> >> >> In patch #1 you add a function to do this, and then you don't make use
>> >> >> of it here ?
>> >> >>
>> >> >> Or am i missing something?
>> >> >
>> >> > I'm using it on the patch #2 for the orion_wdt driver.
>> >> > Do you think it's better to join the #1 and the #2 patch?
>> >> >
>> >> > Best regards
>> >> > --
>> >> > Fabio Porcedda
>> >>
>> >> I'm sorry, only now i understand your question.
>> >> The at91sam9_wdt driver don't use the watchdog core framework si i
>> >> can't use that function cleanly.
>> >
>> >> The patch #1 and #2 are for introducing the same property as the
>> >> at91sam9_wdt driver.
>> >
>> > So maybe split this up into two different patchsets. One patchset to
>> > add the helper function, and the use of this helper to all watchdog
>> > divers that can use it. I think the following drivers should be
>> > modified:
>> >
>> > orion_wdt.c
>> > pnx4008_wdt.c
>> > s3c2410_wdt.c
>> >
>> > In a second patchset, convert the AT91SAM9 driver over to the watchdog
>> > core framework, and then use the helper function.
>>
>> I was thinking to add a more generic helper function like this:
>>
>> static inline void watchdog_get_dttimeout(struct device_node *node,
>> u32 *timeout)
>> {
>>       if (node)
>>               of_property_read_u32(node, "timeout", &wdd->timeout);
>> }
>>
>> This way i can use this helper function in the at91sam9_wdt driver too.
>> What do you think?
> timeout_sec and this can be move at of.h level
>
> as this is not watchdog framework secific

I can not find any property with the "_sec" suffix, you think it's
still fine to use that suffix?

You are speaking about a of_watchdog.h header with a
of_watchdog_get_timeout function?

Best regards and thanks for the review.
-- 
Fabio Porcedda