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()?
