[PATCH 0/4] mfd: eiois200: Add core driver for EIO-IS200

[PATCH 0/4] mfd: eiois200: Add core driver for EIO-IS200

[advantech.susiteam]

The EIO-IS200 Series is an embedded controller (EC) used in various
Advantech hardware products, including embedded systems and industrial
computers. This patch series introduces core support for the Advantech
EIO-IS200 Embedded Controller (EC). The EIO-IS200 is an EC designed
for LPC (Low Pin Count) bus communication, equipped with its own PMC
(Power Management Channel) for communication with its firmware.

The EIO-IS200 EC provides a range of functionalities, including GPIO
management, watchdog timer control, hardware monitoring, I2C
communication, and thermal management. This core driver establishes
the essential infrastructure for interacting with the EIO-IS200 EC
firmware, enabling subsequent child drivers to access and utilize
these features.

This driver is essential for Linux support on hardware equipped with
the EIO-IS200 Series IC. It enables users to access and control both
firmware and hardware features, improving system stability and
functionality.

The patches have been tested on Advantech hardware platforms, and all
functionality is verified to work as expected.

---

[PATCH 1/4] mfd: eiois200: Insert EIO-IS200 core driver to Kconfig
[PATCH 2/4] mfd: eiois200: Insert EIO-IS200 core driver to Makefile
[PATCH 3/4] mfd: eiois200: Add Header for EIO-IS200 Core Driver
[PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[PATCH 1/4] mfd: eiois200: Insert EIO-IS200 core driver to Kconfig

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch adds support for the Advantech EIO-IS200 Embedded
Controller core driver to the Kconfig configuration menu. When
configuring the kernel using tools like menuconfig, you can
select M to build it as a module or select N to exclude it from
being built-in.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/mfd/Kconfig | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index aea95745c73f..85bcb8f95501 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -52,6 +52,23 @@ config MFD_ACT8945A
 	  linear regulators, along with a complete ActivePath battery
 	  charger.
 
+ config MFD_EIOIS200
+	tristate "Advantech EIO-IS200 Embedded Controller core driver"
+	depends on X86
+	depends on m
+	default m
+	select MFD_CORE
+	select REGMAP_MMIO
+	help
+	  This driver provides support for the Advantech EIO-IS200 EC and
+	  activate its sub-drivers, including GPIO, watchdog, hardware
+	  monitor, I2C, and thermal.
+
+	  This driver option only has two states: M and N. It cannot be
+	  built-in. To compile this driver as a module, choose M here; the
+	  module will be called eiois200_core. The system will force it to
+	  M if any one of its sub-drivers is chosen.
+
 config MFD_SUN4I_GPADC
 	tristate "Allwinner sunxi platforms' GPADC MFD driver"
 	select MFD_CORE
-- 
2.34.1

[PATCH 2/4] mfd: eiois200: Insert EIO-IS200 core driver to Makefile

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch adds support for the Advantech EIO-IS200 Embedded
Controller core driver to the Makefile in the drivers/mfd directory.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/mfd/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..46dc303ddae4 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -16,6 +16,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_EIOIS200)	+= eiois200_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
-- 
2.34.1

[PATCH 3/4] mfd: eiois200: Add Header for EIO-IS200 Core Driver

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch adds the header file necessary for the Advantech EIO-IS200
Core Driver. This header file provides the required definitions and
structures for communicating with the EIO-IS200 EC and its various
functionalities.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/mfd/eiois200.h | 137 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)
 create mode 100644 drivers/mfd/eiois200.h

diff --git a/drivers/mfd/eiois200.h b/drivers/mfd/eiois200.h
new file mode 100644
index 000000000000..d4c3f917811c
--- /dev/null
+++ b/drivers/mfd/eiois200.h
@@ -0,0 +1,137 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Header for the Advantech EIO-IS200 core driver and its sub-drivers
+ *
+ * Copyright (C) 2023 Advantech Co., Ltd.
+ * Author: wenkai.chung <wenkai.chung@advantech.com.tw>
+ */
+
+#ifndef _MFD_EIOIS200_H_
+#define _MFD_EIOIS200_H_
+#include <linux/io.h>
+#include <linux/regmap.h>
+
+/* Definition */
+#define EIOIS200_CHIPID1		0x20
+#define EIOIS200_CHIPID2		0x21
+#define EIOIS200_CHIPVER		0x22
+#define EIOIS200_SIOCTRL		0x23
+#define EIOIS200_SIOCTRL_SIOEN		BIT(0)
+#define EIOIS200_SIOCTRL_SWRST		BIT(1)
+#define EIOIS200_IRQCTRL		0x70
+#define EIOIS200_CHIPID			0x9610
+#define EIO201_211_CHIPID		0x9620
+#define EIOIS200_ICCODE			0x10
+#define EIO201_ICCODE			0x20
+#define EIO211_ICCODE			0x21
+
+/* LPC PNP */
+#define EIOIS200_PNP_INDEX		0x299
+#define EIOIS200_PNP_DATA		0x29A
+#define EIOIS200_SUB_PNP_INDEX		0x499
+#define EIOIS200_SUB_PNP_DATA		0x49A
+#define EIOIS200_EXT_MODE_ENTER		0x87
+#define EIOIS200_EXT_MODE_EXIT		0xAA
+
+/* LPC LDN */
+#define EIOIS200_LDN			0x07
+#define EIOIS200_LDN_PMC0		0x0C
+#define EIOIS200_LDN_PMC1		0x0D
+
+/* PMC registers */
+#define EIOIS200_PMC_PORT		0x2F0
+#define EIOIS200_PMC_PORT_SUB		0x60
+#define EIOIS200_PMC_STATUS_IBF		BIT(1)
+#define EIOIS200_PMC_STATUS_OBF		BIT(0)
+#define EIOIS200_LDAR			0x30
+#define EIOIS200_LDAR_LDACT		BIT(0)
+#define EIOIS200_IOBA0H			0x60
+#define EIOIS200_IOBA0L			0x61
+#define EIOIS200_IOBA1H			0x62
+#define EIOIS200_IOBA1L			0x63
+
+/* PMC command list */
+#define EIOIS200_PMC_CMD_ACPIRAM_READ	0x31
+#define EIOIS200_PMC_CMD_CFG_SAVE	0x56
+
+/* OLD PMC */
+#define EIOIS200_PMC_NO_INDEX		0xFF
+
+/* ACPI RAM Address Table */
+#define EIOIS200_ACPIRAM_VERSIONSECTION	(0xFA)
+#define EIOIS200_ACPIRAM_ICVENDOR	(EIOIS200_ACPIRAM_VERSIONSECTION + 0x00)
+#define EIOIS200_ACPIRAM_ICCODE		(EIOIS200_ACPIRAM_VERSIONSECTION + 0x01)
+#define EIOIS200_ACPIRAM_CODEBASE	(EIOIS200_ACPIRAM_VERSIONSECTION + 0x02)
+
+/* Firmware */
+#define EIOIS200_F_SUB_NEW_CODE_BASE	BIT(6)
+#define EIOIS200_F_SUB_CHANGED		BIT(7)
+#define EIOIS200_F_NEW_CODE_BASE	BIT(8)
+#define EIOIS200_F_CHANGED		BIT(9)
+#define EIOIS200_F_SUB_CHIP_EXIST	BIT(30)
+#define EIOIS200_F_CHIP_EXIST		BIT(31)
+
+/* Others */
+#define 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 eiois200_rw_operation {
+	OPERATION_READ,
+	OPERATION_WRITE,
+};
+
+struct eiois200_dev {
+	u32 flag;
+
+	struct _pmc_port  pmc[2];
+
+	struct mutex mutex; /* Protects PMC command access */
+};
+
+/**
+ * eiois200_core_pmc_operation - Execute a new pmc command
+ * @op:		Pointer to an new pmc command.
+ */
+int eiois200_core_pmc_operation(struct _pmc_op *operation);
+
+enum eiois200_pmc_wait {
+	PMC_WAIT_INPUT,
+	PMC_WAIT_OUTPUT,
+};
+
+/**
+ * eiois200_core_pmc_wait - Wait for input / output buffer to be ready
+ * @id:		0 for main chip, 1 for sub chip.
+ * @wait:	%PMC_WAIT_INPUT or %PMC_WAIT_OUTPUT.
+ *		%PMC_WAIT_INPUT for waiting input buffer data ready.
+ *		%PMC_WAIT_OUTPUT for waiting output buffer empty.
+ * @timeout:	The timeout value.
+ */
+int eiois200_core_pmc_wait(int id, enum eiois200_pmc_wait wait, uint timeout);
+
+#define WAIT_IBF(id, timeout)	eiois200_core_pmc_wait(id, PMC_WAIT_INPUT, timeout)
+#define WAIT_OBF(id, timeout)	eiois200_core_pmc_wait(id, PMC_WAIT_OUTPUT, timeout)
+
+#ifdef pr_fmt
+#undef pr_fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#endif
+
+#endif
-- 
2.34.1

[PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[advantech.susiteam]

From: Wenkai <advantech.susiteam@gmail.com>

This patch introduces the Advantech EIO-IS200 Series EC Core Driver.
The EIO-IS200 combines hardware functionality with firmware
capabilities to provide key features through a dedicated Power
Management Channel (PMC).

Key Features:
- Implements the core EIO-IS200 IC driver that serves as the
  interface to the IC's firmware and hardware functions.
- Contains low-level functions for accessing the IC's Power
  Management Channel (PMC).
- Manages PMC command execution, PMC buffer handling, and
  communication.
- Expose a regmap and a mutex for low-level access.
- Activates support for GPIO, I2C, Hwmon, and Watchdog functionalities
  of the EIO-IS200 IC.
- Provides various sysfs attributes to expose information about the
  EC chip, firmware, and motherboard.
- Includes a timeout parameter to allow modification of the default
  PMC command timeout value, which is particularly useful when
  dealing with an extremely slow-responding device.

Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
---
 drivers/mfd/eiois200_core.c | 590 ++++++++++++++++++++++++++++++++++++
 1 file changed, 590 insertions(+)
 create mode 100644 drivers/mfd/eiois200_core.c

diff --git a/drivers/mfd/eiois200_core.c b/drivers/mfd/eiois200_core.c
new file mode 100644
index 000000000000..7459dee1ed53
--- /dev/null
+++ b/drivers/mfd/eiois200_core.c
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Advantech EIO-IS200 Series EC base Driver
+ *
+ * This driver provides an interface to access the EIO-IS200 Series EC
+ * firmware via its own Power Management Channel (PMC) for subdrivers:
+ *
+ * - Watchdog: drivers/watchdog/eiois200_wdt
+ * - GPIO: drivers/gpio/gpio_eiois200
+ * - Hwmon: drivers/hwmon/eiois200_hwmon
+ * - I2C: drivers/i2c/busses/i2c_eiois200
+ * - Thermal: drivers/thermal/eiois200_thermal
+ *
+ * A system may have one or two independent EIO-IS200s.
+ *
+ * Copyright (C) 2023 Advantech Co., Ltd.
+ * Author: wenkai.chung <wenkai.chung@advantech.com.tw>
+ */
+
+#include <linux/delay.h>
+#include <linux/isa.h>
+#include <linux/mfd/core.h>
+#include <linux/mutex.h>
+#include <linux/time.h>
+#include <linux/uaccess.h>
+
+#include "eiois200.h"
+
+#define TIMEOUT_MAX	(10 * 1000 * 1000)
+#define TIMEOUT_MIN	200
+
+static uint timeout = 5000;
+module_param(timeout, uint, 0664);
+MODULE_PARM_DESC(timeout,
+		 "Default PMC command timeout in usec.\n");
+
+struct eiois200_dev_port {
+	u16 idx;
+	u16 data;
+};
+
+struct eiois200_dev_port pnp_port[] = {
+	{ .idx = EIOIS200_PNP_INDEX,	 .data = EIOIS200_PNP_DATA     },
+	{ .idx = EIOIS200_SUB_PNP_INDEX, .data = EIOIS200_SUB_PNP_DATA },
+};
+
+static struct eiois200_dev *eiois200_dev;
+static struct regmap *regmap_is200;
+
+static struct mfd_cell susi_mfd_devs[] = {
+	{ .name = "eiois200_wdt"     },
+	{ .name = "gpio_eiois200"    },
+	{ .name = "eiois200_hwmon"   },
+	{ .name = "eiois200_i2c"     },
+	{ .name = "eiois200_thermal" },
+};
+
+struct regmap_range is200_range[] = {
+	 regmap_reg_range(EIOIS200_PNP_INDEX,	  EIOIS200_PNP_DATA),
+	 regmap_reg_range(EIOIS200_SUB_PNP_INDEX, EIOIS200_SUB_PNP_DATA),
+	 regmap_reg_range(EIOIS200_PMC_PORT,	  EIOIS200_PMC_PORT + 0x0F),
+	 regmap_reg_range(EIOIS200_PMC_PORT_SUB,  EIOIS200_PMC_PORT_SUB + 0x0F),
+};
+
+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges   = is200_range,
+	.n_yes_ranges = ARRAY_SIZE(is200_range),
+};
+
+static const struct regmap_config pnp_regmap_config = {
+	.name		= "eiois200-gpio",
+	.reg_bits	= 16,
+	.val_bits	= 8,
+	.volatile_table = &volatile_regs,
+	.io_port	= true,
+};
+
+/* Following are EIO-IS200 pnp io port access functions */
+static int is200_pnp_read(struct eiois200_dev_port *port, u8 idx)
+{
+	int val;
+
+	if (regmap_write(regmap_is200, port->idx, idx))
+		pr_err("Error port write 0x%X\n", port->idx);
+
+	if (regmap_read(regmap_is200, port->data, &val))
+		pr_err("Error port read 0x%X\n", port->data);
+
+	return val;
+}
+
+static void is200_pnp_write(struct eiois200_dev_port *port, u8 idx, u8 data)
+{
+	if (regmap_write(regmap_is200, port->idx, idx) ||
+	    regmap_write(regmap_is200, port->data, data))
+		pr_err("Error port write 0x%X %X\n",
+		       port->idx, port->data);
+}
+
+static void is200_pnp_enter(struct eiois200_dev_port *port)
+{
+	if (regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER) ||
+	    regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER))
+		pr_err("Error port write 0x%X\n", port->idx);
+}
+
+static void is200_pnp_leave(struct eiois200_dev_port *port)
+{
+	if (regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_EXIT))
+		pr_err("Error port write 0x%X\n", port->idx);
+}
+
+/* Following are EIO-IS200 io port access functions for pmc command */
+static int pmc_write_data(int id, u8 value, u16 timeout)
+{
+	int ret;
+
+	if (WAIT_IBF(id, timeout))
+		return -ETIME;
+
+	ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].data, value);
+	if (ret)
+		pr_err("Error pmc write %X:%X\n",
+		       eiois200_dev->pmc[id].data, value);
+
+	return ret;
+}
+
+static int pmc_write_cmd(int id, u8 value, u16 timeout)
+{
+	int ret;
+
+	if (WAIT_IBF(id, timeout))
+		return -ETIME;
+
+	ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].cmd, value);
+	if (ret)
+		pr_err("Error pmc write %X:%X\n",
+		       eiois200_dev->pmc[id].data, value);
+
+	return ret;
+}
+
+static int pmc_read_data(int id, u8 *value, u16 timeout)
+{
+	int val, ret;
+
+	if (WAIT_OBF(id, timeout))
+		return -ETIME;
+
+	ret = regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val);
+	if (ret)
+		pr_err("Error pmc read %X\n", eiois200_dev->pmc[id].data);
+	else
+		*value = val & 0xFF;
+
+	return ret;
+}
+
+static int pmc_read_status(int id)
+{
+	int val;
+
+	if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val)) {
+		pr_err("Error pmc read %X\n", eiois200_dev->pmc[id].data);
+		return 0;
+	}
+
+	return val;
+}
+
+static void pmc_clear(int id)
+{
+	int val;
+
+	/* Check if input buffer blocked */
+	if ((pmc_read_status(id) & EIOIS200_PMC_STATUS_IBF) == 0)
+		return;
+
+	/* Read out previous garbage */
+	if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val))
+		pr_err("Error pmc clear\n");
+
+	usleep_range(10, 100);
+}
+
+/**
+ * eiois200_core_pmc_wait - Wait for input / output buffer to be ready.
+ * @id:			0 for main chip, 1 for sub chip.
+ * @wait:		%PMC_WAIT_INPUT or %PMC_WAIT_OUTPUT.
+ *			%PMC_WAIT_INPUT for waiting input buffer data ready.
+ *			%PMC_WAIT_OUTPUT for waiting output buffer empty.
+ * max_duration:	The timeout value in usec.
+ */
+int eiois200_core_pmc_wait(int id,
+			   enum eiois200_pmc_wait wait,
+			   uint max_duration)
+{
+	u32	cnt = 0;
+	uint	val;
+	int	ret;
+	int	new_timeout = max_duration ? max_duration : timeout;
+	ktime_t time_end    = ktime_add_us(ktime_get(), new_timeout);
+
+	if (new_timeout > TIMEOUT_MAX ||
+	    new_timeout < TIMEOUT_MIN) {
+		pr_err("Error timeout value: %dus\nTimeout value should between %d and %d\n",
+		       new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
+		return -ETIME;
+	}
+
+	do {
+		ret = regmap_read(regmap_is200,
+				  eiois200_dev->pmc[id].status,
+				  &val);
+		if (ret)
+			return ret;
+
+		if (wait == PMC_WAIT_INPUT) {
+			if ((val & EIOIS200_PMC_STATUS_IBF) == 0)
+				return 0;
+		} else {
+			if ((val & EIOIS200_PMC_STATUS_OBF) != 0)
+				return 0;
+		}
+
+		/* Incremental delay */
+		fsleep(cnt++ * 10);
+
+	} while (ktime_before(ktime_get(), time_end));
+
+	return -ETIME;
+}
+EXPORT_SYMBOL_GPL(eiois200_core_pmc_wait);
+
+/**
+ * eiois200_core_pmc_operation - Execute a pmc command
+ * @op:		Pointer to an pmc command.
+ */
+int eiois200_core_pmc_operation(struct _pmc_op *op)
+{
+	u8 	i;
+	int	ret;
+	bool	read_cmd = op->cmd & 0x01;
+	ktime_t t = ktime_get();
+
+	mutex_lock(&eiois200_dev->mutex);
+
+	pmc_clear(op->chip);
+
+	ret = pmc_write_cmd(op->chip, op->cmd, op->timeout);
+	if (ret)
+		goto err;
+
+	ret = pmc_write_data(op->chip, op->control, op->timeout);
+	if (ret)
+		goto err;
+
+	ret = pmc_write_data(op->chip, op->device_id, op->timeout);
+	if (ret)
+		goto err;
+
+	ret = pmc_write_data(op->chip, op->size, op->timeout);
+	if (ret)
+		goto err;
+
+	for (i = 0 ; i < op->size ; i++) {
+		if (read_cmd)
+			ret = pmc_read_data(op->chip, &op->payload[i], op->timeout);
+		else
+			ret = pmc_write_data(op->chip, op->payload[i], op->timeout);
+
+		if (ret)
+			goto err;
+	}
+
+	mutex_unlock(&eiois200_dev->mutex);
+
+	return 0;
+
+err:
+	mutex_unlock(&eiois200_dev->mutex);
+
+	pr_err("PMC error duration:%lldus\n",
+	       ktime_to_us(ktime_sub(ktime_get(), t)));
+	pr_err(".cmd=0x%02X, .ctrl=0x%02X .id=0x%02X, .size=0x%02X .data=0x%02X%02X\n",
+	       op->cmd, op->control, op->device_id,
+	       op->size, op->payload[0], op->payload[1]);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(eiois200_core_pmc_operation);
+
+static int get_pmc_port(struct device *dev, int id, struct eiois200_dev_port *port)
+{
+	struct _pmc_port *pmc = &eiois200_dev->pmc[id];
+
+	is200_pnp_enter(port);
+
+	/* Switch to PMC device page */
+	is200_pnp_write(port, EIOIS200_LDN, EIOIS200_LDN_PMC1);
+
+	/* Active this device */
+	is200_pnp_write(port, EIOIS200_LDAR, EIOIS200_LDAR_LDACT);
+
+	/* Get PMC cmd and data port */
+	pmc->data  = is200_pnp_read(port, EIOIS200_IOBA0H) << 8;
+	pmc->data |= is200_pnp_read(port, EIOIS200_IOBA0L);
+	pmc->cmd   = is200_pnp_read(port, EIOIS200_IOBA1H) << 8;
+	pmc->cmd  |= is200_pnp_read(port, EIOIS200_IOBA1L);
+
+	/* Disable IRQ */
+	is200_pnp_write(port, EIOIS200_IRQCTRL, 0);
+
+	is200_pnp_leave(port);
+
+	/* Make sure IO ports are not occupied */
+	if (!devm_request_region(dev, pmc->data, 2, KBUILD_MODNAME)) {
+		dev_err(dev, "Request region %X error\n", pmc->data);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int eiois200_exist(struct device *dev)
+{
+	u16  chip_id = 0;
+	u8   tmp = 0;
+	int  i = 0;
+	int  ret = -ENOMEM;
+	char chip[][8] = { "First",  "Second" };
+
+	for (i = 0 ; i < ARRAY_SIZE(pnp_port) ; i++) {
+		struct eiois200_dev_port *port = pnp_port + i;
+
+		if (!devm_request_region(dev,
+					 pnp_port[i].idx,
+					 2,
+					 KBUILD_MODNAME))
+			continue;
+
+		is200_pnp_enter(port);
+
+		chip_id  = is200_pnp_read(port, EIOIS200_CHIPID1) << 8;
+		chip_id |= is200_pnp_read(port, EIOIS200_CHIPID2);
+
+		if (chip_id != EIOIS200_CHIPID &&
+		    chip_id != EIO201_211_CHIPID)
+			continue;
+
+		/* Turn on the enable flag */
+		tmp = is200_pnp_read(port, EIOIS200_SIOCTRL);
+		tmp |= EIOIS200_SIOCTRL_SIOEN;
+		is200_pnp_write(port, EIOIS200_SIOCTRL, tmp);
+
+		is200_pnp_leave(port);
+
+		ret = get_pmc_port(dev, i, port);
+		if (ret)
+			return ret;
+
+		eiois200_dev->flag |= i == 0 ?  EIOIS200_F_CHIP_EXIST :
+						EIOIS200_F_SUB_CHIP_EXIST;
+
+		pr_info("%s chip detected: %04X\n", chip[i], chip_id);
+	}
+
+	return ret;
+}
+
+/* read information about acpi stored in EC */
+static uint8_t acpiram_access(uint8_t offset)
+{
+	u8  val;
+	int ret;
+
+	mutex_lock(&eiois200_dev->mutex);
+
+	pmc_clear(0);
+
+	ret = pmc_write_cmd(0, EIOIS200_PMC_CMD_ACPIRAM_READ, 0);
+	if (ret)
+		goto err;
+
+	ret = pmc_write_data(0, offset, 0);
+	if (ret)
+		goto err;
+
+	ret = pmc_write_data(0, 1, 0);
+	if (ret)
+		goto err;
+
+	ret = pmc_read_data(0, &val, 0);
+	if (ret)
+		goto err;
+
+err:
+	mutex_unlock(&eiois200_dev->mutex);
+	return ret ? 0 : val;
+}
+
+static int firmware_code_base(struct device *dev)
+{
+	u8 ic_vendor, ic_code, code_base;
+
+	ic_vendor = acpiram_access(EIOIS200_ACPIRAM_ICVENDOR);
+	ic_code   = acpiram_access(EIOIS200_ACPIRAM_ICCODE);
+	code_base = acpiram_access(EIOIS200_ACPIRAM_CODEBASE);
+
+	if (ic_vendor != 'R')
+		return -ENODEV;
+
+	if (ic_code != EIOIS200_ICCODE &&
+	    ic_code != EIO201_ICCODE   &&
+	    ic_code != EIO211_ICCODE)
+		goto err;
+
+	if (code_base == 0x80) {
+		eiois200_dev->flag |= EIOIS200_F_NEW_CODE_BASE;
+		return 0;
+	}
+
+	if (code_base == 0 && (ic_code != EIO201_ICCODE &&
+			       ic_code != EIO211_ICCODE)) {
+		pr_info("Old code base not supported, yet.");
+		return -ENODEV;
+	}
+
+ err:
+	dev_err(dev,
+		"Codebase check fail:\n"
+		"ic_vendor: 0x%X  ,ic_code : 0x%X ,code_base : 0x%X\n",
+		ic_vendor, ic_code, code_base);
+	return -ENODEV;
+}
+
+int eiois200_probe(struct device *dev, unsigned int id)
+{
+	int  ret = 0;
+	void __iomem *iomem;
+
+	iomem = devm_ioport_map(dev, 0, EIOIS200_SUB_PNP_DATA + 1);
+	if (!iomem)
+		return -ENOMEM;
+
+	regmap_is200 = devm_regmap_init_mmio(dev, iomem, &pnp_regmap_config);
+	if (!regmap_is200)
+		return -ENOMEM;
+
+	eiois200_dev = devm_kzalloc(dev,
+				    sizeof(struct eiois200_dev),
+				    GFP_KERNEL);
+	if (!eiois200_dev)
+		return -ENOMEM;
+
+	mutex_init(&eiois200_dev->mutex);
+
+	if (eiois200_exist(dev))
+		return -ENODEV;
+
+	if (firmware_code_base(dev)) {
+		dev_err(dev, "Chip code base check fail\n");
+		return -EIO;
+	}
+
+	dev_set_drvdata(dev, eiois200_dev);
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, susi_mfd_devs,
+				   ARRAY_SIZE(susi_mfd_devs),
+				   NULL, 0, NULL);
+	if (ret)
+		dev_err(dev, "Cannot register child devices (error = %d)\n", ret);
+
+	pr_info("%s started", KBUILD_MODNAME);
+
+	return 0;
+}
+
+struct {
+	char name[32];
+	int  cmd;
+	int  ctrl;
+	int  size;
+} attrs[] = {
+	{ "board_name",		0x53, 0x10, 16 },
+	{ "board_serial",	0x53, 0x1F, 16 },
+	{ "board_manufacturer", 0x53, 0x11, 16 },
+	{ "board_id",		0x53, 0x1E,  4 },
+	{ "firmware_version",	0x53, 0x22, 16 },
+	{ "firmware_build",	0x53, 0x23, 26 },
+	{ "firmware_date",	0x53, 0x24, 16 },
+	{ "chip_id",		0x53, 0x12, 12 },
+	{ "chip_detect",	0x53, 0x15, 12 },
+	{ "platform_type",	0x53, 0x13, 16 },
+	{ "platform_revision",	0x53, 0x14,  4 },
+	{ "eapi_version",	0x53, 0x30,  4 },
+	{ "eapi_id",		0x53, 0x31,  4 },
+	{ "boot_count",		0x55, 0x10,  4 },
+	{ "powerup_hour",	0x55, 0x11,  4 },
+};
+
+static ssize_t info_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	uint i;
+
+	for (i = 0 ; i < ARRAY_SIZE(attrs) ; i++)
+		if (!strcmp(attr->attr.name, attrs[i].name)) {
+			int ret;
+			char str[32] = "";
+			struct _pmc_op op = {
+				.cmd     = attrs[i].cmd,
+				.control = attrs[i].ctrl,
+				.payload = (u8 *)str,
+				.size    = attrs[i].size,
+			};
+
+			ret = eiois200_core_pmc_operation(&op);
+			if (ret)
+				return ret;
+
+			if (attrs[i].size == 4)
+				return sysfs_emit(buf, "%X\n", *(u32 *)str);
+			else
+				return sysfs_emit(buf, "%s\n", str);
+		}
+
+	return -EINVAL;
+}
+
+static DEVICE_ATTR(board_name,		0444, info_show, NULL);
+static DEVICE_ATTR(board_serial,	0444, info_show, NULL);
+static DEVICE_ATTR(board_manufacturer,	0444, info_show, NULL);
+static DEVICE_ATTR(firmware_version,	0444, info_show, NULL);
+static DEVICE_ATTR(firmware_build,	0444, info_show, NULL);
+static DEVICE_ATTR(firmware_date,	0444, info_show, NULL);
+static DEVICE_ATTR(chip_id,		0444, info_show, NULL);
+static DEVICE_ATTR(chip_detect,		0444, info_show, NULL);
+static DEVICE_ATTR(platform_type,	0444, info_show, NULL);
+static DEVICE_ATTR(platform_revision,	0444, info_show, NULL);
+static DEVICE_ATTR(board_id,		0444, info_show, NULL);
+static DEVICE_ATTR(eapi_version,	0444, info_show, NULL);
+static DEVICE_ATTR(eapi_id,		0444, info_show, NULL);
+static DEVICE_ATTR(boot_count,		0444, info_show, NULL);
+static DEVICE_ATTR(powerup_hour,	0444, info_show, NULL);
+
+static struct attribute *pmc_attrs[] = {
+	&dev_attr_board_name.attr,
+	&dev_attr_board_serial.attr,
+	&dev_attr_board_manufacturer.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,
+	NULL
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = pmc_attrs,
+};
+
+static const struct attribute_group *attr_groups[] = {
+	&attr_group,
+	NULL
+};
+
+static struct isa_driver eiois200_driver = {
+	.probe    = eiois200_probe,
+
+	.driver = {
+		.owner = THIS_MODULE,
+		.name  = KBUILD_MODNAME,
+		.dev_groups = attr_groups,
+	},
+};
+
+module_isa_driver(eiois200_driver, 1);
+
+MODULE_AUTHOR("susi.driver <susi.driver@advantech.com.tw>");
+MODULE_DESCRIPTION("Advantech EIO-IS200 series EC core driver");
+MODULE_LICENSE("GPL v2");
-- 
2.34.1

Re: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on linus/master next-20230907]
[cannot apply to lee-mfd/for-mfd-fixes v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/advantech-susiteam-gmail-com/mfd-eiois200-Insert-EIO-IS200-core-driver-to-Kconfig/20230907-111628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20230907031320.6814-5-advantech.susiteam%40gmail.com
patch subject: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver
config: i386-randconfig-014-20230907 (https://download.01.org/0day-ci/archive/20230907/202309071531.rvllWPZ0-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309071531.rvllWPZ0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309071531.rvllWPZ0-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mfd/eiois200_core.c:438:5: warning: no previous prototype for 'eiois200_probe' [-Wmissing-prototypes]
     438 | int eiois200_probe(struct device *dev, unsigned int id)
         |     ^~~~~~~~~~~~~~


vim +/eiois200_probe +438 drivers/mfd/eiois200_core.c

   437	
 > 438	int eiois200_probe(struct device *dev, unsigned int id)
   439	{
   440		int  ret = 0;
   441		void __iomem *iomem;
   442	
   443		iomem = devm_ioport_map(dev, 0, EIOIS200_SUB_PNP_DATA + 1);
   444		if (!iomem)
   445			return -ENOMEM;
   446	
   447		regmap_is200 = devm_regmap_init_mmio(dev, iomem, &pnp_regmap_config);
   448		if (!regmap_is200)
   449			return -ENOMEM;
   450	
   451		eiois200_dev = devm_kzalloc(dev,
   452					    sizeof(struct eiois200_dev),
   453					    GFP_KERNEL);
   454		if (!eiois200_dev)
   455			return -ENOMEM;
   456	
   457		mutex_init(&eiois200_dev->mutex);
   458	
   459		if (eiois200_exist(dev))
   460			return -ENODEV;
   461	
   462		if (firmware_code_base(dev)) {
   463			dev_err(dev, "Chip code base check fail\n");
   464			return -EIO;
   465		}
   466	
   467		dev_set_drvdata(dev, eiois200_dev);
   468	
   469		ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, susi_mfd_devs,
   470					   ARRAY_SIZE(susi_mfd_devs),
   471					   NULL, 0, NULL);
   472		if (ret)
   473			dev_err(dev, "Cannot register child devices (error = %d)\n", ret);
   474	
   475		pr_info("%s started", KBUILD_MODNAME);
   476	
   477		return 0;
   478	}
   479	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on linus/master next-20230907]
[cannot apply to lee-mfd/for-mfd-fixes v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/advantech-susiteam-gmail-com/mfd-eiois200-Insert-EIO-IS200-core-driver-to-Kconfig/20230907-111628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20230907031320.6814-5-advantech.susiteam%40gmail.com
patch subject: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver
config: x86_64-randconfig-121-20230907 (https://download.01.org/0day-ci/archive/20230907/202309071936.oPUjyLt5-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309071936.oPUjyLt5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309071936.oPUjyLt5-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/mfd/eiois200_core.c:58:21: sparse: sparse: symbol 'is200_range' was not declared. Should it be static?
>> drivers/mfd/eiois200_core.c:438:5: sparse: sparse: symbol 'eiois200_probe' was not declared. Should it be static?

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[Lee Jones]

On Thu, 07 Sep 2023, advantech.susiteam@gmail.com wrote:

> From: Wenkai <advantech.susiteam@gmail.com>
> 
> This patch introduces the Advantech EIO-IS200 Series EC Core Driver.
> The EIO-IS200 combines hardware functionality with firmware
> capabilities to provide key features through a dedicated Power
> Management Channel (PMC).
> 
> Key Features:
> - Implements the core EIO-IS200 IC driver that serves as the
>   interface to the IC's firmware and hardware functions.
> - Contains low-level functions for accessing the IC's Power
>   Management Channel (PMC).
> - Manages PMC command execution, PMC buffer handling, and
>   communication.
> - Expose a regmap and a mutex for low-level access.
> - Activates support for GPIO, I2C, Hwmon, and Watchdog functionalities
>   of the EIO-IS200 IC.
> - Provides various sysfs attributes to expose information about the
>   EC chip, firmware, and motherboard.
> - Includes a timeout parameter to allow modification of the default
>   PMC command timeout value, which is particularly useful when
>   dealing with an extremely slow-responding device.
> 
> Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
> ---
>  drivers/mfd/eiois200_core.c | 590 ++++++++++++++++++++++++++++++++++++

Please organise patches by functionality, not file.

Each of the patches in this series are worthless without the others.

Have you run static analysis on this?

What about checkpatch.pl?

>  1 file changed, 590 insertions(+)
>  create mode 100644 drivers/mfd/eiois200_core.c
> 
> diff --git a/drivers/mfd/eiois200_core.c b/drivers/mfd/eiois200_core.c
> new file mode 100644
> index 000000000000..7459dee1ed53
> --- /dev/null
> +++ b/drivers/mfd/eiois200_core.c
> @@ -0,0 +1,590 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Advantech EIO-IS200 Series EC base Driver
> + *
> + * This driver provides an interface to access the EIO-IS200 Series EC
> + * firmware via its own Power Management Channel (PMC) for subdrivers:
> + *
> + * - Watchdog: drivers/watchdog/eiois200_wdt
> + * - GPIO: drivers/gpio/gpio_eiois200
> + * - Hwmon: drivers/hwmon/eiois200_hwmon
> + * - I2C: drivers/i2c/busses/i2c_eiois200
> + * - Thermal: drivers/thermal/eiois200_thermal

None of these exist.  I would remove these completely. They will be
highly susceptible to bit-rot.

> + * A system may have one or two independent EIO-IS200s.
> + *
> + * Copyright (C) 2023 Advantech Co., Ltd.
> + * Author: wenkai.chung <wenkai.chung@advantech.com.tw>

Is the name on your government ID "wenkai.chung"?
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/isa.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mutex.h>
> +#include <linux/time.h>
> +#include <linux/uaccess.h>
> +
> +#include "eiois200.h"
> +
> +#define TIMEOUT_MAX	(10 * 1000 * 1000)

There are helpers to convert between time bases.  Please use them.

Where do these values come from?

> +#define TIMEOUT_MIN	200
> +
> +static uint timeout = 5000;

Why 5000?

> +module_param(timeout, uint, 0664);
> +MODULE_PARM_DESC(timeout,
> +		 "Default PMC command timeout in usec.\n");

How useful is it to override this on the commandline, really?

> +struct eiois200_dev_port {
> +	u16 idx;
> +	u16 data;

2 of the worst variable names of all time.

Please ensure variable names describe their purpose.

> +};
> +
> +struct eiois200_dev_port pnp_port[] = {
> +	{ .idx = EIOIS200_PNP_INDEX,	 .data = EIOIS200_PNP_DATA     },
> +	{ .idx = EIOIS200_SUB_PNP_INDEX, .data = EIOIS200_SUB_PNP_DATA },
> +};
> +
> +static struct eiois200_dev *eiois200_dev;
> +static struct regmap *regmap_is200;

Avoid globals at all (most, many ...) costs.

> +static struct mfd_cell susi_mfd_devs[] = {
> +	{ .name = "eiois200_wdt"     },
> +	{ .name = "gpio_eiois200"    },
> +	{ .name = "eiois200_hwmon"   },
> +	{ .name = "eiois200_i2c"     },
> +	{ .name = "eiois200_thermal" },
> +};
> +
> +struct regmap_range is200_range[] = {
> +	 regmap_reg_range(EIOIS200_PNP_INDEX,	  EIOIS200_PNP_DATA),
> +	 regmap_reg_range(EIOIS200_SUB_PNP_INDEX, EIOIS200_SUB_PNP_DATA),
> +	 regmap_reg_range(EIOIS200_PMC_PORT,	  EIOIS200_PMC_PORT + 0x0F),
> +	 regmap_reg_range(EIOIS200_PMC_PORT_SUB,  EIOIS200_PMC_PORT_SUB + 0x0F),
> +};
> +
> +static const struct regmap_access_table volatile_regs = {
> +	.yes_ranges   = is200_range,
> +	.n_yes_ranges = ARRAY_SIZE(is200_range),
> +};
> +
> +static const struct regmap_config pnp_regmap_config = {
> +	.name		= "eiois200-gpio",
> +	.reg_bits	= 16,
> +	.val_bits	= 8,
> +	.volatile_table = &volatile_regs,
> +	.io_port	= true,
> +};
> +
> +/* Following are EIO-IS200 pnp io port access functions */

"IO"

Probably "PNP" too.

> +static int is200_pnp_read(struct eiois200_dev_port *port, u8 idx)
> +{
> +	int val;
> +
> +	if (regmap_write(regmap_is200, port->idx, idx))
> +		pr_err("Error port write 0x%X\n", port->idx);

Why pr_err()?  You should have a 'dev' pointer.  So use dev_err().

> +	if (regmap_read(regmap_is200, port->data, &val))
> +		pr_err("Error port read 0x%X\n", port->data);
> +
> +	return val;
> +}
> +
> +static void is200_pnp_write(struct eiois200_dev_port *port, u8 idx, u8 data)
> +{
> +	if (regmap_write(regmap_is200, port->idx, idx) ||
> +	    regmap_write(regmap_is200, port->data, data))
> +		pr_err("Error port write 0x%X %X\n",
> +		       port->idx, port->data);
> +}
> +
> +static void is200_pnp_enter(struct eiois200_dev_port *port)
> +{
> +	if (regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER) ||
> +	    regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_ENTER))

How are these 2 calls any different to each other?

> +		pr_err("Error port write 0x%X\n", port->idx);

Return error?

> +}
> +
> +static void is200_pnp_leave(struct eiois200_dev_port *port)
> +{
> +	if (regmap_write(regmap_is200, port->idx, EIOIS200_EXT_MODE_EXIT))
> +		pr_err("Error port write 0x%X\n", port->idx);

Return error?

> +}
> +
> +/* Following are EIO-IS200 io port access functions for pmc command */
> +static int pmc_write_data(int id, u8 value, u16 timeout)
> +{
> +	int ret;
> +
> +	if (WAIT_IBF(id, timeout))
> +		return -ETIME;

What's happening here?

> +	ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].data, value);
> +	if (ret)
> +		pr_err("Error pmc write %X:%X\n",
> +		       eiois200_dev->pmc[id].data, value);
> +
> +	return ret;
> +}
> +
> +static int pmc_write_cmd(int id, u8 value, u16 timeout)
> +{
> +	int ret;
> +
> +	if (WAIT_IBF(id, timeout))
> +		return -ETIME;
> +
> +	ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].cmd, value);
> +	if (ret)
> +		pr_err("Error pmc write %X:%X\n",
> +		       eiois200_dev->pmc[id].data, value);
> +
> +	return ret;
> +}
> +
> +static int pmc_read_data(int id, u8 *value, u16 timeout)
> +{
> +	int val, ret;
> +
> +	if (WAIT_OBF(id, timeout))
> +		return -ETIME;
> +
> +	ret = regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val);
> +	if (ret)
> +		pr_err("Error pmc read %X\n", eiois200_dev->pmc[id].data);
> +	else
> +		*value = val & 0xFF;
> +
> +	return ret;
> +}
> +
> +static int pmc_read_status(int id)
> +{
> +	int val;
> +
> +	if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val)) {
> +		pr_err("Error pmc read %X\n", eiois200_dev->pmc[id].data);
> +		return 0;

You've just filed and yet you are returning success?

> +	}
> +
> +	return val;
> +}
> +
> +static void pmc_clear(int id)
> +{
> +	int val;
> +
> +	/* Check if input buffer blocked */
> +	if ((pmc_read_status(id) & EIOIS200_PMC_STATUS_IBF) == 0)
> +		return;
> +
> +	/* Read out previous garbage */
> +	if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val))
> +		pr_err("Error pmc clear\n");
> +
> +	usleep_range(10, 100);
> +}
> +
> +/**
> + * eiois200_core_pmc_wait - Wait for input / output buffer to be ready.
> + * @id:			0 for main chip, 1 for sub chip.
> + * @wait:		%PMC_WAIT_INPUT or %PMC_WAIT_OUTPUT.
> + *			%PMC_WAIT_INPUT for waiting input buffer data ready.
> + *			%PMC_WAIT_OUTPUT for waiting output buffer empty.
> + * max_duration:	The timeout value in usec.
> + */
> +int eiois200_core_pmc_wait(int id,
> +			   enum eiois200_pmc_wait wait,
> +			   uint max_duration)
> +{
> +	u32	cnt = 0;
> +	uint	val;
> +	int	ret;
> +	int	new_timeout = max_duration ? max_duration : timeout;
> +	ktime_t time_end    = ktime_add_us(ktime_get(), new_timeout);

Remove all of this type of lining up.

> +
> +	if (new_timeout > TIMEOUT_MAX ||
> +	    new_timeout < TIMEOUT_MIN) {

Why the '\n' here?

> +		pr_err("Error timeout value: %dus\nTimeout value should between %d and %d\n",

Avoid line breaks in error messages.

> +		       new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
> +		return -ETIME;
> +	}
> +
> +	do {
> +		ret = regmap_read(regmap_is200,
> +				  eiois200_dev->pmc[id].status,
> +				  &val);
> +		if (ret)
> +			return ret;
> +
> +		if (wait == PMC_WAIT_INPUT) {
> +			if ((val & EIOIS200_PMC_STATUS_IBF) == 0)
> +				return 0;
> +		} else {
> +			if ((val & EIOIS200_PMC_STATUS_OBF) != 0)
> +				return 0;
> +		}
> +
> +		/* Incremental delay */
> +		fsleep(cnt++ * 10);
> +
> +	} while (ktime_before(ktime_get(), time_end));
> +
> +	return -ETIME;
> +}
> +EXPORT_SYMBOL_GPL(eiois200_core_pmc_wait);
> +
> +/**
> + * eiois200_core_pmc_operation - Execute a pmc command

"PMC"

> + * @op:		Pointer to an pmc command.
> + */
> +int eiois200_core_pmc_operation(struct _pmc_op *op)
> +{
> +	u8 	i;
> +	int	ret;
> +	bool	read_cmd = op->cmd & 0x01;
> +	ktime_t t = ktime_get();
> +
> +	mutex_lock(&eiois200_dev->mutex);
> +
> +	pmc_clear(op->chip);
> +
> +	ret = pmc_write_cmd(op->chip, op->cmd, op->timeout);
> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_write_data(op->chip, op->control, op->timeout);
> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_write_data(op->chip, op->device_id, op->timeout);
> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_write_data(op->chip, op->size, op->timeout);
> +	if (ret)
> +		goto err;
> +
> +	for (i = 0 ; i < op->size ; i++) {

Remove the spaces before the ';'s.

> +		if (read_cmd)
> +			ret = pmc_read_data(op->chip, &op->payload[i], op->timeout);
> +		else
> +			ret = pmc_write_data(op->chip, op->payload[i], op->timeout);
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +	mutex_unlock(&eiois200_dev->mutex);
> +
> +	return 0;
> +
> +err:
> +	mutex_unlock(&eiois200_dev->mutex);
> +
> +	pr_err("PMC error duration:%lldus\n",
> +	       ktime_to_us(ktime_sub(ktime_get(), t)));

Why the line wrap.

> +	pr_err(".cmd=0x%02X, .ctrl=0x%02X .id=0x%02X, .size=0x%02X .data=0x%02X%02X\n",
> +	       op->cmd, op->control, op->device_id,
> +	       op->size, op->payload[0], op->payload[1]);

Are these prints actually useful?

I'd suggest that they are debug prints at best.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(eiois200_core_pmc_operation);
> +
> +static int get_pmc_port(struct device *dev, int id, struct eiois200_dev_port *port)
> +{
> +	struct _pmc_port *pmc = &eiois200_dev->pmc[id];

What's with the '_'?

This usually means that it's internal and should not be used.

> +	is200_pnp_enter(port);
> +
> +	/* Switch to PMC device page */
> +	is200_pnp_write(port, EIOIS200_LDN, EIOIS200_LDN_PMC1);
> +
> +	/* Active this device */
> +	is200_pnp_write(port, EIOIS200_LDAR, EIOIS200_LDAR_LDACT);
> +
> +	/* Get PMC cmd and data port */
> +	pmc->data  = is200_pnp_read(port, EIOIS200_IOBA0H) << 8;
> +	pmc->data |= is200_pnp_read(port, EIOIS200_IOBA0L);
> +	pmc->cmd   = is200_pnp_read(port, EIOIS200_IOBA1H) << 8;
> +	pmc->cmd  |= is200_pnp_read(port, EIOIS200_IOBA1L);
> +
> +	/* Disable IRQ */
> +	is200_pnp_write(port, EIOIS200_IRQCTRL, 0);
> +
> +	is200_pnp_leave(port);
> +
> +	/* Make sure IO ports are not occupied */
> +	if (!devm_request_region(dev, pmc->data, 2, KBUILD_MODNAME)) {
> +		dev_err(dev, "Request region %X error\n", pmc->data);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int eiois200_exist(struct device *dev)
> +{
> +	u16  chip_id = 0;
> +	u8   tmp = 0;
> +	int  i = 0;
> +	int  ret = -ENOMEM;
> +	char chip[][8] = { "First",  "Second" };
> +
> +	for (i = 0 ; i < ARRAY_SIZE(pnp_port) ; i++) {

Should 'i' be 'port' here?

> +		struct eiois200_dev_port *port = pnp_port + i;
> +
> +		if (!devm_request_region(dev,
> +					 pnp_port[i].idx,
> +					 2,

What's 2?  This should probably be defined.

> +					 KBUILD_MODNAME))
> +			continue;
> +
> +		is200_pnp_enter(port);
> +
> +		chip_id  = is200_pnp_read(port, EIOIS200_CHIPID1) << 8;
> +		chip_id |= is200_pnp_read(port, EIOIS200_CHIPID2);
> +
> +		if (chip_id != EIOIS200_CHIPID &&
> +		    chip_id != EIO201_211_CHIPID)
> +			continue;
> +
> +		/* Turn on the enable flag */
> +		tmp = is200_pnp_read(port, EIOIS200_SIOCTRL);
> +		tmp |= EIOIS200_SIOCTRL_SIOEN;
> +		is200_pnp_write(port, EIOIS200_SIOCTRL, tmp);
> +
> +		is200_pnp_leave(port);
> +
> +		ret = get_pmc_port(dev, i, port);
> +		if (ret)
> +			return ret;
> +
> +		eiois200_dev->flag |= i == 0 ?  EIOIS200_F_CHIP_EXIST :

This logic either needs improving or commented on.

> +						EIOIS200_F_SUB_CHIP_EXIST;
> +
> +		pr_info("%s chip detected: %04X\n", chip[i], chip_id);

We can almost certainly do without these in production code.

> +	}
> +
> +	return ret;
> +}
> +
> +/* read information about acpi stored in EC */

Please use prober grammar.

"Read ACPI information stored in the EC"

> +static uint8_t acpiram_access(uint8_t offset)
> +{
> +	u8  val;
> +	int ret;
> +
> +	mutex_lock(&eiois200_dev->mutex);
> +
> +	pmc_clear(0);
> +
> +	ret = pmc_write_cmd(0, EIOIS200_PMC_CMD_ACPIRAM_READ, 0);

All of the magic numbers in this function need to be defined.

> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_write_data(0, offset, 0);
> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_write_data(0, 1, 0);
> +	if (ret)
> +		goto err;
> +
> +	ret = pmc_read_data(0, &val, 0);
> +	if (ret)
> +		goto err;
> +
> +err:
> +	mutex_unlock(&eiois200_dev->mutex);
> +	return ret ? 0 : val;

If it fails, you return success?

> +}
> +
> +static int firmware_code_base(struct device *dev)
> +{
> +	u8 ic_vendor, ic_code, code_base;
> +
> +	ic_vendor = acpiram_access(EIOIS200_ACPIRAM_ICVENDOR);
> +	ic_code   = acpiram_access(EIOIS200_ACPIRAM_ICCODE);
> +	code_base = acpiram_access(EIOIS200_ACPIRAM_CODEBASE);
> +
> +	if (ic_vendor != 'R')
> +		return -ENODEV;
> +
> +	if (ic_code != EIOIS200_ICCODE &&
> +	    ic_code != EIO201_ICCODE   &&
> +	    ic_code != EIO211_ICCODE)
> +		goto err;
> +
> +	if (code_base == 0x80) {

No magic numbers please.

> +		eiois200_dev->flag |= EIOIS200_F_NEW_CODE_BASE;
> +		return 0;
> +	}
> +
> +	if (code_base == 0 && (ic_code != EIO201_ICCODE &&
> +			       ic_code != EIO211_ICCODE)) {
> +		pr_info("Old code base not supported, yet.");
> +		return -ENODEV;
> +	}
> +
> + err:
> +	dev_err(dev,
> +		"Codebase check fail:\n"
> +		"ic_vendor: 0x%X  ,ic_code : 0x%X ,code_base : 0x%X\n",
> +		ic_vendor, ic_code, code_base);

These are really ugly prints.

Also, they should not contain new lines.

> +	return -ENODEV;
> +}
> +
> +int eiois200_probe(struct device *dev, unsigned int id)

.probe() usually goes near the bottom.

> +{
> +	int  ret = 0;
> +	void __iomem *iomem;

Swap these over.

> +	iomem = devm_ioport_map(dev, 0, EIOIS200_SUB_PNP_DATA + 1);
> +	if (!iomem)
> +		return -ENOMEM;
> +
> +	regmap_is200 = devm_regmap_init_mmio(dev, iomem, &pnp_regmap_config);
> +	if (!regmap_is200)
> +		return -ENOMEM;
> +
> +	eiois200_dev = devm_kzalloc(dev,
> +				    sizeof(struct eiois200_dev),

sizeof(*eiois200_dev)

> +				    GFP_KERNEL);
> +	if (!eiois200_dev)
> +		return -ENOMEM;
> +
> +	mutex_init(&eiois200_dev->mutex);
> +
> +	if (eiois200_exist(dev))
> +		return -ENODEV;
> +
> +	if (firmware_code_base(dev)) {
> +		dev_err(dev, "Chip code base check fail\n");
> +		return -EIO;
> +	}
> +
> +	dev_set_drvdata(dev, eiois200_dev);

What data have you stored in eiois200_dev?

> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, susi_mfd_devs,
> +				   ARRAY_SIZE(susi_mfd_devs),
> +				   NULL, 0, NULL);
> +	if (ret)
> +		dev_err(dev, "Cannot register child devices (error = %d)\n", ret);
> +
> +	pr_info("%s started", KBUILD_MODNAME);

These prints are not useful, please remove them.

> +
> +	return 0;
> +}
> +
> +struct {
> +	char name[32];
> +	int  cmd;
> +	int  ctrl;
> +	int  size;
> +} attrs[] = {
> +	{ "board_name",		0x53, 0x10, 16 },
> +	{ "board_serial",	0x53, 0x1F, 16 },
> +	{ "board_manufacturer", 0x53, 0x11, 16 },
> +	{ "board_id",		0x53, 0x1E,  4 },
> +	{ "firmware_version",	0x53, 0x22, 16 },
> +	{ "firmware_build",	0x53, 0x23, 26 },
> +	{ "firmware_date",	0x53, 0x24, 16 },
> +	{ "chip_id",		0x53, 0x12, 12 },
> +	{ "chip_detect",	0x53, 0x15, 12 },
> +	{ "platform_type",	0x53, 0x13, 16 },
> +	{ "platform_revision",	0x53, 0x14,  4 },
> +	{ "eapi_version",	0x53, 0x30,  4 },
> +	{ "eapi_id",		0x53, 0x31,  4 },
> +	{ "boot_count",		0x55, 0x10,  4 },
> +	{ "powerup_hour",	0x55, 0x11,  4 },
> +};
> +
> +static ssize_t info_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)

SYSFS handling usually goes near the top.

> +{
> +	uint i;
> +
> +	for (i = 0 ; i < ARRAY_SIZE(attrs) ; i++)
> +		if (!strcmp(attr->attr.name, attrs[i].name)) {
> +			int ret;
> +			char str[32] = "";
> +			struct _pmc_op op = {
> +				.cmd     = attrs[i].cmd,
> +				.control = attrs[i].ctrl,
> +				.payload = (u8 *)str,
> +				.size    = attrs[i].size,
> +			};
> +
> +			ret = eiois200_core_pmc_operation(&op);
> +			if (ret)
> +				return ret;
> +
> +			if (attrs[i].size == 4)
> +				return sysfs_emit(buf, "%X\n", *(u32 *)str);
> +			else
> +				return sysfs_emit(buf, "%s\n", str);
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static DEVICE_ATTR(board_name,		0444, info_show, NULL);
> +static DEVICE_ATTR(board_serial,	0444, info_show, NULL);
> +static DEVICE_ATTR(board_manufacturer,	0444, info_show, NULL);
> +static DEVICE_ATTR(firmware_version,	0444, info_show, NULL);
> +static DEVICE_ATTR(firmware_build,	0444, info_show, NULL);
> +static DEVICE_ATTR(firmware_date,	0444, info_show, NULL);
> +static DEVICE_ATTR(chip_id,		0444, info_show, NULL);
> +static DEVICE_ATTR(chip_detect,		0444, info_show, NULL);
> +static DEVICE_ATTR(platform_type,	0444, info_show, NULL);
> +static DEVICE_ATTR(platform_revision,	0444, info_show, NULL);
> +static DEVICE_ATTR(board_id,		0444, info_show, NULL);
> +static DEVICE_ATTR(eapi_version,	0444, info_show, NULL);
> +static DEVICE_ATTR(eapi_id,		0444, info_show, NULL);
> +static DEVICE_ATTR(boot_count,		0444, info_show, NULL);
> +static DEVICE_ATTR(powerup_hour,	0444, info_show, NULL);
> +
> +static struct attribute *pmc_attrs[] = {
> +	&dev_attr_board_name.attr,
> +	&dev_attr_board_serial.attr,
> +	&dev_attr_board_manufacturer.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,
> +	NULL
> +};
> +
> +static const struct attribute_group attr_group = {
> +	.attrs = pmc_attrs,
> +};
> +
> +static const struct attribute_group *attr_groups[] = {
> +	&attr_group,
> +	NULL
> +};

Pretty sure there are helper MACROS for this.

> +static struct isa_driver eiois200_driver = {
> +	.probe    = eiois200_probe,
> +
> +	.driver = {
> +		.owner = THIS_MODULE,

Are you sure the class doesn't do this for you?

> +		.name  = KBUILD_MODNAME,

Use a proper string.

> +		.dev_groups = attr_groups,
> +	},
> +};
> +
> +module_isa_driver(eiois200_driver, 1);
> +
> +MODULE_AUTHOR("susi.driver <susi.driver@advantech.com.tw>");

This does not match the credentials above.

> +MODULE_DESCRIPTION("Advantech EIO-IS200 series EC core driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 4/4] mfd: eiois200: Add EIO-IS200 Series EC Core Driver

[Wenkai]

Hi Jones,

Great news! My patch is finally on lore. Now I can track its progress.
It's been quite a journey.

The EIOIS200 is a custom EC for Advantech, originally designed the
Power Management Channel (PMC) only for power management. We extended
it to control all devices on the EC. This EC is a quite slow MCU, but
still monitors crucial parameters like voltage, temperature, and smart
fan, the PMC commands has varying response times, but it has proved
highly stable in industrial usage.

So, we recommend not loading the modules if the users doesn't intend to
use these devices. Load these modules only before accessing the devices.
This is why we've designed it to be compiled as a module by default and
cannot be built-in.

I've made modifications based on your suggestions. Please point out any
further errors. Thank you.

Lee Jones 於 9/20/2023 7:13 PM 寫道:
 >
 > On Thu, 07 Sep 2023, advantech.susiteam@gmail.com wrote:
 >
 > > From: Wenkai <advantech.susiteam@gmail.com>
 > >
 > > This patch introduces the Advantech EIO-IS200 Series EC Core Driver.
 > > The EIO-IS200 combines hardware functionality with firmware
 > > capabilities to provide key features through a dedicated Power
 > > Management Channel (PMC).
 > >
 > > Key Features:
 > > - Implements the core EIO-IS200 IC driver that serves as the
 > >   interface to the IC's firmware and hardware functions.
 > > - Contains low-level functions for accessing the IC's Power
 > >   Management Channel (PMC).
 > > - Manages PMC command execution, PMC buffer handling, and
 > >   communication.
 > > - Expose a regmap and a mutex for low-level access.
 > > - Activates support for GPIO, I2C, Hwmon, and Watchdog functionalities
 > >   of the EIO-IS200 IC.
 > > - Provides various sysfs attributes to expose information about the
 > >   EC chip, firmware, and motherboard.
 > > - Includes a timeout parameter to allow modification of the default
 > >   PMC command timeout value, which is particularly useful when
 > >   dealing with an extremely slow-responding device.
 > >
 > > Signed-off-by: Wenkai <advantech.susiteam@gmail.com>
 > > ---
 > >  drivers/mfd/eiois200_core.c | 590 ++++++++++++++++++++++++++++++++++++
 >
 > Please organise patches by functionality, not file.
 >
 > Each of the patches in this series are worthless without the others.

I understand.
But, I am currently unsure of how to organize these patches. But I'll
give it a try.

 >
 > Have you run static analysis on this?
 >
 > What about checkpatch.pl?

Those already passed the checkpatch.pl checks.

I just used "sparse" for static code analysis and received two warnings
you had pointed out below.

 >
 > >  1 file changed, 590 insertions(+)
 > >  create mode 100644 drivers/mfd/eiois200_core.c
 > >
 > > diff --git a/drivers/mfd/eiois200_core.c b/drivers/mfd/eiois200_core.c
 > > new file mode 100644
 > > index 000000000000..7459dee1ed53
 > > --- /dev/null
 > > +++ b/drivers/mfd/eiois200_core.c
 > > @@ -0,0 +1,590 @@
 > > +// SPDX-License-Identifier: GPL-2.0-only
 > > +/*
 > > + * Advantech EIO-IS200 Series EC base Driver
 > > + *
 > > + * This driver provides an interface to access the EIO-IS200 Series EC
 > > + * firmware via its own Power Management Channel (PMC) for subdrivers:
 > > + *
 > > + * - Watchdog: drivers/watchdog/eiois200_wdt
 > > + * - GPIO: drivers/gpio/gpio_eiois200
 > > + * - Hwmon: drivers/hwmon/eiois200_hwmon
 > > + * - I2C: drivers/i2c/busses/i2c_eiois200
 > > + * - Thermal: drivers/thermal/eiois200_thermal
 >
 > None of these exist.  I would remove these completely. They will be
 > highly susceptible to bit-rot.

Understand. I'll submit them on sub-drivers upstream one by one. Is that 
correct?

 >
 > > + * A system may have one or two independent EIO-IS200s.
 > > + *
 > > + * Copyright (C) 2023 Advantech Co., Ltd.
 > > + * Author: wenkai.chung <wenkai.chung@advantech.com.tw>
 >
 > Is the name on your government ID "wenkai.chung"?

No, my government ID name is 'WEN-KAI CHUNG', 'wenkai.chung' is my
Advantech ID.

Should I use 'Wenkai <advantech.susiteam@gmail.com', or are there other
rules I should follow?

 > > + */
 > > +
 > > +#include <linux/delay.h>
 > > +#include <linux/isa.h>
 > > +#include <linux/mfd/core.h>
 > > +#include <linux/mutex.h>
 > > +#include <linux/time.h>
 > > +#include <linux/uaccess.h>
 > > +
 > > +#include "eiois200.h"
 > > +
 > > +#define TIMEOUT_MAX  (10 * 1000 * 1000)
 >
 > There are helpers to convert between time bases.  Please use them.

Did you mean I should write like this?

     #define TIMEOUT_MAX     (10 * USEC_PER_SEC)

 >
 > Where do these values come from?

Typically, the response time for PMC commands falls within the range of
200 microseconds to 2 milliseconds, with occasional commands requiring
up to 5 milliseconds. However, when the EC (Embedded Controller)
accesses external, exceptionally slow devices, the response time can
approach one second.

To account for this variability, we have defined a valid timeout range
of 200 microseconds to 10 seconds for PMC commands. To date, we have not
encountered any applications that necessitate timeouts beyond this
range. Any PMC command's timeout value exceeding this range will
immediate return error without executing the PMC command.

 >
 > > +#define TIMEOUT_MIN  200
 > > +
 > > +static uint timeout = 5000;
 >
 > Why 5000?

I think you mentioned that the code needs more explanation.
I modified the code below. Is that OK?

     #define TIMEOUT_MAX     (10 * USEC_PER_SEC)
     #define TIMEOUT_MIN    200
     #define DEFAULT_TIMEOUT 5000

     /**
     * 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
     * In some cases, responses are longer. In such situations, please
     * adding the timeout parameter loading related sub-drivers or
     * this core driver (not recommended).
     */
     static uint timeout = DEFAULT_TIMEOUT;
     module_param(timeout, uint, 0444);
     MODULE_PARM_DESC(timeout,
              "Default PMC command timeout in usec.\n");

 >
 > > +module_param(timeout, uint, 0664);
 > > +MODULE_PARM_DESC(timeout,
 > > +              "Default PMC command timeout in usec.\n");
 >
 > How useful is it to override this on the commandline, really?

You are right. I modified it as below:

     static uint timeout = DEFAULT_TIMEOUT;
     module_param(timeout, uint, 0444);
     MODULE_PARM_DESC(timeout,
              "Default PMC command timeout in usec.\n");

 >
 > > +struct eiois200_dev_port {
 > > +     u16 idx;
 > > +     u16 data;
 >
 > 2 of the worst variable names of all time.
 >
 > Please ensure variable names describe their purpose.

Alright, I'll make the following changes. Is that okay?

     struct eiois200_dev_port {
         u16 idx_port;
         u16 data_port;
     };

 >
 > > +};
 > > +
 > > +struct eiois200_dev_port pnp_port[] = {
 > > +     { .idx = EIOIS200_PNP_INDEX,     .data = EIOIS200_PNP_DATA     },
 > > +     { .idx = EIOIS200_SUB_PNP_INDEX, .data = EIOIS200_SUB_PNP_DATA },
 > > +};
 > > +
 > > +static struct eiois200_dev *eiois200_dev;
 > > +static struct regmap *regmap_is200;
 >
 > Avoid globals at all (most, many ...) costs.

I am sorry about that. I'll be more cautious.

 >
 > > +static struct mfd_cell susi_mfd_devs[] = {
 > > +     { .name = "eiois200_wdt"     },
 > > +     { .name = "gpio_eiois200"    },
 > > +     { .name = "eiois200_hwmon"   },
 > > +     { .name = "eiois200_i2c"     },
 > > +     { .name = "eiois200_thermal" },
 > > +};

Should I remove these, too?

 > > +
 > > +struct regmap_range is200_range[] = {
 > > +      regmap_reg_range(EIOIS200_PNP_INDEX, EIOIS200_PNP_DATA),
 > > +      regmap_reg_range(EIOIS200_SUB_PNP_INDEX, EIOIS200_SUB_PNP_DATA),
 > > +      regmap_reg_range(EIOIS200_PMC_PORT,  EIOIS200_PMC_PORT + 0x0F),
 > > +      regmap_reg_range(EIOIS200_PMC_PORT_SUB, 
  EIOIS200_PMC_PORT_SUB + 0x0F),
 > > +};
 > > +
 > > +static const struct regmap_access_table volatile_regs = {
 > > +     .yes_ranges   = is200_range,
 > > +     .n_yes_ranges = ARRAY_SIZE(is200_range),
 > > +};
 > > +
 > > +static const struct regmap_config pnp_regmap_config = {
 > > +     .name           = "eiois200-gpio",
 > > +     .reg_bits       = 16,
 > > +     .val_bits       = 8,
 > > +     .volatile_table = &volatile_regs,
 > > +     .io_port        = true,
 > > +};
 > > +
 > > +/* Following are EIO-IS200 pnp io port access functions */
 >
 > "IO"
 >
 > Probably "PNP" too.

OK! I'll fix this and others below.

 >
 > > +static int is200_pnp_read(struct eiois200_dev_port *port, u8 idx)
 > > +{
 > > +     int val;
 > > +
 > > +     if (regmap_write(regmap_is200, port->idx, idx))
 > > +             pr_err("Error port write 0x%X\n", port->idx);
 >
 > Why pr_err()?  You should have a 'dev' pointer.  So use dev_err().

OK! I'll fix all others, too.

And I'll remove the definition on 'eiois200.h'.
This definition makes pr_xxx messages look like dev_xxx.

     #ifdef pr_fmt
     #undef pr_fmt
     #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
     #endif

 >
 > > +     if (regmap_read(regmap_is200, port->data, &val))
 > > +             pr_err("Error port read 0x%X\n", port->data);
 > > +
 > > +     return val;
 > > +}
 > > +
 > > +static void is200_pnp_write(struct eiois200_dev_port *port, u8 
idx, u8 data)
 > > +{
 > > +     if (regmap_write(regmap_is200, port->idx, idx) ||
 > > +         regmap_write(regmap_is200, port->data, data))
 > > +             pr_err("Error port write 0x%X %X\n",
 > > +                    port->idx, port->data);
 > > +}
 > > +
 > > +static void is200_pnp_enter(struct eiois200_dev_port *port)
 > > +{
 > > +     if (regmap_write(regmap_is200, port->idx, 
EIOIS200_EXT_MODE_ENTER) ||
 > > +         regmap_write(regmap_is200, port->idx, 
EIOIS200_EXT_MODE_ENTER))
 >
 > How are these 2 calls any different to each other?

Hardware specifications require two consecutive 0x87 commands to unlock it.
Did you mean I should add comments like below?

     static void is200_pnp_enter(struct eiois200_dev_port *port)
     {
         /* Write 0x87 to index port twice to unlock IO port */
         if (regmap_write(regmap_is200, port->idx_port, 
EIOIS200_EXT_MODE_ENTER) ||
             regmap_write(regmap_is200, port->idx_port, 
EIOIS200_EXT_MODE_ENTER))
             pr_err("Error port write 0x%X\n", port->idx_port);
     }

     static void is200_pnp_leave(struct eiois200_dev_port *port)
     {
         /* Write 0xAA to index port once to lock IO port */
         if (regmap_write(regmap_is200, port->idx_port, 
EIOIS200_EXT_MODE_EXIT))
             pr_err("Error port write 0x%X\n", port->idx_port);
     }

 >
 > > +             pr_err("Error port write 0x%X\n", port->idx);
 >
 > Return error?

No, it doesn't return an error.
If an error occurs, it won't retrieve the correct content.
Which can result in a failed initialization.
This was tested locally, and it made the probe procedure stop.

 >
 > > +}
 > > +
 > > +static void is200_pnp_leave(struct eiois200_dev_port *port)
 > > +{
 > > +     if (regmap_write(regmap_is200, port->idx, 
EIOIS200_EXT_MODE_EXIT))
 > > +             pr_err("Error port write 0x%X\n", port->idx);
 >
 > Return error?

Like above. No return error.

 >
 > > +}
 > > +
 > > +/* Following are EIO-IS200 io port access functions for pmc command */
 > > +static int pmc_write_data(int id, u8 value, u16 timeout)
 > > +{
 > > +     int ret;
 > > +
 > > +     if (WAIT_IBF(id, timeout))
 > > +             return -ETIME;
 >
 > What's happening here?

As the definition on eiois200.h I separately patched.

     enum eiois200_pmc_wait {
         PMC_WAIT_INPUT,
         PMC_WAIT_OUTPUT,
     };

     /**
      * eiois200_core_pmc_wait - Wait for input / output buffer to be ready
      * @id:        0 for main chip, 1 for sub chip.
      * @wait:    %PMC_WAIT_INPUT or %PMC_WAIT_OUTPUT.
      *        %PMC_WAIT_INPUT for waiting input buffer data ready.
      *        %PMC_WAIT_OUTPUT for waiting output buffer empty.
      * @timeout:    The timeout value. 0 means use the default value.
      */
     int eiois200_core_pmc_wait(int id, enum eiois200_pmc_wait wait, 
uint timeout);

     #define WAIT_IBF(id, timeout)    eiois200_core_pmc_wait(id, 
PMC_WAIT_INPUT, timeout)
     #define WAIT_OBF(id, timeout)    eiois200_core_pmc_wait(id, 
PMC_WAIT_OUTPUT, timeout)

It waits for the buffer to be ready.
Or, do you mean that I should add some comments?

     /* Wait til the input port clear for write */
     if (WAIT_IBF(id, timeout))
         return -ETIME;

     /* Write one byte of data to the port */
     ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].data, value);
     if (ret)
         pr_err("Error PMC write %X:%X\n",
                eiois200_dev->pmc[id].data, value);

 >
 > > +     ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].data, 
value);
 > > +     if (ret)
 > > +             pr_err("Error pmc write %X:%X\n",
 > > +                    eiois200_dev->pmc[id].data, value);
 > > +
 > > +     return ret;
 > > +}
 > > +
 > > +static int pmc_write_cmd(int id, u8 value, u16 timeout)
 > > +{
 > > +     int ret;
 > > +
 > > +     if (WAIT_IBF(id, timeout))
 > > +             return -ETIME;
 > > +
 > > +     ret = regmap_write(regmap_is200, eiois200_dev->pmc[id].cmd, 
value);
 > > +     if (ret)
 > > +             pr_err("Error pmc write %X:%X\n",
 > > +                    eiois200_dev->pmc[id].data, value);
 > > +
 > > +     return ret;
 > > +}
 > > +
 > > +static int pmc_read_data(int id, u8 *value, u16 timeout)
 > > +{
 > > +     int val, ret;
 > > +
 > > +     if (WAIT_OBF(id, timeout))
 > > +             return -ETIME;
 > > +
 > > +     ret = regmap_read(regmap_is200, eiois200_dev->pmc[id].data, 
&val);
 > > +     if (ret)
 > > +             pr_err("Error pmc read %X\n", 
eiois200_dev->pmc[id].data);
 > > +     else
 > > +             *value = val & 0xFF;
 > > +
 > > +     return ret;
 > > +}
 > > +
 > > +static int pmc_read_status(int id)
 > > +{
 > > +     int val;
 > > +
 > > +     if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, 
&val)) {
 > > +             pr_err("Error pmc read %X\n", 
eiois200_dev->pmc[id].data);
 > > +             return 0;
 >
 > You've just filed and yet you are returning success?

This function returns 0 on error. I'll add comments as below:

     /**
      * pmc_read_status - Read the PMC's current status
      * @id:        0 for main chip, 1 for sub chip.
      * Returns:    The current status read from the PMC, or return 0 on 
error.
     */

 >
 > > +     }
 > > +
 > > +     return val;
 > > +}
 > > +
 > > +static void pmc_clear(int id)
 > > +{
 > > +     int val;
 > > +
 > > +     /* Check if input buffer blocked */
 > > +     if ((pmc_read_status(id) & EIOIS200_PMC_STATUS_IBF) == 0)
 > > +             return;
 > > +
 > > +     /* Read out previous garbage */
 > > +     if (regmap_read(regmap_is200, eiois200_dev->pmc[id].data, &val))
 > > +             pr_err("Error pmc clear\n");
 > > +
 > > +     usleep_range(10, 100);
 > > +}
 > > +
 > > +/**
 > > + * eiois200_core_pmc_wait - Wait for input / output buffer to be 
ready.
 > > + * @id:                      0 for main chip, 1 for sub chip.
 > > + * @wait:            %PMC_WAIT_INPUT or %PMC_WAIT_OUTPUT.
 > > + *                   %PMC_WAIT_INPUT for waiting input buffer data 
ready.
 > > + *                   %PMC_WAIT_OUTPUT for waiting output buffer empty.
 > > + * max_duration:     The timeout value in usec.
 > > + */
 > > +int eiois200_core_pmc_wait(int id,
 > > +                        enum eiois200_pmc_wait wait,
 > > +                        uint max_duration)
 > > +{
 > > +     u32     cnt = 0;
 > > +     uint    val;
 > > +     int     ret;
 > > +     int     new_timeout = max_duration ? max_duration : timeout;
 > > +     ktime_t time_end    = ktime_add_us(ktime_get(), new_timeout);
 >
 > Remove all of this type of lining up.

Got it, Changes it as below:

     int ret;
     uint val;
     u32 cnt = 0;
     int new_timeout = max_duration ? max_duration : timeout;
     ktime_t time_end = ktime_add_us(ktime_get(), new_timeout);

 >
 > > +
 > > +     if (new_timeout > TIMEOUT_MAX ||
 > > +         new_timeout < TIMEOUT_MIN) {
 >
 > Why the '\n' here?

Got it, I'll change it as below:

     if (new_timeout < TIMEOUT_MIN || new_timeout > TIMEOUT_MAX) {

 >
 > > +             pr_err("Error timeout value: %dus\nTimeout value 
should between %d and %d\n",
 >
 > Avoid line breaks in error messages.

Like above. I removed '\n'.

     if (new_timeout < TIMEOUT_MIN || new_timeout > TIMEOUT_MAX) {
         dev_err(dev,
             "Error timeout value: %dus. Timeout value should between %d 
and %ld\n",
             new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
         return -ETIME;
     }

 >
 > > +                    new_timeout, TIMEOUT_MIN, TIMEOUT_MAX);
 > > +             return -ETIME;
 > > +     }
 > > +
 > > +     do {
 > > +             ret = regmap_read(regmap_is200,
 > > + eiois200_dev->pmc[id].status,
 > > +                               &val);
 > > +             if (ret)
 > > +                     return ret;
 > > +
 > > +             if (wait == PMC_WAIT_INPUT) {
 > > +                     if ((val & EIOIS200_PMC_STATUS_IBF) == 0)
 > > +                             return 0;
 > > +             } else {
 > > +                     if ((val & EIOIS200_PMC_STATUS_OBF) != 0)
 > > +                             return 0;
 > > +             }
 > > +
 > > +             /* Incremental delay */
 > > +             fsleep(cnt++ * 10);
 > > +
 > > +     } while (ktime_before(ktime_get(), time_end));
 > > +
 > > +     return -ETIME;
 > > +}
 > > +EXPORT_SYMBOL_GPL(eiois200_core_pmc_wait);
 > > +
 > > +/**
 > > + * eiois200_core_pmc_operation - Execute a pmc command
 >
 > "PMC"

Okay, I'll fix this and all others.

 >
 > > + * @op:              Pointer to an pmc command.
 > > + */
 > > +int eiois200_core_pmc_operation(struct _pmc_op *op)
 > > +{
 > > +     u8      i;
 > > +     int     ret;
 > > +     bool    read_cmd = op->cmd & 0x01;
 > > +     ktime_t t = ktime_get();
 > > +
 > > +     mutex_lock(&eiois200_dev->mutex);
 > > +
 > > +     pmc_clear(op->chip);
 > > +
 > > +     ret = pmc_write_cmd(op->chip, op->cmd, op->timeout);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_write_data(op->chip, op->control, op->timeout);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_write_data(op->chip, op->device_id, op->timeout);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_write_data(op->chip, op->size, op->timeout);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     for (i = 0 ; i < op->size ; i++) {
 >
 > Remove the spaces before the ';'s.

Okay, I'll fix this and all others, too.

 >
 > > +             if (read_cmd)
 > > +                     ret = pmc_read_data(op->chip, 
&op->payload[i], op->timeout);
 > > +             else
 > > +                     ret = pmc_write_data(op->chip, 
op->payload[i], op->timeout);
 > > +
 > > +             if (ret)
 > > +                     goto err;
 > > +     }
 > > +
 > > +     mutex_unlock(&eiois200_dev->mutex);
 > > +
 > > +     return 0;
 > > +
 > > +err:
 > > +     mutex_unlock(&eiois200_dev->mutex);
 > > +
 > > +     pr_err("PMC error duration:%lldus\n",
 > > +            ktime_to_us(ktime_sub(ktime_get(), t)));
 >
 > Why the line wrap.

Did you mean that as below?

     pr_err("PMC error duration:%lldus", 
ktime_to_us(ktime_sub(ktime_get(), t)));

 >
 > > +     pr_err(".cmd=0x%02X, .ctrl=0x%02X .id=0x%02X, .size=0x%02X 
.data=0x%02X%02X\n",
 > > +            op->cmd, op->control, op->device_id,
 > > +            op->size, op->payload[0], op->payload[1]);
 >
 > Are these prints actually useful?
 >
 > I'd suggest that they are debug prints at best.

Yes, I believe this information is helpful.
But, I hope this error message will never be shown on production release.

PMC commands should always succeed,
and their failure indicates a significant issue that requires attention.
The most common reason for failure is insufficient timeout value,
but it could also signify more severe underlying problems.

 >
 > > +     return ret;
 > > +}
 > > +EXPORT_SYMBOL_GPL(eiois200_core_pmc_operation);
 > > +
 > > +static int get_pmc_port(struct device *dev, int id, struct 
eiois200_dev_port *port)
 > > +{
 > > +     struct _pmc_port *pmc = &eiois200_dev->pmc[id];
 >
 > What's with the '_'?
 >
 > This usually means that it's internal and should not be used.

You are right. I'll fix all others, include the header, and the sub-drivers.

 >
 > > +     is200_pnp_enter(port);
 > > +
 > > +     /* Switch to PMC device page */
 > > +     is200_pnp_write(port, EIOIS200_LDN, EIOIS200_LDN_PMC1);
 > > +
 > > +     /* Active this device */
 > > +     is200_pnp_write(port, EIOIS200_LDAR, EIOIS200_LDAR_LDACT);
 > > +
 > > +     /* Get PMC cmd and data port */
 > > +     pmc->data  = is200_pnp_read(port, EIOIS200_IOBA0H) << 8;
 > > +     pmc->data |= is200_pnp_read(port, EIOIS200_IOBA0L);
 > > +     pmc->cmd   = is200_pnp_read(port, EIOIS200_IOBA1H) << 8;
 > > +     pmc->cmd  |= is200_pnp_read(port, EIOIS200_IOBA1L);
 > > +
 > > +     /* Disable IRQ */
 > > +     is200_pnp_write(port, EIOIS200_IRQCTRL, 0);
 > > +
 > > +     is200_pnp_leave(port);
 > > +
 > > +     /* Make sure IO ports are not occupied */
 > > +     if (!devm_request_region(dev, pmc->data, 2, KBUILD_MODNAME)) {
 > > +             dev_err(dev, "Request region %X error\n", pmc->data);
 > > +             return -EBUSY;
 > > +     }
 > > +
 > > +     return 0;
 > > +}
 > > +
 > > +static int eiois200_exist(struct device *dev)
 > > +{
 > > +     u16  chip_id = 0;
 > > +     u8   tmp = 0;
 > > +     int  i = 0;
 > > +     int  ret = -ENOMEM;
 > > +     char chip[][8] = { "First",  "Second" };
 > > +
 > > +     for (i = 0 ; i < ARRAY_SIZE(pnp_port) ; i++) {
 >
 > Should 'i' be 'port' here?

I think using 'chip' would be better.
Here 0 for the first chip, and 1 for the second chip.
I modified the code below. Is this okay?

     int  chip = 0;
     int  ret = -ENOMEM;

     for (chip = 0; chip < ARRAY_SIZE(pnp_port); chip++) {
         struct eiois200_dev_port *port = pnp_port + chip;

 >
 > > +             struct eiois200_dev_port *port = pnp_port + i;
 > > +
 > > +             if (!devm_request_region(dev,
 > > +                                      pnp_port[i].idx,
 > > +                                      2,
 >
 > What's 2?  This should probably be defined.

Yes, It is confusing. Is the following code okay?

if (!devm_request_region(dev,
              pnp_port[chip].idx_port,
              pnp_port[chip].data_port -
              pnp_port[chip].idx_port,
              KBUILD_MODNAME))

 >
 > > +                                      KBUILD_MODNAME))
 > > +                     continue;
 > > +
 > > +             is200_pnp_enter(port);
 > > +
 > > +             chip_id  = is200_pnp_read(port, EIOIS200_CHIPID1) << 8;
 > > +             chip_id |= is200_pnp_read(port, EIOIS200_CHIPID2);
 > > +
 > > +             if (chip_id != EIOIS200_CHIPID &&
 > > +                 chip_id != EIO201_211_CHIPID)
 > > +                     continue;
 > > +
 > > +             /* Turn on the enable flag */
 > > +             tmp = is200_pnp_read(port, EIOIS200_SIOCTRL);
 > > +             tmp |= EIOIS200_SIOCTRL_SIOEN;
 > > +             is200_pnp_write(port, EIOIS200_SIOCTRL, tmp);
 > > +
 > > +             is200_pnp_leave(port);
 > > +
 > > +             ret = get_pmc_port(dev, i, port);
 > > +             if (ret)
 > > +                     return ret;
 > > +
 > > +             eiois200_dev->flag |= i == 0 ?  EIOIS200_F_CHIP_EXIST :
 >
 > This logic either needs improving or commented on.

Is this okay?

     if (chip == 0)
         eiois200_dev->flag |= EIOIS200_F_CHIP_EXIST;
     else
         eiois200_dev->flag |= EIOIS200_F_SUB_CHIP_EXIST;

 >
 > > + EIOIS200_F_SUB_CHIP_EXIST;
 > > +
 > > +             pr_info("%s chip detected: %04X\n", chip[i], chip_id);
 >
 > We can almost certainly do without these in production code.

Yes, I'll remove it.

 >
 > > +     }
 > > +
 > > +     return ret;
 > > +}
 > > +
 > > +/* read information about acpi stored in EC */
 >
 > Please use prober grammar.
 >
 > "Read ACPI information stored in the EC"

Okay.

     /**
      * acpiram_access - Read ACPI information stored in the EC
      * @dev:    The device structure pointer.
      * @offset:    The offset of information.
      * Returns:    The value read from the PMC, or 0 if there was an error.
      */
     static uint8_t acpiram_access(struct device *dev, uint8_t offset)

 >
 > > +static uint8_t acpiram_access(uint8_t offset)
 > > +{
 > > +     u8  val;
 > > +     int ret;
 > > +
 > > +     mutex_lock(&eiois200_dev->mutex);
 > > +
 > > +     pmc_clear(0);
 > > +
 > > +     ret = pmc_write_cmd(0, EIOIS200_PMC_CMD_ACPIRAM_READ, 0);
 >
 > All of the magic numbers in this function need to be defined.

The chip id = 0 means the first chip.
The timeout = 0 means not specified timeout, Use default value.

The code modified as below:

     int timeout = 0;

     /* We only store information only on primary EC */
     int chip = 0;

     mutex_lock(&eiois200_dev->mutex);

     pmc_clear(dev, chip);

     ret = pmc_write_cmd(dev, chip, EIOIS200_PMC_CMD_ACPIRAM_READ, timeout);
     if (ret)
         goto err;

 >
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_write_data(0, offset, 0);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_write_data(0, 1, 0);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +     ret = pmc_read_data(0, &val, 0);
 > > +     if (ret)
 > > +             goto err;
 > > +
 > > +err:
 > > +     mutex_unlock(&eiois200_dev->mutex);
 > > +     return ret ? 0 : val;
 >
 > If it fails, you return success?

It returns 0 on error, or returns the value read.
Maybe I should add the comment below.

     /**
      * acpiram_access - Read ACPI information stored in the EC
      * @offset:    The offset of information.
      * Returns:    The value read from the PMC, or 0 if there was an error.
     */
     static uint8_t acpiram_access(uint8_t offset)

 >
 > > +}
 > > +
 > > +static int firmware_code_base(struct device *dev)
 > > +{
 > > +     u8 ic_vendor, ic_code, code_base;
 > > +
 > > +     ic_vendor = acpiram_access(EIOIS200_ACPIRAM_ICVENDOR);
 > > +     ic_code   = acpiram_access(EIOIS200_ACPIRAM_ICCODE);
 > > +     code_base = acpiram_access(EIOIS200_ACPIRAM_CODEBASE);
 > > +
 > > +     if (ic_vendor != 'R')
 > > +             return -ENODEV;
 > > +
 > > +     if (ic_code != EIOIS200_ICCODE &&
 > > +         ic_code != EIO201_ICCODE   &&
 > > +         ic_code != EIO211_ICCODE)
 > > +             goto err;
 > > +
 > > +     if (code_base == 0x80) {
 >
 > No magic numbers please.

Sorry, I'll fix these and others.

 >
 > > +             eiois200_dev->flag |= EIOIS200_F_NEW_CODE_BASE;
 > > +             return 0;
 > > +     }
 > > +
 > > +     if (code_base == 0 && (ic_code != EIO201_ICCODE &&
 > > +                            ic_code != EIO211_ICCODE)) {
 > > +             pr_info("Old code base not supported, yet.");
 > > +             return -ENODEV;
 > > +     }
 > > +
 > > + err:
 > > +     dev_err(dev,
 > > +             "Codebase check fail:\n"
 > > +             "ic_vendor: 0x%X  ,ic_code : 0x%X ,code_base : 0x%X\n",
 > > +             ic_vendor, ic_code, code_base);
 >
 > These are really ugly prints.
 >
 > Also, they should not contain new lines.

Sorry, Is following better, or any advice?

     /* 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);

Normally, here should not be called. Only on firmware error.

 >
 > > +     return -ENODEV;
 > > +}
 > > +
 > > +int eiois200_probe(struct device *dev, unsigned int id)
 >
 > .probe() usually goes near the bottom.

Got it.

 >
 > > +{
 > > +     int  ret = 0;
 > > +     void __iomem *iomem;
 >
 > Swap these over.

Okay.

 >
 > > +     iomem = devm_ioport_map(dev, 0, EIOIS200_SUB_PNP_DATA + 1);
 > > +     if (!iomem)
 > > +             return -ENOMEM;
 > > +
 > > +     regmap_is200 = devm_regmap_init_mmio(dev, iomem, 
&pnp_regmap_config);
 > > +     if (!regmap_is200)
 > > +             return -ENOMEM;
 > > +
 > > +     eiois200_dev = devm_kzalloc(dev,
 > > +                                 sizeof(struct eiois200_dev),
 >
 > sizeof(*eiois200_dev)

Got it.

     eiois200_dev = devm_kzalloc(dev,
                     sizeof(*eiois200_dev),
                     GFP_KERNEL);

 >
 > > +                                 GFP_KERNEL);
 > > +     if (!eiois200_dev)
 > > +             return -ENOMEM;
 > > +
 > > +     mutex_init(&eiois200_dev->mutex);
 > > +
 > > +     if (eiois200_exist(dev))
 > > +             return -ENODEV;
 > > +
 > > +     if (firmware_code_base(dev)) {
 > > +             dev_err(dev, "Chip code base check fail\n");
 > > +             return -EIO;
 > > +     }
 > > +
 > > +     dev_set_drvdata(dev, eiois200_dev);
 >
 > What data have you stored in eiois200_dev?

Maybe I should change the name 'eiois200_exist' to be 'eiois200_init'.
And this function and firmware_code_base will fill the 'eiois200_dev'
structure.
The structure defined in eiois200.h:

     #define EIOIS200_EC_NUM    2

     ...

     struct eiois200_dev {
         u32 flag;

         struct _pmc_port  pmc[EIOIS200_EC_NUM];

         struct mutex mutex; /* Protects PMC command access */
     };

 >
 > > +     ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, 
susi_mfd_devs,
 > > +  ARRAY_SIZE(susi_mfd_devs),
 > > +                                NULL, 0, NULL);
 > > +     if (ret)
 > > +             dev_err(dev, "Cannot register child devices (error = 
%d)\n", ret);
 > > +
 > > +     pr_info("%s started", KBUILD_MODNAME);
 >
 > These prints are not useful, please remove them.

Yes, I removed it.

 >
 > > +
 > > +     return 0;
 > > +}
 > > +
 > > +struct {
 > > +     char name[32];
 > > +     int  cmd;
 > > +     int  ctrl;
 > > +     int  size;
 > > +} attrs[] = {
 > > +     { "board_name",         0x53, 0x10, 16 },
 > > +     { "board_serial",       0x53, 0x1F, 16 },
 > > +     { "board_manufacturer", 0x53, 0x11, 16 },
 > > +     { "board_id",           0x53, 0x1E,  4 },
 > > +     { "firmware_version",   0x53, 0x22, 16 },
 > > +     { "firmware_build",     0x53, 0x23, 26 },
 > > +     { "firmware_date",      0x53, 0x24, 16 },
 > > +     { "chip_id",            0x53, 0x12, 12 },
 > > +     { "chip_detect",        0x53, 0x15, 12 },
 > > +     { "platform_type",      0x53, 0x13, 16 },
 > > +     { "platform_revision",  0x53, 0x14,  4 },
 > > +     { "eapi_version",       0x53, 0x30,  4 },
 > > +     { "eapi_id",            0x53, 0x31,  4 },
 > > +     { "boot_count",         0x55, 0x10,  4 },
 > > +     { "powerup_hour",       0x55, 0x11,  4 },
 > > +};
 > > +
 > > +static ssize_t info_show(struct device *dev,
 > > +                      struct device_attribute *attr, char *buf)
 >
 > SYSFS handling usually goes near the top.

Got it, I'll move it up.

 >
 > > +{
 > > +     uint i;
 > > +
 > > +     for (i = 0 ; i < ARRAY_SIZE(attrs) ; i++)
 > > +             if (!strcmp(attr->attr.name, attrs[i].name)) {
 > > +                     int ret;
 > > +                     char str[32] = "";
 > > +                     struct _pmc_op op = {
 > > +                             .cmd     = attrs[i].cmd,
 > > +                             .control = attrs[i].ctrl,
 > > +                             .payload = (u8 *)str,
 > > +                             .size    = attrs[i].size,
 > > +                     };
 > > +
 > > +                     ret = eiois200_core_pmc_operation(&op);
 > > +                     if (ret)
 > > +                             return ret;
 > > +
 > > +                     if (attrs[i].size == 4)
 > > +                             return sysfs_emit(buf, "%X\n", *(u32 
*)str);
 > > +                     else
 > > +                             return sysfs_emit(buf, "%s\n", str);
 > > +             }
 > > +
 > > +     return -EINVAL;
 > > +}
 > > +
 > > +static DEVICE_ATTR(board_name,               0444, info_show, NULL);
 > > +static DEVICE_ATTR(board_serial,     0444, info_show, NULL);
 > > +static DEVICE_ATTR(board_manufacturer,       0444, info_show, NULL);
 > > +static DEVICE_ATTR(firmware_version, 0444, info_show, NULL);
 > > +static DEVICE_ATTR(firmware_build,   0444, info_show, NULL);
 > > +static DEVICE_ATTR(firmware_date,    0444, info_show, NULL);
 > > +static DEVICE_ATTR(chip_id,          0444, info_show, NULL);
 > > +static DEVICE_ATTR(chip_detect,              0444, info_show, NULL);
 > > +static DEVICE_ATTR(platform_type,    0444, info_show, NULL);
 > > +static DEVICE_ATTR(platform_revision,        0444, info_show, NULL);
 > > +static DEVICE_ATTR(board_id,         0444, info_show, NULL);
 > > +static DEVICE_ATTR(eapi_version,     0444, info_show, NULL);
 > > +static DEVICE_ATTR(eapi_id,          0444, info_show, NULL);
 > > +static DEVICE_ATTR(boot_count,               0444, info_show, NULL);
 > > +static DEVICE_ATTR(powerup_hour,     0444, info_show, NULL);
 > > +
 > > +static struct attribute *pmc_attrs[] = {
 > > +     &dev_attr_board_name.attr,
 > > +     &dev_attr_board_serial.attr,
 > > +     &dev_attr_board_manufacturer.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,
 > > +     NULL
 > > +};
 > > +
 > > +static const struct attribute_group attr_group = {
 > > +     .attrs = pmc_attrs,
 > > +};
 > > +
 > > +static const struct attribute_group *attr_groups[] = {
 > > +     &attr_group,
 > > +     NULL
 > > +};
 >
 > Pretty sure there are helper MACROS for this.

Yes, there are. Is the following okay?

     #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)

     PMC_DEVICE_ATTR_RO(board_name);
     PMC_DEVICE_ATTR_RO(board_serial);
     PMC_DEVICE_ATTR_RO(board_manufacturer);
     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);

     static struct attribute *pmc_attrs[] = {
         &dev_attr_board_name.attr,
         &dev_attr_board_serial.attr,
         &dev_attr_board_manufacturer.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,
         NULL
     };

     ATTRIBUTE_GROUPS(pmc);

 >
 > > +static struct isa_driver eiois200_driver = {
 > > +     .probe    = eiois200_probe,
 > > +
 > > +     .driver = {
 > > +             .owner = THIS_MODULE,
 >
 > Are you sure the class doesn't do this for you?

You are right. I'll remove it.

 >
 > > +             .name  = KBUILD_MODNAME,
 >
 > Use a proper string.

Is the following acceptable?

     static struct isa_driver eiois200_driver = {
         .probe    = eiois200_probe,

         .driver = {
             .name = "eiois200_core",
             .dev_groups = pmc_groups,
         },
     };
     module_isa_driver(eiois200_driver, 1);

 >
 > > +             .dev_groups = attr_groups,
 > > +     },
 > > +};
 > > +
 > > +module_isa_driver(eiois200_driver, 1);
 > > +
 > > +MODULE_AUTHOR("susi.driver <susi.driver@advantech.com.tw>");
 >
 > This does not match the credentials above.

You are right. I should use 'Wenkai <advantech.susiteam@gmail.com>', Right?

 >
 > > +MODULE_DESCRIPTION("Advantech EIO-IS200 series EC core driver");
 > > +MODULE_LICENSE("GPL v2");
 > > --
 > > 2.34.1
 > >
 >
 > --
 > Lee Jones [李琼斯]