| Message ID | 20220429131349.21229-1-pali@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: watchdog: max63xx: Add GPIO binding | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Fri, Apr 29, 2022 at 03:13:49PM +0200, Pali Rohár wrote: > @@ -27,6 +27,7 @@ > #include <linux/io.h> > #include <linux/slab.h> > #include <linux/property.h> > +#include <linux/gpio/consumer.h> It would be better to keep them alphabetically. Anyway, they aren't sorted originally... > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > +{ > + spin_lock(&wdt->lock); Does it really need to acquire the lock? It looks like the lock is to prevent concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set(). > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > + udelay(1); Doesn't it need to include <linux/delay.h> for udelay()? > @@ -225,10 +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) Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of the check for -ENOENT. > + return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi), > + "unable to request gpio: %ld\n", > + PTR_ERR(wdt->gpio_wdi)); It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() prints the error. > err = max63xx_mmap_init(pdev, wdt); > if (err) > return err; > > + if (!IS_ERR(wdt->gpio_wdi)) > + wdt->ping = max63xx_gpio_ping; Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was provided? It would be better to mention the behavior in the commit message. Also, could both the assignments of `wdt->gpio_wdi` and `wdt->ping` happen after max63xx_mmap_init()?
On 5/2/22 20:57, Tzung-Bi Shih wrote: > On Fri, Apr 29, 2022 at 03:13:49PM +0200, Pali Rohár wrote: >> @@ -27,6 +27,7 @@ >> #include <linux/io.h> >> #include <linux/slab.h> >> #include <linux/property.h> >> +#include <linux/gpio/consumer.h> > > It would be better to keep them alphabetically. Anyway, they aren't sorted > originally... > >> +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) >> +{ >> + spin_lock(&wdt->lock); > > Does it really need to acquire the lock? It looks like the lock is to prevent > concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set(). > Actually, that doesn't work at all. spin_lock() directly contradicts with gpiod_set_value_cansleep(). >> + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); >> + udelay(1); > > Doesn't it need to include <linux/delay.h> for udelay()? > >> @@ -225,10 +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) > > Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of > the check for -ENOENT. > >> + return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi), >> + "unable to request gpio: %ld\n", >> + PTR_ERR(wdt->gpio_wdi)); > > It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() > prints the error. > >> err = max63xx_mmap_init(pdev, wdt); >> if (err) >> return err; >> >> + if (!IS_ERR(wdt->gpio_wdi)) >> + wdt->ping = max63xx_gpio_ping; > > Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was > provided? It would be better to mention the behavior in the commit message. > > Also, could both the assignments of `wdt->gpio_wdi` and `wdt->ping` happen > after max63xx_mmap_init()?
On Fri, Apr 29, 2022 at 03:13:48PM +0200, Pali Rohár wrote: > GPIO is optional and used for WDI logic. Nowhere is WDI defined. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++ > 1 file changed, 4 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: Usually, we want a name here. Maybe wdi-gpios, but I don't know what WDI is nor have I read the pin name in the datasheet for inspiration. > + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory > + maxItems: 1 > + > required: > - compatible > - reg > -- > 2.20.1 > >
On Tuesday 03 May 2022 16:51:37 Rob Herring wrote: > On Fri, Apr 29, 2022 at 03:13:48PM +0200, Pali Rohár wrote: > > GPIO is optional and used for WDI logic. > > Nowhere is WDI defined. > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++ > > 1 file changed, 4 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: > > Usually, we want a name here. Maybe wdi-gpios, but I don't know what WDI > is nor have I read the pin name in the datasheet for inspiration. WDI is name of logic used in the datasheet, it is abbreviation of WatchDog Input (meaning that from watchdog chip this GPIO has input direction). I'm not sure if we need to put gpio direction into the property name or also word watchdog (or its some abbrev) into name. As node is already named "watchdog" and direction depends on point of view (chip vs CPU), which can be in DTS misleading (because DTS describe direction from CPU point of view). What for sure makes sense is extending description by explaining WDI abbreviation. > > + description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory > > + maxItems: 1 > > + > > required: > > - compatible > > - reg > > -- > > 2.20.1 > > > >
On Monday 02 May 2022 21:37:16 Guenter Roeck wrote: > On 5/2/22 20:57, Tzung-Bi Shih wrote: > > On Fri, Apr 29, 2022 at 03:13:49PM +0200, Pali Rohár wrote: > > > @@ -27,6 +27,7 @@ > > > #include <linux/io.h> > > > #include <linux/slab.h> > > > #include <linux/property.h> > > > +#include <linux/gpio/consumer.h> > > > > It would be better to keep them alphabetically. Anyway, they aren't sorted > > originally... > > > > > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > > > +{ > > > + spin_lock(&wdt->lock); > > > > Does it really need to acquire the lock? It looks like the lock is to prevent > > concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set(). > > > > Actually, that doesn't work at all. spin_lock() directly contradicts > with gpiod_set_value_cansleep(). > > > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > > > + udelay(1); > > > > Doesn't it need to include <linux/delay.h> for udelay()? > > > > > @@ -225,10 +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) > > > > Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of > > the check for -ENOENT. > > > > > + return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi), > > > + "unable to request gpio: %ld\n", > > > + PTR_ERR(wdt->gpio_wdi)); > > > > It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() > > prints the error. > > > > > err = max63xx_mmap_init(pdev, wdt); > > > if (err) > > > return err; > > > + if (!IS_ERR(wdt->gpio_wdi)) > > > + wdt->ping = max63xx_gpio_ping; > > > > Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was > > provided? It would be better to mention the behavior in the commit message. > > > > Also, could both the assignments of `wdt->gpio_wdi` and `wdt->ping` happen > > after max63xx_mmap_init()? > Hello! I'm going to look at all these issues. Recently I sent max63 watchdog driver also into U-Boot and seems that I mixed DTS and driver code between U-Boot and Kernel... and tested something mixed. I will do new testing again, and will check that I'm testing correct code.
On Wednesday 04 May 2022 00:05:50 Pali Rohár wrote: > On Monday 02 May 2022 21:37:16 Guenter Roeck wrote: > > On 5/2/22 20:57, Tzung-Bi Shih wrote: > > > On Fri, Apr 29, 2022 at 03:13:49PM +0200, Pali Rohár wrote: > > > > @@ -27,6 +27,7 @@ > > > > #include <linux/io.h> > > > > #include <linux/slab.h> > > > > #include <linux/property.h> > > > > +#include <linux/gpio/consumer.h> > > > > > > It would be better to keep them alphabetically. Anyway, they aren't sorted > > > originally... > > > > > > > +static void max63xx_gpio_ping(struct max63xx_wdt *wdt) > > > > +{ > > > > + spin_lock(&wdt->lock); > > > > > > Does it really need to acquire the lock? It looks like the lock is to prevent > > > concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set(). > > > > > > > Actually, that doesn't work at all. spin_lock() directly contradicts > > with gpiod_set_value_cansleep(). > > > > > > + gpiod_set_value_cansleep(wdt->gpio_wdi, 1); > > > > + udelay(1); > > > > > > Doesn't it need to include <linux/delay.h> for udelay()? > > > > > > > @@ -225,10 +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) > > > > > > Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of > > > the check for -ENOENT. > > > > > > > + return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi), > > > > + "unable to request gpio: %ld\n", > > > > + PTR_ERR(wdt->gpio_wdi)); > > > > > > It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() > > > prints the error. > > > > > > > err = max63xx_mmap_init(pdev, wdt); > > > > if (err) > > > > return err; > > > > + if (!IS_ERR(wdt->gpio_wdi)) > > > > + wdt->ping = max63xx_gpio_ping; > > > > > > Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was > > > provided? It would be better to mention the behavior in the commit message. > > > > > > Also, could both the assignments of `wdt->gpio_wdi` and `wdt->ping` happen > > > after max63xx_mmap_init()? > > > > Hello! I'm going to look at all these issues. Recently I sent max63 > watchdog driver also into U-Boot and seems that I mixed DTS and driver > code between U-Boot and Kernel... and tested something mixed. > > I will do new testing again, and will check that I'm testing correct > code. Hello! Now I sent a new version V3. I have tested it on PowerPC P2020 based board where watchdog registers are exported via CPLD and new V3 version is working fine.
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
GPIO is optional and used for WDI logic. Signed-off-by: Pali Rohár <pali@kernel.org> --- Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++ 1 file changed, 4 insertions(+)