| Message ID | 20220428091603.6838-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: max63xx_wdt: Add support for specifying WDI logic via GPIO | expand |
On 4/28/22 02:16, Pali Rohár wrote: > On some boards is WDI logic of max6370 chip connected via GPIO. So extend > max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO. > > Signed-off-by: Pali Rohár <pali@kernel.org> How is that different to just using the gpio watchdog driver ? Guenter > --- > .../bindings/watchdog/maxim,max63xx.yaml | 4 +++ > drivers/watchdog/max63xx_wdt.c | 28 +++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > index ab9641e845db..a97aa0135ef9 100644 > --- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > +++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > @@ -27,6 +27,10 @@ properties: > description: This is a 1-byte memory-mapped address > maxItems: 1 > > + gpios: > + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory > + maxItems: 1 > + > required: > - compatible > - reg > diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c > index 9e1541cfae0d..eaf00c3f06a5 100644 > --- a/drivers/watchdog/max63xx_wdt.c > +++ b/drivers/watchdog/max63xx_wdt.c > @@ -27,6 +27,7 @@ > #include <linux/io.h> > #include <linux/slab.h> > #include <linux/property.h> > +#include <linux/gpio/consumer.h> > > #define DEFAULT_HEARTBEAT 60 > #define MAX_HEARTBEAT 60 > @@ -53,6 +54,9 @@ struct max63xx_wdt { > void __iomem *base; > spinlock_t lock; > > + /* GPIOs */ > + struct gpio_desc *gpio_wdi; > + > /* WDI and WSET bits write access routines */ > void (*ping)(struct max63xx_wdt *wdt); > void (*set)(struct max63xx_wdt *wdt, u8 set); > @@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = { > .identity = "max63xx Watchdog", > }; > > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > +{ > + spin_lock(&wdt->lock); > + > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > + udelay(1); > + gpiod_set_value_cansleep(wdt->gpio_wdi, 0); > + > + spin_unlock(&wdt->lock); > +} > + > static void max63xx_mmap_ping(struct max63xx_wdt *wdt) > { > u8 val; > @@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev) > return -EINVAL; > } > > + wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT); > + if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) { > + if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER) > + dev_err(dev, "unable to request gpio: %ld\n", > + PTR_ERR(wdt->gpio_wdi)); > + return PTR_ERR(wdt->gpio_wdi); > + } > + > + if (!IS_ERR(wdt->gpio_wdi)) > + wdt->ping = max63xx_gpio_ping; > + else > + wdt->gpio_wdi = NULL; > + > err = max63xx_mmap_init(pdev, wdt); > if (err) > return err;
On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote: > On 4/28/22 02:16, Pali Rohár wrote: > > On some boards is WDI logic of max6370 chip connected via GPIO. So extend > > max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > How is that different to just using the gpio watchdog driver ? GPIO watchdog driver does not support max6370 memory mapped configuration. With this change, max6370 can use memory mapped space for watchdog configuration and GPIO WDI for pinging. > Guenter > > > --- > > .../bindings/watchdog/maxim,max63xx.yaml | 4 +++ > > drivers/watchdog/max63xx_wdt.c | 28 +++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > index ab9641e845db..a97aa0135ef9 100644 > > --- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > +++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > @@ -27,6 +27,10 @@ properties: > > description: This is a 1-byte memory-mapped address > > maxItems: 1 > > + gpios: > > + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory > > + maxItems: 1 > > + > > required: > > - compatible > > - reg > > diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c > > index 9e1541cfae0d..eaf00c3f06a5 100644 > > --- a/drivers/watchdog/max63xx_wdt.c > > +++ b/drivers/watchdog/max63xx_wdt.c > > @@ -27,6 +27,7 @@ > > #include <linux/io.h> > > #include <linux/slab.h> > > #include <linux/property.h> > > +#include <linux/gpio/consumer.h> > > #define DEFAULT_HEARTBEAT 60 > > #define MAX_HEARTBEAT 60 > > @@ -53,6 +54,9 @@ struct max63xx_wdt { > > void __iomem *base; > > spinlock_t lock; > > + /* GPIOs */ > > + struct gpio_desc *gpio_wdi; > > + > > /* WDI and WSET bits write access routines */ > > void (*ping)(struct max63xx_wdt *wdt); > > void (*set)(struct max63xx_wdt *wdt, u8 set); > > @@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = { > > .identity = "max63xx Watchdog", > > }; > > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > > +{ > > + spin_lock(&wdt->lock); > > + > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > > + udelay(1); > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 0); > > + > > + spin_unlock(&wdt->lock); > > +} > > + > > static void max63xx_mmap_ping(struct max63xx_wdt *wdt) > > { > > u8 val; > > @@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > + wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT); > > + if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) { > > + if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER) > > + dev_err(dev, "unable to request gpio: %ld\n", > > + PTR_ERR(wdt->gpio_wdi)); > > + return PTR_ERR(wdt->gpio_wdi); > > + } > > + > > + if (!IS_ERR(wdt->gpio_wdi)) > > + wdt->ping = max63xx_gpio_ping; > > + else > > + wdt->gpio_wdi = NULL; > > + > > err = max63xx_mmap_init(pdev, wdt); > > if (err) > > return err; >
On 4/28/22 07:32, Pali Rohár wrote: > On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote: >> On 4/28/22 02:16, Pali Rohár wrote: >>> On some boards is WDI logic of max6370 chip connected via GPIO. So extend >>> max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO. >>> >>> Signed-off-by: Pali Rohár <pali@kernel.org> >> >> How is that different to just using the gpio watchdog driver ? > > GPIO watchdog driver does not support max6370 memory mapped > configuration. > > With this change, max6370 can use memory mapped space for watchdog > configuration and GPIO WDI for pinging. > Ok, that makes sense. Comments below. Thanks, Guenter >> Guenter >> >>> --- >>> .../bindings/watchdog/maxim,max63xx.yaml | 4 +++ >>> drivers/watchdog/max63xx_wdt.c | 28 +++++++++++++++++++ >>> 2 files changed, 32 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml >>> index ab9641e845db..a97aa0135ef9 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml >>> +++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml >>> @@ -27,6 +27,10 @@ properties: >>> description: This is a 1-byte memory-mapped address >>> maxItems: 1 >>> + gpios: >>> + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory >>> + maxItems: 1 >>> + >>> required: >>> - compatible >>> - reg Devicetree patches needs to be separate. >>> diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c >>> index 9e1541cfae0d..eaf00c3f06a5 100644 >>> --- a/drivers/watchdog/max63xx_wdt.c >>> +++ b/drivers/watchdog/max63xx_wdt.c >>> @@ -27,6 +27,7 @@ >>> #include <linux/io.h> >>> #include <linux/slab.h> >>> #include <linux/property.h> >>> +#include <linux/gpio/consumer.h> >>> #define DEFAULT_HEARTBEAT 60 >>> #define MAX_HEARTBEAT 60 >>> @@ -53,6 +54,9 @@ struct max63xx_wdt { >>> void __iomem *base; >>> spinlock_t lock; >>> + /* GPIOs */ >>> + struct gpio_desc *gpio_wdi; >>> + >>> /* WDI and WSET bits write access routines */ >>> void (*ping)(struct max63xx_wdt *wdt); >>> void (*set)(struct max63xx_wdt *wdt, u8 set); >>> @@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = { >>> .identity = "max63xx Watchdog", >>> }; >>> +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) >>> +{ >>> + spin_lock(&wdt->lock); >>> + >>> + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); >>> + udelay(1); >>> + gpiod_set_value_cansleep(wdt->gpio_wdi, 0); >>> + >>> + spin_unlock(&wdt->lock); >>> +} >>> + >>> static void max63xx_mmap_ping(struct max63xx_wdt *wdt) >>> { >>> u8 val; >>> @@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev) >>> return -EINVAL; >>> } >>> + wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT); >>> + if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) { >>> + if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER) >>> + dev_err(dev, "unable to request gpio: %ld\n", >>> + PTR_ERR(wdt->gpio_wdi)); Please use dev_err_probe(). >>> + return PTR_ERR(wdt->gpio_wdi); >>> + } >>> + >>> + if (!IS_ERR(wdt->gpio_wdi)) >>> + wdt->ping = max63xx_gpio_ping; >>> + else >>> + wdt->gpio_wdi = NULL; Why set gpio_wdi to NULL? It isn't used if the ping function is not set. >>> + >>> err = max63xx_mmap_init(pdev, wdt); Doesn't this override the gpio ping function set above ? >>> if (err) >>> return err; >>
On Thursday 28 April 2022 08:29:52 Guenter Roeck wrote: > On 4/28/22 07:32, Pali Rohár wrote: > > On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote: > > > On 4/28/22 02:16, Pali Rohár wrote: > > > > On some boards is WDI logic of max6370 chip connected via GPIO. So extend > > > > max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO. > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > > > How is that different to just using the gpio watchdog driver ? > > > > GPIO watchdog driver does not support max6370 memory mapped > > configuration. > > > > With this change, max6370 can use memory mapped space for watchdog > > configuration and GPIO WDI for pinging. > > > > Ok, that makes sense. Comments below. > > Thanks, > Guenter > > > > Guenter > > > > > > > --- > > > > .../bindings/watchdog/maxim,max63xx.yaml | 4 +++ > > > > drivers/watchdog/max63xx_wdt.c | 28 +++++++++++++++++++ > > > > 2 files changed, 32 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > > > index ab9641e845db..a97aa0135ef9 100644 > > > > --- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > > > +++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml > > > > @@ -27,6 +27,10 @@ properties: > > > > description: This is a 1-byte memory-mapped address > > > > maxItems: 1 > > > > + gpios: > > > > + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory > > > > + maxItems: 1 > > > > + > > > > required: > > > > - compatible > > > > - reg > > Devicetree patches needs to be separate. Ok. > > > > diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c > > > > index 9e1541cfae0d..eaf00c3f06a5 100644 > > > > --- a/drivers/watchdog/max63xx_wdt.c > > > > +++ b/drivers/watchdog/max63xx_wdt.c > > > > @@ -27,6 +27,7 @@ > > > > #include <linux/io.h> > > > > #include <linux/slab.h> > > > > #include <linux/property.h> > > > > +#include <linux/gpio/consumer.h> > > > > #define DEFAULT_HEARTBEAT 60 > > > > #define MAX_HEARTBEAT 60 > > > > @@ -53,6 +54,9 @@ struct max63xx_wdt { > > > > void __iomem *base; > > > > spinlock_t lock; > > > > + /* GPIOs */ > > > > + struct gpio_desc *gpio_wdi; > > > > + > > > > /* WDI and WSET bits write access routines */ > > > > void (*ping)(struct max63xx_wdt *wdt); > > > > void (*set)(struct max63xx_wdt *wdt, u8 set); > > > > @@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = { > > > > .identity = "max63xx Watchdog", > > > > }; > > > > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > > > > +{ > > > > + spin_lock(&wdt->lock); > > > > + > > > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > > > > + udelay(1); > > > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 0); > > > > + > > > > + spin_unlock(&wdt->lock); > > > > +} > > > > + > > > > static void max63xx_mmap_ping(struct max63xx_wdt *wdt) > > > > { > > > > u8 val; > > > > @@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev) > > > > return -EINVAL; > > > > } > > > > + wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT); > > > > + if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) { > > > > + if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER) > > > > + dev_err(dev, "unable to request gpio: %ld\n", > > > > + PTR_ERR(wdt->gpio_wdi)); > > Please use dev_err_probe(). Ok. > > > > + return PTR_ERR(wdt->gpio_wdi); > > > > + } > > > > + > > > > + if (!IS_ERR(wdt->gpio_wdi)) > > > > + wdt->ping = max63xx_gpio_ping; > > > > + else > > > > + wdt->gpio_wdi = NULL; > > Why set gpio_wdi to NULL? It isn't used if the ping function is not set. I thought that it would be more safer to have NULL value stored in pointer variable as opposite of some non-NULL value used for the error reporting. But I can remove this part of the code. > > > > + > > > > err = max63xx_mmap_init(pdev, wdt); > > Doesn't this override the gpio ping function set above ? Yes, it does. I forgot to amend local change to this patch. wdt->ping has to be changed to gpio_ping _after_ max63xx_mmap_init call. I will fix all those issues in v2. > > > > if (err) > > > > return err; > > > >
diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml index ab9641e845db..a97aa0135ef9 100644 --- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml @@ -27,6 +27,10 @@ properties: description: This is a 1-byte memory-mapped address maxItems: 1 + gpios: + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory + maxItems: 1 + required: - compatible - reg diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c index 9e1541cfae0d..eaf00c3f06a5 100644 --- a/drivers/watchdog/max63xx_wdt.c +++ b/drivers/watchdog/max63xx_wdt.c @@ -27,6 +27,7 @@ #include <linux/io.h> #include <linux/slab.h> #include <linux/property.h> +#include <linux/gpio/consumer.h> #define DEFAULT_HEARTBEAT 60 #define MAX_HEARTBEAT 60 @@ -53,6 +54,9 @@ struct max63xx_wdt { void __iomem *base; spinlock_t lock; + /* GPIOs */ + struct gpio_desc *gpio_wdi; + /* WDI and WSET bits write access routines */ void (*ping)(struct max63xx_wdt *wdt); void (*set)(struct max63xx_wdt *wdt, u8 set); @@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = { .identity = "max63xx Watchdog", }; +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) +{ + spin_lock(&wdt->lock); + + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); + udelay(1); + gpiod_set_value_cansleep(wdt->gpio_wdi, 0); + + spin_unlock(&wdt->lock); +} + static void max63xx_mmap_ping(struct max63xx_wdt *wdt) { u8 val; @@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev) return -EINVAL; } + wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT); + if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) { + if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER) + dev_err(dev, "unable to request gpio: %ld\n", + PTR_ERR(wdt->gpio_wdi)); + return PTR_ERR(wdt->gpio_wdi); + } + + if (!IS_ERR(wdt->gpio_wdi)) + wdt->ping = max63xx_gpio_ping; + else + wdt->gpio_wdi = NULL; + err = max63xx_mmap_init(pdev, wdt); if (err) return err;
On some boards is WDI logic of max6370 chip connected via GPIO. So extend max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO. Signed-off-by: Pali Rohár <pali@kernel.org> --- .../bindings/watchdog/maxim,max63xx.yaml | 4 +++ drivers/watchdog/max63xx_wdt.c | 28 +++++++++++++++++++ 2 files changed, 32 insertions(+)