diff mbox series

[v2] watchdog: ebc-c384_wdt: Migrate to the regmap API

Message ID 20230314152916.185939-1-william.gray@linaro.org (mailing list archive)
State Rejected
Headers show
Series [v2] watchdog: ebc-c384_wdt: Migrate to the regmap API | expand

Commit Message

William Breathitt Gray March 14, 2023, 3:29 p.m. UTC
The regmap API supports IO port accessors so we can take advantage of
regmap abstractions rather than handling access to the device registers
directly in the driver.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
Changes in v2:
 - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()
 - Map watchdog control registers based on offset 0x1 and adjust regmap
   configurations accordingly; offset 0x0 is unused in this driver so we
   should avoid unnecessary exposure of it

 drivers/watchdog/Kconfig        |  1 +
 drivers/watchdog/ebc-c384_wdt.c | 67 +++++++++++++++++++++++----------
 2 files changed, 49 insertions(+), 19 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Andy Shevchenko March 14, 2023, 3:50 p.m. UTC | #1
On Tue, Mar 14, 2023 at 11:29:16AM -0400, William Breathitt Gray wrote:
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver.

...

>  - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()

I'm wondering why you can't use dev_get_regmap() instead.

>  - Map watchdog control registers based on offset 0x1 and adjust regmap
>    configurations accordingly; offset 0x0 is unused in this driver so we
>    should avoid unnecessary exposure of it

I'm wondering what bad could happen if you expose it.
Guenter Roeck March 14, 2023, 4:14 p.m. UTC | #2
On 3/14/23 08:50, Andy Shevchenko wrote:
> On Tue, Mar 14, 2023 at 11:29:16AM -0400, William Breathitt Gray wrote:
>> The regmap API supports IO port accessors so we can take advantage of
>> regmap abstractions rather than handling access to the device registers
>> directly in the driver.
> 
> ...
> 
>>   - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()
> 
> I'm wondering why you can't use dev_get_regmap() instead.
> 

That function is quite expensive to use in code that is called
for each register access. Its typical use is to get the regmap
for a driver once and store it in a local data structure, not
to use it for each access.

Guenter

>>   - Map watchdog control registers based on offset 0x1 and adjust regmap
>>     configurations accordingly; offset 0x0 is unused in this driver so we
>>     should avoid unnecessary exposure of it
> 
> I'm wondering what bad could happen if you expose it.
>
William Breathitt Gray March 14, 2023, 4:31 p.m. UTC | #3
On Tue, Mar 14, 2023 at 05:50:42PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 14, 2023 at 11:29:16AM -0400, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> 
> ...
> 
> >  - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()
> 
> I'm wondering why you can't use dev_get_regmap() instead.

We can set `wdd->parent = dev` in ebc_c384_wdt_probe(), and then use
`dev_get_regmap(wdev->parent)` to retrieve the regmap. The only downside
I see if perhaps the added latency a call to devres_find(), whereas
using watchdog_get_drvdata() is just a pointer dereference.

I'm indifferent to either choice, so if Guenter or Wim have a preference
here I'll follow their decision.

> 
> >  - Map watchdog control registers based on offset 0x1 and adjust regmap
> >    configurations accordingly; offset 0x0 is unused in this driver so we
> >    should avoid unnecessary exposure of it
> 
> I'm wondering what bad could happen if you expose it.

The WINSYSTEMS EBC-C384 documentation I have does not specify what
offset 0x0 does (nor offsets 0x3-0x4), so I don't know if there are side
effects to reading those addresses. Really, I'm just avoiding the hassle
of writing an explicit precious registers table for those offsets by not
exposing them at all.

William Breathitt Gray
Guenter Roeck March 14, 2023, 4:42 p.m. UTC | #4
On 3/14/23 09:31, William Breathitt Gray wrote:
> On Tue, Mar 14, 2023 at 05:50:42PM +0200, Andy Shevchenko wrote:
>> On Tue, Mar 14, 2023 at 11:29:16AM -0400, William Breathitt Gray wrote:
>>> The regmap API supports IO port accessors so we can take advantage of
>>> regmap abstractions rather than handling access to the device registers
>>> directly in the driver.
>>
>> ...
>>
>>>   - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()
>>
>> I'm wondering why you can't use dev_get_regmap() instead.
> 
> We can set `wdd->parent = dev` in ebc_c384_wdt_probe(), and then use
> `dev_get_regmap(wdev->parent)` to retrieve the regmap. The only downside
> I see if perhaps the added latency a call to devres_find(), whereas
> using watchdog_get_drvdata() is just a pointer dereference.
> 
> I'm indifferent to either choice, so if Guenter or Wim have a preference
> here I'll follow their decision.
> 

I am not inclined to accept a patch which calls dev_get_regmap() more
than once. It is not just added latency, it is unnecessarily executing
a lot of code. Maybe that call is abused nowadays, and/or maybe people do not
care about wasting CPU cycles anymore, but that is not its intended use case.

>>
>>>   - Map watchdog control registers based on offset 0x1 and adjust regmap
>>>     configurations accordingly; offset 0x0 is unused in this driver so we
>>>     should avoid unnecessary exposure of it
>>
>> I'm wondering what bad could happen if you expose it.
> 
> The WINSYSTEMS EBC-C384 documentation I have does not specify what
> offset 0x0 does (nor offsets 0x3-0x4), so I don't know if there are side
> effects to reading those addresses. Really, I'm just avoiding the hassle
> of writing an explicit precious registers table for those offsets by not
> exposing them at all.
> 

Counter questions:
- What would be the purpose of exposing register addresses if they are not needed ?
- What bad can happen by _not_ exposing those register addresses ?

Guenter
Guenter Roeck March 14, 2023, 5:47 p.m. UTC | #5
On 3/14/23 08:29, William Breathitt Gray wrote:
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: William Breathitt Gray <william.gray@linaro.org>

The changes are too invasive to accept without testing.
I hope we can get a Tested-by: tag from someone. Other than
that, we are good to go from my perspective.

Thanks,
Guenter

> ---
> Changes in v2:
>   - Utilize watchdog_set_drvdata() and watchdog_get_drvdata()
>   - Map watchdog control registers based on offset 0x1 and adjust regmap
>     configurations accordingly; offset 0x0 is unused in this driver so we
>     should avoid unnecessary exposure of it
> 
>   drivers/watchdog/Kconfig        |  1 +
>   drivers/watchdog/ebc-c384_wdt.c | 67 +++++++++++++++++++++++----------
>   2 files changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f0872970daf9..301cfe79263c 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1089,6 +1089,7 @@ config EBC_C384_WDT
>   	tristate "WinSystems EBC-C384 Watchdog Timer"
>   	depends on X86
>   	select ISA_BUS_API
> +	select REGMAP_MMIO
>   	select WATCHDOG_CORE
>   	help
>   	  Enables watchdog timer support for the watchdog timer on the
> diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
> index 8ef4b0df3855..2f9fec5073b3 100644
> --- a/drivers/watchdog/ebc-c384_wdt.c
> +++ b/drivers/watchdog/ebc-c384_wdt.c
> @@ -3,15 +3,15 @@
>    * Watchdog timer driver for the WinSystems EBC-C384
>    * Copyright (C) 2016 William Breathitt Gray
>    */
> +#include <linux/bits.h>
>   #include <linux/device.h>
>   #include <linux/dmi.h>
> -#include <linux/errno.h>
> -#include <linux/io.h>
> -#include <linux/ioport.h>
> +#include <linux/err.h>
>   #include <linux/isa.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
> +#include <linux/regmap.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
>   
> @@ -24,8 +24,14 @@
>   #define WATCHDOG_MAX_TIMEOUT	15300
>   #define BASE_ADDR		0x564
>   #define ADDR_EXTENT		5
> -#define CFG_ADDR		(BASE_ADDR + 1)
> -#define PET_ADDR		(BASE_ADDR + 2)
> +#define CTRL_BASE_ADDR		(BASE_ADDR + 0x1)
> +#define CTRL_ADDR_EXTENT	2
> +#define CTRL_MAX_REGISTER	(CTRL_ADDR_EXTENT - 1)
> +#define CFG_REG			0x0
> +#define PET_REG			0x1
> +#define CFG_MINUTES		0x00
> +#define CFG_SECONDS		BIT(7)
> +#define PET_DISABLED		0x00
>   
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   module_param(nowayout, bool, 0);
> @@ -37,43 +43,54 @@ module_param(timeout, uint, 0);
>   MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
>   	__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
>   
> +static const struct regmap_range ebc_c384_wdt_wr_ranges[] = {
> +	regmap_reg_range(0x0, 0x1),
> +};
> +static const struct regmap_access_table ebc_c384_wdt_wr_table = {
> +	.yes_ranges = ebc_c384_wdt_wr_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(ebc_c384_wdt_wr_ranges),
> +};
> +static const struct regmap_config ebc_c384_wdt_regmap_config = {
> +	.reg_bits = 8,
> +	.reg_stride = 1,
> +	.val_bits = 8,
> +	.io_port = true,
> +	.max_register = CTRL_MAX_REGISTER,
> +	.wr_table = &ebc_c384_wdt_wr_table,
> +};
> +
>   static int ebc_c384_wdt_start(struct watchdog_device *wdev)
>   {
> +	struct regmap *const map = watchdog_get_drvdata(wdev);
>   	unsigned t = wdev->timeout;
>   
>   	/* resolution is in minutes for timeouts greater than 255 seconds */
>   	if (t > 255)
>   		t = DIV_ROUND_UP(t, 60);
>   
> -	outb(t, PET_ADDR);
> -
> -	return 0;
> +	return regmap_write(map, PET_REG, t);
>   }
>   
>   static int ebc_c384_wdt_stop(struct watchdog_device *wdev)
>   {
> -	outb(0x00, PET_ADDR);
> +	struct regmap *const map = watchdog_get_drvdata(wdev);
>   
> -	return 0;
> +	return regmap_write(map, PET_REG, PET_DISABLED);
>   }
>   
>   static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>   {
> +	struct regmap *const map = watchdog_get_drvdata(wdev);
> +
>   	/* resolution is in minutes for timeouts greater than 255 seconds */
>   	if (t > 255) {
>   		/* round second resolution up to minute granularity */
>   		wdev->timeout = roundup(t, 60);
> -
> -		/* set watchdog timer for minutes */
> -		outb(0x00, CFG_ADDR);
> -	} else {
> -		wdev->timeout = t;
> -
> -		/* set watchdog timer for seconds */
> -		outb(0x80, CFG_ADDR);
> +		return regmap_write(map, CFG_REG, CFG_MINUTES);
>   	}
>   
> -	return 0;
> +	wdev->timeout = t;
> +	return regmap_write(map, CFG_REG, CFG_SECONDS);
>   }
>   
>   static const struct watchdog_ops ebc_c384_wdt_ops = {
> @@ -89,6 +106,8 @@ static const struct watchdog_info ebc_c384_wdt_info = {
>   
>   static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>   {
> +	void __iomem *regs;
> +	struct regmap *map;
>   	struct watchdog_device *wdd;
>   
>   	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
> @@ -97,6 +116,15 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>   		return -EBUSY;
>   	}
>   
> +	regs = devm_ioport_map(dev, CTRL_BASE_ADDR, CTRL_ADDR_EXTENT);
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	map = devm_regmap_init_mmio(dev, regs, &ebc_c384_wdt_regmap_config);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "Unable to initialize register map\n");
> +
>   	wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
>   	if (!wdd)
>   		return -ENOMEM;
> @@ -107,6 +135,7 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>   	wdd->min_timeout = 1;
>   	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
>   
> +	watchdog_set_drvdata(wdd, map);
>   	watchdog_set_nowayout(wdd, nowayout);
>   	watchdog_init_timeout(wdd, timeout, dev);
>   
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
William Breathitt Gray April 2, 2023, 2:07 p.m. UTC | #6
On Tue, Mar 14, 2023 at 10:47:28AM -0700, Guenter Roeck wrote:
> On 3/14/23 08:29, William Breathitt Gray wrote:
> > The regmap API supports IO port accessors so we can take advantage of
> > regmap abstractions rather than handling access to the device registers
> > directly in the driver.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
> 
> The changes are too invasive to accept without testing.
> I hope we can get a Tested-by: tag from someone. Other than
> that, we are good to go from my perspective.
> 
> Thanks,
> Guenter

Paul,

Is anyone at WINSYSTEMS interested in this driver? I no longer have
access to this hardware so I can no longer maintain this driver, and the
original users of this driver indicated to me that they have moved on to
another device. If there is no longer interest at WINSYSTEMS to maintain
this driver, perhaps we should discuss removal of it from the Linux
codebase.

William Breathitt Gray
diff mbox series

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f0872970daf9..301cfe79263c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1089,6 +1089,7 @@  config EBC_C384_WDT
 	tristate "WinSystems EBC-C384 Watchdog Timer"
 	depends on X86
 	select ISA_BUS_API
+	select REGMAP_MMIO
 	select WATCHDOG_CORE
 	help
 	  Enables watchdog timer support for the watchdog timer on the
diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
index 8ef4b0df3855..2f9fec5073b3 100644
--- a/drivers/watchdog/ebc-c384_wdt.c
+++ b/drivers/watchdog/ebc-c384_wdt.c
@@ -3,15 +3,15 @@ 
  * Watchdog timer driver for the WinSystems EBC-C384
  * Copyright (C) 2016 William Breathitt Gray
  */
+#include <linux/bits.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
-#include <linux/errno.h>
-#include <linux/io.h>
-#include <linux/ioport.h>
+#include <linux/err.h>
 #include <linux/isa.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/regmap.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
@@ -24,8 +24,14 @@ 
 #define WATCHDOG_MAX_TIMEOUT	15300
 #define BASE_ADDR		0x564
 #define ADDR_EXTENT		5
-#define CFG_ADDR		(BASE_ADDR + 1)
-#define PET_ADDR		(BASE_ADDR + 2)
+#define CTRL_BASE_ADDR		(BASE_ADDR + 0x1)
+#define CTRL_ADDR_EXTENT	2
+#define CTRL_MAX_REGISTER	(CTRL_ADDR_EXTENT - 1)
+#define CFG_REG			0x0
+#define PET_REG			0x1
+#define CFG_MINUTES		0x00
+#define CFG_SECONDS		BIT(7)
+#define PET_DISABLED		0x00
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
@@ -37,43 +43,54 @@  module_param(timeout, uint, 0);
 MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
 	__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
 
+static const struct regmap_range ebc_c384_wdt_wr_ranges[] = {
+	regmap_reg_range(0x0, 0x1),
+};
+static const struct regmap_access_table ebc_c384_wdt_wr_table = {
+	.yes_ranges = ebc_c384_wdt_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ebc_c384_wdt_wr_ranges),
+};
+static const struct regmap_config ebc_c384_wdt_regmap_config = {
+	.reg_bits = 8,
+	.reg_stride = 1,
+	.val_bits = 8,
+	.io_port = true,
+	.max_register = CTRL_MAX_REGISTER,
+	.wr_table = &ebc_c384_wdt_wr_table,
+};
+
 static int ebc_c384_wdt_start(struct watchdog_device *wdev)
 {
+	struct regmap *const map = watchdog_get_drvdata(wdev);
 	unsigned t = wdev->timeout;
 
 	/* resolution is in minutes for timeouts greater than 255 seconds */
 	if (t > 255)
 		t = DIV_ROUND_UP(t, 60);
 
-	outb(t, PET_ADDR);
-
-	return 0;
+	return regmap_write(map, PET_REG, t);
 }
 
 static int ebc_c384_wdt_stop(struct watchdog_device *wdev)
 {
-	outb(0x00, PET_ADDR);
+	struct regmap *const map = watchdog_get_drvdata(wdev);
 
-	return 0;
+	return regmap_write(map, PET_REG, PET_DISABLED);
 }
 
 static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
 {
+	struct regmap *const map = watchdog_get_drvdata(wdev);
+
 	/* resolution is in minutes for timeouts greater than 255 seconds */
 	if (t > 255) {
 		/* round second resolution up to minute granularity */
 		wdev->timeout = roundup(t, 60);
-
-		/* set watchdog timer for minutes */
-		outb(0x00, CFG_ADDR);
-	} else {
-		wdev->timeout = t;
-
-		/* set watchdog timer for seconds */
-		outb(0x80, CFG_ADDR);
+		return regmap_write(map, CFG_REG, CFG_MINUTES);
 	}
 
-	return 0;
+	wdev->timeout = t;
+	return regmap_write(map, CFG_REG, CFG_SECONDS);
 }
 
 static const struct watchdog_ops ebc_c384_wdt_ops = {
@@ -89,6 +106,8 @@  static const struct watchdog_info ebc_c384_wdt_info = {
 
 static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
 {
+	void __iomem *regs;
+	struct regmap *map;
 	struct watchdog_device *wdd;
 
 	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
@@ -97,6 +116,15 @@  static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
+	regs = devm_ioport_map(dev, CTRL_BASE_ADDR, CTRL_ADDR_EXTENT);
+	if (!regs)
+		return -ENOMEM;
+
+	map = devm_regmap_init_mmio(dev, regs, &ebc_c384_wdt_regmap_config);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map),
+				     "Unable to initialize register map\n");
+
 	wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
 	if (!wdd)
 		return -ENOMEM;
@@ -107,6 +135,7 @@  static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
 	wdd->min_timeout = 1;
 	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
 
+	watchdog_set_drvdata(wdd, map);
 	watchdog_set_nowayout(wdd, nowayout);
 	watchdog_init_timeout(wdd, timeout, dev);