[PATCH 0/2] DS1374 Watchdog fixes

[PATCH 0/2] DS1374 Watchdog fixes

[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

[rtc-linux] [PATCH 0/2] DS1374 Watchdog fixes

[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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks

[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

[rtc-linux] [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks

[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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL

[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

[rtc-linux] [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL

[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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Alexandre Belloni]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Alexandre Belloni]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

[-- 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 --]

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Moritz Fischer]

[-- Attachment #1: Type: text/plain, Size: 2207 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 ...

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

[rtc-linux] Re: [PATCH 0/2] DS1374 Watchdog fixes

[Guenter Roeck]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks

[Alexandre Belloni]

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

[rtc-linux] Re: [PATCH 1/2] rtc: ds1374: wdt: Fix issue with timeout scaling from secs to wdt ticks

[Alexandre Belloni]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL

[Alexandre Belloni]

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

[rtc-linux] Re: [PATCH 2/2] rtc: ds1374: wdt: Fix stop/start ioctl always returning -EINVAL

[Alexandre Belloni]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.