Re: [PATCH 1/8] Add Advantech EIO MFD driver

From: Lee Jones
Date: Thu Jan 08 2026 - 09:49:46 EST

Next message: Mostafa Saleh: "Re: [PATCH] iommu/io-pgtable-arm: Drop DMA API usage for CMOs"
Previous message: Michael S. Tsirkin: "Re: [PATCH] [v3] Documentation: Provide guidelines for tool-generated content"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/platform/x86/adv_swbutton.c
>
> +ADVANTECH EIO DRIVER
> +M: Ramiro Oliveira <ramiro.oliveira@xxxxxxxxxxxxx>
> +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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Ramiro Oliveira <ramiro.oliveira@xxxxxxxxxxxxx>");
> +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 [李琼斯]

Next message: Mostafa Saleh: "Re: [PATCH] iommu/io-pgtable-arm: Drop DMA API usage for CMOs"
Previous message: Michael S. Tsirkin: "Re: [PATCH] [v3] Documentation: Provide guidelines for tool-generated content"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]