[PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Campion Kang]

Changed in V7:
	1. According to the reviewer's comment, add two files:
	   Documentation/hwmon/ahc1ec0-hwmon.rst and
	   drivers/platform/x86/ahc1ec0-core.c

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 MAINTAINERS | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 83c2b1867586..984795eb6b5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -572,6 +572,19 @@ S:	Maintained
 F:	Documentation/scsi/advansys.rst
 F:	drivers/scsi/advansys.c
 
+ADVANTECH AHC1EC0 EMBEDDED CONTROLLER DRIVER
+M:	Campion Kang <campion.kang@advantech.com.tw>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
+F:	Documentation/hwmon/ahc1ec0-hwmon.rst
+F:	drivers/hwmon/ahc1ec0-hwmon.c
+F:	drivers/mfd/ahc1ec0.c
+F:	drivers/platform/x86/ahc1ec0-core.c
+F:	drivers/watchdog/ahc1ec0-wdt.c
+F:	include/dt-bindings/mfd/ahc1ec0-dt.h
+F:	include/linux/platform_data/ahc1ec0.h
+
 ADVANTECH SWBTN DRIVER
 M:	Andrea Ho <Andrea.Ho@advantech.com.tw>
 L:	platform-driver-x86@vger.kernel.org
-- 
2.17.1

[PATCH v7 2/7] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings

[Campion Kang]

This files defines general used and HWMON profiles support by Advantech
embedded controller AHC1EC0.

Changed in v7:
	- remove unnecessary Sub-device definitions

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 include/dt-bindings/mfd/ahc1ec0-dt.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 include/dt-bindings/mfd/ahc1ec0-dt.h

diff --git a/include/dt-bindings/mfd/ahc1ec0-dt.h b/include/dt-bindings/mfd/ahc1ec0-dt.h
new file mode 100644
index 000000000000..a35ecd5ce9d2
--- /dev/null
+++ b/include/dt-bindings/mfd/ahc1ec0-dt.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Device Tree defines for Advantech Embedded Controller (AHC1EC0)
+ */
+
+#ifndef _DT_BINDINGS_MFD_AHC1EC0_H
+#define _DT_BINDINGS_MFD_AHC1EC0_H
+
+/* HWMON Profile Definitions */
+#define AHC1EC0_HWMON_PRO_TEMPLATE	0x0
+#define AHC1EC0_HWMON_PRO_TPC5XXX	0x1
+#define AHC1EC0_HWMON_PRO_PRVR4		0x2
+#define AHC1EC0_HWMON_PRO_UNO2271G	0x3
+#define AHC1EC0_HWMON_PRO_UNO1172A	0x4
+#define AHC1EC0_HWMON_PRO_UNO1372G	0x5
+
+#endif /* _DT_BINDINGS_MFD_AHC1EC0_H */
-- 
2.17.1

[PATCH v7 3/7] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0

[Campion Kang]

Add DT binding schema for Advantech embedded controller AHC1EC0.

Changed in v7:
	Fix the patch according to reviewer's comment:
	-remove inappropriate attributes: sub-dev-nb, sub-dev

Changed in v6:
	-rename dt-bindings/mfd/ahc1ec0.h to dt-bindings/mfd/ahc1ec0-dt.h
	 that found errors by bot 'make dt_binding_check'

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 .../devicetree/bindings/mfd/ahc1ec0.yaml      | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/ahc1ec0.yaml

diff --git a/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
new file mode 100644
index 000000000000..1bc96933ce29
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ahc1ec0.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Advantech Embedded Controller (AHC1EC0)
+
+maintainers:
+  - Campion Kang <campion.kang@advantech.com.tw>
+
+description: |
+  AHC1EC0 is one of the embedded controllers used by Advantech to provide several
+  functions such as watchdog, hwmon, brightness, GPIO, LED, etc. Advantech related
+  applications can control the whole system via these functions.
+
+properties:
+  compatible:
+    const: advantech,ahc1ec0
+
+  advantech,hwmon-profile:
+    description:
+      The number of sub-devices specified in the platform. Defines for the
+      hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  advantech,has-watchdog:
+    description:
+      Some implementations of the EC include a watchdog used to monitor the
+      system. This boolean flag is used to specify whether this watchdog is
+      present or not. Default is true, otherwise set to false.
+    type: boolean
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/mfd/ahc1ec0-dt.h>
+    ahc1ec0 {
+        compatible = "advantech,ahc1ec0";
+
+        advantech,hwmon-profile = <AHC1EC0_HWMON_PRO_UNO2271G>;
+        advantech,has-watchdog;
+    };
-- 
2.17.1

[PATCH v7 4/7] platform: x86: ahc1ec0-core: Add support for Advantech embedded controller

[Campion Kang]

AHC1EC0 is the embedded controller for Advantech industrial products.
This provides core functions in lower layer to access EC chip.

Changed in V7:
	Fix the patch according to reviewer's comment:
	- move some common functions from drivers/mfd/ahc1ec0.c
	- add prefix for function name

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 drivers/platform/x86/Kconfig          |   9 +
 drivers/platform/x86/Makefile         |   1 +
 drivers/platform/x86/ahc1ec0-core.c   | 570 ++++++++++++++++++++++++++
 include/linux/platform_data/ahc1ec0.h | 279 +++++++++++++
 4 files changed, 859 insertions(+)
 create mode 100644 drivers/platform/x86/ahc1ec0-core.c
 create mode 100644 include/linux/platform_data/ahc1ec0.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 2714f7c3843e..4afeca634432 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -215,6 +215,15 @@ config ADV_SWBUTTON
 	  To compile this driver as a module, choose M here. The module will
 	  be called adv_swbutton.
 
+config AHC1EC0_CORE
+	tristate "Advantech AHC1EC0 Embedded Controller Core"
+	depends on X86
+	help
+	  This driver provides core functionality for the Advantech AHC1EC0
+	  Embedded Controller (EC) along with registration for HWMON and
+	  Watchdog sub-devices. It also provides lower read and write APIs for
+	  communication with the EC chip.
+
 config APPLE_GMUX
 	tristate "Apple Gmux Driver"
 	depends on ACPI && PCI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index dcc8cdb95b4d..74f057343f77 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_AMD_PMC)		+= amd-pmc.o
 
 # Advantech
 obj-$(CONFIG_ADV_SWBUTTON)	+= adv_swbutton.o
+obj-$(CONFIG_AHC1EC0_CORE)	+= ahc1ec0-core.o
 
 # Apple
 obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
diff --git a/drivers/platform/x86/ahc1ec0-core.c b/drivers/platform/x86/ahc1ec0-core.c
new file mode 100644
index 000000000000..863916d17220
--- /dev/null
+++ b/drivers/platform/x86/ahc1ec0-core.c
@@ -0,0 +1,570 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Advantech AHC1EC0 Embedded Controller Core
+ *
+ * Copyright 2021 Advantech IIoT Group
+ */
+
+#include <linux/delay.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/platform_data/ahc1ec0.h>
+#include <linux/module.h>
+
+/* Wait IBF (Input Buffer Full) clear */
+static int ec_wait_write(void)
+{
+	int i;
+
+	for (i = 0; i < EC_MAX_TIMEOUT_COUNT; i++) {
+		if ((inb(EC_COMMAND_PORT) & EC_COMMAND_BIT_IBF) == 0)
+			return 0;
+
+		udelay(EC_RETRY_UDELAY);
+	}
+
+	return -ETIMEDOUT;
+}
+
+/* Wait OBF (Output Buffer Full) data ready */
+static int ec_wait_read(void)
+{
+	int i;
+
+	for (i = 0; i < EC_MAX_TIMEOUT_COUNT; i++) {
+		if ((inb(EC_COMMAND_PORT) & EC_COMMAND_BIT_OBF) != 0)
+			return 0;
+
+		udelay(EC_RETRY_UDELAY);
+	}
+
+	return -ETIMEDOUT;
+}
+
+/* Read data from EC HW RAM, the process is the following:
+ * Step 0. Wait IBF clear to send command
+ * Step 1. Send read command to EC command port
+ * Step 2. Wait IBF clear that means command is got by EC
+ * Step 3. Send read address to EC data port
+ * Step 4. Wait OBF data ready
+ * Step 5. Get data from EC data port
+ */
+int ahc1ec_read_hw_ram(struct adv_ec_ddata *ddata,
+		       unsigned char addr, unsigned char *data)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_HW_RAM_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(addr, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	*data = inb(EC_STATUS_PORT);
+
+	mutex_unlock(&ddata->lock);
+
+	return ret;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_read_hw_ram);
+
+/* Write data to EC HW RAM
+ * Step 0. Wait IBF clear to send command
+ * Step 1. Send write command to EC command port
+ * Step 2. Wait IBF clear that means command is got by EC
+ * Step 3. Send write address to EC data port
+ * Step 4. Wait IBF clear that means command is got by EC
+ * Step 5. Send data to EC data port
+ */
+int ahc1ec_write_hw_ram(struct adv_ec_ddata *ddata,
+			unsigned char addr, unsigned char data)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_HW_RAM_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(addr, EC_STATUS_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(data, EC_STATUS_PORT);
+
+	mutex_unlock(&ddata->lock);
+
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_write_hw_ram);
+
+/* Get dynamic control table */
+int adv_get_dynamic_tab(struct adv_ec_ddata *ddata)
+{
+	int i, ret;
+	unsigned char pin_tmp, device_id;
+
+	mutex_lock(&ddata->lock);
+
+	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
+		ddata->dym_tbl[i].device_id = EC_TBL_NOTFOUND;
+		ddata->dym_tbl[i].hw_pin_num = EC_TBL_NOTFOUND;
+	}
+
+	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
+		ret = ec_wait_write();
+		if (ret)
+			goto error;
+		outb(EC_TBL_WRITE_ITEM, EC_COMMAND_PORT);
+
+		ret = ec_wait_write();
+		if (ret)
+			goto error;
+		outb(i, EC_STATUS_PORT);
+
+		ret = ec_wait_read();
+		if (ret)
+			goto error;
+
+		/*
+		 *  If item is defined, EC will return item number.
+		 *  If table item is not defined, EC will return EC_TBL_NOTFOUND(0xFF).
+		 */
+		pin_tmp = inb(EC_STATUS_PORT);
+		if (pin_tmp == EC_TBL_NOTFOUND)
+			goto pass;
+
+		ret = ec_wait_write();
+		if (ret)
+			goto error;
+		outb(EC_TBL_GET_PIN, EC_COMMAND_PORT);
+
+		ret = ec_wait_read();
+		if (ret)
+			goto error;
+		pin_tmp = inb(EC_STATUS_PORT) & EC_STATUS_BIT;
+		if (pin_tmp == EC_TBL_NOTFOUND)
+			goto pass;
+
+		ret = ec_wait_write();
+		if (ret)
+			goto error;
+		outb(EC_TBL_GET_DEVID, EC_COMMAND_PORT);
+
+		ret = ec_wait_read();
+		if (ret)
+			goto error;
+		device_id = inb(EC_STATUS_PORT) & EC_STATUS_BIT;
+
+		ddata->dym_tbl[i].device_id = device_id;
+		ddata->dym_tbl[i].hw_pin_num = pin_tmp;
+	}
+
+pass:
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+
+	return ret;
+}
+EXPORT_SYMBOL(adv_get_dynamic_tab);
+
+int adv_ec_get_productname(struct adv_ec_ddata *ddata, char *product)
+{
+	const char *vendor, *device;
+	int length = 0;
+
+	/* Check it is Advantech board */
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (memcmp(vendor, "Advantech", sizeof("Advantech")) != 0)
+		return -ENODEV;
+
+	/* Get product model name */
+	device = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (device) {
+		while ((device[length] != ' ') && (length < AMI_ADVANTECH_BOARD_ID_LENGTH))
+			length++;
+		memset(product, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
+		memmove(product, device, length);
+
+		dev_info(ddata->dev, "BIOS Product Name = %s\n", product);
+
+		return 0;
+	}
+
+	dev_warn(ddata->dev, "This device is not Advantech Devices (%s)!\n", product);
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(adv_ec_get_productname);
+
+int ahc1ec_read_adc_value(struct adv_ec_ddata *ddata, unsigned char hwpin,
+			  unsigned char multi)
+{
+	int ret;
+	u32 ret_val;
+	unsigned int LSB, MSB;
+
+	mutex_lock(&ddata->lock);
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_ADC_INDEX_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(hwpin, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == EC_TBL_NOTFOUND) {
+		mutex_unlock(&ddata->lock);
+		return -1;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_ADC_LSB_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	LSB = inb(EC_STATUS_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_ADC_MSB_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	MSB = inb(EC_STATUS_PORT);
+	ret_val = ((MSB << 8) | LSB) & EC_ADC_VALID_BIT;
+	ret_val = ret_val * multi * 100;
+
+	mutex_unlock(&ddata->lock);
+	return ret_val;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_read_adc_value);
+
+int ahc1ec_read_acpi_value(struct adv_ec_ddata *ddata,
+			   unsigned char addr, unsigned char *pvalue)
+{
+	int ret;
+	unsigned char value;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_ACPI_RAM_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(addr, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	value = inb(EC_STATUS_PORT);
+	*pvalue = value;
+
+	mutex_unlock(&ddata->lock);
+
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_read_acpi_value);
+
+int ahc1ec_write_acpi_value(struct adv_ec_ddata *ddata,
+			    unsigned char addr, unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_ACPI_DATA_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(addr, EC_STATUS_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(value, EC_STATUS_PORT);
+
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_write_acpi_value);
+
+int ahc1ec_read_gpio_status(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			    unsigned char *pvalue)
+{
+	int ret;
+
+	unsigned char gpio_status_value;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(pin_number, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == EC_TBL_NOTFOUND) {
+		ret = -1;
+		goto error;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_STATUS_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	gpio_status_value = inb(EC_STATUS_PORT);
+
+	*pvalue = gpio_status_value;
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_read_gpio_status);
+
+int ahc1ec_write_gpio_status(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			     unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(pin_number, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == EC_TBL_NOTFOUND) {
+		ret = -1;
+		goto error;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_STATUS_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(value, EC_STATUS_PORT);
+
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_write_gpio_status);
+
+int ahc1ec_read_gpio_dir(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			 unsigned char *pvalue)
+{
+	int ret;
+	unsigned char gpio_dir_value;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(pin_number, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == EC_TBL_NOTFOUND) {
+		ret = -1;
+		goto error;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_DIR_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	gpio_dir_value = inb(EC_STATUS_PORT);
+	*pvalue = gpio_dir_value;
+
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_read_gpio_dir);
+
+int ahc1ec_write_gpio_dir(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			  unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_INDEX_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(pin_number, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == EC_TBL_NOTFOUND) {
+		mutex_unlock(&ddata->lock);
+		return -1;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_GPIO_DIR_WRITE, EC_COMMAND_PORT);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(value, EC_STATUS_PORT);
+
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_write_gpio_dir);
+
+int ahc1ec_write_hwram_command(struct adv_ec_ddata *ddata, unsigned char data)
+{
+	int ret;
+
+	mutex_lock(&ddata->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(data, EC_COMMAND_PORT);
+
+	mutex_unlock(&ddata->lock);
+	return 0;
+
+error:
+	mutex_unlock(&ddata->lock);
+	dev_err(ddata->dev, "Wait for IBF or OBF too long.\n");
+	return ret;
+}
+EXPORT_SYMBOL(ahc1ec_write_hwram_command);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ahc1ec0-core");
+MODULE_DESCRIPTION("Advantech AHC1EC0 Embedded Controller Core");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_VERSION("1.0");
diff --git a/include/linux/platform_data/ahc1ec0.h b/include/linux/platform_data/ahc1ec0.h
new file mode 100644
index 000000000000..934cfe60e0b4
--- /dev/null
+++ b/include/linux/platform_data/ahc1ec0.h
@@ -0,0 +1,279 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_AHC1EC0_H
+#define __LINUX_AHC1EC0_H
+
+#include <linux/device.h>
+
+#define EC_COMMAND_PORT		0x29A /* EC I/O command port */
+#define EC_STATUS_PORT		0x299 /* EC I/O data port */
+
+#define EC_RETRY_UDELAY		200  /* EC command retry delay in microseconds */
+#define EC_MAX_TIMEOUT_COUNT	5000 /* EC command max retry count */
+#define EC_COMMAND_BIT_OBF	0x01 /* Bit 0 is for OBF ready (Output buffer full) */
+#define EC_COMMAND_BIT_IBF	0x02 /* Bit 1 is for IBF ready (Input buffer full) */
+
+/* Analog to digital converter command */
+#define EC_ADC_INDEX_WRITE	0x15 /* Write ADC port number into index */
+#define EC_ADC_LSB_READ		0x16 /* Read ADC LSB value from ADC port */
+#define EC_ADC_MSB_READ		0x1F /* Read ADC MSB value from ADC port */
+#define EC_ADC_VALID_BIT	0x03FF
+
+/* Voltage device ID */
+#define EC_DID_SMBOEM0		0x28 /* SMBUS/I2C. Smbus channel 0 */
+#define EC_DID_CMOSBAT		0x50 /* CMOS coin battery voltage */
+#define EC_DID_CMOSBAT_X2	0x51 /* CMOS coin battery voltage*2 */
+#define EC_DID_CMOSBAT_X10	0x52 /* CMOS coin battery voltage*10 */
+#define EC_DID_5VS0		0x56 /* 5VS0 voltage */
+#define EC_DID_5VS0_X2		0x57 /* 5VS0 voltage*2 */
+#define EC_DID_5VS0_X10		0x58 /* 5VS0 voltage*10 */
+#define EC_DID_5VS5		0x59 /* 5VS5 voltage */
+#define EC_DID_5VS5_X2		0x5A /* 5VS5 voltage*2 */
+#define EC_DID_5VS5_X10		0x5B /* 5VS5 voltage*10 */
+#define EC_DID_12VS0		0x62 /* 12VS0 voltage */
+#define EC_DID_12VS0_X2		0x63 /* 12VS0 voltage*2 */
+#define EC_DID_12VS0_X10	0x64 /* 12VS0 voltage*10 */
+#define EC_DID_VCOREA		0x65 /* CPU A core voltage */
+#define EC_DID_VCOREA_X2	0x66 /* CPU A core voltage*2 */
+#define EC_DID_VCOREA_X10	0x67 /* CPU A core voltage*10 */
+#define EC_DID_VCOREB		0x68 /* CPU B core voltage */
+#define EC_DID_VCOREB_X2	0x69 /* CPU B core voltage*2 */
+#define EC_DID_VCOREB_X10	0x6A /* CPU B core voltage*10 */
+#define EC_DID_DC		0x6B /* ADC. onboard voltage */
+#define EC_DID_DC_X2		0x6C /* ADC. onboard voltage*2 */
+#define EC_DID_DC_X10		0x6D /* ADC. onboard voltage*10 */
+
+/* Current device ID */
+#define EC_DID_CURRENT		0x74
+
+/* ACPI commands */
+#define EC_ACPI_RAM_READ	0x80
+#define EC_ACPI_RAM_WRITE	0x81
+
+/*
+ *  Dynamic control table commands
+ *  The table includes HW pin number, Device ID, and Pin polarity
+ */
+#define EC_TBL_WRITE_ITEM		0x20
+#define EC_TBL_GET_PIN			0x21
+#define EC_TBL_GET_DEVID		0x22
+#define EC_MAX_TBL_NUM			32
+#define EC_TBL_NOTFOUND			0xFF /*If table item is not defined, EC will return 0xFF.*/
+#define EC_STATUS_BIT			0xFF
+
+/* LED Device ID table */
+#define EC_DID_LED_RUN			0xE1
+#define EC_DID_LED_ERR			0xE2
+#define EC_DID_LED_SYS_RECOVERY		0xE3
+#define EC_DID_LED_D105_G		0xE4
+#define EC_DID_LED_D106_G		0xE5
+#define EC_DID_LED_D107_G		0xE6
+
+/* LED control HW RAM address 0xA0-0xAF */
+#define EC_HWRAM_LED_BASE_ADDR		0xA0
+#define EC_HWRAM_LED_PIN(N)		(EC_HWRAM_LED_BASE_ADDR + (4 * (N))) /* N:0-3 */
+#define EC_HWRAM_LED_CTRL_HIBYTE(N)	(EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 1)
+#define EC_HWRAM_LED_CTRL_LOBYTE(N)	(EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 2)
+#define EC_HWRAM_LED_DEVICE_ID(N)	(EC_HWRAM_LED_BASE_ADDR + (4 * (N)) + 3)
+
+/* LED control bit */
+#define LED_CTRL_ENABLE_BIT()		BIT(4)
+#define LED_CTRL_INTCTL_BIT()		BIT(5)
+#define LED_CTRL_LEDBIT_MASK		(0x03FF << 6)
+#define LED_CTRL_POLARITY_MASK		(0x000F << 0)
+#define LED_CTRL_INTCTL_EXTERNAL	0
+#define LED_CTRL_INTCTL_INTERNAL	1
+
+#define LED_DISABLE	0x0
+#define LED_ON		0x1
+#define LED_FAST	0x3
+#define LED_NORMAL	0x5
+#define LED_SLOW	0x7
+#define LED_MANUAL	0xF
+
+#define LED_CTRL_LEDBIT_DISABLE		0x0000
+#define LED_CTRL_LEDBIT_ON		0x03FF
+#define LED_CTRL_LEDBIT_FAST		0x02AA
+#define LED_CTRL_LEDBIT_NORMAL		0x0333
+#define LED_CTRL_LEDBIT_SLOW		0x03E0
+
+/* Get the device name */
+#define AMI_ADVANTECH_BOARD_ID_LENGTH	32
+
+/*
+ * Advantech Embedded Controller watchdog commands
+ * EC can send multi-stage watchdog event. System can setup watchdog event
+ * independently to make up event sequence.
+ */
+#define EC_COMMANS_PORT_IBF_MASK	0x02
+#define EC_RESET_EVENT			0x04
+#define	EC_WDT_START			0x28
+#define	EC_WDT_STOP			0x29
+#define	EC_WDT_RESET			0x2A
+#define	EC_WDT_BOOTTMEWDT_STOP		0x2B
+
+#define EC_HW_RAM			0x89
+
+#define EC_EVENT_FLAG			0x57
+#define EC_ENABLE_DELAY_H		0x58
+#define EC_ENABLE_DELAY_L		0x59
+#define EC_POWER_BTN_TIME_H		0x5A
+#define EC_POWER_BTN_TIME_L		0x5B
+#define EC_RESET_DELAY_TIME_H		0x5E
+#define EC_RESET_DELAY_TIME_L		0x5F
+#define EC_PIN_DELAY_TIME_H		0x60
+#define EC_PIN_DELAY_TIME_L		0x61
+#define EC_SCI_DELAY_TIME_H		0x62
+#define EC_SCI_DELAY_TIME_L		0x63
+
+/* EC ACPI commands */
+#define EC_ACPI_DATA_READ		0x80
+#define EC_ACPI_DATA_WRITE		0x81
+
+/* Brightness ACPI Addr */
+#define BRIGHTNESS_ACPI_ADDR		0x50
+
+/* EC HW RAM commands */
+#define EC_HW_EXTEND_RAM_READ		0x86
+#define EC_HW_EXTEND_RAM_WRITE		0x87
+#define	EC_HW_RAM_READ			0x88
+#define EC_HW_RAM_WRITE			0x89
+
+/* EC Smbus commands */
+#define EC_SMBUS_CHANNEL_SET		0x8A /* Set selector number (SMBUS channel) */
+#define EC_SMBUS_ENABLE_I2C		0x8C /* Enable channel I2C */
+#define EC_SMBUS_DISABLE_I2C		0x8D /* Disable channel I2C */
+
+/* Smbus transmit protocol */
+#define EC_SMBUS_PROTOCOL		0x00
+
+/* SMBUS status */
+#define EC_SMBUS_STATUS			0x01
+
+/* SMBUS device slave address (bit0 must be 0) */
+#define EC_SMBUS_SLV_ADDR		0x02
+
+/* SMBUS device command */
+#define EC_SMBUS_CMD			0x03
+
+/* 0x04-0x24 Data In read process, return data are stored in this address */
+#define EC_SMBUS_DATA			0x04
+
+#define EC_SMBUS_DAT_OFFSET(n)	(EC_SMBUS_DATA + (n))
+
+/* SMBUS channel selector (0-4) */
+#define EC_SMBUS_CHANNEL		0x2B
+
+/* EC SMBUS transmit Protocol code */
+#define SMBUS_QUICK_WRITE		0x02 /* Write Quick Command */
+#define SMBUS_QUICK_READ		0x03 /* Read Quick Command */
+#define SMBUS_BYTE_SEND			0x04 /* Send Byte */
+#define SMBUS_BYTE_RECEIVE		0x05 /* Receive Byte */
+#define SMBUS_BYTE_WRITE		0x06 /* Write Byte */
+#define SMBUS_BYTE_READ			0x07 /* Read Byte */
+#define SMBUS_WORD_WRITE		0x08 /* Write Word */
+#define SMBUS_WORD_READ			0x09 /* Read Word */
+#define SMBUS_BLOCK_WRITE		0x0A /* Write Block */
+#define SMBUS_BLOCK_READ		0x0B /* Read Block */
+#define SMBUS_PROC_CALL			0x0C /* Process Call */
+#define SMBUS_BLOCK_PROC_CALL		0x0D /* Write Block-Read Block Process Call */
+#define SMBUS_I2C_READ_WRITE		0x0E /* I2C block Read-Write */
+#define SMBUS_I2C_WRITE_READ		0x0F /* I2C block Write-Read */
+
+/* GPIO control commands */
+#define EC_GPIO_INDEX_WRITE		0x10
+#define EC_GPIO_STATUS_READ		0x11
+#define EC_GPIO_STATUS_WRITE		0x12
+#define EC_GPIO_DIR_READ		0x1D
+#define EC_GPIO_DIR_WRITE		0x1E
+
+/* One Key Recovery commands */
+#define EC_ONE_KEY_FLAG			0x9C
+
+/* ASG OEM commands */
+#define EC_ASG_OEM			0xEA
+#define EC_ASG_OEM_READ			0x00
+#define EC_ASG_OEM_WRITE		0x01
+#define EC_OEM_POWER_STATUS_VIN1	0X10
+#define EC_OEM_POWER_STATUS_VIN2	0X11
+#define EC_OEM_POWER_STATUS_BAT1	0X12
+#define EC_OEM_POWER_STATUS_BAT2	0X13
+
+/* GPIO DEVICE ID */
+#define EC_DID_ALTGPIO_0		0x10    /* 0x10 AltGpio0 User define gpio */
+#define EC_DID_ALTGPIO_1		0x11    /* 0x11 AltGpio1 User define gpio */
+#define EC_DID_ALTGPIO_2		0x12    /* 0x12 AltGpio2 User define gpio */
+#define EC_DID_ALTGPIO_3		0x13    /* 0x13 AltGpio3 User define gpio */
+#define EC_DID_ALTGPIO_4		0x14    /* 0x14 AltGpio4 User define gpio */
+#define EC_DID_ALTGPIO_5		0x15    /* 0x15 AltGpio5 User define gpio */
+#define EC_DID_ALTGPIO_6		0x16    /* 0x16 AltGpio6 User define gpio */
+#define EC_DID_ALTGPIO_7		0x17    /* 0x17 AltGpio7 User define gpio */
+
+/* Lmsensor Chip Register */
+#define NSLM96163_CHANNEL		0x02
+
+/* NS_LM96163 address 0x98 */
+#define NSLM96163_ADDR			0x98
+
+/* LM96163 index(0x00) Local Temperature (Signed MSB) */
+#define NSLM96163_LOC_TEMP		0x00
+
+/* HWMON registers */
+#define INA266_REG_VOLTAGE		0x02    /* 1.25mV */
+#define INA266_REG_POWER		0x03    /* 25mW */
+#define INA266_REG_CURRENT		0x04    /* 1mA */
+
+struct ec_hw_pin_table {
+	unsigned int vbat[2];
+	unsigned int v5[2];
+	unsigned int v12[2];
+	unsigned int vcore[2];
+	unsigned int vdc[2];
+	unsigned int ec_current[2];
+	unsigned int power[2];
+};
+
+struct ec_dynamic_table {
+	unsigned char device_id;
+	unsigned char hw_pin_num;
+};
+
+struct ec_smbuso_em0 {
+	unsigned char hw_pin_num;
+};
+
+struct pled_hw_pin_tbl {
+	unsigned int pled[6];
+};
+
+struct adv_ec_ddata {
+	char *bios_product_name;
+	struct mutex lock; /* lock EC access */
+	struct device *dev;
+	struct class *adv_ec_class;
+
+	struct ec_dynamic_table *dym_tbl;
+};
+
+int adv_get_dynamic_tab(struct adv_ec_ddata *ddata);
+int adv_ec_get_productname(struct adv_ec_ddata *ddata, char *product);
+int ahc1ec_read_adc_value(struct adv_ec_ddata *ddata, unsigned char hwpin,
+			  unsigned char multi);
+int ahc1ec_read_acpi_value(struct adv_ec_ddata *ddata,
+			   unsigned char addr, unsigned char *pvalue);
+int ahc1ec_write_acpi_value(struct adv_ec_ddata *ddata,
+			    unsigned char addr, unsigned char value);
+int ahc1ec_read_hw_ram(struct adv_ec_ddata *ddata,
+		       unsigned char addr, unsigned char *data);
+int ahc1ec_write_hw_ram(struct adv_ec_ddata *ddata,
+			unsigned char addr, unsigned char data);
+int ahc1ec_write_hwram_command(struct adv_ec_ddata *ddata, unsigned char data);
+int ahc1ec_read_gpio_status(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			    unsigned char *pvalue);
+int ahc1ec_write_gpio_status(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			     unsigned char value);
+int ahc1ec_read_gpio_dir(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			 unsigned char *pvalue);
+int ahc1ec_write_gpio_dir(struct adv_ec_ddata *ddata, unsigned char pin_number,
+			  unsigned char value);
+
+#endif /* __LINUX_AHC1EC0_H */
-- 
2.17.1

[PATCH v7 5/7] mfd: ahc1ec0: Add support for Advantech embedded controller

[Campion Kang]

AHC1EC0 is the embedded controller for Advantech industrial products.
This provides sub-devices such as HWMON and Watchdog, and also exposes
functions for sub-devices to read/write the value to embedded
controller.

Changed in V7:
	Fix the patch according to reviewer's comment:
	- donot need to do memory clear since devm_kzalloc() do it
	  internal
	- rename ADVEC_SUBDEV_{DEVICE} to ADVEC_ACPI_ID_{DEVICE}
	- move some common functions to
	  drivers/platform/x86/ahc1ec0-core.c
	- change the data content of ACPI table, use a clear sub-devices
	  name and properties instead of index to start sub-devices
	- correction of words

Changed in V6:
	- Kconfig: add "AHC1EC0" string to clearly define the EC name
	- fix the code according to reviewer's suggestion
	- remove unnecessary header files
	- change the structure name to lower case, align with others
	  naming

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 drivers/mfd/Kconfig   |  11 +++
 drivers/mfd/Makefile  |   1 +
 drivers/mfd/ahc1ec0.c | 172 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 184 insertions(+)
 create mode 100644 drivers/mfd/ahc1ec0.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 5c7f2b100191..b61b11506103 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2174,5 +2174,16 @@ config MFD_INTEL_M10_BMC
 	  additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_AHC1EC0
+	tristate "Advantech AHC1EC0 Embedded Controller"
+	depends on X86
+	depends on AHC1EC0_CORE
+	select MFD_CORE
+	help
+	  Select this to get support for Advantech Embedded Controller sub-devices.
+	  This driver will instantiate additional drivers such as HWMON, Watchdog,
+	  etc., you also have to select the individual drivers.
+	  The controller is running firmware customized for Advantech hardware.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 4f6d2b8a5f76..270db521e1d8 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -271,3 +271,4 @@ obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
 
 obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
 obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
+obj-$(CONFIG_MFD_AHC1EC0)	+= ahc1ec0.o
diff --git a/drivers/mfd/ahc1ec0.c b/drivers/mfd/ahc1ec0.c
new file mode 100644
index 000000000000..575b654ade5f
--- /dev/null
+++ b/drivers/mfd/ahc1ec0.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Advantech AHC1EC0 Embedded Controller
+ *
+ * Copyright 2021 Advantech IIoT Group
+ */
+
+#include <linux/acpi.h>
+#include <linux/errno.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/ahc1ec0.h>
+
+/* This order cannot be changed, it is use in enum and may in BIOS ACPI table. */
+enum {
+	ADVEC_ACPI_ID_BRIGHTNESS = 0,
+	ADVEC_ACPI_ID_EEPROM,
+	ADVEC_ACPI_ID_GPIO,
+	ADVEC_ACPI_ID_HWMON,
+	ADVEC_ACPI_ID_LED,
+	ADVEC_ACPI_ID_WDT,
+	ADVEC_ACPI_ID_MAX,
+};
+
+static const struct mfd_cell adv_ec_sub_cells[] = {
+	{ .name = "adv-ec-brightness", },
+	{ .name = "adv-ec-eeprom", },
+	{ .name = "adv-ec-gpio", },
+	{ .name = "ahc1ec0-hwmon", },
+	{ .name = "adv-ec-led", },
+	{ .name = "ahc1ec0-wdt", },
+};
+
+static int adv_ec_init_ec_data(struct adv_ec_ddata *ddata)
+{
+	int ret;
+
+	mutex_init(&ddata->lock);
+
+	/* Get product name */
+	ddata->bios_product_name =
+		devm_kzalloc(ddata->dev, AMI_ADVANTECH_BOARD_ID_LENGTH, GFP_KERNEL);
+	if (!ddata->bios_product_name)
+		return -ENOMEM;
+
+	ret = adv_ec_get_productname(ddata, ddata->bios_product_name);
+	if (ret)
+		return ret;
+
+	/* Get pin table */
+	ddata->dym_tbl = devm_kzalloc(ddata->dev,
+				      EC_MAX_TBL_NUM * sizeof(struct ec_dynamic_table),
+				      GFP_KERNEL);
+	if (!ddata->dym_tbl)
+		return -ENOMEM;
+
+	return adv_get_dynamic_tab(ddata);
+}
+
+static int adv_ec_parse_prop(struct adv_ec_ddata *ddata)
+{
+	int ret;
+	u32 value;
+	bool has_watchdog = true;
+
+	/* check whether this EC has the following subdevices, hwmon and watchdog. */
+	if (device_property_read_u32(ddata->dev, "advantech,hwmon-profile", &value) >= 0) {
+		ret = mfd_add_hotplug_devices(ddata->dev,
+					      &adv_ec_sub_cells[ADVEC_ACPI_ID_HWMON], 1);
+		if (ret) {
+			dev_err(ddata->dev, "Failed to add %s subdevice: %d\n",
+				adv_ec_sub_cells[ADVEC_ACPI_ID_HWMON].name, ret);
+		} else {
+			dev_info(ddata->dev, "Success to add %s subdevice\n",
+				 adv_ec_sub_cells[ADVEC_ACPI_ID_HWMON].name);
+		}
+	} else {
+		dev_err(ddata->dev, "No 'advantech,hwmon-profile' property: %d\n",
+			ret);
+	}
+
+	/* default is true for watchdog even if it is not existed */
+	if (device_property_present(ddata->dev, "advantech,has-watchdog"))
+		has_watchdog = device_property_read_bool(ddata->dev, "advantech,has-watchdog");
+	if (has_watchdog) {
+		ret = mfd_add_hotplug_devices(ddata->dev, &adv_ec_sub_cells[ADVEC_ACPI_ID_WDT], 1);
+		if (ret) {
+			dev_err(ddata->dev, "Failed to add %s subdevice: %d\n",
+				adv_ec_sub_cells[ADVEC_ACPI_ID_WDT].name, ret);
+		} else {
+			dev_info(ddata->dev, "Success to add %s subdevice\n",
+				 adv_ec_sub_cells[ADVEC_ACPI_ID_WDT].name);
+		}
+	}
+
+	return 0;
+}
+
+static int adv_ec_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct adv_ec_ddata *ddata;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ddata);
+	ddata->dev = dev;
+
+	ret = adv_ec_init_ec_data(ddata);
+	if (ret)
+		goto err_prop;
+
+	ret = adv_ec_parse_prop(ddata);
+	if (ret)
+		goto err_prop;
+
+	dev_info(ddata->dev, "Advantech AHC1EC0 probe done");
+
+	return 0;
+
+err_prop:
+	mutex_destroy(&ddata->lock);
+
+	dev_dbg(dev, "Failed to init data and probe\n");
+	return ret;
+}
+
+static int adv_ec_remove(struct platform_device *pdev)
+{
+	struct adv_ec_ddata *ddata;
+
+	ddata = dev_get_drvdata(&pdev->dev);
+
+	mutex_destroy(&ddata->lock);
+	return 0;
+}
+
+static const struct of_device_id adv_ec_of_match[] __maybe_unused = {
+	{ .compatible = "advantech,ahc1ec0" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adv_ec_of_match);
+
+static const struct acpi_device_id adv_ec_acpi_match[] __maybe_unused = {
+	{ "AHC1EC0", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, adv_ec_acpi_match);
+
+static struct platform_driver adv_ec_driver = {
+	.driver = {
+		.name = "ahc1ec0",
+		.of_match_table = of_match_ptr(adv_ec_of_match),
+		.acpi_match_table = ACPI_PTR(adv_ec_acpi_match),
+	},
+	.probe = adv_ec_probe,
+	.remove = adv_ec_remove,
+};
+module_platform_driver(adv_ec_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ahc1ec0");
+MODULE_DESCRIPTION("Advantech AHC1EC0 Embedded Controller");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
+MODULE_VERSION("1.0");
-- 
2.17.1

[PATCH v7 6/7] Watchdog: ahc1ec0-wdt: Add sub-device Watchdog for Advantech embedded controller

[Campion Kang]

This is one of sub-device driver for Advantech embedded controller
AHC1EC0. This driver provide Watchdog functionality for Advantech
related applications to restart the system.

Changed in V7:
	Fix the patch according to reviewer's comment:
	- remove unnecessary variables and fix error checks
	- remove error messages that avoid clogging the kernel log
	- remove advwdt_notify_sys(), use watchdog subsystem to process
	  reboot or shutdown
	- delete mutex lock, the watchdog core has processed it

Changed in V6:
	- remove unnecessary header files
	- bug fixed: reboot halt if watchdog enabled
	- Kconfig: add "AHC1EC0" string to clearly define the EC name

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 drivers/watchdog/Kconfig       |  11 +++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/ahc1ec0-wdt.c | 171 +++++++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 drivers/watchdog/ahc1ec0-wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 355100dad60a..5fe9add0a12d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1005,6 +1005,17 @@ config ADVANTECH_WDT
 	  feature. More information can be found at
 	  <https://www.advantech.com.tw/products/>
 
+config AHC1EC0_WDT
+	tristate "Advantech AHC1EC0 Watchdog Function"
+	depends on MFD_AHC1EC0
+	help
+	  This is sub-device for Advantech AHC1EC0 embedded controller.
+
+	  This driver provide watchdog functionality for Advantech related
+	  applications to restart the system.
+	  To compile this driver as a module, choose M here: the module will be
+	  called ahc1ec0-wdt.
+
 config ALIM1535_WDT
 	tristate "ALi M1535 PMU Watchdog Timer"
 	depends on X86 && PCI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a7eade8b4d45..0d78211e1412 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
 obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
 obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
 obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
+obj-$(CONFIG_AHC1EC0_WDT) += ahc1ec0-wdt.o
 
 # M68K Architecture
 obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
diff --git a/drivers/watchdog/ahc1ec0-wdt.c b/drivers/watchdog/ahc1ec0-wdt.c
new file mode 100644
index 000000000000..efa955c41a81
--- /dev/null
+++ b/drivers/watchdog/ahc1ec0-wdt.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Watchdog Driver for Advantech AHC1EC0 Embedded Controller
+ *
+ * Copyright 2021, Advantech IIoT Group
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_data/ahc1ec0.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+struct ec_wdt_data {
+	struct watchdog_device wdtdev;
+	struct adv_ec_ddata *ddata;
+	unsigned short timeout_in_ds; /* a decisecond */
+};
+
+#define EC_WDT_MIN_TIMEOUT	1   /* The watchdog devices minimum timeout value (in seconds). */
+#define EC_WDT_MAX_TIMEOUT	600 /* The watchdog devices maximum timeout value (in seconds) */
+#define EC_WDT_DEFAULT_TIMEOUT	45
+
+static int set_delay(struct adv_ec_ddata *ddata, unsigned short delay_timeout_in_ms)
+{
+	if (ahc1ec_write_hw_ram(ddata, EC_RESET_DELAY_TIME_L,
+				delay_timeout_in_ms & 0x00FF))
+		return -EINVAL;
+
+	if (ahc1ec_write_hw_ram(ddata, EC_RESET_DELAY_TIME_H,
+				(delay_timeout_in_ms & 0xFF00) >> 8))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ec_wdt_start(struct watchdog_device *wdd)
+{
+	int ret;
+	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
+	struct adv_ec_ddata *ddata;
+
+	ddata = ec_wdt_data->ddata;
+
+	/*
+	 * Because an unit of ahc1ec0_wdt is 0.1 seconds, timeout 100 is 10 seconds
+	 */
+	ec_wdt_data->timeout_in_ds = wdd->timeout * 10;
+
+	ret = set_delay(ddata, (ec_wdt_data->timeout_in_ds - 1));
+	if (ret)
+		goto exit;
+
+	ahc1ec_write_hwram_command(ddata, EC_WDT_STOP);
+	ret = ahc1ec_write_hwram_command(ddata, EC_WDT_START);
+	if (ret)
+		goto exit;
+
+exit:
+	return ret;
+}
+
+static int ec_wdt_stop(struct watchdog_device *wdd)
+{
+	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
+	struct adv_ec_ddata *ddata;
+
+	ddata = ec_wdt_data->ddata;
+
+	return ahc1ec_write_hwram_command(ddata, EC_WDT_STOP);
+}
+
+static int ec_wdt_ping(struct watchdog_device *wdd)
+{
+	int ret;
+	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
+	struct adv_ec_ddata *ddata;
+
+	ddata = ec_wdt_data->ddata;
+
+	ret = ahc1ec_write_hwram_command(ddata, EC_WDT_RESET);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ec_wdt_set_timeout(struct watchdog_device *wdd,
+			      unsigned int timeout)
+{
+	wdd->timeout = timeout;
+
+	if (watchdog_active(wdd))
+		return ec_wdt_start(wdd);
+
+	return 0;
+}
+
+static const struct watchdog_info ec_watchdog_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "AHC1EC0 Watchdog",
+};
+
+static const struct watchdog_ops ec_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = ec_wdt_start,
+	.stop = ec_wdt_stop,
+	.ping = ec_wdt_ping,
+	.set_timeout = ec_wdt_set_timeout,
+};
+
+static int adv_ec_wdt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct adv_ec_ddata *ddata;
+	struct ec_wdt_data *ec_wdt_data;
+	struct watchdog_device *wdd;
+
+	ddata = dev_get_drvdata(dev->parent);
+	if (!ddata)
+		return -EINVAL;
+
+	ec_wdt_data = devm_kzalloc(dev, sizeof(*ec_wdt_data), GFP_KERNEL);
+	if (!ec_wdt_data)
+		return -ENOMEM;
+
+	ec_wdt_data->ddata = ddata;
+	wdd = &ec_wdt_data->wdtdev;
+
+	watchdog_init_timeout(&ec_wdt_data->wdtdev, 0, dev);
+
+	/* watchdog_set_nowayout(&ec_wdt_data->wdtdev, WATCHDOG_NOWAYOUT); */
+	watchdog_set_drvdata(&ec_wdt_data->wdtdev, ec_wdt_data);
+	platform_set_drvdata(pdev, ec_wdt_data);
+
+	wdd->info = &ec_watchdog_info;
+	wdd->ops = &ec_watchdog_ops;
+	wdd->min_timeout = EC_WDT_MIN_TIMEOUT;
+	wdd->max_timeout = EC_WDT_MAX_TIMEOUT;
+	wdd->parent = dev;
+
+	ec_wdt_data->wdtdev.timeout = EC_WDT_DEFAULT_TIMEOUT;
+
+	watchdog_stop_on_reboot(wdd);
+	watchdog_stop_on_unregister(wdd);
+
+	ret = devm_watchdog_register_device(dev, wdd);
+	if (ret == 0)
+		dev_info(dev, "ahc1ec0 watchdog register success\n");
+
+	return ret;
+}
+
+static struct platform_driver adv_wdt_drv = {
+	.driver = {
+		.name = "ahc1ec0-wdt",
+	},
+	.probe = adv_ec_wdt_probe,
+};
+module_platform_driver(adv_wdt_drv);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ahc1ec0-wdt");
+MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Driver.");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_VERSION("1.0");
-- 
2.17.1

Re: [PATCH v7 6/7] Watchdog: ahc1ec0-wdt: Add sub-device Watchdog for Advantech embedded controller

[Guenter Roeck]

On Thu, May 06, 2021 at 04:16:18PM +0800, Campion Kang wrote:
> This is one of sub-device driver for Advantech embedded controller
> AHC1EC0. This driver provide Watchdog functionality for Advantech
> related applications to restart the system.
> 
> Changed in V7:
> 	Fix the patch according to reviewer's comment:
> 	- remove unnecessary variables and fix error checks
> 	- remove error messages that avoid clogging the kernel log
> 	- remove advwdt_notify_sys(), use watchdog subsystem to process
> 	  reboot or shutdown
> 	- delete mutex lock, the watchdog core has processed it
> 
> Changed in V6:
> 	- remove unnecessary header files
> 	- bug fixed: reboot halt if watchdog enabled
> 	- Kconfig: add "AHC1EC0" string to clearly define the EC name
> 
> Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
> ---
>  drivers/watchdog/Kconfig       |  11 +++
>  drivers/watchdog/Makefile      |   1 +
>  drivers/watchdog/ahc1ec0-wdt.c | 171 +++++++++++++++++++++++++++++++++
>  3 files changed, 183 insertions(+)
>  create mode 100644 drivers/watchdog/ahc1ec0-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 355100dad60a..5fe9add0a12d 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1005,6 +1005,17 @@ config ADVANTECH_WDT
>  	  feature. More information can be found at
>  	  <https://www.advantech.com.tw/products/>
>  
> +config AHC1EC0_WDT
> +	tristate "Advantech AHC1EC0 Watchdog Function"
> +	depends on MFD_AHC1EC0
> +	help
> +	  This is sub-device for Advantech AHC1EC0 embedded controller.
> +
> +	  This driver provide watchdog functionality for Advantech related
> +	  applications to restart the system.
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ahc1ec0-wdt.
> +
>  config ALIM1535_WDT
>  	tristate "ALi M1535 PMU Watchdog Timer"
>  	depends on X86 && PCI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a7eade8b4d45..0d78211e1412 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>  obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
>  obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
>  obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
> +obj-$(CONFIG_AHC1EC0_WDT) += ahc1ec0-wdt.o
>  
>  # M68K Architecture
>  obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
> diff --git a/drivers/watchdog/ahc1ec0-wdt.c b/drivers/watchdog/ahc1ec0-wdt.c
> new file mode 100644
> index 000000000000..efa955c41a81
> --- /dev/null
> +++ b/drivers/watchdog/ahc1ec0-wdt.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0-only

GPL or GPLv2 ?

> +/*
> + * Watchdog Driver for Advantech AHC1EC0 Embedded Controller
> + *
> + * Copyright 2021, Advantech IIoT Group
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>

Unnecessary.

> +#include <linux/platform_data/ahc1ec0.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>

Unnecessary.

> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +
> +struct ec_wdt_data {
> +	struct watchdog_device wdtdev;
> +	struct adv_ec_ddata *ddata;
> +	unsigned short timeout_in_ds; /* a decisecond */

This variable is unnecessary.

> +};
> +
> +#define EC_WDT_MIN_TIMEOUT	1   /* The watchdog devices minimum timeout value (in seconds). */
> +#define EC_WDT_MAX_TIMEOUT	600 /* The watchdog devices maximum timeout value (in seconds) */

Please use constant alignment, and "The watchdog devices " is really
unnecessary.

> +#define EC_WDT_DEFAULT_TIMEOUT	45
> +
> +static int set_delay(struct adv_ec_ddata *ddata, unsigned short delay_timeout_in_ms)
> +{
> +	if (ahc1ec_write_hw_ram(ddata, EC_RESET_DELAY_TIME_L,
> +				delay_timeout_in_ms & 0x00FF))
> +		return -EINVAL;
> +
> +	if (ahc1ec_write_hw_ram(ddata, EC_RESET_DELAY_TIME_H,
> +				(delay_timeout_in_ms & 0xFF00) >> 8))

Those functions presumably return a valid error code which should be passed
on and not replaced.

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ec_wdt_start(struct watchdog_device *wdd)
> +{
> +	int ret;
> +	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +	struct adv_ec_ddata *ddata;
> +
> +	ddata = ec_wdt_data->ddata;
> +

Might as well be written as
	struct adv_ec_ddata *ddata = ec_wdt_data->ddata;

> +	/*
> +	 * Because an unit of ahc1ec0_wdt is 0.1 seconds, timeout 100 is 10 seconds
> +	 */
> +	ec_wdt_data->timeout_in_ds = wdd->timeout * 10;

timeout_in_ds can be a local variable (if not omitted entirely).

> +
> +	ret = set_delay(ddata, (ec_wdt_data->timeout_in_ds - 1));

Unnecessary ( ) around second parameter.

> +	if (ret)
> +		goto exit;

	if (ret)
		return ret;

> +
> +	ahc1ec_write_hwram_command(ddata, EC_WDT_STOP);
> +	ret = ahc1ec_write_hwram_command(ddata, EC_WDT_START);
> +	if (ret)
> +		goto exit;

Unnecessary.

> +
> +exit:

Unnecessary label.

> +	return ret;
> +}
> +
> +static int ec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +	struct adv_ec_ddata *ddata;
> +
> +	ddata = ec_wdt_data->ddata;
> +
> +	return ahc1ec_write_hwram_command(ddata, EC_WDT_STOP);
> +}
> +
> +static int ec_wdt_ping(struct watchdog_device *wdd)
> +{
> +	int ret;
> +	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +	struct adv_ec_ddata *ddata;
> +
> +	ddata = ec_wdt_data->ddata;
> +
> +	ret = ahc1ec_write_hwram_command(ddata, EC_WDT_RESET);
> +	if (ret)
> +		return -EINVAL;

	return ret;

but then above you have
	return ahc1ec_write_hwram_command(ddata, EC_WDT_STOP);

Please be consistent.

> +
> +	return 0;
> +}
> +
> +static int ec_wdt_set_timeout(struct watchdog_device *wdd,
> +			      unsigned int timeout)
> +{
> +	wdd->timeout = timeout;
> +
> +	if (watchdog_active(wdd))
> +		return ec_wdt_start(wdd);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info ec_watchdog_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +	.identity = "AHC1EC0 Watchdog",
> +};
> +
> +static const struct watchdog_ops ec_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = ec_wdt_start,
> +	.stop = ec_wdt_stop,
> +	.ping = ec_wdt_ping,
> +	.set_timeout = ec_wdt_set_timeout,
> +};
> +
> +static int adv_ec_wdt_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct adv_ec_ddata *ddata;
> +	struct ec_wdt_data *ec_wdt_data;
> +	struct watchdog_device *wdd;
> +
> +	ddata = dev_get_drvdata(dev->parent);
> +	if (!ddata)
> +		return -EINVAL;
> +
> +	ec_wdt_data = devm_kzalloc(dev, sizeof(*ec_wdt_data), GFP_KERNEL);
> +	if (!ec_wdt_data)
> +		return -ENOMEM;
> +
> +	ec_wdt_data->ddata = ddata;
> +	wdd = &ec_wdt_data->wdtdev;
> +
> +	watchdog_init_timeout(&ec_wdt_data->wdtdev, 0, dev);
> +

This should be after wdd->timeout is set, and please use wdd.

> +	/* watchdog_set_nowayout(&ec_wdt_data->wdtdev, WATCHDOG_NOWAYOUT); */
> +	watchdog_set_drvdata(&ec_wdt_data->wdtdev, ec_wdt_data);

Please use wdd.

> +	platform_set_drvdata(pdev, ec_wdt_data);

Is this used anywhere ?

> +
> +	wdd->info = &ec_watchdog_info;
> +	wdd->ops = &ec_watchdog_ops;
> +	wdd->min_timeout = EC_WDT_MIN_TIMEOUT;
> +	wdd->max_timeout = EC_WDT_MAX_TIMEOUT;
> +	wdd->parent = dev;
> +
> +	ec_wdt_data->wdtdev.timeout = EC_WDT_DEFAULT_TIMEOUT;

	wdd->timeout = EC_WDT_DEFAULT_TIMEOUT;

> +
> +	watchdog_stop_on_reboot(wdd);
> +	watchdog_stop_on_unregister(wdd);
> +
> +	ret = devm_watchdog_register_device(dev, wdd);
> +	if (ret == 0)
> +		dev_info(dev, "ahc1ec0 watchdog register success\n");

This is noise.

> +
> +	return ret;
> +}
> +
> +static struct platform_driver adv_wdt_drv = {
> +	.driver = {
> +		.name = "ahc1ec0-wdt",
> +	},
> +	.probe = adv_ec_wdt_probe,
> +};
> +module_platform_driver(adv_wdt_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ahc1ec0-wdt");
> +MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Driver.");
> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_VERSION("1.0");
> -- 
> 2.17.1
> 

[PATCH v7 7/7] hwmon: ahc1ec0-hwmon: Add sub-device HWMON for Advantech embedded controller

[Campion Kang]

This is one of sub-device driver for Advantech embedded controller
AHC1EC0. This driver provides sysfs ABI for Advantech related
applications to monitor the system status.

Changed in V7:
	Fix the patch according to reviewer's comment:
	- add new document Documentation/hwmon/ahc1ec0-hwmon.rst to describe
	  the sensors attributes
	- pass the checking by checkpatch --strict command
	- remove unnecessary error checks
	- check channel account, return 0 if the second sensor is not
	  supported
	- make current sensor alone, not in hwmon sensors array

Changed in V6:
	- remove unnecessary header files
	- Using [devm_]hwmon_device_register_with_info() to register
	  HWMON driver based on reviewer's suggestion

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 Documentation/hwmon/ahc1ec0-hwmon.rst |  73 +++
 drivers/hwmon/Kconfig                 |  10 +
 drivers/hwmon/Makefile                |   1 +
 drivers/hwmon/ahc1ec0-hwmon.c         | 701 ++++++++++++++++++++++++++
 4 files changed, 785 insertions(+)
 create mode 100644 Documentation/hwmon/ahc1ec0-hwmon.rst
 create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c

diff --git a/Documentation/hwmon/ahc1ec0-hwmon.rst b/Documentation/hwmon/ahc1ec0-hwmon.rst
new file mode 100644
index 000000000000..7fcfb8b025d9
--- /dev/null
+++ b/Documentation/hwmon/ahc1ec0-hwmon.rst
@@ -0,0 +1,73 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver ahc1ec0-hwmon
+=================================
+
+Supported chips:
+
+ * Advantech AHC1 Embedded Controller Chip for Advantech Devices
+
+   Prefix: 'ahc1ec0-hwmon'
+
+   Datasheet: Datasheet is not publicly available.
+
+Author: Campion Kang <campion.kang@advantech.com.tw>
+
+
+Description
+-----------
+
+This driver adds the temperature, voltage, current support for the Advantech
+Devices with AHC1 Embedded Controller in Advantech IIoT Group.
+The AHC1EC0 firmware is responsible for sensor data sampling and recording in
+shared registers. The firmware is impleted by Advantech firmware team, it is
+a common design suitable for different hardware pins of Advantech devices.
+The host driver according to its hardware dynamic table and profile access its
+registers and exposes them to users as hwmon interfaces.
+
+The driver now is supports the AHC1EC0 for Advantech UNO, TPC series
+devices.
+
+Usage Notes
+-----------
+
+This driver will automatically probe and start via ahc1ec0 mfd driver
+according to the attributes in ACPI table or device tree. More detail settings
+you can refer the Documentation\devicetree\bindings\mfd\ahc1ec0.yaml.
+
+The ahc1ec0 driver will not probe automatic. You will have to instantiate
+devices explicitly. You can add it to /etc/modules.conf or insert module by
+the following command:
+
+	# insmod ahc1ec0
+
+
+Sysfs attributes
+----------------
+
+The following attributes are supported:
+
+- Advantech AHC1 Embedded Controller for Advantech UNO, TPC series:
+
+======================= =======================================================
+tempX_input             Temperature of the component (specified by tempX_label)
+tempX_crit              Temperature critical setpoint of the component
+temp1_label             "CPU Temp"
+temp2_label             "System Temp"
+
+inX_input               Measured voltage of the component (specified by
+                        inX_label and may different with devices)
+in0_label               "VBAT"
+in1_label               "5VSB"
+in2_label               "Vin"
+in3_label               "VCore"
+in4_label               "Vin1"
+in5_label               "Vin2"
+in6_label               "System Voltage"
+
+curr1_input             Measured current of Vin
+curr1_label             "Current"
+
+======================= =======================================================
+
+All the attributes are read-only.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 87624902ea80..242ea59e994b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2147,6 +2147,16 @@ config SENSORS_INTEL_M10_BMC_HWMON
 	  sensors monitor various telemetry data of different components on the
 	  card, e.g. board temperature, FPGA core temperature/voltage/current.
 
+config SENSORS_AHC1EC0_HWMON
+	tristate "Advantech AHC1EC0 Hardware Monitor Function"
+	depends on MFD_AHC1EC0
+	help
+	  This driver provide support for the hardware monitoring functionality
+	  for Advantech AHC1EC0 embedded controller on the board.
+
+	  This driver provides the sysfs attributes for applications to monitor
+	  the system status, including system temperatures, voltages, current.
+
 if ACPI
 
 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 59e78bc212cf..2df3381bf124 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_SENSORS_ADT7411)	+= adt7411.o
 obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
 obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
 obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
+obj-$(CONFIG_SENSORS_AHC1EC0_HWMON) += ahc1ec0-hwmon.o
 obj-$(CONFIG_SENSORS_AHT10)	+= aht10.o
 obj-$(CONFIG_SENSORS_AMD_ENERGY) += amd_energy.o
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
diff --git a/drivers/hwmon/ahc1ec0-hwmon.c b/drivers/hwmon/ahc1ec0-hwmon.c
new file mode 100644
index 000000000000..5502e645048b
--- /dev/null
+++ b/drivers/hwmon/ahc1ec0-hwmon.c
@@ -0,0 +1,701 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HWMON Driver for Advantech AHC1EC0 Embedded Controller
+ *
+ * Copyright 2021, Advantech IIoT Group
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/platform_data/ahc1ec0.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#define EC_ACPI_THERMAL1_LOCAL_TEMP	0x60
+#define EC_ACPI_THERMAL1_REMOTE_TEMP	0x61
+
+struct ec_hwmon_attrs {
+	const char		*name;
+	umode_t			mode;
+	int (*read)(struct device *dev, long *val);
+};
+
+struct adv_hwmon_profile {
+	int offset;
+	unsigned long resolution, resolution_vin, resolution_sys, resolution_curr, resolution_power;
+	unsigned long r1, r1_vin, r1_sys, r1_curr, r1_power;
+	unsigned long r2, r2_vin, r2_sys, r2_curr, r2_power;
+	int hwmon_in_list_cnt;
+	int curr_list_cnt;
+	int temp_list_cnt;
+	int *hwmon_in_list;
+	int *curr_list;
+	int *temp_list;
+};
+
+struct ec_hwmon_data {
+	struct device *dev;
+	struct device *hwmon_dev;
+	struct adv_ec_ddata *ddata;
+	unsigned long temperature[3];
+	unsigned long ec_current[5];
+	unsigned long power[5];
+	unsigned long voltage[7];
+
+	struct ec_hw_pin_table pin_tbl;
+	struct ec_smbuso_em0 ec_smboem0;
+	struct adv_hwmon_profile *profile;
+};
+
+enum ec_hwmon_in_type {
+	EC_HWMON_IN_VBAT,
+	EC_HWMON_IN_5VSB,
+	EC_HWMON_IN_12V,
+	EC_HWMON_IN_VCORE,
+	EC_HWMON_IN_VIN1,
+	EC_HWMON_IN_VIN2,
+	EC_HWMON_IN_SYS_VOL,
+};
+
+enum ec_curr_type {
+	EC_VIN_CURRENT,
+};
+
+enum ec_temp_type {
+	EC_TEMP_CPU,
+	EC_TEMP_SYS,
+};
+
+static int hwmon_in_list_0[] = {
+	EC_HWMON_IN_VBAT,
+	EC_HWMON_IN_5VSB,
+	EC_HWMON_IN_12V,
+	EC_HWMON_IN_VCORE,
+};
+
+static int hwmon_in_list_1[] = {
+	EC_HWMON_IN_VBAT,
+	EC_HWMON_IN_5VSB,
+	EC_HWMON_IN_12V,
+	EC_HWMON_IN_VCORE,
+};
+
+static int curr_list_0[] = {
+	EC_VIN_CURRENT,
+};
+
+static int temp_list_0[] = {
+	EC_TEMP_CPU,
+};
+
+static int temp_list_1[] = {
+	EC_TEMP_CPU,
+	EC_TEMP_SYS,
+};
+
+static struct adv_hwmon_profile advec_profile[] = {
+	/* [0] AHC1EC0_HWMON_PRO_TEMPLATE
+	 * The following Advantech hardware devices are for this configuration:
+	 *		TPC-8100TR, TPC-651T-E3AE, TPC-1251T-E3AE, TPC-1551T-E3AE,
+	 *		TPC-1751T-E3AE, TPC-1051WP-E3AE, TPC-1551WP-E3AE, TPC-1581WP-433AE,
+	 *		TPC-1782H-433AE, UNO-1483G-434AE, UNO-2483G-434AE, UNO-3483G-374AE,
+	 *		UNO-2473G, UNO-2484G-6???AE, UNO-2484G-7???AE, UNO-3283G-674AE,
+	 *		UNO-3285G-674AE
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
+		.hwmon_in_list = hwmon_in_list_0,
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.temp_list = temp_list_0,
+		.curr_list_cnt = ARRAY_SIZE(curr_list_0),
+		.curr_list = curr_list_0,
+	},
+	/* [1] AHC1EC0_HWMON_PRO_TPC5XXX
+	 * The following Advantech hardware devices are for 2nd configuration:
+	 *		TPC-B500-6??AE, TPC-5???T-6??AE, TPC-5???W-6??AE, TPC-B200-???AE,
+	 *		TPC-2???T-???AE, TPC-2???W-???AE
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.temp_list = temp_list_0,
+		.curr_list_cnt = 0,
+	},
+	/* [2] AHC1EC0_HWMON_PRO_PRVR4
+	 * The PR/VR4 devices are this configuration.
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list_cnt = ARRAY_SIZE(temp_list_1),
+		.temp_list = temp_list_1,
+		.curr_list_cnt = 0,
+	},
+	/* [3] AHC1EC0_HWMON_PRO_UNO2271G
+	 * The following Advantech hardware devices are using this configuration:
+	 *     UNO-2271G-E22AE/E23AE/E022AE/E023AE series and UNO-420 devices
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.temp_list = temp_list_0,
+		.curr_list_cnt = 0,
+	},
+};
+
+static void adv_ec_init_hwmon_profile(u32 profile, struct ec_hwmon_data *lmsensor_data)
+{
+	int i;
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+	struct ec_dynamic_table *dym_tbl = ddata->dym_tbl;
+
+	lmsensor_data->profile = &advec_profile[profile];
+
+	for (i = 0; i < EC_MAX_TBL_NUM ; i++) {
+		switch (dym_tbl[i].device_id) {
+		case EC_DID_CMOSBAT:
+			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vbat[1] = 1;
+			break;
+		case EC_DID_CMOSBAT_X2:
+			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vbat[1] = 2;
+			break;
+		case EC_DID_CMOSBAT_X10:
+			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vbat[1] = 10;
+			break;
+		case EC_DID_5VS0:
+		case EC_DID_5VS5:
+			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v5[1] = 1;
+			break;
+		case EC_DID_5VS0_X2:
+		case EC_DID_5VS5_X2:
+			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v5[1] = 2;
+			break;
+		case EC_DID_5VS0_X10:
+		case EC_DID_5VS5_X10:
+			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v5[1] = 10;
+			break;
+		case EC_DID_12VS0:
+			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v12[1] = 1;
+			break;
+		case EC_DID_12VS0_X2:
+			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v12[1] = 2;
+			break;
+		case EC_DID_12VS0_X10:
+			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
+			ptbl->v12[1] = 10;
+			break;
+		case EC_DID_VCOREA:
+		case EC_DID_VCOREB:
+			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vcore[1] = 1;
+			break;
+		case EC_DID_VCOREA_X2:
+		case EC_DID_VCOREB_X2:
+			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vcore[1] = 2;
+			break;
+		case EC_DID_VCOREA_X10:
+		case EC_DID_VCOREB_X10:
+			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vcore[1] = 10;
+			break;
+		case EC_DID_DC:
+			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vdc[1] = 1;
+			break;
+		case EC_DID_DC_X2:
+			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vdc[1] = 2;
+			break;
+		case EC_DID_DC_X10:
+			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
+			ptbl->vdc[1] = 10;
+			break;
+		case EC_DID_CURRENT:
+			ptbl->ec_current[0] = dym_tbl[i].hw_pin_num;
+			ptbl->ec_current[1] = 1;
+			break;
+		case EC_DID_SMBOEM0:
+			lmsensor_data->ec_smboem0.hw_pin_num = dym_tbl[i].hw_pin_num;
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+static int get_ec_in_vbat_input(struct device *dev, long *val)
+{
+	unsigned int temp = 0;
+	unsigned long voltage = 0;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_hwmon_profile *profile = lmsensor_data->profile;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	temp = ahc1ec_read_adc_value(ddata, ptbl->vbat[0], ptbl->vbat[1]);
+
+	if (profile->r2 != 0)
+		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
+
+	if (profile->resolution != 0)
+		voltage =  temp * profile->resolution / 1000 / 1000;
+
+	if (profile->offset != 0)
+		voltage += (int)profile->offset * 100;
+
+	lmsensor_data->voltage[0] = 10 * voltage;
+
+	*val = lmsensor_data->voltage[0];
+	return 0;
+}
+
+static int get_ec_in_v5_input(struct device *dev, long *val)
+{
+	unsigned int temp;
+	unsigned long voltage = 0;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_hwmon_profile *profile = lmsensor_data->profile;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	temp = ahc1ec_read_adc_value(ddata, ptbl->v5[0], ptbl->v5[1]);
+
+	if (profile->r2 != 0)
+		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
+
+	if (profile->resolution != 0)
+		voltage =  temp * profile->resolution / 1000 / 1000;
+
+	if (profile->offset != 0)
+		voltage += (int)profile->offset * 100;
+
+	lmsensor_data->voltage[1] = 10 * voltage;
+
+	*val = lmsensor_data->voltage[1];
+	return 0;
+}
+
+static int get_ec_in_v12_input(struct device *dev, long *val)
+{
+	int temp;
+	unsigned long voltage = 0;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_hwmon_profile *profile = lmsensor_data->profile;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	temp = ahc1ec_read_adc_value(ddata, ptbl->v12[0], ptbl->v12[1]);
+	if (temp == -1)
+		temp = ahc1ec_read_adc_value(ddata, ptbl->vdc[0], ptbl->vdc[1]);
+
+	if (profile->r2 != 0)
+		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
+
+	if (profile->resolution != 0)
+		voltage =  temp * profile->resolution / 1000 / 1000;
+
+	if (profile->offset != 0)
+		voltage += profile->offset * 100;
+
+	lmsensor_data->voltage[2] = 10 * voltage;
+
+	*val = lmsensor_data->voltage[2];
+	return 0;
+}
+
+static int get_ec_in_vcore_input(struct device *dev, long *val)
+{
+	int temp;
+	unsigned int voltage = 0;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_hwmon_profile *profile = lmsensor_data->profile;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	temp = ahc1ec_read_adc_value(ddata, ptbl->vcore[0], ptbl->vcore[1]);
+
+	if (profile->r2 != 0)
+		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
+
+	if (profile->resolution != 0)
+		voltage = temp * profile->resolution / 1000 / 1000;
+
+	if (profile->offset != 0)
+		voltage += profile->offset * 100;
+
+	lmsensor_data->voltage[3] = 10 * voltage;
+
+	*val = lmsensor_data->voltage[3];
+	return 0;
+}
+
+static int get_ec_current1_input(struct device *dev, long *val)
+{
+	int temp;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
+	struct adv_hwmon_profile *profile = lmsensor_data->profile;
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	temp = ahc1ec_read_adc_value(ddata, ptbl->ec_current[0], ptbl->ec_current[1]);
+
+	if (profile->r2 != 0)
+		temp = temp * (profile->r1 + profile->r2) / profile->r2;
+
+	if (profile->resolution != 0)
+		temp = temp * profile->resolution / 1000 / 1000;
+
+	if (profile->offset != 0)
+		temp += profile->offset * 100;
+
+	lmsensor_data->ec_current[3] = 10 * temp;
+
+	*val = lmsensor_data->ec_current[3];
+	return 0;
+}
+
+static int get_ec_cpu_temp(struct device *dev, long *val)
+{
+	int ret;
+	unsigned char value;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	ret = ahc1ec_read_acpi_value(ddata, EC_ACPI_THERMAL1_REMOTE_TEMP, &value);
+	if (!ret)
+		*val = 1000 * value;
+	return ret;
+}
+
+static int get_ec_sys_temp(struct device *dev, long *val)
+{
+	int ret;
+	unsigned char value;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
+
+	ret = ahc1ec_read_acpi_value(ddata, EC_ACPI_THERMAL1_LOCAL_TEMP, &value);
+	if (!ret)
+		*val = 1000 * value;
+	return ret;
+}
+
+const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
+	{"VBAT",	0444, get_ec_in_vbat_input},
+	{"5VSB",	0444, get_ec_in_v5_input},
+	{"Vin",		0444, get_ec_in_v12_input},
+	{"VCORE",	0444, get_ec_in_vcore_input},
+	{"Vin1",	0444, NULL},
+	{"Vin2",	0444, NULL},
+	{"System Voltage", 0444, NULL},
+	{"Current",	0444, get_ec_current1_input},
+};
+
+const struct ec_hwmon_attrs ec_curr_attr_template[] = {
+	{"Current",	0444, get_ec_current1_input},
+};
+
+const struct ec_hwmon_attrs ec_temp_attrs_template[] = {
+	{"CPU Temp",	0444, get_ec_cpu_temp},
+	{"System Temp",	0444, get_ec_sys_temp},
+};
+
+static int ahc1ec0_read_in(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+
+	if (attr == hwmon_in_input) {
+		int index = lmsensor_data->profile->hwmon_in_list[channel];
+		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_hwmon_in_attr_template[index];
+
+		return ec_hwmon_attr->read(dev, val);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int ahc1ec0_read_curr(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+
+	if (attr == hwmon_curr_input) {
+		int index = lmsensor_data->profile->curr_list[channel];
+		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_curr_attr_template[index];
+
+		return ec_hwmon_attr->read(dev, val);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int ahc1ec0_read_temp(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_temp_input: {
+		int index = lmsensor_data->profile->temp_list[channel];
+		const struct ec_hwmon_attrs *devec_hwmon_attr =
+			&ec_temp_attrs_template[index];
+
+		return devec_hwmon_attr->read(dev, val);
+	}
+	case hwmon_temp_crit:
+		/* both CPU temp and System temp are all this value */
+		*val = 100000;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int ahc1ec0_read_string(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, const char **str)
+{
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+
+	if (type == hwmon_in && attr == hwmon_in_label) {
+		int index = lmsensor_data->profile->hwmon_in_list[channel];
+		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_hwmon_in_attr_template[index];
+
+		*str = ec_hwmon_attr->name;
+		return 0;
+	}
+
+	if (type == hwmon_curr && attr == hwmon_curr_label) {
+		int index = lmsensor_data->profile->curr_list[channel];
+		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_curr_attr_template[index];
+
+		*str = ec_hwmon_attr->name;
+		return 0;
+	}
+
+	if (type == hwmon_temp && attr == hwmon_temp_label) {
+		int index = lmsensor_data->profile->temp_list[channel];
+		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_temp_attrs_template[index];
+
+		*str = ec_hwmon_attr->name;
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int ahc1ec0_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_in:
+		return ahc1ec0_read_in(dev, attr, channel, val);
+	case hwmon_curr:
+		return ahc1ec0_read_curr(dev, attr, channel, val);
+	case hwmon_temp:
+		return ahc1ec0_read_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t ec_hwmon_in_visible(const void *data, u32 attr, int channel)
+{
+	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
+
+	switch (attr) {
+	case hwmon_in_input:
+	case hwmon_in_label:
+		if (lmsensor_data->profile->hwmon_in_list_cnt > channel)
+			return 0444;
+		else
+			return 0;
+	default:
+		return 0;
+	}
+}
+
+static umode_t ec_curr_visible(const void *data, u32 attr, int channel)
+{
+	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
+
+	switch (attr) {
+	case hwmon_curr_input:
+	case hwmon_curr_label:
+		if (lmsensor_data->profile->curr_list_cnt > channel)
+			return 0444;
+		else
+			return 0;
+	default:
+		return 0;
+	}
+}
+
+static umode_t ec_temp_visible(const void *data, u32 attr, int channel)
+{
+	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_crit:
+	case hwmon_temp_label:
+		if (lmsensor_data->profile->temp_list_cnt > channel)
+			return 0444;
+		else
+			return 0;
+	default:
+		return 0;
+	}
+}
+
+static umode_t ahc1ec0_is_visible(const void *data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		return ec_hwmon_in_visible(data, attr, channel);
+	case hwmon_curr:
+		return ec_curr_visible(data, attr, channel);
+	case hwmon_temp:
+		return ec_temp_visible(data, attr, channel);
+	default:
+		return 0;
+	}
+}
+
+static const u32 ahc1ec0_in_config[] = {
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	HWMON_I_INPUT | HWMON_I_LABEL,
+	0
+};
+
+static const struct hwmon_channel_info ahc1ec0_in = {
+	.type = hwmon_in,
+	.config = ahc1ec0_in_config,
+};
+
+static const u32 ahc1ec0_curr_config[] = {
+	HWMON_C_INPUT | HWMON_C_LABEL,
+	0
+};
+
+static const struct hwmon_channel_info ahc1ec0_curr = {
+	.type = hwmon_curr,
+	.config = ahc1ec0_curr_config,
+};
+
+static const u32 ahc1ec0_temp_config[] = {
+	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,
+	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,
+	0
+};
+
+static const struct hwmon_channel_info ahc1ec0_temp = {
+	.type = hwmon_temp,
+	.config = ahc1ec0_temp_config,
+};
+
+static const struct hwmon_channel_info *ahc1ec0_info[] = {
+	&ahc1ec0_in,
+	&ahc1ec0_curr,
+	&ahc1ec0_temp,
+	NULL
+};
+
+static const struct hwmon_ops ahc1ec0_hwmon_ops = {
+	.is_visible = ahc1ec0_is_visible,
+	.read = ahc1ec0_read,
+	.read_string = ahc1ec0_read_string,
+};
+
+static const struct hwmon_chip_info ahc1ec0_chip_info = {
+	.ops = &ahc1ec0_hwmon_ops,
+	.info = ahc1ec0_info,
+};
+
+static int adv_ec_hwmon_probe(struct platform_device *pdev)
+{
+	int ret;
+	u32 profile;
+	struct device *dev = &pdev->dev;
+	struct adv_ec_ddata *ddata;
+	struct ec_hwmon_data *lmsensor_data;
+
+	ddata = dev_get_drvdata(dev->parent);
+	if (!ddata)
+		return -EINVAL;
+
+	ret = device_property_read_u32(dev->parent, "advantech,hwmon-profile", &profile);
+	if (ret < 0) {
+		dev_dbg(dev, "get hwmon-profile failed! (%d)", ret);
+		return ret;
+	}
+
+	if (profile >= ARRAY_SIZE(advec_profile)) {
+		dev_dbg(dev, "not support hwmon profile(%d)!\n", profile);
+		return -EINVAL;
+	}
+
+	lmsensor_data = devm_kzalloc(dev, sizeof(*lmsensor_data), GFP_KERNEL);
+	if (!lmsensor_data)
+		return -ENOMEM;
+
+	lmsensor_data->ddata = ddata;
+	lmsensor_data->dev = dev;
+	dev_set_drvdata(dev, lmsensor_data);
+
+	adv_ec_init_hwmon_profile(profile, lmsensor_data);
+
+	lmsensor_data->hwmon_dev  =
+		devm_hwmon_device_register_with_info(dev, "ahc1ec0.hwmon", lmsensor_data,
+						     &ahc1ec0_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(lmsensor_data->hwmon_dev);
+}
+
+static struct platform_driver adv_hwmon_drv = {
+	.driver = {
+		.name = "ahc1ec0-hwmon",
+	},
+	.probe = adv_ec_hwmon_probe,
+};
+module_platform_driver(adv_hwmon_drv);
+
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_ALIAS("platform:ahc1ec0-hwmon");
+MODULE_DESCRIPTION("Advantech Embedded Controller HWMON Driver.");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
+MODULE_VERSION("1.0");
-- 
2.17.1

Re: [PATCH v7 7/7] hwmon: ahc1ec0-hwmon: Add sub-device HWMON for Advantech embedded controller

[Guenter Roeck]

On Thu, May 06, 2021 at 04:16:19PM +0800, Campion Kang wrote:
> This is one of sub-device driver for Advantech embedded controller
> AHC1EC0. This driver provides sysfs ABI for Advantech related
> applications to monitor the system status.
> 
> Changed in V7:
> 	Fix the patch according to reviewer's comment:
> 	- add new document Documentation/hwmon/ahc1ec0-hwmon.rst to describe
> 	  the sensors attributes
> 	- pass the checking by checkpatch --strict command
> 	- remove unnecessary error checks
> 	- check channel account, return 0 if the second sensor is not
> 	  supported
> 	- make current sensor alone, not in hwmon sensors array
> 
> Changed in V6:
> 	- remove unnecessary header files
> 	- Using [devm_]hwmon_device_register_with_info() to register
> 	  HWMON driver based on reviewer's suggestion
> 
> Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
> ---
>  Documentation/hwmon/ahc1ec0-hwmon.rst |  73 +++
>  drivers/hwmon/Kconfig                 |  10 +
>  drivers/hwmon/Makefile                |   1 +
>  drivers/hwmon/ahc1ec0-hwmon.c         | 701 ++++++++++++++++++++++++++
>  4 files changed, 785 insertions(+)
>  create mode 100644 Documentation/hwmon/ahc1ec0-hwmon.rst
>  create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c
> 
> diff --git a/Documentation/hwmon/ahc1ec0-hwmon.rst b/Documentation/hwmon/ahc1ec0-hwmon.rst
> new file mode 100644
> index 000000000000..7fcfb8b025d9
> --- /dev/null
> +++ b/Documentation/hwmon/ahc1ec0-hwmon.rst
> @@ -0,0 +1,73 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ahc1ec0-hwmon
> +=================================
> +
> +Supported chips:
> +
> + * Advantech AHC1 Embedded Controller Chip for Advantech Devices
> +
> +   Prefix: 'ahc1ec0-hwmon'
> +
> +   Datasheet: Datasheet is not publicly available.
> +
> +Author: Campion Kang <campion.kang@advantech.com.tw>
> +
> +
> +Description
> +-----------
> +
> +This driver adds the temperature, voltage, current support for the Advantech
> +Devices with AHC1 Embedded Controller in Advantech IIoT Group.
> +The AHC1EC0 firmware is responsible for sensor data sampling and recording in
> +shared registers. The firmware is impleted by Advantech firmware team, it is
> +a common design suitable for different hardware pins of Advantech devices.
> +The host driver according to its hardware dynamic table and profile access its
> +registers and exposes them to users as hwmon interfaces.
> +
> +The driver now is supports the AHC1EC0 for Advantech UNO, TPC series
> +devices.
> +
> +Usage Notes
> +-----------
> +
> +This driver will automatically probe and start via ahc1ec0 mfd driver
> +according to the attributes in ACPI table or device tree. More detail settings
> +you can refer the Documentation\devicetree\bindings\mfd\ahc1ec0.yaml.
> +
> +The ahc1ec0 driver will not probe automatic. You will have to instantiate
> +devices explicitly. You can add it to /etc/modules.conf or insert module by
> +the following command:
> +
> +	# insmod ahc1ec0
> +
> +
> +Sysfs attributes
> +----------------
> +
> +The following attributes are supported:
> +
> +- Advantech AHC1 Embedded Controller for Advantech UNO, TPC series:
> +
> +======================= =======================================================
> +tempX_input             Temperature of the component (specified by tempX_label)
> +tempX_crit              Temperature critical setpoint of the component
> +temp1_label             "CPU Temp"
> +temp2_label             "System Temp"
> +
> +inX_input               Measured voltage of the component (specified by
> +                        inX_label and may different with devices)
> +in0_label               "VBAT"
> +in1_label               "5VSB"
> +in2_label               "Vin"
> +in3_label               "VCore"
> +in4_label               "Vin1"
> +in5_label               "Vin2"
> +in6_label               "System Voltage"
> +
> +curr1_input             Measured current of Vin
> +curr1_label             "Current"
> +
> +======================= =======================================================
> +
> +All the attributes are read-only.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 87624902ea80..242ea59e994b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2147,6 +2147,16 @@ config SENSORS_INTEL_M10_BMC_HWMON
>  	  sensors monitor various telemetry data of different components on the
>  	  card, e.g. board temperature, FPGA core temperature/voltage/current.
>  
> +config SENSORS_AHC1EC0_HWMON
> +	tristate "Advantech AHC1EC0 Hardware Monitor Function"
> +	depends on MFD_AHC1EC0
> +	help
> +	  This driver provide support for the hardware monitoring functionality
> +	  for Advantech AHC1EC0 embedded controller on the board.
> +
> +	  This driver provides the sysfs attributes for applications to monitor
> +	  the system status, including system temperatures, voltages, current.
> +
>  if ACPI
>  
>  comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 59e78bc212cf..2df3381bf124 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_SENSORS_ADT7411)	+= adt7411.o
>  obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
>  obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
>  obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
> +obj-$(CONFIG_SENSORS_AHC1EC0_HWMON) += ahc1ec0-hwmon.o
>  obj-$(CONFIG_SENSORS_AHT10)	+= aht10.o
>  obj-$(CONFIG_SENSORS_AMD_ENERGY) += amd_energy.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
> diff --git a/drivers/hwmon/ahc1ec0-hwmon.c b/drivers/hwmon/ahc1ec0-hwmon.c
> new file mode 100644
> index 000000000000..5502e645048b
> --- /dev/null
> +++ b/drivers/hwmon/ahc1ec0-hwmon.c
> @@ -0,0 +1,701 @@
> +// SPDX-License-Identifier: GPL-2.0-only

This does not match MODULE_LICENSE below.

> +/*
> + * HWMON Driver for Advantech AHC1EC0 Embedded Controller
> + *
> + * Copyright 2021, Advantech IIoT Group
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon-sysfs.h>

Unnecessary include.

> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/ahc1ec0.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#define EC_ACPI_THERMAL1_LOCAL_TEMP	0x60
> +#define EC_ACPI_THERMAL1_REMOTE_TEMP	0x61
> +
> +struct ec_hwmon_attrs {
> +	const char		*name;
> +	umode_t			mode;
> +	int (*read)(struct device *dev, long *val);
> +};
> +
> +struct adv_hwmon_profile {
> +	int offset;
> +	unsigned long resolution, resolution_vin, resolution_sys, resolution_curr, resolution_power;
> +	unsigned long r1, r1_vin, r1_sys, r1_curr, r1_power;
> +	unsigned long r2, r2_vin, r2_sys, r2_curr, r2_power;
> +	int hwmon_in_list_cnt;
> +	int curr_list_cnt;
> +	int temp_list_cnt;
> +	int *hwmon_in_list;
> +	int *curr_list;
> +	int *temp_list;
> +};
> +
> +struct ec_hwmon_data {
> +	struct device *dev;

Unnecessary.

> +	struct device *hwmon_dev;

Unnecessary.

> +	struct adv_ec_ddata *ddata;
> +	unsigned long temperature[3];

Unused.

> +	unsigned long ec_current[5];

ec_current[0] and ec_current[1] are profile variables, ec_current[3]
is written to but never read, the others are unused.

> +	unsigned long power[5];

Not used at all.

> +	unsigned long voltage[7];

Unnecessary array.

> +
> +	struct ec_hw_pin_table pin_tbl;

I don't see this structure used in any other driver but here.
Please declare it here.

> +	struct ec_smbuso_em0 ec_smboem0;

This is only written to and does not appear to be used.

> +	struct adv_hwmon_profile *profile;
> +};
> +
> +enum ec_hwmon_in_type {
> +	EC_HWMON_IN_VBAT,
> +	EC_HWMON_IN_5VSB,
> +	EC_HWMON_IN_12V,
> +	EC_HWMON_IN_VCORE,
> +	EC_HWMON_IN_VIN1,
> +	EC_HWMON_IN_VIN2,
> +	EC_HWMON_IN_SYS_VOL,
> +};

Some of the above values do not appear to be used anywhere.

> +
> +enum ec_curr_type {
> +	EC_VIN_CURRENT,
> +};
> +
> +enum ec_temp_type {
> +	EC_TEMP_CPU,
> +	EC_TEMP_SYS,
> +};
> +
> +static int hwmon_in_list_0[] = {
> +	EC_HWMON_IN_VBAT,
> +	EC_HWMON_IN_5VSB,
> +	EC_HWMON_IN_12V,
> +	EC_HWMON_IN_VCORE,
> +};
> +
> +static int hwmon_in_list_1[] = {
> +	EC_HWMON_IN_VBAT,
> +	EC_HWMON_IN_5VSB,
> +	EC_HWMON_IN_12V,
> +	EC_HWMON_IN_VCORE,
> +};
> +
> +static int curr_list_0[] = {
> +	EC_VIN_CURRENT,
> +};
> +
> +static int temp_list_0[] = {
> +	EC_TEMP_CPU,
> +};
> +
> +static int temp_list_1[] = {
> +	EC_TEMP_CPU,
> +	EC_TEMP_SYS,
> +};
> +
> +static struct adv_hwmon_profile advec_profile[] = {
> +	/* [0] AHC1EC0_HWMON_PRO_TEMPLATE
> +	 * The following Advantech hardware devices are for this configuration:
> +	 *		TPC-8100TR, TPC-651T-E3AE, TPC-1251T-E3AE, TPC-1551T-E3AE,
> +	 *		TPC-1751T-E3AE, TPC-1051WP-E3AE, TPC-1551WP-E3AE, TPC-1581WP-433AE,
> +	 *		TPC-1782H-433AE, UNO-1483G-434AE, UNO-2483G-434AE, UNO-3483G-374AE,
> +	 *		UNO-2473G, UNO-2484G-6???AE, UNO-2484G-7???AE, UNO-3283G-674AE,
> +	 *		UNO-3285G-674AE
> +	 */

/*
 * This is not the networking subsystem.
 * Please use standfard multi-line comments.
 */

> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
> +		.hwmon_in_list = hwmon_in_list_0,
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.temp_list = temp_list_0,
> +		.curr_list_cnt = ARRAY_SIZE(curr_list_0),
> +		.curr_list = curr_list_0,
> +	},
> +	/* [1] AHC1EC0_HWMON_PRO_TPC5XXX
> +	 * The following Advantech hardware devices are for 2nd configuration:
> +	 *		TPC-B500-6??AE, TPC-5???T-6??AE, TPC-5???W-6??AE, TPC-B200-???AE,
> +	 *		TPC-2???T-???AE, TPC-2???W-???AE
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.temp_list = temp_list_0,
> +		.curr_list_cnt = 0,
> +	},
> +	/* [2] AHC1EC0_HWMON_PRO_PRVR4
> +	 * The PR/VR4 devices are this configuration.
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_1),
> +		.temp_list = temp_list_1,
> +		.curr_list_cnt = 0,
> +	},
> +	/* [3] AHC1EC0_HWMON_PRO_UNO2271G
> +	 * The following Advantech hardware devices are using this configuration:
> +	 *     UNO-2271G-E22AE/E23AE/E022AE/E023AE series and UNO-420 devices
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.temp_list = temp_list_0,
> +		.curr_list_cnt = 0,

0 initializers are unnecessary.

> +	},
> +};
> +
> +static void adv_ec_init_hwmon_profile(u32 profile, struct ec_hwmon_data *lmsensor_data)
> +{
> +	int i;
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +	struct ec_dynamic_table *dym_tbl = ddata->dym_tbl;
> +
> +	lmsensor_data->profile = &advec_profile[profile];
> +
> +	for (i = 0; i < EC_MAX_TBL_NUM ; i++) {
> +		switch (dym_tbl[i].device_id) {
> +		case EC_DID_CMOSBAT:
> +			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vbat[1] = 1;
> +			break;
> +		case EC_DID_CMOSBAT_X2:
> +			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vbat[1] = 2;
> +			break;
> +		case EC_DID_CMOSBAT_X10:
> +			ptbl->vbat[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vbat[1] = 10;
> +			break;
> +		case EC_DID_5VS0:
> +		case EC_DID_5VS5:
> +			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v5[1] = 1;
> +			break;
> +		case EC_DID_5VS0_X2:
> +		case EC_DID_5VS5_X2:
> +			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v5[1] = 2;
> +			break;
> +		case EC_DID_5VS0_X10:
> +		case EC_DID_5VS5_X10:
> +			ptbl->v5[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v5[1] = 10;
> +			break;
> +		case EC_DID_12VS0:
> +			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v12[1] = 1;
> +			break;
> +		case EC_DID_12VS0_X2:
> +			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v12[1] = 2;
> +			break;
> +		case EC_DID_12VS0_X10:
> +			ptbl->v12[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->v12[1] = 10;
> +			break;
> +		case EC_DID_VCOREA:
> +		case EC_DID_VCOREB:
> +			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vcore[1] = 1;
> +			break;
> +		case EC_DID_VCOREA_X2:
> +		case EC_DID_VCOREB_X2:
> +			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vcore[1] = 2;
> +			break;
> +		case EC_DID_VCOREA_X10:
> +		case EC_DID_VCOREB_X10:
> +			ptbl->vcore[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vcore[1] = 10;
> +			break;
> +		case EC_DID_DC:
> +			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vdc[1] = 1;
> +			break;
> +		case EC_DID_DC_X2:
> +			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vdc[1] = 2;
> +			break;
> +		case EC_DID_DC_X10:
> +			ptbl->vdc[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->vdc[1] = 10;
> +			break;
> +		case EC_DID_CURRENT:
> +			ptbl->ec_current[0] = dym_tbl[i].hw_pin_num;
> +			ptbl->ec_current[1] = 1;
> +			break;
> +		case EC_DID_SMBOEM0:
> +			lmsensor_data->ec_smboem0.hw_pin_num = dym_tbl[i].hw_pin_num;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +static int get_ec_in_vbat_input(struct device *dev, long *val)
> +{
> +	unsigned int temp = 0;

Unnecessary initialization.

> +	unsigned long voltage = 0;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	temp = ahc1ec_read_adc_value(ddata, ptbl->vbat[0], ptbl->vbat[1]);
> +
> +	if (profile->r2 != 0)
> +		voltage = temp * (profile->r1 + profile->r2) / profile->r2;

r2 is always != 0.

> +
> +	if (profile->resolution != 0)
> +		voltage =  temp * profile->resolution / 1000 / 1000;
> +
> +	if (profile->offset != 0)
> +		voltage += (int)profile->offset * 100;
> +
> +	lmsensor_data->voltage[0] = 10 * voltage;

This results in unnecessary loss of resolution. Please fix the calculations
to avoid it.

> +
> +	*val = lmsensor_data->voltage[0];
> +	return 0;
> +}
> +
> +static int get_ec_in_v5_input(struct device *dev, long *val)
> +{
> +	unsigned int temp;
> +	unsigned long voltage = 0;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	temp = ahc1ec_read_adc_value(ddata, ptbl->v5[0], ptbl->v5[1]);
> +
> +	if (profile->r2 != 0)
> +		voltage = temp * (profile->r1 + profile->r2) / profile->r2;

r2 is always != 0.

> +
> +	if (profile->resolution != 0)
> +		voltage =  temp * profile->resolution / 1000 / 1000;
> +
> +	if (profile->offset != 0)
> +		voltage += (int)profile->offset * 100;
> +
> +	lmsensor_data->voltage[1] = 10 * voltage;
> +
> +	*val = lmsensor_data->voltage[1];
> +	return 0;
> +}

All those read functions pretty much repeat the same code.
Please add a helper function which does all the common stuff.

> +
> +static int get_ec_in_v12_input(struct device *dev, long *val)
> +{
> +	int temp;
> +	unsigned long voltage = 0;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	temp = ahc1ec_read_adc_value(ddata, ptbl->v12[0], ptbl->v12[1]);
> +	if (temp == -1)
> +		temp = ahc1ec_read_adc_value(ddata, ptbl->vdc[0], ptbl->vdc[1]);
> +
> +	if (profile->r2 != 0)
> +		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
> +
Same everywhere.

> +	if (profile->resolution != 0)
> +		voltage =  temp * profile->resolution / 1000 / 1000;
> +
> +	if (profile->offset != 0)
> +		voltage += profile->offset * 100;
> +
> +	lmsensor_data->voltage[2] = 10 * voltage;
> +
> +	*val = lmsensor_data->voltage[2];
> +	return 0;
> +}
> +
> +static int get_ec_in_vcore_input(struct device *dev, long *val)
> +{
> +	int temp;
> +	unsigned int voltage = 0;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	temp = ahc1ec_read_adc_value(ddata, ptbl->vcore[0], ptbl->vcore[1]);
> +
> +	if (profile->r2 != 0)
> +		voltage = temp * (profile->r1 + profile->r2) / profile->r2;
> +
> +	if (profile->resolution != 0)
> +		voltage = temp * profile->resolution / 1000 / 1000;
> +
> +	if (profile->offset != 0)
> +		voltage += profile->offset * 100;
> +
> +	lmsensor_data->voltage[3] = 10 * voltage;
> +
> +	*val = lmsensor_data->voltage[3];
> +	return 0;
> +}
> +
> +static int get_ec_current1_input(struct device *dev, long *val)
> +{
> +	int temp;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	temp = ahc1ec_read_adc_value(ddata, ptbl->ec_current[0], ptbl->ec_current[1]);

This function returns a negative error code which needs to be checked.

> +
> +	if (profile->r2 != 0)
> +		temp = temp * (profile->r1 + profile->r2) / profile->r2;
> +
> +	if (profile->resolution != 0)
> +		temp = temp * profile->resolution / 1000 / 1000;
> +
> +	if (profile->offset != 0)
> +		temp += profile->offset * 100;
> +
> +	lmsensor_data->ec_current[3] = 10 * temp;
> +
> +	*val = lmsensor_data->ec_current[3];
> +	return 0;
> +}
> +
> +static int get_ec_cpu_temp(struct device *dev, long *val)
> +{
> +	int ret;
> +	unsigned char value;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	ret = ahc1ec_read_acpi_value(ddata, EC_ACPI_THERMAL1_REMOTE_TEMP, &value);
> +	if (!ret)
> +		*val = 1000 * value;
> +	return ret;
> +}
> +
> +static int get_ec_sys_temp(struct device *dev, long *val)
> +{
> +	int ret;
> +	unsigned char value;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct adv_ec_ddata *ddata = lmsensor_data->ddata;
> +
> +	ret = ahc1ec_read_acpi_value(ddata, EC_ACPI_THERMAL1_LOCAL_TEMP, &value);

This function could return both the error code and the value without extra
pointer to the result.

> +	if (!ret)
> +		*val = 1000 * value;
> +	return ret;

This should be
	if (ret)
		return ret;
	*val = ...
	return 0;

> +}

Those two functions can be simplified into one with the sensor
as additional parameter.

> +
> +const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
> +	{"VBAT",	0444, get_ec_in_vbat_input},
> +	{"5VSB",	0444, get_ec_in_v5_input},
> +	{"Vin",		0444, get_ec_in_v12_input},
> +	{"VCORE",	0444, get_ec_in_vcore_input},
> +	{"Vin1",	0444, NULL},
> +	{"Vin2",	0444, NULL},
> +	{"System Voltage", 0444, NULL},

What is the point of above three entries ?

> +	{"Current",	0444, get_ec_current1_input},
> +};
> +
> +const struct ec_hwmon_attrs ec_curr_attr_template[] = {
> +	{"Current",	0444, get_ec_current1_input},
> +};
> +
> +const struct ec_hwmon_attrs ec_temp_attrs_template[] = {
> +	{"CPU Temp",	0444, get_ec_cpu_temp},
> +	{"System Temp",	0444, get_ec_sys_temp},
> +};
> +
> +static int ahc1ec0_read_in(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +	if (attr == hwmon_in_input) {
> +		int index = lmsensor_data->profile->hwmon_in_list[channel];
> +		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_hwmon_in_attr_template[index];
> +
> +		return ec_hwmon_attr->read(dev, val);
> +	}

There is only one attribute, so the if check is really unnecessary.

This entire code could be rearranged to not require the additional
indirect read function, by providing appropriate parameters to a single
function. This would reduce code size significantly. Please do that.

> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ahc1ec0_read_curr(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +	if (attr == hwmon_curr_input) {
> +		int index = lmsensor_data->profile->curr_list[channel];
> +		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_curr_attr_template[index];
> +
> +		return ec_hwmon_attr->read(dev, val);
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ahc1ec0_read_temp(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_input: {
> +		int index = lmsensor_data->profile->temp_list[channel];
> +		const struct ec_hwmon_attrs *devec_hwmon_attr =
> +			&ec_temp_attrs_template[index];
> +
> +		return devec_hwmon_attr->read(dev, val);
> +	}
> +	case hwmon_temp_crit:
> +		/* both CPU temp and System temp are all this value */
> +		*val = 100000;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int ahc1ec0_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, const char **str)
> +{
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_in && attr == hwmon_in_label) {
> +		int index = lmsensor_data->profile->hwmon_in_list[channel];
> +		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_hwmon_in_attr_template[index];
> +
> +		*str = ec_hwmon_attr->name;
> +		return 0;
> +	}
> +
> +	if (type == hwmon_curr && attr == hwmon_curr_label) {
> +		int index = lmsensor_data->profile->curr_list[channel];
> +		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_curr_attr_template[index];
> +
> +		*str = ec_hwmon_attr->name;
> +		return 0;
> +	}
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_label) {
> +		int index = lmsensor_data->profile->temp_list[channel];
> +		const struct ec_hwmon_attrs *ec_hwmon_attr = &ec_temp_attrs_template[index];
> +
> +		*str = ec_hwmon_attr->name;
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ahc1ec0_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ahc1ec0_read_in(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return ahc1ec0_read_curr(dev, attr, channel, val);
> +	case hwmon_temp:
> +		return ahc1ec0_read_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t ec_hwmon_in_visible(const void *data, u32 attr, int channel)
> +{
> +	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +	case hwmon_in_label:
> +		if (lmsensor_data->profile->hwmon_in_list_cnt > channel)

This is confusing. Please use
		if (channel < ...)

> +			return 0444;
> +		else
> +			return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t ec_curr_visible(const void *data, u32 attr, int channel)
> +{
> +	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
> +
> +	switch (attr) {
> +	case hwmon_curr_input:
> +	case hwmon_curr_label:
> +		if (lmsensor_data->profile->curr_list_cnt > channel)
> +			return 0444;
> +		else
> +			return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t ec_temp_visible(const void *data, u32 attr, int channel)
> +{
> +	struct ec_hwmon_data *lmsensor_data = (struct ec_hwmon_data *)data;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_label:
> +		if (lmsensor_data->profile->temp_list_cnt > channel)
> +			return 0444;
> +		else
> +			return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t ahc1ec0_is_visible(const void *data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ec_hwmon_in_visible(data, attr, channel);
> +	case hwmon_curr:
> +		return ec_curr_visible(data, attr, channel);
> +	case hwmon_temp:
> +		return ec_temp_visible(data, attr, channel);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const u32 ahc1ec0_in_config[] = {
> +	HWMON_I_INPUT | HWMON_I_LABEL,
> +	HWMON_I_INPUT | HWMON_I_LABEL,
> +	HWMON_I_INPUT | HWMON_I_LABEL,
> +	HWMON_I_INPUT | HWMON_I_LABEL,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ahc1ec0_in = {
> +	.type = hwmon_in,
> +	.config = ahc1ec0_in_config,
> +};
> +
> +static const u32 ahc1ec0_curr_config[] = {
> +	HWMON_C_INPUT | HWMON_C_LABEL,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ahc1ec0_curr = {
> +	.type = hwmon_curr,
> +	.config = ahc1ec0_curr_config,
> +};
> +
> +static const u32 ahc1ec0_temp_config[] = {
> +	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,
> +	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ahc1ec0_temp = {
> +	.type = hwmon_temp,
> +	.config = ahc1ec0_temp_config,
> +};
> +
> +static const struct hwmon_channel_info *ahc1ec0_info[] = {
> +	&ahc1ec0_in,
> +	&ahc1ec0_curr,
> +	&ahc1ec0_temp,
> +	NULL
> +};

All the above can be simplified by using the HWMON_CHANNEL_INFO()
macro.

> +
> +static const struct hwmon_ops ahc1ec0_hwmon_ops = {
> +	.is_visible = ahc1ec0_is_visible,
> +	.read = ahc1ec0_read,
> +	.read_string = ahc1ec0_read_string,
> +};
> +
> +static const struct hwmon_chip_info ahc1ec0_chip_info = {
> +	.ops = &ahc1ec0_hwmon_ops,
> +	.info = ahc1ec0_info,
> +};
> +
> +static int adv_ec_hwmon_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	u32 profile;
> +	struct device *dev = &pdev->dev;
> +	struct adv_ec_ddata *ddata;
> +	struct ec_hwmon_data *lmsensor_data;
> +
> +	ddata = dev_get_drvdata(dev->parent);
> +	if (!ddata)
> +		return -EINVAL;
> +
> +	ret = device_property_read_u32(dev->parent, "advantech,hwmon-profile", &profile);
> +	if (ret < 0) {
> +		dev_dbg(dev, "get hwmon-profile failed! (%d)", ret);
> +		return ret;
> +	}
> +
> +	if (profile >= ARRAY_SIZE(advec_profile)) {
> +		dev_dbg(dev, "not support hwmon profile(%d)!\n", profile);

s/not support/unsupported/

> +		return -EINVAL;
> +	}
> +
> +	lmsensor_data = devm_kzalloc(dev, sizeof(*lmsensor_data), GFP_KERNEL);
> +	if (!lmsensor_data)
> +		return -ENOMEM;
> +
> +	lmsensor_data->ddata = ddata;
> +	lmsensor_data->dev = dev;
> +	dev_set_drvdata(dev, lmsensor_data);
> +
> +	adv_ec_init_hwmon_profile(profile, lmsensor_data);
> +
> +	lmsensor_data->hwmon_dev  =
> +		devm_hwmon_device_register_with_info(dev, "ahc1ec0.hwmon", lmsensor_data,
> +						     &ahc1ec0_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(lmsensor_data->hwmon_dev);
> +}
> +
> +static struct platform_driver adv_hwmon_drv = {
> +	.driver = {
> +		.name = "ahc1ec0-hwmon",
> +	},
> +	.probe = adv_ec_hwmon_probe,
> +};
> +module_platform_driver(adv_hwmon_drv);
> +
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("platform:ahc1ec0-hwmon");
> +MODULE_DESCRIPTION("Advantech Embedded Controller HWMON Driver.");
> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
> +MODULE_VERSION("1.0");
> -- 
> 2.17.1
> 

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Hans de Goede]

Hi,

I'm replying here since this series has no cover-letter, for
the next version for a series touching so many different
sub-systems it would be good to start with a cover-letter
providing some background info on the series.

I see this is binding to an ACPI device, yet it is also using
devicetree bindings and properties.

So I take it this means that your ACPI tables are using the
optional capability of embedded device-tree blobs inside the
ACPI tables ?

That is an unusual combination on a x86 device, note it is
not wrong but AFAIK you are the first to do this on x86.

Other then that question, for the next version please give
all commits a proper commit message and not just a short
1 line Subject and put the changelog after a scissors line
after your Signed-off-by like this:

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
Changed in V7:
1. According to the reviewer's comment, add two files:
   Documentation/hwmon/ahc1ec0-hwmon.rst and
   drivers/platform/x86/ahc1ec0-core.c

And please also include older changelog entries.

Regards,

Hans
 




On 5/6/21 10:16 AM, Campion Kang wrote:
> Changed in V7:
> 	1. According to the reviewer's comment, add two files:
> 	   Documentation/hwmon/ahc1ec0-hwmon.rst and
> 	   drivers/platform/x86/ahc1ec0-core.c
> 
> Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
> ---
>  MAINTAINERS | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83c2b1867586..984795eb6b5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -572,6 +572,19 @@ S:	Maintained
>  F:	Documentation/scsi/advansys.rst
>  F:	drivers/scsi/advansys.c
>  
> +ADVANTECH AHC1EC0 EMBEDDED CONTROLLER DRIVER
> +M:	Campion Kang <campion.kang@advantech.com.tw>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> +F:	Documentation/hwmon/ahc1ec0-hwmon.rst
> +F:	drivers/hwmon/ahc1ec0-hwmon.c
> +F:	drivers/mfd/ahc1ec0.c
> +F:	drivers/platform/x86/ahc1ec0-core.c
> +F:	drivers/watchdog/ahc1ec0-wdt.c
> +F:	include/dt-bindings/mfd/ahc1ec0-dt.h
> +F:	include/linux/platform_data/ahc1ec0.h
> +
>  ADVANTECH SWBTN DRIVER
>  M:	Andrea Ho <Andrea.Ho@advantech.com.tw>
>  L:	platform-driver-x86@vger.kernel.org
> 

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Andy Shevchenko]

On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> wrote:
> I'm replying here since this series has no cover-letter, for
> the next version for a series touching so many different
> sub-systems it would be good to start with a cover-letter
> providing some background info on the series.
>
> I see this is binding to an ACPI device, yet it is also using
> devicetree bindings and properties.
>
> So I take it this means that your ACPI tables are using the
> optional capability of embedded device-tree blobs inside the
> ACPI tables ?
>
> That is an unusual combination on a x86 device, note it is
> not wrong

It's actually not okay. We have agreed at some point with DT people,
that ACPI should not use non-native variants of natively supported
things. For example, it shouldn't use "interrupt" property for IOxAPIC
(or xIC) provided interrupts, rather Interrupt() has to be used and so
on.

> but AFAIK you are the first to do this on x86.

No, not the first. Once Intel tried to invent the pin control
configuration and muxing properties in ACPI, it was luckily rejected
(ACPI 6.x OTOH provides a set of special resources for that).

So, NAK from me, *if* it's really the case. ACPI tables must be revisited.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Hans de Goede]

Hi,

On 5/6/21 11:23 AM, Andy Shevchenko wrote:
> On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> I'm replying here since this series has no cover-letter, for
>> the next version for a series touching so many different
>> sub-systems it would be good to start with a cover-letter
>> providing some background info on the series.
>>
>> I see this is binding to an ACPI device, yet it is also using
>> devicetree bindings and properties.
>>
>> So I take it this means that your ACPI tables are using the
>> optional capability of embedded device-tree blobs inside the
>> ACPI tables ?
>>
>> That is an unusual combination on a x86 device, note it is
>> not wrong
> 
> It's actually not okay. We have agreed at some point with DT people,
> that ACPI should not use non-native variants of natively supported
> things. For example, it shouldn't use "interrupt" property for IOxAPIC
> (or xIC) provided interrupts, rather Interrupt() has to be used and so
> on.

Right, but that is not the case here, they are using 2 device-tree
properties (1), from patch 3/7:

+properties:
+  compatible:
+    const: advantech,ahc1ec0
+
+  advantech,hwmon-profile:
+    description:
+      The number of sub-devices specified in the platform. Defines for the
+      hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  advantech,has-watchdog:
+    description:
+      Some implementations of the EC include a watchdog used to monitor the
+      system. This boolean flag is used to specify whether this watchdog is
+      present or not. Default is true, otherwise set to false.
+    type: boolean


>> but AFAIK you are the first to do this on x86.
> 
> No, not the first. Once Intel tried to invent the pin control
> configuration and muxing properties in ACPI, it was luckily rejected
> (ACPI 6.x OTOH provides a set of special resources for that).
> 
> So, NAK from me, *if* it's really the case. ACPI tables must be revisited.

AFAIK Advantech are not defining things for which an ACPI standard exists,
although these 2 properties might just as well may be 2 simple ACPI integer
methods, which would actually make things a bit simpler (.e.g it would
allow dropping patch 2/7 and 3/7 from the set).

Campion, any reason why you went this route; and can the ACPI tables
still be changed? 

Regards,

Hans

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Andy Shevchenko]

On Thu, May 6, 2021 at 12:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/6/21 11:23 AM, Andy Shevchenko wrote:

...

> Campion, any reason why you went this route; and can the ACPI tables
> still be changed?

Yes, the main problem with the series is the absence of a cover letter
that should basically answer the (obvious) questions and give a
justification.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Campion Kang]

Hi, Very thanks your time for reviewing.

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Thursday, May 6, 2021 5:39 PM
>Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded
>controller entry
>
>Hi,
>
>On 5/6/21 11:23 AM, Andy Shevchenko wrote:
>> On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com>
>wrote:
>>> I'm replying here since this series has no cover-letter, for
>>> the next version for a series touching so many different
>>> sub-systems it would be good to start with a cover-letter
>>> providing some background info on the series.
>>>

Sorry about that, i will study what is cover-letter and its content.
Would you kindly provide me a good reference?
Can I resend a [Patch v7 0/7] for these patch or provide it in next version?


>>> I see this is binding to an ACPI device, yet it is also using
>>> devicetree bindings and properties.
>>>
>>> So I take it this means that your ACPI tables are using the
>>> optional capability of embedded device-tree blobs inside the
>>> ACPI tables ?
>>>
>>> That is an unusual combination on a x86 device, note it is
>>> not wrong
>>
>> It's actually not okay. We have agreed at some point with DT people,
>> that ACPI should not use non-native variants of natively supported
>> things. For example, it shouldn't use "interrupt" property for IOxAPIC
>> (or xIC) provided interrupts, rather Interrupt() has to be used and so
>> on.

In our experience, most risc platforms are using devicetree, and x86/64 platforms
are using ACPI table or grub configure for their specific settings in different HW paltform.
In this case, EC chip is a LPC interface that can be integrated in whenever risc or x86/64.
So in my understand, I think it is not conflict.
(please correct me if i am misunderstanding, i will try to describe more)

If the EC chip is connected to the risc processor, we will bind its properties in the device-tree without modifing the source.
If the EC chip is connected to the X86/64 processor, we bind its the properties in the ACPI table and also without modifing the source.
Why do we need to bind the properties in ACPI or in the device-tree? Because it is an LPC interface, it cannot automatically load the driver like a USB or PCI device.
In the early days, we had to install the EC driver module in our HW platform and manually load it at every boot. Different Advantech HW platforms have different properties for HWMON and others sub-systems. This causes the EC source to be a bit dirty. It is necessary to obtain the hardware platform name from the BIOS DMI table and determine its attributes according to its platform name.
Now bind the attributes to ACPI table or device-tree, the EC source is more clear and universal for Advantech devices, and it is important that if the ACPI table matches, it can be automatically loaded.

>
>Right, but that is not the case here, they are using 2 device-tree
>properties (1), from patch 3/7:
>
>+properties:
>+  compatible:
>+    const: advantech,ahc1ec0
>+
>+  advantech,hwmon-profile:
>+    description:
>+      The number of sub-devices specified in the platform. Defines for the
>+      hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+    maxItems: 1
>+
>+  advantech,has-watchdog:
>+    description:
>+      Some implementations of the EC include a watchdog used to monitor
>the
>+      system. This boolean flag is used to specify whether this watchdog is
>+      present or not. Default is true, otherwise set to false.
>+    type: boolean
>
>
>>> but AFAIK you are the first to do this on x86.
>>
>> No, not the first. Once Intel tried to invent the pin control
>> configuration and muxing properties in ACPI, it was luckily rejected
>> (ACPI 6.x OTOH provides a set of special resources for that).
>>
>> So, NAK from me, *if* it's really the case. ACPI tables must be revisited.
>

I am not sure it supports vendor self-defined attributes for ACPI table?

>AFAIK Advantech are not defining things for which an ACPI standard exists,
>although these 2 properties might just as well may be 2 simple ACPI integer
>methods, which would actually make things a bit simpler (.e.g it would
>allow dropping patch 2/7 and 3/7 from the set).
>
>Campion, any reason why you went this route; and can the ACPI tables
>still be changed?
>

If patches 2/7 and 3/7 are removed, it will be even simpler.
This means that there is no device-tree binding designed, in fact, the EC chip only be integrated in the x86/64 platform at present.
Sorry, ACPI table now is integrated in the BIOS for Advantech UNO device, 
it may be revert to previous rule, that is, there is no ACPI table binding and manually loaded the EC driver. If you have any suggestons I would be very grateful.

>Regards,
>
>Hans

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Hans de Goede]

Hi,

On 5/7/21 1:53 PM, Campion Kang wrote:
> Hi, Very thanks your time for reviewing.
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Thursday, May 6, 2021 5:39 PM
>> Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded
>> controller entry
>>
>> Hi,
>>
>> On 5/6/21 11:23 AM, Andy Shevchenko wrote:
>>> On Thu, May 6, 2021 at 11:48 AM Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>> I'm replying here since this series has no cover-letter, for
>>>> the next version for a series touching so many different
>>>> sub-systems it would be good to start with a cover-letter
>>>> providing some background info on the series.
>>>>
> 
> Sorry about that, i will study what is cover-letter and its content.
> Would you kindly provide me a good reference?
> Can I resend a [Patch v7 0/7] for these patch or provide it in next version?

Please add a cover letter to the next version, which will hopefully
also address some of the other remarks already made.

Regards,

Hans


> 
> 
>>>> I see this is binding to an ACPI device, yet it is also using
>>>> devicetree bindings and properties.
>>>>
>>>> So I take it this means that your ACPI tables are using the
>>>> optional capability of embedded device-tree blobs inside the
>>>> ACPI tables ?
>>>>
>>>> That is an unusual combination on a x86 device, note it is
>>>> not wrong
>>>
>>> It's actually not okay. We have agreed at some point with DT people,
>>> that ACPI should not use non-native variants of natively supported
>>> things. For example, it shouldn't use "interrupt" property for IOxAPIC
>>> (or xIC) provided interrupts, rather Interrupt() has to be used and so
>>> on.
> 
> In our experience, most risc platforms are using devicetree, and x86/64 platforms
> are using ACPI table or grub configure for their specific settings in different HW paltform.
> In this case, EC chip is a LPC interface that can be integrated in whenever risc or x86/64.
> So in my understand, I think it is not conflict.
> (please correct me if i am misunderstanding, i will try to describe more)
> 
> If the EC chip is connected to the risc processor, we will bind its properties in the device-tree without modifing the source.
> If the EC chip is connected to the X86/64 processor, we bind its the properties in the ACPI table and also without modifing the source.
> Why do we need to bind the properties in ACPI or in the device-tree? Because it is an LPC interface, it cannot automatically load the driver like a USB or PCI device.
> In the early days, we had to install the EC driver module in our HW platform and manually load it at every boot. Different Advantech HW platforms have different properties for HWMON and others sub-systems. This causes the EC source to be a bit dirty. It is necessary to obtain the hardware platform name from the BIOS DMI table and determine its attributes according to its platform name.
> Now bind the attributes to ACPI table or device-tree, the EC source is more clear and universal for Advantech devices, and it is important that if the ACPI table matches, it can be automatically loaded.
> 
>>
>> Right, but that is not the case here, they are using 2 device-tree
>> properties (1), from patch 3/7:
>>
>> +properties:
>> +  compatible:
>> +    const: advantech,ahc1ec0
>> +
>> +  advantech,hwmon-profile:
>> +    description:
>> +      The number of sub-devices specified in the platform. Defines for the
>> +      hwmon profiles can found in dt-bindings/mfd/ahc1ec0-dt.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maxItems: 1
>> +
>> +  advantech,has-watchdog:
>> +    description:
>> +      Some implementations of the EC include a watchdog used to monitor
>> the
>> +      system. This boolean flag is used to specify whether this watchdog is
>> +      present or not. Default is true, otherwise set to false.
>> +    type: boolean
>>
>>
>>>> but AFAIK you are the first to do this on x86.
>>>
>>> No, not the first. Once Intel tried to invent the pin control
>>> configuration and muxing properties in ACPI, it was luckily rejected
>>> (ACPI 6.x OTOH provides a set of special resources for that).
>>>
>>> So, NAK from me, *if* it's really the case. ACPI tables must be revisited.
>>
> 
> I am not sure it supports vendor self-defined attributes for ACPI table?
> 
>> AFAIK Advantech are not defining things for which an ACPI standard exists,
>> although these 2 properties might just as well may be 2 simple ACPI integer
>> methods, which would actually make things a bit simpler (.e.g it would
>> allow dropping patch 2/7 and 3/7 from the set).
>>
>> Campion, any reason why you went this route; and can the ACPI tables
>> still be changed?
>>
> 
> If patches 2/7 and 3/7 are removed, it will be even simpler.
> This means that there is no device-tree binding designed, in fact, the EC chip only be integrated in the x86/64 platform at present.
> Sorry, ACPI table now is integrated in the BIOS for Advantech UNO device, 
> it may be revert to previous rule, that is, there is no ACPI table binding and manually loaded the EC driver. If you have any suggestons I would be very grateful.
> 
>> Regards,
>>
>> Hans
> 

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Rob Herring]

On Thu, May 06, 2021 at 10:47:22AM +0200, Hans de Goede wrote:
> Hi,
> 
> I'm replying here since this series has no cover-letter, for
> the next version for a series touching so many different
> sub-systems it would be good to start with a cover-letter
> providing some background info on the series.
> 
> I see this is binding to an ACPI device, yet it is also using
> devicetree bindings and properties.
> 
> So I take it this means that your ACPI tables are using the
> optional capability of embedded device-tree blobs inside the
> ACPI tables ?

Ugg, really. I would have stopped caring if I had realized.

Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry

[Campion Kang]

>-----Original Message-----
>From: Rob Herring <robh@kernel.org>
>Sent: Friday, May 7, 2021 8:50 AM
>Subject: Re: [PATCH v7 1/7] MAINTAINERS: Add Advantech AHC1EC0
>embedded controller entry
>
>On Thu, May 06, 2021 at 10:47:22AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> I'm replying here since this series has no cover-letter, for
>> the next version for a series touching so many different
>> sub-systems it would be good to start with a cover-letter
>> providing some background info on the series.
>>
>> I see this is binding to an ACPI device, yet it is also using
>> devicetree bindings and properties.
>>
>> So I take it this means that your ACPI tables are using the
>> optional capability of embedded device-tree blobs inside the
>> ACPI tables ?
>
>Ugg, really. I would have stopped caring if I had realized.

I am very grateful for any comments you have made in the past, and please feel free to give friendly guidance.