[PATCH 0/5] hwmon: (nct6683) Fan control for ASRock B850 Steel Legend WiFi

[PATCH 0/5] hwmon: (nct6683) Fan control for ASRock B850 Steel Legend WiFi

[Johan Dahlin]

Fan control does not work on an ASRock B850 Steel Legend WiFi (NCT6686D,
EC firmware 1.0 build 10/29/24). The driver does not recognise the board's
customer ID, so it only probes with force=1. Its pwm attributes are then
read-only, and even once writable the EC ignores them until the fan is put
in manual mode through register 0xa00, which the driver does not expose.
Writes that do get through are sometimes silently discarded, because the
settling delay in store_pwm() is too short on this firmware.

This series fixes those one at a time. Patch 1 stands on its own: a write
that succeeds on the first attempt issues the same register sequence as
today, so boards that work now are unaffected. The fan control changes are
exposed on verified boards only.

With the whole series applied, all six fans report and pwm[1-6]_enable read
2 after probe:

  fan1: 815 RPM   fan2: 631 RPM   fan5: 386 RPM   fan6: 354 RPM

With pwm5 in manual mode:

  written   applied   fan5
  60        60        337 RPM
  100       100       443 RPM
  150       150       627 RPM
  200       200       794 RPM
  255       255       933 RPM

Writing 2 back returns the fan to the EC, as does unloading the driver.
Without patch 1, two of six writes were lost and the fan kept following its
previous value.

Each patch was also built individually. I have no access to Mitac, MSI or
Intel hardware.

Johan Dahlin (5):
  hwmon: (nct6683) Retry pwm writes until they take effect
  hwmon: (nct6683) Add customer ID for ASRock B850 Steel Legend WiFi
  hwmon: (nct6683) Add pwm[1-8]_enable
  hwmon: (nct6683) Restore fan control mode on driver removal
  hwmon: (nct6683) Enable pwm control on ASRock B850 Steel Legend WiFi

 Documentation/hwmon/nct6683.rst |   7 ++
 drivers/hwmon/nct6683.c         | 124 +++++++++++++++++++++++++++++---
 2 files changed, 122 insertions(+), 9 deletions(-)

-- 
2.53.0

[PATCH 1/5] hwmon: (nct6683) Retry pwm writes until they take effect

[Johan Dahlin]

store_pwm() waits a fixed 1-2 ms for the EC to hand over the fan
configuration registers before writing. That delay dates back to
commit 91918d13eb17 ("hwmon: (nct6683) Add basic support for NCT6683 on
Mitac boards") and, pwm being read-only elsewhere, has never been
exercised against other firmware.

It is too short for at least one of them: on an ASRock B850 Steel Legend
WiFi (NCT6686D), two of six consecutive pwm writes were silently discarded,
leaving the fan on its previous setting.

Read the value back and repeat the sequence when it did not stick, waiting
longer each time and reporting -EIO if it never does. Raising the delay for
everyone would penalise boards that work today, while a write that succeeds
first time issues the same register sequence as before.

Signed-off-by: Johan Dahlin <jdahlin@gmail.com>
---
 drivers/hwmon/nct6683.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
index e1c36c95affb..b524a45b1471 100644
--- a/drivers/hwmon/nct6683.c
+++ b/drivers/hwmon/nct6683.c
@@ -934,18 +934,29 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 	struct nct6683_data *data = dev_get_drvdata(dev);
 	int index = sattr->index;
 	unsigned long val;
+	int tries;
 
 	if (kstrtoul(buf, 10, &val) || val > 255)
 		return -EINVAL;
 
+	/*
+	 * The EC does not always release the fan configuration registers
+	 * within the settling delay, and a write issued before it does is
+	 * silently discarded. Repeat the sequence until the value sticks.
+	 */
 	mutex_lock(&data->update_lock);
-	nct6683_write(data, NCT6683_REG_FAN_CFG_CTRL, NCT6683_FAN_CFG_REQ);
-	usleep_range(1000, 2000);
-	nct6683_write(data, NCT6683_REG_PWM_WRITE(index), val);
-	nct6683_write(data, NCT6683_REG_FAN_CFG_CTRL, NCT6683_FAN_CFG_DONE);
+	for (tries = 0; tries < 3; tries++) {
+		nct6683_write(data, NCT6683_REG_FAN_CFG_CTRL, NCT6683_FAN_CFG_REQ);
+		/* A failed attempt means the EC needed longer than we waited */
+		usleep_range(1000 * (tries + 1), 2000 * (tries + 1));
+		nct6683_write(data, NCT6683_REG_PWM_WRITE(index), val);
+		nct6683_write(data, NCT6683_REG_FAN_CFG_CTRL, NCT6683_FAN_CFG_DONE);
+		if (nct6683_read(data, NCT6683_REG_PWM_WRITE(index)) == val)
+			break;
+	}
 	mutex_unlock(&data->update_lock);
 
-	return count;
+	return tries == 3 ? -EIO : count;
 }
 
 SENSOR_TEMPLATE(pwm, "pwm%d", S_IRUGO, show_pwm, store_pwm, 0);
-- 
2.53.0

[PATCH 2/5] hwmon: (nct6683) Add customer ID for ASRock B850 Steel Legend WiFi

[Johan Dahlin]

The ASRock B850 Steel Legend WiFi uses an NCT6686D chip reporting customer
ID 0x164a. Without this ID the driver refuses to probe unless the force
module parameter is set, leaving the board without hardware monitoring.

Add NCT6683_CUSTOMER_ID_ASROCK8 and handle it in the probe function.

Signed-off-by: Johan Dahlin <jdahlin@gmail.com>
---
 Documentation/hwmon/nct6683.rst | 1 +
 drivers/hwmon/nct6683.c         | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/hwmon/nct6683.rst b/Documentation/hwmon/nct6683.rst
index 45eec9dd349a..908c0b10c642 100644
--- a/Documentation/hwmon/nct6683.rst
+++ b/Documentation/hwmon/nct6683.rst
@@ -66,6 +66,7 @@ ASRock X570			NCT6683D EC firmware version 1.0 build 06/28/19
 ASRock X670E			NCT6686D EC firmware version 1.0 build 05/19/22
 ASRock B650 Steel Legend WiFi	NCT6686D EC firmware version 1.0 build 11/09/23
 ASRock Z590 Taichi		NCT6686D EC firmware version 1.0 build 01/25/21
+ASRock B850 Steel Legend WiFi	NCT6686D EC firmware version 1.0 build 10/29/24
 MSI B550			NCT6687D EC firmware version 1.0 build 05/07/20
 MSI X670-P			NCT6687D EC firmware version 0.0 build 09/27/22
 MSI X870E			NCT6687D EC firmware version 0.0 build 11/13/24
diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
index b524a45b1471..b43e915f9d0f 100644
--- a/drivers/hwmon/nct6683.c
+++ b/drivers/hwmon/nct6683.c
@@ -184,6 +184,7 @@ superio_exit(int ioreg)
 #define NCT6683_CUSTOMER_ID_ASROCK5	0x1621
 #define NCT6683_CUSTOMER_ID_ASROCK6	0x1633
 #define NCT6683_CUSTOMER_ID_ASROCK7	0x163d
+#define NCT6683_CUSTOMER_ID_ASROCK8	0x164a
 
 #define NCT6683_REG_BUILD_YEAR		0x604
 #define NCT6683_REG_BUILD_MONTH		0x605
@@ -1262,6 +1263,8 @@ static int nct6683_probe(struct platform_device *pdev)
 		break;
 	case NCT6683_CUSTOMER_ID_ASROCK7:
 		break;
+	case NCT6683_CUSTOMER_ID_ASROCK8:
+		break;
 	default:
 		if (!force)
 			return -ENODEV;
-- 
2.53.0

[PATCH 3/5] hwmon: (nct6683) Add pwm[1-8]_enable

[Johan Dahlin]

NCT668x keeps a per-fan manual-control bitmap in register 0xa00. While a
fan's bit is clear the EC runs its own control loop and ignores the pwm
registers, so a pwm write succeeds and then has no effect on the fan.

Expose the bitmap as pwm[1-8]_enable following the hwmon ABI, with 1 for
manual control and 2 for the EC's automatic control. The attribute group
now has two attributes per channel, so is_visible derives the channel from
the attribute index.

Intel boards run a firmware variant that Nuvoton confirms uses different
register addresses, so 0xa00 cannot be assumed to mean the same thing
there. Only expose the attribute where fan control has been verified.

The register was derived from the out-of-tree nct6687d driver, which
documents it from LibreHardwareMonitor's reverse engineering.

Link: https://github.com/Fred78290/nct6687d
Link: https://github.com/LibreHardwareMonitor/LibreHardwareMonitor
Signed-off-by: Johan Dahlin <jdahlin@gmail.com>
---
 Documentation/hwmon/nct6683.rst |  6 +++
 drivers/hwmon/nct6683.c         | 77 +++++++++++++++++++++++++++++++--
 2 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/nct6683.rst b/Documentation/hwmon/nct6683.rst
index 908c0b10c642..ff5edb34c5c5 100644
--- a/Documentation/hwmon/nct6683.rst
+++ b/Documentation/hwmon/nct6683.rst
@@ -49,6 +49,12 @@ The driver has only been tested with the Intel firmware, and by default
 only instantiates on Intel boards. To enable it on non-Intel boards,
 set the 'force' module parameter to 1.
 
+Fan control is likewise restricted. The EC runs its own control loop unless
+a fan is switched to manual mode by writing 1 to its pwm[1-8]_enable
+attribute, and pwm values are only writable on boards where this has been
+verified to work. On all other boards the pwm attributes are read-only and
+the fan curve must be configured from the BIOS.
+
 Tested Boards and Firmware Versions
 -----------------------------------
 
diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
index b43e915f9d0f..0324ace707a5 100644
--- a/drivers/hwmon/nct6683.c
+++ b/drivers/hwmon/nct6683.c
@@ -165,6 +165,7 @@ superio_exit(int ioreg)
 
 #define NCT6683_REG_FAN_MIN(x)		(0x3b8 + (x) * 2)	/* 16 bit */
 
+#define NCT6683_REG_FAN_CTRL_MODE	0xa00
 #define NCT6683_REG_FAN_CFG_CTRL	0xa01
 #define NCT6683_FAN_CFG_REQ		0x80
 #define NCT6683_FAN_CFG_DONE		0x40
@@ -962,25 +963,93 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 
 SENSOR_TEMPLATE(pwm, "pwm%d", S_IRUGO, show_pwm, store_pwm, 0);
 
+/*
+ * Fan control has only been verified on the boards listed here. Intel boards
+ * in particular run a firmware variant that Nuvoton confirms uses different
+ * register addresses, so 0xa00 cannot be assumed to mean the same thing
+ * there. Leave the fans to the EC everywhere else.
+ */
+static bool nct6683_has_fan_control(struct nct6683_data *data)
+{
+	return data->customer_id == NCT6683_CUSTOMER_ID_MITAC;
+}
+
+/*
+ * NCT668x keeps a per-fan manual-control bitmap. While a fan's bit is clear
+ * the EC runs its own control loop and ignores the pwm registers, so pwm
+ * writes only take effect once the fan has been switched to manual mode.
+ */
+static ssize_t
+show_pwm_enable(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct nct6683_data *data = dev_get_drvdata(dev);
+	u8 mode;
+
+	mutex_lock(&data->update_lock);
+	mode = nct6683_read(data, NCT6683_REG_FAN_CTRL_MODE);
+	mutex_unlock(&data->update_lock);
+
+	return sysfs_emit(buf, "%d\n", (mode & BIT(sattr->index)) ? 1 : 2);
+}
+
+static ssize_t
+store_pwm_enable(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct nct6683_data *data = dev_get_drvdata(dev);
+	u8 bit = BIT(sattr->index);
+	unsigned long val;
+	bool manual;
+	int err = 0;
+	u8 mode;
+
+	if (kstrtoul(buf, 10, &val) || (val != 1 && val != 2))
+		return -EINVAL;
+
+	manual = val == 1;
+
+	mutex_lock(&data->update_lock);
+	mode = nct6683_read(data, NCT6683_REG_FAN_CTRL_MODE);
+	if (manual)
+		mode |= bit;
+	else
+		mode &= ~bit;
+	nct6683_write(data, NCT6683_REG_FAN_CTRL_MODE, mode);
+
+	if (!!(nct6683_read(data, NCT6683_REG_FAN_CTRL_MODE) & bit) != manual)
+		err = -EIO;
+	mutex_unlock(&data->update_lock);
+
+	return err ? err : count;
+}
+
+SENSOR_TEMPLATE(pwm_enable, "pwm%d_enable", 0444, show_pwm_enable,
+		store_pwm_enable, 0);
+
 static umode_t nct6683_pwm_is_visible(struct kobject *kobj,
 				      struct attribute *attr, int index)
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct nct6683_data *data = dev_get_drvdata(dev);
-	int pwm = index;	/* pwm index */
+	int pwm = index / 2;	/* pwm index */
+	int nr = index % 2;	/* attribute index */
 
 	if (!(data->have_pwm & (1 << pwm)))
 		return 0;
 
-	/* Only update pwm values for Mitac boards */
-	if (data->customer_id == NCT6683_CUSTOMER_ID_MITAC)
+	/* Only touch fan control on boards where it has been verified */
+	if (nct6683_has_fan_control(data))
 		return attr->mode | S_IWUSR;
 
-	return attr->mode;
+	/* Elsewhere hide pwm_enable and keep pwm read-only */
+	return nr ? 0 : attr->mode;
 }
 
 static struct sensor_device_template *nct6683_attributes_pwm_template[] = {
 	&sensor_dev_template_pwm,
+	&sensor_dev_template_pwm_enable,
 	NULL
 };
 
-- 
2.53.0

[PATCH 4/5] hwmon: (nct6683) Restore fan control mode on driver removal

[Johan Dahlin]

A fan switched to manual mode stays there when the driver is unloaded,
leaving it pinned at whatever pwm value was last written with nothing left
to update it.

Record the fan control mode register during probe and put it back with a
devres action, so the EC resumes control of the fans the driver took over
while anything the firmware had already set is left alone. The action is
registered before the hwmon device, so it runs after the attributes are
gone and cannot race with a pwm write, and it is a no-op if probe fails
before the driver touches the register.

Boards where the driver does not manage the register do not register it.

Signed-off-by: Johan Dahlin <jdahlin@gmail.com>
---
 drivers/hwmon/nct6683.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
index 0324ace707a5..6825e6c3af34 100644
--- a/drivers/hwmon/nct6683.c
+++ b/drivers/hwmon/nct6683.c
@@ -344,6 +344,7 @@ struct nct6683_data {
 	u16 have_fan;			/* some fan inputs can be disabled */
 
 	u8 have_pwm;
+	u8 initial_fan_ctrl_mode;
 	u8 pwm[NCT6683_NUM_REG_PWM];
 
 #ifdef CONFIG_PM
@@ -1275,6 +1276,17 @@ static void nct6683_setup_sensors(struct nct6683_data *data)
 	}
 }
 
+/* Put the fan control mode register back the way probe found it. */
+static void nct6683_restore_fan_control(void *_data)
+{
+	struct nct6683_data *data = _data;
+
+	mutex_lock(&data->update_lock);
+	nct6683_write(data, NCT6683_REG_FAN_CTRL_MODE,
+		      data->initial_fan_ctrl_mode);
+	mutex_unlock(&data->update_lock);
+}
+
 static int nct6683_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1284,6 +1296,7 @@ static int nct6683_probe(struct platform_device *pdev)
 	struct device *hwmon_dev;
 	struct resource *res;
 	int groups = 0;
+	int err;
 	char build[16];
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
@@ -1401,6 +1414,15 @@ static int nct6683_probe(struct platform_device *pdev)
 		 nct6683_read(data, NCT6683_REG_VERSION_LO),
 		 build);
 
+	if (nct6683_has_fan_control(data)) {
+		data->initial_fan_ctrl_mode =
+			nct6683_read(data, NCT6683_REG_FAN_CTRL_MODE);
+		err = devm_add_action_or_reset(dev, nct6683_restore_fan_control,
+					       data);
+		if (err)
+			return err;
+	}
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
 			nct6683_device_names[data->kind], data, data->groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
-- 
2.53.0

[PATCH 5/5] hwmon: (nct6683) Enable pwm control on ASRock B850 Steel Legend WiFi

[Johan Dahlin]

Fan control has been measured on this board: with a fan in manual mode the
applied pwm value tracks what is written, and the fan responds
proportionally from 337 RPM at pwm 60 to 933 RPM at pwm 255.

Allow pwm writes on this board.

Signed-off-by: Johan Dahlin <jdahlin@gmail.com>
---
 drivers/hwmon/nct6683.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/nct6683.c b/drivers/hwmon/nct6683.c
index 6825e6c3af34..df28055b1a1c 100644
--- a/drivers/hwmon/nct6683.c
+++ b/drivers/hwmon/nct6683.c
@@ -972,7 +972,8 @@ SENSOR_TEMPLATE(pwm, "pwm%d", S_IRUGO, show_pwm, store_pwm, 0);
  */
 static bool nct6683_has_fan_control(struct nct6683_data *data)
 {
-	return data->customer_id == NCT6683_CUSTOMER_ID_MITAC;
+	return data->customer_id == NCT6683_CUSTOMER_ID_MITAC ||
+	       data->customer_id == NCT6683_CUSTOMER_ID_ASROCK8;
 }
 
 /*
-- 
2.53.0