* [PATCH 0/2] DS1374 Watchdog fixes @ 2017-04-24 22:05 Moritz Fischer 2017-04-24 22:05 ` [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks Moritz Fischer ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Moritz Fischer @ 2017-04-24 22:05 UTC (permalink / raw) To: linux-kernel Cc: moritz.fischer, linux-watchdog, linux, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams, Moritz Fischer Hi all, this series fixes two issues that I ran into when trying to use the watchdog part of the DS1374 on of our products. This series is basically a precursor to some further cleanup work, that turns the DS1374 into a MFD device with an RTC and WDT in separate drivers [1] each integrated in either the watchdog and the rtc frameworks. I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling the watchdog behavior and currently I'm investigating how to make that work via DT. Watchdog maintainers, do you have an idea on how to do that in a non breaking fashion? Thanks, Moritz [1] https://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git/log/?h=wip/mfd-ds1374-rfc Moritz Fischer (2): rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL drivers/rtc/rtc-ds1374.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks 2017-04-24 22:05 [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer @ 2017-04-24 22:05 ` Moritz Fischer 2017-05-04 12:47 ` Alexandre Belloni 2017-04-24 22:05 ` [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL Moritz Fischer 2017-04-25 5:03 ` [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck 2 siblings, 1 reply; 14+ messages in thread From: Moritz Fischer @ 2017-04-24 22:05 UTC (permalink / raw) To: linux-kernel Cc: moritz.fischer, linux-watchdog, linux, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams, Moritz Fischer Fix commit 920f91e50c5b ("drivers/rtc/rtc-ds1374.c: add watchdog support") The issue is that the internal counter that triggers the watchdog reset is actually running at 4096 Hz instead of 1Hz, therefore the value given by userland (in sec) needs to be multiplied by 4096 to get the correct behavior. Signed-off-by: Moritz Fischer <mdf@kernel.org> --- drivers/rtc/rtc-ds1374.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index 4cd115e..2a8b5b3 100644 --- a/drivers/rtc/rtc-ds1374.c +++ b/drivers/rtc/rtc-ds1374.c @@ -525,6 +525,10 @@ static long ds1374_wdt_ioctl(struct file *file, unsigned int cmd, if (get_user(new_margin, (int __user *)arg)) return -EFAULT; + /* the hardware's tick rate is 4096 Hz, so + * the counter value needs to be scaled accordingly + */ + new_margin <<= 12; if (new_margin < 1 || new_margin > 16777216) return -EINVAL; @@ -533,7 +537,8 @@ static long ds1374_wdt_ioctl(struct file *file, unsigned int cmd, ds1374_wdt_ping(); /* fallthrough */ case WDIOC_GETTIMEOUT: - return put_user(wdt_margin, (int __user *)arg); + /* when returning ... inverse is true */ + return put_user((wdt_margin >> 12), (int __user *)arg); case WDIOC_SETOPTIONS: if (copy_from_user(&options, (int __user *)arg, sizeof(int))) return -EFAULT; -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks 2017-04-24 22:05 ` [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks Moritz Fischer @ 2017-05-04 12:47 ` Alexandre Belloni 0 siblings, 0 replies; 14+ messages in thread From: Alexandre Belloni @ 2017-05-04 12:47 UTC (permalink / raw) To: Moritz Fischer Cc: linux-kernel, moritz.fischer, linux-watchdog, linux, wim, a.zummo, rtc-linux, alex.williams On 24/04/2017 at 15:05:11 -0700, Moritz Fischer wrote: > Fix commit 920f91e50c5b ("drivers/rtc/rtc-ds1374.c: add watchdog support") > > The issue is that the internal counter that triggers the watchdog reset > is actually running at 4096 Hz instead of 1Hz, therefore the value > given by userland (in sec) needs to be multiplied by 4096 to get the > correct behavior. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > drivers/rtc/rtc-ds1374.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Applied, thanks. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL 2017-04-24 22:05 [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer 2017-04-24 22:05 ` [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks Moritz Fischer @ 2017-04-24 22:05 ` Moritz Fischer 2017-05-04 12:47 ` Alexandre Belloni 2017-04-25 5:03 ` [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck 2 siblings, 1 reply; 14+ messages in thread From: Moritz Fischer @ 2017-04-24 22:05 UTC (permalink / raw) To: linux-kernel Cc: moritz.fischer, linux-watchdog, linux, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams, Moritz Fischer Fix commit 920f91e50c5b ("drivers/rtc/rtc-ds1374.c: add watchdog support") The WDIOC_SETOPTIONS case in the watchdog ioctl would alwayss falls through to the -EINVAL case. This is wrong since thew watchdog does actually get stopped or started correctly. Signed-off-by: Moritz Fischer <mdf@kernel.org> --- drivers/rtc/rtc-ds1374.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index 2a8b5b3..38a2e9e 100644 --- a/drivers/rtc/rtc-ds1374.c +++ b/drivers/rtc/rtc-ds1374.c @@ -546,14 +546,15 @@ static long ds1374_wdt_ioctl(struct file *file, unsigned int cmd, if (options & WDIOS_DISABLECARD) { pr_info("disable watchdog\n"); ds1374_wdt_disable(); + return 0; } if (options & WDIOS_ENABLECARD) { pr_info("enable watchdog\n"); ds1374_wdt_settimeout(wdt_margin); ds1374_wdt_ping(); + return 0; } - return -EINVAL; } return -ENOTTY; -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL 2017-04-24 22:05 ` [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL Moritz Fischer @ 2017-05-04 12:47 ` Alexandre Belloni 0 siblings, 0 replies; 14+ messages in thread From: Alexandre Belloni @ 2017-05-04 12:47 UTC (permalink / raw) To: Moritz Fischer Cc: linux-kernel, moritz.fischer, linux-watchdog, linux, wim, a.zummo, rtc-linux, alex.williams On 24/04/2017 at 15:05:12 -0700, Moritz Fischer wrote: > Fix commit 920f91e50c5b ("drivers/rtc/rtc-ds1374.c: add watchdog support") > > The WDIOC_SETOPTIONS case in the watchdog ioctl would alwayss falls > through to the -EINVAL case. This is wrong since thew watchdog does > actually get stopped or started correctly. > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > drivers/rtc/rtc-ds1374.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Applied, thanks. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-24 22:05 [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer 2017-04-24 22:05 ` [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks Moritz Fischer 2017-04-24 22:05 ` [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL Moritz Fischer @ 2017-04-25 5:03 ` Guenter Roeck 2017-04-25 14:55 ` Moritz Fischer 2 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2017-04-25 5:03 UTC (permalink / raw) To: Moritz Fischer, linux-kernel Cc: moritz.fischer, linux-watchdog, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams On 04/24/2017 03:05 PM, Moritz Fischer wrote: > Hi all, > > this series fixes two issues that I ran into when trying to use the > watchdog part of the DS1374 on of our products. > > This series is basically a precursor to some further cleanup work, > that turns the DS1374 into a MFD device with an RTC and WDT in > separate drivers [1] each integrated in either the watchdog and > the rtc frameworks. > > I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling > the watchdog behavior and currently I'm investigating how to make > that work via DT. > > Watchdog maintainers, do you have an idea on how to do that in a > non breaking fashion? > Depends on what you mean with "non breaking". Just using the normal mfd mechanisms, ie define an mfd cell for each client driver, should work. Do you see any problems with that ? Either case, that doesn't seem to be a watchdog driver problem, or am I missing something ? > Thanks, > Moritz > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git/log/?h=wip/mfd-ds1374-rfc > > Moritz Fischer (2): > rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt > ticks > rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL > I don't really see the point of doing that if you plan to move the watchdog part of the driver into the watchdog directory. We for sure won't accept a watchdog driver that does not use the watchdog infrastructure. Regarding + /* WHY? */ + ds1374->wdd.timeout = t; Assuming you mean why the driver has to set the timeout value - not every watchdog hardware supports timeouts in multiples of 1 second. The driver is expected to set the value to the real timeout, not to the timeout asked for by the infrastructure. Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 5:03 ` [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck @ 2017-04-25 14:55 ` Moritz Fischer 2017-04-25 16:17 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Moritz Fischer @ 2017-04-25 14:55 UTC (permalink / raw) To: Guenter Roeck Cc: Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams Hi Guenter, On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 04/24/2017 03:05 PM, Moritz Fischer wrote: >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling >> the watchdog behavior and currently I'm investigating how to make >> that work via DT. >> >> Watchdog maintainers, do you have an idea on how to do that in a >> non breaking fashion? >> > > Depends on what you mean with "non breaking". Just using the normal mfd > mechanisms, ie define an mfd cell for each client driver, should work. > Do you see any problems with that ? Either case, that doesn't seem > to be a watchdog driver problem, or am I missing something ? Well so currently watchdog behavior is selected (out of the two options alarm, or watchdog) by enabling the configuration option mentioned above. If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>; to select the behavior in the mfd for example, won't that break people that relied on the old behavior? If everyone involved is ok with that, I'm happy to just add it to the binding. > I don't really see the point of doing that if you plan to move the watchdog > part of the driver into the watchdog directory. We for sure won't accept a > watchdog driver that does not use the watchdog infrastructure. The idea was to fix what's broken currently (this patchset) and then refactor. But if you prefer I can do all in one go instead. > > Regarding > + /* WHY? */ > + ds1374->wdd.timeout = t; > > Assuming you mean why the driver has to set the timeout value - not every > watchdog hardware supports timeouts in multiples of 1 second. The driver > is expected to set the value to the real timeout, not to the timeout asked > for by the infrastructure. Yeah that branch is work in progress and needs cleanup. Leftover from testing, before I had understood why. Branch needs cleanup. It also doesn't really use regmap_update_bits etc. Thanks for the explanation, Moritz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 14:55 ` Moritz Fischer @ 2017-04-25 16:17 ` Guenter Roeck 2017-04-25 16:32 ` Alexandre Belloni 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2017-04-25 16:17 UTC (permalink / raw) To: Moritz Fischer Cc: Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, alexandre.belloni, rtc-linux, alex.williams On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote: > Hi Guenter, > > On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On 04/24/2017 03:05 PM, Moritz Fischer wrote: > > >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling > >> the watchdog behavior and currently I'm investigating how to make > >> that work via DT. > >> > >> Watchdog maintainers, do you have an idea on how to do that in a > >> non breaking fashion? > >> > > > > Depends on what you mean with "non breaking". Just using the normal mfd > > mechanisms, ie define an mfd cell for each client driver, should work. > > Do you see any problems with that ? Either case, that doesn't seem > > to be a watchdog driver problem, or am I missing something ? > > Well so currently watchdog behavior is selected (out of the two options alarm, > or watchdog) by enabling the configuration option mentioned above. > If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>; > to select the behavior in the mfd for example, won't that break people that > relied on the old behavior? If everyone involved is ok with that, I'm happy > to just add it to the binding. > Sorry, I must be missing something. Looking into the driver code, my understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in addition to rtc functionality, not one or the other. Sure you would need a different configuration option if you were to move the watchdog code into drivers/watchdog, but other than that I don't really understand the problem. What is the issue with, for example, config DS1374_WDT bool "Dallas/Maxim DS1374 watchdog timer" depends on MFD_DS1374 help If you say Y here you will get support for the watchdog timer in the Dallas Semiconductor DS1374 real-time clock chips. in drivers/watchdog/Kconfig, and the mfd driver instantiating it like any other mfd client driver ? Either case, limiting support to DT based systems seems to be the wrong approach. There might be Intel platforms using this chip. > > I don't really see the point of doing that if you plan to move the watchdog > > part of the driver into the watchdog directory. We for sure won't accept a > > watchdog driver that does not use the watchdog infrastructure. > > The idea was to fix what's broken currently (this patchset) and then refactor. > But if you prefer I can do all in one go instead. > It just seemed a waste to me to change/fix a function which is going to be removed in a subsequent patch (I seem to recall that there was a fix to the ioctl function). If/when you move the driver to drivers/watchdog, please make sure that it doesn't use any instantiation related static variables (ie other than module parameters). > > > > Regarding > > + /* WHY? */ > > + ds1374->wdd.timeout = t; > > > > Assuming you mean why the driver has to set the timeout value - not every > > watchdog hardware supports timeouts in multiples of 1 second. The driver > > is expected to set the value to the real timeout, not to the timeout asked > > for by the infrastructure. > > Yeah that branch is work in progress and needs cleanup. Leftover from testing, > before I had understood why. Branch needs cleanup. It also doesn't really use > regmap_update_bits etc. > Well, you did enhance the code to use regmap, which by itself is a significant improvement ... Thanks, Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 16:17 ` Guenter Roeck @ 2017-04-25 16:32 ` Alexandre Belloni 2017-04-25 16:58 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Alexandre Belloni @ 2017-04-25 16:32 UTC (permalink / raw) To: Guenter Roeck Cc: Moritz Fischer, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams On 25/04/2017 at 09:17:43 -0700, Guenter Roeck wrote: > On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote: > > Hi Guenter, > > > > On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > On 04/24/2017 03:05 PM, Moritz Fischer wrote: > > > > >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling > > >> the watchdog behavior and currently I'm investigating how to make > > >> that work via DT. > > >> > > >> Watchdog maintainers, do you have an idea on how to do that in a > > >> non breaking fashion? > > >> > > > > > > Depends on what you mean with "non breaking". Just using the normal mfd > > > mechanisms, ie define an mfd cell for each client driver, should work. > > > Do you see any problems with that ? Either case, that doesn't seem > > > to be a watchdog driver problem, or am I missing something ? > > > > Well so currently watchdog behavior is selected (out of the two options alarm, > > or watchdog) by enabling the configuration option mentioned above. > > If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>; > > to select the behavior in the mfd for example, won't that break people that > > relied on the old behavior? If everyone involved is ok with that, I'm happy > > to just add it to the binding. > > > > Sorry, I must be missing something. Looking into the driver code, my > understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in > addition to rtc functionality, not one or the other. Sure you would need > a different configuration option if you were to move the watchdog code into > drivers/watchdog, but other than that I don't really understand the problem. > What is the issue with, for example, > The watchdog functionality and the rtc alarm are mutually exclusive. > > The idea was to fix what's broken currently (this patchset) and then refactor. > > But if you prefer I can do all in one go instead. > > > > It just seemed a waste to me to change/fix a function which is going to > be removed in a subsequent patch (I seem to recall that there was a fix > to the ioctl function). > I'd say that it depends on whether you want to backport the fixes to the stable kernels. Backporting the full rework is probably riskier. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 16:32 ` Alexandre Belloni @ 2017-04-25 16:58 ` Guenter Roeck 2017-04-25 19:58 ` Moritz Fischer 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2017-04-25 16:58 UTC (permalink / raw) To: Alexandre Belloni Cc: Moritz Fischer, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams On Tue, Apr 25, 2017 at 06:32:04PM +0200, Alexandre Belloni wrote: > On 25/04/2017 at 09:17:43 -0700, Guenter Roeck wrote: > > On Tue, Apr 25, 2017 at 07:55:28AM -0700, Moritz Fischer wrote: > > > Hi Guenter, > > > > > > On Mon, Apr 24, 2017 at 10:03 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 04/24/2017 03:05 PM, Moritz Fischer wrote: > > > > > > >> I'm very unhappy with the CONFIG_DRV_RTC_DS1374_WDT way of enabling > > > >> the watchdog behavior and currently I'm investigating how to make > > > >> that work via DT. > > > >> > > > >> Watchdog maintainers, do you have an idea on how to do that in a > > > >> non breaking fashion? > > > >> > > > > > > > > Depends on what you mean with "non breaking". Just using the normal mfd > > > > mechanisms, ie define an mfd cell for each client driver, should work. > > > > Do you see any problems with that ? Either case, that doesn't seem > > > > to be a watchdog driver problem, or am I missing something ? > > > > > > Well so currently watchdog behavior is selected (out of the two options alarm, > > > or watchdog) by enabling the configuration option mentioned above. > > > If I change this over to use a dt-based approach like dallas,ds1374-mode = <2>; > > > to select the behavior in the mfd for example, won't that break people that > > > relied on the old behavior? If everyone involved is ok with that, I'm happy > > > to just add it to the binding. > > > > > > > Sorry, I must be missing something. Looking into the driver code, my > > understanding is that CONFIG_RTC_DRV_DS1374_WDT enables the watchdog in > > addition to rtc functionality, not one or the other. Sure you would need > > a different configuration option if you were to move the watchdog code into > > drivers/watchdog, but other than that I don't really understand the problem. > > What is the issue with, for example, > > > > The watchdog functionality and the rtc alarm are mutually exclusive. > Ah, I missed the "n" in various #ifndef statements. I can't really comment on how to solve that; I simply don't know. Also, even with a dt property, it still would be necessary to have a non-DT means to configure one or the other. Making whatever solution backward compatible also seems tricky; I don't have a solution for that problem either. > > > The idea was to fix what's broken currently (this patchset) and then refactor. > > > But if you prefer I can do all in one go instead. > > > > > > > It just seemed a waste to me to change/fix a function which is going to > > be removed in a subsequent patch (I seem to recall that there was a fix > > to the ioctl function). > > > > I'd say that it depends on whether you want to backport the fixes to the > stable kernels. Backporting the full rework is probably riskier. > Good point. Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 16:58 ` Guenter Roeck @ 2017-04-25 19:58 ` Moritz Fischer 2017-04-25 20:22 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Moritz Fischer @ 2017-04-25 19:58 UTC (permalink / raw) To: Guenter Roeck Cc: Alexandre Belloni, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote: > Ah, I missed the "n" in various #ifndef statements. > > I can't really comment on how to solve that; I simply don't know. > Also, even with a dt property, it still would be necessary to have > a non-DT means to configure one or the other. Making whatever solution > backward compatible also seems tricky; I don't have a solution for that > problem either. How does one do these things in a non-dt context? Platform data? I'd let the MFD select the 'mode'. Maybe being backwards compatible isn't possible in any case. Is there a rule somewhere that we guarantee you'll never have to change your CONFIG_FOO options? > > > > > The idea was to fix what's broken currently (this patchset) and then refactor. > > > > But if you prefer I can do all in one go instead. > > > > > > > > > > It just seemed a waste to me to change/fix a function which is going to > > > be removed in a subsequent patch (I seem to recall that there was a fix > > > to the ioctl function). > > > > > > > I'd say that it depends on whether you want to backport the fixes to the > > stable kernels. Backporting the full rework is probably riskier. I suck at communicating these days. But yeah. That was basically my concern when I split it up into 'Fixes' and 'Rework'. Mostly since the rework might take a couple of rounds of review, while the fix can unbrick stuff (might still need review of course) Cheers, Moritz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 19:58 ` Moritz Fischer @ 2017-04-25 20:22 ` Guenter Roeck 2017-04-25 20:34 ` Moritz Fischer 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2017-04-25 20:22 UTC (permalink / raw) To: Moritz Fischer Cc: Alexandre Belloni, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote: > On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote: > > > Ah, I missed the "n" in various #ifndef statements. > > > > I can't really comment on how to solve that; I simply don't know. > > Also, even with a dt property, it still would be necessary to have > > a non-DT means to configure one or the other. Making whatever solution > > backward compatible also seems tricky; I don't have a solution for that > > problem either. > > How does one do these things in a non-dt context? Platform data? I'd let Platform data is out of favor. You'd probably want to use device properties (see drivers/base/property.c). Question though is if this is considered configuration, hardware description, or both. Presumably the watchdog only makes sense if the reset signal is wired, and the alarm only makes sense if the interrupt is wired, but what if both are wired ? > the MFD select the 'mode'. Maybe being backwards compatible isn't > possible in any case. Is there a rule somewhere that we guarantee you'll > never have to change your CONFIG_FOO options? > That would be nice, but no, there is no such rule. You can probably argue that no published kernel configuration enables the watchdog flag, so there is nothing to be concerned about. Guenter > > > > > > > The idea was to fix what's broken currently (this patchset) and then refactor. > > > > > But if you prefer I can do all in one go instead. > > > > > > > > > > > > > It just seemed a waste to me to change/fix a function which is going to > > > > be removed in a subsequent patch (I seem to recall that there was a fix > > > > to the ioctl function). > > > > > > > > > > I'd say that it depends on whether you want to backport the fixes to the > > > stable kernels. Backporting the full rework is probably riskier. > > I suck at communicating these days. But yeah. That was basically my > concern when I split it up into 'Fixes' and 'Rework'. > > Mostly since the rework might take a couple of rounds of review, while the > fix can unbrick stuff (might still need review of course) > > Cheers, > > Moritz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 20:22 ` Guenter Roeck @ 2017-04-25 20:34 ` Moritz Fischer 2017-04-25 21:05 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Moritz Fischer @ 2017-04-25 20:34 UTC (permalink / raw) To: Guenter Roeck Cc: Moritz Fischer, Alexandre Belloni, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams [-- Attachment #1: Type: text/plain, Size: 1732 bytes --] On Tue, Apr 25, 2017 at 01:22:10PM -0700, Guenter Roeck wrote: > On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote: > > On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote: > > > > > Ah, I missed the "n" in various #ifndef statements. > > > > > > I can't really comment on how to solve that; I simply don't know. > > > Also, even with a dt property, it still would be necessary to have > > > a non-DT means to configure one or the other. Making whatever solution > > > backward compatible also seems tricky; I don't have a solution for that > > > problem either. > > > > How does one do these things in a non-dt context? Platform data? I'd let > > Platform data is out of favor. You'd probably want to use device properties > (see drivers/base/property.c). Question though is if this is considered > configuration, hardware description, or both. Presumably the watchdog > only makes sense if the reset signal is wired, and the alarm only makes > sense if the interrupt is wired, but what if both are wired ? To make things worse you can even remap the reset output to the INT pin (which my platform does). I'll look at device properties. Thanks for the pointer. > > > the MFD select the 'mode'. Maybe being backwards compatible isn't > > possible in any case. Is there a rule somewhere that we guarantee you'll > > never have to change your CONFIG_FOO options? > > > > That would be nice, but no, there is no such rule. You can probably argue > that no published kernel configuration enables the watchdog flag, > so there is nothing to be concerned about. Alright, cool. Thanks Moritz PS: Haven't forgotten about the cros-ec-hwmon patch that I sent out ... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] DS1374 Watchdog fixes 2017-04-25 20:34 ` Moritz Fischer @ 2017-04-25 21:05 ` Guenter Roeck 0 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2017-04-25 21:05 UTC (permalink / raw) To: Moritz Fischer Cc: Alexandre Belloni, Linux Kernel Mailing List, Moritz Fischer, linux-watchdog, wim, a.zummo, rtc-linux, alex.williams On Tue, Apr 25, 2017 at 01:34:18PM -0700, Moritz Fischer wrote: > On Tue, Apr 25, 2017 at 01:22:10PM -0700, Guenter Roeck wrote: > > On Tue, Apr 25, 2017 at 12:58:36PM -0700, Moritz Fischer wrote: > > > On Tue, Apr 25, 2017 at 09:58:24AM -0700, Guenter Roeck wrote: > > > > > > > Ah, I missed the "n" in various #ifndef statements. > > > > > > > > I can't really comment on how to solve that; I simply don't know. > > > > Also, even with a dt property, it still would be necessary to have > > > > a non-DT means to configure one or the other. Making whatever solution > > > > backward compatible also seems tricky; I don't have a solution for that > > > > problem either. > > > > > > How does one do these things in a non-dt context? Platform data? I'd let > > > > Platform data is out of favor. You'd probably want to use device properties > > (see drivers/base/property.c). Question though is if this is considered > > configuration, hardware description, or both. Presumably the watchdog > > only makes sense if the reset signal is wired, and the alarm only makes > > sense if the interrupt is wired, but what if both are wired ? > > To make things worse you can even remap the reset output to the INT pin > (which my platform does). > So that is what the weird 250ms "interrupt signal" is for. I had wondered what that is supposed to be used for. > I'll look at device properties. Thanks for the pointer. > > > > > > the MFD select the 'mode'. Maybe being backwards compatible isn't > > > possible in any case. Is there a rule somewhere that we guarantee you'll > > > never have to change your CONFIG_FOO options? > > > > > > > That would be nice, but no, there is no such rule. You can probably argue > > that no published kernel configuration enables the watchdog flag, > > so there is nothing to be concerned about. > > Alright, cool. Thanks > > Moritz > > PS: Haven't forgotten about the cros-ec-hwmon patch that I sent out ... No, still trying to get internal feedback. I'll have to ask again. Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-05-04 12:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-24 22:05 [PATCH 0/2] DS1374 Watchdog fixes Moritz Fischer 2017-04-24 22:05 ` [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks Moritz Fischer 2017-05-04 12:47 ` Alexandre Belloni 2017-04-24 22:05 ` [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL Moritz Fischer 2017-05-04 12:47 ` Alexandre Belloni 2017-04-25 5:03 ` [PATCH 0/2] DS1374 Watchdog fixes Guenter Roeck 2017-04-25 14:55 ` Moritz Fischer 2017-04-25 16:17 ` Guenter Roeck 2017-04-25 16:32 ` Alexandre Belloni 2017-04-25 16:58 ` Guenter Roeck 2017-04-25 19:58 ` Moritz Fischer 2017-04-25 20:22 ` Guenter Roeck 2017-04-25 20:34 ` Moritz Fischer 2017-04-25 21:05 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).