Message-ID: <CAMRc=MensaCPF4PL3C0PgfgR=YY++KRqa3EcXzKkQFu4VftQMA@mail.gmail.com>
Date: Wed, 9 Apr 2025 13:46:04 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Yen-Chi Huang <jesse.huang@...twell.com.tw>
Cc: hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com, 
	linus.walleij@...aro.org, wim@...ux-watchdog.org, linux@...ck-us.net, 
	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 v2] platform/x86: portwell-ec: Add GPIO and WDT driver for
 Portwell EC

On Wed, Apr 9, 2025 at 1:26 PM Yen-Chi Huang
<jesse.huang@...twell.com.tw> 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>
> ---
> V2:
>   - 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       |  14 ++
>  drivers/platform/x86/Makefile      |   3 +
>  drivers/platform/x86/portwell-ec.c | 268 +++++++++++++++++++++++++++++
>  4 files changed, 291 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..2f26d1bf0a75 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -779,6 +779,20 @@ 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
> +       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..e94377906745
> --- /dev/null
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -0,0 +1,268 @@
> +// 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/string.h>
> +#include <linux/watchdog.h>
> +
> +#define PORTWELL_EC_IOSPACE 0xe300
> +#define PORTWELL_EC_IOSPACE_LEN 0x100
> +
> +#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 15300 //255*60secs
> +
> +#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 void pwec_gpio_set(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);
> +}
> +
> +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;
> +}
> +
> +/*
> + * 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 = pwec_gpio_set,

Please use the set_rv() variant, set() is deprecated as of v6.15-rc1.

> +       .base = -1,
> +       .ngpio = PORTWELL_GPIO_PINS,
> +};
> +
> +/* Watchdog functions*/
> +
> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
> +{
> +       int retry = 10;
> +       u8 min, sec;
> +       unsigned int timeout;
> +
> +       do {
> +               pwec_write(PORTWELL_WDT_EC_COUNT_MIN_ADDR, wdd->timeout / 60);
> +               pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout % 60);
> +               pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, PORTWELL_WDT_CONFIG_ENABLE);
> +               min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
> +               sec = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +               timeout = min * 60 + sec;
> +               retry--;
> +       } while (timeout != wdd->timeout && retry >= 0);
> +       if (retry < 0) {
> +               pr_err("watchdog started failed\n");
> +               return -EIO;
> +       } else
> +               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;
> +       wdd->timeout = timeout;
> +       pwec_write(PORTWELL_WDT_EC_COUNT_MIN_ADDR, wdd->timeout / 60);
> +       pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout % 60);
> +       return 0;
> +}
> +
> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +       u8 min, sec;
> +
> +       min = pwec_read(PORTWELL_WDT_EC_COUNT_MIN_ADDR);
> +       sec = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> +       return min  * 60 + sec;
> +}
> +
> +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) ? 0 : -ENODEV;
> +}
> +
> +static int __init pwec_init(void)
> +{

I'm not an expert in x86 platform drivers but shouldn't this be
implemented as an actual platform driver, not a hand-coded
quasi-driver?

Bart

> +       int result;
> +
> +       if (!force) {
> +               if (!dmi_check_system(pwec_dmi_table)) {
> +                       pr_info("unsupported platform\n");
> +                       return -ENODEV;
> +               }
> +       }
> +
> +       if (!request_region(PORTWELL_EC_IOSPACE, PORTWELL_EC_IOSPACE_LEN, "portwell-ec")) {
> +               pr_err("I/O region 0xE300-0xE3FF busy\n");
> +               return -EBUSY;
> +       }
> +
> +       result = pwec_firmware_vendor_check();
> +       if (result < 0)
> +               return result;
> +
> +       result = gpiochip_add_data(&pwec_gpio_chip, NULL);
> +       if (result < 0) {
> +               pr_err("failed to register Portwell EC GPIO\n");
> +               return result;
> +       }
> +
> +       result = watchdog_register_device(&ec_wdt_dev);
> +       if (result < 0) {
> +               gpiochip_remove(&pwec_gpio_chip);
> +               pr_err("failed to register Portwell EC Watchdog\n");
> +               return result;
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit pwec_exit(void)
> +{
> +       watchdog_unregister_device(&ec_wdt_dev);
> +       gpiochip_remove(&pwec_gpio_chip);
> +       release_region(PORTWELL_EC_IOSPACE, PORTWELL_EC_IOSPACE_LEN);
> +}
> +
> +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");
> --
> 2.34.1
>

Powered by blists - more mailing lists