From: Guenter Roeck <linux@roeck-us.net>
To: William Breathitt Gray <william.gray@linaro.org>
Cc: wim@linux-watchdog.org, linux-watchdog@vger.kernel.org,
linux-kernel@vger.kernel.org,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH] watchdog: ebc-c384_wdt: Migrate to the regmap API
Date: Sat, 11 Mar 2023 06:04:08 -0800 [thread overview]
Message-ID: <b65233f4-c8e2-435d-8321-03cc5f6b1c3d@roeck-us.net> (raw)
In-Reply-To: <20230311004404.62980-1-william.gray@linaro.org>
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
>
next prev parent reply other threads:[~2023-03-11 14:04 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-11 0:44 [PATCH] watchdog: ebc-c384_wdt: Migrate to the regmap API William Breathitt Gray
2023-03-11 14:04 ` Guenter Roeck [this message]
2023-03-11 15:06 ` William Breathitt Gray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b65233f4-c8e2-435d-8321-03cc5f6b1c3d@roeck-us.net \
--to=linux@roeck-us.net \
--cc=andriy.shevchenko@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=william.gray@linaro.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).