| Message ID | 1349094281-28889-4-git-send-email-fabio.porcedda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@gmail.com> > --- > .../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? Thanks Andrew
On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> >> --- >> .../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
On Mon, Oct 1, 2012 at 2:48 PM, Fabio Porcedda <fabio.porcedda@gmail.com> wrote: > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> >>> --- >>> .../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. When the at91sam9_wdt is converted to the watchdog core framework, the driver can use finally the new function. Best regards
On Mon, Oct 01, 2012 at 02:48:01PM +0200, Fabio Porcedda wrote: > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> > >> --- > >> .../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? It makes little sense doing this here, because you are going to have to clean this up sometime very soon and use the helper, unless there is a very good reason not to use the helper. Andrew
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@gmail.com> wrote: > > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> > >>> --- > >>> .../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
On Mon, Oct 1, 2012 at 3:06 PM, Andrew Lunn <andrew@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@gmail.com> wrote: >> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> >> >>> --- >> >>> .../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. 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? Best regards
On 17:04 Tue 02 Oct , Fabio Porcedda wrote: > On Mon, Oct 1, 2012 at 3:06 PM, Andrew Lunn <andrew@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@gmail.com> wrote: > >> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@gmail.com> > >> >>> --- > >> >>> .../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. > > 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 Best Regards, J.
> 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); > } You forgot to change the function signature. Also, if you are adding a generic function, it should be a generic function for the framework. All drivers should be slowly moving towards the framework, so adding functions which help you not move towards the framework are wrong. Andrew
On Tue, Oct 2, 2012 at 9:00 PM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > On 17:04 Tue 02 Oct , Fabio Porcedda wrote: >> On Mon, Oct 1, 2012 at 3:06 PM, Andrew Lunn <andrew@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@gmail.com> wrote: >> >> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@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.
On Tue, Oct 2, 2012 at 9:59 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> 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); >> } > > You forgot to change the function signature. > > Also, if you are adding a generic function, it should be a generic > function for the framework. All drivers should be slowly moving > towards the framework, so adding functions which help you not move > towards the framework are wrong. Maybe i can use a framework specific function and use a dummy watchdog_device for the non framerwork drivers. If it's ok i will send an updated patch. Best regards and thanks for the review.
On 18:32 Wed 03 Oct , Fabio Porcedda wrote: > On Tue, Oct 2, 2012 at 9:00 PM, Jean-Christophe PLAGNIOL-VILLARD > <plagnioj@jcrosoft.com> wrote: > > On 17:04 Tue 02 Oct , Fabio Porcedda wrote: > >> On Mon, Oct 1, 2012 at 3:06 PM, Andrew Lunn <andrew@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@gmail.com> wrote: > >> >> > On Mon, Oct 1, 2012 at 2:45 PM, Andrew Lunn <andrew@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@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? no a global helper not watchdog specific for timeout binding that why I said of.h not of_watchdog.h Best Regards, J.
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); +} + static int __init at91wdt_probe(struct platform_device *pdev) { struct resource *r; @@ -272,6 +281,8 @@ static int __init at91wdt_probe(struct platform_device *pdev) return -ENOMEM; } + at91wdt_probe_dt(pdev->dev.of_node); + /* Set watchdog */ res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000)); if (res) @@ -302,11 +313,21 @@ static int __exit at91wdt_remove(struct platform_device *pdev) return res; } +#if defined(CONFIG_OF) +static const struct of_device_id at91_wdt_dt_ids[] = { + { .compatible = "atmel,at91sam9260-wdt" }, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids); +#endif + static struct platform_driver at91wdt_driver = { .remove = __exit_p(at91wdt_remove), .driver = { .name = "at91_wdt", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(at91_wdt_dt_ids), }, };
Tested on an at91sam9260 board (evk-pro3) Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com> --- .../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