watchdog: ebc-c384_wdt: Migrate to the regmap API

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>
---
 drivers/watchdog/Kconfig        |  1 +
 drivers/watchdog/ebc-c384_wdt.c | 64 +++++++++++++++++++++++----------
 2 files changed, 46 insertions(+), 19 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

On Fri, Mar 10, 2023 at 07:44:04PM -0500, 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>

I assume you did, but just to be sure: Did you test this on hardware ?

> ---
>  drivers/watchdog/Kconfig        |  1 +
>  drivers/watchdog/ebc-c384_wdt.c | 64 +++++++++++++++++++++++----------
>  2 files changed, 46 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..3776d32cb863 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,11 @@
>  #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 CFG_REG			0x1
> +#define PET_REG			0x2
> +#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 +40,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(0x1, 0x2),

Any reasons for not using defines ?

> +};
> +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 = 0x2,

Any reason for not using a define ?

> +	.wr_table = &ebc_c384_wdt_wr_table,
> +};
> +
>  static int ebc_c384_wdt_start(struct watchdog_device *wdev)
>  {
> +	struct regmap *const map = wdev->driver_data;

Please use watchdog_get_drvdata() and watchdog_set_drvdata() when accessing
or setting watchdog driver data.

Guenter

>  	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 = wdev->driver_data;
>  
> -	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 = wdev->driver_data;
> +
>  	/* 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 +103,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 +113,15 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>  		return -EBUSY;
>  	}
>  
> +	regs = devm_ioport_map(dev, BASE_ADDR, 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;
> @@ -106,6 +131,7 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
>  	wdd->timeout = WATCHDOG_TIMEOUT;
>  	wdd->min_timeout = 1;
>  	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
> +	wdd->driver_data = map;
>  
>  	watchdog_set_nowayout(wdd, nowayout);
>  	watchdog_init_timeout(wdd, timeout, dev);
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> -- 
> 2.39.2
>

On Sat, Mar 11, 2023 at 06:04:08AM -0800, Guenter Roeck wrote:
> On Fri, Mar 10, 2023 at 07:44:04PM -0500, 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>
> 
> I assume you did, but just to be sure: Did you test this on hardware ?

I've only done a build test; I no longer have access to a WINSYSTEMS
EBC-C384 motherboard to test this on real hardware. I've CC'd Paul
Demetrotion and the WINSYSTEMS tech support list to this thread as
hopefully one of the WINSYSTEMS engineers may help us test this.

> > @@ -37,43 +40,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(0x1, 0x2),
> 
> Any reasons for not using defines ?

I feel direct addresses are somewhat clearer in this context. Regmap
configurations describe the address range of valid read and write
operations. Although the range for this device is simple, other devices
might have multiple ranges with gaps of invalid addresses.

For example, our valid write address range is 0x1-0x2 here:

    regmap_reg_range(0x1, 0x2)

Which is much clearer than trying to match these to register defines:

    regmap_reg_range(CFG_REG, PET_REG)

Because it's not immediately clear that CFG_REG to PET_REG is a
contiguous address range.

> > +};
> > +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 = 0x2,
> 
> Any reason for not using a define ?

Same reason as above: `max_register = 0x2` is already clear enough and
`max_register = EBC_C384_MAX_REGISTER` wouldn't add any substantial
clarity.

> > +	.wr_table = &ebc_c384_wdt_wr_table,
> > +};
> > +
> >  static int ebc_c384_wdt_start(struct watchdog_device *wdev)
> >  {
> > +	struct regmap *const map = wdev->driver_data;
> 
> Please use watchdog_get_drvdata() and watchdog_set_drvdata() when accessing
> or setting watchdog driver data.
> 
> Guenter

I'll adjust the driver_data interactions in my v2 submission to utilize
watchdog_get_drvdata() and watchdog_set_drvdata().

William Breathitt Gray