[PATCH 1/2] watchdog: gpio: Add some local helper variables

[PATCH 1/2] watchdog: gpio: Add some local helper variables

[Linus Walleij]

This add "dev" and "np" variables to make the probe() function
a bit easier to read.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/watchdog/gpio_wdt.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index cb66c2f99ff1..448e6c3ba73c 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -101,6 +101,8 @@ static const struct watchdog_ops gpio_wdt_ops = {
 
 static int gpio_wdt_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct gpio_wdt_priv *priv;
 	enum of_gpio_flags flags;
 	unsigned int hw_margin;
@@ -108,19 +110,19 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	const char *algo;
 	int ret;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, priv);
 
-	priv->gpio = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
+	priv->gpio = of_get_gpio_flags(np, 0, &flags);
 	if (!gpio_is_valid(priv->gpio))
 		return priv->gpio;
 
 	priv->active_low = flags & OF_GPIO_ACTIVE_LOW;
 
-	ret = of_property_read_string(pdev->dev.of_node, "hw_algo", &algo);
+	ret = of_property_read_string(np, "hw_algo", &algo);
 	if (ret)
 		return ret;
 	if (!strcmp(algo, "toggle")) {
@@ -133,12 +135,12 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = devm_gpio_request_one(&pdev->dev, priv->gpio, f,
-				    dev_name(&pdev->dev));
+	ret = devm_gpio_request_one(dev, priv->gpio, f,
+				    dev_name(dev));
 	if (ret)
 		return ret;
 
-	ret = of_property_read_u32(pdev->dev.of_node,
+	ret = of_property_read_u32(np,
 				   "hw_margin_ms", &hw_margin);
 	if (ret)
 		return ret;
@@ -146,7 +148,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	if (hw_margin < 2 || hw_margin > 65535)
 		return -EINVAL;
 
-	priv->always_running = of_property_read_bool(pdev->dev.of_node,
+	priv->always_running = of_property_read_bool(np,
 						     "always-running");
 
 	watchdog_set_drvdata(&priv->wdd, priv);
@@ -155,9 +157,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	priv->wdd.ops		= &gpio_wdt_ops;
 	priv->wdd.min_timeout	= SOFT_TIMEOUT_MIN;
 	priv->wdd.max_hw_heartbeat_ms = hw_margin;
-	priv->wdd.parent	= &pdev->dev;
+	priv->wdd.parent	= dev;
 
-	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
+	if (watchdog_init_timeout(&priv->wdd, 0, dev) < 0)
 		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
 
 	watchdog_stop_on_reboot(&priv->wdd);
-- 
2.13.5

[PATCH 2/2] watchdog: gpio: Convert to use GPIO descriptors

[Linus Walleij]

This converts the GPIO watchdog driver to use GPIO descriptors
instead of relying on the old method to read out GPIO numbers
from the device tree and then using those with the old GPIO
API.

The descriptor API keeps track of whether the line is active
low so we can remove all active low handling and rely on the
GPIO descriptor to deal with this for us.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/watchdog/gpio_wdt.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 448e6c3ba73c..bb699dfa099e 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -12,7 +12,8 @@
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 
@@ -25,8 +26,7 @@ enum {
 };
 
 struct gpio_wdt_priv {
-	int			gpio;
-	bool			active_low;
+	struct gpio_desc	*gpiod;
 	bool			state;
 	bool			always_running;
 	unsigned int		hw_algo;
@@ -35,11 +35,11 @@ struct gpio_wdt_priv {
 
 static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
 {
-	gpio_set_value_cansleep(priv->gpio, !priv->active_low);
+	gpiod_set_value_cansleep(priv->gpiod, 0);
 
 	/* Put GPIO back to tristate */
 	if (priv->hw_algo == HW_ALGO_TOGGLE)
-		gpio_direction_input(priv->gpio);
+		gpiod_direction_input(priv->gpiod);
 }
 
 static int gpio_wdt_ping(struct watchdog_device *wdd)
@@ -50,13 +50,13 @@ static int gpio_wdt_ping(struct watchdog_device *wdd)
 	case HW_ALGO_TOGGLE:
 		/* Toggle output pin */
 		priv->state = !priv->state;
-		gpio_set_value_cansleep(priv->gpio, priv->state);
+		gpiod_set_value_cansleep(priv->gpiod, priv->state);
 		break;
 	case HW_ALGO_LEVEL:
 		/* Pulse */
-		gpio_set_value_cansleep(priv->gpio, !priv->active_low);
+		gpiod_set_value_cansleep(priv->gpiod, 1);
 		udelay(1);
-		gpio_set_value_cansleep(priv->gpio, priv->active_low);
+		gpiod_set_value_cansleep(priv->gpiod, 0);
 		break;
 	}
 	return 0;
@@ -66,8 +66,8 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
 {
 	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	priv->state = priv->active_low;
-	gpio_direction_output(priv->gpio, priv->state);
+	priv->state = 0;
+	gpiod_direction_output(priv->gpiod, priv->state);
 
 	set_bit(WDOG_HW_RUNNING, &wdd->status);
 
@@ -104,9 +104,8 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct gpio_wdt_priv *priv;
-	enum of_gpio_flags flags;
+	enum gpiod_flags gflags;
 	unsigned int hw_margin;
-	unsigned long f = 0;
 	const char *algo;
 	int ret;
 
@@ -116,29 +115,22 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
-	priv->gpio = of_get_gpio_flags(np, 0, &flags);
-	if (!gpio_is_valid(priv->gpio))
-		return priv->gpio;
-
-	priv->active_low = flags & OF_GPIO_ACTIVE_LOW;
-
 	ret = of_property_read_string(np, "hw_algo", &algo);
 	if (ret)
 		return ret;
 	if (!strcmp(algo, "toggle")) {
 		priv->hw_algo = HW_ALGO_TOGGLE;
-		f = GPIOF_IN;
+		gflags = GPIOD_IN;
 	} else if (!strcmp(algo, "level")) {
 		priv->hw_algo = HW_ALGO_LEVEL;
-		f = priv->active_low ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
+		gflags = GPIOD_OUT_LOW;
 	} else {
 		return -EINVAL;
 	}
 
-	ret = devm_gpio_request_one(dev, priv->gpio, f,
-				    dev_name(dev));
-	if (ret)
-		return ret;
+	priv->gpiod = devm_gpiod_get(dev, NULL, gflags);
+	if (IS_ERR(priv->gpiod))
+		return PTR_ERR(priv->gpiod);
 
 	ret = of_property_read_u32(np,
 				   "hw_margin_ms", &hw_margin);
-- 
2.13.5

Re: [PATCH 1/2] watchdog: gpio: Add some local helper variables

[Guenter Roeck]

On 09/26/2017 01:07 PM, Linus Walleij wrote:
> This add "dev" and "np" variables to make the probe() function
> a bit easier to read.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/gpio_wdt.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index cb66c2f99ff1..448e6c3ba73c 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -101,6 +101,8 @@ static const struct watchdog_ops gpio_wdt_ops = {
>   
>   static int gpio_wdt_probe(struct platform_device *pdev)
>   {
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
>   	struct gpio_wdt_priv *priv;
>   	enum of_gpio_flags flags;
>   	unsigned int hw_margin;
> @@ -108,19 +110,19 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	const char *algo;
>   	int ret;
>   
> -	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
>   		return -ENOMEM;
>   
>   	platform_set_drvdata(pdev, priv);
>   
> -	priv->gpio = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
> +	priv->gpio = of_get_gpio_flags(np, 0, &flags);
>   	if (!gpio_is_valid(priv->gpio))
>   		return priv->gpio;
>   
>   	priv->active_low = flags & OF_GPIO_ACTIVE_LOW;
>   
> -	ret = of_property_read_string(pdev->dev.of_node, "hw_algo", &algo);
> +	ret = of_property_read_string(np, "hw_algo", &algo);
>   	if (ret)
>   		return ret;
>   	if (!strcmp(algo, "toggle")) {
> @@ -133,12 +135,12 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	}
>   
> -	ret = devm_gpio_request_one(&pdev->dev, priv->gpio, f,
> -				    dev_name(&pdev->dev));
> +	ret = devm_gpio_request_one(dev, priv->gpio, f,
> +				    dev_name(dev));
>   	if (ret)
>   		return ret;
>   
> -	ret = of_property_read_u32(pdev->dev.of_node,
> +	ret = of_property_read_u32(np,
>   				   "hw_margin_ms", &hw_margin);
>   	if (ret)
>   		return ret;
> @@ -146,7 +148,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	if (hw_margin < 2 || hw_margin > 65535)
>   		return -EINVAL;
>   
> -	priv->always_running = of_property_read_bool(pdev->dev.of_node,
> +	priv->always_running = of_property_read_bool(np,
>   						     "always-running");
>   
>   	watchdog_set_drvdata(&priv->wdd, priv);
> @@ -155,9 +157,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	priv->wdd.ops		= &gpio_wdt_ops;
>   	priv->wdd.min_timeout	= SOFT_TIMEOUT_MIN;
>   	priv->wdd.max_hw_heartbeat_ms = hw_margin;
> -	priv->wdd.parent	= &pdev->dev;
> +	priv->wdd.parent	= dev;
>   
> -	if (watchdog_init_timeout(&priv->wdd, 0, &pdev->dev) < 0)
> +	if (watchdog_init_timeout(&priv->wdd, 0, dev) < 0)
>   		priv->wdd.timeout = SOFT_TIMEOUT_DEF;
>   
>   	watchdog_stop_on_reboot(&priv->wdd);
> 

Re: [PATCH 2/2] watchdog: gpio: Convert to use GPIO descriptors

[Guenter Roeck]

On 09/26/2017 01:07 PM, Linus Walleij wrote:
> This converts the GPIO watchdog driver to use GPIO descriptors
> instead of relying on the old method to read out GPIO numbers
> from the device tree and then using those with the old GPIO
> API.
> 
> The descriptor API keeps track of whether the line is active
> low so we can remove all active low handling and rely on the
> GPIO descriptor to deal with this for us.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/watchdog/gpio_wdt.c | 40 ++++++++++++++++------------------------
>   1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 448e6c3ba73c..bb699dfa099e 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -12,7 +12,8 @@
>   #include <linux/err.h>
>   #include <linux/delay.h>
>   #include <linux/module.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
>   #include <linux/platform_device.h>
>   #include <linux/watchdog.h>
>   
> @@ -25,8 +26,7 @@ enum {
>   };
>   
>   struct gpio_wdt_priv {
> -	int			gpio;
> -	bool			active_low;
> +	struct gpio_desc	*gpiod;
>   	bool			state;
>   	bool			always_running;
>   	unsigned int		hw_algo;
> @@ -35,11 +35,11 @@ struct gpio_wdt_priv {
>   
>   static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
>   {
> -	gpio_set_value_cansleep(priv->gpio, !priv->active_low);
> +	gpiod_set_value_cansleep(priv->gpiod, 0);

This ...

>   
>   	/* Put GPIO back to tristate */
>   	if (priv->hw_algo == HW_ALGO_TOGGLE)
> -		gpio_direction_input(priv->gpio);
> +		gpiod_direction_input(priv->gpiod);
>   }
>   
>   static int gpio_wdt_ping(struct watchdog_device *wdd)
> @@ -50,13 +50,13 @@ static int gpio_wdt_ping(struct watchdog_device *wdd)
>   	case HW_ALGO_TOGGLE:
>   		/* Toggle output pin */
>   		priv->state = !priv->state;
> -		gpio_set_value_cansleep(priv->gpio, priv->state);
> +		gpiod_set_value_cansleep(priv->gpiod, priv->state);
>   		break;
>   	case HW_ALGO_LEVEL:
>   		/* Pulse */
> -		gpio_set_value_cansleep(priv->gpio, !priv->active_low);
> +		gpiod_set_value_cansleep(priv->gpiod, 1);

... and this seem to contradict each other.

Guenter

>   		udelay(1);
> -		gpio_set_value_cansleep(priv->gpio, priv->active_low);
> +		gpiod_set_value_cansleep(priv->gpiod, 0);
>   		break;
>   	}
>   	return 0;
> @@ -66,8 +66,8 @@ static int gpio_wdt_start(struct watchdog_device *wdd)
>   {
>   	struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
>   
> -	priv->state = priv->active_low;
> -	gpio_direction_output(priv->gpio, priv->state);
> +	priv->state = 0;
> +	gpiod_direction_output(priv->gpiod, priv->state);
>   
>   	set_bit(WDOG_HW_RUNNING, &wdd->status);
>   
> @@ -104,9 +104,8 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct device_node *np = dev->of_node;
>   	struct gpio_wdt_priv *priv;
> -	enum of_gpio_flags flags;
> +	enum gpiod_flags gflags;
>   	unsigned int hw_margin;
> -	unsigned long f = 0;
>   	const char *algo;
>   	int ret;
>   
> @@ -116,29 +115,22 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, priv);
>   
> -	priv->gpio = of_get_gpio_flags(np, 0, &flags);
> -	if (!gpio_is_valid(priv->gpio))
> -		return priv->gpio;
> -
> -	priv->active_low = flags & OF_GPIO_ACTIVE_LOW;
> -
>   	ret = of_property_read_string(np, "hw_algo", &algo);
>   	if (ret)
>   		return ret;
>   	if (!strcmp(algo, "toggle")) {
>   		priv->hw_algo = HW_ALGO_TOGGLE;
> -		f = GPIOF_IN;
> +		gflags = GPIOD_IN;
>   	} else if (!strcmp(algo, "level")) {
>   		priv->hw_algo = HW_ALGO_LEVEL;
> -		f = priv->active_low ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
> +		gflags = GPIOD_OUT_LOW;
>   	} else {
>   		return -EINVAL;
>   	}
>   
> -	ret = devm_gpio_request_one(dev, priv->gpio, f,
> -				    dev_name(dev));
> -	if (ret)
> -		return ret;
> +	priv->gpiod = devm_gpiod_get(dev, NULL, gflags);
> +	if (IS_ERR(priv->gpiod))
> +		return PTR_ERR(priv->gpiod);
>   
>   	ret = of_property_read_u32(np,
>   				   "hw_margin_ms", &hw_margin);
> 

Re: [PATCH 2/2] watchdog: gpio: Convert to use GPIO descriptors

[Linus Walleij]

On Sun, Oct 8, 2017 at 6:22 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/26/2017 01:07 PM, Linus Walleij wrote:

>> @@ -35,11 +35,11 @@ struct gpio_wdt_priv {
>>     static void gpio_wdt_disable(struct gpio_wdt_priv *priv)
>>   {
>> -       gpio_set_value_cansleep(priv->gpio, !priv->active_low);
>> +       gpiod_set_value_cansleep(priv->gpiod, 0);
>
> This ...

Bah that's wrong. As anyone can see.

>>         case HW_ALGO_LEVEL:
>>                 /* Pulse */
>> -               gpio_set_value_cansleep(priv->gpio, !priv->active_low);
>> +               gpiod_set_value_cansleep(priv->gpiod, 1);
>
>
> ... and this seem to contradict each other.

Sorry, fixing.

Thanks a lot for spotting this!

Yours,
Linus Walleij