[PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
select bit, but never lets userspace pick a different output frequency.
Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
combination for the requested value in Hz, gated by manual mode like
set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
PWM register (matching the existing scaling assumption in show_pwm1).

The hardware LUT hysteresis register is shared by all LUT entries, so
the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
without N-to-1 cross-attribute side effects. Following the max31760
precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
holding the hysteresis amount in millidegrees; the per-point attributes
stay RO and continue to show the resulting absolute trip-down
temperature for each entry.

This was tested on a Linksys LGS328MPC switch hardware where the fan
would not spin with the default PWM Frequency, which is why this change
is required.

Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>
---
 drivers/hwmon/lm63.c | 112 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index a0d77a7386a9..0fc4a2f0c308 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -92,6 +92,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
 #define LM96163_REG_CONFIG_ENHANCED	0x45
 
+#define LM63_PWM_BASE_FAST_HZ		180000
+#define LM63_PWM_BASE_SLOW_HZ		700
+
 #define LM63_MAX_CONVRATE		9
 
 #define LM63_MAX_CONVRATE_HZ		32
@@ -455,6 +458,75 @@ static ssize_t pwm1_enable_store(struct device *dev,
 	return count;
 }
 
+static ssize_t pwm1_freq_show(struct device *dev,
+			      struct device_attribute *dummy, char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+	unsigned int base = (data->config_fan & 0x08) ?
+			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
+
+	return sprintf(buf, "%u\n", base / data->pwm1_freq);
+}
+
+/*
+ * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
+ * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
+ * bytes are register-relative; rewrite them after a frequency change
+ * if duty cycles must be preserved.
+ */
+static ssize_t pwm1_freq_store(struct device *dev,
+			       struct device_attribute *dummy,
+			       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
+	bool slow_clock;
+	int err;
+
+	if (!(data->config_fan & 0x20)) /* register is read-only */
+		return -EPERM;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+	if (val == 0)
+		return -EINVAL;
+
+	pfr_fast = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_FAST_HZ, val),
+			     1UL, 31UL);
+	pfr_slow = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_SLOW_HZ, val),
+			     1UL, 31UL);
+	err_fast = abs_diff(LM63_PWM_BASE_FAST_HZ / pfr_fast, val);
+	err_slow = abs_diff(LM63_PWM_BASE_SLOW_HZ / pfr_slow, val);
+
+	if (err_slow < err_fast) {
+		slow_clock = true;
+		pfr = pfr_slow;
+	} else {
+		slow_clock = false;
+		pfr = pfr_fast;
+	}
+
+	mutex_lock(&data->update_lock);
+	data->config_fan = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (slow_clock)
+		data->config_fan |= 0x08;
+	else
+		data->config_fan &= ~0x08;
+	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
+	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
+	data->pwm1_freq = pfr;
+
+	if (data->kind == lm96163) {
+		u8 enh = i2c_smbus_read_byte_data(client,
+						  LM96163_REG_CONFIG_ENHANCED);
+		data->pwm_highres = !slow_clock && pfr == 8 && (enh & 0x10);
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * There are 8bit registers for both local(temp1) and remote(temp2) sensor.
  * For remote sensor registers temp2_offset has to be considered,
@@ -629,6 +701,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
 	return sprintf(buf, "%d\n", temp);
 }
 
+/*
+ * The LM63 has a single hysteresis register shared by all LUT entries.
+ * Expose it as a chip-wide hysteresis amount in millidegrees; the
+ * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
+ * show the resulting absolute trip-down temperature for each entry.
+ */
+static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
+					      struct device_attribute *dummy,
+					      char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+
+	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));
+}
+
+static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
+					       struct device_attribute *dummy,
+					       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+	data->lut_temp_hyst = HYST_TO_REG(val);
+	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
+				  data->lut_temp_hyst);
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * And now the other way around, user-space provides an absolute
  * hysteresis value and we have to store a relative one
@@ -764,6 +872,8 @@ static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan,
 
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm1, set_pwm1, 0);
 static DEVICE_ATTR_RW(pwm1_enable);
+static DEVICE_ATTR_RW(pwm1_freq);
+static DEVICE_ATTR_RW(pwm1_auto_point_temp_hyst);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO,
 	show_pwm1, set_pwm1, 1);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO,
@@ -869,6 +979,8 @@ static DEVICE_ATTR_RW(update_interval);
 static struct attribute *lm63_attributes[] = {
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&dev_attr_pwm1_enable.attr,
+	&dev_attr_pwm1_freq.attr,
+	&dev_attr_pwm1_auto_point_temp_hyst.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp_hyst.dev_attr.attr,
-- 
2.48.1

Re: [PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver fails to check if `i2c_smbus_read_byte_data()` returns a negative error code before assigning the result to an unsigned 8-bit variable, leading to hardware register corruption on I2C failure.
--

commit 280d2e57aec56ef87aeac8e747586012f93f520e
Author: Jan-Henrik Bruhn <kernel@jhbruhn.de>

hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

This commit exposes the PWM frequency and LUT hysteresis as writable attributes
for the LM63 hardware monitoring driver. It adds pwm1_freq and a single
chip-wide pwm1_auto_point_temp_hyst attribute to configure the fan control.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..3c9e65e3d81db 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
[ ... ]
> @@ -455,6 +458,75 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
[ ... ]
> +static ssize_t pwm1_freq_store(struct device *dev,
> +			       struct device_attribute *dummy,
> +			       const char *buf, size_t count)
> +{
[ ... ]
> +	mutex_lock(&data->update_lock);
> +	data->config_fan = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);

[Severity: High]
Does this code handle I2C read failures correctly?

If i2c_smbus_read_byte_data() fails, it returns a negative error code.
Because data->config_fan is an 8-bit unsigned variable, this negative
error code is silently truncated into a large positive value.

This corrupted value is then modified and unconditionally written back to
LM63_REG_CONFIG_FAN below, which could clobber other configuration bits
in the register and potentially break hardware fan control.

> +	if (slow_clock)
> +		data->config_fan |= 0x08;
> +	else
> +		data->config_fan &= ~0x08;
> +	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
> +	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
> +	data->pwm1_freq = pfr;
> +
> +	if (data->kind == lm96163) {
> +		u8 enh = i2c_smbus_read_byte_data(client,
> +						  LM96163_REG_CONFIG_ENHANCED);

[Severity: High]
Can this read also fail and silently truncate a negative error code to an
8-bit unsigned variable?

> +		data->pwm_highres = !slow_clock && pfr == 8 && (enh & 0x10);
> +	}
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522135421.2951534-1-kernel@jhbruhn.de?part=1

Re: [PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

> Does this code handle I2C read failures correctly?

It does not, but none of the code in this driver does.
 
> Can this read also fail and silently truncate a negative error code to an
> 8-bit unsigned variable?

Same here - judging on the existing code, hardware failure cases are out of
scope for this driver if it has already successfully initialized.

Re: [PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Guenter Roeck]

On 5/22/26 11:47, Jan-Henrik Bruhn wrote:
>> Does this code handle I2C read failures correctly?
> 
> It does not, but none of the code in this driver does.
>   
>> Can this read also fail and silently truncate a negative error code to an
>> 8-bit unsigned variable?
> 
> Same here - judging on the existing code, hardware failure cases are out of
> scope for this driver if it has already successfully initialized.
> 

Interesting declaration. This isn't about accepting your patch as-is or not,
but please explain why you think that fixing bugs would be out of scope for
this (or any other) driver.

Guenter

Re: [PATCH] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

> Interesting declaration. This isn't about accepting your patch as-is or not,
> but please explain why you think that fixing bugs would be out of scope for
> this (or any other) driver.

Valid point, I framed that wrong - I wanted to stay consistent with the existing 
error handling in this driver, but that isn't a good excuse to skip over potential bugs
when adding new functionality.

I'll submit a v2 which propagates the read errors for the functions added in this patch.

[PATCH v2] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
select bit, but never lets userspace pick a different output frequency.
Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
combination for the requested value in Hz, gated by manual mode like
set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
PWM register (matching the existing scaling assumption in show_pwm1).

The hardware LUT hysteresis register is shared by all LUT entries, so
the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
without N-to-1 cross-attribute side effects. Following the max31760
precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
holding the hysteresis amount in millidegrees; the per-point attributes
stay RO and continue to show the resulting absolute trip-down
temperature for each entry.

This was tested on a Linksys LGS328MPC switch hardware where the fan
would not spin with the default PWM Frequency, which is why this change
is required.

Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>
---
v2:
 - check return value of i2c_smbus_read_byte_data() in pwm1_freq_store()
   and propagate the error instead of silently using truncated values.
---
 drivers/hwmon/lm63.c | 120 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index a0d77a7386a9..586661880db7 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -92,6 +92,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
 #define LM96163_REG_CONFIG_ENHANCED	0x45
 
+#define LM63_PWM_BASE_FAST_HZ		180000
+#define LM63_PWM_BASE_SLOW_HZ		700
+
 #define LM63_MAX_CONVRATE		9
 
 #define LM63_MAX_CONVRATE_HZ		32
@@ -455,6 +458,83 @@ static ssize_t pwm1_enable_store(struct device *dev,
 	return count;
 }
 
+static ssize_t pwm1_freq_show(struct device *dev,
+			      struct device_attribute *dummy, char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+	unsigned int base = (data->config_fan & 0x08) ?
+			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
+
+	return sprintf(buf, "%u\n", base / data->pwm1_freq);
+}
+
+/*
+ * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
+ * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
+ * bytes are register-relative; rewrite them after a frequency change
+ * if duty cycles must be preserved.
+ */
+static ssize_t pwm1_freq_store(struct device *dev,
+			       struct device_attribute *dummy,
+			       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
+	bool slow_clock;
+	int ret;
+
+	if (!(data->config_fan & 0x20)) /* register is read-only */
+		return -EPERM;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+	if (val == 0)
+		return -EINVAL;
+
+	pfr_fast = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_FAST_HZ, val),
+			     1UL, 31UL);
+	pfr_slow = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_SLOW_HZ, val),
+			     1UL, 31UL);
+	err_fast = abs_diff(LM63_PWM_BASE_FAST_HZ / pfr_fast, val);
+	err_slow = abs_diff(LM63_PWM_BASE_SLOW_HZ / pfr_slow, val);
+
+	if (err_slow < err_fast) {
+		slow_clock = true;
+		pfr = pfr_slow;
+	} else {
+		slow_clock = false;
+		pfr = pfr_fast;
+	}
+
+	mutex_lock(&data->update_lock);
+	ret = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (ret < 0) {
+		mutex_unlock(&data->update_lock);
+		return ret;
+	}
+	data->config_fan = ret;
+	if (slow_clock)
+		data->config_fan |= 0x08;
+	else
+		data->config_fan &= ~0x08;
+	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
+	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
+	data->pwm1_freq = pfr;
+
+	if (data->kind == lm96163) {
+		ret = i2c_smbus_read_byte_data(client, LM96163_REG_CONFIG_ENHANCED);
+		if (ret < 0) {
+			mutex_unlock(&data->update_lock);
+			return ret;
+		}
+		data->pwm_highres = !slow_clock && pfr == 8 && (ret & 0x10);
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * There are 8bit registers for both local(temp1) and remote(temp2) sensor.
  * For remote sensor registers temp2_offset has to be considered,
@@ -629,6 +709,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
 	return sprintf(buf, "%d\n", temp);
 }
 
+/*
+ * The LM63 has a single hysteresis register shared by all LUT entries.
+ * Expose it as a chip-wide hysteresis amount in millidegrees; the
+ * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
+ * show the resulting absolute trip-down temperature for each entry.
+ */
+static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
+					      struct device_attribute *dummy,
+					      char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+
+	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));
+}
+
+static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
+					       struct device_attribute *dummy,
+					       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+	data->lut_temp_hyst = HYST_TO_REG(val);
+	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
+				  data->lut_temp_hyst);
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * And now the other way around, user-space provides an absolute
  * hysteresis value and we have to store a relative one
@@ -764,6 +880,8 @@ static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan,
 
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm1, set_pwm1, 0);
 static DEVICE_ATTR_RW(pwm1_enable);
+static DEVICE_ATTR_RW(pwm1_freq);
+static DEVICE_ATTR_RW(pwm1_auto_point_temp_hyst);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO,
 	show_pwm1, set_pwm1, 1);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO,
@@ -869,6 +987,8 @@ static DEVICE_ATTR_RW(update_interval);
 static struct attribute *lm63_attributes[] = {
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&dev_attr_pwm1_enable.attr,
+	&dev_attr_pwm1_freq.attr,
+	&dev_attr_pwm1_auto_point_temp_hyst.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp_hyst.dev_attr.attr,
-- 
2.48.1

Re: [PATCH v2] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Error handling in `pwm1_freq_store` leaves the software cache and hardware out of sync.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `pwm1_freq_store` allows writes to read-only hardware registers.
- [Medium] `pwm1_auto_point_temp_hyst_store` writes to the LUT Hysteresis register without verifying if the LUT is unlocked.
- [Medium] Unprotected reads in `pwm1_freq_show` lead to inconsistent state calculation.
--

commit 14b83e16319e09218194853a200383c1b94b855a
Author: Jan-Henrik Bruhn <kernel@jhbruhn.de>

hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

This commit adds sysfs attributes to allow configuring the PWM frequency 
and LUT hysteresis in the lm63 driver. It exposes pwm1_freq for PWM base 
frequency and a single chip-wide pwm1_auto_point_temp_hyst for hysteresis.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..b95e866ed1ee4 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
[ ... ]
> @@ -455,6 +458,83 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t pwm1_freq_show(struct device *dev,
> +			      struct device_attribute *dummy, char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +	unsigned int base = (data->config_fan & 0x08) ?
> +			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
> +
> +	return sprintf(buf, "%u\n", base / data->pwm1_freq);
> +}

[Severity: Medium]
Does lm63_update_device() release the update_lock before returning?

If so, is there a race condition here? If another thread modifying these
variables in pwm1_freq_store() runs concurrently, could this code read the
new config_fan clock base but the old pwm1_freq divider, resulting in an
incorrect frequency calculation?

> +
> +/*
> + * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
> + * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
> + * bytes are register-relative; rewrite them after a frequency change
> + * if duty cycles must be preserved.
> + */
> +static ssize_t pwm1_freq_store(struct device *dev,
> +			       struct device_attribute *dummy,
> +			       const char *buf, size_t count)
> +{
> +	struct lm63_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
> +	bool slow_clock;
> +	int ret;
> +
> +	if (!(data->config_fan & 0x20)) /* register is read-only */
> +		return -EPERM;

[Severity: Medium]
Can a concurrent update to manual mode lead to a desynchronized cache?

Since this check for manual mode is outside the data->update_lock critical
section, could another thread concurrently disable manual mode before the lock
is acquired below?

If that happens, the hardware would ignore the write to LM63_REG_PWM_FREQ,
but the driver unconditionally updates data->pwm1_freq, potentially leaving the
software cache permanently desynchronized from the hardware.

> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
[ ... ]
> +	mutex_lock(&data->update_lock);
> +	ret = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
> +	if (ret < 0) {
> +		mutex_unlock(&data->update_lock);
> +		return ret;
> +	}
> +	data->config_fan = ret;
> +	if (slow_clock)
> +		data->config_fan |= 0x08;
> +	else
> +		data->config_fan &= ~0x08;
> +	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
> +	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
> +	data->pwm1_freq = pfr;
> +
> +	if (data->kind == lm96163) {
> +		ret = i2c_smbus_read_byte_data(client, LM96163_REG_CONFIG_ENHANCED);
> +		if (ret < 0) {
> +			mutex_unlock(&data->update_lock);
> +			return ret;
> +		}

[Severity: High]
If this read fails, the function aborts and returns an error code without
updating data->pwm_highres.

However, data->config_fan and data->pwm1_freq have already been updated in
the cache and written to hardware. Does returning an error here leave the
hardware state out of sync with what userspace expects, while keeping the
driver's PWM scaling formula permanently desynchronized from the hardware's
actual operating mode?

> +		data->pwm_highres = !slow_clock && pfr == 8 && (ret & 0x10);
> +	}
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}
[ ... ]
> @@ -629,6 +709,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
>  	return sprintf(buf, "%d\n", temp);
>  }
>  
> +/*
> + * The LM63 has a single hysteresis register shared by all LUT entries.
> + * Expose it as a chip-wide hysteresis amount in millidegrees; the
> + * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
> + * show the resulting absolute trip-down temperature for each entry.
> + */
> +static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
> +					      struct device_attribute *dummy,
> +					      char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));
> +}
> +
> +static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
> +					       struct device_attribute *dummy,
> +					       const char *buf, size_t count)
> +{
> +	struct lm63_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &val);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&data->update_lock);
> +	data->lut_temp_hyst = HYST_TO_REG(val);
> +	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
> +				  data->lut_temp_hyst);

[Severity: Medium]
Should this function check if the device is in manual mode before writing?

If a user attempts to write to this attribute while the device is in
automatic mode, it looks like the hardware will silently ignore the write
because the LUT is locked, but the driver updates data->lut_temp_hyst.
Does this cause the sysfs interface to reflect a false state?

> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522202250.3184791-1-kernel@jhbruhn.de?part=1

Re: [PATCH v2] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

Thanks, responses inline:

> Does lm63_update_device() release the update_lock before returning?
>
> If so, is there a race condition here? If another thread modifying these
> variables in pwm1_freq_store() runs concurrently, could this code read the
> new config_fan clock base but the old pwm1_freq divider, resulting in an
> incorrect frequency calculation?

The new show function follows the same pattern as the existing show functions
in this driver: lm63_update_device() refreshes the cache under update_lock
and returns, and the caller then reads cached fields without holding the
lock. Changing that would be a driver-wide change which I do not want 
to slip in here.

> Can a concurrent update to manual mode lead to a desynchronized cache?
>
> Since this check for manual mode is outside the data->update_lock critical
> section, could another thread concurrently disable manual mode before the lock
> is acquired below?
>
> If that happens, the hardware would ignore the write to LM63_REG_PWM_FREQ,
> but the driver unconditionally updates data->pwm1_freq, potentially leaving the
> software cache permanently desynchronized from the hardware.

Fixed in v3: the manual-mode bit is now tested after the in-lock read of
LM63_REG_CONFIG_FAN.

> If this read fails, the function aborts and returns an error code without
> updating data->pwm_highres.
>
> However, data->config_fan and data->pwm1_freq have already been updated in
> the cache and written to hardware. Does returning an error here leave the
> hardware state out of sync with what userspace expects, while keeping the
> driver's PWM scaling formula permanently desynchronized from the hardware's
> actual operating mode?

Fixed in v3: the LM96163_REG_CONFIG_ENHANCED read now happens before the
LM63_REG_CONFIG_FAN / LM63_REG_PWM_FREQ writes, so a failure aborts without
touching hardware.

> Should this function check if the device is in manual mode before writing?
>
> If a user attempts to write to this attribute while the device is in
> automatic mode, it looks like the hardware will silently ignore the write
> because the LUT is locked, but the driver updates data->lut_temp_hyst.
> Does this cause the sysfs interface to reflect a false state?

Per the LM63 datasheet ("Register Descriptions in Functional Order",
page 21), register 0x4F (LUT_TEMP_HYST) is writable regardless of the LUT
lock state. Only the LUT entries themselves are gated by manual mode.

[PATCH v3] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
select bit, but never lets userspace pick a different output frequency.
Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
combination for the requested value in Hz, gated by manual mode like
set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
PWM register (matching the existing scaling assumption in show_pwm1).

The hardware LUT hysteresis register is shared by all LUT entries, so
the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
without N-to-1 cross-attribute side effects. Following the max31760
precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
holding the hysteresis amount in millidegrees; the per-point attributes
stay RO and continue to show the resulting absolute trip-down
temperature for each entry.

This was tested on a Linksys LGS328MPC switch hardware where the fan
would not spin with the default PWM Frequency, which is why this change
is required.

Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>
---
v3:
 - pwm1_freq_store: re-check the manual-mode bit under update_lock
   after the in-lock read of LM63_REG_CONFIG_FAN, instead of relying
   on the cached value before locking.
 - pwm1_freq_store: read LM96163_REG_CONFIG_ENHANCED before writing
   LM63_REG_CONFIG_FAN and LM63_REG_PWM_FREQ so an SMBus failure
   aborts cleanly without leaving the cache out of sync.

v2:
 - check return value of i2c_smbus_read_byte_data() in pwm1_freq_store()
   and propagate the error instead of silently using truncated values.
---
 drivers/hwmon/lm63.c | 123 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index a0d77a7386a9..2a11b9987dfc 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -92,6 +92,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
 #define LM96163_REG_CONFIG_ENHANCED	0x45
 
+#define LM63_PWM_BASE_FAST_HZ		180000
+#define LM63_PWM_BASE_SLOW_HZ		700
+
 #define LM63_MAX_CONVRATE		9
 
 #define LM63_MAX_CONVRATE_HZ		32
@@ -455,6 +458,86 @@ static ssize_t pwm1_enable_store(struct device *dev,
 	return count;
 }
 
+static ssize_t pwm1_freq_show(struct device *dev,
+			      struct device_attribute *dummy, char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+	unsigned int base = (data->config_fan & 0x08) ?
+			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
+
+	return sprintf(buf, "%u\n", base / data->pwm1_freq);
+}
+
+/*
+ * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
+ * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
+ * bytes are register-relative; rewrite them after a frequency change
+ * if duty cycles must be preserved.
+ */
+static ssize_t pwm1_freq_store(struct device *dev,
+			       struct device_attribute *dummy,
+			       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
+	bool slow_clock;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+	if (val == 0)
+		return -EINVAL;
+
+	pfr_fast = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_FAST_HZ, val),
+			     1UL, 31UL);
+	pfr_slow = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_SLOW_HZ, val),
+			     1UL, 31UL);
+	err_fast = abs_diff(LM63_PWM_BASE_FAST_HZ / pfr_fast, val);
+	err_slow = abs_diff(LM63_PWM_BASE_SLOW_HZ / pfr_slow, val);
+
+	if (err_slow < err_fast) {
+		slow_clock = true;
+		pfr = pfr_slow;
+	} else {
+		slow_clock = false;
+		pfr = pfr_fast;
+	}
+
+	mutex_lock(&data->update_lock);
+	ret = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (ret < 0) {
+		mutex_unlock(&data->update_lock);
+		return ret;
+	}
+	data->config_fan = ret;
+
+	if (!(data->config_fan & 0x20)) { /* register is read-only */
+		mutex_unlock(&data->update_lock);
+		return -EPERM;
+	}
+
+	if (data->kind == lm96163) {
+		ret = i2c_smbus_read_byte_data(client, LM96163_REG_CONFIG_ENHANCED);
+		if (ret < 0) {
+			mutex_unlock(&data->update_lock);
+			return ret;
+		}
+		data->pwm_highres = !slow_clock && pfr == 8 && (ret & 0x10);
+	}
+
+	if (slow_clock)
+		data->config_fan |= 0x08;
+	else
+		data->config_fan &= ~0x08;
+	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
+	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
+	data->pwm1_freq = pfr;
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * There are 8bit registers for both local(temp1) and remote(temp2) sensor.
  * For remote sensor registers temp2_offset has to be considered,
@@ -629,6 +712,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
 	return sprintf(buf, "%d\n", temp);
 }
 
+/*
+ * The LM63 has a single hysteresis register shared by all LUT entries.
+ * Expose it as a chip-wide hysteresis amount in millidegrees; the
+ * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
+ * show the resulting absolute trip-down temperature for each entry.
+ */
+static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
+					      struct device_attribute *dummy,
+					      char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+
+	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));
+}
+
+static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
+					       struct device_attribute *dummy,
+					       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+	data->lut_temp_hyst = HYST_TO_REG(val);
+	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
+				  data->lut_temp_hyst);
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * And now the other way around, user-space provides an absolute
  * hysteresis value and we have to store a relative one
@@ -764,6 +883,8 @@ static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan,
 
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm1, set_pwm1, 0);
 static DEVICE_ATTR_RW(pwm1_enable);
+static DEVICE_ATTR_RW(pwm1_freq);
+static DEVICE_ATTR_RW(pwm1_auto_point_temp_hyst);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO,
 	show_pwm1, set_pwm1, 1);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO,
@@ -869,6 +990,8 @@ static DEVICE_ATTR_RW(update_interval);
 static struct attribute *lm63_attributes[] = {
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&dev_attr_pwm1_enable.attr,
+	&dev_attr_pwm1_freq.attr,
+	&dev_attr_pwm1_auto_point_temp_hyst.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp_hyst.dev_attr.attr,
-- 
2.48.1

Re: [PATCH v3] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing lock in `pwm1_freq_show` causes a data race and a potential divide-by-zero kernel panic.
--

commit 1bf46f1b90af417821722eaed56484734ef9e3ae
Author: Jan-Henrik Bruhn <kernel@jhbruhn.de>

hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

This commit exposes the PWM frequency and LUT hysteresis as writable
sysfs attributes. It adds a pwm1_freq attribute to select the closest
SCS + PFR combination, and exposes a chip-wide pwm1_auto_point_temp_hyst
attribute for the hysteresis amount.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 30500b4d22212..90c49c448c624 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c

[ ... ]

> @@ -455,6 +458,86 @@ static ssize_t pwm1_enable_store(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t pwm1_freq_show(struct device *dev,
> +			      struct device_attribute *dummy, char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +	unsigned int base = (data->config_fan & 0x08) ?
> +			    LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
> +
> +	return sprintf(buf, "%u\n", base / data->pwm1_freq);

[Severity: High]
Can this read of data->pwm1_freq race with an update, resulting in a
divide-by-zero exception?

Looking at lm63_update_device(), it acquires data->update_lock and can
temporarily set data->pwm1_freq to 0 before clamping it to 1:

lm63_update_device() {
    mutex_lock(&data->update_lock);
    ...
    data->pwm1_freq = i2c_smbus_read_byte_data(client, LM63_REG_PWM_FREQ);
    if (data->pwm1_freq == 0)
        data->pwm1_freq = 1;
    ...
}

Since pwm1_freq_show() does not hold data->update_lock, could it read
data->pwm1_freq during this transient 0 state and cause a kernel panic?

> +}
> +

[ ... ]

> @@ -629,6 +712,42 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
>  	return sprintf(buf, "%d\n", temp);
>  }
>  
> +/*
> + * The LM63 has a single hysteresis register shared by all LUT entries.
> + * Expose it as a chip-wide hysteresis amount in millidegrees; the
> + * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
> + * show the resulting absolute trip-down temperature for each entry.
> + */
> +static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
> +					      struct device_attribute *dummy,
> +					      char *buf)
> +{
> +	struct lm63_data *data = lm63_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", TEMP8_FROM_REG(data->lut_temp_hyst));

[Severity: High]
Does accessing data->lut_temp_hyst here without holding data->update_lock
also result in a data race with concurrent updates?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523095412.3341202-1-kernel@jhbruhn.de?part=1

Re: [PATCH v2] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Guenter Roeck]

On 5/23/26 02:53, Jan-Henrik Bruhn wrote:
> Thanks, responses inline:
> 
>> Does lm63_update_device() release the update_lock before returning?
>>
>> If so, is there a race condition here? If another thread modifying these
>> variables in pwm1_freq_store() runs concurrently, could this code read the
>> new config_fan clock base but the old pwm1_freq divider, resulting in an
>> incorrect frequency calculation?
> 
> The new show function follows the same pattern as the existing show functions
> in this driver: lm63_update_device() refreshes the cache under update_lock
> and returns, and the caller then reads cached fields without holding the
> lock. Changing that would be a driver-wide change which I do not want
> to slip in here.
> 

You keep telling me that the existence of bugs in the driver warrants
introducing new bugs. Sorry, this is unacceptable. I won't expect you to fix
existing bugs, but I will not accept introducing new ones.

Guenter

Re: [PATCH v2] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

> You keep telling me that the existence of bugs in the driver warrants
> introducing new bugs. Sorry, this is unacceptable. I won't expect you to fix
> existing bugs, but I will not accept introducing new ones.

Understood, v4 will fix this bug with an atomic snapshot using the mutex.

Jan-Henrik

[PATCH v4] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Jan-Henrik Bruhn]

The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
select bit, but never lets userspace pick a different output frequency.
Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
combination for the requested value in Hz, gated by manual mode like
set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
PWM register (matching the existing scaling assumption in show_pwm1).

The hardware LUT hysteresis register is shared by all LUT entries, so
the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
without N-to-1 cross-attribute side effects. Following the max31760
precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
holding the hysteresis amount in millidegrees; the per-point attributes
stay RO and continue to show the resulting absolute trip-down
temperature for each entry.

This was tested on a Linksys LGS328MPC switch hardware where the fan
would not spin with the default PWM Frequency, which is why this change
is required.

Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>
---
v4:
 - pwm1_freq_show: snapshot data->config_fan and data->pwm1_freq
   together under update_lock to prevent observing a concurrent
   pwm1_freq_store.
 - pwm1_auto_point_temp_hyst_show: read data->lut_temp_hs under
   update_lock for the same reason.
v3:
 - pwm1_freq_store: re-check the manual-mode bit under update_lock
   after the in-lock read of LM63_REG_CONFIG_FAN, instead of relying
   on the cached value before locking.
 - pwm1_freq_store: read LM96163_REG_CONFIG_ENHANCED before writing
   LM63_REG_CONFIG_FAN and LM63_REG_PWM_FREQ so an SMBus failure
   aborts cleanly without leaving the cache out of sync.

v2:
 - check return value of i2c_smbus_read_byte_data() in pwm1_freq_store()
   and propagate the error instead of silently using truncated values.
---
 drivers/hwmon/lm63.c | 133 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index a0d77a7386a9..e2a429e579ac 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -92,6 +92,9 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
 #define LM96163_REG_CONFIG_ENHANCED	0x45
 
+#define LM63_PWM_BASE_FAST_HZ		180000
+#define LM63_PWM_BASE_SLOW_HZ		700
+
 #define LM63_MAX_CONVRATE		9
 
 #define LM63_MAX_CONVRATE_HZ		32
@@ -455,6 +458,91 @@ static ssize_t pwm1_enable_store(struct device *dev,
 	return count;
 }
 
+static ssize_t pwm1_freq_show(struct device *dev,
+			      struct device_attribute *dummy, char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+	unsigned int base, freq;
+
+	mutex_lock(&data->update_lock);
+	base = (data->config_fan & 0x08) ?
+	       LM63_PWM_BASE_SLOW_HZ : LM63_PWM_BASE_FAST_HZ;
+	freq = data->pwm1_freq;
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%u\n", base / freq);
+}
+
+/*
+ * Pick the closest CONFIG_FAN.SCS + PFR for the requested frequency.
+ * PWM_FREQ writes need the LUT unlocked, same as set_pwm1(). LUT PWM
+ * bytes are register-relative; rewrite them after a frequency change
+ * if duty cycles must be preserved.
+ */
+static ssize_t pwm1_freq_store(struct device *dev,
+			       struct device_attribute *dummy,
+			       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val, pfr_fast, pfr_slow, err_fast, err_slow, pfr;
+	bool slow_clock;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+	if (val == 0)
+		return -EINVAL;
+
+	pfr_fast = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_FAST_HZ, val),
+			     1UL, 31UL);
+	pfr_slow = clamp_val(DIV_ROUND_CLOSEST((unsigned long)LM63_PWM_BASE_SLOW_HZ, val),
+			     1UL, 31UL);
+	err_fast = abs_diff(LM63_PWM_BASE_FAST_HZ / pfr_fast, val);
+	err_slow = abs_diff(LM63_PWM_BASE_SLOW_HZ / pfr_slow, val);
+
+	if (err_slow < err_fast) {
+		slow_clock = true;
+		pfr = pfr_slow;
+	} else {
+		slow_clock = false;
+		pfr = pfr_fast;
+	}
+
+	mutex_lock(&data->update_lock);
+	ret = i2c_smbus_read_byte_data(client, LM63_REG_CONFIG_FAN);
+	if (ret < 0) {
+		mutex_unlock(&data->update_lock);
+		return ret;
+	}
+	data->config_fan = ret;
+
+	if (!(data->config_fan & 0x20)) { /* register is read-only */
+		mutex_unlock(&data->update_lock);
+		return -EPERM;
+	}
+
+	if (data->kind == lm96163) {
+		ret = i2c_smbus_read_byte_data(client, LM96163_REG_CONFIG_ENHANCED);
+		if (ret < 0) {
+			mutex_unlock(&data->update_lock);
+			return ret;
+		}
+		data->pwm_highres = !slow_clock && pfr == 8 && (ret & 0x10);
+	}
+
+	if (slow_clock)
+		data->config_fan |= 0x08;
+	else
+		data->config_fan &= ~0x08;
+	i2c_smbus_write_byte_data(client, LM63_REG_CONFIG_FAN, data->config_fan);
+	i2c_smbus_write_byte_data(client, LM63_REG_PWM_FREQ, pfr);
+	data->pwm1_freq = pfr;
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * There are 8bit registers for both local(temp1) and remote(temp2) sensor.
  * For remote sensor registers temp2_offset has to be considered,
@@ -629,6 +717,47 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
 	return sprintf(buf, "%d\n", temp);
 }
 
+/*
+ * The LM63 has a single hysteresis register shared by all LUT entries.
+ * Expose it as a chip-wide hysteresis amount in millidegrees; the
+ * per-point pwm1_auto_pointN_temp_hyst attributes remain read-only and
+ * show the resulting absolute trip-down temperature for each entry.
+ */
+static ssize_t pwm1_auto_point_temp_hyst_show(struct device *dev,
+					      struct device_attribute *dummy,
+					      char *buf)
+{
+	struct lm63_data *data = lm63_update_device(dev);
+	u8 hyst;
+
+	mutex_lock(&data->update_lock);
+	hyst = data->lut_temp_hyst;
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%d\n", TEMP8_FROM_REG(hyst));
+}
+
+static ssize_t pwm1_auto_point_temp_hyst_store(struct device *dev,
+					       struct device_attribute *dummy,
+					       const char *buf, size_t count)
+{
+	struct lm63_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+	data->lut_temp_hyst = HYST_TO_REG(val);
+	i2c_smbus_write_byte_data(client, LM63_REG_LUT_TEMP_HYST,
+				  data->lut_temp_hyst);
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
 /*
  * And now the other way around, user-space provides an absolute
  * hysteresis value and we have to store a relative one
@@ -764,6 +893,8 @@ static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan,
 
 static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm1, set_pwm1, 0);
 static DEVICE_ATTR_RW(pwm1_enable);
+static DEVICE_ATTR_RW(pwm1_freq);
+static DEVICE_ATTR_RW(pwm1_auto_point_temp_hyst);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO,
 	show_pwm1, set_pwm1, 1);
 static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO,
@@ -869,6 +1000,8 @@ static DEVICE_ATTR_RW(update_interval);
 static struct attribute *lm63_attributes[] = {
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&dev_attr_pwm1_enable.attr,
+	&dev_attr_pwm1_freq.attr,
+	&dev_attr_pwm1_auto_point_temp_hyst.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_temp_hyst.dev_attr.attr,
-- 
2.48.1

Re: [PATCH v4] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[sashiko-bot]

> The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
> select bit, but never lets userspace pick a different output frequency.
> Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
> combination for the requested value in Hz, gated by manual mode like
> set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
> PWM register (matching the existing scaling assumption in show_pwm1).
> 
> The hardware LUT hysteresis register is shared by all LUT entries, so
> the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
> without N-to-1 cross-attribute side effects. Following the max31760
> precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
> holding the hysteresis amount in millidegrees; the per-point attributes
> stay RO and continue to show the resulting absolute trip-down
> temperature for each entry.
> 
> This was tested on a Linksys LGS328MPC switch hardware where the fan
> would not spin with the default PWM Frequency, which is why this change
> is required.
> 
> Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523133617.3439102-1-kernel@jhbruhn.de?part=1

Re: [PATCH v4] hwmon: (lm63) expose PWM frequency and LUT hysteresis as writable

[Guenter Roeck]

On Sat, May 23, 2026 at 03:36:17PM +0200, Jan-Henrik Bruhn wrote:
> The driver caches the PWM frequency register and the CONFIG_FAN slow-clock
> select bit, but never lets userspace pick a different output frequency.
> Add a pwm1_freq sysfs attribute that selects the closest SCS + PFR
> combination for the requested value in Hz, gated by manual mode like
> set_pwm1(). PFR is clamped to 31 so that 2*PFR fits in the chip's 6-bit
> PWM register (matching the existing scaling assumption in show_pwm1).
> 
> The hardware LUT hysteresis register is shared by all LUT entries, so
> the per-point pwm1_auto_pointN_temp_hyst attributes can't be made RW
> without N-to-1 cross-attribute side effects. Following the max31760
> precedent, expose a single chip-wide pwm1_auto_point_temp_hyst attribute
> holding the hysteresis amount in millidegrees; the per-point attributes
> stay RO and continue to show the resulting absolute trip-down
> temperature for each entry.
> 
> This was tested on a Linksys LGS328MPC switch hardware where the fan
> would not spin with the default PWM Frequency, which is why this change
> is required.
> 
> Signed-off-by: Jan-Henrik Bruhn <kernel@jhbruhn.de>

Applied.

Thanks,
Guenter