Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Mon, Apr 2, 2018 at 9:32 AM, Andrew Lunn wrote: >> The 'use case' we have been using this in for a couple years is that >> users who want to use this watchdog will enable it externally (we have >> a command in the bootloader) and if enabled the kernel driver (that >> I'm proposing here which we've been using out-of-tree) will register >> the watchdog device and the userspace watchdog process can open the >> device and start tickling it. If the watchdog is never enabled (or >> disabled via the bootloader command) the kernel driver fails to probe >> and the SoC's watchdog can be used. > > Hi Tim > > Is there any reason not to give the user the choice to use both > watchdogs? Normally you write drivers to expose the hardware, and then > let the users choice if they want to use it. > Andrew, I don't know what value the SoC watchdog has if you have the GSC watchdog available that can power cycle the board but I would agree normally you would expose all of them. I suppose this may be a different case because I'm expecting the user to go and manually enable the watchdog instead of using the kernel to do it which means if its not enabled its not really available to be used. Tim
Re: [v3,4/4] watchdog: add Gateworks System Controller support
> The 'use case' we have been using this in for a couple years is that > users who want to use this watchdog will enable it externally (we have > a command in the bootloader) and if enabled the kernel driver (that > I'm proposing here which we've been using out-of-tree) will register > the watchdog device and the userspace watchdog process can open the > device and start tickling it. If the watchdog is never enabled (or > disabled via the bootloader command) the kernel driver fails to probe > and the SoC's watchdog can be used. Hi Tim Is there any reason not to give the user the choice to use both watchdogs? Normally you write drivers to expose the hardware, and then let the users choice if they want to use it. Andrew
Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Fri, Mar 30, 2018 at 11:19 AM, Guenter Roeck wrote:
> On Fri, Mar 30, 2018 at 10:49:38AM -0700, Tim Harvey wrote:
>> On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck wrote:
>> > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
>> >> Signed-off-by: Tim Harvey
>> >> ---
>> >> drivers/watchdog/Kconfig | 10
>> >> drivers/watchdog/Makefile | 1 +
>> >> drivers/watchdog/gsc_wdt.c | 146
>> >> +
>> >> 3 files changed, 157 insertions(+)
>> >> create mode 100644 drivers/watchdog/gsc_wdt.c
>> >>
>>
>> >> +
>> >> +static const struct watchdog_info gsc_wdt_info = {
>> >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
>> >
>> > Please confirm that WDIOF_MAGICCLOSE is not set on purpose.
>> >
>> >> + .identity = "GSC Watchdog"
>> >> +};
>> >> +
>>
>> >> +
>> >> +static int gsc_wdt_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> >> + struct device *dev = &pdev->dev;
>> >> + struct gsc_wdt *wdt;
>> >> + int ret;
>> >> + unsigned int reg;
>> >> +
>>
>> >> + /* ensure WD bit enabled */
>> >> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®))
>> >> + return -EIO;
>> >> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) {
>> >
>> > BIT()
>> >
>> >> + dev_err(dev, "not enabled - must be manually enabled\n");
>> >
>> > This doesn't make sense. Bail out if the watchdog is disabled ? Why ?
>> >
>> >> + return -EINVAL;
>> >> + }
>> >> +
>>
>> >> +
>> >> + watchdog_set_nowayout(&wdt->wdt_dev, 1);
>> >
>> > WATCHDOG_NOWAYOUT ?
>> >
>>
>> Guenter,
>>
>> Thanks for the review!
>>
>> The watchdog implementation of the GSC is such that it is enabled and
>> reset via a single non-volatile I2C register bit. If this bit is set
>> the watchdog will start ticking down automatically on board power up.
>> The register definitions don't provide a condition where it can be
>> enabled in a volatile way such that after board power-cycle it is
>> disabled again nor do they provide a separate register for enable vs
>> reset.
>>
>> In the typical case the user boots the board, driver registers
>> watchdog, userspace watchdog daemon enables watchdog and it starts
>> ticking. User now powers down the board and later powers it back up.
>> The watchdog was enabled previously by userspace and the register is
>> non-volatile so the watchdog starts ticking before the kernel driver
>> and watchdog daemon yet the user breaks out into the bootloader or
>> boots a different OS without a watchdog daemon and the board resets
>> without them expecting it. The feature that the watchdog starts
>> ticking at board power-up before the CPU has even fetched code was
>> part of its design and was put there to work around some SoC errata
>> that can cause the CPU to fail to fetch boot code. This has caused me
>> to implement a watchdog driver that never actually 'enables' or
>> 'disables' the watchdog which is why there is no MAGIC CLOSE and why I
>
> Yet the driver does enable and disable the watchdog in its start and stop
> functions. And I have no idea what that has to do with the MAGICCLOSE
> functionality, which is quite orthogonal to the start/stop functionality.
>
>> always set nowayout. Its possible this is a fairly unique case of a
>> watchdog. The probe failure if the watchdog isn't enabled is because I
>> don't want a non-enabled watchdog to get enabled just because the
>> driver/daemon were there.
>>
> Huh ? The whole purpose of a watchdog is for it to be enabled when
> the watchdog device is opened.
>
>> I agree it's a very strange behavior and I'm not sure how to best
>> document or support it with the Linux watchdog API. I welcome any
>> recomendations!
>>
>
> Sorry, I fail to understand your logic.
>
> You do not explain why your code bails out if the watchdog is not already
> running. That does not make sense.
>
> You are saying that you don't want the watchdog driver to enable the watchdog.
> Since its whole purpose is to enable the watchdog if/when the watchdog device
> is opened, that doesn't make sense either.
>
> At the same time, you do not tell the watchdog core that the watchdog is
> already running, meaning the system _will_ reboot unless the watchdog
> device is opened within the watchdog timeout period. Again, that does not
> make sense.
>
> Maybe it all makes sense to you, but not to me, sorry.
Guenter,
Right, I'm likely not explaining it well. Let me show the registers
and describe the feature from the GSC perspective:
I2C registers: non-volatile registers (battery backed)
0x01: GSC_CTRL_1: Sleep Wakeup Timer Control
bit 4: WATCHDOG_TIME: 0=30 second timeout, 1=60 second timeout
bit 5: WATCHDOG_ENABLE: 0=disable watchdog, 1=enable/reset watchdog timer
The GSC has the ability to enable/disable the primary board power
supply. In the event that the watchdog timer is enabled and reaches 0
it will power cycle the board by
Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Fri, Mar 30, 2018 at 10:49:38AM -0700, Tim Harvey wrote:
> On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck wrote:
> > On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
> >> Signed-off-by: Tim Harvey
> >> ---
> >> drivers/watchdog/Kconfig | 10
> >> drivers/watchdog/Makefile | 1 +
> >> drivers/watchdog/gsc_wdt.c | 146
> >> +
> >> 3 files changed, 157 insertions(+)
> >> create mode 100644 drivers/watchdog/gsc_wdt.c
> >>
>
> >> +
> >> +static const struct watchdog_info gsc_wdt_info = {
> >> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> >
> > Please confirm that WDIOF_MAGICCLOSE is not set on purpose.
> >
> >> + .identity = "GSC Watchdog"
> >> +};
> >> +
>
> >> +
> >> +static int gsc_wdt_probe(struct platform_device *pdev)
> >> +{
> >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> >> + struct device *dev = &pdev->dev;
> >> + struct gsc_wdt *wdt;
> >> + int ret;
> >> + unsigned int reg;
> >> +
>
> >> + /* ensure WD bit enabled */
> >> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®))
> >> + return -EIO;
> >> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) {
> >
> > BIT()
> >
> >> + dev_err(dev, "not enabled - must be manually enabled\n");
> >
> > This doesn't make sense. Bail out if the watchdog is disabled ? Why ?
> >
> >> + return -EINVAL;
> >> + }
> >> +
>
> >> +
> >> + watchdog_set_nowayout(&wdt->wdt_dev, 1);
> >
> > WATCHDOG_NOWAYOUT ?
> >
>
> Guenter,
>
> Thanks for the review!
>
> The watchdog implementation of the GSC is such that it is enabled and
> reset via a single non-volatile I2C register bit. If this bit is set
> the watchdog will start ticking down automatically on board power up.
> The register definitions don't provide a condition where it can be
> enabled in a volatile way such that after board power-cycle it is
> disabled again nor do they provide a separate register for enable vs
> reset.
>
> In the typical case the user boots the board, driver registers
> watchdog, userspace watchdog daemon enables watchdog and it starts
> ticking. User now powers down the board and later powers it back up.
> The watchdog was enabled previously by userspace and the register is
> non-volatile so the watchdog starts ticking before the kernel driver
> and watchdog daemon yet the user breaks out into the bootloader or
> boots a different OS without a watchdog daemon and the board resets
> without them expecting it. The feature that the watchdog starts
> ticking at board power-up before the CPU has even fetched code was
> part of its design and was put there to work around some SoC errata
> that can cause the CPU to fail to fetch boot code. This has caused me
> to implement a watchdog driver that never actually 'enables' or
> 'disables' the watchdog which is why there is no MAGIC CLOSE and why I
Yet the driver does enable and disable the watchdog in its start and stop
functions. And I have no idea what that has to do with the MAGICCLOSE
functionality, which is quite orthogonal to the start/stop functionality.
> always set nowayout. Its possible this is a fairly unique case of a
> watchdog. The probe failure if the watchdog isn't enabled is because I
> don't want a non-enabled watchdog to get enabled just because the
> driver/daemon were there.
>
Huh ? The whole purpose of a watchdog is for it to be enabled when
the watchdog device is opened.
> I agree it's a very strange behavior and I'm not sure how to best
> document or support it with the Linux watchdog API. I welcome any
> recomendations!
>
Sorry, I fail to understand your logic.
You do not explain why your code bails out if the watchdog is not already
running. That does not make sense.
You are saying that you don't want the watchdog driver to enable the watchdog.
Since its whole purpose is to enable the watchdog if/when the watchdog device
is opened, that doesn't make sense either.
At the same time, you do not tell the watchdog core that the watchdog is
already running, meaning the system _will_ reboot unless the watchdog
device is opened within the watchdog timeout period. Again, that does not
make sense.
Maybe it all makes sense to you, but not to me, sorry.
Guenter
Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Thu, Mar 29, 2018 at 6:07 PM, Guenter Roeck wrote:
> On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
>> Signed-off-by: Tim Harvey
>> ---
>> drivers/watchdog/Kconfig | 10
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/gsc_wdt.c | 146
>> +
>> 3 files changed, 157 insertions(+)
>> create mode 100644 drivers/watchdog/gsc_wdt.c
>>
>> +
>> +static const struct watchdog_info gsc_wdt_info = {
>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
>
> Please confirm that WDIOF_MAGICCLOSE is not set on purpose.
>
>> + .identity = "GSC Watchdog"
>> +};
>> +
>> +
>> +static int gsc_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> + struct device *dev = &pdev->dev;
>> + struct gsc_wdt *wdt;
>> + int ret;
>> + unsigned int reg;
>> +
>> + /* ensure WD bit enabled */
>> + if (regmap_read(gsc->regmap, GSC_CTRL_1, ®))
>> + return -EIO;
>> + if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) {
>
> BIT()
>
>> + dev_err(dev, "not enabled - must be manually enabled\n");
>
> This doesn't make sense. Bail out if the watchdog is disabled ? Why ?
>
>> + return -EINVAL;
>> + }
>> +
>> +
>> + watchdog_set_nowayout(&wdt->wdt_dev, 1);
>
> WATCHDOG_NOWAYOUT ?
>
Guenter,
Thanks for the review!
The watchdog implementation of the GSC is such that it is enabled and
reset via a single non-volatile I2C register bit. If this bit is set
the watchdog will start ticking down automatically on board power up.
The register definitions don't provide a condition where it can be
enabled in a volatile way such that after board power-cycle it is
disabled again nor do they provide a separate register for enable vs
reset.
In the typical case the user boots the board, driver registers
watchdog, userspace watchdog daemon enables watchdog and it starts
ticking. User now powers down the board and later powers it back up.
The watchdog was enabled previously by userspace and the register is
non-volatile so the watchdog starts ticking before the kernel driver
and watchdog daemon yet the user breaks out into the bootloader or
boots a different OS without a watchdog daemon and the board resets
without them expecting it. The feature that the watchdog starts
ticking at board power-up before the CPU has even fetched code was
part of its design and was put there to work around some SoC errata
that can cause the CPU to fail to fetch boot code. This has caused me
to implement a watchdog driver that never actually 'enables' or
'disables' the watchdog which is why there is no MAGIC CLOSE and why I
always set nowayout. Its possible this is a fairly unique case of a
watchdog. The probe failure if the watchdog isn't enabled is because I
don't want a non-enabled watchdog to get enabled just because the
driver/daemon were there.
I agree it's a very strange behavior and I'm not sure how to best
document or support it with the Linux watchdog API. I welcome any
recomendations!
Regards,
Tim
Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Thu, Mar 29, 2018 at 06:07:57PM -0700, Guenter Roeck wrote:
> On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
> > Signed-off-by: Tim Harvey
> > ---
> > drivers/watchdog/Kconfig | 10
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/gsc_wdt.c | 146
> > +
> > 3 files changed, 157 insertions(+)
> > create mode 100644 drivers/watchdog/gsc_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index ca200d1..c9d4b2e 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL
> > arch_initcall.
> > If in doubt, say N.
> >
> > +config GSC_WATCHDOG
> > + tristate "Gateworks System Controller (GSC) Watchdog support"
> > + depends on MFD_GATEWORKS_GSC
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support for the GSC Watchdog.
> > +
> > + This driver can also be built as a module. If so the module
> > + will be called gsc_wdt.
> > +
> > config MENF21BMC_WATCHDOG
> > tristate "MEN 14F021P00 BMC Watchdog"
> > depends on MFD_MENF21BMC || COMPILE_TEST
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 715a210..499327e 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> > obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
> > obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o
> > obj-$(CONFIG_GPIO_WATCHDOG)+= gpio_wdt.o
> > +obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o
> > obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o
> > obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o
> > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
> > diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c
> > new file mode 100644
> > index 000..b43d083
> > --- /dev/null
> > +++ b/drivers/watchdog/gsc_wdt.c
> > @@ -0,0 +1,146 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Copyright (C) 2018 Gateworks Corporation
> > + *
> > + * This driver registers a Linux Watchdog for the GSC
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define WDT_DEFAULT_TIMEOUT60
> > +
> > +struct gsc_wdt {
> > + struct watchdog_device wdt_dev;
> > + struct gsc_dev *gsc;
> > +};
> > +
> > +static int gsc_wdt_start(struct watchdog_device *wdd)
> > +{
> > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
>
> Please use BIT().
>
> > + int ret;
> > +
> > + dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout);
>
> I don't think those debug messages add any value.
>
> > +
> > + /* clear first as regmap_update_bits will not write if no change */
> > + ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> > + if (ret)
> > + return ret;
> > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg);
> > +}
> > +
> > +static int gsc_wdt_stop(struct watchdog_device *wdd)
> > +{
> > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
> > +
>
> BIT(). You might as well drop the variable and just use
> BIT(GSC_CTRL_1_WDT_ENABLE) below.
>
> > + dev_dbg(wdd->parent, "%s\n", __func__);
> > +
> > + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> > +}
> > +
> > +static int gsc_wdt_set_timeout(struct watchdog_device *wdd,
> > + unsigned int timeout)
> > +{
> > + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > + unsigned int long_sel = 0;
> > +
> > + dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout);
> > +
> > + switch (timeout) {
> > + case 60:
> > + long_sel = (1 << GSC_CTRL_1_WDT_TIME);
> > + case 30:
> > + regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
> > + (1 << GSC_CTRL_1_WDT_TIME),
>
> BIT()
>
> > + (long_sel << GSC_CTRL_1_WDT_TIME));
This looks wrong. Are you sure that in case of 60 timeout you want to
actually write
((1 << GSC_CTRL_1_WDT_TIME) << GSC_CTRL_1_WDT_TIME)
to the register?
Or you meant to write:
long_sel = timeout > 30;
regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
(long_sel << GSC_CTRL_1_WDT_TIME),
Thanks.
--
Dmitry
Re: [v3,4/4] watchdog: add Gateworks System Controller support
On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
> Signed-off-by: Tim Harvey
> ---
> drivers/watchdog/Kconfig | 10
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/gsc_wdt.c | 146
> +
> 3 files changed, 157 insertions(+)
> create mode 100644 drivers/watchdog/gsc_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index ca200d1..c9d4b2e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL
> arch_initcall.
> If in doubt, say N.
>
> +config GSC_WATCHDOG
> + tristate "Gateworks System Controller (GSC) Watchdog support"
> + depends on MFD_GATEWORKS_GSC
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the GSC Watchdog.
> +
> + This driver can also be built as a module. If so the module
> + will be called gsc_wdt.
> +
> config MENF21BMC_WATCHDOG
> tristate "MEN 14F021P00 BMC Watchdog"
> depends on MFD_MENF21BMC || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 715a210..499327e 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
> obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o
> obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o
> +obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o
> obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o
> obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o
> obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
> diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c
> new file mode 100644
> index 000..b43d083
> --- /dev/null
> +++ b/drivers/watchdog/gsc_wdt.c
> @@ -0,0 +1,146 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2018 Gateworks Corporation
> + *
> + * This driver registers a Linux Watchdog for the GSC
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define WDT_DEFAULT_TIMEOUT 60
> +
> +struct gsc_wdt {
> + struct watchdog_device wdt_dev;
> + struct gsc_dev *gsc;
> +};
> +
> +static int gsc_wdt_start(struct watchdog_device *wdd)
> +{
> + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
Please use BIT().
> + int ret;
> +
> + dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout);
I don't think those debug messages add any value.
> +
> + /* clear first as regmap_update_bits will not write if no change */
> + ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> + if (ret)
> + return ret;
> + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg);
> +}
> +
> +static int gsc_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
> +
BIT(). You might as well drop the variable and just use
BIT(GSC_CTRL_1_WDT_ENABLE) below.
> + dev_dbg(wdd->parent, "%s\n", __func__);
> +
> + return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> +}
> +
> +static int gsc_wdt_set_timeout(struct watchdog_device *wdd,
> +unsigned int timeout)
> +{
> + struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> + unsigned int long_sel = 0;
> +
> + dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout);
> +
> + switch (timeout) {
> + case 60:
> + long_sel = (1 << GSC_CTRL_1_WDT_TIME);
> + case 30:
> + regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
> +(1 << GSC_CTRL_1_WDT_TIME),
BIT()
> +(long_sel << GSC_CTRL_1_WDT_TIME));
> + wdd->timeout = timeout;
> + return 0;
> + }
Please use rounding and accept other values as well. We don't want to let
user space guessing valid timeouts.
> +
> + return -EINVAL;
> +}
> +
> +static const struct watchdog_info gsc_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
Please confirm that WDIOF_MAGICCLOSE is not set on purpose.
> + .identity = "GSC Watchdog"
> +};
> +
> +static const struct watchdog_ops gsc_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = gsc_wdt_start,
> + .stop = gsc_wdt_stop,
> + .set_timeout= gsc_wdt_set_timeout,
> +};
> +
> +static int gsc_wdt_probe(struct platform_device *pdev)
> +{
> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct gsc_wdt *wdt;
> + int ret;
> + unsigned int reg;
> +
> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + /* ensure GSC fw supports WD functionality */
> + if (gsc->fwver < 44) {
What if gsc is NULL ?
