Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RESEND] watchdog: wm831x: Use GPIO descriptor
@ 2020-02-10 10:22 Linus Walleij
  2020-02-10 14:25 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Linus Walleij @ 2020-02-10 10:22 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: linux-watchdog, Linus Walleij, Richard Fitzgerald,
	Charles Keepax, Mark Brown

The WM831x watchdog driver passes a global GPIO number from
platform data into this driver, this is discouraged so pass
a GPIO descriptor instead.

More thorough approaches are possible passing descriptors
associated with the device through machine descriptor tables,
but no boardfiles in the kernel currently use this driver
so it is hard to test.

Cc: Richard Fitzgerald <rf@opensource.cirrus.com>
Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/watchdog/wm831x_wdt.c    | 22 ++++++----------------
 include/linux/mfd/wm831x/pdata.h |  3 ++-
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/wm831x_wdt.c b/drivers/watchdog/wm831x_wdt.c
index 030ce240620d..75c14287757f 100644
--- a/drivers/watchdog/wm831x_wdt.c
+++ b/drivers/watchdog/wm831x_wdt.c
@@ -13,7 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 #include <linux/uaccess.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <linux/mfd/wm831x/core.h>
 #include <linux/mfd/wm831x/pdata.h>
@@ -29,7 +29,7 @@ struct wm831x_wdt_drvdata {
 	struct watchdog_device wdt;
 	struct wm831x *wm831x;
 	struct mutex lock;
-	int update_gpio;
+	struct gpio_desc *update_gpiod;
 	int update_state;
 };
 
@@ -103,8 +103,8 @@ static int wm831x_wdt_ping(struct watchdog_device *wdt_dev)
 
 	mutex_lock(&driver_data->lock);
 
-	if (driver_data->update_gpio) {
-		gpio_set_value_cansleep(driver_data->update_gpio,
+	if (driver_data->update_gpiod) {
+		gpiod_set_value_cansleep(driver_data->update_gpiod,
 					driver_data->update_state);
 		driver_data->update_state = !driver_data->update_state;
 		ret = 0;
@@ -239,18 +239,8 @@ static int wm831x_wdt_probe(struct platform_device *pdev)
 		reg |= pdata->secondary << WM831X_WDOG_SECACT_SHIFT;
 		reg |= pdata->software << WM831X_WDOG_RST_SRC_SHIFT;
 
-		if (pdata->update_gpio) {
-			ret = devm_gpio_request_one(dev, pdata->update_gpio,
-						    GPIOF_OUT_INIT_LOW,
-						    "Watchdog update");
-			if (ret < 0) {
-				dev_err(wm831x->dev,
-					"Failed to request update GPIO: %d\n",
-					ret);
-				return ret;
-			}
-
-			driver_data->update_gpio = pdata->update_gpio;
+		if (pdata->update_gpiod) {
+			driver_data->update_gpiod = pdata->update_gpiod;
 
 			/* Make sure the watchdog takes hardware updates */
 			reg |= WM831X_WDOG_RST_SRC;
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index 986986fe4e4e..c6ce1b94d053 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -10,6 +10,7 @@
 #ifndef __MFD_WM831X_PDATA_H__
 #define __MFD_WM831X_PDATA_H__
 
+struct gpio_desc;
 struct wm831x;
 struct regulator_init_data;
 
@@ -89,7 +90,7 @@ enum wm831x_watchdog_action {
 
 struct wm831x_watchdog_pdata {
 	enum wm831x_watchdog_action primary, secondary;
-	int update_gpio;
+	struct gpio_desc *update_gpiod;
 	unsigned int software:1;
 };
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH RESEND] watchdog: wm831x: Use GPIO descriptor
  2020-02-10 10:22 [PATCH RESEND] watchdog: wm831x: Use GPIO descriptor Linus Walleij
@ 2020-02-10 14:25 ` Guenter Roeck
  2020-02-14 14:26   ` Charles Keepax
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2020-02-10 14:25 UTC (permalink / raw)
  To: Linus Walleij, Wim Van Sebroeck
  Cc: linux-watchdog, Richard Fitzgerald, Charles Keepax, Mark Brown

On 2/10/20 2:22 AM, Linus Walleij wrote:
> The WM831x watchdog driver passes a global GPIO number from
> platform data into this driver, this is discouraged so pass
> a GPIO descriptor instead.
> 
> More thorough approaches are possible passing descriptors
> associated with the device through machine descriptor tables,
> but no boardfiles in the kernel currently use this driver
> so it is hard to test.
> 
> Cc: Richard Fitzgerald <rf@opensource.cirrus.com>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Interesting, I don't see evidence of the original patch in
watchdog patchwork.

Anyway, it seems to me it would be better to remove the gpio code
entirely from this driver. It is instantiated from an mfd driver
which doesn't set the gpio pin. It is quite unlikely that it is
ever going to be used, so we might as well remove it (instead of
modifying it without ability to test it).

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH RESEND] watchdog: wm831x: Use GPIO descriptor
  2020-02-10 14:25 ` Guenter Roeck
@ 2020-02-14 14:26   ` Charles Keepax
  0 siblings, 0 replies; 3+ messages in thread
From: Charles Keepax @ 2020-02-14 14:26 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Linus Walleij, Wim Van Sebroeck, linux-watchdog,
	Richard Fitzgerald, Mark Brown

On Mon, Feb 10, 2020 at 06:25:35AM -0800, Guenter Roeck wrote:
> On 2/10/20 2:22 AM, Linus Walleij wrote:
> >The WM831x watchdog driver passes a global GPIO number from
> >platform data into this driver, this is discouraged so pass
> >a GPIO descriptor instead.
> >
> >More thorough approaches are possible passing descriptors
> >associated with the device through machine descriptor tables,
> >but no boardfiles in the kernel currently use this driver
> >so it is hard to test.
> >
> >Cc: Richard Fitzgerald <rf@opensource.cirrus.com>
> >Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> >Cc: Mark Brown <broonie@kernel.org>
> >Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Interesting, I don't see evidence of the original patch in
> watchdog patchwork.
> 
> Anyway, it seems to me it would be better to remove the gpio code
> entirely from this driver. It is instantiated from an mfd driver
> which doesn't set the gpio pin. It is quite unlikely that it is
> ever going to be used, so we might as well remove it (instead of
> modifying it without ability to test it).
> 

We don't have any internal users for the WDT GPIOs here, given
the age of the part perhaps removing the GPIO is the simplest
solution.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 10:22 [PATCH RESEND] watchdog: wm831x: Use GPIO descriptor Linus Walleij
2020-02-10 14:25 ` Guenter Roeck
2020-02-14 14:26   ` Charles Keepax

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git