[PATCH] hwmon: (nct6775) Support access via Asus WMI

[PATCH] hwmon: (nct6775) Support access via Asus WMI

[Bernhard Seibold]

Support accessing the chip via Asus WMI. This enables the driver to work
on boards where the IO region is reserved by ACPI.

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
Tested-by: Artem S. Tashkinov <aros@gmx.com>
---
 drivers/hwmon/Kconfig   |  15 ++
 drivers/hwmon/nct6775.c | 406 ++++++++++++++++++++++++++++++----------
 2 files changed, 318 insertions(+), 103 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..1f374ccd8f54 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1444,6 +1444,21 @@ config SENSORS_NCT6775
 	  This driver can also be built as a module. If so, the module
 	  will be called nct6775.
 
+if SENSORS_NCT6775
+
+config SENSORS_NCT6775_ASUSWMI
+	bool "Support access via Asus WMI"
+	depends on ACPI_WMI
+	default y if ACPI_WMI
+	help
+	  Support accessing the NCT677x via Asus WMI functions.
+
+	  On mainboards that support this way of accessing the chip,
+	  the driver will usually not work without this option since
+	  in these mainboards, ACPI will mark the I/O port as used.
+
+endif # SENSORS_NCT6775
+
 config SENSORS_NCT7802
 	tristate "Nuvoton NCT7802Y"
 	depends on I2C
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 5bd15622a85f..3b26afd4ff84 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -55,6 +55,7 @@
 #include <linux/dmi.h>
 #include <linux/io.h>
 #include <linux/nospec.h>
+#include <linux/wmi.h>
 #include "lm75.h"
 
 #define USE_ALTERNATE
@@ -133,30 +134,150 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
 
 enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
 
+struct nct6775_sio_data {
+	int sioreg;
+	int ld;
+	enum kinds kind;
+};
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+
+#define ASUSWMI_MGMT2_GUID		"466747A0-70EC-11DE-8A39-0800200C9A66"
+#define ASUSWMI_METHODID_RSIO		0x5253494F
+#define ASUSWMI_METHODID_WSIO		0x5753494F
+#define ASUSWMI_METHODID_RHWM		0x5248574D
+#define ASUSWMI_METHODID_WHWM		0x5748574D
+#define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
+
+static int asuswmi_evaluate_method(u32 method_id,
+		u8 bank, u8 reg, u8 val, u32 *retval)
+{
+	u32 args = bank | (reg << 8) | (val << 16);
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	union acpi_object *obj;
+	u32 tmp = 0;
+
+	status = wmi_evaluate_method(ASUSWMI_MGMT2_GUID, 0, method_id,
+				     &input, &output);
+
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	obj = (union acpi_object *)output.pointer;
+	if (obj && obj->type == ACPI_TYPE_INTEGER)
+		tmp = (u32) obj->integer.value;
+
+	if (retval)
+		*retval = tmp;
+
+	kfree(obj);
+
+	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
+		return -ENODEV;
+	return 0;
+}
+
+static inline int
+nct6775_asuswmi_write(u8 bank, u8 reg, u8 val)
+{
+	return asuswmi_evaluate_method(ASUSWMI_METHODID_WHWM, bank, reg, val, 0);
+}
+
+static inline int
+nct6775_asuswmi_read(u8 bank, u8 reg, u8 *val)
+{
+	int ret;
+	u32 tmp;
+
+	ret = asuswmi_evaluate_method(ASUSWMI_METHODID_RHWM, bank, reg, 0, &tmp);
+	if (val)
+		*val = tmp & 0xff;
+	return ret;
+}
+
+static inline int
+superio_wmi_inb(struct nct6775_sio_data *sio_data, int reg)
+{
+	int tmp;
+
+	asuswmi_evaluate_method(ASUSWMI_METHODID_RSIO,
+			sio_data->ld, reg, 0, &tmp);
+	return tmp;
+}
+
 static inline void
-superio_outb(int ioreg, int reg, int val)
+superio_wmi_outb(struct nct6775_sio_data *sio_data, int reg, int val)
+{
+	asuswmi_evaluate_method(ASUSWMI_METHODID_WSIO,
+			sio_data->ld, reg, val, 0);
+}
+
+static inline bool
+superio_use_asuswmi(struct nct6775_sio_data *sio_data)
 {
+	return sio_data->sioreg == 0;
+}
+
+#endif
+
+static inline void
+superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
+{
+	int ioreg = sio_data->sioreg;
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (superio_use_asuswmi(sio_data)) {
+		superio_wmi_outb(sio_data, reg, val);
+		return;
+	}
+#endif
+
 	outb(reg, ioreg);
 	outb(val, ioreg + 1);
 }
 
 static inline int
-superio_inb(int ioreg, int reg)
+superio_inb(struct nct6775_sio_data *sio_data, int reg)
 {
+	int ioreg = sio_data->sioreg;
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (superio_use_asuswmi(sio_data))
+		return superio_wmi_inb(sio_data, reg);
+#endif
+
 	outb(reg, ioreg);
 	return inb(ioreg + 1);
 }
 
 static inline void
-superio_select(int ioreg, int ld)
+superio_select(struct nct6775_sio_data *sio_data, int ld)
 {
+	int ioreg = sio_data->sioreg;
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (superio_use_asuswmi(sio_data)) {
+		sio_data->ld = ld;
+		return;
+	}
+#endif
+
 	outb(SIO_REG_LDSEL, ioreg);
 	outb(ld, ioreg + 1);
 }
 
 static inline int
-superio_enter(int ioreg)
+superio_enter(struct nct6775_sio_data *sio_data)
 {
+	int ioreg = sio_data->sioreg;
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (superio_use_asuswmi(sio_data))
+		return 0;
+#endif
+
 	/*
 	 * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
 	 */
@@ -170,8 +291,15 @@ superio_enter(int ioreg)
 }
 
 static inline void
-superio_exit(int ioreg)
+superio_exit(struct nct6775_sio_data *sio_data)
 {
+	int ioreg = sio_data->sioreg;
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (superio_use_asuswmi(sio_data))
+		return;
+#endif
+
 	outb(0xaa, ioreg);
 	outb(0x02, ioreg);
 	outb(0x02, ioreg + 1);
@@ -1217,11 +1345,6 @@ struct nct6775_data {
 	u8 sio_reg_enable;
 };
 
-struct nct6775_sio_data {
-	int sioreg;
-	enum kinds kind;
-};
-
 struct sensor_device_template {
 	struct device_attribute dev_attr;
 	union {
@@ -1407,6 +1530,15 @@ static bool is_word_sized(struct nct6775_data *data, u16 reg)
 	return false;
 }
 
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+
+static bool use_asuswmi(struct nct6775_data *data)
+{
+	return (data->addr <= 0);
+}
+
+#endif
+
 /*
  * On older chips, only registers 0x50-0x5f are banked.
  * On more recent chips, all registers are banked.
@@ -1417,6 +1549,13 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
 {
 	u8 bank = reg >> 8;
 
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (use_asuswmi(data)) {
+		data->bank = bank;
+		return;
+	}
+#endif
+
 	if (data->bank != bank) {
 		outb_p(NCT6775_REG_BANK, data->addr + ADDR_REG_OFFSET);
 		outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -1427,8 +1566,25 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
 static u16 nct6775_read_value(struct nct6775_data *data, u16 reg)
 {
 	int res, word_sized = is_word_sized(data, reg);
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	u8 tmp;
+#endif
 
 	nct6775_set_bank(data, reg);
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (use_asuswmi(data)) {
+		nct6775_asuswmi_read(data->bank, reg, &tmp);
+		res = (tmp & 0xff);
+		if (word_sized) {
+			nct6775_asuswmi_read(data->bank,
+					(reg & 0xff) + 1, &tmp);
+			res = (res << 8) + (tmp & 0xff);
+		}
+		return res;
+	}
+#endif
+
 	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
 	res = inb_p(data->addr + DATA_REG_OFFSET);
 	if (word_sized) {
@@ -1444,6 +1600,23 @@ static int nct6775_write_value(struct nct6775_data *data, u16 reg, u16 value)
 	int word_sized = is_word_sized(data, reg);
 
 	nct6775_set_bank(data, reg);
+
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+	if (use_asuswmi(data)) {
+		if (word_sized) {
+			nct6775_asuswmi_write(data->bank, (reg & 0xff),
+					(value >> 8) & 0xff);
+			nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
+					value & 0xff);
+		} else {
+			nct6775_asuswmi_write(data->bank, (reg & 0xff),
+					value);
+		}
+
+		return 0;
+	}
+#endif
+
 	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
 	if (word_sized) {
 		outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
@@ -3410,6 +3583,7 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	       const char *buf, size_t count)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
+	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
 	int nr = to_sensor_dev_attr(attr)->index - INTRUSION_ALARM_BASE;
 	unsigned long val;
 	u8 reg;
@@ -3425,19 +3599,19 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	 * The CR registers are the same for all chips, and not all chips
 	 * support clearing the caseopen status through "regular" registers.
 	 */
-	ret = superio_enter(data->sioreg);
+	ret = superio_enter(sio_data);
 	if (ret) {
 		count = ret;
 		goto error;
 	}
 
-	superio_select(data->sioreg, NCT6775_LD_ACPI);
-	reg = superio_inb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
+	superio_select(sio_data, NCT6775_LD_ACPI);
+	reg = superio_inb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
 	reg |= NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
 	reg &= ~NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
-	superio_exit(data->sioreg);
+	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	superio_exit(sio_data);
 
 	data->valid = false;	/* Force cache refresh */
 error:
@@ -3542,29 +3716,29 @@ static inline void nct6775_init_device(struct nct6775_data *data)
 }
 
 static void
-nct6775_check_fan_inputs(struct nct6775_data *data)
+nct6775_check_fan_inputs(struct nct6775_data *data,
+		struct nct6775_sio_data *sio_data)
 {
 	bool fan3pin = false, fan4pin = false, fan4min = false;
 	bool fan5pin = false, fan6pin = false, fan7pin = false;
 	bool pwm3pin = false, pwm4pin = false, pwm5pin = false;
 	bool pwm6pin = false, pwm7pin = false;
-	int sioreg = data->sioreg;
 
 	/* Store SIO_REG_ENABLE for use during resume */
-	superio_select(sioreg, NCT6775_LD_HWM);
-	data->sio_reg_enable = superio_inb(sioreg, SIO_REG_ENABLE);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	data->sio_reg_enable = superio_inb(sio_data, SIO_REG_ENABLE);
 
 	/* fan4 and fan5 share some pins with the GPIO and serial flash */
 	if (data->kind == nct6775) {
-		int cr2c = superio_inb(sioreg, 0x2c);
+		int cr2c = superio_inb(sio_data, 0x2c);
 
 		fan3pin = cr2c & BIT(6);
 		pwm3pin = cr2c & BIT(7);
 
 		/* On NCT6775, fan4 shares pins with the fdc interface */
-		fan4pin = !(superio_inb(sioreg, 0x2A) & 0x80);
+		fan4pin = !(superio_inb(sio_data, 0x2A) & 0x80);
 	} else if (data->kind == nct6776) {
-		bool gpok = superio_inb(sioreg, 0x27) & 0x80;
+		bool gpok = superio_inb(sio_data, 0x27) & 0x80;
 		const char *board_vendor, *board_name;
 
 		board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
@@ -3580,7 +3754,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 			if (!strcmp(board_name, "Z77 Pro4-M")) {
 				if ((data->sio_reg_enable & 0xe0) != 0xe0) {
 					data->sio_reg_enable |= 0xe0;
-					superio_outb(sioreg, SIO_REG_ENABLE,
+					superio_outb(sio_data, SIO_REG_ENABLE,
 						     data->sio_reg_enable);
 				}
 			}
@@ -3589,32 +3763,32 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 		if (data->sio_reg_enable & 0x80)
 			fan3pin = gpok;
 		else
-			fan3pin = !(superio_inb(sioreg, 0x24) & 0x40);
+			fan3pin = !(superio_inb(sio_data, 0x24) & 0x40);
 
 		if (data->sio_reg_enable & 0x40)
 			fan4pin = gpok;
 		else
-			fan4pin = superio_inb(sioreg, 0x1C) & 0x01;
+			fan4pin = superio_inb(sio_data, 0x1C) & 0x01;
 
 		if (data->sio_reg_enable & 0x20)
 			fan5pin = gpok;
 		else
-			fan5pin = superio_inb(sioreg, 0x1C) & 0x02;
+			fan5pin = superio_inb(sio_data, 0x1C) & 0x02;
 
 		fan4min = fan4pin;
 		pwm3pin = fan3pin;
 	} else if (data->kind == nct6106) {
-		int cr24 = superio_inb(sioreg, 0x24);
+		int cr24 = superio_inb(sio_data, 0x24);
 
 		fan3pin = !(cr24 & 0x80);
 		pwm3pin = cr24 & 0x08;
 	} else if (data->kind == nct6116) {
-		int cr1a = superio_inb(sioreg, 0x1a);
-		int cr1b = superio_inb(sioreg, 0x1b);
-		int cr24 = superio_inb(sioreg, 0x24);
-		int cr2a = superio_inb(sioreg, 0x2a);
-		int cr2b = superio_inb(sioreg, 0x2b);
-		int cr2f = superio_inb(sioreg, 0x2f);
+		int cr1a = superio_inb(sio_data, 0x1a);
+		int cr1b = superio_inb(sio_data, 0x1b);
+		int cr24 = superio_inb(sio_data, 0x24);
+		int cr2a = superio_inb(sio_data, 0x2a);
+		int cr2b = superio_inb(sio_data, 0x2b);
+		int cr2f = superio_inb(sio_data, 0x2f);
 
 		fan3pin = !(cr2b & 0x10);
 		fan4pin = (cr2b & 0x80) ||			// pin 1(2)
@@ -3630,24 +3804,24 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 		 * NCT6779D, NCT6791D, NCT6792D, NCT6793D, NCT6795D, NCT6796D,
 		 * NCT6797D, NCT6798D
 		 */
-		int cr1a = superio_inb(sioreg, 0x1a);
-		int cr1b = superio_inb(sioreg, 0x1b);
-		int cr1c = superio_inb(sioreg, 0x1c);
-		int cr1d = superio_inb(sioreg, 0x1d);
-		int cr2a = superio_inb(sioreg, 0x2a);
-		int cr2b = superio_inb(sioreg, 0x2b);
-		int cr2d = superio_inb(sioreg, 0x2d);
-		int cr2f = superio_inb(sioreg, 0x2f);
+		int cr1a = superio_inb(sio_data, 0x1a);
+		int cr1b = superio_inb(sio_data, 0x1b);
+		int cr1c = superio_inb(sio_data, 0x1c);
+		int cr1d = superio_inb(sio_data, 0x1d);
+		int cr2a = superio_inb(sio_data, 0x2a);
+		int cr2b = superio_inb(sio_data, 0x2b);
+		int cr2d = superio_inb(sio_data, 0x2d);
+		int cr2f = superio_inb(sio_data, 0x2f);
 		bool dsw_en = cr2f & BIT(3);
 		bool ddr4_en = cr2f & BIT(4);
 		int cre0;
 		int creb;
 		int cred;
 
-		superio_select(sioreg, NCT6775_LD_12);
-		cre0 = superio_inb(sioreg, 0xe0);
-		creb = superio_inb(sioreg, 0xeb);
-		cred = superio_inb(sioreg, 0xed);
+		superio_select(sio_data, NCT6775_LD_12);
+		cre0 = superio_inb(sio_data, 0xe0);
+		creb = superio_inb(sio_data, 0xeb);
+		cred = superio_inb(sio_data, 0xed);
 
 		fan3pin = !(cr1c & BIT(5));
 		fan4pin = !(cr1c & BIT(6));
@@ -3806,9 +3980,10 @@ static int nct6775_probe(struct platform_device *pdev)
 	int num_attr_groups = 0;
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
-				 DRVNAME))
-		return -EBUSY;
+	if (res)
+		if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
+					 DRVNAME))
+			return -EBUSY;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(struct nct6775_data),
 			    GFP_KERNEL);
@@ -3817,7 +3992,10 @@ static int nct6775_probe(struct platform_device *pdev)
 
 	data->kind = sio_data->kind;
 	data->sioreg = sio_data->sioreg;
-	data->addr = res->start;
+	if (res)
+		data->addr = res->start;
+	else
+		data->addr = 0;
 	mutex_init(&data->update_lock);
 	data->name = nct6775_device_names[data->kind];
 	data->bank = 0xff;		/* Force initial bank selection */
@@ -4502,11 +4680,11 @@ static int nct6775_probe(struct platform_device *pdev)
 	/* Initialize the chip */
 	nct6775_init_device(data);
 
-	err = superio_enter(sio_data->sioreg);
+	err = superio_enter(sio_data);
 	if (err)
 		return err;
 
-	cr2a = superio_inb(sio_data->sioreg, 0x2a);
+	cr2a = superio_inb(sio_data, 0x2a);
 	switch (data->kind) {
 	case nct6775:
 		data->have_vid = (cr2a & 0x40);
@@ -4532,16 +4710,16 @@ static int nct6775_probe(struct platform_device *pdev)
 	 * We can get the VID input values directly at logical device D 0xe3.
 	 */
 	if (data->have_vid) {
-		superio_select(sio_data->sioreg, NCT6775_LD_VID);
-		data->vid = superio_inb(sio_data->sioreg, 0xe3);
+		superio_select(sio_data, NCT6775_LD_VID);
+		data->vid = superio_inb(sio_data, 0xe3);
 		data->vrm = vid_which_vrm();
 	}
 
 	if (fan_debounce) {
 		u8 tmp;
 
-		superio_select(sio_data->sioreg, NCT6775_LD_HWM);
-		tmp = superio_inb(sio_data->sioreg,
+		superio_select(sio_data, NCT6775_LD_HWM);
+		tmp = superio_inb(sio_data,
 				  NCT6775_REG_CR_FAN_DEBOUNCE);
 		switch (data->kind) {
 		case nct6106:
@@ -4565,15 +4743,15 @@ static int nct6775_probe(struct platform_device *pdev)
 			tmp |= 0x7e;
 			break;
 		}
-		superio_outb(sio_data->sioreg, NCT6775_REG_CR_FAN_DEBOUNCE,
+		superio_outb(sio_data, NCT6775_REG_CR_FAN_DEBOUNCE,
 			     tmp);
 		dev_info(&pdev->dev, "Enabled fan debounce for chip %s\n",
 			 data->name);
 	}
 
-	nct6775_check_fan_inputs(data);
+	nct6775_check_fan_inputs(data, sio_data);
 
-	superio_exit(sio_data->sioreg);
+	superio_exit(sio_data);
 
 	/* Read fan clock dividers immediately */
 	nct6775_init_fan_common(dev, data);
@@ -4613,14 +4791,14 @@ static int nct6775_probe(struct platform_device *pdev)
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-static void nct6791_enable_io_mapping(int sioaddr)
+static void nct6791_enable_io_mapping(struct nct6775_sio_data *sio_data)
 {
 	int val;
 
-	val = superio_inb(sioaddr, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
+	val = superio_inb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
 	if (val & 0x10) {
 		pr_info("Enabling hardware monitor logical device mappings.\n");
-		superio_outb(sioaddr, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
+		superio_outb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
 			     val & ~0x10);
 	}
 }
@@ -4643,29 +4821,29 @@ static int __maybe_unused nct6775_suspend(struct device *dev)
 static int __maybe_unused nct6775_resume(struct device *dev)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
-	int sioreg = data->sioreg;
+	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
 	int i, j, err = 0;
 	u8 reg;
 
 	mutex_lock(&data->update_lock);
 	data->bank = 0xff;		/* Force initial bank selection */
 
-	err = superio_enter(sioreg);
+	err = superio_enter(sio_data);
 	if (err)
 		goto abort;
 
-	superio_select(sioreg, NCT6775_LD_HWM);
-	reg = superio_inb(sioreg, SIO_REG_ENABLE);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	reg = superio_inb(sio_data, SIO_REG_ENABLE);
 	if (reg != data->sio_reg_enable)
-		superio_outb(sioreg, SIO_REG_ENABLE, data->sio_reg_enable);
+		superio_outb(sio_data, SIO_REG_ENABLE, data->sio_reg_enable);
 
 	if (data->kind == nct6791 || data->kind == nct6792 ||
 	    data->kind == nct6793 || data->kind == nct6795 ||
 	    data->kind == nct6796 || data->kind == nct6797 ||
 	    data->kind == nct6798)
-		nct6791_enable_io_mapping(sioreg);
+		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sioreg);
+	superio_exit(sio_data);
 
 	/* Restore limits */
 	for (i = 0; i < data->in_num; i++) {
@@ -4722,18 +4900,19 @@ static struct platform_driver nct6775_driver = {
 };
 
 /* nct6775_find() looks for a '627 in the Super-I/O config space */
-static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
+static int __init nct6775_find(struct nct6775_sio_data *sio_data)
 {
 	u16 val;
 	int err;
 	int addr;
 
-	err = superio_enter(sioaddr);
+	err = superio_enter(sio_data);
 	if (err)
 		return err;
 
-	val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8) |
-		superio_inb(sioaddr, SIO_REG_DEVID + 1);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	val = (superio_inb(sio_data, SIO_REG_DEVID) << 8) |
+		superio_inb(sio_data, SIO_REG_DEVID + 1);
 	if (force_id && val != 0xffff)
 		val = force_id;
 
@@ -4777,38 +4956,37 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	default:
 		if (val != 0xffff)
 			pr_debug("unsupported chip ID: 0x%04x\n", val);
-		superio_exit(sioaddr);
+		superio_exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* We have a known chip, find the HWM I/O address */
-	superio_select(sioaddr, NCT6775_LD_HWM);
-	val = (superio_inb(sioaddr, SIO_REG_ADDR) << 8)
-	    | superio_inb(sioaddr, SIO_REG_ADDR + 1);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	val = (superio_inb(sio_data, SIO_REG_ADDR) << 8)
+	    | superio_inb(sio_data, SIO_REG_ADDR + 1);
 	addr = val & IOREGION_ALIGNMENT;
 	if (addr == 0) {
 		pr_err("Refusing to enable a Super-I/O device with a base I/O port 0\n");
-		superio_exit(sioaddr);
+		superio_exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* Activate logical device if needed */
-	val = superio_inb(sioaddr, SIO_REG_ENABLE);
+	val = superio_inb(sio_data, SIO_REG_ENABLE);
 	if (!(val & 0x01)) {
 		pr_warn("Forcibly enabling Super-I/O. Sensor is probably unusable.\n");
-		superio_outb(sioaddr, SIO_REG_ENABLE, val | 0x01);
+		superio_outb(sio_data, SIO_REG_ENABLE, val | 0x01);
 	}
 
 	if (sio_data->kind == nct6791 || sio_data->kind == nct6792 ||
 	    sio_data->kind == nct6793 || sio_data->kind == nct6795 ||
 	    sio_data->kind == nct6796 || sio_data->kind == nct6797 ||
 	    sio_data->kind == nct6798)
-		nct6791_enable_io_mapping(sioaddr);
+		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sioaddr);
+	superio_exit(sio_data);
 	pr_info("Found %s or compatible chip at %#x:%#x\n",
-		nct6775_sio_names[sio_data->kind], sioaddr, addr);
-	sio_data->sioreg = sioaddr;
+		nct6775_sio_names[sio_data->kind], sio_data->sioreg, addr);
 
 	return addr;
 }
@@ -4819,16 +4997,18 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
  * track of the nct6775 driver. But since we use platform_device_alloc(), we
  * must keep track of the device
  */
-static struct platform_device *pdev[2];
+static struct platform_device *pdev[3];
+
+#define NCT6775_REG_CHIPID 0x58
 
 static int __init sensors_nct6775_init(void)
 {
 	int i, err;
 	bool found = false;
 	int address;
-	struct resource res;
+	struct resource res, siores;
 	struct nct6775_sio_data sio_data;
-	int sioaddr[2] = { 0x2e, 0x4e };
+	int sioaddr[3] = { 0, 0x2e, 0x4e };
 
 	err = platform_driver_register(&nct6775_driver);
 	if (err)
@@ -4842,12 +5022,29 @@ static int __init sensors_nct6775_init(void)
 	 * nct6775 hardware monitor, and call probe()
 	 */
 	for (i = 0; i < ARRAY_SIZE(pdev); i++) {
-		address = nct6775_find(sioaddr[i], &sio_data);
+		siores.name = DRVNAME;
+		siores.start = sioaddr[i];
+		siores.end = sioaddr[i] + 1;
+		siores.flags = IORESOURCE_IO;
+		err = acpi_check_resource_conflict(&siores);
+		if (err)
+			continue;
+
+		sio_data.sioreg = sioaddr[i];
+		address = nct6775_find(&sio_data);
 		if (address <= 0)
 			continue;
 
 		found = true;
 
+#ifdef CONFIG_SENSORS_NCT6775_ASUSWMI
+		// if reading chip id via WMI succeeds, use WMI
+		if (!nct6775_asuswmi_read(0, NCT6775_REG_CHIPID, NULL)) {
+			pr_info("Using Asus WMI to access chip");
+			address = 0;
+		}
+#endif
+
 		pdev[i] = platform_device_alloc(DRVNAME, address);
 		if (!pdev[i]) {
 			err = -ENOMEM;
@@ -4859,27 +5056,30 @@ static int __init sensors_nct6775_init(void)
 		if (err)
 			goto exit_device_put;
 
-		memset(&res, 0, sizeof(res));
-		res.name = DRVNAME;
-		res.start = address + IOREGION_OFFSET;
-		res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
-		res.flags = IORESOURCE_IO;
+		if (address > 0) {
+			memset(&res, 0, sizeof(res));
+			res.name = DRVNAME;
+			res.start = address + IOREGION_OFFSET;
+			res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
+			res.flags = IORESOURCE_IO;
+
+			err = acpi_check_resource_conflict(&res);
+			if (err) {
+				platform_device_put(pdev[i]);
+				pdev[i] = NULL;
+				continue;
+			}
 
-		err = acpi_check_resource_conflict(&res);
-		if (err) {
-			platform_device_put(pdev[i]);
-			pdev[i] = NULL;
-			continue;
+			err = platform_device_add_resources(pdev[i], &res, 1);
+			if (err)
+				goto exit_device_put;
 		}
-
-		err = platform_device_add_resources(pdev[i], &res, 1);
-		if (err)
-			goto exit_device_put;
-
 		/* platform_device_add calls probe() */
 		err = platform_device_add(pdev[i]);
 		if (err)
 			goto exit_device_put;
+
+		break;
 	}
 	if (!found) {
 		err = -ENODEV;
-- 
2.26.3

Re: [PATCH] hwmon: (nct6775) Support access via Asus WMI

[Guenter Roeck]

On 5/5/21 1:12 PM, Bernhard Seibold wrote:
> Support accessing the chip via Asus WMI. This enables the driver to work
> on boards where the IO region is reserved by ACPI.
> 
> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> Tested-by: Artem S. Tashkinov <aros@gmx.com>

No, this makes the driver unmaintainable. There should be a separate
driver which only makes WMI/ACPI accesses for everything.

Guenter

Re: [PATCH] hwmon: (nct6775) Support access via Asus WMI

[Bernhard Seibold]

On 06.05.2021 04:04, Guenter Roeck wrote:
> On 5/5/21 1:12 PM, Bernhard Seibold wrote:
>> Support accessing the chip via Asus WMI. This enables the driver to work
>> on boards where the IO region is reserved by ACPI.
>>
>> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
>> Tested-by: Artem S. Tashkinov <aros@gmx.com>
> 
> No, this makes the driver unmaintainable. There should be a separate
> driver which only makes WMI/ACPI accesses for everything.
> 
> Guenter
> 

I'm not sure what exactly you are suggesting. I assume your suggestion
isn't to duplicate 5000 lines of code in order to avoid having 100 lines
of ifdef'ed code?

Re: [PATCH] hwmon: (nct6775) Support access via Asus WMI

[Guenter Roeck]

On Thu, May 06, 2021 at 09:02:01AM +0200, Bernhard Seibold wrote:
> On 06.05.2021 04:04, Guenter Roeck wrote:
> > On 5/5/21 1:12 PM, Bernhard Seibold wrote:
> >> Support accessing the chip via Asus WMI. This enables the driver to work
> >> on boards where the IO region is reserved by ACPI.
> >>
> >> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> >> Tested-by: Artem S. Tashkinov <aros@gmx.com>
> > 
> > No, this makes the driver unmaintainable. There should be a separate
> > driver which only makes WMI/ACPI accesses for everything.
> > 
> > Guenter
> > 
> 
> I'm not sure what exactly you are suggesting. I assume your suggestion
> isn't to duplicate 5000 lines of code in order to avoid having 100 lines
> of ifdef'ed code?

No idea, quite frankly. Sprinkling the driver with ifdefs and conditional
code for sure isn't the solution. You might consider dropping the ifdefs
and use dmi detection instead, possibly with a module parameter to bypass
the dmi detection. I'd rather have no ifdefs and miss some platforms where
this is untested.

You could try by splitting the single patch into several patches, with one
or more preparatory patches, and maybe write accessor functions to avoid
the runtime conditonals. Oh, and don't use C++ comments.

Guenter

[PATCH 1/3] hwmon: (nct6775) Use sio_data in superio_* (v2)

[Denis Pauk]

Rearrange code for directly use struct nct6775_sio_data in superio_* 
functions

v2: split changes to separate patches

Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
---
 drivers/hwmon/nct6775.c | 173 ++++++++++++++++++++++------------------
 1 file changed, 94 insertions(+), 79 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 5bd15622a85f..3390de4c00f4 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -133,30 +133,43 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
 
 enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
 
+struct nct6775_sio_data {
+	int sioreg;
+	enum kinds kind;
+};
+
 static inline void
-superio_outb(int ioreg, int reg, int val)
+superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
 {
+	int ioreg = sio_data->sioreg;
+
 	outb(reg, ioreg);
 	outb(val, ioreg + 1);
 }
 
 static inline int
-superio_inb(int ioreg, int reg)
+superio_inb(struct nct6775_sio_data *sio_data, int reg)
 {
+	int ioreg = sio_data->sioreg;
+
 	outb(reg, ioreg);
 	return inb(ioreg + 1);
 }
 
 static inline void
-superio_select(int ioreg, int ld)
+superio_select(struct nct6775_sio_data *sio_data, int ld)
 {
+	int ioreg = sio_data->sioreg;
+
 	outb(SIO_REG_LDSEL, ioreg);
 	outb(ld, ioreg + 1);
 }
 
 static inline int
-superio_enter(int ioreg)
+superio_enter(struct nct6775_sio_data *sio_data)
 {
+	int ioreg = sio_data->sioreg;
+
 	/*
 	 * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
 	 */
@@ -170,8 +183,10 @@ superio_enter(int ioreg)
 }
 
 static inline void
-superio_exit(int ioreg)
+superio_exit(struct nct6775_sio_data *sio_data)
 {
+	int ioreg = sio_data->sioreg;
+
 	outb(0xaa, ioreg);
 	outb(0x02, ioreg);
 	outb(0x02, ioreg + 1);
@@ -1217,11 +1232,6 @@ struct nct6775_data {
 	u8 sio_reg_enable;
 };
 
-struct nct6775_sio_data {
-	int sioreg;
-	enum kinds kind;
-};
-
 struct sensor_device_template {
 	struct device_attribute dev_attr;
 	union {
@@ -3410,6 +3420,7 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	       const char *buf, size_t count)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
+	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
 	int nr = to_sensor_dev_attr(attr)->index - INTRUSION_ALARM_BASE;
 	unsigned long val;
 	u8 reg;
@@ -3425,19 +3436,19 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	 * The CR registers are the same for all chips, and not all chips
 	 * support clearing the caseopen status through "regular" registers.
 	 */
-	ret = superio_enter(data->sioreg);
+	ret = superio_enter(sio_data);
 	if (ret) {
 		count = ret;
 		goto error;
 	}
 
-	superio_select(data->sioreg, NCT6775_LD_ACPI);
-	reg = superio_inb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
+	superio_select(sio_data, NCT6775_LD_ACPI);
+	reg = superio_inb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
 	reg |= NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
 	reg &= ~NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(data->sioreg, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
-	superio_exit(data->sioreg);
+	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	superio_exit(sio_data);
 
 	data->valid = false;	/* Force cache refresh */
 error:
@@ -3542,29 +3553,29 @@ static inline void nct6775_init_device(struct nct6775_data *data)
 }
 
 static void
-nct6775_check_fan_inputs(struct nct6775_data *data)
+nct6775_check_fan_inputs(struct nct6775_data *data,
+		struct nct6775_sio_data *sio_data)
 {
 	bool fan3pin = false, fan4pin = false, fan4min = false;
 	bool fan5pin = false, fan6pin = false, fan7pin = false;
 	bool pwm3pin = false, pwm4pin = false, pwm5pin = false;
 	bool pwm6pin = false, pwm7pin = false;
-	int sioreg = data->sioreg;
 
 	/* Store SIO_REG_ENABLE for use during resume */
-	superio_select(sioreg, NCT6775_LD_HWM);
-	data->sio_reg_enable = superio_inb(sioreg, SIO_REG_ENABLE);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	data->sio_reg_enable = superio_inb(sio_data, SIO_REG_ENABLE);
 
 	/* fan4 and fan5 share some pins with the GPIO and serial flash */
 	if (data->kind == nct6775) {
-		int cr2c = superio_inb(sioreg, 0x2c);
+		int cr2c = superio_inb(sio_data, 0x2c);
 
 		fan3pin = cr2c & BIT(6);
 		pwm3pin = cr2c & BIT(7);
 
 		/* On NCT6775, fan4 shares pins with the fdc interface */
-		fan4pin = !(superio_inb(sioreg, 0x2A) & 0x80);
+		fan4pin = !(superio_inb(sio_data, 0x2A) & 0x80);
 	} else if (data->kind == nct6776) {
-		bool gpok = superio_inb(sioreg, 0x27) & 0x80;
+		bool gpok = superio_inb(sio_data, 0x27) & 0x80;
 		const char *board_vendor, *board_name;
 
 		board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
@@ -3580,7 +3591,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 			if (!strcmp(board_name, "Z77 Pro4-M")) {
 				if ((data->sio_reg_enable & 0xe0) != 0xe0) {
 					data->sio_reg_enable |= 0xe0;
-					superio_outb(sioreg, SIO_REG_ENABLE,
+					superio_outb(sio_data, SIO_REG_ENABLE,
 						     data->sio_reg_enable);
 				}
 			}
@@ -3589,32 +3600,32 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 		if (data->sio_reg_enable & 0x80)
 			fan3pin = gpok;
 		else
-			fan3pin = !(superio_inb(sioreg, 0x24) & 0x40);
+			fan3pin = !(superio_inb(sio_data, 0x24) & 0x40);
 
 		if (data->sio_reg_enable & 0x40)
 			fan4pin = gpok;
 		else
-			fan4pin = superio_inb(sioreg, 0x1C) & 0x01;
+			fan4pin = superio_inb(sio_data, 0x1C) & 0x01;
 
 		if (data->sio_reg_enable & 0x20)
 			fan5pin = gpok;
 		else
-			fan5pin = superio_inb(sioreg, 0x1C) & 0x02;
+			fan5pin = superio_inb(sio_data, 0x1C) & 0x02;
 
 		fan4min = fan4pin;
 		pwm3pin = fan3pin;
 	} else if (data->kind == nct6106) {
-		int cr24 = superio_inb(sioreg, 0x24);
+		int cr24 = superio_inb(sio_data, 0x24);
 
 		fan3pin = !(cr24 & 0x80);
 		pwm3pin = cr24 & 0x08;
 	} else if (data->kind == nct6116) {
-		int cr1a = superio_inb(sioreg, 0x1a);
-		int cr1b = superio_inb(sioreg, 0x1b);
-		int cr24 = superio_inb(sioreg, 0x24);
-		int cr2a = superio_inb(sioreg, 0x2a);
-		int cr2b = superio_inb(sioreg, 0x2b);
-		int cr2f = superio_inb(sioreg, 0x2f);
+		int cr1a = superio_inb(sio_data, 0x1a);
+		int cr1b = superio_inb(sio_data, 0x1b);
+		int cr24 = superio_inb(sio_data, 0x24);
+		int cr2a = superio_inb(sio_data, 0x2a);
+		int cr2b = superio_inb(sio_data, 0x2b);
+		int cr2f = superio_inb(sio_data, 0x2f);
 
 		fan3pin = !(cr2b & 0x10);
 		fan4pin = (cr2b & 0x80) ||			// pin 1(2)
@@ -3630,24 +3641,24 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
 		 * NCT6779D, NCT6791D, NCT6792D, NCT6793D, NCT6795D, NCT6796D,
 		 * NCT6797D, NCT6798D
 		 */
-		int cr1a = superio_inb(sioreg, 0x1a);
-		int cr1b = superio_inb(sioreg, 0x1b);
-		int cr1c = superio_inb(sioreg, 0x1c);
-		int cr1d = superio_inb(sioreg, 0x1d);
-		int cr2a = superio_inb(sioreg, 0x2a);
-		int cr2b = superio_inb(sioreg, 0x2b);
-		int cr2d = superio_inb(sioreg, 0x2d);
-		int cr2f = superio_inb(sioreg, 0x2f);
+		int cr1a = superio_inb(sio_data, 0x1a);
+		int cr1b = superio_inb(sio_data, 0x1b);
+		int cr1c = superio_inb(sio_data, 0x1c);
+		int cr1d = superio_inb(sio_data, 0x1d);
+		int cr2a = superio_inb(sio_data, 0x2a);
+		int cr2b = superio_inb(sio_data, 0x2b);
+		int cr2d = superio_inb(sio_data, 0x2d);
+		int cr2f = superio_inb(sio_data, 0x2f);
 		bool dsw_en = cr2f & BIT(3);
 		bool ddr4_en = cr2f & BIT(4);
 		int cre0;
 		int creb;
 		int cred;
 
-		superio_select(sioreg, NCT6775_LD_12);
-		cre0 = superio_inb(sioreg, 0xe0);
-		creb = superio_inb(sioreg, 0xeb);
-		cred = superio_inb(sioreg, 0xed);
+		superio_select(sio_data, NCT6775_LD_12);
+		cre0 = superio_inb(sio_data, 0xe0);
+		creb = superio_inb(sio_data, 0xeb);
+		cred = superio_inb(sio_data, 0xed);
 
 		fan3pin = !(cr1c & BIT(5));
 		fan4pin = !(cr1c & BIT(6));
@@ -3793,7 +3804,7 @@ static int nct6775_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
 	struct nct6775_data *data;
-	struct resource *res;
+	struct resource *res = NULL;
 	int i, s, err = 0;
 	int src, mask, available;
 	const u16 *reg_temp, *reg_temp_over, *reg_temp_hyst, *reg_temp_config;
@@ -3806,6 +3817,9 @@ static int nct6775_probe(struct platform_device *pdev)
 	int num_attr_groups = 0;
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res)
+		return -EBUSY;
+
 	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
 				 DRVNAME))
 		return -EBUSY;
@@ -4502,11 +4516,11 @@ static int nct6775_probe(struct platform_device *pdev)
 	/* Initialize the chip */
 	nct6775_init_device(data);
 
-	err = superio_enter(sio_data->sioreg);
+	err = superio_enter(sio_data);
 	if (err)
 		return err;
 
-	cr2a = superio_inb(sio_data->sioreg, 0x2a);
+	cr2a = superio_inb(sio_data, 0x2a);
 	switch (data->kind) {
 	case nct6775:
 		data->have_vid = (cr2a & 0x40);
@@ -4532,16 +4546,16 @@ static int nct6775_probe(struct platform_device *pdev)
 	 * We can get the VID input values directly at logical device D 0xe3.
 	 */
 	if (data->have_vid) {
-		superio_select(sio_data->sioreg, NCT6775_LD_VID);
-		data->vid = superio_inb(sio_data->sioreg, 0xe3);
+		superio_select(sio_data, NCT6775_LD_VID);
+		data->vid = superio_inb(sio_data, 0xe3);
 		data->vrm = vid_which_vrm();
 	}
 
 	if (fan_debounce) {
 		u8 tmp;
 
-		superio_select(sio_data->sioreg, NCT6775_LD_HWM);
-		tmp = superio_inb(sio_data->sioreg,
+		superio_select(sio_data, NCT6775_LD_HWM);
+		tmp = superio_inb(sio_data,
 				  NCT6775_REG_CR_FAN_DEBOUNCE);
 		switch (data->kind) {
 		case nct6106:
@@ -4565,15 +4579,15 @@ static int nct6775_probe(struct platform_device *pdev)
 			tmp |= 0x7e;
 			break;
 		}
-		superio_outb(sio_data->sioreg, NCT6775_REG_CR_FAN_DEBOUNCE,
+		superio_outb(sio_data, NCT6775_REG_CR_FAN_DEBOUNCE,
 			     tmp);
 		dev_info(&pdev->dev, "Enabled fan debounce for chip %s\n",
 			 data->name);
 	}
 
-	nct6775_check_fan_inputs(data);
+	nct6775_check_fan_inputs(data, sio_data);
 
-	superio_exit(sio_data->sioreg);
+	superio_exit(sio_data);
 
 	/* Read fan clock dividers immediately */
 	nct6775_init_fan_common(dev, data);
@@ -4613,14 +4627,14 @@ static int nct6775_probe(struct platform_device *pdev)
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-static void nct6791_enable_io_mapping(int sioaddr)
+static void nct6791_enable_io_mapping(struct nct6775_sio_data *sio_data)
 {
 	int val;
 
-	val = superio_inb(sioaddr, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
+	val = superio_inb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
 	if (val & 0x10) {
 		pr_info("Enabling hardware monitor logical device mappings.\n");
-		superio_outb(sioaddr, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
+		superio_outb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
 			     val & ~0x10);
 	}
 }
@@ -4643,29 +4657,29 @@ static int __maybe_unused nct6775_suspend(struct device *dev)
 static int __maybe_unused nct6775_resume(struct device *dev)
 {
 	struct nct6775_data *data = dev_get_drvdata(dev);
-	int sioreg = data->sioreg;
+	struct nct6775_sio_data *sio_data = dev_get_platdata(dev);
 	int i, j, err = 0;
 	u8 reg;
 
 	mutex_lock(&data->update_lock);
 	data->bank = 0xff;		/* Force initial bank selection */
 
-	err = superio_enter(sioreg);
+	err = superio_enter(sio_data);
 	if (err)
 		goto abort;
 
-	superio_select(sioreg, NCT6775_LD_HWM);
-	reg = superio_inb(sioreg, SIO_REG_ENABLE);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	reg = superio_inb(sio_data, SIO_REG_ENABLE);
 	if (reg != data->sio_reg_enable)
-		superio_outb(sioreg, SIO_REG_ENABLE, data->sio_reg_enable);
+		superio_outb(sio_data, SIO_REG_ENABLE, data->sio_reg_enable);
 
 	if (data->kind == nct6791 || data->kind == nct6792 ||
 	    data->kind == nct6793 || data->kind == nct6795 ||
 	    data->kind == nct6796 || data->kind == nct6797 ||
 	    data->kind == nct6798)
-		nct6791_enable_io_mapping(sioreg);
+		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sioreg);
+	superio_exit(sio_data);
 
 	/* Restore limits */
 	for (i = 0; i < data->in_num; i++) {
@@ -4728,12 +4742,14 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	int err;
 	int addr;
 
-	err = superio_enter(sioaddr);
+	sio_data->sioreg = sioaddr;
+
+	err = superio_enter(sio_data);
 	if (err)
 		return err;
 
-	val = (superio_inb(sioaddr, SIO_REG_DEVID) << 8) |
-		superio_inb(sioaddr, SIO_REG_DEVID + 1);
+	val = (superio_inb(sio_data, SIO_REG_DEVID) << 8) |
+		superio_inb(sio_data, SIO_REG_DEVID + 1);
 	if (force_id && val != 0xffff)
 		val = force_id;
 
@@ -4777,38 +4793,37 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	default:
 		if (val != 0xffff)
 			pr_debug("unsupported chip ID: 0x%04x\n", val);
-		superio_exit(sioaddr);
+		superio_exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* We have a known chip, find the HWM I/O address */
-	superio_select(sioaddr, NCT6775_LD_HWM);
-	val = (superio_inb(sioaddr, SIO_REG_ADDR) << 8)
-	    | superio_inb(sioaddr, SIO_REG_ADDR + 1);
+	superio_select(sio_data, NCT6775_LD_HWM);
+	val = (superio_inb(sio_data, SIO_REG_ADDR) << 8)
+	    | superio_inb(sio_data, SIO_REG_ADDR + 1);
 	addr = val & IOREGION_ALIGNMENT;
 	if (addr == 0) {
 		pr_err("Refusing to enable a Super-I/O device with a base I/O port 0\n");
-		superio_exit(sioaddr);
+		superio_exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* Activate logical device if needed */
-	val = superio_inb(sioaddr, SIO_REG_ENABLE);
+	val = superio_inb(sio_data, SIO_REG_ENABLE);
 	if (!(val & 0x01)) {
 		pr_warn("Forcibly enabling Super-I/O. Sensor is probably unusable.\n");
-		superio_outb(sioaddr, SIO_REG_ENABLE, val | 0x01);
+		superio_outb(sio_data, SIO_REG_ENABLE, val | 0x01);
 	}
 
 	if (sio_data->kind == nct6791 || sio_data->kind == nct6792 ||
 	    sio_data->kind == nct6793 || sio_data->kind == nct6795 ||
 	    sio_data->kind == nct6796 || sio_data->kind == nct6797 ||
 	    sio_data->kind == nct6798)
-		nct6791_enable_io_mapping(sioaddr);
+		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sioaddr);
+	superio_exit(sio_data);
 	pr_info("Found %s or compatible chip at %#x:%#x\n",
 		nct6775_sio_names[sio_data->kind], sioaddr, addr);
-	sio_data->sioreg = sioaddr;
 
 	return addr;
 }
-- 
2.33.0

[PATCH 2/3] hwmon: (nct6775) Use superio function pointers (v2)

[Denis Pauk]

Use superio function pointers in nct6775_sio_data instead direct calls.

v2: split changes to separate patches

Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
---
 drivers/hwmon/nct6775.c | 141 ++++++++++++++++++++++------------------
 1 file changed, 76 insertions(+), 65 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 3390de4c00f4..908521ce2ee8 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -136,9 +136,14 @@ enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
 struct nct6775_sio_data {
 	int sioreg;
 	enum kinds kind;
+	void (*outb)(struct nct6775_sio_data *sio_data, int reg, int val);
+	int (*inb)(struct nct6775_sio_data *sio_data, int reg);
+	void (*select)(struct nct6775_sio_data *sio_data, int ld);
+	int (*enter)(struct nct6775_sio_data *sio_data);
+	void (*exit)(struct nct6775_sio_data *sio_data);
 };
 
-static inline void
+static void
 superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
 {
 	int ioreg = sio_data->sioreg;
@@ -147,7 +152,7 @@ superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
 	outb(val, ioreg + 1);
 }
 
-static inline int
+static int
 superio_inb(struct nct6775_sio_data *sio_data, int reg)
 {
 	int ioreg = sio_data->sioreg;
@@ -156,7 +161,7 @@ superio_inb(struct nct6775_sio_data *sio_data, int reg)
 	return inb(ioreg + 1);
 }
 
-static inline void
+static void
 superio_select(struct nct6775_sio_data *sio_data, int ld)
 {
 	int ioreg = sio_data->sioreg;
@@ -165,7 +170,7 @@ superio_select(struct nct6775_sio_data *sio_data, int ld)
 	outb(ld, ioreg + 1);
 }
 
-static inline int
+static int
 superio_enter(struct nct6775_sio_data *sio_data)
 {
 	int ioreg = sio_data->sioreg;
@@ -182,7 +187,7 @@ superio_enter(struct nct6775_sio_data *sio_data)
 	return 0;
 }
 
-static inline void
+static void
 superio_exit(struct nct6775_sio_data *sio_data)
 {
 	int ioreg = sio_data->sioreg;
@@ -3436,19 +3441,19 @@ clear_caseopen(struct device *dev, struct device_attribute *attr,
 	 * The CR registers are the same for all chips, and not all chips
 	 * support clearing the caseopen status through "regular" registers.
 	 */
-	ret = superio_enter(sio_data);
+	ret = sio_data->enter(sio_data);
 	if (ret) {
 		count = ret;
 		goto error;
 	}
 
-	superio_select(sio_data, NCT6775_LD_ACPI);
-	reg = superio_inb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
+	sio_data->select(sio_data, NCT6775_LD_ACPI);
+	reg = sio_data->inb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr]);
 	reg |= NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	sio_data->outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
 	reg &= ~NCT6775_CR_CASEOPEN_CLR_MASK[nr];
-	superio_outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
-	superio_exit(sio_data);
+	sio_data->outb(sio_data, NCT6775_REG_CR_CASEOPEN_CLR[nr], reg);
+	sio_data->exit(sio_data);
 
 	data->valid = false;	/* Force cache refresh */
 error:
@@ -3562,20 +3567,20 @@ nct6775_check_fan_inputs(struct nct6775_data *data,
 	bool pwm6pin = false, pwm7pin = false;
 
 	/* Store SIO_REG_ENABLE for use during resume */
-	superio_select(sio_data, NCT6775_LD_HWM);
-	data->sio_reg_enable = superio_inb(sio_data, SIO_REG_ENABLE);
+	sio_data->select(sio_data, NCT6775_LD_HWM);
+	data->sio_reg_enable = sio_data->inb(sio_data, SIO_REG_ENABLE);
 
 	/* fan4 and fan5 share some pins with the GPIO and serial flash */
 	if (data->kind == nct6775) {
-		int cr2c = superio_inb(sio_data, 0x2c);
+		int cr2c = sio_data->inb(sio_data, 0x2c);
 
 		fan3pin = cr2c & BIT(6);
 		pwm3pin = cr2c & BIT(7);
 
 		/* On NCT6775, fan4 shares pins with the fdc interface */
-		fan4pin = !(superio_inb(sio_data, 0x2A) & 0x80);
+		fan4pin = !(sio_data->inb(sio_data, 0x2A) & 0x80);
 	} else if (data->kind == nct6776) {
-		bool gpok = superio_inb(sio_data, 0x27) & 0x80;
+		bool gpok = sio_data->inb(sio_data, 0x27) & 0x80;
 		const char *board_vendor, *board_name;
 
 		board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
@@ -3591,7 +3596,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data,
 			if (!strcmp(board_name, "Z77 Pro4-M")) {
 				if ((data->sio_reg_enable & 0xe0) != 0xe0) {
 					data->sio_reg_enable |= 0xe0;
-					superio_outb(sio_data, SIO_REG_ENABLE,
+					sio_data->outb(sio_data, SIO_REG_ENABLE,
 						     data->sio_reg_enable);
 				}
 			}
@@ -3600,32 +3605,32 @@ nct6775_check_fan_inputs(struct nct6775_data *data,
 		if (data->sio_reg_enable & 0x80)
 			fan3pin = gpok;
 		else
-			fan3pin = !(superio_inb(sio_data, 0x24) & 0x40);
+			fan3pin = !(sio_data->inb(sio_data, 0x24) & 0x40);
 
 		if (data->sio_reg_enable & 0x40)
 			fan4pin = gpok;
 		else
-			fan4pin = superio_inb(sio_data, 0x1C) & 0x01;
+			fan4pin = sio_data->inb(sio_data, 0x1C) & 0x01;
 
 		if (data->sio_reg_enable & 0x20)
 			fan5pin = gpok;
 		else
-			fan5pin = superio_inb(sio_data, 0x1C) & 0x02;
+			fan5pin = sio_data->inb(sio_data, 0x1C) & 0x02;
 
 		fan4min = fan4pin;
 		pwm3pin = fan3pin;
 	} else if (data->kind == nct6106) {
-		int cr24 = superio_inb(sio_data, 0x24);
+		int cr24 = sio_data->inb(sio_data, 0x24);
 
 		fan3pin = !(cr24 & 0x80);
 		pwm3pin = cr24 & 0x08;
 	} else if (data->kind == nct6116) {
-		int cr1a = superio_inb(sio_data, 0x1a);
-		int cr1b = superio_inb(sio_data, 0x1b);
-		int cr24 = superio_inb(sio_data, 0x24);
-		int cr2a = superio_inb(sio_data, 0x2a);
-		int cr2b = superio_inb(sio_data, 0x2b);
-		int cr2f = superio_inb(sio_data, 0x2f);
+		int cr1a = sio_data->inb(sio_data, 0x1a);
+		int cr1b = sio_data->inb(sio_data, 0x1b);
+		int cr24 = sio_data->inb(sio_data, 0x24);
+		int cr2a = sio_data->inb(sio_data, 0x2a);
+		int cr2b = sio_data->inb(sio_data, 0x2b);
+		int cr2f = sio_data->inb(sio_data, 0x2f);
 
 		fan3pin = !(cr2b & 0x10);
 		fan4pin = (cr2b & 0x80) ||			// pin 1(2)
@@ -3641,24 +3646,24 @@ nct6775_check_fan_inputs(struct nct6775_data *data,
 		 * NCT6779D, NCT6791D, NCT6792D, NCT6793D, NCT6795D, NCT6796D,
 		 * NCT6797D, NCT6798D
 		 */
-		int cr1a = superio_inb(sio_data, 0x1a);
-		int cr1b = superio_inb(sio_data, 0x1b);
-		int cr1c = superio_inb(sio_data, 0x1c);
-		int cr1d = superio_inb(sio_data, 0x1d);
-		int cr2a = superio_inb(sio_data, 0x2a);
-		int cr2b = superio_inb(sio_data, 0x2b);
-		int cr2d = superio_inb(sio_data, 0x2d);
-		int cr2f = superio_inb(sio_data, 0x2f);
+		int cr1a = sio_data->inb(sio_data, 0x1a);
+		int cr1b = sio_data->inb(sio_data, 0x1b);
+		int cr1c = sio_data->inb(sio_data, 0x1c);
+		int cr1d = sio_data->inb(sio_data, 0x1d);
+		int cr2a = sio_data->inb(sio_data, 0x2a);
+		int cr2b = sio_data->inb(sio_data, 0x2b);
+		int cr2d = sio_data->inb(sio_data, 0x2d);
+		int cr2f = sio_data->inb(sio_data, 0x2f);
 		bool dsw_en = cr2f & BIT(3);
 		bool ddr4_en = cr2f & BIT(4);
 		int cre0;
 		int creb;
 		int cred;
 
-		superio_select(sio_data, NCT6775_LD_12);
-		cre0 = superio_inb(sio_data, 0xe0);
-		creb = superio_inb(sio_data, 0xeb);
-		cred = superio_inb(sio_data, 0xed);
+		sio_data->select(sio_data, NCT6775_LD_12);
+		cre0 = sio_data->inb(sio_data, 0xe0);
+		creb = sio_data->inb(sio_data, 0xeb);
+		cred = sio_data->inb(sio_data, 0xed);
 
 		fan3pin = !(cr1c & BIT(5));
 		fan4pin = !(cr1c & BIT(6));
@@ -4516,11 +4521,11 @@ static int nct6775_probe(struct platform_device *pdev)
 	/* Initialize the chip */
 	nct6775_init_device(data);
 
-	err = superio_enter(sio_data);
+	err = sio_data->enter(sio_data);
 	if (err)
 		return err;
 
-	cr2a = superio_inb(sio_data, 0x2a);
+	cr2a = sio_data->inb(sio_data, 0x2a);
 	switch (data->kind) {
 	case nct6775:
 		data->have_vid = (cr2a & 0x40);
@@ -4546,16 +4551,16 @@ static int nct6775_probe(struct platform_device *pdev)
 	 * We can get the VID input values directly at logical device D 0xe3.
 	 */
 	if (data->have_vid) {
-		superio_select(sio_data, NCT6775_LD_VID);
-		data->vid = superio_inb(sio_data, 0xe3);
+		sio_data->select(sio_data, NCT6775_LD_VID);
+		data->vid = sio_data->inb(sio_data, 0xe3);
 		data->vrm = vid_which_vrm();
 	}
 
 	if (fan_debounce) {
 		u8 tmp;
 
-		superio_select(sio_data, NCT6775_LD_HWM);
-		tmp = superio_inb(sio_data,
+		sio_data->select(sio_data, NCT6775_LD_HWM);
+		tmp = sio_data->inb(sio_data,
 				  NCT6775_REG_CR_FAN_DEBOUNCE);
 		switch (data->kind) {
 		case nct6106:
@@ -4579,7 +4584,7 @@ static int nct6775_probe(struct platform_device *pdev)
 			tmp |= 0x7e;
 			break;
 		}
-		superio_outb(sio_data, NCT6775_REG_CR_FAN_DEBOUNCE,
+		sio_data->outb(sio_data, NCT6775_REG_CR_FAN_DEBOUNCE,
 			     tmp);
 		dev_info(&pdev->dev, "Enabled fan debounce for chip %s\n",
 			 data->name);
@@ -4587,7 +4592,7 @@ static int nct6775_probe(struct platform_device *pdev)
 
 	nct6775_check_fan_inputs(data, sio_data);
 
-	superio_exit(sio_data);
+	sio_data->exit(sio_data);
 
 	/* Read fan clock dividers immediately */
 	nct6775_init_fan_common(dev, data);
@@ -4631,10 +4636,10 @@ static void nct6791_enable_io_mapping(struct nct6775_sio_data *sio_data)
 {
 	int val;
 
-	val = superio_inb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
+	val = sio_data->inb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE);
 	if (val & 0x10) {
 		pr_info("Enabling hardware monitor logical device mappings.\n");
-		superio_outb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
+		sio_data->outb(sio_data, NCT6791_REG_HM_IO_SPACE_LOCK_ENABLE,
 			     val & ~0x10);
 	}
 }
@@ -4664,14 +4669,14 @@ static int __maybe_unused nct6775_resume(struct device *dev)
 	mutex_lock(&data->update_lock);
 	data->bank = 0xff;		/* Force initial bank selection */
 
-	err = superio_enter(sio_data);
+	err = sio_data->enter(sio_data);
 	if (err)
 		goto abort;
 
-	superio_select(sio_data, NCT6775_LD_HWM);
-	reg = superio_inb(sio_data, SIO_REG_ENABLE);
+	sio_data->select(sio_data, NCT6775_LD_HWM);
+	reg = sio_data->inb(sio_data, SIO_REG_ENABLE);
 	if (reg != data->sio_reg_enable)
-		superio_outb(sio_data, SIO_REG_ENABLE, data->sio_reg_enable);
+		sio_data->outb(sio_data, SIO_REG_ENABLE, data->sio_reg_enable);
 
 	if (data->kind == nct6791 || data->kind == nct6792 ||
 	    data->kind == nct6793 || data->kind == nct6795 ||
@@ -4679,7 +4684,7 @@ static int __maybe_unused nct6775_resume(struct device *dev)
 	    data->kind == nct6798)
 		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sio_data);
+	sio_data->exit(sio_data);
 
 	/* Restore limits */
 	for (i = 0; i < data->in_num; i++) {
@@ -4744,12 +4749,12 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 
 	sio_data->sioreg = sioaddr;
 
-	err = superio_enter(sio_data);
+	err = sio_data->enter(sio_data);
 	if (err)
 		return err;
 
-	val = (superio_inb(sio_data, SIO_REG_DEVID) << 8) |
-		superio_inb(sio_data, SIO_REG_DEVID + 1);
+	val = (sio_data->inb(sio_data, SIO_REG_DEVID) << 8) |
+		sio_data->inb(sio_data, SIO_REG_DEVID + 1);
 	if (force_id && val != 0xffff)
 		val = force_id;
 
@@ -4793,26 +4798,26 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	default:
 		if (val != 0xffff)
 			pr_debug("unsupported chip ID: 0x%04x\n", val);
-		superio_exit(sio_data);
+		sio_data->exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* We have a known chip, find the HWM I/O address */
-	superio_select(sio_data, NCT6775_LD_HWM);
-	val = (superio_inb(sio_data, SIO_REG_ADDR) << 8)
-	    | superio_inb(sio_data, SIO_REG_ADDR + 1);
+	sio_data->select(sio_data, NCT6775_LD_HWM);
+	val = (sio_data->inb(sio_data, SIO_REG_ADDR) << 8)
+	    | sio_data->inb(sio_data, SIO_REG_ADDR + 1);
 	addr = val & IOREGION_ALIGNMENT;
 	if (addr == 0) {
 		pr_err("Refusing to enable a Super-I/O device with a base I/O port 0\n");
-		superio_exit(sio_data);
+		sio_data->exit(sio_data);
 		return -ENODEV;
 	}
 
 	/* Activate logical device if needed */
-	val = superio_inb(sio_data, SIO_REG_ENABLE);
+	val = sio_data->inb(sio_data, SIO_REG_ENABLE);
 	if (!(val & 0x01)) {
 		pr_warn("Forcibly enabling Super-I/O. Sensor is probably unusable.\n");
-		superio_outb(sio_data, SIO_REG_ENABLE, val | 0x01);
+		sio_data->outb(sio_data, SIO_REG_ENABLE, val | 0x01);
 	}
 
 	if (sio_data->kind == nct6791 || sio_data->kind == nct6792 ||
@@ -4821,7 +4826,7 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	    sio_data->kind == nct6798)
 		nct6791_enable_io_mapping(sio_data);
 
-	superio_exit(sio_data);
+	sio_data->exit(sio_data);
 	pr_info("Found %s or compatible chip at %#x:%#x\n",
 		nct6775_sio_names[sio_data->kind], sioaddr, addr);
 
@@ -4857,6 +4862,12 @@ static int __init sensors_nct6775_init(void)
 	 * nct6775 hardware monitor, and call probe()
 	 */
 	for (i = 0; i < ARRAY_SIZE(pdev); i++) {
+		sio_data.outb = superio_outb;
+		sio_data.inb = superio_inb;
+		sio_data.select = superio_select;
+		sio_data.enter = superio_enter;
+		sio_data.exit = superio_exit;
+
 		address = nct6775_find(sioaddr[i], &sio_data);
 		if (address <= 0)
 			continue;
-- 
2.33.0

Re: [PATCH 2/3] hwmon: (nct6775) Use superio function pointers (v2)

[Andy Shevchenko]

On Thu, Sep 09, 2021 at 12:36:03AM +0300, Denis Pauk wrote:
> Use superio function pointers in nct6775_sio_data instead direct calls.
> 
> v2: split changes to separate patches

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>

Similar comments as per patch 1.

...

> +	void (*outb)(struct nct6775_sio_data *sio_data, int reg, int val);
> +	int (*inb)(struct nct6775_sio_data *sio_data, int reg);

This part should be split separately and actually be a part of patch 1.

...

> -static inline void
> +static void
>  superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)

I guess it's one line and it should be a part of patch 1.

...

> -static inline int
> +static int
>  superio_inb(struct nct6775_sio_data *sio_data, int reg)

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] hwmon: (nct6775) Support access via Asus WMI (v2)

[Denis Pauk]

Support accessing the NCT677x via Asus WMI functions.

On mainboards that support this way of accessing the chip,
the driver will usually not work without this option since
in these mainboards, ACPI will mark the I/O port as used.

v2: split changes to separate patches
    limit WMI usage by DMI_BOARD_NAME in checked ASUS motherboards

Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
Tested-by: Pär Ekholm <pehlm@pekholm.org>
Tested-by: <to.eivind@gmail.com>
Tested-by: Artem S. Tashkinov <aros@gmx.com>
Tested-by: Vittorio Roberto Alfieri <me@rebtoor.com>
---
 drivers/hwmon/nct6775.c | 219 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 198 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 908521ce2ee8..ae0344811821 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -55,6 +55,7 @@
 #include <linux/dmi.h>
 #include <linux/io.h>
 #include <linux/nospec.h>
+#include <linux/wmi.h>
 #include "lm75.h"
 
 #define USE_ALTERNATE
@@ -132,10 +133,13 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
 #define SIO_ID_MASK		0xFFF8
 
 enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
+enum sensor_access { access_direct, access_asuswmi };
 
 struct nct6775_sio_data {
 	int sioreg;
+	int ld;
 	enum kinds kind;
+	enum sensor_access access;
 	void (*outb)(struct nct6775_sio_data *sio_data, int reg, int val);
 	int (*inb)(struct nct6775_sio_data *sio_data, int reg);
 	void (*select)(struct nct6775_sio_data *sio_data, int ld);
@@ -143,6 +147,92 @@ struct nct6775_sio_data {
 	void (*exit)(struct nct6775_sio_data *sio_data);
 };
 
+#define ASUSWMI_MGMT2_GUID		"466747A0-70EC-11DE-8A39-0800200C9A66"
+#define ASUSWMI_METHODID_RSIO		0x5253494F
+#define ASUSWMI_METHODID_WSIO		0x5753494F
+#define ASUSWMI_METHODID_RHWM		0x5248574D
+#define ASUSWMI_METHODID_WHWM		0x5748574D
+#define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
+
+static int asuswmi_evaluate_method(u32 method_id,
+		u8 bank, u8 reg, u8 val, u32 *retval)
+{
+	u32 args = bank | (reg << 8) | (val << 16);
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	union acpi_object *obj;
+	u32 tmp = 0;
+
+	status = wmi_evaluate_method(ASUSWMI_MGMT2_GUID, 0, method_id,
+				     &input, &output);
+
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	obj = (union acpi_object *)output.pointer;
+	if (obj && obj->type == ACPI_TYPE_INTEGER)
+		tmp = (u32) obj->integer.value;
+
+	if (retval)
+		*retval = tmp;
+
+	kfree(obj);
+
+	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
+		return -ENODEV;
+	return 0;
+}
+
+static inline int
+nct6775_asuswmi_write(u8 bank, u8 reg, u8 val)
+{
+	return asuswmi_evaluate_method(ASUSWMI_METHODID_WHWM, bank, reg, val, 0);
+}
+
+static inline int
+nct6775_asuswmi_read(u8 bank, u8 reg, u8 *val)
+{
+	u32 tmp;
+	int ret = asuswmi_evaluate_method(ASUSWMI_METHODID_RHWM, bank, reg, 0, &tmp);
+	*val = tmp & 0xff;
+	return ret;
+}
+
+static int
+superio_wmi_inb(struct nct6775_sio_data *sio_data, int reg)
+{
+	int tmp;
+
+	asuswmi_evaluate_method(ASUSWMI_METHODID_RSIO,
+			sio_data->ld, reg, 0, &tmp);
+	return tmp;
+}
+
+static void
+superio_wmi_outb(struct nct6775_sio_data *sio_data, int reg, int val)
+{
+	asuswmi_evaluate_method(ASUSWMI_METHODID_WSIO,
+			sio_data->ld, reg, val, 0);
+}
+
+static void
+superio_wmi_select(struct nct6775_sio_data *sio_data, int ld)
+{
+	sio_data->ld = ld;
+}
+
+static int
+superio_wmi_enter(struct nct6775_sio_data *sio_data)
+{
+	return 0;
+}
+
+static void
+superio_wmi_exit(struct nct6775_sio_data *sio_data)
+{
+}
+
 static void
 superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
 {
@@ -1090,6 +1180,7 @@ struct nct6775_data {
 	int addr;	/* IO base of hw monitor block */
 	int sioreg;	/* SIO register address */
 	enum kinds kind;
+	enum sensor_access access;
 	const char *name;
 
 	const struct attribute_group *groups[6];
@@ -1432,6 +1523,11 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
 {
 	u8 bank = reg >> 8;
 
+	if (data->access == access_asuswmi) {
+		data->bank = bank;
+		return;
+	}
+
 	if (data->bank != bank) {
 		outb_p(NCT6775_REG_BANK, data->addr + ADDR_REG_OFFSET);
 		outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -1442,8 +1538,21 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
 static u16 nct6775_read_value(struct nct6775_data *data, u16 reg)
 {
 	int res, word_sized = is_word_sized(data, reg);
+	u8 tmp;
 
 	nct6775_set_bank(data, reg);
+
+	if (data->access == access_asuswmi) {
+		nct6775_asuswmi_read(data->bank, reg, &tmp);
+		res = (tmp & 0xff);
+		if (word_sized) {
+			nct6775_asuswmi_read(data->bank,
+					(reg & 0xff) + 1, &tmp);
+			res = (res << 8) + (tmp & 0xff);
+		}
+		return res;
+	}
+
 	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
 	res = inb_p(data->addr + DATA_REG_OFFSET);
 	if (word_sized) {
@@ -1459,6 +1568,21 @@ static int nct6775_write_value(struct nct6775_data *data, u16 reg, u16 value)
 	int word_sized = is_word_sized(data, reg);
 
 	nct6775_set_bank(data, reg);
+
+	if (data->access == access_asuswmi) {
+		if (word_sized) {
+			nct6775_asuswmi_write(data->bank, (reg & 0xff),
+					(value >> 8) & 0xff);
+			nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
+					value & 0xff);
+		} else {
+			nct6775_asuswmi_write(data->bank, (reg & 0xff),
+					value);
+		}
+
+		return 0;
+	}
+
 	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
 	if (word_sized) {
 		outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
@@ -3821,13 +3945,15 @@ static int nct6775_probe(struct platform_device *pdev)
 	struct device *hwmon_dev;
 	int num_attr_groups = 0;
 
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!res)
-		return -EBUSY;
+	if (sio_data->access == access_direct) {
+		res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+		if (!res)
+			return -EBUSY;
 
-	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
-				 DRVNAME))
-		return -EBUSY;
+		if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
+					 DRVNAME))
+			return -EBUSY;
+	}
 
 	data = devm_kzalloc(&pdev->dev, sizeof(struct nct6775_data),
 			    GFP_KERNEL);
@@ -3836,7 +3962,11 @@ static int nct6775_probe(struct platform_device *pdev)
 
 	data->kind = sio_data->kind;
 	data->sioreg = sio_data->sioreg;
-	data->addr = res->start;
+	data->access = sio_data->access;
+	if (sio_data->access == access_direct)
+		data->addr = res->start;
+	else
+		data->addr = 0;
 	mutex_init(&data->update_lock);
 	data->name = nct6775_device_names[data->kind];
 	data->bank = 0xff;		/* Force initial bank selection */
@@ -4747,6 +4877,7 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 	int err;
 	int addr;
 
+	sio_data->access = access_direct;
 	sio_data->sioreg = sioaddr;
 
 	err = sio_data->enter(sio_data);
@@ -4841,6 +4972,21 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
  */
 static struct platform_device *pdev[2];
 
+#define NCT6775_REG_CHIPID 0x58
+
+static const char * const asus_wmi_boards[] = {
+	"PRIME B460-PLUS",
+	"ROG CROSSHAIR VIII IMPACT",
+	"ROG STRIX B550-E GAMING",
+	"ROG STRIX B550-F GAMING (WI-FI)",
+	"ROG STRIX Z490-I GAMING",
+	"TUF GAMING B550M-PLUS",
+	"TUF GAMING B550M-PLUS (WI-FI)",
+	"TUF GAMING B550-PLUS",
+	"TUF GAMING X570-PLUS",
+	"TUF GAMING X570-PRO (WI-FI)"
+};
+
 static int __init sensors_nct6775_init(void)
 {
 	int i, err;
@@ -4849,11 +4995,29 @@ static int __init sensors_nct6775_init(void)
 	struct resource res;
 	struct nct6775_sio_data sio_data;
 	int sioaddr[2] = { 0x2e, 0x4e };
+	const char *board_vendor, *board_name;
+	enum sensor_access access = access_direct;
+	u8 tmp;
 
 	err = platform_driver_register(&nct6775_driver);
 	if (err)
 		return err;
 
+	board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
+	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+
+	if (board_name && board_vendor &&
+	    !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
+		if (match_string(asus_wmi_boards,
+				ARRAY_SIZE(asus_wmi_boards), board_name) != -EINVAL) {
+			/* if reading chip id via WMI succeeds, use WMI */
+			if (!nct6775_asuswmi_read(0, NCT6775_REG_CHIPID, &tmp)) {
+				pr_info("Using Asus WMI to access chip\n");
+				access = access_asuswmi;
+			}
+		}
+	}
+
 	/*
 	 * initialize sio_data->kind and sio_data->sioreg.
 	 *
@@ -4874,6 +5038,17 @@ static int __init sensors_nct6775_init(void)
 
 		found = true;
 
+		/* Update access method */
+		sio_data.access = access;
+
+		if (access == access_asuswmi) {
+			sio_data.outb = superio_wmi_outb;
+			sio_data.inb = superio_wmi_inb;
+			sio_data.select = superio_wmi_select;
+			sio_data.enter = superio_wmi_enter;
+			sio_data.exit = superio_wmi_exit;
+		}
+
 		pdev[i] = platform_device_alloc(DRVNAME, address);
 		if (!pdev[i]) {
 			err = -ENOMEM;
@@ -4885,23 +5060,25 @@ static int __init sensors_nct6775_init(void)
 		if (err)
 			goto exit_device_put;
 
-		memset(&res, 0, sizeof(res));
-		res.name = DRVNAME;
-		res.start = address + IOREGION_OFFSET;
-		res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
-		res.flags = IORESOURCE_IO;
+		if (sio_data.access == access_direct) {
+			memset(&res, 0, sizeof(res));
+			res.name = DRVNAME;
+			res.start = address + IOREGION_OFFSET;
+			res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
+			res.flags = IORESOURCE_IO;
+
+			err = acpi_check_resource_conflict(&res);
+			if (err) {
+				platform_device_put(pdev[i]);
+				pdev[i] = NULL;
+				continue;
+			}
 
-		err = acpi_check_resource_conflict(&res);
-		if (err) {
-			platform_device_put(pdev[i]);
-			pdev[i] = NULL;
-			continue;
+			err = platform_device_add_resources(pdev[i], &res, 1);
+			if (err)
+				goto exit_device_put;
 		}
 
-		err = platform_device_add_resources(pdev[i], &res, 1);
-		if (err)
-			goto exit_device_put;
-
 		/* platform_device_add calls probe() */
 		err = platform_device_add(pdev[i]);
 		if (err)
-- 
2.33.0

Re: [PATCH 3/3] hwmon: (nct6775) Support access via Asus WMI (v2)

[Guenter Roeck]

On Thu, Sep 09, 2021 at 12:36:04AM +0300, Denis Pauk wrote:
> Support accessing the NCT677x via Asus WMI functions.
> 
> On mainboards that support this way of accessing the chip,
> the driver will usually not work without this option since
> in these mainboards, ACPI will mark the I/O port as used.
> 
> v2: split changes to separate patches
>     limit WMI usage by DMI_BOARD_NAME in checked ASUS motherboards
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807
> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
> Tested-by: Pär Ekholm <pehlm@pekholm.org>
> Tested-by: <to.eivind@gmail.com>
> Tested-by: Artem S. Tashkinov <aros@gmx.com>
> Tested-by: Vittorio Roberto Alfieri <me@rebtoor.com>
> ---
>  drivers/hwmon/nct6775.c | 219 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 198 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 908521ce2ee8..ae0344811821 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -55,6 +55,7 @@
>  #include <linux/dmi.h>
>  #include <linux/io.h>
>  #include <linux/nospec.h>
> +#include <linux/wmi.h>
>  #include "lm75.h"
>  
>  #define USE_ALTERNATE
> @@ -132,10 +133,13 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
>  #define SIO_ID_MASK		0xFFF8
>  
>  enum pwm_enable { off, manual, thermal_cruise, speed_cruise, sf3, sf4 };
> +enum sensor_access { access_direct, access_asuswmi };
>  
>  struct nct6775_sio_data {
>  	int sioreg;
> +	int ld;
>  	enum kinds kind;
> +	enum sensor_access access;
>  	void (*outb)(struct nct6775_sio_data *sio_data, int reg, int val);
>  	int (*inb)(struct nct6775_sio_data *sio_data, int reg);
>  	void (*select)(struct nct6775_sio_data *sio_data, int ld);
> @@ -143,6 +147,92 @@ struct nct6775_sio_data {
>  	void (*exit)(struct nct6775_sio_data *sio_data);
>  };
>  
> +#define ASUSWMI_MGMT2_GUID		"466747A0-70EC-11DE-8A39-0800200C9A66"
> +#define ASUSWMI_METHODID_RSIO		0x5253494F
> +#define ASUSWMI_METHODID_WSIO		0x5753494F
> +#define ASUSWMI_METHODID_RHWM		0x5248574D
> +#define ASUSWMI_METHODID_WHWM		0x5748574D
> +#define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
> +
> +static int asuswmi_evaluate_method(u32 method_id,
> +		u8 bank, u8 reg, u8 val, u32 *retval)
> +{
> +	u32 args = bank | (reg << 8) | (val << 16);
> +	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	union acpi_object *obj;
> +	u32 tmp = 0;
> +
> +	status = wmi_evaluate_method(ASUSWMI_MGMT2_GUID, 0, method_id,
> +				     &input, &output);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	obj = (union acpi_object *)output.pointer;
> +	if (obj && obj->type == ACPI_TYPE_INTEGER)
> +		tmp = (u32) obj->integer.value;
> +
> +	if (retval)
> +		*retval = tmp;
> +
> +	kfree(obj);
> +
> +	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
> +		return -ENODEV;
> +	return 0;
> +}
> +
> +static inline int
> +nct6775_asuswmi_write(u8 bank, u8 reg, u8 val)
> +{
> +	return asuswmi_evaluate_method(ASUSWMI_METHODID_WHWM, bank, reg, val, 0);
> +}
> +
> +static inline int
> +nct6775_asuswmi_read(u8 bank, u8 reg, u8 *val)
> +{
> +	u32 tmp;
> +	int ret = asuswmi_evaluate_method(ASUSWMI_METHODID_RHWM, bank, reg, 0, &tmp);
> +	*val = tmp & 0xff;
> +	return ret;
> +}
> +
> +static int
> +superio_wmi_inb(struct nct6775_sio_data *sio_data, int reg)
> +{
> +	int tmp;
> +
> +	asuswmi_evaluate_method(ASUSWMI_METHODID_RSIO,
> +			sio_data->ld, reg, 0, &tmp);
> +	return tmp;
> +}
> +
> +static void
> +superio_wmi_outb(struct nct6775_sio_data *sio_data, int reg, int val)
> +{
> +	asuswmi_evaluate_method(ASUSWMI_METHODID_WSIO,
> +			sio_data->ld, reg, val, 0);
> +}
> +
> +static void
> +superio_wmi_select(struct nct6775_sio_data *sio_data, int ld)
> +{
> +	sio_data->ld = ld;
> +}
> +
> +static int
> +superio_wmi_enter(struct nct6775_sio_data *sio_data)
> +{
> +	return 0;
> +}
> +
> +static void
> +superio_wmi_exit(struct nct6775_sio_data *sio_data)
> +{
> +}
> +
>  static void
>  superio_outb(struct nct6775_sio_data *sio_data, int reg, int val)
>  {
> @@ -1090,6 +1180,7 @@ struct nct6775_data {
>  	int addr;	/* IO base of hw monitor block */
>  	int sioreg;	/* SIO register address */
>  	enum kinds kind;
> +	enum sensor_access access;
>  	const char *name;
>  
>  	const struct attribute_group *groups[6];
> @@ -1432,6 +1523,11 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
>  {
>  	u8 bank = reg >> 8;
>  
> +	if (data->access == access_asuswmi) {
> +		data->bank = bank;
> +		return;
> +	}
> +

All that preparation, and then this is still needed. The accessor functions
(nct6775_set_bank, nct6775_read_value, nct6775_write_value) should be
implemented as pointers instead.

Guenter

>  	if (data->bank != bank) {
>  		outb_p(NCT6775_REG_BANK, data->addr + ADDR_REG_OFFSET);
>  		outb_p(bank, data->addr + DATA_REG_OFFSET);
> @@ -1442,8 +1538,21 @@ static inline void nct6775_set_bank(struct nct6775_data *data, u16 reg)
>  static u16 nct6775_read_value(struct nct6775_data *data, u16 reg)
>  {
>  	int res, word_sized = is_word_sized(data, reg);
> +	u8 tmp;
>  
>  	nct6775_set_bank(data, reg);
> +
> +	if (data->access == access_asuswmi) {
> +		nct6775_asuswmi_read(data->bank, reg, &tmp);
> +		res = (tmp & 0xff);
> +		if (word_sized) {
> +			nct6775_asuswmi_read(data->bank,
> +					(reg & 0xff) + 1, &tmp);
> +			res = (res << 8) + (tmp & 0xff);
> +		}
> +		return res;
> +	}
> +
>  	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
>  	res = inb_p(data->addr + DATA_REG_OFFSET);
>  	if (word_sized) {
> @@ -1459,6 +1568,21 @@ static int nct6775_write_value(struct nct6775_data *data, u16 reg, u16 value)
>  	int word_sized = is_word_sized(data, reg);
>  
>  	nct6775_set_bank(data, reg);
> +
> +	if (data->access == access_asuswmi) {
> +		if (word_sized) {
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff),
> +					(value >> 8) & 0xff);
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
> +					value & 0xff);
> +		} else {
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff),
> +					value);
> +		}
> +
> +		return 0;
> +	}
> +
>  	outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
>  	if (word_sized) {
>  		outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
> @@ -3821,13 +3945,15 @@ static int nct6775_probe(struct platform_device *pdev)
>  	struct device *hwmon_dev;
>  	int num_attr_groups = 0;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> -	if (!res)
> -		return -EBUSY;
> +	if (sio_data->access == access_direct) {
> +		res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +		if (!res)
> +			return -EBUSY;
>  
> -	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> -				 DRVNAME))
> -		return -EBUSY;
> +		if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> +					 DRVNAME))
> +			return -EBUSY;
> +	}
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(struct nct6775_data),
>  			    GFP_KERNEL);
> @@ -3836,7 +3962,11 @@ static int nct6775_probe(struct platform_device *pdev)
>  
>  	data->kind = sio_data->kind;
>  	data->sioreg = sio_data->sioreg;
> -	data->addr = res->start;
> +	data->access = sio_data->access;
> +	if (sio_data->access == access_direct)
> +		data->addr = res->start;
> +	else
> +		data->addr = 0;
>  	mutex_init(&data->update_lock);
>  	data->name = nct6775_device_names[data->kind];
>  	data->bank = 0xff;		/* Force initial bank selection */
> @@ -4747,6 +4877,7 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
>  	int err;
>  	int addr;
>  
> +	sio_data->access = access_direct;
>  	sio_data->sioreg = sioaddr;
>  
>  	err = sio_data->enter(sio_data);
> @@ -4841,6 +4972,21 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
>   */
>  static struct platform_device *pdev[2];
>  
> +#define NCT6775_REG_CHIPID 0x58
> +
> +static const char * const asus_wmi_boards[] = {
> +	"PRIME B460-PLUS",
> +	"ROG CROSSHAIR VIII IMPACT",
> +	"ROG STRIX B550-E GAMING",
> +	"ROG STRIX B550-F GAMING (WI-FI)",
> +	"ROG STRIX Z490-I GAMING",
> +	"TUF GAMING B550M-PLUS",
> +	"TUF GAMING B550M-PLUS (WI-FI)",
> +	"TUF GAMING B550-PLUS",
> +	"TUF GAMING X570-PLUS",
> +	"TUF GAMING X570-PRO (WI-FI)"
> +};
> +
>  static int __init sensors_nct6775_init(void)
>  {
>  	int i, err;
> @@ -4849,11 +4995,29 @@ static int __init sensors_nct6775_init(void)
>  	struct resource res;
>  	struct nct6775_sio_data sio_data;
>  	int sioaddr[2] = { 0x2e, 0x4e };
> +	const char *board_vendor, *board_name;
> +	enum sensor_access access = access_direct;
> +	u8 tmp;
>  
>  	err = platform_driver_register(&nct6775_driver);
>  	if (err)
>  		return err;
>  
> +	board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
> +	board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> +	if (board_name && board_vendor &&
> +	    !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
> +		if (match_string(asus_wmi_boards,
> +				ARRAY_SIZE(asus_wmi_boards), board_name) != -EINVAL) {
> +			/* if reading chip id via WMI succeeds, use WMI */
> +			if (!nct6775_asuswmi_read(0, NCT6775_REG_CHIPID, &tmp)) {
> +				pr_info("Using Asus WMI to access chip\n");
> +				access = access_asuswmi;
> +			}
> +		}
> +	}
> +
>  	/*
>  	 * initialize sio_data->kind and sio_data->sioreg.
>  	 *
> @@ -4874,6 +5038,17 @@ static int __init sensors_nct6775_init(void)
>  
>  		found = true;
>  
> +		/* Update access method */
> +		sio_data.access = access;
> +
> +		if (access == access_asuswmi) {
> +			sio_data.outb = superio_wmi_outb;
> +			sio_data.inb = superio_wmi_inb;
> +			sio_data.select = superio_wmi_select;
> +			sio_data.enter = superio_wmi_enter;
> +			sio_data.exit = superio_wmi_exit;
> +		}
> +
>  		pdev[i] = platform_device_alloc(DRVNAME, address);
>  		if (!pdev[i]) {
>  			err = -ENOMEM;
> @@ -4885,23 +5060,25 @@ static int __init sensors_nct6775_init(void)
>  		if (err)
>  			goto exit_device_put;
>  
> -		memset(&res, 0, sizeof(res));
> -		res.name = DRVNAME;
> -		res.start = address + IOREGION_OFFSET;
> -		res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
> -		res.flags = IORESOURCE_IO;
> +		if (sio_data.access == access_direct) {
> +			memset(&res, 0, sizeof(res));
> +			res.name = DRVNAME;
> +			res.start = address + IOREGION_OFFSET;
> +			res.end = address + IOREGION_OFFSET + IOREGION_LENGTH - 1;
> +			res.flags = IORESOURCE_IO;
> +
> +			err = acpi_check_resource_conflict(&res);
> +			if (err) {
> +				platform_device_put(pdev[i]);
> +				pdev[i] = NULL;
> +				continue;
> +			}
>  
> -		err = acpi_check_resource_conflict(&res);
> -		if (err) {
> -			platform_device_put(pdev[i]);
> -			pdev[i] = NULL;
> -			continue;
> +			err = platform_device_add_resources(pdev[i], &res, 1);
> +			if (err)
> +				goto exit_device_put;
>  		}
>  
> -		err = platform_device_add_resources(pdev[i], &res, 1);
> -		if (err)
> -			goto exit_device_put;
> -
>  		/* platform_device_add calls probe() */
>  		err = platform_device_add(pdev[i]);
>  		if (err)
> -- 
> 2.33.0
> 

Re: [PATCH 3/3] hwmon: (nct6775) Support access via Asus WMI (v2)

[Andy Shevchenko]

On Thu, Sep 09, 2021 at 12:36:04AM +0300, Denis Pauk wrote:
> Support accessing the NCT677x via Asus WMI functions.
> 
> On mainboards that support this way of accessing the chip,
> the driver will usually not work without this option since
> in these mainboards, ACPI will mark the I/O port as used.

...

> +static int asuswmi_evaluate_method(u32 method_id,
> +		u8 bank, u8 reg, u8 val, u32 *retval)

Indentation can be better.
Ditto for many lines in this change.

> +{
> +	u32 args = bank | (reg << 8) | (val << 16);
> +	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	union acpi_object *obj;
> +	u32 tmp = 0;
> +
> +	status = wmi_evaluate_method(ASUSWMI_MGMT2_GUID, 0, method_id,
> +				     &input, &output);
> +
> +	if (ACPI_FAILURE(status))
> +		return -EIO;

> +	obj = (union acpi_object *)output.pointer;

Do you need casting?

> +	if (obj && obj->type == ACPI_TYPE_INTEGER)
> +		tmp = (u32) obj->integer.value;

Ditto.

> +	if (retval)
> +		*retval = tmp;
> +
> +	kfree(obj);
> +
> +	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
> +		return -ENODEV;
> +	return 0;
> +}

...

> +static inline int
> +nct6775_asuswmi_read(u8 bank, u8 reg, u8 *val)

One line.

> +{
> +	u32 tmp;
> +	int ret = asuswmi_evaluate_method(ASUSWMI_METHODID_RHWM, bank, reg, 0, &tmp);

> +	*val = tmp & 0xff;

Do you need  ' & 0xff' part?

> +	return ret;
> +}

...

> +	if (data->access == access_asuswmi) {
> +		data->bank = bank;
> +		return;
> +	}

It means you have to introduce a new callback ->set_bank() (in a separate change).

...

> +	if (data->access == access_asuswmi) {
> +		nct6775_asuswmi_read(data->bank, reg, &tmp);
> +		res = (tmp & 0xff);
> +		if (word_sized) {
> +			nct6775_asuswmi_read(data->bank,
> +					(reg & 0xff) + 1, &tmp);
> +			res = (res << 8) + (tmp & 0xff);
> +		}
> +		return res;
> +	}

Similar.

...

> +	if (data->access == access_asuswmi) {
> +		if (word_sized) {
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff),
> +					(value >> 8) & 0xff);
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
> +					value & 0xff);
> +		} else {
> +			nct6775_asuswmi_write(data->bank, (reg & 0xff),
> +					value);
> +		}
> +
> +		return 0;
> +	}

Similar.

...

> +	if (sio_data->access == access_direct) {
> +		res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +		if (!res)
> +			return -EBUSY;
>  
> -	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> -				 DRVNAME))
> -		return -EBUSY;
> +		if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
> +					 DRVNAME))
> +			return -EBUSY;
> +	}

Maybe it should be part of some kind of ->setup()?

...

> +static const char * const asus_wmi_boards[] = {
> +	"PRIME B460-PLUS",
> +	"ROG CROSSHAIR VIII IMPACT",
> +	"ROG STRIX B550-E GAMING",
> +	"ROG STRIX B550-F GAMING (WI-FI)",
> +	"ROG STRIX Z490-I GAMING",
> +	"TUF GAMING B550M-PLUS",
> +	"TUF GAMING B550M-PLUS (WI-FI)",
> +	"TUF GAMING B550-PLUS",
> +	"TUF GAMING X570-PLUS",

> +	"TUF GAMING X570-PRO (WI-FI)"

+ comma at the end.

> +};

...

> +		if (match_string(asus_wmi_boards,
> +				ARRAY_SIZE(asus_wmi_boards), board_name) != -EINVAL) {

	err = match_string(...);
	if (err < 0) {
		...
	}

> +			/* if reading chip id via WMI succeeds, use WMI */
> +			if (!nct6775_asuswmi_read(0, NCT6775_REG_CHIPID, &tmp)) {
> +				pr_info("Using Asus WMI to access chip\n");
> +				access = access_asuswmi;
> +			}
> +		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] hwmon: (nct6775) Use sio_data in superio_* (v2)

[Andy Shevchenko]

On Thu, Sep 09, 2021 at 12:36:02AM +0300, Denis Pauk wrote:

Thanks for your contribution!
My comments below.

> Rearrange code for directly use struct nct6775_sio_data in superio_*
> functions

Missed period.

We refer to the functions as superio_*().

The commit message may need more elaboration (why you are doing this).

> v2: split changes to separate patches

This should go after '---' (cutter) line below. But entire series needs:
1) a proper versioning (use `git format-patch -v<n> ...`)
2) to NOT be a continuation of the previous one (start a new thread!)
3) to have a cover letter (use `git format-patch --cover-letter`)

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807

BugLink

> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>

This is wrong. My understanding that you have to preserve Bernhard's
authorship and add yourself as Co-developer (see Submitting Patches on
how to properly use tags).

...

> +struct nct6775_sio_data {

> +	int sioreg;

It should be unsigned short.

> +	enum kinds kind;
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] hwmon: (nct6775) Use sio_data in superio_* (v2)

[Denis Pauk]

Thanks for the feedback!

On Thu, 9 Sep 2021 19:50:02 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Sep 09, 2021 at 12:36:02AM +0300, Denis Pauk wrote:
> 
> Thanks for your contribution!
> My comments below.
> 
> > Rearrange code for directly use struct nct6775_sio_data in superio_*
> > functions  
> 
> Missed period.
> 
> We refer to the functions as superio_*().
> 
> The commit message may need more elaboration (why you are doing this).
> 
> > v2: split changes to separate patches  
> 
> This should go after '---' (cutter) line below. But entire series
> needs: 1) a proper versioning (use `git format-patch -v<n> ...`)
> 2) to NOT be a continuation of the previous one (start a new thread!)
> 3) to have a cover letter (use `git format-patch --cover-letter`)
> 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=204807  
> 
> BugLink
> 
> > Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> > Signed-off-by: Denis Pauk <pauk.denis@gmail.com>  
> 
> This is wrong. My understanding that you have to preserve Bernhard's
> authorship and add yourself as Co-developer (see Submitting Patches on
> how to properly use tags).
> 
> ...


Should it be such ?

----
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
Co-Developed-by: Bernhard Seibold <mail@bernhard-seibold.de>
----


> 
> > +struct nct6775_sio_data {  
> 
> > +	int sioreg;  
> 
> It should be unsigned short.
> 
> > +	enum kinds kind;
> > +};  
> 

Should I change all occurrences of sioreg to unsigned short?
Before my patch it was integer. 

--
Best regards,
                  Denis.

Re: [PATCH 1/3] hwmon: (nct6775) Use sio_data in superio_* (v2)

[Andy Shevchenko]

On Sun, Sep 12, 2021 at 1:26 AM Denis Pauk <pauk.denis@gmail.com> wrote:
> On Thu, 9 Sep 2021 19:50:02 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Thu, Sep 09, 2021 at 12:36:02AM +0300, Denis Pauk wrote:

...

> Should it be such ?

I don't know, but if so, then SoB of Bernhard is missing below.

It's all written in the Submitting Patches document:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
> Co-Developed-by: Bernhard Seibold <mail@bernhard-seibold.de>

...

> > > +   int sioreg;
> >
> > It should be unsigned short.

> Should I change all occurrences of sioreg to unsigned short?
> Before my patch it was integer.

Ah, then it's fine. Keep the same type then as is.

-- 
With Best Regards,
Andy Shevchenko