Message-ID: <79e085e1-23a0-4db1-93b6-fcebbc4b31d1@roeck-us.net>
Date: Tue, 15 Apr 2025 21:30:38 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Yen-Chi Huang <jesse.huang@...twell.com.tw>, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com, linus.walleij@...aro.org, brgl@...ev.pl,
 wim@...ux-watchdog.org
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-watchdog@...r.kernel.org,
 jay.chen@...onical.com
Subject: Re: [PATCH v4] platform/x86: portwell-ec: Add GPIO and WDT driver for
 Portwell EC

On 4/15/25 21:10, Yen-Chi Huang wrote:
> Adds a driver for the ITE Embedded Controller (EC) on Portwell boards.
> It integrates with the Linux GPIO and watchdog subsystems to provide:
> 
> - Control/monitoring of up to 8 EC GPIO pins.
> - Hardware watchdog timer with 1-255 second timeouts.
> 
> The driver communicates with the EC via I/O port 0xe300 and identifies
> the hardware by the "PWG" firmware signature. This enables enhanced
> system management for Portwell embedded/industrial platforms.
> 
> Signed-off-by: Yen-Chi Huang <jesse.huang@...twell.com.tw>
> ---
> V3->V4:
>    - Added select WATCHDOG_CORE in Kconfig
>    - Removed redundant retry logic from pwec_wdt_trigger()
>    - Added pwec_wdt_write_timeout() helper
>    - Handled second wraparound when reading min/sec in get_timeleft()
>    - Reworked DMI check and force parameter logic
>    - Fixed error handling for GPIO and platform device registration
>    - Fixed typos, log messages, and formatting issues
>    
> V2->V3:
>    - Reworked based on review from Bartosz Golaszewski
>    - Changed to use platform_driver and platform_device
>    - Updated GPIO to use .set_rv() instead of .set()
>    - Used devm_ helpers for request_region, GPIO and watchdog registration
> 
> V1->V2:
>    - Addressed review comments from Ilpo Jarvinen
>    - Add DMI system check to avoid running on unsupported platforms
>    - Add 'force' module parameter to override DMI matching
>    - Use request_region() to claim I/O port access
>    - Extend WDT timeout handling to use both minute and second registers
>    - Increase WDT max timeout from 255s to 15300s
>    - Use named defines for WDT enable/disable
>    - Remove dummy pr_info() messages
>    - Fix several coding style issues (comments, alignment, spacing)
> 
> ---
>   MAINTAINERS                        |   6 +
>   drivers/platform/x86/Kconfig       |  15 ++
>   drivers/platform/x86/Makefile      |   3 +
>   drivers/platform/x86/portwell-ec.c | 298 +++++++++++++++++++++++++++++
>   4 files changed, 322 insertions(+)
>   create mode 100644 drivers/platform/x86/portwell-ec.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d5dfb9186962..c52f819786dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19132,6 +19132,12 @@ F:	kernel/time/itimer.c
>   F:	kernel/time/posix-*
>   F:	kernel/time/namespace.c
>   
> +PORTWELL EC DRIVER
> +M:	Yen-Chi Huang <jesse.huang@...twell.com.tw>
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/portwell-ec.c
> +
>   POWER MANAGEMENT CORE
>   M:	"Rafael J. Wysocki" <rafael@...nel.org>
>   L:	linux-pm@...r.kernel.org
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 43407e76476b..3ceeb70897eb 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -779,6 +779,21 @@ config PCENGINES_APU2
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called pcengines-apuv2.
>   
> +config PORTWELL_EC
> +	tristate "Portwell Embedded Controller driver"
> +	depends on X86 && HAS_IOPORT && WATCHDOG && GPIOLIB
> +	select WATCHDOG_CORE
> +	help
> +	  This driver provides support for the GPIO pins and watchdog timer
> +	  embedded in Portwell's EC.
> +
> +	  Theoretically, this driver should work on multiple Portwell platforms,
> +	  but it has only been tested on the Portwell NANO-6064 board.
> +	  If you encounter any issues on other boards, please report them.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called portwell-ec.
> +
>   config BARCO_P50_GPIO
>   	tristate "Barco P50 GPIO driver for identify LED/button"
>   	depends on GPIOLIB
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 650dfbebb6c8..83dd82e04457 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -92,6 +92,9 @@ obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
>   # PC Engines
>   obj-$(CONFIG_PCENGINES_APU2)	+= pcengines-apuv2.o
>   
> +# Portwell
> +obj-$(CONFIG_PORTWELL_EC)	+= portwell-ec.o
> +
>   # Barco
>   obj-$(CONFIG_BARCO_P50_GPIO)	+= barco-p50-gpio.o
>   
> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
> new file mode 100644
> index 000000000000..a16dcd96eab2
> --- /dev/null
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * portwell-ec.c: Portwell embedded controller driver.
> + *
> + * Tested on:
> + *  - Portwell NANO-6064
> + *
> + * This driver provides support for GPIO and Watchdog Timer
> + * functionalities of the Portwell boards with ITE embedded controller (EC).
> + * The EC is accessed through I/O ports and provides:
> + *  - 8 GPIO pins for control and monitoring
> + *  - Hardware watchdog with 1-15300 second timeout range
> + *
> + * It integrates with the Linux GPIO and Watchdog subsystems, allowing
> + * userspace interaction with EC GPIO pins and watchdog control,
> + * ensuring system stability and configurability.
> + *
> + * (C) Copyright 2025 Portwell, Inc.
> + * Author: Yen-Chi Huang (jesse.huang@...twell.com.tw)
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/string.h>
> +#include <linux/watchdog.h>
> +
> +#define PORTWELL_EC_IOSPACE              0xe300
> +#define PORTWELL_EC_IOSPACE_LEN          SZ_256
> +
> +#define PORTWELL_GPIO_PINS               8
> +#define PORTWELL_GPIO_DIR_REG            0x2b
> +#define PORTWELL_GPIO_VAL_REG            0x2c
> +
> +#define PORTWELL_WDT_EC_CONFIG_ADDR      0x06
> +#define PORTWELL_WDT_CONFIG_ENABLE       0x1
> +#define PORTWELL_WDT_CONFIG_DISABLE      0x0
> +#define PORTWELL_WDT_EC_COUNT_MIN_ADDR   0x07
> +#define PORTWELL_WDT_EC_COUNT_SEC_ADDR   0x08
> +#define PORTWELL_WDT_EC_MAX_COUNT_SECOND (255 * 60)
> +
> +#define PORTWELL_EC_FW_VENDOR_ADDRESS    0x4d
> +#define PORTWELL_EC_FW_VENDOR_LENGTH     3
> +#define PORTWELL_EC_FW_VENDOR_NAME       "PWG"
> +
> +static bool force;
> +module_param(force, bool, 0444);
> +MODULE_PARM_DESC(force, "Force loading EC driver without checking DMI boardname");
> +
> +static const struct dmi_system_id pwec_dmi_table[] = {
> +	{
> +		.ident = "NANO-6064 series",
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "NANO-6064"),
> +		},
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dmi, pwec_dmi_table);
> +
> +/* Functions for access EC via IOSPACE */
> +
> +static void pwec_write(u8 index, u8 data)
> +{
> +	outb(data, PORTWELL_EC_IOSPACE + index);
> +}
> +
> +static u8 pwec_read(u8 address)
> +{
> +	return inb(PORTWELL_EC_IOSPACE + address);
> +}
> +
> +/* GPIO functions */
> +
> +static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	return (pwec_read(PORTWELL_GPIO_VAL_REG) & (1 << offset)) ? 1 : 0;
> +}
> +
> +static int pwec_gpio_set_rv(struct gpio_chip *chip, unsigned int offset, int val)
> +{
> +	u8 tmp = pwec_read(PORTWELL_GPIO_VAL_REG);
> +
> +	if (val)
> +		tmp |= (1 << offset);
> +	else
> +		tmp &= ~(1 << offset);
> +	pwec_write(PORTWELL_GPIO_VAL_REG, tmp);
> +
> +	return 0;
> +}
> +
> +static int pwec_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	u8 direction = pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset);
> +
> +	if (direction)
> +		return GPIO_LINE_DIRECTION_IN;
> +	else
> +		return GPIO_LINE_DIRECTION_OUT;

Just a side note, you'll get a static analyzer warning telling you
that else after return is not necessary.

> +}
> +
> +/*
> + * Changing direction causes issues on some boards,
> + * so direction_input and direction_output are disabled for now.
> + */
> +
> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct gpio_chip pwec_gpio_chip = {
> +	.label = "portwell-ec-gpio",
> +	.get_direction = pwec_gpio_get_direction,
> +	.direction_input = pwec_gpio_direction_input,
> +	.direction_output = pwec_gpio_direction_output,
> +	.get = pwec_gpio_get,
> +	.set_rv = pwec_gpio_set_rv,
> +	.base = -1,
> +	.ngpio = PORTWELL_GPIO_PINS,
> +};
> +
> +/* Watchdog functions */
> +
> +static void pwec_wdt_write_timeout(unsigned int timeout)
> +{
> +	pwec_write(PORTWELL_WDT_EC_COUNT_MIN_ADDR, timeout / 60);
> +	pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, timeout % 60);
> +}
> +
> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
> +{
> +	pwec_wdt_write_timeout(wdd->timeout);
> +	pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, PORTWELL_WDT_CONFIG_ENABLE);
> +
> +	return 0;
> +}
> +
> +static int pwec_wdt_start(struct watchdog_device *wdd)
> +{
> +	return pwec_wdt_trigger(wdd);
> +}
> +
> +static int pwec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, PORTWELL_WDT_CONFIG_DISABLE);
> +	return 0;
> +}
> +
> +static int pwec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
> +{
> +	if (timeout == 0 || timeout > PORTWELL_WDT_EC_MAX_COUNT_SECOND)
> +		return -EINVAL;
> +

Below:

 > +	.min_timeout = 1,
 > +	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,

This means the watchdog core validates the range. Why check it again here ?

> +	wdd->timeout = timeout;
> +	pwec_wdt_write_timeout(wdd->timeout);
> +
> +	return 0;
> +}
> +
> +/* Ensure consistent min/sec read in case of second rollover. */
> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	u8 min, sec1, sec2;
> +
> +	sec1 = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +	min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
> +	sec2 = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +
> +	if (sec2 > sec1) {
> +		min--;
> +		sec1 = sec2;
> +	}
> +
> +	return min * 60 + sec1;
> +}
> +
> +static const struct watchdog_ops pwec_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pwec_wdt_start,
> +	.stop = pwec_wdt_stop,
> +	.ping = pwec_wdt_trigger,
> +	.set_timeout = pwec_wdt_set_timeout,
> +	.get_timeleft = pwec_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_device ec_wdt_dev = {
> +	.info = &(struct watchdog_info){
> +		.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +		.identity = "Portwell EC watchdog",
> +	},
> +	.ops = &pwec_wdt_ops,
> +	.timeout = 60,
> +	.min_timeout = 1,
> +	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
> +};
> +
> +static int pwec_firmware_vendor_check(void)
> +{
> +	u8 buf[PORTWELL_EC_FW_VENDOR_LENGTH + 1];
> +	u8 i;
> +
> +	for (i = 0; i < PORTWELL_EC_FW_VENDOR_LENGTH; i++)
> +		buf[i] = pwec_read(PORTWELL_EC_FW_VENDOR_ADDRESS + i);
> +	buf[PORTWELL_EC_FW_VENDOR_LENGTH] = '\0';
> +
> +	return (!strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf)) ? 0 : -ENODEV;
> +}
> +
> +static int pwec_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	if (!devm_request_region(&pdev->dev, PORTWELL_EC_IOSPACE,
> +				PORTWELL_EC_IOSPACE_LEN, dev_name(&pdev->dev))) {
> +		dev_err(&pdev->dev, "I/O region 0xE300-0xE3FF busy\n");

... or failed to allocate memory.

> +		return -EBUSY;
> +	}
> +
> +	ret = pwec_firmware_vendor_check();
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &pwec_gpio_chip, NULL);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register Portwell EC GPIO\n");
> +		return ret;
> +	}
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, &ec_wdt_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register Portwell EC Watchdog\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver pwec_driver = {
> +	.driver = {
> +		.name = "portwell-ec",
> +	},
> +	.probe = pwec_probe
> +};
> +
> +static struct platform_device *pwec_dev;
> +
> +static int __init pwec_init(void)
> +{
> +	int ret;
> +
> +	if (!dmi_check_system(pwec_dmi_table)) {
> +		if (!force)
> +			return -ENODEV;
> +		pr_warn("force load portwell-ec without DMI check\n");
> +	}
> +
> +	ret = platform_driver_register(&pwec_driver);
> +	if (ret)
> +		return ret;
> +
> +	pwec_dev = platform_device_register_simple("portwell-ec", -1, NULL, 0);
> +	if (IS_ERR(pwec_dev)) {
> +		platform_driver_unregister(&pwec_driver);
> +		return PTR_ERR(pwec_dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit pwec_exit(void)
> +{
> +	if (pwec_dev)
> +		platform_device_unregister(pwec_dev);
> +	platform_driver_unregister(&pwec_driver);
> +}
> +
> +module_init(pwec_init);
> +module_exit(pwec_exit);
> +
> +MODULE_AUTHOR("Yen-Chi Huang <jesse.huang@...twell.com.tw>");
> +MODULE_DESCRIPTION("Portwell EC Driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists