| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260108144526.GJ302752@google.com>
Date: Thu, 8 Jan 2026 14:45:26 +0000
From: Lee Jones <lee@...nel.org>
To: Ramiro Oliveira <ramiro.oliveira@...antech.com>
Cc: Linus Walleij <linusw@...nel.org>,
Bartosz Golaszewski <brgl@...nel.org>,
Guenter Roeck <linux@...ck-us.net>,
Andi Shyti <andi.shyti@...nel.org>,
Daniel Thompson <danielt@...nel.org>,
Jingoo Han <jingoohan1@...il.com>, Helge Deller <deller@....de>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-hwmon@...r.kernel.org, linux-i2c@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-pm@...r.kernel.org,
Wenkai Chung <wenkai.chung@...antech.com.tw>,
Francisco Aragon-Trivino <francisco.aragon-trivino@...antech.com>,
Hongzhi Wang <hongzhi.wang@...antech.com>,
Mikhail Tsukerman <mikhail.tsukerman@...antech.com>,
Thomas Kastner <thomas.kastner@...antech.com>
Subject: Re: [PATCH 1/8] Add Advantech EIO MFD driver
On Fri, 12 Dec 2025, Ramiro Oliveira wrote:
> Creating the MFD core driver for Advantech EIO, all other drivers (GPIO,
> I2C, etc) depend on this core driver.
You're going to have to come up with a MUCH better commit message than
that for 800 line driver!
> Signed-off-by: Ramiro Oliveira <ramiro.oliveira@...antech.com>
> ---
> MAINTAINERS | 6 +
> drivers/mfd/Kconfig | 10 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/eio_core.c | 621 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/eio.h | 127 ++++++++++
> 5 files changed, 765 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 663e86eb9ff1..bd9279796c2f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -616,6 +616,12 @@ L: platform-driver-x86@...r.kernel.org
> S: Maintained
> F: drivers/platform/x86/adv_swbutton.c
>
> +ADVANTECH EIO DRIVER
> +M: Ramiro Oliveira <ramiro.oliveira@...antech.com>
> +S: Maintained
> +F: drivers/mfd/eio_core.c
> +F: include/linux/mfd/eio.h
> +
> ADXL313 THREE-AXIS DIGITAL ACCELEROMETER DRIVER
> M: Lucas Stankus <lucas.p.stankus@...il.com>
> S: Supported
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index aace5766b38a..02a0b324eb6a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -506,6 +506,16 @@ config MFD_DLN2
> etc. must be enabled in order to use the functionality of
> the device.
>
> +config MFD_EIO
> + tristate "Advantech EIO MFD core"
Drop the term MFD, it doesn't mean anything. We made it up.
What is this device?
> + select MFD_CORE
> + help
> + This enables support for the Advantech EIO multi-function device.
Remove all mentions of MFD.
> + This core driver provides register access and coordination for the
> + EIO's subdevices (GPIO, watchdog, hwmon, thermal, backlight, I2C).
> + This driver supports EIO-IS200, EIO-201, EIO-210 and EIO-211.
Which are?
> +
> +
> config MFD_ENE_KB3930
> tristate "ENE KB3930 Embedded Controller support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..f8c53b55b679 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
> obj-$(CONFIG_MFD_CS42L43) += cs42l43.o
> obj-$(CONFIG_MFD_CS42L43_I2C) += cs42l43-i2c.o
> obj-$(CONFIG_MFD_CS42L43_SDW) += cs42l43-sdw.o
> +obj-$(CONFIG_MFD_EIO) += eio_core.o
> obj-$(CONFIG_MFD_ENE_KB3930) += ene-kb3930.o
> obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
> obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o
> diff --git a/drivers/mfd/eio_core.c b/drivers/mfd/eio_core.c
> new file mode 100644
> index 000000000000..7a58c62595a5
> --- /dev/null
> +++ b/drivers/mfd/eio_core.c
> @@ -0,0 +1,621 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Advantech Embedded Controller base Driver
> + *
> + * This driver provides an interface to access the EIO Series EC
> + * firmware via its own Power Management Channel (PMC) for subdrivers:
':' without follow-up looks odd.
> + * A system may have one or two independent EIO devices.
> + *
> + * Copyright (C) 2025 Advantech Co., Ltd.
This needs updating on the next iteration.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/isa.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +#include <linux/time.h>
> +#include <linux/uaccess.h>
> +#include <linux/version.h>
> +#include <linux/mfd/eio.h>
Alphabetical.
Can you make sure that _all_ of these are in use.
> +#define TIMEOUT_MAX (10 * USEC_PER_SEC)
> +#define TIMEOUT_MIN 200
> +#define SLEEP_MAX 200
> +#define DEFAULT_TIMEOUT 5000
Tab these out.
Are these values arbitrary or do they come from some spec?
> +
> +/**
Why are you using kernel-doc comments here?
Did you compile with W=1?
> + * Timeout: Default timeout in microseconds when a PMC command's
> + * timeout is unspecified. PMC command responses typically range
> + * from 200us to 2ms. 5ms is quite a safe value for timeout. In
Superfluous "In".
> + * In some cases, responses are longer. In such situations, please
In what cases?
> + * adding the timeout parameter loading related sub-drivers or
> + * this core driver (not recommended).
I can't read this.
> + */
> +static uint timeout = DEFAULT_TIMEOUT;
> +module_param(timeout, uint, 0444);
> +MODULE_PARM_DESC(timeout, "Default PMC command timeout in usec.\n");
You want the user to override the timeout? Are you sure?
> +struct eio_dev_port {
> + u16 idx_port;
> + u16 data_port;
> +};
> +
> +static struct eio_dev_port pnp_port[] = {
> + { .idx_port = EIO_PNP_INDEX, .data_port = EIO_PNP_DATA },
> + { .idx_port = EIO_SUB_PNP_INDEX,
> + .data_port = EIO_SUB_PNP_DATA },
Either place this on the line above or use proper multi-line format.
> +};
> +
> +static struct mfd_cell mfd_devs[] = {
eio_devs
> + { .name = "eio_wdt" },
> + { .name = "gpio_eio" },
> + { .name = "eio_hwmon" },
> + { .name = "i2c_eio" },
> + { .name = "eio_thermal" },
> + { .name = "eio_fan" },
> + { .name = "eio_bl" },
MFD_CELL_NAME()
> +};
> +
> +static const struct regmap_range eio_range[] = {
> + regmap_reg_range(EIO_PNP_INDEX, EIO_PNP_DATA),
> + regmap_reg_range(EIO_SUB_PNP_INDEX, EIO_SUB_PNP_DATA),
> + regmap_reg_range(0x200, 0x3FF),
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> + .yes_ranges = eio_range,
> + .n_yes_ranges = ARRAY_SIZE(eio_range),
> +};
> +
> +static const struct regmap_config pnp_regmap_config = {
> + .name = "eio_core",
> + .reg_bits = 16,
> + .val_bits = 8,
> + .volatile_table = &volatile_regs,
> + .io_port = true,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static struct {
> + char name[32];
> + int cmd;
> + int ctrl;
> + int dev;
> + int size;
> + enum {
> + HEX,
> + NUMBER,
> + PNP_ID,
> + } type;
> +
Remove this line.
> +} attrs[] = {
> + { "board_name", 0x53, 0x10, 0, 16 },
> + { "board_serial", 0x53, 0x1F, 0, 16 },
> + { "board_manufacturer", 0x53, 0x11, 0, 16 },
> + { "board_id", 0x53, 0x1E, 0, 4 },
> + { "firmware_version", 0x53, 0x21, 0, 4 },
> + { "firmware_name", 0x53, 0x22, 0, 16 },
> + { "firmware_build", 0x53, 0x23, 0, 26 },
> + { "firmware_date", 0x53, 0x24, 0, 16 },
> + { "chip_id", 0x53, 0x12, 0, 12 },
> + { "chip_detect", 0x53, 0x15, 0, 12 },
> + { "platform_type", 0x53, 0x13, 0, 16 },
> + { "platform_revision", 0x53, 0x04, 0x44, 4 },
> + { "eapi_version", 0x53, 0x04, 0x64, 4 },
> + { "eapi_id", 0x53, 0x31, 0, 4 },
> + { "boot_count", 0x55, 0x10, 0, 4, NUMBER },
> + { "powerup_hour", 0x55, 0x11, 0, 4, NUMBER },
> + { "pnp_id", 0x53, 0x04, 0x68, 4, PNP_ID },
> +};
As "fun" as all of these sysfs entries are, how useful are they to you?
Can you say in good conscience that they are all in active use?
> +static ssize_t info_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
Use 100-chars to avoid these line-wraps.
> +{
> + uint i;
> +
> + for (i = 0; i < ARRAY_SIZE(attrs); i++) {
> + int ret;
> + char str[32] = "";
> + int val;
> +
> + struct pmc_op op = {
> + .cmd = attrs[i].cmd,
> + .control = attrs[i].ctrl,
> + .device_id = attrs[i].dev,
> + .payload = (u8 *)str,
> + .size = attrs[i].size,
> + };
> +
> + if (strcmp(attr->attr.name, attrs[i].name))
> + continue;
> +
> + ret = eio_core_pmc_operation(dev, &op);
> + if (ret)
> + return ret;
> +
> + if (attrs[i].size != 4)
Is this dictated by the user?
> + return sprintf(buf, "%s\n", str);
sprintf() is unsafe. Use sysfs_emit() instead. Throughout.
> + val = *(u32 *)str;
> +
> + if (attrs[i].type == HEX)
> + return sprintf(buf, "0x%08X\n", val);
> +
> + if (attrs[i].type == NUMBER)
> + return sprintf(buf, "%d\n", val);
> +
> + /* Should be pnp_id */
"Should"?
Not good enough. Why not check for PNP_ID instead?
> + return sprintf(buf, "%c%c%c, %X\n", (val >> 14 & 0x3F) + 0x40,
> + ((val >> 9 & 0x18) | (val >> 25 & 0x07)) + 0x40,
> + (val >> 20 & 0x1F) + 0x40, val & 0xFFF);
> + }
> +
> + return -EINVAL;
> +}
> +
> +#define PMC_DEVICE_ATTR_RO(_name) \
> + static ssize_t _name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> + { \
> + return info_show(dev, attr, buf); \
> + } \
> + static DEVICE_ATTR_RO(_name)
Place this out the way, in a header file.
> +PMC_DEVICE_ATTR_RO(board_name);
> +PMC_DEVICE_ATTR_RO(board_serial);
> +PMC_DEVICE_ATTR_RO(board_manufacturer);
> +PMC_DEVICE_ATTR_RO(firmware_name);
> +PMC_DEVICE_ATTR_RO(firmware_version);
> +PMC_DEVICE_ATTR_RO(firmware_build);
> +PMC_DEVICE_ATTR_RO(firmware_date);
> +PMC_DEVICE_ATTR_RO(chip_id);
> +PMC_DEVICE_ATTR_RO(chip_detect);
> +PMC_DEVICE_ATTR_RO(platform_type);
> +PMC_DEVICE_ATTR_RO(platform_revision);
> +PMC_DEVICE_ATTR_RO(board_id);
> +PMC_DEVICE_ATTR_RO(eapi_version);
> +PMC_DEVICE_ATTR_RO(eapi_id);
> +PMC_DEVICE_ATTR_RO(boot_count);
> +PMC_DEVICE_ATTR_RO(powerup_hour);
> +PMC_DEVICE_ATTR_RO(pnp_id);
> +
> +static struct attribute *pmc_attrs[] = { &dev_attr_board_name.attr,
The attribute goes on a new line, then you can reign in all of the
crazy tabbing that follows.
> + &dev_attr_board_serial.attr,
> + &dev_attr_board_manufacturer.attr,
> + &dev_attr_firmware_name.attr,
> + &dev_attr_firmware_version.attr,
> + &dev_attr_firmware_build.attr,
> + &dev_attr_firmware_date.attr,
> + &dev_attr_chip_id.attr,
> + &dev_attr_chip_detect.attr,
> + &dev_attr_platform_type.attr,
> + &dev_attr_platform_revision.attr,
> + &dev_attr_board_id.attr,
> + &dev_attr_eapi_version.attr,
> + &dev_attr_eapi_id.attr,
> + &dev_attr_boot_count.attr,
> + &dev_attr_powerup_hour.attr,
> + &dev_attr_pnp_id.attr,
> + NULL };
> +
> +ATTRIBUTE_GROUPS(pmc);
> +
> +static unsigned int eio_pnp_read(struct device *dev,
> + struct eio_dev_port *port, u8 idx)
100-chars throughout.
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + if (regmap_write(eio->map, port->idx_port, idx))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +
> + if (regmap_read(eio->map, port->data_port, &val))
> + dev_err(dev, "Error port read 0x%X\n", port->data_port);
> +
> + return val;
> +}
> +
> +static void eio_pnp_write(struct device *dev, struct eio_dev_port *port,
Why are these functions not propagating errors?
> + u8 idx, u8 data)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> +
> + if (regmap_write(eio->map, port->idx_port, idx) ||
> + regmap_write(eio->map, port->data_port, data))
> + dev_err(dev, "Error port write 0x%X %X\n", port->idx_port,
> + port->data_port);
You cannot print and error, then return like everything is okay.
> +}
> +
> +static void eio_pnp_enter(struct device *dev, struct eio_dev_port *port)
_unlock_port()
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
'\n'
> + /* Write 0x87 to index port twice to unlock IO port */
> + if (regmap_write(eio->map, port->idx_port,
> + EIO_EXT_MODE_ENTER) ||
This does on the line above.
> + regmap_write(eio->map, port->idx_port, EIO_EXT_MODE_ENTER))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +}
> +
> +static void eio_pnp_leave(struct device *dev, struct eio_dev_port *port)
_lock_port()
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
'\n'
> + /* Write 0xAA to index port once to lock IO port */
> + if (regmap_write(eio->map, port->idx_port, EIO_EXT_MODE_EXIT))
> + dev_err(dev, "Error port write 0x%X\n", port->idx_port);
> +}
What's the difference between these eio_ calls and the pmc_ ones below?
Please find a way to make that clear - a header comment?
> +static int pmc_write_data(struct device *dev, int id, u8 value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + int ret;
> +
> + if (WAIT_IBF(dev, id, timeout))
This is not a macro.
Just use eio_core_pmc_wait() and have done.
> + return -ETIME;
I think you mean -ETIMEDOUT, throughout.
Also, eio_core_pmc_wait() returns its own error value which you are now
overwriting. Why not simply propagate the original error?
> +
> + ret = regmap_write(eio->map, eio->pmc[id].data, value);
> + if (ret)
> + dev_err(dev, "Error PMC write %X:%X\n",
> + eio->pmc[id].data, value);
> +
> + return ret;
> +}
> +
> +static int pmc_write_cmd(struct device *dev, int id, u8 value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + int ret;
> +
> + if (WAIT_IBF(dev, id, timeout))
> + return -ETIME;
> +
> + ret = regmap_write(eio->map, eio->pmc[id].cmd, value);
> + if (ret)
> + dev_err(dev, "Error PMC write %X:%X\n",
> + eio->pmc[id].cmd, value);
> +
> + return ret;
> +}
> +
> +static int pmc_read_data(struct device *dev, int id, u8 *value, u16 timeout)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + if (WAIT_OBF(dev, id, timeout))
> + return -ETIME;
> +
> + ret = regmap_read(eio->map, eio->pmc[id].data, &val);
> + if (ret)
> + dev_err(dev, "Error PMC read %X\n", eio->pmc[id].data);
> + else
> + *value = (u8)(val & 0xFF);
> +
> + return ret;
> +}
> +
> +static int pmc_read_status(struct device *dev, int id)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + if (regmap_read(eio->map, eio->pmc[id].status, &val)) {
> + dev_err(dev, "Error PMC read %X\n",
> + eio->pmc[id].status);
> + return 0;
> + }
> +
> + return val;
> +}
> +
> +static void pmc_clear(struct device *dev, int id)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + unsigned int val;
> +
> + /* Check if input buffer blocked */
> + if ((pmc_read_status(dev, id) & EIO_PMC_STATUS_IBF) == 0)
> + return;
> +
> + /* Read out previous garbage */
> + if (regmap_read(eio->map, eio->pmc[id].data, &val))
> + dev_err(dev, "Error pmc clear\n");
What do you expect the user to do about this?
Why is it an issue that there is nothing to read?
> +
> + usleep_range(10, 100);
> +}
> +
> +int eio_core_pmc_wait(struct device *dev, int id,
> + enum eio_pmc_wait wait, uint max_duration)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + uint val;
> + int new_timeout = max_duration ? max_duration : timeout;
max_duration?: timeout;
> +
> + if (new_timeout < TIMEOUT_MIN || new_timeout > TIMEOUT_MAX) {
> + dev_err(dev,
> + "Error timeout value: %dus. Timeout value should between %d and %ld\n",
Suggest that the user should not specify a timeout, then all of this can go.
> + new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
> + return -ETIME;
> + }
> +
> + if (wait == PMC_WAIT_INPUT)
> + return regmap_read_poll_timeout(eio->map, eio->pmc[id].status,
> + val, (val & EIO_PMC_STATUS_IBF) == 0,
> + SLEEP_MAX, new_timeout);
> + return regmap_read_poll_timeout(eio->map,
> + eio->pmc[id].status, val,
> + (val & EIO_PMC_STATUS_OBF) != 0,
> + SLEEP_MAX, new_timeout);
These stacked timeouts are going to need some explanation.
> +}
> +EXPORT_SYMBOL_GPL(eio_core_pmc_wait);
> +
> +int eio_core_pmc_operation(struct device *dev, struct pmc_op *op)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u8 i;
> + int ret;
> + bool read_cmd = op->cmd & EIO_FLAG_PMC_READ;
Suggest a rename.
read_cmd sounds more like a function call or command value rather than a
bool to match on.
What about "reading".
> + ktime_t t = ktime_get();
Nit: Reverse Christmas tree is kinder on the reader.
> + mutex_lock(&eio->mutex);
> +
> + pmc_clear(dev, op->chip);
> +
> + ret = pmc_write_cmd(dev, op->chip, op->cmd, op->timeout);
Why not just provide "op" and let the callee extract what it needs?
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->control, op->timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->device_id, op->timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, op->chip, op->size, op->timeout);
> + if (ret)
> + goto err;
> +
> + for (i = 0; i < op->size; i++) {
> + if (read_cmd)
> + ret = pmc_read_data(dev, op->chip, &op->payload[i],
> + op->timeout);
> + else
> + ret = pmc_write_data(dev, op->chip, op->payload[i],
> + op->timeout);
> +
> + if (ret)
> + goto err;
Why not break, unlock, then return 0 if (!ret).
> + }
> +
> + mutex_unlock(&eio->mutex);
> +
> + return 0;
> +
> +err:
> + mutex_unlock(&eio->mutex);
> +
> + dev_err(dev, "PMC error duration:%lldus",
> + ktime_to_us(ktime_sub(ktime_get(), t)));
Who is this helpful to?
> + dev_err(dev,
> + ".cmd=0x%02X, .ctrl=0x%02X .id=0x%02X, .size=0x%02X .data=0x%02X%02X",
> + op->cmd, op->control, op->device_id, op->size, op->payload[0],
> + op->payload[1]);
This looks like debug crud that can be removed when the driver is published.
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eio_core_pmc_operation);
> +
> +static int get_pmc_port(struct device *dev, int id,
This does not tell me what the function does.
Please improve the nomenclature.
> + struct eio_dev_port *port)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + struct _pmc_port *pmc = &eio->pmc[id];
> +
> + eio_pnp_enter(dev, port);
> +
> + /* Switch to PMC device page */
> + eio_pnp_write(dev, port, EIO_LDN, EIO_LDN_PMC1);
> +
> + /* Active this device */
> + eio_pnp_write(dev, port, EIO_LDAR, EIO_LDAR_LDACT);
> +
> + /* Get PMC cmd and data port */
> + pmc->data = eio_pnp_read(dev, port, EIO_IOBA0H) << 8;
> + pmc->data |= eio_pnp_read(dev, port, EIO_IOBA0L);
> + pmc->cmd = eio_pnp_read(dev, port, EIO_IOBA1H) << 8;
> + pmc->cmd |= eio_pnp_read(dev, port, EIO_IOBA1L);
> +
> + /* Disable IRQ */
> + eio_pnp_write(dev, port, EIO_IRQCTRL, 0);
> +
> + eio_pnp_leave(dev, port);
> +
> + /* Make sure IO ports are not occupied */
> + if (!devm_request_region(dev, pmc->data, 2, KBUILD_MODNAME)) {
Break the call out of the if please.
> + dev_err(dev, "Request region %X error\n", pmc->data);
> + return -EBUSY;
> + }
> +
> + return 0;
> +}
> +
> +static int eio_init(struct device *dev)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u16 chip_id = 0;
> + u8 tmp = 0;
control
> + int chip = 0;
Why are all of these being pre-initialised?
> + int ret = -ENOMEM;
> +
> + for (chip = 0; chip < ARRAY_SIZE(pnp_port); chip++) {
for (int chip_id ...)
Hold on, you have chip_id below.
What's the difference between chip and chip_id?
> + struct eio_dev_port *port = pnp_port + chip;
> +
> + if (!devm_request_region(dev, pnp_port[chip].idx_port,
Break this out.
> + pnp_port[chip].data_port -
> + pnp_port[chip].idx_port,
> + KBUILD_MODNAME))
> + continue;
> +
> + eio_pnp_enter(dev, port);
> +
> + chip_id = eio_pnp_read(dev, port, EIO_CHIPID1) << 8;
> + chip_id |= eio_pnp_read(dev, port, EIO_CHIPID2);
> +
> + if (chip_id != EIO200_CHIPID && chip_id != EIO201_211_CHIPID)
> + continue;
> +
> + /* Turn on the enable flag */
> + tmp = eio_pnp_read(dev, port, EIO_SIOCTRL);
> + tmp |= EIO_SIOCTRL_SIOEN;
> +
> + eio_pnp_write(dev, port, EIO_SIOCTRL, tmp);
> +
> + eio_pnp_leave(dev, port);
> +
> + ret = get_pmc_port(dev, chip, port);
> + if (ret)
> + return ret;
> +
> + if (chip == 0)
> + eio->flag |= EIO_F_CHIP_EXIST;
> + else
> + eio->flag |= EIO_F_SUB_CHIP_EXIST;
> + }
> +
> + return ret;
> +}
> +
> +static uint8_t acpiram_access(struct device *dev, uint8_t offset)
What's actually happening here?
Should that be "acpi_ram"
> +{
> + u8 val;
> + int ret;
> + int timeout = 0;
> + struct eio_dev *eio = dev_get_drvdata(dev);
> +
> + /* We only store information on primary EC */
> + int chip = 0;
> +
> + mutex_lock(&eio->mutex);
> +
> + pmc_clear(dev, chip);
> +
> + ret = pmc_write_cmd(dev, chip, EIO_PMC_CMD_ACPIRAM_READ, timeout);
Isn't timeout always 0?
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, chip, offset, timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_write_data(dev, chip, sizeof(val), timeout);
> + if (ret)
> + goto err;
> +
> + ret = pmc_read_data(dev, chip, &val, timeout);
> + if (ret)
> + goto err;
> +
> +err:
> + mutex_unlock(&eio->mutex);
> + return ret ? 0 : val;
What? No. Return the error.
> +}
> +
> +static int firmware_code_base(struct device *dev)
> +{
> + struct eio_dev *eio = dev_get_drvdata(dev);
> + u8 ic_vendor, ic_code, code_base;
> +
> + ic_vendor = acpiram_access(dev, EIO_ACPIRAM_ICVENDOR);
> + ic_code = acpiram_access(dev, EIO_ACPIRAM_ICCODE);
> + code_base = acpiram_access(dev, EIO_ACPIRAM_CODEBASE);
> +
> + if (ic_vendor != 'R')
> + return -ENODEV;
> +
> + if (ic_code != EIO200_ICCODE && ic_code != EIO201_ICCODE &&
> + ic_code != EIO211_ICCODE)
> + goto err;
> +
> + if (code_base == EIO_ACPIRAM_CODEBASE_NEW) {
> + eio->flag |= EIO_F_NEW_CODE_BASE;
> + return 0;
> + }
> +
> + if (code_base == 0 &&
> + (ic_code != EIO201_ICCODE && ic_code != EIO211_ICCODE)) {
> + dev_info(dev, "Old code base not supported, yet.");
Drop the yet. If it becomes supported later, so be it.
> + return -ENODEV;
> + }
> +
> +err:
> + /* Codebase error. This should only happen on firmware error. */
> + dev_err(dev,
> + "Codebase check fail: vendor: 0x%X, code: 0x%X, base: 0x%X\n",
> + ic_vendor, ic_code, code_base);
> + return -ENODEV;
> +}
> +
> +static int eio_probe(struct device *dev, unsigned int id)
> +{
> + int ret = 0;
> + struct eio_dev *eio;
> +
> + eio = devm_kzalloc(dev, sizeof(*eio), GFP_KERNEL);
> + if (!eio)
> + return -ENOMEM;
> +
> + eio->dev = dev;
> + mutex_init(&eio->mutex);
> +
> + eio->iomem = devm_ioport_map(dev, 0, EIO_SUB_PNP_DATA + 1);
> + if (IS_ERR(eio->iomem))
> + return PTR_ERR(eio->iomem);
> +
> + eio->map = devm_regmap_init_mmio(dev, eio->iomem, &pnp_regmap_config);
> + if (IS_ERR(eio->map))
> + return PTR_ERR(eio->map);
> +
> + /* publish instance for subdrivers (dev_get_drvdata(dev->parent)) */
"Publish"
Actually drop this - it's not required.
> + dev_set_drvdata(dev, eio);
> +
> + if (eio_init(dev)) {
> + dev_dbg(dev, "No device found\n");
Drop all debug cruft.
> + return -ENODEV;
> + }
> +
> + ret = firmware_code_base(dev);
> + if (ret) {
> + dev_err(dev, "Chip code base check fail\n");
> + return ret; /* keep helper's return (e.g., -EIO) */
Always do this.
> + }
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> + mfd_devs, ARRAY_SIZE(mfd_devs),
> + NULL, 0, NULL);
> + if (ret)
> + dev_err(dev, "Cannot register child devices (error = %d)\n", ret);
> +
> + dev_dbg(dev, "Module insert completed\n");
Drop.
> + return 0;
> +}
> +
> +static struct isa_driver eio_driver = {
> + .probe = eio_probe,
> +
Remove this line.
Nothing to remove?
> + .driver = {
> + .name = "eio_core",
> + .dev_groups = pmc_groups,
> + },
> +};
> +module_isa_driver(eio_driver, 1);
What's 1? Please define.
> +MODULE_AUTHOR("Wenkai Chung <wenkai.chung@...antech.com.tw>");
> +MODULE_AUTHOR("Ramiro Oliveira <ramiro.oliveira@...antech.com>");
> +MODULE_DESCRIPTION("Advantech EIO series EC core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/eio.h b/include/linux/mfd/eio.h
> new file mode 100644
> index 000000000000..b87614274201
> --- /dev/null
> +++ b/include/linux/mfd/eio.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Header for the Advantech EIO core driver and its sub-drivers
No need for the header part. We can see that this is a header file.
> + *
> + * Copyright (C) 2025 Advantech Co., Ltd.
> + */
> +
> +#ifndef _MFD_EIO_H_
> +#define _MFD_EIO_H_
'\n'
> +#include <linux/io.h>
> +#include <linux/regmap.h>
> +
> +/* Definition */
???
> +#define EIO_CHIPID1 0x20
> +#define EIO_CHIPID2 0x21
> +#define EIO_CHIPVER 0x22
> +#define EIO_SIOCTRL 0x23
> +#define EIO_SIOCTRL_SIOEN BIT(0)
> +#define EIO_SIOCTRL_SWRST BIT(1)
> +#define EIO_IRQCTRL 0x70
> +#define EIO200_CHIPID 0x9610
> +#define EIO201_211_CHIPID 0x9620
> +#define EIO200_ICCODE 0x10
> +#define EIO201_ICCODE 0x20
> +#define EIO211_ICCODE 0x21
> +
> +/* LPC PNP */
> +#define EIO_PNP_INDEX 0x299
> +#define EIO_PNP_DATA 0x29A
> +#define EIO_SUB_PNP_INDEX 0x499
> +#define EIO_SUB_PNP_DATA 0x49A
> +#define EIO_EXT_MODE_ENTER 0x87
> +#define EIO_EXT_MODE_EXIT 0xAA
> +
> +/* LPC LDN */
> +#define EIO_LDN 0x07
> +#define EIO_LDN_PMC0 0x0C
> +#define EIO_LDN_PMC1 0x0D
> +
> +/* PMC registers */
> +#define EIO_PMC_STATUS_IBF BIT(1)
> +#define EIO_PMC_STATUS_OBF BIT(0)
> +#define EIO_LDAR 0x30
> +#define EIO_LDAR_LDACT BIT(0)
> +#define EIO_IOBA0H 0x60
> +#define EIO_IOBA0L 0x61
> +#define EIO_IOBA1H 0x62
> +#define EIO_IOBA1L 0x63
> +#define EIO_FLAG_PMC_READ BIT(0)
> +
> +/* PMC command list */
> +#define EIO_PMC_CMD_ACPIRAM_READ 0x31
> +#define EIO_PMC_CMD_CFG_SAVE 0x56
> +
> +/* OLD PMC */
> +#define EIO_PMC_NO_INDEX 0xFF
> +
> +/* ACPI RAM Address Table */
> +#define EIO_ACPIRAM_VERSIONSECTION (0xFA)
> +#define EIO_ACPIRAM_ICVENDOR (EIO_ACPIRAM_VERSIONSECTION + 0x00)
> +#define EIO_ACPIRAM_ICCODE (EIO_ACPIRAM_VERSIONSECTION + 0x01)
> +#define EIO_ACPIRAM_CODEBASE (EIO_ACPIRAM_VERSIONSECTION + 0x02)
> +
> +#define EIO_ACPIRAM_CODEBASE_NEW BIT(7)
> +
> +/* Firmware */
> +#define EIO_F_SUB_NEW_CODE_BASE BIT(6)
> +#define EIO_F_SUB_CHANGED BIT(7)
> +#define EIO_F_NEW_CODE_BASE BIT(8)
> +#define EIO_F_CHANGED BIT(9)
> +#define EIO_F_SUB_CHIP_EXIST BIT(30)
> +#define EIO_F_CHIP_EXIST BIT(31)
> +
> +/* Others */
> +#define EIO_EC_NUM 2
> +
> +struct _pmc_port {
> + union {
> + u16 cmd;
> + u16 status;
> + };
> + u16 data;
> +};
> +
> +struct pmc_op {
> + u8 cmd;
> + u8 control;
> + u8 device_id;
> + u8 size;
> + u8 *payload;
> + u8 chip;
> + u16 timeout;
> +};
> +
> +enum eio_rw_operation {
> + OPERATION_READ,
> + OPERATION_WRITE,
> +};
> +
> +struct eio_dev {
> + struct device *dev;
> + struct regmap *map;
> + void __iomem *iomem;
> + struct mutex mutex; /* Protects PMC command access */
> + struct _pmc_port pmc[EIO_EC_NUM];
> + u32 flag;
> +};
> +
> +int eio_core_pmc_operation(struct device *dev, struct pmc_op *operation);
> +
> +enum eio_pmc_wait {
> + PMC_WAIT_INPUT,
> + PMC_WAIT_OUTPUT,
> +};
> +
> +int eio_core_pmc_wait(struct device *dev, int id, enum eio_pmc_wait wait,
> + uint timeout);
> +
> +#define WAIT_IBF(dev, id, timeout) eio_core_pmc_wait(dev, id, PMC_WAIT_INPUT, timeout)
> +#define WAIT_OBF(dev, id, timeout) eio_core_pmc_wait(dev, id, PMC_WAIT_OUTPUT, timeout)
> +
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#endif
Does this really do anything valuable?
> +
> +#endif
>
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists