Added 'advantech_ec_wdt' watchdog driver

This patch adds the 'advantech_ec_wdt' kernel module which provides
WDT support for Advantech platforms with ITE based Embedded Controller.

Signed-off-by: Thomas Kastner <thomas.kastner@advantech.com>
---
 drivers/watchdog/Kconfig            |   7 +
 drivers/watchdog/Makefile           |   1 +
 drivers/watchdog/advantech_ec_wdt.c | 247 ++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+)
 create mode 100644 drivers/watchdog/advantech_ec_wdt.c

On Wed, Oct 12, 2022 at 12:01:19PM +0200, Thomas Kastner wrote:
> This patch adds the 'advantech_ec_wdt' kernel module which provides
> WDT support for Advantech platforms with ITE based Embedded Controller.
> 
> Signed-off-by: Thomas Kastner <thomas.kastner@advantech.com>

Subject: s/Added/Add/

> ---
>  drivers/watchdog/Kconfig            |   7 +
>  drivers/watchdog/Makefile           |   1 +
>  drivers/watchdog/advantech_ec_wdt.c | 247 ++++++++++++++++++++++++++++
>  3 files changed, 255 insertions(+)
>  create mode 100644 drivers/watchdog/advantech_ec_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 688922fc4edb..afe14f530291 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1055,6 +1055,13 @@ config ADVANTECH_WDT
>  	  feature. More information can be found at
>  	  <https://www.advantech.com.tw/products/>
>  
> +config ADVANTECH_EC_WDT
> +	tristate "Advantech Embedded Controller Watchdog Timer"
> +	depends on X86
> +	help
> +	   	This driver supports Advantech products with ITE based Embedded Controller.
> + 		It does not support Advantech products with other ECs or without EC.
> +
>  config ALIM1535_WDT
>  	tristate "ALi M1535 PMU Watchdog Timer"
>  	depends on X86 && PCI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index cdeb119e6e61..2768dc2348af 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -102,6 +102,7 @@ obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
>  # X86 (i386 + ia64 + x86_64) Architecture
>  obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
>  obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
> +obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
>  obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
>  obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
>  obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
> diff --git a/drivers/watchdog/advantech_ec_wdt.c b/drivers/watchdog/advantech_ec_wdt.c
> new file mode 100644
> index 000000000000..f3babaa918f7
> --- /dev/null
> +++ b/drivers/watchdog/advantech_ec_wdt.c
> @@ -0,0 +1,247 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *	Advantech Embedded Controller Watchdog Driver
> + *
> + *	This driver supports Advantech products with ITE based Embedded Controller.
> + *	It does not support Advantech products with other ECs or without EC.
> + *
> + *	This software is provided "as is" without warranty of any kind.
> + *
> + *	Copyright (C) 2022 Advantech Europe B.V.
> + *	<thomas.kastner@advantech.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/isa.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +#include <linux/seq_file.h>
> +#include <linux/uaccess.h>
> +#include <linux/spinlock.h>

Alphabetic include file order, please.

> +
> +#define DRIVER_NAME		"advantech_ec_wdt"
> +
> +/* EC IO region */
> +#define BASE_ADDR		0x299
> +#define ADDR_EXTENT		2
> +
> +/* EC interface definitions */
> +#define EC_ADDR_CMD		0x29A
> +#define EC_ADDR_DATA		0x299
> +#define EC_CMD_EC_PROBE		0x30
> +#define EC_CMD_COMM		0x89
> +#define EC_CMD_WDT_START	0x28
> +#define EC_CMD_WDT_STOP		0x29
> +#define EC_CMD_WDT_RESET	0x2A
> +#define EC_DAT_EN_DLY_H		0x58
> +#define EC_DAT_EN_DLY_L		0x59
> +#define EC_DAT_RST_DLY_H	0x5E
> +#define EC_DAT_RST_DLY_L	0x5F
> +#define EC_MAGIC		0x95
> +
> +/* module parameters */
> +#define MIN_TIME		1
> +#define MAX_TIME		6000 /* 100 minutes */
> +#define DEFAULT_TIME		60
> +
> +static unsigned int timeout = DEFAULT_TIME;

This should be set to 0. The default timeout should be set
in struct watchdog_device.

> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +	"Default Watchdog timer setting ("
> +	__MODULE_STRING(DEFAULT_TIME) "s)."
> +	"The range is from " __MODULE_STRING(MIN_TIME)
> +	" to " __MODULE_STRING(MAX_TIME) ".");
> +
> +static int adv_ec_wdt_ping(struct watchdog_device *wdd)
> +{
> +	pr_debug("%s: Strobing watchdog\n", __func__);
> +
> +	/* reset watchdog */
> +	outb(EC_CMD_WDT_RESET, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);

Is this really needed ? If so, write an accessor function.
It would be preferred to have that accessor function wait
before accesses (cache the time of the last access and wait
if not enough time has expired). This would prevent unnecessary
wait operations in places like this one, where only a single
access happens.

> +	return 0;
> +}
> +
> +static int adv_ec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
> +{
> +	unsigned int val;
> +
> +	/* scale time to EC 100 ms base */
> +	val = t*10;

Space before and after '*'. Please run checkpatch; that should tell you.

> +
> +	pr_debug("%s: Setting timeout value of %i\n", __func__, val);
> +	/* reset enable delay, just in case it was set by BIOS etc. */
> +	outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +	outb(EC_DAT_EN_DLY_H, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +	outb(0, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +
> +	outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +	outb(EC_DAT_EN_DLY_L, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +	outb(0, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +
> +	/* set reset delay */
> +	outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +	outb(EC_DAT_RST_DLY_H, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +	outb((val >> 8), EC_ADDR_DATA);

Unnecessary ( )

> +	usleep_range(10000, 11000);
> +
> +	outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +	outb(EC_DAT_RST_DLY_L, EC_ADDR_DATA);
> +	usleep_range(10000, 11000);
> +	outb((val & 0xFF), EC_ADDR_DATA);

Unnecessary ( )

> +	usleep_range(10000, 11000);
> +
> +	wdd->timeout = t;
> +	return 0;
> +}
> +
> +
> +static int adv_ec_probe(void)
> +{
> +	unsigned int val;
> +
> +	/* Check for magic byte */
> +	outb(EC_CMD_EC_PROBE, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +	val = inb(EC_ADDR_DATA);
> +
> +	pr_debug("%s: probe result: %02X\n", __func__, val);
> +
> +	if (val == EC_MAGIC)
> +		return 0;
> +	else
> +		return -ENODEV;

This should be
	if (val != EC_MAGIC)
		return -ENODEV;

	return 0;

Error handling comes first, and else after return is unnecessary
(and static analyzers will complain about it).

> +}
> +
> +
> +static int adv_ec_wdt_start(struct watchdog_device *wdd)
> +{
> +	adv_ec_wdt_set_timeout(wdd, wdd->timeout);
> +
> +	pr_debug("%s: Enabling watchdog timer\n", __func__);
> +
> +	/* Enable the watchdog timer */
> +	outb(EC_CMD_WDT_START, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +
> +	return 0;
> +}
> +
> +static int adv_ec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	pr_debug("%s: Disabling watchdog timer\n", __func__);
> +
> +	/* Disable the watchdog timer */

Those comments really don't add any value. What else would the
start and stop functions do ?

> +	outb(EC_CMD_WDT_STOP, EC_ADDR_CMD);
> +	usleep_range(10000, 11000);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info adv_ec_wdt_info = {
> +	.identity =	DRIVER_NAME,
> +	.options =	WDIOF_SETTIMEOUT |
> +			WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +};
> +
> +static const struct watchdog_ops adv_ec_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	adv_ec_wdt_start,
> +	.stop =		adv_ec_wdt_stop,
> +	.ping =		adv_ec_wdt_ping,
> +	.set_timeout =	adv_ec_wdt_set_timeout,
> +};
> +
> +static struct watchdog_device adv_ec_wdt_dev = {
> +	.info =		&adv_ec_wdt_info,
> +	.ops =		&adv_ec_wdt_ops,
> +	.min_timeout =	MIN_TIME,
> +	.max_timeout =	MAX_TIME,

.timeout should be set to the default timeout.

> +};
> +
> +static int adv_ec_wdt_probe(struct device *dev, unsigned int id)
> +{
> +	int ret;
> +
> +	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
> +		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
> +			BASE_ADDR, BASE_ADDR + ADDR_EXTENT);
> +		return -EBUSY;
> +	}
> +
> +	/* probe for EC */
> +	ret = adv_ec_probe();
> +	if (ret) {
> +		dev_err(dev, "Error: cannot detect Advantech EC\n");

No error message before -ENODEV (that would pollute the kernel log
for everyone else).

> +		return -ENODEV;
> +	}

The chip should be detected in the init function, and the driver should
only probe is a chip was detected.

> +
> +	adv_ec_wdt_dev.timeout = timeout;

As mentioned, initialize .timeout with the default, and call
watchdog_init_timeout().

> +
> +	ret = devm_watchdog_register_device(dev, &adv_ec_wdt_dev);
> +
> +	return ret;
> +}
> +
> +static void adv_ec_wdt_remove(struct device *dev, unsigned int id)
> +{
> +	/* stop the watchdog */
> +	adv_ec_wdt_stop(NULL);

Call watchdog_stop_on_unregister() before registering the watchdog
instead. Also, again, the comment doesn't add value.

> +
> +	/* watchdog_unregister() and release_region() are called automatically */

Also unnecessary (implied with devm_). And you don't actually
need a remove function when using watchdog_stop_on_unregister().

> +
> +	return;
> +}
> +
> +static struct isa_driver adv_ec_wdt_driver = {
> +	.probe		= adv_ec_wdt_probe,
> +	.remove		= adv_ec_wdt_remove,
> +	.driver		= {
> +	.name		= DRIVER_NAME,
> +	},
> +};
> +
> +
> +static int __init adv_ec_wdt_init(void)
> +{
> +	/* Check boot parameters to verify that their initial values */
> +	/* are in range. */
> +	if ((timeout < MIN_TIME) ||
> +	    (timeout > MAX_TIME)) {

Unnecessary ( ) around expressions.

> +		pr_err("Watchdog timer: value of timeout %d (dec) "
> +		  "is out of range from %d to %d (dec)\n",
> +		  timeout, MIN_TIME, MAX_TIME);
> +		return -EINVAL;

This does not belong here. The probe function should call
watchdog_init_timeout() to set and validate the timeout module
parameter, and keep the default (instead of failing) if it is
out of range.

The init function should check if the hardware exists, and only
instantiate the device if it does.

> +	}
> +
> +	return isa_register_driver(&adv_ec_wdt_driver, 1);
> +}
> +
> +static void __exit adv_ec_wdt_exit(void)
> +{
> +	isa_unregister_driver(&adv_ec_wdt_driver);
> +}
> +
> +module_init(adv_ec_wdt_init);
> +module_exit(adv_ec_wdt_exit);
> +
> +MODULE_AUTHOR("Thomas Kastner <thomas.kastne@advantech.com>");
> +MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("20221012");
> +MODULE_ALIAS("isa:" DRIVER_NAME);
> -- 
> 2.34.1
>

On Wed, Oct 12, 2022 at 04:52:36PM +0000, Thomas.Kastner wrote:
> Guenter, thank you for your detailed review. I will address the issues and resubmit. Comments below.
> 
> ...
> > > +#include <linux/uaccess.h>
> > > +#include <linux/spinlock.h>
> > 
> > Alphabetic include file order, please.
> 
> Copied from existing driver, will fix regardless.
> 

That is never an good or even acceptable argument. Old drivers will always
have lots of oddities and don't typically use more recently introduced
helper functions. Case in point: both watchdog_init_timeout() and
watchdog_stop_on_unregister() were introduced after your example driver
was added to the kernel.

Also, please check if all the include files are really needed. For example,
I don't think <linux/uaccess.h> is needed.

> > > +#define DEFAULT_TIME         60
> > > +
> > > +static unsigned int timeout = DEFAULT_TIME;
> > 
> > This should be set to 0. The default timeout should be set
> > in struct watchdog_device.
>  
> Copied from existing driver, will fix regardless.
> 
> > > +     /* reset watchdog */
> > > +     outb(EC_CMD_WDT_RESET, EC_ADDR_CMD);
> > > +     usleep_range(10000, 11000);
> > 
> > Is this really needed ? If so, write an accessor function.
> > It would be preferred to have that accessor function wait
> > before accesses (cache the time of the last access and wait
> > if not enough time has expired). This would prevent unnecessary
> > wait operations in places like this one, where only a single
> > access happens.
> 
> Good point. Unfortunately the delay between individual accesses is required. 
> My reasoning here was that the watchdog reset function typically is called in many-second intervals only, so the potentially unnecessary wait therefore would be negligible and not justify the overhead of an accessor function.
> 

The problem is not the overhead in the code, but code pollution.
The argument should be that the code is not called often, thus
using an accessor function is cleaner and its runtime overhead
is negligible. Also, if that _was_ a concern, one could always
declare the accessor function as "static inline".

The the same time, forcing userspace to wait for 10ms for no good
reason is never a good idea, and "it is not called that often" is
not a valid argument. Add a thousand "not called that often" pieces
of code to the kernel, and userspace won't do anything but wait
for nothing.

Thanks,
Guenter

> > > +     val = t*10;
> > 
> > Space before and after '*'. Please run checkpatch; that should tell you.
> 
> I did, but either the script missed this or I did. Will fix.
> 
> > > +static struct watchdog_device adv_ec_wdt_dev = {
> > > +     .info =         &adv_ec_wdt_info,
> > > +     .ops =          &adv_ec_wdt_ops,
> > > +     .min_timeout =  MIN_TIME,
> > > +     .max_timeout =  MAX_TIME,
> > 
> > .timeout should be set to the default timeout.
>  
> Copied from existing driver, will fix regardless.
>  
> > > +static void adv_ec_wdt_remove(struct device *dev, unsigned int id)
> > > +{
> > > +     /* stop the watchdog */
> > > +     adv_ec_wdt_stop(NULL);
> > 
> > Call watchdog_stop_on_unregister() before registering the watchdog
> > instead. Also, again, the comment doesn't add value.
> > 
> > > +
> > > +     /* watchdog_unregister() and release_region() are called automatically
> > */
> > 
> > Also unnecessary (implied with devm_). And you don't actually
> > need a remove function when using watchdog_stop_on_unregister().
> 
> Thanks for pointing this out, I had missed that this function & feature exist. 
> 
> > > +             pr_err("Watchdog timer: value of timeout %d (dec) "
> > > +               "is out of range from %d to %d (dec)\n",
> > > +               timeout, MIN_TIME, MAX_TIME);
> > > +             return -EINVAL;
> > 
> > This does not belong here. The probe function should call
> > watchdog_init_timeout() to set and validate the timeout module
> > parameter, and keep the default (instead of failing) if it is
> > out of range.
> 
> Copied from existing driver, will fix regardless.