watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
Following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt to make use of
the parameter logic.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/sama5d4_wdt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 0ae947c3d7bc..e6c679383734 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -33,7 +33,7 @@ struct sama5d4_wdt {
unsigned long last_ping;
};
-static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
+static int wdt_timeout;
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(wdt_timeout, int, 0);
@@ -212,7 +212,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
return -ENOMEM;
wdd = &wdt->wdd;
- wdd->timeout = wdt_timeout;
+ wdd->timeout = WDT_DEFAULT_TIMEOUT;
wdd->info = &sama5d4_wdt_info;
wdd->ops = &sama5d4_wdt_ops;
wdd->min_timeout = MIN_WDT_TIMEOUT;
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
Following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt to make use of
the parameter logic.
Signed-off-by: Marcus Folkesson <[email protected]>
---
v2:
- Set .timeout in coh901327_wdt structure declaration.
- Set .min_timeout to 1 instead of 0. I could not find a datasheet
for coh901327, so I'm not sure if 0 is valid. However, 0 seems
wrong to me and most driver has 1 as min value. If it should
be 0, please let me know and I have to set another initial
value for margin.
drivers/watchdog/coh901327_wdt.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
index 4410337f4f7f..5d8eb9a30879 100644
--- a/drivers/watchdog/coh901327_wdt.c
+++ b/drivers/watchdog/coh901327_wdt.c
@@ -67,7 +67,9 @@
#define U300_WDOG_IFR_WILL_BARK_IRQ_FORCE_ENABLE 0x0001U
/* Default timeout in seconds = 1 minute */
-static unsigned int margin = 60;
+#define U300_WDOG_DEFAULT_TIMEOUT 60
+
+static unsigned int margin;
static int irq;
static void __iomem *virtbase;
static struct device *parent;
@@ -235,8 +237,9 @@ static struct watchdog_device coh901327_wdt = {
* timeout register is max
* 0x7FFF = 327670ms ~= 327s.
*/
- .min_timeout = 0,
+ .min_timeout = 1,
.max_timeout = 327,
+ .timeout = U300_WDOG_DEFAULT_TIMEOUT,
};
static int __exit coh901327_remove(struct platform_device *pdev)
@@ -315,9 +318,7 @@ static int __init coh901327_probe(struct platform_device *pdev)
goto out_no_irq;
}
- ret = watchdog_init_timeout(&coh901327_wdt, margin, dev);
- if (ret < 0)
- coh901327_wdt.timeout = 60;
+ watchdog_init_timeout(&coh901327_wdt, margin, dev);
coh901327_wdt.parent = dev;
ret = watchdog_register_device(&coh901327_wdt);
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
By following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt, it also
let us to set timout-sec property in devicetree.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 4 ++++
drivers/watchdog/mtk_wdt.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index 5b38a30e608c..859dee167b91 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -11,9 +11,13 @@ Required properties:
- reg : Specifies base physical address and size of the registers.
+Optional properties:
+- timeout-sec: contains the watchdog timeout in seconds.
+
Example:
wdt: watchdog@10000000 {
compatible = "mediatek,mt6589-wdt";
reg = <0x10000000 0x18>;
+ timeout-sec = <10>;
};
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 7ed417a765c7..fcdc10ec28a3 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -57,7 +57,7 @@
#define DRV_VERSION "1.0"
static bool nowayout = WATCHDOG_NOWAYOUT;
-static unsigned int timeout = WDT_MAX_TIMEOUT;
+static unsigned int timeout;
struct mtk_wdt_dev {
struct watchdog_device wdt_dev;
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
By following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt, it also
let us to set timout-sec property in devicetree.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Documentation/devicetree/bindings/watchdog/meson-wdt.txt | 4 ++++
drivers/watchdog/meson_wdt.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/watchdog/meson-wdt.txt b/Documentation/devicetree/bindings/watchdog/meson-wdt.txt
index 8a6d84cb36c9..7588cc3971bf 100644
--- a/Documentation/devicetree/bindings/watchdog/meson-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/meson-wdt.txt
@@ -9,9 +9,13 @@ Required properties:
"amlogic,meson8m2-wdt" and "amlogic,meson8b-wdt" on Meson8m2 SoCs
- reg : Specifies base physical address and size of the registers.
+Optional properties:
+- timeout-sec: contains the watchdog timeout in seconds.
+
Example:
wdt: watchdog@c1109900 {
compatible = "amlogic,meson6-wdt";
reg = <0xc1109900 0x8>;
+ timeout-sec = <10>;
};
diff --git a/drivers/watchdog/meson_wdt.c b/drivers/watchdog/meson_wdt.c
index 304274c67735..cd0275a6cdac 100644
--- a/drivers/watchdog/meson_wdt.c
+++ b/drivers/watchdog/meson_wdt.c
@@ -36,7 +36,7 @@
#define MESON_SEC_TO_TC(s, c) ((s) * (c))
static bool nowayout = WATCHDOG_NOWAYOUT;
-static unsigned int timeout = MESON_WDT_TIMEOUT;
+static unsigned int timeout;
struct meson_wdt_data {
unsigned int enable;
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
Following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt to make use of
the parameter logic.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/pnx4008_wdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 0529aed158a4..8e261799c84e 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -78,7 +78,7 @@
#define WDOG_COUNTER_RATE 13000000 /*the counter clock is 13 MHz fixed */
static bool nowayout = WATCHDOG_NOWAYOUT;
-static unsigned int heartbeat = DEFAULT_HEARTBEAT;
+static unsigned int heartbeat;
static DEFINE_SPINLOCK(io_lock);
static void __iomem *wdt_base;
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
By following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt, it also
let us to set timout-sec property in devicetree.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Documentation/devicetree/bindings/watchdog/sirfsoc_wdt.txt | 4 ++++
drivers/watchdog/sirfsoc_wdt.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/watchdog/sirfsoc_wdt.txt b/Documentation/devicetree/bindings/watchdog/sirfsoc_wdt.txt
index 9cbc76c89b2b..0dce5e3100b4 100644
--- a/Documentation/devicetree/bindings/watchdog/sirfsoc_wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/sirfsoc_wdt.txt
@@ -5,10 +5,14 @@ Required properties:
- reg: Address range of tick timer/WDT register set
- interrupts: interrupt number to the cpu
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
Example:
timer@b0020000 {
compatible = "sirf,prima2-tick";
reg = <0xb0020000 0x1000>;
interrupts = <0>;
+ timeout-sec = <30>;
};
diff --git a/drivers/watchdog/sirfsoc_wdt.c b/drivers/watchdog/sirfsoc_wdt.c
index 4eea351e09b0..ac0c9d2c4aee 100644
--- a/drivers/watchdog/sirfsoc_wdt.c
+++ b/drivers/watchdog/sirfsoc_wdt.c
@@ -29,7 +29,7 @@
#define SIRFSOC_WDT_MAX_TIMEOUT (10 * 60) /* 10 mins */
#define SIRFSOC_WDT_DEFAULT_TIMEOUT 30 /* 30 secs */
-static unsigned int timeout = SIRFSOC_WDT_DEFAULT_TIMEOUT;
+static unsigned int timeout;
static bool nowayout = WATCHDOG_NOWAYOUT;
module_param(timeout, uint, 0);
--
2.15.1
watchdog_init_timeout() will allways pick timeout_param since it
defaults to a valid timeout.
By following best practice described in
Documentation/watchdog/watchdog-kernel-api.txt, it also
let us to set timout-sec property in devicetree.
Signed-off-by: Marcus Folkesson <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
Documentation/devicetree/bindings/watchdog/sunxi-wdt.txt | 4 ++++
drivers/watchdog/sunxi_wdt.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/watchdog/sunxi-wdt.txt b/Documentation/devicetree/bindings/watchdog/sunxi-wdt.txt
index 62dd5baad70e..49900e72f6b1 100644
--- a/Documentation/devicetree/bindings/watchdog/sunxi-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/sunxi-wdt.txt
@@ -6,9 +6,13 @@ Required properties:
"allwinner,sun6i-a31-wdt"
- reg : Specifies base physical address and size of the registers.
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
Example:
wdt: watchdog@1c20c90 {
compatible = "allwinner,sun4i-a10-wdt";
reg = <0x01c20c90 0x10>;
+ timeout-sec = <10>;
};
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index 9728fa32c357..55f166bec0ca 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -39,7 +39,7 @@
#define DRV_VERSION "1.0"
static bool nowayout = WATCHDOG_NOWAYOUT;
-static unsigned int timeout = WDT_MAX_TIMEOUT;
+static unsigned int timeout;
/*
* This structure stores the register offsets for different variants
--
2.15.1
Hi, Marcus
The changes you made for dt-bindings and driver should be put into
separate patches.
And the property timeout-sec seems to be generic enough to all devices,
so why not add a common document to describe it and allow those devices
to refer to, like other dt-bindings for other kinds of devices usually
did.
Sean
On Sat, 2018-02-10 at 10:19 +0100, Marcus Folkesson wrote:
> watchdog_init_timeout() will allways pick timeout_param since it
> defaults to a valid timeout.
>
> By following best practice described in
> Documentation/watchdog/watchdog-kernel-api.txt, it also
> let us to set timout-sec property in devicetree.
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> ---
> Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 4 ++++
> drivers/watchdog/mtk_wdt.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> index 5b38a30e608c..859dee167b91 100644
> --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> @@ -11,9 +11,13 @@ Required properties:
>
> - reg : Specifies base physical address and size of the registers.
>
> +Optional properties:
> +- timeout-sec: contains the watchdog timeout in seconds.
> +
> Example:
>
> wdt: watchdog@10000000 {
> compatible = "mediatek,mt6589-wdt";
> reg = <0x10000000 0x18>;
> + timeout-sec = <10>;
> };
> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 7ed417a765c7..fcdc10ec28a3 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -57,7 +57,7 @@
> #define DRV_VERSION "1.0"
>
> static bool nowayout = WATCHDOG_NOWAYOUT;
> -static unsigned int timeout = WDT_MAX_TIMEOUT;
> +static unsigned int timeout;
>
> struct mtk_wdt_dev {
> struct watchdog_device wdt_dev;
Hello Sean,
On Sat, Feb 10, 2018 at 07:10:02PM +0800, Sean Wang wrote:
>
> Hi, Marcus
>
> The changes you made for dt-bindings and driver should be put into
> separate patches.
I actually thought about it but chose to have it in the same patch because I
did not see any direct advantage to separating them.
But I can do that.
I will come up with a v3 with this change if no one thinks differently.
>
> And the property timeout-sec seems to be generic enough to all devices,
> so why not add a common document to describe it and allow those devices
> to refer to, like other dt-bindings for other kinds of devices usually
> did.
It should be, but it requires that the driver is using
watchdog_init_timeout() to set timeout, most of the drivers does not.
Most drivers does not even use the watchdog API but register itself as
misc devices.
When we have all wdt drivers using the watchdog API, we should probably
move the dt-binding to a common document.
>
> Sean
>
Thanks,
Best regards
Marcus Folkesson
> On Sat, 2018-02-10 at 10:19 +0100, Marcus Folkesson wrote:
> > watchdog_init_timeout() will allways pick timeout_param since it
> > defaults to a valid timeout.
> >
> > By following best practice described in
> > Documentation/watchdog/watchdog-kernel-api.txt, it also
> > let us to set timout-sec property in devicetree.
> >
> > Signed-off-by: Marcus Folkesson <[email protected]>
> > Reviewed-by: Guenter Roeck <[email protected]>
> > ---
> > Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 4 ++++
> > drivers/watchdog/mtk_wdt.c | 2 +-
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > index 5b38a30e608c..859dee167b91 100644
> > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > @@ -11,9 +11,13 @@ Required properties:
> >
> > - reg : Specifies base physical address and size of the registers.
> >
> > +Optional properties:
> > +- timeout-sec: contains the watchdog timeout in seconds.
> > +
> > Example:
> >
> > wdt: watchdog@10000000 {
> > compatible = "mediatek,mt6589-wdt";
> > reg = <0x10000000 0x18>;
> > + timeout-sec = <10>;
> > };
> > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > index 7ed417a765c7..fcdc10ec28a3 100644
> > --- a/drivers/watchdog/mtk_wdt.c
> > +++ b/drivers/watchdog/mtk_wdt.c
> > @@ -57,7 +57,7 @@
> > #define DRV_VERSION "1.0"
> >
> > static bool nowayout = WATCHDOG_NOWAYOUT;
> > -static unsigned int timeout = WDT_MAX_TIMEOUT;
> > +static unsigned int timeout;
> >
> > struct mtk_wdt_dev {
> > struct watchdog_device wdt_dev;
>
>
On 10/02/2018 at 10:19:05 +0100, Marcus Folkesson wrote:
> watchdog_init_timeout() will allways pick timeout_param since it
> defaults to a valid timeout.
>
> Following best practice described in
> Documentation/watchdog/watchdog-kernel-api.txt to make use of
> the parameter logic.
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
Reviewed-by: Alexandre Belloni <[email protected]>
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Hello Sean,
On Sat, Feb 10, 2018 at 01:43:28PM +0100, Marcus Folkesson wrote:
> Hello Sean,
>
> On Sat, Feb 10, 2018 at 07:10:02PM +0800, Sean Wang wrote:
> >
> > Hi, Marcus
> >
> > The changes you made for dt-bindings and driver should be put into
> > separate patches.
>
> I actually thought about it but chose to have it in the same patch because I
> did not see any direct advantage to separating them.
>
> But I can do that.
> I will come up with a v3 with this change if no one thinks differently.
>
When looking at the git log, I'm not that convinced it should be
separate patches.
For example, I found a4f741e3e157c3a5c8aea5f2ea62b692fbf17338 that is
doing the exact same thing as this patch.
There is plenty of patches that mixes the code change and dt bindings
updates.
Could it not be useful to overview both the implementation and
dt-mapping change in one view?
If you or anyone else still think it should be separated, please let me know and I will
come up with a v3.
> >
> > And the property timeout-sec seems to be generic enough to all devices,
> > so why not add a common document to describe it and allow those devices
> > to refer to, like other dt-bindings for other kinds of devices usually
> > did.
>
> It should be, but it requires that the driver is using
> watchdog_init_timeout() to set timeout, most of the drivers does not.
> Most drivers does not even use the watchdog API but register itself as
> misc devices.
>
> When we have all wdt drivers using the watchdog API, we should probably
> move the dt-binding to a common document.
>
> >
> > Sean
> >
>
> Thanks,
>
> Best regards
> Marcus Folkesson
>
> > On Sat, 2018-02-10 at 10:19 +0100, Marcus Folkesson wrote:
> > > watchdog_init_timeout() will allways pick timeout_param since it
> > > defaults to a valid timeout.
> > >
> > > By following best practice described in
> > > Documentation/watchdog/watchdog-kernel-api.txt, it also
> > > let us to set timout-sec property in devicetree.
> > >
> > > Signed-off-by: Marcus Folkesson <[email protected]>
> > > Reviewed-by: Guenter Roeck <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 4 ++++
> > > drivers/watchdog/mtk_wdt.c | 2 +-
> > > 2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > index 5b38a30e608c..859dee167b91 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > @@ -11,9 +11,13 @@ Required properties:
> > >
> > > - reg : Specifies base physical address and size of the registers.
> > >
> > > +Optional properties:
> > > +- timeout-sec: contains the watchdog timeout in seconds.
> > > +
> > > Example:
> > >
> > > wdt: watchdog@10000000 {
> > > compatible = "mediatek,mt6589-wdt";
> > > reg = <0x10000000 0x18>;
> > > + timeout-sec = <10>;
> > > };
> > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> > > index 7ed417a765c7..fcdc10ec28a3 100644
> > > --- a/drivers/watchdog/mtk_wdt.c
> > > +++ b/drivers/watchdog/mtk_wdt.c
> > > @@ -57,7 +57,7 @@
> > > #define DRV_VERSION "1.0"
> > >
> > > static bool nowayout = WATCHDOG_NOWAYOUT;
> > > -static unsigned int timeout = WDT_MAX_TIMEOUT;
> > > +static unsigned int timeout;
> > >
> > > struct mtk_wdt_dev {
> > > struct watchdog_device wdt_dev;
> >
> >
Best regards
Marcus Folkesson
On 02/10/2018 12:12 PM, Marcus Folkesson wrote:
> Hello Sean,
>
> On Sat, Feb 10, 2018 at 01:43:28PM +0100, Marcus Folkesson wrote:
>> Hello Sean,
>>
>> On Sat, Feb 10, 2018 at 07:10:02PM +0800, Sean Wang wrote:
>>>
>>> Hi, Marcus
>>>
>>> The changes you made for dt-bindings and driver should be put into
>>> separate patches.
>>
>> I actually thought about it but chose to have it in the same patch because I
>> did not see any direct advantage to separating them.
>>
>> But I can do that.
>> I will come up with a v3 with this change if no one thinks differently.
>>
>
> When looking at the git log, I'm not that convinced it should be
> separate patches.
>
> For example, I found a4f741e3e157c3a5c8aea5f2ea62b692fbf17338 that is
> doing the exact same thing as this patch.
>
> There is plenty of patches that mixes the code change and dt bindings
> updates.
> Could it not be useful to overview both the implementation and
> dt-mapping change in one view?
>
> If you or anyone else still think it should be separated, please let me know and I will
> come up with a v3.
>
If we were talking about something new, specifically new and unapproved DT bindings,
it should be separate patches. However, that is not the case here. The DT bindings
are well established. Sure, we could be pedantic and request a split into two
patches. However, the only benefit of that would be more work for the maintainers,
ie Wim and myself (including me having to send this e-mail). I don't really see
the point of that.
I have already sent my Reviewed-by:, and I don't intend to withdraw it.
Thanks,
Guenter
On Sat, 2018-02-10 at 17:52 -0800, Guenter Roeck wrote:
> On 02/10/2018 12:12 PM, Marcus Folkesson wrote:
> > Hello Sean,
> >
> > On Sat, Feb 10, 2018 at 01:43:28PM +0100, Marcus Folkesson wrote:
> >> Hello Sean,
> >>
> >> On Sat, Feb 10, 2018 at 07:10:02PM +0800, Sean Wang wrote:
> >>>
> >>> Hi, Marcus
> >>>
> >>> The changes you made for dt-bindings and driver should be put into
> >>> separate patches.
> >>
> >> I actually thought about it but chose to have it in the same patch because I
> >> did not see any direct advantage to separating them.
> >>
> >> But I can do that.
> >> I will come up with a v3 with this change if no one thinks differently.
> >>
> >
> > When looking at the git log, I'm not that convinced it should be
> > separate patches.
> >
> > For example, I found a4f741e3e157c3a5c8aea5f2ea62b692fbf17338 that is
> > doing the exact same thing as this patch.
> >
> > There is plenty of patches that mixes the code change and dt bindings
> > updates.
> > Could it not be useful to overview both the implementation and
> > dt-mapping change in one view?
> >
> > If you or anyone else still think it should be separated, please let me know and I will
> > come up with a v3.
> >
>
> If we were talking about something new, specifically new and unapproved DT bindings,
> it should be separate patches. However, that is not the case here. The DT bindings
> are well established. Sure, we could be pedantic and request a split into two
> patches. However, the only benefit of that would be more work for the maintainers,
> ie Wim and myself (including me having to send this e-mail). I don't really see
> the point of that.
>
> I have already sent my Reviewed-by:, and I don't intend to withdraw it.
>
Hi, both
Sorry for that if I caused any inconvenience to you. I didn't really
insist on if the patch is needed to split into two, which totally
depends on whether dt maintainers like it.
The change for dt-binding is usually added as a split patch with
dt-bindings as a prefix. This way I thought dt maintainers is not
easy to miss those patches and also can give some useful feedback
for them.
Sean
> Thanks,
> Guenter
>
On 02/10/2018 11:46 PM, Sean Wang wrote:
> On Sat, 2018-02-10 at 17:52 -0800, Guenter Roeck wrote:
>> On 02/10/2018 12:12 PM, Marcus Folkesson wrote:
>>> Hello Sean,
>>>
>>> On Sat, Feb 10, 2018 at 01:43:28PM +0100, Marcus Folkesson wrote:
>>>> Hello Sean,
>>>>
>>>> On Sat, Feb 10, 2018 at 07:10:02PM +0800, Sean Wang wrote:
>>>>>
>>>>> Hi, Marcus
>>>>>
>>>>> The changes you made for dt-bindings and driver should be put into
>>>>> separate patches.
>>>>
>>>> I actually thought about it but chose to have it in the same patch because I
>>>> did not see any direct advantage to separating them.
>>>>
>>>> But I can do that.
>>>> I will come up with a v3 with this change if no one thinks differently.
>>>>
>>>
>>> When looking at the git log, I'm not that convinced it should be
>>> separate patches.
>>>
>>> For example, I found a4f741e3e157c3a5c8aea5f2ea62b692fbf17338 that is
>>> doing the exact same thing as this patch.
>>>
>>> There is plenty of patches that mixes the code change and dt bindings
>>> updates.
>>> Could it not be useful to overview both the implementation and
>>> dt-mapping change in one view?
>>>
>>> If you or anyone else still think it should be separated, please let me know and I will
>>> come up with a v3.
>>>
>>
>> If we were talking about something new, specifically new and unapproved DT bindings,
>> it should be separate patches. However, that is not the case here. The DT bindings
>> are well established. Sure, we could be pedantic and request a split into two
>> patches. However, the only benefit of that would be more work for the maintainers,
>> ie Wim and myself (including me having to send this e-mail). I don't really see
>> the point of that.
>>
>> I have already sent my Reviewed-by:, and I don't intend to withdraw it.
>>
> Hi, both
>
> Sorry for that if I caused any inconvenience to you. I didn't really
> insist on if the patch is needed to split into two, which totally
> depends on whether dt maintainers like it.
>
> The change for dt-binding is usually added as a split patch with
> dt-bindings as a prefix. This way I thought dt maintainers is not
> easy to miss those patches and also can give some useful feedback
> for them.
>
With all the trouble this one-line change is making, I feel inclined to drop
the patch for this driver.
Guenter
On Sat, Feb 10, 2018 at 10:19:11AM +0100, Marcus Folkesson wrote:
> watchdog_init_timeout() will allways pick timeout_param since it
> defaults to a valid timeout.
>
> Following best practice described in
> Documentation/watchdog/watchdog-kernel-api.txt to make use of
> the parameter logic.
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> ---
>
> v2:
> - Set .timeout in coh901327_wdt structure declaration.
> - Set .min_timeout to 1 instead of 0. I could not find a datasheet
> for coh901327, so I'm not sure if 0 is valid. However, 0 seems
> wrong to me and most driver has 1 as min value. If it should
> be 0, please let me know and I have to set another initial
> value for margin.
Makes sense to me.
>
> drivers/watchdog/coh901327_wdt.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/coh901327_wdt.c b/drivers/watchdog/coh901327_wdt.c
> index 4410337f4f7f..5d8eb9a30879 100644
> --- a/drivers/watchdog/coh901327_wdt.c
> +++ b/drivers/watchdog/coh901327_wdt.c
> @@ -67,7 +67,9 @@
> #define U300_WDOG_IFR_WILL_BARK_IRQ_FORCE_ENABLE 0x0001U
>
> /* Default timeout in seconds = 1 minute */
> -static unsigned int margin = 60;
> +#define U300_WDOG_DEFAULT_TIMEOUT 60
> +
> +static unsigned int margin;
I just realized that 'margin' instead of the actual timeout is used
at the end of the probe function to display the selected timeout.
This will have to change as well (and I'll have to go back to the
other patches to make sure that this doesn't happen there as well).
Guenter
> static int irq;
> static void __iomem *virtbase;
> static struct device *parent;
> @@ -235,8 +237,9 @@ static struct watchdog_device coh901327_wdt = {
> * timeout register is max
> * 0x7FFF = 327670ms ~= 327s.
> */
> - .min_timeout = 0,
> + .min_timeout = 1,
> .max_timeout = 327,
> + .timeout = U300_WDOG_DEFAULT_TIMEOUT,
> };
>
> static int __exit coh901327_remove(struct platform_device *pdev)
> @@ -315,9 +318,7 @@ static int __init coh901327_probe(struct platform_device *pdev)
> goto out_no_irq;
> }
>
> - ret = watchdog_init_timeout(&coh901327_wdt, margin, dev);
> - if (ret < 0)
> - coh901327_wdt.timeout = 60;
> + watchdog_init_timeout(&coh901327_wdt, margin, dev);
>
> coh901327_wdt.parent = dev;
> ret = watchdog_register_device(&coh901327_wdt);
On Sat, Feb 10, 2018 at 10:19 AM, Marcus Folkesson
<[email protected]> wrote:
> watchdog_init_timeout() will allways pick timeout_param since it
> defaults to a valid timeout.
>
> Following best practice described in
> Documentation/watchdog/watchdog-kernel-api.txt to make use of
> the parameter logic.
>
> Signed-off-by: Marcus Folkesson <[email protected]>
> ---
>
> v2:
> - Set .timeout in coh901327_wdt structure declaration.
> - Set .min_timeout to 1 instead of 0. I could not find a datasheet
> for coh901327, so I'm not sure if 0 is valid. However, 0 seems
> wrong to me and most driver has 1 as min value. If it should
> be 0, please let me know and I have to set another initial
> value for margin.
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij