Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry
@ 2021-01-18 12:37 Campion Kang
  2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, Campion Kang

Add Advantech AHC1EC0 embedded controller entry

Changed since V5:
	- add include/linux/mfd/ahc1ec0.h

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 00836f6452f0..e1ccdaadb5ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -562,6 +562,17 @@ 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:	drivers/hwmon/ahc1ec0-hwmon.c
+F:	drivers/mfd/ahc1ec0.c
+F:	drivers/watchdog/ahc1ec0-wdt.c
+F:	include/dt-bindings/mfd/ahc1ec0-dt.h
+F:	include/linux/mfd/ahc1ec0.h
+
 ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings
  2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
@ 2021-01-18 12:37 ` Campion Kang
  2021-02-04 10:32   ` Lee Jones
  2021-01-18 12:37 ` [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0 Campion Kang
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, Campion Kang

This files defines the sud-device types and hwmon profiles support by
Advantech embedded controller.

Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
---
 include/dt-bindings/mfd/ahc1ec0-dt.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 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..389a7a7f8f02
--- /dev/null
+++ b/include/dt-bindings/mfd/ahc1ec0-dt.h
@@ -0,0 +1,25 @@
+/* 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
+
+/* Sub-device Definitions */
+#define AHC1EC0_SUBDEV_BRIGHTNESS 0x0
+#define AHC1EC0_SUBDEV_EEPROM     0x1
+#define AHC1EC0_SUBDEV_GPIO       0x2
+#define AHC1EC0_SUBDEV_HWMON      0x3
+#define AHC1EC0_SUBDEV_LED        0x4
+#define AHC1EC0_SUBDEV_WDT        0x5
+
+/* 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0
  2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
  2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
@ 2021-01-18 12:37 ` Campion Kang
  2021-02-05 21:21   ` Rob Herring
  2021-01-18 12:37 ` [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller Campion Kang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, Campion Kang

Add DT binding schema for Advantech embedded controller AHC1EC0.

Changed since V5:
	- 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      | 69 +++++++++++++++++++
 1 file changed, 69 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..40af14bb9c0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
@@ -0,0 +1,69 @@
+# 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, etc. Advantech related applications
+  can control the whole system via these functions.
+
+properties:
+  compatible:
+    const: advantech,ahc1ec0
+
+  advantech,sub-dev-nb:
+    description:
+      The number of sub-devices specified in the platform.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maxItems: 1
+
+  advantech,sub-dev:
+    description:
+      A list of the sub-devices supported in the platform. Defines for the
+      appropriate values can found in dt-bindings/mfd/ahc1ec0-dt.h.
+    $ref: "/schemas/types.yaml#/definitions/uint32-array"
+    minItems: 1
+    maxItems: 6
+
+  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
+
+required:
+  - compatible
+  - advantech,sub-dev-nb
+  - advantech,sub-dev
+
+if:
+  properties:
+    advantech,sub-dev:
+      contains:
+        const: 0x3
+then:
+  required:
+    - advantech,hwmon-profile
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/mfd/ahc1ec0-dt.h>
+    ahc1ec0 {
+        compatible = "advantech,ahc1ec0";
+
+        advantech,sub-dev-nb = <2>;
+        advantech,sub-dev = <AHC1EC0_SUBDEV_HWMON
+                             AHC1EC0_SUBDEV_WDT>;
+
+        advantech,hwmon-profile = <AHC1EC0_HWMON_PRO_UNO2271G>;
+    };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller
  2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
  2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
  2021-01-18 12:37 ` [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0 Campion Kang
@ 2021-01-18 12:37 ` Campion Kang
  2021-01-19  7:25   ` AceLan Kao
  2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
  2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
  4 siblings, 1 reply; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, Campion Kang

AHC1EC0 is the embedded controller driver for Advantech industrial
products. This provides sub-devices such as hwmon and watchdog, and also
expose functions for sub-devices to read/write the value to embedded
controller.

Changed since V5:
	- 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         |  10 +
 drivers/mfd/Makefile        |   2 +
 drivers/mfd/ahc1ec0.c       | 808 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ahc1ec0.h | 276 ++++++++++++
 4 files changed, 1096 insertions(+)
 create mode 100644 drivers/mfd/ahc1ec0.c
 create mode 100644 include/linux/mfd/ahc1ec0.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bdfce7b15621..7d5fb5c17d9a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2154,5 +2154,15 @@ 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 Module"
+	depends on X86
+	select MFD_CORE
+	help
+	  This is the core function that for Advantech EC drivers. It
+	  includes the sub-devices such as hwmon, watchdog, etc. And also
+	  provides expose functions for sub-devices to read/write the value
+	  to embedded controller.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 14fdb188af02..a6af9d8825f4 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -268,3 +268,5 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.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..015f4307a54e
--- /dev/null
+++ b/drivers/mfd/ahc1ec0.c
@@ -0,0 +1,808 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Advantech embedded controller core driver AHC1EC0
+ *
+ * Copyright 2020 Advantech IIoT Group
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
+#include <linux/mfd/ahc1ec0.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME      "ahc1ec0"
+
+enum {
+	ADVEC_SUBDEV_BRIGHTNESS = 0,
+	ADVEC_SUBDEV_EEPROM,
+	ADVEC_SUBDEV_GPIO,
+	ADVEC_SUBDEV_HWMON,
+	ADVEC_SUBDEV_LED,
+	ADVEC_SUBDEV_WDT,
+	ADVEC_SUBDEV_MAX,
+};
+
+/* 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 read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char *data)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
+
+	return ret;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+	       __LINE__);
+
+	return ret;
+}
+
+/* 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 write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char data)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
+
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+	       __LINE__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(write_hw_ram);
+
+/* Get dynamic control table */
+static int adv_get_dynamic_tab(struct adv_ec_platform_data *adv_ec_data)
+{
+	int i, ret;
+	unsigned char pin_tmp, device_id;
+
+	mutex_lock(&adv_ec_data->lock);
+
+	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
+		adv_ec_data->dym_tbl[i].device_id = 0xff;
+		adv_ec_data->dym_tbl[i].hw_pin_num = 0xff;
+	}
+
+	for (i = 0; i < EC_MAX_TBL_NUM; i++) {
+		ret = ec_wait_write();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+		outb(EC_TBL_WRITE_ITEM, EC_COMMAND_PORT);
+
+		ret = ec_wait_write();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+		outb(i, EC_STATUS_PORT);
+
+		ret = ec_wait_read();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+
+		/*
+		 *  If item is defined, EC will return item number.
+		 *  If table item is not defined, EC will return 0xFF.
+		 */
+		pin_tmp = inb(EC_STATUS_PORT);
+		if (pin_tmp == 0xff) {
+			dev_dbg(adv_ec_data->dev, "%s: inb(EC_STATUS_PORT)=0x%02x != 0xff.\n",
+				__func__, pin_tmp);
+			goto pass;
+		}
+
+		ret = ec_wait_write();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+		outb(EC_TBL_GET_PIN, EC_COMMAND_PORT);
+
+		ret = ec_wait_read();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+		pin_tmp = inb(EC_STATUS_PORT) & 0xff;
+		if (pin_tmp == 0xff) {
+			dev_dbg(adv_ec_data->dev, "%s: pin_tmp(0x%02X). line: %d\n", __func__,
+				pin_tmp, __LINE__);
+			goto pass;
+		}
+
+		ret = ec_wait_write();
+		if (ret)
+			goto error;
+		outb(EC_TBL_GET_DEVID, EC_COMMAND_PORT);
+
+		ret = ec_wait_read();
+		if (ret) {
+			dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
+				__LINE__);
+			goto error;
+		}
+		device_id = inb(EC_STATUS_PORT) & 0xff;
+
+		dev_dbg(adv_ec_data->dev, "%s: device_id=0x%02X. line: %d\n", __func__,
+			device_id, __LINE__);
+
+		adv_ec_data->dym_tbl[i].device_id = device_id;
+		adv_ec_data->dym_tbl[i].hw_pin_num = pin_tmp;
+	}
+
+pass:
+	mutex_unlock(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n",
+		__func__, __LINE__);
+	return ret;
+}
+
+int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,
+		unsigned char multi)
+{
+	int ret;
+	u32 ret_val;
+	unsigned int LSB, MSB;
+
+	mutex_lock(&adv_ec_data->lock);
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_AD_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) == 0xff) {
+		mutex_unlock(&adv_ec_data->lock);
+		return -1;
+	}
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(EC_AD_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_AD_MSB_READ, EC_COMMAND_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+	MSB = inb(EC_STATUS_PORT);
+	ret_val = ((MSB << 8) | LSB) & 0x03FF;
+	ret_val = ret_val * multi * 100;
+
+	mutex_unlock(&adv_ec_data->lock);
+	return ret_val;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+		__LINE__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(read_ad_value);
+
+int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+		unsigned char *pvalue)
+{
+	int ret;
+	unsigned char value;
+
+	mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
+
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+		__LINE__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(read_acpi_value);
+
+int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+		unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+		__LINE__);
+
+	return ret;
+}
+
+int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+		unsigned char *pvalue)
+{
+	int ret;
+
+	unsigned char gpio_status_value;
+
+	mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == 0xff) {
+		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
+		mutex_unlock(&adv_ec_data->lock);
+		return -1;
+	}
+
+	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(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+		__LINE__);
+	return ret;
+}
+
+int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+		unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == 0xff) {
+		mutex_unlock(&adv_ec_data->lock);
+		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
+		return -1;
+	}
+
+	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(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+	dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d", __func__,
+		__LINE__);
+
+	return ret;
+}
+
+int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+		unsigned char *pvalue)
+{
+	int ret;
+	unsigned char gpio_dir_value;
+
+	mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == 0xff) {
+		mutex_unlock(&adv_ec_data->lock);
+		dev_err(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
+			__LINE__);
+		return -1;
+	}
+
+	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(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+			__LINE__);
+
+	return ret;
+}
+
+int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+		unsigned char value)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
+
+	ret = ec_wait_read();
+	if (ret)
+		goto error;
+
+	if (inb(EC_STATUS_PORT) == 0xff) {
+		mutex_unlock(&adv_ec_data->lock);
+		dev_warn(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
+			__LINE__);
+
+		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(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+			__LINE__);
+
+	return ret;
+}
+
+int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data)
+{
+	int ret;
+
+	mutex_lock(&adv_ec_data->lock);
+
+	ret = ec_wait_write();
+	if (ret)
+		goto error;
+	outb(data, EC_COMMAND_PORT);
+
+	mutex_unlock(&adv_ec_data->lock);
+	return 0;
+
+error:
+	mutex_unlock(&adv_ec_data->lock);
+
+	dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
+			__LINE__);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(write_hwram_command);
+
+static int adv_ec_get_productname(struct adv_ec_platform_data *adv_ec_data, 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(adv_ec_data->dev, "BIOS Product Name = %s\n", product);
+
+		return 0;
+	}
+
+	dev_warn(adv_ec_data->dev, "This device is not Advantech Board (%s)!\n", product);
+
+	return -ENODEV;
+}
+
+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_platform_data *adv_ec_data)
+{
+	int ret;
+
+	adv_ec_data->sub_dev_mask = 0;
+	adv_ec_data->sub_dev_nb = 0;
+	adv_ec_data->dym_tbl = NULL;
+	adv_ec_data->bios_product_name = NULL;
+
+	mutex_init(&adv_ec_data->lock);
+
+	/* Get product name */
+	adv_ec_data->bios_product_name =
+		devm_kzalloc(adv_ec_data->dev, AMI_ADVANTECH_BOARD_ID_LENGTH, GFP_KERNEL);
+	if (!adv_ec_data->bios_product_name)
+		return -ENOMEM;
+
+	memset(adv_ec_data->bios_product_name, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
+	ret = adv_ec_get_productname(adv_ec_data, adv_ec_data->bios_product_name);
+	if (ret)
+		return ret;
+
+	/* Get pin table */
+	adv_ec_data->dym_tbl = devm_kzalloc(adv_ec_data->dev,
+					EC_MAX_TBL_NUM * sizeof(struct ec_dynamic_table),
+					GFP_KERNEL);
+	if (!adv_ec_data->dym_tbl)
+		return -ENOMEM;
+
+	ret = adv_get_dynamic_tab(adv_ec_data);
+
+	return ret;
+}
+
+static int adv_ec_parse_prop(struct adv_ec_platform_data *adv_ec_data)
+{
+	int i, ret;
+	u32 nb, sub_dev[ADVEC_SUBDEV_MAX];
+
+	ret = device_property_read_u32(adv_ec_data->dev, "advantech,sub-dev-nb", &nb);
+	if (ret < 0) {
+		dev_err(adv_ec_data->dev, "get sub-dev-nb failed! (%d)\n", ret);
+		return ret;
+	}
+	adv_ec_data->sub_dev_nb = nb;
+
+	ret = device_property_read_u32_array(adv_ec_data->dev, "advantech,sub-dev",
+					     sub_dev, nb);
+	if (ret < 0) {
+		dev_err(adv_ec_data->dev, "get sub-dev failed! (%d)\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < nb; i++) {
+		switch (sub_dev[i]) {
+		case ADVEC_SUBDEV_BRIGHTNESS:
+		case ADVEC_SUBDEV_EEPROM:
+		case ADVEC_SUBDEV_GPIO:
+		case ADVEC_SUBDEV_HWMON:
+		case ADVEC_SUBDEV_LED:
+		case ADVEC_SUBDEV_WDT:
+			adv_ec_data->sub_dev_mask |= BIT(sub_dev[i]);
+			break;
+		default:
+			dev_err(adv_ec_data->dev, "invalid prop value(%d)!\n",
+				sub_dev[i]);
+		}
+	}
+	dev_info(adv_ec_data->dev, "sub-dev mask = 0x%x\n", adv_ec_data->sub_dev_mask);
+
+	return 0;
+}
+
+static int adv_ec_probe(struct platform_device *pdev)
+{
+	int ret, i;
+	struct device *dev = &pdev->dev;
+	struct adv_ec_platform_data *adv_ec_data;
+
+	adv_ec_data = devm_kzalloc(dev, sizeof(struct adv_ec_platform_data), GFP_KERNEL);
+	if (!adv_ec_data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, adv_ec_data);
+	adv_ec_data->dev = dev;
+
+	ret = adv_ec_init_ec_data(adv_ec_data);
+	if (ret)
+		goto err_init_data;
+
+	ret = adv_ec_parse_prop(adv_ec_data);
+	if (ret)
+		goto err_prop;
+
+	/* check whether this EC has the following subdevices. */
+	for (i = 0; i < ARRAY_SIZE(adv_ec_sub_cells); i++) {
+		if (adv_ec_data->sub_dev_mask & BIT(i)) {
+			ret = mfd_add_hotplug_devices(dev, &adv_ec_sub_cells[i], 1);
+			dev_info(adv_ec_data->dev, "mfd_add_hotplug_devices[%d] %s\n", i,
+				adv_ec_sub_cells[i].name);
+			if (ret)
+				dev_err(dev, "failed to add %s subdevice: %d\n",
+					adv_ec_sub_cells[i].name, ret);
+		}
+	}
+
+	dev_info(adv_ec_data->dev, "Advantech EC probe done");
+
+	return 0;
+
+err_prop:
+	dev_err(dev, "failed to probe\n");
+
+err_init_data:
+	mutex_destroy(&adv_ec_data->lock);
+
+	dev_err(dev, "failed to init data\n");
+
+	return ret;
+}
+
+static int adv_ec_remove(struct platform_device *pdev)
+{
+	struct adv_ec_platform_data *adv_ec_data;
+
+	adv_ec_data = dev_get_drvdata(&pdev->dev);
+
+	mutex_destroy(&adv_ec_data->lock);
+
+	mfd_remove_devices(&pdev->dev);
+
+	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 = DRV_NAME,
+		.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:" DRV_NAME);
+MODULE_DESCRIPTION("Advantech Embedded Controller core driver.");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
+MODULE_VERSION("1.0");
diff --git a/include/linux/mfd/ahc1ec0.h b/include/linux/mfd/ahc1ec0.h
new file mode 100644
index 000000000000..1b01e10c1fef
--- /dev/null
+++ b/include/linux/mfd/ahc1ec0.h
@@ -0,0 +1,276 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __LINUX_MFD_AHC1EC0_H
+#define __LINUX_MFD_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_AD_INDEX_WRITE   0x15 /* Write ADC port number into index */
+#define EC_AD_LSB_READ      0x16 /* Read ADC LSB value from ADC port */
+#define EC_AD_MSB_READ      0x1F /* Read ADC MSB value from ADC port */
+
+/* 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
+
+/* 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_platform_data {
+	char *bios_product_name;
+	int sub_dev_nb;
+	u32 sub_dev_mask;
+	struct mutex lock;
+	struct device *dev;
+	struct class *adv_ec_class;
+
+	struct ec_dynamic_table *dym_tbl;
+};
+
+int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,
+			unsigned char multi);
+int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+			unsigned char *pvalue);
+int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+			unsigned char value);
+int read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+			unsigned char *data);
+int write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
+			unsigned char data);
+int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data);
+int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+			unsigned char *pvalue);
+int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+			unsigned char value);
+int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+			unsigned char *pvalue);
+int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
+			unsigned char value);
+
+#endif /* __LINUX_MFD_AHC1EC0_H */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon for Advantech embedded controller
  2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
                   ` (2 preceding siblings ...)
  2021-01-18 12:37 ` [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller Campion Kang
@ 2021-01-18 12:37 ` Campion Kang
  2021-01-19  7:26   ` AceLan Kao
  2021-01-23 16:35   ` Guenter Roeck
  2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
  4 siblings, 2 replies; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, 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 since V5:
	- 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>
---
 drivers/hwmon/Kconfig         |  10 +
 drivers/hwmon/Makefile        |   1 +
 drivers/hwmon/ahc1ec0-hwmon.c | 660 ++++++++++++++++++++++++++++++++++
 3 files changed, 671 insertions(+)
 create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1ecf697d8d99..bfa007026679 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2139,6 +2139,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 09a86c5e1d29..0c37747e8c4f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -45,6 +45,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_AMD_ENERGY) += amd_energy.o
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
 obj-$(CONFIG_SENSORS_ARM_SCMI)	+= scmi-hwmon.o
diff --git a/drivers/hwmon/ahc1ec0-hwmon.c b/drivers/hwmon/ahc1ec0-hwmon.c
new file mode 100644
index 000000000000..688f07e6a6e0
--- /dev/null
+++ b/drivers/hwmon/ahc1ec0-hwmon.c
@@ -0,0 +1,660 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HWMON Driver for Advantech Embedded Controller chip AHC1EC0
+ *
+ * Copyright 2020, Advantech IIoT Group
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/mfd/ahc1ec0.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+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 temp_list_cnt;
+	int *hwmon_in_list;
+	int *temp_list;
+};
+
+struct ec_hwmon_data {
+	struct device *dev;
+	struct device *hwmon_dev;
+	struct adv_ec_platform_data *adv_ec_data;
+	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;
+};
+
+static int get_ec_in_vbat_input(struct device *dev, long *val);
+static int get_ec_in_v5_input(struct device *dev, long *val);
+static int get_ec_in_v12_input(struct device *dev, long *val);
+static int get_ec_in_vcore_input(struct device *dev, long *val);
+static int get_ec_current1_input(struct device *dev, long *val);
+static int get_ec_cpu_temp(struct device *dev, long *val);
+static int get_ec_sys_temp(struct device *dev, long *val);
+
+const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
+	{"VBAT",	0444, get_ec_in_vbat_input},	// in1
+	{"5VSB",	0444, get_ec_in_v5_input},	// in2
+	{"Vin",		0444, get_ec_in_v12_input},	// in3 (== in8)
+	{"VCORE",	0444, get_ec_in_vcore_input},	// in4
+	{"Vin1",	0444, NULL},	// in5
+	{"Vin2",	0444, NULL},	// in6
+	{"System Voltage", 0444, NULL},	// in7
+	{"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},
+};
+
+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,
+	EC_HWMON_IN_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,
+	EC_HWMON_IN_CURRENT,
+};
+
+static int hwmon_in_list_1[] = {
+	EC_HWMON_IN_VBAT,
+	EC_HWMON_IN_5VSB,
+	EC_HWMON_IN_12V,
+	EC_HWMON_IN_VCORE,
+};
+
+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[] = {
+	/*
+	 * 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
+	 * [0] AHC1EC0_HWMON_PRO_TEMPLATE
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.hwmon_in_list = hwmon_in_list_0,
+		.temp_list = temp_list_0,
+	},
+	/*
+	 * 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
+	 * [1] AHC1EC0_HWMON_PRO_TPC5XXX
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list = temp_list_0,
+	},
+	/* PR/VR4
+	 * [2] AHC1EC0_HWMON_PRO_PRVR4
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.temp_list_cnt = ARRAY_SIZE(temp_list_1),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list = temp_list_1,
+	},
+	/* UNO-2271G-E22AE/E23AE/E022AE/E023AE,UNO-420
+	 * [3] AHC1EC0_HWMON_PRO_UNO2271G
+	 */
+	{
+		.resolution = 2929,
+		.r1 = 1912,
+		.r2 = 1000,
+		.offset = 0,
+		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
+		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
+		.hwmon_in_list = hwmon_in_list_1,
+		.temp_list = temp_list_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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+	struct ec_dynamic_table *dym_tbl = adv_ec_data->dym_tbl;
+
+	if (profile >= ARRAY_SIZE(advec_profile))
+		return;
+
+	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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	temp = read_ad_value(adv_ec_data, ptbl->v12[0], ptbl->v12[1]);
+	if (temp == -1)
+		temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	temp = read_ad_value(adv_ec_data, 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)
+{
+	#define EC_ACPI_THERMAL1_REMOTE_TEMP 0x61
+
+	unsigned char value;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_REMOTE_TEMP, &value);
+	*val = 1000 * value;
+	return 0;
+}
+
+static int get_ec_sys_temp(struct device *dev, long *val)
+{
+	#define EC_ACPI_THERMAL1_LOCAL_TEMP 0x60
+
+	unsigned char value;
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+	struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
+
+	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_LOCAL_TEMP, &value);
+	*val = 1000 * value;
+	return 0;
+}
+
+
+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 (!(lmsensor_data && lmsensor_data->profile))
+		return -EINVAL;
+
+	if (attr == hwmon_in_input &&
+		lmsensor_data->profile->hwmon_in_list_cnt > channel) {
+		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_temp(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
+
+	if (!(lmsensor_data && lmsensor_data->profile))
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		if (lmsensor_data->profile->temp_list_cnt > channel) {
+			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);
+		}
+		return -EOPNOTSUPP;
+	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 (!(lmsensor_data && lmsensor_data->profile))
+		return -EINVAL;
+
+	if ((type == hwmon_in && attr == hwmon_in_label) &&
+		(lmsensor_data->profile->hwmon_in_list_cnt > channel)) {
+		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_temp && attr == hwmon_temp_label) &&
+		(lmsensor_data->profile->temp_list_cnt > channel)) {
+		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_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)
+{
+	switch (attr) {
+	case hwmon_in_input:
+	case hwmon_in_label:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+ec_temp_in_visible(const void *data, u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_crit:
+	case hwmon_temp_label:
+		return 0444;
+	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_temp:
+		return ec_temp_in_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_temp_config[] = {
+	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_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_platform_data *adv_ec_data;
+	struct ec_hwmon_data *lmsensor_data;
+
+	adv_ec_data = dev_get_drvdata(dev->parent);
+	if (!adv_ec_data)
+		return -EINVAL;
+
+	ret = device_property_read_u32(dev->parent, "advantech,hwmon-profile", &profile);
+	if (ret < 0) {
+		dev_err(dev, "get hwmon-profile failed! (%d)", ret);
+		return ret;
+	}
+
+	if (!(profile < ARRAY_SIZE(advec_profile))) {
+		dev_err(dev, "not support hwmon profile(%d)!\n", profile);
+		return -EINVAL;
+	}
+
+	lmsensor_data = devm_kzalloc(dev, sizeof(struct ec_hwmon_data), GFP_KERNEL);
+	if (!lmsensor_data)
+		return -ENOMEM;
+
+	lmsensor_data->adv_ec_data = adv_ec_data;
+	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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog for Advantech embedded controller
  2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
                   ` (3 preceding siblings ...)
  2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
@ 2021-01-18 12:37 ` Campion Kang
  2021-01-19  7:26   ` AceLan Kao
  2021-01-23 17:00   ` Guenter Roeck
  4 siblings, 2 replies; 15+ messages in thread
From: Campion Kang @ 2021-01-18 12:37 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao, 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 since V5:
	- 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 | 261 +++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/watchdog/ahc1ec0-wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7ff941e71b79..1a27836883ac 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1636,6 +1636,17 @@ config NIC7018_WDT
 	  To compile this driver as a module, choose M here: the module will be
 	  called nic7018_wdt.
 
+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 thie driver as a module, choose M here: the module will be
+	  called ahc1ec0-wdt.
+
 # M68K Architecture
 
 config M54xx_WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c74ee19d441..7190811b1e50 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -145,6 +145,7 @@ obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
 obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
 obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
 obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
+obj-$(CONFIG_AHC1EC0_WDT) += ahc1ec0-wdt.o
 obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
 
 # M68K Architecture
diff --git a/drivers/watchdog/ahc1ec0-wdt.c b/drivers/watchdog/ahc1ec0-wdt.c
new file mode 100644
index 000000000000..4497b6106b24
--- /dev/null
+++ b/drivers/watchdog/ahc1ec0-wdt.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Watchdog Driver for Advantech Embedded Controller chip AHC1EC0
+ *
+ * Copyright 2020, Advantech IIoT Group
+ *
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/mfd/ahc1ec0.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define DRV_NAME      "ahc1ec0-wdt"
+
+struct ec_wdt_data {
+	struct watchdog_device wdtdev;
+	struct adv_ec_platform_data *adv_ec_data;
+	struct notifier_block reboot_nb;
+	struct mutex lock_ioctl;
+	int is_enable;
+	int current_timeout;
+};
+
+#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_platform_data *adv_ec_data, unsigned short delay_timeout_in_ms)
+{
+	if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_L, delay_timeout_in_ms & 0x00FF)) {
+		pr_err("Failed to set Watchdog Retset Time Low byte.");
+		return -EINVAL;
+	}
+
+	if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_H, (delay_timeout_in_ms & 0xFF00) >> 8)) {
+		pr_err("Failed to set Watchdog Retset Time Hight byte.");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int advwdt_set_heartbeat(unsigned long t)
+{
+	if (t < 1 || t > 6553) {
+		pr_err("%s: the input timeout is out of range.",  __func__);
+		pr_err("Please choose valid data between 1 ~ 6553.");
+		return -EINVAL;
+	}
+
+	return (t * 10);
+}
+
+/* Notifier for system down */
+static int advwdt_notify_sys(struct notifier_block *nb, unsigned long code, void *data)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		struct ec_wdt_data *ec_wdt_data;
+
+		ec_wdt_data = container_of(nb, struct ec_wdt_data, reboot_nb);
+		if (!ec_wdt_data)
+			return NOTIFY_BAD;
+
+		/* Turn the WDT off */
+		if (write_hwram_command(ec_wdt_data->adv_ec_data, EC_WDT_STOP)) {
+			pr_err("Failed to set Watchdog stop.");
+			return -EINVAL;
+		}
+		ec_wdt_data->is_enable = 0;
+		pr_info("%s: notify sys shutdown", __func__);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int ec_wdt_start(struct watchdog_device *wdd)
+{
+	int ret;
+	int timeout, timeout_in_ms;
+	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
+	struct adv_ec_platform_data *adv_ec_data;
+
+	dev_dbg(wdd->parent, "%s\n", __func__);
+
+	adv_ec_data = ec_wdt_data->adv_ec_data;
+	timeout = wdd->timeout; /* The watchdog devices timeout value (in seconds). */
+
+	mutex_lock(&ec_wdt_data->lock_ioctl);
+
+	timeout_in_ms = advwdt_set_heartbeat(timeout);
+	if (timeout_in_ms < 0) {
+		mutex_unlock(&ec_wdt_data->lock_ioctl);
+		return timeout_in_ms;
+	}
+
+	ret = set_delay(adv_ec_data, (unsigned short)(timeout_in_ms-1));
+	if (ret) {
+		dev_err(wdd->parent, "Failed to set Watchdog delay (ret=%x).\n", ret);
+		mutex_unlock(&ec_wdt_data->lock_ioctl);
+		return ret;
+	}
+	ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);
+	ret = write_hwram_command(adv_ec_data, EC_WDT_START);
+	if (ret) {
+		dev_err(wdd->parent, "Failed to set Watchdog start (ret=%x).\n", ret);
+		mutex_unlock(&ec_wdt_data->lock_ioctl);
+		return ret;
+	}
+	ec_wdt_data->is_enable = 1;
+	ec_wdt_data->current_timeout = timeout_in_ms/10;
+
+	mutex_unlock(&ec_wdt_data->lock_ioctl);
+	return 0;
+}
+
+static int ec_wdt_stop(struct watchdog_device *wdd)
+{
+	int ret;
+	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
+	struct adv_ec_platform_data *adv_ec_data;
+
+	dev_dbg(wdd->parent, "%s\n", __func__);
+
+	adv_ec_data = ec_wdt_data->adv_ec_data;
+
+	mutex_lock(&ec_wdt_data->lock_ioctl);
+	ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);
+	mutex_unlock(&ec_wdt_data->lock_ioctl);
+	if (ret)
+		pr_err("Failed to set Watchdog stop.");
+	else
+		ec_wdt_data->is_enable = 0;
+
+	return ret;
+}
+
+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_platform_data *adv_ec_data;
+
+	dev_dbg(wdd->parent, "%s\n", __func__);
+
+	adv_ec_data = ec_wdt_data->adv_ec_data;
+
+	mutex_lock(&ec_wdt_data->lock_ioctl);
+	ret = write_hwram_command(adv_ec_data, EC_WDT_RESET);
+	mutex_unlock(&ec_wdt_data->lock_ioctl);
+	if (ret) {
+		pr_err("Failed to set Watchdog reset.");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ec_wdt_set_timeout(struct watchdog_device *wdd,
+				unsigned int timeout)
+{
+	dev_dbg(wdd->parent, "%s, timeout=%d\n", __func__, 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_platform_data *adv_ec_data;
+	struct ec_wdt_data *ec_wdt_data;
+	struct watchdog_device *wdd;
+
+	dev_info(dev, "watchdog probe start\n");
+
+	adv_ec_data = dev_get_drvdata(dev->parent);
+	if (!adv_ec_data)
+		return -EINVAL;
+
+	ec_wdt_data = devm_kzalloc(dev, sizeof(struct ec_wdt_data), GFP_KERNEL);
+	if (!ec_wdt_data)
+		return -ENOMEM;
+
+	mutex_init(&ec_wdt_data->lock_ioctl);
+
+	ec_wdt_data->adv_ec_data = adv_ec_data;
+	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;
+	ec_wdt_data->is_enable = 0;
+	ec_wdt_data->current_timeout = EC_WDT_DEFAULT_TIMEOUT;
+
+	watchdog_stop_on_unregister(wdd);
+
+	ec_wdt_data->reboot_nb.notifier_call = advwdt_notify_sys;
+	ret = devm_register_reboot_notifier(dev, &ec_wdt_data->reboot_nb);
+	if (ret) {
+		dev_err(dev, "watchdog%d: Cannot register reboot notifier (%d)\n",
+			wdd->id, ret);
+		return ret;
+	}
+
+	ret = devm_watchdog_register_device(dev, wdd);
+	if (ret != 0)
+		dev_err(dev, "watchdog_register_device() failed: %d\n",
+			ret);
+
+	dev_info(dev, "watchdog register success\n");
+
+	return 0;
+}
+
+static struct platform_driver adv_wdt_drv = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = adv_ec_wdt_probe,
+};
+module_platform_driver(adv_wdt_drv);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Driver.");
+MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
+MODULE_VERSION("1.0");
-- 
2.17.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller
  2021-01-18 12:37 ` [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller Campion Kang
@ 2021-01-19  7:25   ` AceLan Kao
  2021-01-19  8:23     ` Lee Jones
  0 siblings, 1 reply; 15+ messages in thread
From: AceLan Kao @ 2021-01-19  7:25 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, Rob Herring, Linux-Kernel@Vger. Kernel. Org,
	devicetree, Jean Delvare, Guenter Roeck, Wim Van Sebroeck,
	linux-hwmon, linux-watchdog

Campion Kang <campion.kang@advantech.com.tw> 於 2021年1月18日 週一 下午8:37寫道:
>
> AHC1EC0 is the embedded controller driver for Advantech industrial
> products. This provides sub-devices such as hwmon and watchdog, and also
> expose functions for sub-devices to read/write the value to embedded
> controller.
>
> Changed since V5:
>         - 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         |  10 +
>  drivers/mfd/Makefile        |   2 +
>  drivers/mfd/ahc1ec0.c       | 808 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ahc1ec0.h | 276 ++++++++++++
>  4 files changed, 1096 insertions(+)
>  create mode 100644 drivers/mfd/ahc1ec0.c
>  create mode 100644 include/linux/mfd/ahc1ec0.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..7d5fb5c17d9a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2154,5 +2154,15 @@ 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 Module"
> +       depends on X86
> +       select MFD_CORE
> +       help
> +         This is the core function that for Advantech EC drivers. It
> +         includes the sub-devices such as hwmon, watchdog, etc. And also
> +         provides expose functions for sub-devices to read/write the value
> +         to embedded controller.
> +
>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..a6af9d8825f4 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -268,3 +268,5 @@ obj-$(CONFIG_MFD_KHADAS_MCU)        += khadas-mcu.o
>  obj-$(CONFIG_SGI_MFD_IOC3)     += ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)       += simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.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..015f4307a54e
> --- /dev/null
> +++ b/drivers/mfd/ahc1ec0.c
> @@ -0,0 +1,808 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Advantech embedded controller core driver AHC1EC0
> + *
> + * Copyright 2020 Advantech IIoT Group
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#include <linux/errno.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME      "ahc1ec0"
> +
> +enum {
> +       ADVEC_SUBDEV_BRIGHTNESS = 0,
> +       ADVEC_SUBDEV_EEPROM,
> +       ADVEC_SUBDEV_GPIO,
> +       ADVEC_SUBDEV_HWMON,
> +       ADVEC_SUBDEV_LED,
> +       ADVEC_SUBDEV_WDT,
> +       ADVEC_SUBDEV_MAX,
> +};
> +
> +/* 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 read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char *data)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
> +
> +       return ret;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +       dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +              __LINE__);
> +
> +       return ret;
> +}
> +
> +/* 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 write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr, unsigned char data)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
> +
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +              __LINE__);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(write_hw_ram);
> +
> +/* Get dynamic control table */
> +static int adv_get_dynamic_tab(struct adv_ec_platform_data *adv_ec_data)
> +{
> +       int i, ret;
> +       unsigned char pin_tmp, device_id;
> +
> +       mutex_lock(&adv_ec_data->lock);
> +
> +       for (i = 0; i < EC_MAX_TBL_NUM; i++) {
> +               adv_ec_data->dym_tbl[i].device_id = 0xff;
> +               adv_ec_data->dym_tbl[i].hw_pin_num = 0xff;
> +       }
> +
> +       for (i = 0; i < EC_MAX_TBL_NUM; i++) {
> +               ret = ec_wait_write();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +               outb(EC_TBL_WRITE_ITEM, EC_COMMAND_PORT);
> +
> +               ret = ec_wait_write();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +               outb(i, EC_STATUS_PORT);
> +
> +               ret = ec_wait_read();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +
> +               /*
> +                *  If item is defined, EC will return item number.
> +                *  If table item is not defined, EC will return 0xFF.
> +                */
> +               pin_tmp = inb(EC_STATUS_PORT);
> +               if (pin_tmp == 0xff) {
> +                       dev_dbg(adv_ec_data->dev, "%s: inb(EC_STATUS_PORT)=0x%02x != 0xff.\n",
> +                               __func__, pin_tmp);
> +                       goto pass;
> +               }
> +
> +               ret = ec_wait_write();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_write. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +               outb(EC_TBL_GET_PIN, EC_COMMAND_PORT);
> +
> +               ret = ec_wait_read();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +               pin_tmp = inb(EC_STATUS_PORT) & 0xff;
> +               if (pin_tmp == 0xff) {
> +                       dev_dbg(adv_ec_data->dev, "%s: pin_tmp(0x%02X). line: %d\n", __func__,
> +                               pin_tmp, __LINE__);
> +                       goto pass;
> +               }
> +
> +               ret = ec_wait_write();
> +               if (ret)
> +                       goto error;
> +               outb(EC_TBL_GET_DEVID, EC_COMMAND_PORT);
> +
> +               ret = ec_wait_read();
> +               if (ret) {
> +                       dev_dbg(adv_ec_data->dev, "%s: ec_wait_read. line: %d\n", __func__,
> +                               __LINE__);
> +                       goto error;
> +               }
> +               device_id = inb(EC_STATUS_PORT) & 0xff;
> +
> +               dev_dbg(adv_ec_data->dev, "%s: device_id=0x%02X. line: %d\n", __func__,
> +                       device_id, __LINE__);
> +
> +               adv_ec_data->dym_tbl[i].device_id = device_id;
> +               adv_ec_data->dym_tbl[i].hw_pin_num = pin_tmp;
> +       }
> +
> +pass:
> +       mutex_unlock(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +       dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n",
> +               __func__, __LINE__);
> +       return ret;
> +}
> +
> +int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,
> +               unsigned char multi)
> +{
> +       int ret;
> +       u32 ret_val;
> +       unsigned int LSB, MSB;
> +
> +       mutex_lock(&adv_ec_data->lock);
> +       ret = ec_wait_write();
> +       if (ret)
> +               goto error;
> +       outb(EC_AD_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) == 0xff) {
> +               mutex_unlock(&adv_ec_data->lock);
> +               return -1;
> +       }
> +
> +       ret = ec_wait_write();
> +       if (ret)
> +               goto error;
> +       outb(EC_AD_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_AD_MSB_READ, EC_COMMAND_PORT);
> +
> +       ret = ec_wait_read();
> +       if (ret)
> +               goto error;
> +       MSB = inb(EC_STATUS_PORT);
> +       ret_val = ((MSB << 8) | LSB) & 0x03FF;
> +       ret_val = ret_val * multi * 100;
> +
> +       mutex_unlock(&adv_ec_data->lock);
> +       return ret_val;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +               __LINE__);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(read_ad_value);
> +
> +int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +               unsigned char *pvalue)
> +{
> +       int ret;
> +       unsigned char value;
> +
> +       mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
> +
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +               __LINE__);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(read_acpi_value);
> +
> +int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +               unsigned char value)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->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(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +               __LINE__);
> +
> +       return ret;
> +}
> +
> +int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +               unsigned char *pvalue)
> +{
> +       int ret;
> +
> +       unsigned char gpio_status_value;
> +
> +       mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
> +
> +       ret = ec_wait_read();
> +       if (ret)
> +               goto error;
> +
> +       if (inb(EC_STATUS_PORT) == 0xff) {
> +               dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
> +               mutex_unlock(&adv_ec_data->lock);
> +               return -1;
> +       }
> +
> +       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(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +               __LINE__);
> +       return ret;
> +}
> +
> +int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +               unsigned char value)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
> +
> +       ret = ec_wait_read();
> +       if (ret)
> +               goto error;
> +
> +       if (inb(EC_STATUS_PORT) == 0xff) {
> +               mutex_unlock(&adv_ec_data->lock);
> +               dev_err(adv_ec_data->dev, "%s: Read Pin Number error!!\n", __func__);
> +               return -1;
> +       }
> +
> +       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(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +       dev_err(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d", __func__,
> +               __LINE__);
> +
> +       return ret;
> +}
> +
> +int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +               unsigned char *pvalue)
> +{
> +       int ret;
> +       unsigned char gpio_dir_value;
> +
> +       mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
> +
> +       ret = ec_wait_read();
> +       if (ret)
> +               goto error;
> +
> +       if (inb(EC_STATUS_PORT) == 0xff) {
> +               mutex_unlock(&adv_ec_data->lock);
> +               dev_err(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
> +                       __LINE__);
> +               return -1;
> +       }
> +
> +       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(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +                       __LINE__);
> +
> +       return ret;
> +}
> +
> +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +               unsigned char value)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->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(PinNumber, EC_STATUS_PORT);
> +
> +       ret = ec_wait_read();
> +       if (ret)
> +               goto error;
> +
> +       if (inb(EC_STATUS_PORT) == 0xff) {
> +               mutex_unlock(&adv_ec_data->lock);
> +               dev_warn(adv_ec_data->dev, "%s: Read Pin Number error!! line: %d\n", __func__,
> +                       __LINE__);
> +
> +               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(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +                       __LINE__);
> +
> +       return ret;
> +}
> +
> +int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data)
> +{
> +       int ret;
> +
> +       mutex_lock(&adv_ec_data->lock);
> +
> +       ret = ec_wait_write();
> +       if (ret)
> +               goto error;
> +       outb(data, EC_COMMAND_PORT);
> +
> +       mutex_unlock(&adv_ec_data->lock);
> +       return 0;
> +
> +error:
> +       mutex_unlock(&adv_ec_data->lock);
> +
> +       dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> +                       __LINE__);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(write_hwram_command);
> +
> +static int adv_ec_get_productname(struct adv_ec_platform_data *adv_ec_data, 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(adv_ec_data->dev, "BIOS Product Name = %s\n", product);
> +
> +               return 0;
> +       }
> +
> +       dev_warn(adv_ec_data->dev, "This device is not Advantech Board (%s)!\n", product);
> +
> +       return -ENODEV;
> +}
> +
> +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_platform_data *adv_ec_data)
> +{
> +       int ret;
> +
> +       adv_ec_data->sub_dev_mask = 0;
> +       adv_ec_data->sub_dev_nb = 0;
> +       adv_ec_data->dym_tbl = NULL;
> +       adv_ec_data->bios_product_name = NULL;
> +
> +       mutex_init(&adv_ec_data->lock);
> +
> +       /* Get product name */
> +       adv_ec_data->bios_product_name =
> +               devm_kzalloc(adv_ec_data->dev, AMI_ADVANTECH_BOARD_ID_LENGTH, GFP_KERNEL);
> +       if (!adv_ec_data->bios_product_name)
> +               return -ENOMEM;
> +
> +       memset(adv_ec_data->bios_product_name, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
> +       ret = adv_ec_get_productname(adv_ec_data, adv_ec_data->bios_product_name);
> +       if (ret)
> +               return ret;
> +
> +       /* Get pin table */
> +       adv_ec_data->dym_tbl = devm_kzalloc(adv_ec_data->dev,
> +                                       EC_MAX_TBL_NUM * sizeof(struct ec_dynamic_table),
> +                                       GFP_KERNEL);
> +       if (!adv_ec_data->dym_tbl)
> +               return -ENOMEM;
> +
> +       ret = adv_get_dynamic_tab(adv_ec_data);
> +
> +       return ret;
> +}
> +
> +static int adv_ec_parse_prop(struct adv_ec_platform_data *adv_ec_data)
> +{
> +       int i, ret;
> +       u32 nb, sub_dev[ADVEC_SUBDEV_MAX];
> +
> +       ret = device_property_read_u32(adv_ec_data->dev, "advantech,sub-dev-nb", &nb);
> +       if (ret < 0) {
> +               dev_err(adv_ec_data->dev, "get sub-dev-nb failed! (%d)\n", ret);
> +               return ret;
> +       }
> +       adv_ec_data->sub_dev_nb = nb;
> +
> +       ret = device_property_read_u32_array(adv_ec_data->dev, "advantech,sub-dev",
> +                                            sub_dev, nb);
> +       if (ret < 0) {
> +               dev_err(adv_ec_data->dev, "get sub-dev failed! (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       for (i = 0; i < nb; i++) {
> +               switch (sub_dev[i]) {
> +               case ADVEC_SUBDEV_BRIGHTNESS:
> +               case ADVEC_SUBDEV_EEPROM:
> +               case ADVEC_SUBDEV_GPIO:
> +               case ADVEC_SUBDEV_HWMON:
> +               case ADVEC_SUBDEV_LED:
> +               case ADVEC_SUBDEV_WDT:
> +                       adv_ec_data->sub_dev_mask |= BIT(sub_dev[i]);
> +                       break;
> +               default:
> +                       dev_err(adv_ec_data->dev, "invalid prop value(%d)!\n",
> +                               sub_dev[i]);
> +               }
> +       }
> +       dev_info(adv_ec_data->dev, "sub-dev mask = 0x%x\n", adv_ec_data->sub_dev_mask);
> +
> +       return 0;
> +}
> +
> +static int adv_ec_probe(struct platform_device *pdev)
> +{
> +       int ret, i;
> +       struct device *dev = &pdev->dev;
> +       struct adv_ec_platform_data *adv_ec_data;
> +
> +       adv_ec_data = devm_kzalloc(dev, sizeof(struct adv_ec_platform_data), GFP_KERNEL);
> +       if (!adv_ec_data)
> +               return -ENOMEM;
> +
> +       dev_set_drvdata(dev, adv_ec_data);
> +       adv_ec_data->dev = dev;
> +
> +       ret = adv_ec_init_ec_data(adv_ec_data);
> +       if (ret)
> +               goto err_init_data;
> +
> +       ret = adv_ec_parse_prop(adv_ec_data);
> +       if (ret)
> +               goto err_prop;
> +
> +       /* check whether this EC has the following subdevices. */
> +       for (i = 0; i < ARRAY_SIZE(adv_ec_sub_cells); i++) {
> +               if (adv_ec_data->sub_dev_mask & BIT(i)) {
> +                       ret = mfd_add_hotplug_devices(dev, &adv_ec_sub_cells[i], 1);
> +                       dev_info(adv_ec_data->dev, "mfd_add_hotplug_devices[%d] %s\n", i,
> +                               adv_ec_sub_cells[i].name);
> +                       if (ret)
> +                               dev_err(dev, "failed to add %s subdevice: %d\n",
> +                                       adv_ec_sub_cells[i].name, ret);
> +               }
> +       }
> +
> +       dev_info(adv_ec_data->dev, "Advantech EC probe done");
> +
> +       return 0;
> +
> +err_prop:
> +       dev_err(dev, "failed to probe\n");
> +
> +err_init_data:
> +       mutex_destroy(&adv_ec_data->lock);
> +
> +       dev_err(dev, "failed to init data\n");
> +
> +       return ret;
> +}
> +
> +static int adv_ec_remove(struct platform_device *pdev)
> +{
> +       struct adv_ec_platform_data *adv_ec_data;
> +
> +       adv_ec_data = dev_get_drvdata(&pdev->dev);
> +
> +       mutex_destroy(&adv_ec_data->lock);
> +
> +       mfd_remove_devices(&pdev->dev);
> +
> +       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 = DRV_NAME,
> +               .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:" DRV_NAME);
> +MODULE_DESCRIPTION("Advantech Embedded Controller core driver.");
> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_AUTHOR("Jianfeng Dai <jianfeng.dai@advantech.com.cn>");
> +MODULE_VERSION("1.0");
> diff --git a/include/linux/mfd/ahc1ec0.h b/include/linux/mfd/ahc1ec0.h
> new file mode 100644
> index 000000000000..1b01e10c1fef
> --- /dev/null
> +++ b/include/linux/mfd/ahc1ec0.h
> @@ -0,0 +1,276 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_MFD_AHC1EC0_H
> +#define __LINUX_MFD_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_AD_INDEX_WRITE   0x15 /* Write ADC port number into index */
> +#define EC_AD_LSB_READ      0x16 /* Read ADC LSB value from ADC port */
> +#define EC_AD_MSB_READ      0x1F /* Read ADC MSB value from ADC port */
> +
> +/* 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
> +
> +/* 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_platform_data {
> +       char *bios_product_name;
> +       int sub_dev_nb;
> +       u32 sub_dev_mask;
> +       struct mutex lock;
> +       struct device *dev;
> +       struct class *adv_ec_class;
> +
> +       struct ec_dynamic_table *dym_tbl;
> +};
> +
> +int read_ad_value(struct adv_ec_platform_data *adv_ec_data, unsigned char hwpin,
> +                       unsigned char multi);
> +int read_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +                       unsigned char *pvalue);
> +int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +                       unsigned char value);
> +int read_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +                       unsigned char *data);
> +int write_hw_ram(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> +                       unsigned char data);
> +int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data);
> +int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +                       unsigned char *pvalue);
> +int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +                       unsigned char value);
> +int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +                       unsigned char *pvalue);
> +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> +                       unsigned char value);
> +
> +#endif /* __LINUX_MFD_AHC1EC0_H */
> --
> 2.17.1
>

Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon for Advantech embedded controller
  2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
@ 2021-01-19  7:26   ` AceLan Kao
  2021-01-23 16:35   ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: AceLan Kao @ 2021-01-19  7:26 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, Rob Herring, Linux-Kernel@Vger. Kernel. Org,
	devicetree, Jean Delvare, Guenter Roeck, Wim Van Sebroeck,
	linux-hwmon, linux-watchdog

Campion Kang <campion.kang@advantech.com.tw> 於 2021年1月18日 週一 下午8:37寫道:
>
> 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 since V5:
>         - 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>
> ---
>  drivers/hwmon/Kconfig         |  10 +
>  drivers/hwmon/Makefile        |   1 +
>  drivers/hwmon/ahc1ec0-hwmon.c | 660 ++++++++++++++++++++++++++++++++++
>  3 files changed, 671 insertions(+)
>  create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..bfa007026679 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2139,6 +2139,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 09a86c5e1d29..0c37747e8c4f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -45,6 +45,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_AMD_ENERGY) += amd_energy.o
>  obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
> diff --git a/drivers/hwmon/ahc1ec0-hwmon.c b/drivers/hwmon/ahc1ec0-hwmon.c
> new file mode 100644
> index 000000000000..688f07e6a6e0
> --- /dev/null
> +++ b/drivers/hwmon/ahc1ec0-hwmon.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HWMON Driver for Advantech Embedded Controller chip AHC1EC0
> + *
> + * Copyright 2020, Advantech IIoT Group
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +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 temp_list_cnt;
> +       int *hwmon_in_list;
> +       int *temp_list;
> +};
> +
> +struct ec_hwmon_data {
> +       struct device *dev;
> +       struct device *hwmon_dev;
> +       struct adv_ec_platform_data *adv_ec_data;
> +       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;
> +};
> +
> +static int get_ec_in_vbat_input(struct device *dev, long *val);
> +static int get_ec_in_v5_input(struct device *dev, long *val);
> +static int get_ec_in_v12_input(struct device *dev, long *val);
> +static int get_ec_in_vcore_input(struct device *dev, long *val);
> +static int get_ec_current1_input(struct device *dev, long *val);
> +static int get_ec_cpu_temp(struct device *dev, long *val);
> +static int get_ec_sys_temp(struct device *dev, long *val);
> +
> +const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
> +       {"VBAT",        0444, get_ec_in_vbat_input},    // in1
> +       {"5VSB",        0444, get_ec_in_v5_input},      // in2
> +       {"Vin",         0444, get_ec_in_v12_input},     // in3 (== in8)
> +       {"VCORE",       0444, get_ec_in_vcore_input},   // in4
> +       {"Vin1",        0444, NULL},    // in5
> +       {"Vin2",        0444, NULL},    // in6
> +       {"System Voltage", 0444, NULL}, // in7
> +       {"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},
> +};
> +
> +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,
> +       EC_HWMON_IN_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,
> +       EC_HWMON_IN_CURRENT,
> +};
> +
> +static int hwmon_in_list_1[] = {
> +       EC_HWMON_IN_VBAT,
> +       EC_HWMON_IN_5VSB,
> +       EC_HWMON_IN_12V,
> +       EC_HWMON_IN_VCORE,
> +};
> +
> +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[] = {
> +       /*
> +        * 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
> +        * [0] AHC1EC0_HWMON_PRO_TEMPLATE
> +        */
> +       {
> +               .resolution = 2929,
> +               .r1 = 1912,
> +               .r2 = 1000,
> +               .offset = 0,
> +               .hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
> +               .temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +               .hwmon_in_list = hwmon_in_list_0,
> +               .temp_list = temp_list_0,
> +       },
> +       /*
> +        * 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
> +        * [1] AHC1EC0_HWMON_PRO_TPC5XXX
> +        */
> +       {
> +               .resolution = 2929,
> +               .r1 = 1912,
> +               .r2 = 1000,
> +               .offset = 0,
> +               .hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +               .temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +               .hwmon_in_list = hwmon_in_list_1,
> +               .temp_list = temp_list_0,
> +       },
> +       /* PR/VR4
> +        * [2] AHC1EC0_HWMON_PRO_PRVR4
> +        */
> +       {
> +               .resolution = 2929,
> +               .r1 = 1912,
> +               .r2 = 1000,
> +               .offset = 0,
> +               .hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +               .temp_list_cnt = ARRAY_SIZE(temp_list_1),
> +               .hwmon_in_list = hwmon_in_list_1,
> +               .temp_list = temp_list_1,
> +       },
> +       /* UNO-2271G-E22AE/E23AE/E022AE/E023AE,UNO-420
> +        * [3] AHC1EC0_HWMON_PRO_UNO2271G
> +        */
> +       {
> +               .resolution = 2929,
> +               .r1 = 1912,
> +               .r2 = 1000,
> +               .offset = 0,
> +               .hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +               .temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +               .hwmon_in_list = hwmon_in_list_1,
> +               .temp_list = temp_list_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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +       struct ec_dynamic_table *dym_tbl = adv_ec_data->dym_tbl;
> +
> +       if (profile >= ARRAY_SIZE(advec_profile))
> +               return;
> +
> +       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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       temp = read_ad_value(adv_ec_data, ptbl->v12[0], ptbl->v12[1]);
> +       if (temp == -1)
> +               temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       temp = read_ad_value(adv_ec_data, 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)
> +{
> +       #define EC_ACPI_THERMAL1_REMOTE_TEMP 0x61
> +
> +       unsigned char value;
> +       struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +       struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_REMOTE_TEMP, &value);
> +       *val = 1000 * value;
> +       return 0;
> +}
> +
> +static int get_ec_sys_temp(struct device *dev, long *val)
> +{
> +       #define EC_ACPI_THERMAL1_LOCAL_TEMP 0x60
> +
> +       unsigned char value;
> +       struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +       struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +       read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_LOCAL_TEMP, &value);
> +       *val = 1000 * value;
> +       return 0;
> +}
> +
> +
> +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 (!(lmsensor_data && lmsensor_data->profile))
> +               return -EINVAL;
> +
> +       if (attr == hwmon_in_input &&
> +               lmsensor_data->profile->hwmon_in_list_cnt > channel) {
> +               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_temp(struct device *dev, u32 attr, int channel, long *val)
> +{
> +       struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +       if (!(lmsensor_data && lmsensor_data->profile))
> +               return -EINVAL;
> +
> +       switch (attr) {
> +       case hwmon_temp_input:
> +               if (lmsensor_data->profile->temp_list_cnt > channel) {
> +                       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);
> +               }
> +               return -EOPNOTSUPP;
> +       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 (!(lmsensor_data && lmsensor_data->profile))
> +               return -EINVAL;
> +
> +       if ((type == hwmon_in && attr == hwmon_in_label) &&
> +               (lmsensor_data->profile->hwmon_in_list_cnt > channel)) {
> +               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_temp && attr == hwmon_temp_label) &&
> +               (lmsensor_data->profile->temp_list_cnt > channel)) {
> +               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_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)
> +{
> +       switch (attr) {
> +       case hwmon_in_input:
> +       case hwmon_in_label:
> +               return 0444;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +ec_temp_in_visible(const void *data, u32 attr, int channel)
> +{
> +       switch (attr) {
> +       case hwmon_temp_input:
> +       case hwmon_temp_crit:
> +       case hwmon_temp_label:
> +               return 0444;
> +       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_temp:
> +               return ec_temp_in_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_temp_config[] = {
> +       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_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_platform_data *adv_ec_data;
> +       struct ec_hwmon_data *lmsensor_data;
> +
> +       adv_ec_data = dev_get_drvdata(dev->parent);
> +       if (!adv_ec_data)
> +               return -EINVAL;
> +
> +       ret = device_property_read_u32(dev->parent, "advantech,hwmon-profile", &profile);
> +       if (ret < 0) {
> +               dev_err(dev, "get hwmon-profile failed! (%d)", ret);
> +               return ret;
> +       }
> +
> +       if (!(profile < ARRAY_SIZE(advec_profile))) {
> +               dev_err(dev, "not support hwmon profile(%d)!\n", profile);
> +               return -EINVAL;
> +       }
> +
> +       lmsensor_data = devm_kzalloc(dev, sizeof(struct ec_hwmon_data), GFP_KERNEL);
> +       if (!lmsensor_data)
> +               return -ENOMEM;
> +
> +       lmsensor_data->adv_ec_data = adv_ec_data;
> +       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
>

Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog for Advantech embedded controller
  2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
@ 2021-01-19  7:26   ` AceLan Kao
  2021-01-23 17:00   ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: AceLan Kao @ 2021-01-19  7:26 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, Rob Herring, Linux-Kernel@Vger. Kernel. Org,
	devicetree, Jean Delvare, Guenter Roeck, Wim Van Sebroeck,
	linux-hwmon, linux-watchdog

Campion Kang <campion.kang@advantech.com.tw> 於 2021年1月18日 週一 下午8:39寫道:
>
> 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 since V5:
>         - 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 | 261 +++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/watchdog/ahc1ec0-wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7ff941e71b79..1a27836883ac 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1636,6 +1636,17 @@ config NIC7018_WDT
>           To compile this driver as a module, choose M here: the module will be
>           called nic7018_wdt.
>
> +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 thie driver as a module, choose M here: the module will be
> +         called ahc1ec0-wdt.
> +
>  # M68K Architecture
>
>  config M54xx_WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c74ee19d441..7190811b1e50 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -145,6 +145,7 @@ obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
>  obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
>  obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>  obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> +obj-$(CONFIG_AHC1EC0_WDT) += ahc1ec0-wdt.o
>  obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
>
>  # M68K Architecture
> diff --git a/drivers/watchdog/ahc1ec0-wdt.c b/drivers/watchdog/ahc1ec0-wdt.c
> new file mode 100644
> index 000000000000..4497b6106b24
> --- /dev/null
> +++ b/drivers/watchdog/ahc1ec0-wdt.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Watchdog Driver for Advantech Embedded Controller chip AHC1EC0
> + *
> + * Copyright 2020, Advantech IIoT Group
> + *
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +
> +#define DRV_NAME      "ahc1ec0-wdt"
> +
> +struct ec_wdt_data {
> +       struct watchdog_device wdtdev;
> +       struct adv_ec_platform_data *adv_ec_data;
> +       struct notifier_block reboot_nb;
> +       struct mutex lock_ioctl;
> +       int is_enable;
> +       int current_timeout;
> +};
> +
> +#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_platform_data *adv_ec_data, unsigned short delay_timeout_in_ms)
> +{
> +       if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_L, delay_timeout_in_ms & 0x00FF)) {
> +               pr_err("Failed to set Watchdog Retset Time Low byte.");
> +               return -EINVAL;
> +       }
> +
> +       if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_H, (delay_timeout_in_ms & 0xFF00) >> 8)) {
> +               pr_err("Failed to set Watchdog Retset Time Hight byte.");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int advwdt_set_heartbeat(unsigned long t)
> +{
> +       if (t < 1 || t > 6553) {
> +               pr_err("%s: the input timeout is out of range.",  __func__);
> +               pr_err("Please choose valid data between 1 ~ 6553.");
> +               return -EINVAL;
> +       }
> +
> +       return (t * 10);
> +}
> +
> +/* Notifier for system down */
> +static int advwdt_notify_sys(struct notifier_block *nb, unsigned long code, void *data)
> +{
> +       if (code == SYS_DOWN || code == SYS_HALT) {
> +               struct ec_wdt_data *ec_wdt_data;
> +
> +               ec_wdt_data = container_of(nb, struct ec_wdt_data, reboot_nb);
> +               if (!ec_wdt_data)
> +                       return NOTIFY_BAD;
> +
> +               /* Turn the WDT off */
> +               if (write_hwram_command(ec_wdt_data->adv_ec_data, EC_WDT_STOP)) {
> +                       pr_err("Failed to set Watchdog stop.");
> +                       return -EINVAL;
> +               }
> +               ec_wdt_data->is_enable = 0;
> +               pr_info("%s: notify sys shutdown", __func__);
> +       }
> +
> +       return NOTIFY_DONE;
> +}
> +
> +static int ec_wdt_start(struct watchdog_device *wdd)
> +{
> +       int ret;
> +       int timeout, timeout_in_ms;
> +       struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +       struct adv_ec_platform_data *adv_ec_data;
> +
> +       dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +       adv_ec_data = ec_wdt_data->adv_ec_data;
> +       timeout = wdd->timeout; /* The watchdog devices timeout value (in seconds). */
> +
> +       mutex_lock(&ec_wdt_data->lock_ioctl);
> +
> +       timeout_in_ms = advwdt_set_heartbeat(timeout);
> +       if (timeout_in_ms < 0) {
> +               mutex_unlock(&ec_wdt_data->lock_ioctl);
> +               return timeout_in_ms;
> +       }
> +
> +       ret = set_delay(adv_ec_data, (unsigned short)(timeout_in_ms-1));
> +       if (ret) {
> +               dev_err(wdd->parent, "Failed to set Watchdog delay (ret=%x).\n", ret);
> +               mutex_unlock(&ec_wdt_data->lock_ioctl);
> +               return ret;
> +       }
> +       ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);
> +       ret = write_hwram_command(adv_ec_data, EC_WDT_START);
> +       if (ret) {
> +               dev_err(wdd->parent, "Failed to set Watchdog start (ret=%x).\n", ret);
> +               mutex_unlock(&ec_wdt_data->lock_ioctl);
> +               return ret;
> +       }
> +       ec_wdt_data->is_enable = 1;
> +       ec_wdt_data->current_timeout = timeout_in_ms/10;
> +
> +       mutex_unlock(&ec_wdt_data->lock_ioctl);
> +       return 0;
> +}
> +
> +static int ec_wdt_stop(struct watchdog_device *wdd)
> +{
> +       int ret;
> +       struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +       struct adv_ec_platform_data *adv_ec_data;
> +
> +       dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +       adv_ec_data = ec_wdt_data->adv_ec_data;
> +
> +       mutex_lock(&ec_wdt_data->lock_ioctl);
> +       ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);
> +       mutex_unlock(&ec_wdt_data->lock_ioctl);
> +       if (ret)
> +               pr_err("Failed to set Watchdog stop.");
> +       else
> +               ec_wdt_data->is_enable = 0;
> +
> +       return ret;
> +}
> +
> +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_platform_data *adv_ec_data;
> +
> +       dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +       adv_ec_data = ec_wdt_data->adv_ec_data;
> +
> +       mutex_lock(&ec_wdt_data->lock_ioctl);
> +       ret = write_hwram_command(adv_ec_data, EC_WDT_RESET);
> +       mutex_unlock(&ec_wdt_data->lock_ioctl);
> +       if (ret) {
> +               pr_err("Failed to set Watchdog reset.");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ec_wdt_set_timeout(struct watchdog_device *wdd,
> +                               unsigned int timeout)
> +{
> +       dev_dbg(wdd->parent, "%s, timeout=%d\n", __func__, 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_platform_data *adv_ec_data;
> +       struct ec_wdt_data *ec_wdt_data;
> +       struct watchdog_device *wdd;
> +
> +       dev_info(dev, "watchdog probe start\n");
> +
> +       adv_ec_data = dev_get_drvdata(dev->parent);
> +       if (!adv_ec_data)
> +               return -EINVAL;
> +
> +       ec_wdt_data = devm_kzalloc(dev, sizeof(struct ec_wdt_data), GFP_KERNEL);
> +       if (!ec_wdt_data)
> +               return -ENOMEM;
> +
> +       mutex_init(&ec_wdt_data->lock_ioctl);
> +
> +       ec_wdt_data->adv_ec_data = adv_ec_data;
> +       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;
> +       ec_wdt_data->is_enable = 0;
> +       ec_wdt_data->current_timeout = EC_WDT_DEFAULT_TIMEOUT;
> +
> +       watchdog_stop_on_unregister(wdd);
> +
> +       ec_wdt_data->reboot_nb.notifier_call = advwdt_notify_sys;
> +       ret = devm_register_reboot_notifier(dev, &ec_wdt_data->reboot_nb);
> +       if (ret) {
> +               dev_err(dev, "watchdog%d: Cannot register reboot notifier (%d)\n",
> +                       wdd->id, ret);
> +               return ret;
> +       }
> +
> +       ret = devm_watchdog_register_device(dev, wdd);
> +       if (ret != 0)
> +               dev_err(dev, "watchdog_register_device() failed: %d\n",
> +                       ret);
> +
> +       dev_info(dev, "watchdog register success\n");
> +
> +       return 0;
> +}
> +
> +static struct platform_driver adv_wdt_drv = {
> +       .driver = {
> +               .name = DRV_NAME,
> +       },
> +       .probe = adv_ec_wdt_probe,
> +};
> +module_platform_driver(adv_wdt_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Driver.");
> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_VERSION("1.0");
> --
> 2.17.1
>
Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller
  2021-01-19  7:25   ` AceLan Kao
@ 2021-01-19  8:23     ` Lee Jones
  0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2021-01-19  8:23 UTC (permalink / raw)
  To: AceLan Kao
  Cc: Campion Kang, Rob Herring, Linux-Kernel@Vger. Kernel. Org,
	devicetree, Jean Delvare, Guenter Roeck, Wim Van Sebroeck,
	linux-hwmon, linux-watchdog

On Tue, 19 Jan 2021, AceLan Kao wrote:

> Campion Kang <campion.kang@advantech.com.tw> 於 2021年1月18日 週一 下午8:37寫道:
> >
> > AHC1EC0 is the embedded controller driver for Advantech industrial
> > products. This provides sub-devices such as hwmon and watchdog, and also
> > expose functions for sub-devices to read/write the value to embedded
> > controller.
> >
> > Changed since V5:
> >         - 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         |  10 +
> >  drivers/mfd/Makefile        |   2 +
> >  drivers/mfd/ahc1ec0.c       | 808 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/ahc1ec0.h | 276 ++++++++++++
> >  4 files changed, 1096 insertions(+)
> >  create mode 100644 drivers/mfd/ahc1ec0.c
> >  create mode 100644 include/linux/mfd/ahc1ec0.h

[...]

NB: Snipped 1000 lines.

> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

Would you be kind enough to snip your replies please AceLan?

It would have the benefit of saving a lot of people a little time.

TIA.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon for Advantech embedded controller
  2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
  2021-01-19  7:26   ` AceLan Kao
@ 2021-01-23 16:35   ` Guenter Roeck
       [not found]     ` <66338d379bb14c2fbd8a6507075384ce@Taipei11.ADVANTECH.CORP>
  1 sibling, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-01-23 16:35 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Wim Van Sebroeck, linux-hwmon, linux-watchdog, AceLan Kao

On Mon, Jan 18, 2021 at 08:37:48PM +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 since V5:
> 	- 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>
> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

checkpatch --strict says:

total: 0 errors, 1 warnings, 10 checks, 683 lines checked

The MAINTAINERS warning is irrelevant, but there are a number of alignment
and other style issues. Please fix those.

> ---
>  drivers/hwmon/Kconfig         |  10 +
>  drivers/hwmon/Makefile        |   1 +
>  drivers/hwmon/ahc1ec0-hwmon.c | 660 ++++++++++++++++++++++++++++++++++

Documentation is missing.

>  3 files changed, 671 insertions(+)
>  create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..bfa007026679 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2139,6 +2139,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 09a86c5e1d29..0c37747e8c4f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -45,6 +45,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_AMD_ENERGY) += amd_energy.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCMI)	+= scmi-hwmon.o
> diff --git a/drivers/hwmon/ahc1ec0-hwmon.c b/drivers/hwmon/ahc1ec0-hwmon.c
> new file mode 100644
> index 000000000000..688f07e6a6e0
> --- /dev/null
> +++ b/drivers/hwmon/ahc1ec0-hwmon.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HWMON Driver for Advantech Embedded Controller chip AHC1EC0
> + *
> + * Copyright 2020, Advantech IIoT Group
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +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 temp_list_cnt;
> +	int *hwmon_in_list;
> +	int *temp_list;
> +};
> +
> +struct ec_hwmon_data {
> +	struct device *dev;
> +	struct device *hwmon_dev;
> +	struct adv_ec_platform_data *adv_ec_data;
> +	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;
> +};
> +
> +static int get_ec_in_vbat_input(struct device *dev, long *val);
> +static int get_ec_in_v5_input(struct device *dev, long *val);
> +static int get_ec_in_v12_input(struct device *dev, long *val);
> +static int get_ec_in_vcore_input(struct device *dev, long *val);
> +static int get_ec_current1_input(struct device *dev, long *val);
> +static int get_ec_cpu_temp(struct device *dev, long *val);
> +static int get_ec_sys_temp(struct device *dev, long *val);
> +

Please reorder code to not require those declarations.

> +const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
> +	{"VBAT",	0444, get_ec_in_vbat_input},	// in1
> +	{"5VSB",	0444, get_ec_in_v5_input},	// in2
> +	{"Vin",		0444, get_ec_in_v12_input},	// in3 (== in8)
> +	{"VCORE",	0444, get_ec_in_vcore_input},	// in4
> +	{"Vin1",	0444, NULL},	// in5
> +	{"Vin2",	0444, NULL},	// in6
> +	{"System Voltage", 0444, NULL},	// in7
> +	{"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},
> +};
> +
> +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,
> +	EC_HWMON_IN_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,
> +	EC_HWMON_IN_CURRENT,
> +};
> +
> +static int hwmon_in_list_1[] = {
> +	EC_HWMON_IN_VBAT,
> +	EC_HWMON_IN_5VSB,
> +	EC_HWMON_IN_12V,
> +	EC_HWMON_IN_VCORE,
> +};
> +
> +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[] = {
> +	/*
> +	 * 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

What is all this ? Please add a brief explanation (affected hardware ? chips ?)

> +	 * [0] AHC1EC0_HWMON_PRO_TEMPLATE
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.hwmon_in_list = hwmon_in_list_0,
> +		.temp_list = temp_list_0,
> +	},
> +	/*
> +	 * 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
> +	 * [1] AHC1EC0_HWMON_PRO_TPC5XXX
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list = temp_list_0,
> +	},
> +	/* PR/VR4
> +	 * [2] AHC1EC0_HWMON_PRO_PRVR4
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_1),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list = temp_list_1,
> +	},
> +	/* UNO-2271G-E22AE/E23AE/E022AE/E023AE,UNO-420
> +	 * [3] AHC1EC0_HWMON_PRO_UNO2271G
> +	 */
> +	{
> +		.resolution = 2929,
> +		.r1 = 1912,
> +		.r2 = 1000,
> +		.offset = 0,
> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
> +		.hwmon_in_list = hwmon_in_list_1,
> +		.temp_list = temp_list_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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +	struct ec_dynamic_table *dym_tbl = adv_ec_data->dym_tbl;
> +
> +	if (profile >= ARRAY_SIZE(advec_profile))
> +		return;

This can not happen. Please no unnecessary error checks.

> +
> +	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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	temp = read_ad_value(adv_ec_data, 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);
> +

Please no empty lines in variable declarations.

> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
> +	struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	temp = read_ad_value(adv_ec_data, ptbl->v12[0], ptbl->v12[1]);
> +	if (temp == -1)
> +		temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	temp = read_ad_value(adv_ec_data, 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)
> +{
> +	#define EC_ACPI_THERMAL1_REMOTE_TEMP 0x61

Pease move defines to top of file. Also, please use

#define<space>IDENTIFER<tab>value

> +
> +	unsigned char value;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_REMOTE_TEMP, &value);

This is not an appropriate function name for a device specific exported
function. The function name should have a prefix.

Also, the error code from read_acpi_value needs to be checked
and handled.

> +	*val = 1000 * value;
> +	return 0;
> +}
> +
> +static int get_ec_sys_temp(struct device *dev, long *val)
> +{
> +	#define EC_ACPI_THERMAL1_LOCAL_TEMP 0x60
> +
> +	unsigned char value;
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +	struct adv_ec_platform_data *adv_ec_data = lmsensor_data->adv_ec_data;
> +
> +	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_LOCAL_TEMP, &value);
> +	*val = 1000 * value;
> +	return 0;
> +}
> +
> +
> +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 (!(lmsensor_data && lmsensor_data->profile))
> +		return -EINVAL;

This can not happen. Also, please don't use double negations.

> +
> +	if (attr == hwmon_in_input &&
> +		lmsensor_data->profile->hwmon_in_list_cnt > channel) {

This is unnecessary. Again, please drop all those unnecessary checks.

> +		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_temp(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
> +
> +	if (!(lmsensor_data && lmsensor_data->profile))
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		if (lmsensor_data->profile->temp_list_cnt > channel) {
> +			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);
> +		}
> +		return -EOPNOTSUPP;
> +	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 (!(lmsensor_data && lmsensor_data->profile))
> +		return -EINVAL;
> +
> +	if ((type == hwmon_in && attr == hwmon_in_label) &&
> +		(lmsensor_data->profile->hwmon_in_list_cnt > channel)) {

lmsensor_data->profile->hwmon_in_list_cnt > channel is unnecessary, and
the () around it as well.

> +		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_temp && attr == hwmon_temp_label) &&
> +		(lmsensor_data->profile->temp_list_cnt > channel)) {
> +		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)

Too many line splits. Please reduce to minuimum required.

> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ahc1ec0_read_in(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)
> +{
> +	switch (attr) {
> +	case hwmon_in_input:
> +	case hwmon_in_label:
> +		return 0444;
> +	default:
> +		return 0;
> +	}

The code needs to take channel into account, and return 0 if a voltage sensor
is not supported.

> +}
> +
> +static umode_t
> +ec_temp_in_visible(const void *data, u32 attr, int channel)
> +{
> +	switch (attr) {
> +	case hwmon_temp_input:
> +	case hwmon_temp_crit:
> +	case hwmon_temp_label:
> +		return 0444;
> +	default:
> +		return 0;
> +	}

The code needs to take channel into account, and return 0 if the second
temperature sensor is not supported.

> +}
> +
> +static umode_t
> +ahc1ec0_is_visible(const void *data,
> +					enum hwmon_sensor_types type, u32 attr, int channel)

Now this is a really odd (nd unnecessary) continuation line.

> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ec_hwmon_in_visible(data, attr, channel);
> +	case hwmon_temp:
> +		return ec_temp_in_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_temp_config[] = {
> +	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,

I don't think this works as intended. it only lists one temperature
sensor, but the code clearly suggests that there can be two.

> +	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_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_platform_data *adv_ec_data;
> +	struct ec_hwmon_data *lmsensor_data;
> +
> +	adv_ec_data = dev_get_drvdata(dev->parent);
> +	if (!adv_ec_data)
> +		return -EINVAL;
> +
> +	ret = device_property_read_u32(dev->parent, "advantech,hwmon-profile", &profile);
> +	if (ret < 0) {
> +		dev_err(dev, "get hwmon-profile failed! (%d)", ret);
> +		return ret;
> +	}
> +
> +	if (!(profile < ARRAY_SIZE(advec_profile))) {

Please no double negations.
	if (profile >= ARRAY_SIZE(advec_profile))
is much easier to understand.

> +		dev_err(dev, "not support hwmon profile(%d)!\n", profile);
> +		return -EINVAL;
> +	}
> +
> +	lmsensor_data = devm_kzalloc(dev, sizeof(struct ec_hwmon_data), GFP_KERNEL);
> +	if (!lmsensor_data)
> +		return -ENOMEM;
> +
> +	lmsensor_data->adv_ec_data = adv_ec_data;
> +	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");

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog for Advantech embedded controller
  2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
  2021-01-19  7:26   ` AceLan Kao
@ 2021-01-23 17:00   ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-01-23 17:00 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Wim Van Sebroeck, linux-hwmon, linux-watchdog, AceLan Kao

On Mon, Jan 18, 2021 at 08:37:49PM +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 since V5:
> 	- 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 | 261 +++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/watchdog/ahc1ec0-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7ff941e71b79..1a27836883ac 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1636,6 +1636,17 @@ config NIC7018_WDT
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called nic7018_wdt.
>  
> +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 thie driver as a module, choose M here: the module will be
> +	  called ahc1ec0-wdt.
> +
>  # M68K Architecture
>  
>  config M54xx_WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c74ee19d441..7190811b1e50 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -145,6 +145,7 @@ obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
>  obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
>  obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>  obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> +obj-$(CONFIG_AHC1EC0_WDT) += ahc1ec0-wdt.o
>  obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
>  
>  # M68K Architecture
> diff --git a/drivers/watchdog/ahc1ec0-wdt.c b/drivers/watchdog/ahc1ec0-wdt.c
> new file mode 100644
> index 000000000000..4497b6106b24
> --- /dev/null
> +++ b/drivers/watchdog/ahc1ec0-wdt.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Watchdog Driver for Advantech Embedded Controller chip AHC1EC0
> + *
> + * Copyright 2020, Advantech IIoT Group
> + *
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/ahc1ec0.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +
> +#define DRV_NAME      "ahc1ec0-wdt"
> +
> +struct ec_wdt_data {
> +	struct watchdog_device wdtdev;
> +	struct adv_ec_platform_data *adv_ec_data;
> +	struct notifier_block reboot_nb;
> +	struct mutex lock_ioctl;
> +	int is_enable;
> +	int current_timeout;

is_enable and current_timeout are only written to and never read, and thus pointless.

> +};
> +
> +#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_platform_data *adv_ec_data, unsigned short delay_timeout_in_ms)
> +{
> +	if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_L, delay_timeout_in_ms & 0x00FF)) {

write_hw_ram() is too generic for a device specific function. Please add a
prefix.

> +		pr_err("Failed to set Watchdog Retset Time Low byte.");

Reset ? 

Either case, please drop those error messages. If they happen they would likely
be persistent and end up clogging the kernel log.

> +		return -EINVAL;
> +	}
> +
> +	if (write_hw_ram(adv_ec_data, EC_RESET_DELAY_TIME_H, (delay_timeout_in_ms & 0xFF00) >> 8)) {
> +		pr_err("Failed to set Watchdog Retset Time Hight byte.");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int advwdt_set_heartbeat(unsigned long t)
> +{
> +	if (t < 1 || t > 6553) {
> +		pr_err("%s: the input timeout is out of range.",  __func__);
> +		pr_err("Please choose valid data between 1 ~ 6553.");
> +		return -EINVAL;
> +	}

Unecessary check. The watchdog core performs limit checks.
Repeating them does not add value.

> +
> +	return (t * 10);

Given that, this helper function is quite pointless (and misleading, since
it doesn't set anything).

> +}
> +
> +/* Notifier for system down */
> +static int advwdt_notify_sys(struct notifier_block *nb, unsigned long code, void *data)
> +{
> +	if (code == SYS_DOWN || code == SYS_HALT) {
> +		struct ec_wdt_data *ec_wdt_data;
> +
> +		ec_wdt_data = container_of(nb, struct ec_wdt_data, reboot_nb);
> +		if (!ec_wdt_data)
> +			return NOTIFY_BAD;
> +
> +		/* Turn the WDT off */
> +		if (write_hwram_command(ec_wdt_data->adv_ec_data, EC_WDT_STOP)) {
> +			pr_err("Failed to set Watchdog stop.");
> +			return -EINVAL;
> +		}
> +		ec_wdt_data->is_enable = 0;
> +		pr_info("%s: notify sys shutdown", __func__);
> +	}
> +
> +	return NOTIFY_DONE;
> +}

Please use the watchdog subsystem instead of declaring yor own notifier.

> +
> +static int ec_wdt_start(struct watchdog_device *wdd)
> +{
> +	int ret;
> +	int timeout, timeout_in_ms;
> +	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +	struct adv_ec_platform_data *adv_ec_data;
> +
> +	dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +	adv_ec_data = ec_wdt_data->adv_ec_data;
> +	timeout = wdd->timeout; /* The watchdog devices timeout value (in seconds). */

'timeout' is used just once. Just use wdd->timeout directly below.

> +
> +	mutex_lock(&ec_wdt_data->lock_ioctl);

The watchdog core ensures that there will be no more than one caller.
What is this lock supposed to protect ?

> +
> +	timeout_in_ms = advwdt_set_heartbeat(timeout);

timeout_in_ms is the timeout in 10th of seconds. "_ms" is misleading.

> +	if (timeout_in_ms < 0) {
> +		mutex_unlock(&ec_wdt_data->lock_ioctl);
> +		return timeout_in_ms;
> +	}
> +
> +	ret = set_delay(adv_ec_data, (unsigned short)(timeout_in_ms-1));

Unnecessary type cast.

> +	if (ret) {
> +		dev_err(wdd->parent, "Failed to set Watchdog delay (ret=%x).\n", ret);

This driver is extremely noisy when it comes to error messages.

> +		mutex_unlock(&ec_wdt_data->lock_ioctl);
> +		return ret;
> +	}
> +	ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);

Errors from this call are ignored. If that is on purpose, the assignment
to 'ret' is unnecessary.

> +	ret = write_hwram_command(adv_ec_data, EC_WDT_START);
> +	if (ret) {
> +		dev_err(wdd->parent, "Failed to set Watchdog start (ret=%x).\n", ret);
> +		mutex_unlock(&ec_wdt_data->lock_ioctl);
> +		return ret;

This should be  handled with a goto to the end of the function.
See error handling in coding style.

> +	}
> +	ec_wdt_data->is_enable = 1;
> +	ec_wdt_data->current_timeout = timeout_in_ms/10;

The conversion from timeout -> timeout_in_ms -> timeout
seems to be quite pointless.

> +
> +	mutex_unlock(&ec_wdt_data->lock_ioctl);
> +	return 0;
> +}
> +
> +static int ec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	int ret;
> +	struct ec_wdt_data *ec_wdt_data = watchdog_get_drvdata(wdd);
> +	struct adv_ec_platform_data *adv_ec_data;
> +
> +	dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +	adv_ec_data = ec_wdt_data->adv_ec_data;
> +
> +	mutex_lock(&ec_wdt_data->lock_ioctl);
> +	ret = write_hwram_command(adv_ec_data, EC_WDT_STOP);
> +	mutex_unlock(&ec_wdt_data->lock_ioctl);
> +	if (ret)
> +		pr_err("Failed to set Watchdog stop.");
> +	else
> +		ec_wdt_data->is_enable = 0;
> +
> +	return ret;
> +}
> +
> +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_platform_data *adv_ec_data;
> +
> +	dev_dbg(wdd->parent, "%s\n", __func__);
> +
> +	adv_ec_data = ec_wdt_data->adv_ec_data;
> +
> +	mutex_lock(&ec_wdt_data->lock_ioctl);
> +	ret = write_hwram_command(adv_ec_data, EC_WDT_RESET);
> +	mutex_unlock(&ec_wdt_data->lock_ioctl);
> +	if (ret) {
> +		pr_err("Failed to set Watchdog reset.");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ec_wdt_set_timeout(struct watchdog_device *wdd,
> +				unsigned int timeout)
> +{
> +	dev_dbg(wdd->parent, "%s, timeout=%d\n", __func__, 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_platform_data *adv_ec_data;
> +	struct ec_wdt_data *ec_wdt_data;
> +	struct watchdog_device *wdd;
> +
> +	dev_info(dev, "watchdog probe start\n");
> +
> +	adv_ec_data = dev_get_drvdata(dev->parent);
> +	if (!adv_ec_data)
> +		return -EINVAL;
> +
> +	ec_wdt_data = devm_kzalloc(dev, sizeof(struct ec_wdt_data), GFP_KERNEL);
> +	if (!ec_wdt_data)
> +		return -ENOMEM;
> +
> +	mutex_init(&ec_wdt_data->lock_ioctl);
> +
> +	ec_wdt_data->adv_ec_data = adv_ec_data;
> +	wdd = &ec_wdt_data->wdtdev;
> +
> +	watchdog_init_timeout(&ec_wdt_data->wdtdev, 0, dev);
> +
> +	//watchdog_set_nowayout(&ec_wdt_data->wdtdev, WATCHDOG_NOWAYOUT);

Please no C++ comments in the code.

> +	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;
> +	ec_wdt_data->is_enable = 0;
> +	ec_wdt_data->current_timeout = EC_WDT_DEFAULT_TIMEOUT;
> +
> +	watchdog_stop_on_unregister(wdd);
> +
> +	ec_wdt_data->reboot_nb.notifier_call = advwdt_notify_sys;
> +	ret = devm_register_reboot_notifier(dev, &ec_wdt_data->reboot_nb);
> +	if (ret) {
> +		dev_err(dev, "watchdog%d: Cannot register reboot notifier (%d)\n",
> +			wdd->id, ret);
> +		return ret;
> +	}

Use watchdog_stop_on_reboot().

> +
> +	ret = devm_watchdog_register_device(dev, wdd);
> +	if (ret != 0)
> +		dev_err(dev, "watchdog_register_device() failed: %d\n",
> +			ret);

And right afterwards "watchdog register success", and the error
is ignored.

> +
> +	dev_info(dev, "watchdog register success\n");
> +
> +	return 0;
> +}
> +
> +static struct platform_driver adv_wdt_drv = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = adv_ec_wdt_probe,
> +};
> +module_platform_driver(adv_wdt_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Driver.");
> +MODULE_AUTHOR("Campion Kang <campion.kang@advantech.com.tw>");
> +MODULE_VERSION("1.0");
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon for Advantech embedded controller
       [not found]     ` <66338d379bb14c2fbd8a6507075384ce@Taipei11.ADVANTECH.CORP>
@ 2021-01-28 15:13       ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-01-28 15:13 UTC (permalink / raw)
  To: Campion.Kang
  Cc: Lee Jones, Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Wim Van Sebroeck, linux-hwmon, linux-watchdog, AceLan Kao

On 1/28/21 2:28 AM, Campion.Kang wrote:
> Hello Guenter,
> 
> Thanks your review and suggestion. I will fix them. 
> One issue I am not clear, Documentation is missing, which document?
> Did you mean this file Documentation\devicetree\bindings\mfd\ahc1ec0.yaml? If yes, it submit in this patch "[PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0".
> Would you kindly give me more information? 
> 

Per Documentation/hwmon/submitting-patches.rst:

3. New drivers
--------------
...
* Document the driver in Documentation/hwmon/<driver_name>.rst.

Please read and follow that document. It will save us both a lot of time.

Thanks,
Guenter

> Best regards,
> Campion
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Sunday, January 24, 2021 12:35 AM
>> To: Campion.Kang <Campion.Kang@advantech.com.tw>
>> Cc: Lee Jones <lee.jones@linaro.org>; Rob Herring <robh+dt@kernel.org>;
>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Jean Delvare
>> <jdelvare@suse.com>; Wim Van Sebroeck <wim@linux-watchdog.org>;
>> linux-hwmon@vger.kernel.org; linux-watchdog@vger.kernel.org; AceLan Kao
>> <chia-lin.kao@canonical.com>
>> Subject: Re: [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon
>> for Advantech embedded controller
>>
>> On Mon, Jan 18, 2021 at 08:37:48PM +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 since V5:
>>> 	- 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>
>>> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
>>
>> checkpatch --strict says:
>>
>> total: 0 errors, 1 warnings, 10 checks, 683 lines checked
>>
>> The MAINTAINERS warning is irrelevant, but there are a number of
>> alignment and other style issues. Please fix those.
>>
>>> ---
>>>  drivers/hwmon/Kconfig         |  10 +
>>>  drivers/hwmon/Makefile        |   1 +
>>>  drivers/hwmon/ahc1ec0-hwmon.c | 660
>>> ++++++++++++++++++++++++++++++++++
>>
>> Documentation is missing.
>>
>>>  3 files changed, 671 insertions(+)
>>>  create mode 100644 drivers/hwmon/ahc1ec0-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index
>>> 1ecf697d8d99..bfa007026679 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -2139,6 +2139,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
>>> 09a86c5e1d29..0c37747e8c4f 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -45,6 +45,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_AMD_ENERGY) += amd_energy.o
>>>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>>>  obj-$(CONFIG_SENSORS_ARM_SCMI)	+= scmi-hwmon.o
>>> diff --git a/drivers/hwmon/ahc1ec0-hwmon.c
>>> b/drivers/hwmon/ahc1ec0-hwmon.c new file mode 100644 index
>>> 000000000000..688f07e6a6e0
>>> --- /dev/null
>>> +++ b/drivers/hwmon/ahc1ec0-hwmon.c
>>> @@ -0,0 +1,660 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * HWMON Driver for Advantech Embedded Controller chip AHC1EC0
>>> + *
>>> + * Copyright 2020, Advantech IIoT Group
>>> + *
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/mfd/ahc1ec0.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/property.h>
>>> +#include <linux/string.h>
>>> +#include <linux/types.h>
>>> +
>>> +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 temp_list_cnt;
>>> +	int *hwmon_in_list;
>>> +	int *temp_list;
>>> +};
>>> +
>>> +struct ec_hwmon_data {
>>> +	struct device *dev;
>>> +	struct device *hwmon_dev;
>>> +	struct adv_ec_platform_data *adv_ec_data;
>>> +	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;
>>> +};
>>> +
>>> +static int get_ec_in_vbat_input(struct device *dev, long *val);
>>> +static int get_ec_in_v5_input(struct device *dev, long *val); static
>>> +int get_ec_in_v12_input(struct device *dev, long *val); static int
>>> +get_ec_in_vcore_input(struct device *dev, long *val); static int
>>> +get_ec_current1_input(struct device *dev, long *val); static int
>>> +get_ec_cpu_temp(struct device *dev, long *val); static int
>>> +get_ec_sys_temp(struct device *dev, long *val);
>>> +
>>
>> Please reorder code to not require those declarations.
>>
>>> +const struct ec_hwmon_attrs ec_hwmon_in_attr_template[] = {
>>> +	{"VBAT",	0444, get_ec_in_vbat_input},	// in1
>>> +	{"5VSB",	0444, get_ec_in_v5_input},	// in2
>>> +	{"Vin",		0444, get_ec_in_v12_input},	// in3 (== in8)
>>> +	{"VCORE",	0444, get_ec_in_vcore_input},	// in4
>>> +	{"Vin1",	0444, NULL},	// in5
>>> +	{"Vin2",	0444, NULL},	// in6
>>> +	{"System Voltage", 0444, NULL},	// in7
>>> +	{"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},
>>> +};
>>> +
>>> +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,
>>> +	EC_HWMON_IN_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,
>>> +	EC_HWMON_IN_CURRENT,
>>> +};
>>> +
>>> +static int hwmon_in_list_1[] = {
>>> +	EC_HWMON_IN_VBAT,
>>> +	EC_HWMON_IN_5VSB,
>>> +	EC_HWMON_IN_12V,
>>> +	EC_HWMON_IN_VCORE,
>>> +};
>>> +
>>> +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[] = {
>>> +	/*
>>> +	 * 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
>>
>> What is all this ? Please add a brief explanation (affected hardware ? chips ?)
>>
>>> +	 * [0] AHC1EC0_HWMON_PRO_TEMPLATE
>>> +	 */
>>> +	{
>>> +		.resolution = 2929,
>>> +		.r1 = 1912,
>>> +		.r2 = 1000,
>>> +		.offset = 0,
>>> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_0),
>>> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
>>> +		.hwmon_in_list = hwmon_in_list_0,
>>> +		.temp_list = temp_list_0,
>>> +	},
>>> +	/*
>>> +	 * 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
>>> +	 * [1] AHC1EC0_HWMON_PRO_TPC5XXX
>>> +	 */
>>> +	{
>>> +		.resolution = 2929,
>>> +		.r1 = 1912,
>>> +		.r2 = 1000,
>>> +		.offset = 0,
>>> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
>>> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
>>> +		.hwmon_in_list = hwmon_in_list_1,
>>> +		.temp_list = temp_list_0,
>>> +	},
>>> +	/* PR/VR4
>>> +	 * [2] AHC1EC0_HWMON_PRO_PRVR4
>>> +	 */
>>> +	{
>>> +		.resolution = 2929,
>>> +		.r1 = 1912,
>>> +		.r2 = 1000,
>>> +		.offset = 0,
>>> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
>>> +		.temp_list_cnt = ARRAY_SIZE(temp_list_1),
>>> +		.hwmon_in_list = hwmon_in_list_1,
>>> +		.temp_list = temp_list_1,
>>> +	},
>>> +	/* UNO-2271G-E22AE/E23AE/E022AE/E023AE,UNO-420
>>> +	 * [3] AHC1EC0_HWMON_PRO_UNO2271G
>>> +	 */
>>> +	{
>>> +		.resolution = 2929,
>>> +		.r1 = 1912,
>>> +		.r2 = 1000,
>>> +		.offset = 0,
>>> +		.hwmon_in_list_cnt = ARRAY_SIZE(hwmon_in_list_1),
>>> +		.temp_list_cnt = ARRAY_SIZE(temp_list_0),
>>> +		.hwmon_in_list = hwmon_in_list_1,
>>> +		.temp_list = temp_list_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_platform_data *adv_ec_data =
>> lmsensor_data->adv_ec_data;
>>> +	struct ec_dynamic_table *dym_tbl = adv_ec_data->dym_tbl;
>>> +
>>> +	if (profile >= ARRAY_SIZE(advec_profile))
>>> +		return;
>>
>> This can not happen. Please no unnecessary error checks.
>>
>>> +
>>> +	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_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	temp = read_ad_value(adv_ec_data, 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);
>>> +
>>
>> Please no empty lines in variable declarations.
>>
>>> +	struct ec_hw_pin_table *ptbl = &lmsensor_data->pin_tbl;
>>> +	struct adv_hwmon_profile *profile = lmsensor_data->profile;
>>> +	struct adv_ec_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	temp = read_ad_value(adv_ec_data, ptbl->v12[0], ptbl->v12[1]);
>>> +	if (temp == -1)
>>> +		temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	temp = read_ad_value(adv_ec_data, 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_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	temp = read_ad_value(adv_ec_data, 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) {
>>> +	#define EC_ACPI_THERMAL1_REMOTE_TEMP 0x61
>>
>> Pease move defines to top of file. Also, please use
>>
>> #define<space>IDENTIFER<tab>value
>>
>>> +
>>> +	unsigned char value;
>>> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
>>> +	struct adv_ec_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_REMOTE_TEMP,
>> &value);
>>
>> This is not an appropriate function name for a device specific exported
>> function. The function name should have a prefix.
>>
>> Also, the error code from read_acpi_value needs to be checked and
>> handled.
>>
>>> +	*val = 1000 * value;
>>> +	return 0;
>>> +}
>>> +
>>> +static int get_ec_sys_temp(struct device *dev, long *val) {
>>> +	#define EC_ACPI_THERMAL1_LOCAL_TEMP 0x60
>>> +
>>> +	unsigned char value;
>>> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
>>> +	struct adv_ec_platform_data *adv_ec_data =
>>> +lmsensor_data->adv_ec_data;
>>> +
>>> +	read_acpi_value(adv_ec_data, EC_ACPI_THERMAL1_LOCAL_TEMP,
>> &value);
>>> +	*val = 1000 * value;
>>> +	return 0;
>>> +}
>>> +
>>> +
>>> +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 (!(lmsensor_data && lmsensor_data->profile))
>>> +		return -EINVAL;
>>
>> This can not happen. Also, please don't use double negations.
>>
>>> +
>>> +	if (attr == hwmon_in_input &&
>>> +		lmsensor_data->profile->hwmon_in_list_cnt > channel) {
>>
>> This is unnecessary. Again, please drop all those unnecessary checks.
>>
>>> +		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_temp(struct device *dev, u32 attr, int channel, long
>>> +*val) {
>>> +	struct ec_hwmon_data *lmsensor_data = dev_get_drvdata(dev);
>>> +
>>> +	if (!(lmsensor_data && lmsensor_data->profile))
>>> +		return -EINVAL;
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +		if (lmsensor_data->profile->temp_list_cnt > channel) {
>>> +			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);
>>> +		}
>>> +		return -EOPNOTSUPP;
>>> +	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 (!(lmsensor_data && lmsensor_data->profile))
>>> +		return -EINVAL;
>>> +
>>> +	if ((type == hwmon_in && attr == hwmon_in_label) &&
>>> +		(lmsensor_data->profile->hwmon_in_list_cnt > channel)) {
>>
>> lmsensor_data->profile->hwmon_in_list_cnt > channel is unnecessary, and
>> the () around it as well.
>>
>>> +		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_temp && attr == hwmon_temp_label) &&
>>> +		(lmsensor_data->profile->temp_list_cnt > channel)) {
>>> +		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)
>>
>> Too many line splits. Please reduce to minuimum required.
>>
>>> +{
>>> +	switch (type) {
>>> +	case hwmon_in:
>>> +		return ahc1ec0_read_in(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) {
>>> +	switch (attr) {
>>> +	case hwmon_in_input:
>>> +	case hwmon_in_label:
>>> +		return 0444;
>>> +	default:
>>> +		return 0;
>>> +	}
>>
>> The code needs to take channel into account, and return 0 if a voltage
>> sensor is not supported.
>>
>>> +}
>>> +
>>> +static umode_t
>>> +ec_temp_in_visible(const void *data, u32 attr, int channel) {
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +	case hwmon_temp_crit:
>>> +	case hwmon_temp_label:
>>> +		return 0444;
>>> +	default:
>>> +		return 0;
>>> +	}
>>
>> The code needs to take channel into account, and return 0 if the second
>> temperature sensor is not supported.
>>
>>> +}
>>> +
>>> +static umode_t
>>> +ahc1ec0_is_visible(const void *data,
>>> +					enum hwmon_sensor_types type, u32 attr, int
>> channel)
>>
>> Now this is a really odd (nd unnecessary) continuation line.
>>
>>> +{
>>> +	switch (type) {
>>> +	case hwmon_in:
>>> +		return ec_hwmon_in_visible(data, attr, channel);
>>> +	case hwmon_temp:
>>> +		return ec_temp_in_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_temp_config[] = {
>>> +	HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_LABEL,
>>
>> I don't think this works as intended. it only lists one temperature sensor, but
>> the code clearly suggests that there can be two.
>>
>>> +	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_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_platform_data *adv_ec_data;
>>> +	struct ec_hwmon_data *lmsensor_data;
>>> +
>>> +	adv_ec_data = dev_get_drvdata(dev->parent);
>>> +	if (!adv_ec_data)
>>> +		return -EINVAL;
>>> +
>>> +	ret = device_property_read_u32(dev->parent,
>> "advantech,hwmon-profile", &profile);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "get hwmon-profile failed! (%d)", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (!(profile < ARRAY_SIZE(advec_profile))) {
>>
>> Please no double negations.
>> 	if (profile >= ARRAY_SIZE(advec_profile)) is much easier to understand.
>>
>>> +		dev_err(dev, "not support hwmon profile(%d)!\n", profile);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	lmsensor_data = devm_kzalloc(dev, sizeof(struct ec_hwmon_data),
>> GFP_KERNEL);
>>> +	if (!lmsensor_data)
>>> +		return -ENOMEM;
>>> +
>>> +	lmsensor_data->adv_ec_data = adv_ec_data;
>>> +	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");


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings
  2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
@ 2021-02-04 10:32   ` Lee Jones
  0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2021-02-04 10:32 UTC (permalink / raw)
  To: Campion Kang
  Cc: Rob Herring, linux-kernel, devicetree, Jean Delvare,
	Guenter Roeck, Wim Van Sebroeck, linux-hwmon, linux-watchdog,
	AceLan Kao

On Mon, 18 Jan 2021, Campion Kang wrote:

> This files defines the sud-device types and hwmon profiles support by
> Advantech embedded controller.
> 
> Signed-off-by: Campion Kang <campion.kang@advantech.com.tw>
> ---
>  include/dt-bindings/mfd/ahc1ec0-dt.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 include/dt-bindings/mfd/ahc1ec0-dt.h

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0
  2021-01-18 12:37 ` [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0 Campion Kang
@ 2021-02-05 21:21   ` Rob Herring
  0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2021-02-05 21:21 UTC (permalink / raw)
  To: Campion Kang
  Cc: Lee Jones, linux-kernel, devicetree, Jean Delvare, Guenter Roeck,
	Wim Van Sebroeck, linux-hwmon, linux-watchdog, AceLan Kao

On Mon, Jan 18, 2021 at 08:37:46PM +0800, Campion Kang wrote:
> Add DT binding schema for Advantech embedded controller AHC1EC0.
> 
> Changed since V5:
> 	- 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      | 69 +++++++++++++++++++
>  1 file changed, 69 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..40af14bb9c0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ahc1ec0.yaml
> @@ -0,0 +1,69 @@
> +# 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, etc. Advantech related applications
> +  can control the whole system via these functions.
> +
> +properties:
> +  compatible:
> +    const: advantech,ahc1ec0
> +
> +  advantech,sub-dev-nb:
> +    description:
> +      The number of sub-devices specified in the platform.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    maxItems: 1

You don't need this, just count the length of the next property:

> +
> +  advantech,sub-dev:
> +    description:
> +      A list of the sub-devices supported in the platform. Defines for the
> +      appropriate values can found in dt-bindings/mfd/ahc1ec0-dt.h.
> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> +    minItems: 1
> +    maxItems: 6

But as I said before, this binding is odd. It doesn't look like how we 
do any other MFD. Either we have child nodes or they are implicit.

> +
> +  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
> +
> +required:
> +  - compatible
> +  - advantech,sub-dev-nb
> +  - advantech,sub-dev
> +
> +if:
> +  properties:
> +    advantech,sub-dev:
> +      contains:
> +        const: 0x3
> +then:
> +  required:
> +    - advantech,hwmon-profile
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/mfd/ahc1ec0-dt.h>
> +    ahc1ec0 {
> +        compatible = "advantech,ahc1ec0";
> +
> +        advantech,sub-dev-nb = <2>;
> +        advantech,sub-dev = <AHC1EC0_SUBDEV_HWMON
> +                             AHC1EC0_SUBDEV_WDT>;
> +
> +        advantech,hwmon-profile = <AHC1EC0_HWMON_PRO_UNO2271G>;
> +    };
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
2021-02-04 10:32   ` Lee Jones
2021-01-18 12:37 ` [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0 Campion Kang
2021-02-05 21:21   ` Rob Herring
2021-01-18 12:37 ` [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller Campion Kang
2021-01-19  7:25   ` AceLan Kao
2021-01-19  8:23     ` Lee Jones
2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
2021-01-19  7:26   ` AceLan Kao
2021-01-23 16:35   ` Guenter Roeck
     [not found]     ` <66338d379bb14c2fbd8a6507075384ce@Taipei11.ADVANTECH.CORP>
2021-01-28 15:13       ` Guenter Roeck
2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
2021-01-19  7:26   ` AceLan Kao
2021-01-23 17:00   ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git