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 <[email protected]> > --- > 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: [email protected] > S: Maintained > F: drivers/platform/x86/adv_swbutton.c > > +ADVANTECH EIO DRIVER > +M: Ramiro Oliveira <[email protected]> > +S: Maintained > +F: drivers/mfd/eio_core.c > +F: include/linux/mfd/eio.h > + > ADXL313 THREE-AXIS DIGITAL ACCELEROMETER DRIVER > M: Lucas Stankus <[email protected]> > 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 <[email protected]>"); > +MODULE_AUTHOR("Ramiro Oliveira <[email protected]>"); > +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 [李琼斯]
