[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ae9e7c27-e5b0-b431-2811-c5d8d3549e43@linux.intel.com>
Date: Thu, 3 Apr 2025 15:37:21 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Yen-Chi Huang <jesse.huang@...twell.com.tw>
cc: Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, jay.chen@...onical.com
Subject: Re: [PATCH] platform/x86: portwell-ec: Add GPIO and WDT driver for
Portwell EC
On Thu, 3 Apr 2025, Yen-Chi Huang wrote:
Please add watchdog drivers people/lists from MAINTAINERS file into the
next submission.
> 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>
> ---
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 14 ++
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/portwell-ec.c | 224 +++++++++++++++++++++++++++++
> 4 files changed, 247 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..5b61ab422953 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 && 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..59c51b80a148
> --- /dev/null
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -0,0 +1,224 @@
> +// 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-255 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)
> + *
Remove the extra line.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/watchdog.h>
> +#include <linux/io.h>
> +#include <linux/string.h>
Please sort alphabetically.
> +
> +#define PORTWELL_EC_IOSPACE 0xe300
> +#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_EC_COUNT_MIN_ADDR 0x07
> +#define PORTWELL_WDT_EC_COUNT_SEC_ADDR 0x08
> +#define PORTWELL_WDT_EC_MAX_COUNT_SECOND 255
> +#define PORTWELL_EC_FW_VENDOR_ADDRESS 0x4d
> +#define PORTWELL_EC_FW_VENDOR_LENGTH 3
> +#define PORTWELL_EC_FW_VENDOR_NAME "PWG"
> +
> + /* Functions for access EC via IOSPACE*/
Missing space.
> +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);
IIRC, CONFIG_HAS_IOPORT is these days required for iob/outb() so you
should add depends on HAS_IOPORT into Kconfig.
> +}
> +
> +/* GPIO functions*/
Missing space.
> +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)
> +{
> + return (pwec_read(PORTWELL_GPIO_DIR_REG) & (1 << offset))
> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
Please move ? to the preceeding line
I'd add a local variable for the read result to make this more readable.
> +}
> +
> +static int pwec_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> + /* Changing direction causes issues on some boards, so it's disabled for now. */
> + return -EOPNOTSUPP;
> +}
> +
> +static int pwec_gpio_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> + /* Changing direction causes issues on some boards, so it's disabled for now. */
Perhaps just one comment above both functions would suffice. The functions
are right after another so it seems overkill to have the same comment for
both.
> + 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,
> + .ngpio = PORTWELL_GPIO_PINS,
> +};
> +
> +/* Watchdog functions*/
Missing space
> +static int pwec_wdt_start(struct watchdog_device *wdd)
> +{
> + int retry = 10;
> + u8 timeout;
> +
> + do {
> + pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
> + pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
Please use named defines and FIELD_PREP() instead of comments.
> + timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> + retry--;
> + } while (timeout != wdd->timeout && retry > 0);
> + pr_info("Portwell EC: Watchdog started with timeout: %d seconds\n", wdd->timeout);
> + return (retry > 0) ? 0 : -EIO;
> +}
> +
> +static int pwec_wdt_stop(struct watchdog_device *wdd)
> +{
> + pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x00);
> + pr_info("Portwell EC: Watchdog stopped\n");
> + return 0;
> +}
> +
> +static int pwec_wdt_trigger(struct watchdog_device *wdd)
> +{
> + int retry = 10;
> + u8 timeout;
> +
> + pr_info("Portwell EC: Watchdog triggered with timeout: %d seconds\n", wdd->timeout);
This going to be pretty noisy.
> + do {
> + pwec_write(PORTWELL_WDT_EC_COUNT_SEC_ADDR, wdd->timeout);
> + pwec_write(PORTWELL_WDT_EC_CONFIG_ADDR, 0x01); // WDTCFG[1:0]=01
> + timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> + retry--;
> + } while (timeout != wdd->timeout && retry > 0);
> + return (retry > 0) ? 0 : -EIO;
Duplicated code.
> +}
> +
> +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_SEC_ADDR, wdd->timeout);
> + pr_info("Portwell EC: Watchdog timeout is set: %d seconds\n", wdd->timeout);
> + return 0;
> +}
> +
> +static unsigned int pwec_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + unsigned int timeout;
> +
> + timeout = pwec_read(PORTWELL_WDT_EC_COUNT_SEC_ADDR);
> + pr_info("Portwell EC: Watchdog timeout left: %d seconds\n", timeout);
> + return timeout;
> +}
> +
> +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 = 10,
> + .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];
Add spaces around +
> + u8 i;
> +
> + for (i = 0; i < PORTWELL_EC_FW_VENDOR_LENGTH; i++)
> + buf[i] = pwec_read(PORTWELL_EC_FW_VENDOR_ADDRESS+i);
Spaces around +
> + buf[PORTWELL_EC_FW_VENDOR_LENGTH] = '\0';
> + return (strcmp(PORTWELL_EC_FW_VENDOR_NAME, buf) == 0) ? 0 : -ENODEV;
> +}
> +
> +static int __init pwec_init(void)
> +{
> + int result;
> +
> + result = pwec_firmware_vendor_check();
So this goes immediately to poke some io ports? On any x86 machine?
The cases should be narrowed down first with dmi matching.
> + 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;
> + }
> +
> + pr_info("Portwell EC driver initialized\n");
Ok path should be silent.
> + return 0;
> +}
> +
> +static void __exit pwec_exit(void)
> +{
> + watchdog_unregister_device(&ec_wdt_dev);
> + gpiochip_remove(&pwec_gpio_chip);
> + pr_info("Portwell EC driver removed\n");
Remove print.
> +}
> +
> +module_init(pwec_init);
> +module_exit(pwec_exit);
> +
> +MODULE_AUTHOR("Yen-Chi Huang");
> +MODULE_DESCRIPTION("Portwell EC Driver");
> +MODULE_LICENSE("GPL");
>
--
i.
Powered by blists - more mailing lists