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

[Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> ---
> >>>  .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
> >>>  drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
> >>>  2 files changed, 40 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> >>> new file mode 100644
> >>> index 0000000..65c1755
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
> >>> @@ -0,0 +1,19 @@
> >>> +* Atmel Watchdog Timers
> >>> +
> >>> +** at91sam9-wdt
> >>> +
> >>> +Required properties:
> >>> +- compatible: must be "atmel,at91sam9260-wdt".
> >>> +- reg: physical base address of the controller and length of memory mapped
> >>> +  region.
> >>> +
> >>> +Optional properties:
> >>> +- timeout: contains the watchdog timeout in seconds.
> >>> +
> >>> +Example:
> >>> +
> >>> +     watchdog@fffffd40 {
> >>> +             compatible = "atmel,at91sam9260-wdt";
> >>> +             reg = <0xfffffd40 0x10>;
> >>> +             timeout = <10>;
> >>> +     };
> >>> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> >>> index 05e1be8..c9e6bfa 100644
> >>> --- a/drivers/watchdog/at91sam9_wdt.c
> >>> +++ b/drivers/watchdog/at91sam9_wdt.c
> >>> @@ -32,6 +32,7 @@
> >>>  #include <linux/timer.h>
> >>>  #include <linux/bitops.h>
> >>>  #include <linux/uaccess.h>
> >>> +#include <linux/of.h>
> >>>
> >>>  #include "at91sam9_wdt.h"
> >>>
> >>> @@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
> >>>       .fops           = &at91wdt_fops,
> >>>  };
> >>>
> >>> +static inline void __init at91wdt_probe_dt(struct device_node *node)
> >>> +{
> >>> +     if (!node)
> >>> +             return;
> >>> +
> >>> +     of_property_read_u32(node, "timeout", &heartbeat);
> >>> +}
> >>> +
> >>
> >> 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.

     Andrew