[PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Richard Weinberger]

The Nuvoton 6775 chip family also offers PWM functionality to control
various fans. Depending on the hardware design, the PWM can also be
used for other purposes, such as controlling a display backlight.

Historically, the driver implemented its own sysfs-based interface to
control the PWMs. This made it impossible to use the PWM as a backend
for other drivers, such as pwm-backlight.

This patch registers the PWM functionality as a PWM chip within the
PWM subsystem. Since the Nuvoton chip has non-trivial control logic,
the following constraints exist:

 - Exporting a PWM for external use is only allowed when the fan mode
   is set to manual or off.
 - As soon as a PWM is exported, changing its configuration is no
   longer possible through the hwmon sysfs interface, reading is
   still allowed.
 - Changing the PWM period is not supported. IMHO, it is too risky
   since the PWMs usually control system fans and similar components.
 - Reading and decoding the current PWM period is only supported for
   one chip variant so far, for all other chips, a fixed period of
   100ms is assumed.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/hwmon/nct6775-core.c | 249 +++++++++++++++++++++++++++++++++--
 drivers/hwmon/nct6775.h      |   3 +
 2 files changed, 238 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
index 79bc67ffb9986..bc3b0fb505c39 100644
--- a/drivers/hwmon/nct6775-core.c
+++ b/drivers/hwmon/nct6775-core.c
@@ -56,6 +56,7 @@
 #include <linux/bitops.h>
 #include <linux/nospec.h>
 #include <linux/regmap.h>
+#include <linux/pwm.h>
 #include "lm75.h"
 #include "nct6775.h"
 
@@ -770,6 +771,7 @@ static const u16 NCT6106_FAN_PULSE_SHIFT[] = { 0, 2, 4 };
 static const u8 NCT6106_REG_PWM_MODE[] = { 0xf3, 0xf3, 0xf3, 0, 0 };
 static const u8 NCT6106_PWM_MODE_MASK[] = { 0x01, 0x02, 0x04, 0, 0 };
 static const u16 NCT6106_REG_PWM_READ[] = { 0x4a, 0x4b, 0x4c, 0xd8, 0xd9 };
+static const u16 NCT6106_REG_PWM_FREQ[] = { 0xf0, 0xf1, 0xf2 };
 static const u16 NCT6106_REG_FAN_MODE[] = { 0x113, 0x123, 0x133 };
 static const u16 NCT6106_REG_TEMP_SOURCE[] = {
 	0xb0, 0xb1, 0xb2, 0xb3, 0xb4, 0xb5 };
@@ -2482,6 +2484,9 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
 	int err;
 	u16 reg;
 
+	if (data->pwm_exported[nr])
+		return -EBUSY;
+
 	err = kstrtoul(buf, 10, &val);
 	if (err < 0)
 		return err;
@@ -2510,13 +2515,8 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
 	return err ? : count;
 }
 
-static ssize_t
-show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
+static int read_pwm(struct nct6775_data *data, int nr, int index)
 {
-	struct nct6775_data *data = nct6775_update_device(dev);
-	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
-	int nr = sattr->nr;
-	int index = sattr->index;
 	int err;
 	u16 pwm;
 
@@ -2535,18 +2535,87 @@ show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
 		pwm = data->pwm[index][nr];
 	}
 
-	return sprintf(buf, "%d\n", pwm);
+	return pwm;
+}
+
+static u64 nct6106d_calc_period(u8 val)
+{
+	/*
+	 * Case 1: Bit 7 is set
+	 * --------------------
+	 *
+	 * frequency f in kHz is 92.5 / (val[6:0] + 1)
+	 * persiod_ns = 1000000 / (92.5 / (val[6:0] + 1))
+	 * ...rearrange
+	 * persiod_ns = (1000000 * (val[6:0] + 1)) / 92.5
+	 * ...eleminate decimals places by muliplying with 2 / 2
+	 * persiod_ns = (2000000 * (val[6:0] + 1)) / 185
+	 *
+	 * Case 2: Bit 7 is unset
+	 * ----------------------
+	 *
+	 * frequency f in Hz is 1008 / (val[3:0] + 1)
+	 * persiod_ns = 1000000000 / (1008 / (val[3:0] + 1))
+	 * ...rearrange
+	 * persiod_ns = (1000000000 * (val[3:0] + 1)) / 1008
+	 */
+	if (val & 0x80)
+		return DIV_ROUND_CLOSEST_ULL(2000000ULL * ((val & 0x7F) + 1), 185);
+	else
+		return DIV_ROUND_CLOSEST_ULL(1000000000ULL * ((val & 0x0F) + 1), 1008);
+}
+
+static int get_pwm_period(struct nct6775_data *data, int nr, u64 *period)
+{
+	int err;
+	u16 val;
+
+	if (!data->REG_PWM_FREQ) {
+		/*
+		 * Use 100ms period if PWM frequency can't be obtained.
+		 */
+		*period = 100000000ULL;
+		return 0;
+	}
+
+	if (!data->pwm_freq[nr]) {
+		err = nct6775_read_value(data, data->REG_PWM_FREQ[nr], &val);
+		if (err)
+			return err;
+
+		data->pwm_freq[nr] = val & 0xFF;
+	}
+
+	switch (data->kind) {
+	case nct6106:
+		*period = nct6106d_calc_period(data->pwm_freq[nr]);
+		break;
+	default:
+		WARN_ONCE(1, "REG_PWM_FREQ configured but no calc function");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static ssize_t
-store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
-	  size_t count)
+show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct nct6775_data *data = dev_get_drvdata(dev);
+	struct nct6775_data *data = nct6775_update_device(dev);
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	int nr = sattr->nr;
 	int index = sattr->index;
-	unsigned long val;
+	u16 pwm;
+
+	pwm = read_pwm(data, nr, index);
+	if (pwm < 0)
+		return pwm;
+
+	return sprintf(buf, "%d\n", pwm);
+}
+
+static int write_pwm(struct nct6775_data *data, int nr, int index, unsigned long val)
+{
 	int minval[7] = { 0, 1, 1, data->pwm[2][nr], 0, 0, 0 };
 	int maxval[7]
 	  = { 255, 255, data->pwm[3][nr] ? : 255, 255, 255, 255, 255 };
@@ -2560,9 +2629,6 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 	if (index == 0 && data->pwm_enable[nr] > manual)
 		return -EBUSY;
 
-	err = kstrtoul(buf, 10, &val);
-	if (err < 0)
-		return err;
 	val = clamp_val(val, minval[index], maxval[index]);
 
 	mutex_lock(&data->update_lock);
@@ -2581,6 +2647,28 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
 	}
 out:
 	mutex_unlock(&data->update_lock);
+
+	return 0;
+}
+
+static ssize_t
+store_pwm(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 nct6775_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int err;
+
+	if (data->pwm_exported[sattr->nr])
+		return -EBUSY;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err < 0)
+		return err;
+
+	err = write_pwm(data, sattr->nr, sattr->index, val);
+
 	return err ? : count;
 }
 
@@ -2681,6 +2769,9 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
 	int err;
 	u16 reg;
 
+	if (data->pwm_exported[nr])
+		return -EBUSY;
+
 	err = kstrtoul(buf, 10, &val);
 	if (err < 0)
 		return err;
@@ -3501,6 +3592,131 @@ static int add_temp_sensors(struct nct6775_data *data, const u16 *regp,
 	return 0;
 }
 
+static int nct6775_pwm_round_waveform_fromhw(struct pwm_chip *chip,
+					     struct pwm_device *pwm,
+					     const void *_wfhw,
+					     struct pwm_waveform *wf)
+{
+	struct nct6775_data *data = pwmchip_get_drvdata(chip);
+	const u8 *wfhw = _wfhw;
+
+	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
+		return 1;
+
+	wf->duty_length_ns = mul_u64_u64_div_u64(*wfhw, wf->period_length_ns, 255);
+	wf->duty_offset_ns = 0;
+
+	return 0;
+}
+
+static int nct6775_pwm_round_waveform_tohw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const struct pwm_waveform *wf,
+					   void *_wfhw)
+{
+	struct nct6775_data *data = pwmchip_get_drvdata(chip);
+	u8 *wfhw = _wfhw;
+	u64 cur_period;
+
+	if (wf->period_length_ns == 0) {
+		*wfhw = 0;
+		return 0;
+	}
+
+	if (get_pwm_period(data, pwm->hwpwm, &cur_period))
+		return 1;
+
+	if (wf->duty_length_ns >= cur_period)
+		*wfhw = 255;
+	else
+		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, 255, wf->period_length_ns);
+
+	if (wf->period_length_ns != cur_period)
+		return 1;
+
+	return 0;
+}
+
+
+static int nct6775_pwm_write_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      const void *_wfhw)
+{
+	struct nct6775_data *data = pwmchip_get_drvdata(chip);
+	const u8 *wfhw = _wfhw;
+
+	return write_pwm(data, pwm->hwpwm, 0, *wfhw);
+}
+
+static int nct6775_pwm_read_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     void *_wfhw)
+{
+	struct nct6775_data *data = nct6775_update_device(pwmchip_parent(chip));
+	u8 *wfhw = _wfhw;
+	int val;
+
+	val = read_pwm(data, pwm->hwpwm, 0);
+	if (val < 0)
+		return val;
+
+	*wfhw = (u8)val;
+
+	return 0;
+}
+
+static int nct6775_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct nct6775_data *data = pwmchip_get_drvdata(chip);
+
+	if (data->pwm_enable[pwm->hwpwm] > manual)
+		return -EBUSY;
+
+	data->pwm_exported[pwm->hwpwm] = true;
+
+	return 0;
+}
+
+static void nct6775_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct nct6775_data *data = pwmchip_get_drvdata(chip);
+
+	data->pwm_exported[pwm->hwpwm] = false;
+}
+
+static const struct pwm_ops nct6775_pwm_ops = {
+	.sizeof_wfhw = sizeof(u8),
+	.request = nct6775_pwm_request,
+	.free = nct6775_pwm_free,
+	.round_waveform_fromhw = nct6775_pwm_round_waveform_fromhw,
+	.round_waveform_tohw = nct6775_pwm_round_waveform_tohw,
+	.write_waveform = nct6775_pwm_write_waveform,
+	.read_waveform = nct6775_pwm_read_waveform,
+};
+
+static int nct6775_register_pwm_chip(struct device *dev, struct nct6775_data *data)
+{
+	struct pwm_chip *chip;
+	int ret;
+
+	if (data->pwm_num < 1)
+		return 0;
+
+	chip = devm_pwmchip_alloc(dev, data->pwm_num, 0);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	chip->ops = &nct6775_pwm_ops;
+	pwmchip_set_drvdata(chip, data);
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "Could not add PWM chip\n");
+
+
+	return 0;
+}
+
 int nct6775_probe(struct device *dev, struct nct6775_data *data,
 		  const struct regmap_config *regmapcfg)
 {
@@ -3563,6 +3779,7 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data,
 		data->REG_PWM[6] = NCT6106_REG_WEIGHT_DUTY_BASE;
 		data->REG_PWM_READ = NCT6106_REG_PWM_READ;
 		data->REG_PWM_MODE = NCT6106_REG_PWM_MODE;
+		data->REG_PWM_FREQ = NCT6106_REG_PWM_FREQ;
 		data->PWM_MODE_MASK = NCT6106_PWM_MODE_MASK;
 		data->REG_AUTO_TEMP = NCT6106_REG_AUTO_TEMP;
 		data->REG_AUTO_PWM = NCT6106_REG_AUTO_PWM;
@@ -4379,6 +4596,10 @@ int nct6775_probe(struct device *dev, struct nct6775_data *data,
 	if (err)
 		return err;
 
+	err = nct6775_register_pwm_chip(dev, data);
+	if (err)
+		return err;
+
 	if (data->have_tsi_temp) {
 		tsi_temp_tg.templates = nct6775_tsi_temp_template;
 		tsi_temp_tg.is_visible = nct6775_tsi_temp_is_visible;
diff --git a/drivers/hwmon/nct6775.h b/drivers/hwmon/nct6775.h
index 296eff99d0038..2032962f980f7 100644
--- a/drivers/hwmon/nct6775.h
+++ b/drivers/hwmon/nct6775.h
@@ -65,6 +65,7 @@ struct nct6775_data {
 				 * [5]=weight_duty_step, [6]=weight_duty_base
 				 */
 	const u16 *REG_PWM_READ;
+	const u16 *REG_PWM_FREQ;
 
 	const u16 *REG_CRITICAL_PWM_ENABLE;
 	u8 CRITICAL_PWM_ENABLE_MASK;
@@ -137,6 +138,8 @@ struct nct6775_data {
 				 * [3]=pwm_max, [4]=pwm_step,
 				 * [5]=weight_duty_step, [6]=weight_duty_base
 				 */
+	u8 pwm_freq[NUM_FAN];
+	bool pwm_exported[NUM_FAN];
 
 	u8 target_temp[NUM_FAN];
 	u8 target_temp_mask;
-- 
2.51.0

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4169 bytes --]

Hello Richard,

just a very quick look:

On Wed, Feb 25, 2026 at 01:51:59PM +0100, Richard Weinberger wrote:
> @@ -3501,6 +3592,131 @@ static int add_temp_sensors(struct nct6775_data *data, const u16 *regp,
>  	return 0;
>  }
>  
> +static int nct6775_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					     struct pwm_device *pwm,
> +					     const void *_wfhw,
> +					     struct pwm_waveform *wf)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	const u8 *wfhw = _wfhw;
> +
> +	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
> +		return 1;

That looks wrong. In principle nct6775_pwm_round_waveform_fromhw()
doesn't depend on hardware state. It's supposed to just convert the
settings stored in _wfhw to wf. If you know that some things are
constant during the lifetime of the PWM and you read those from
hardware, return a proper error code, not 1.

> +	wf->duty_length_ns = mul_u64_u64_div_u64(*wfhw, wf->period_length_ns, 255);
> +	wf->duty_offset_ns = 0;
> +
> +	return 0;
> +}
> +
> +static int nct6775_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const struct pwm_waveform *wf,
> +					   void *_wfhw)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	u8 *wfhw = _wfhw;
> +	u64 cur_period;
> +
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = 0;
> +		return 0;
> +	}
> +
> +	if (get_pwm_period(data, pwm->hwpwm, &cur_period))
> +		return 1;
> +
> +	if (wf->duty_length_ns >= cur_period)
> +		*wfhw = 255;
> +	else
> +		*wfhw = mul_u64_u64_div_u64(wf->duty_length_ns, 255, wf->period_length_ns);
> +
> +	if (wf->period_length_ns != cur_period)
> +		return 1;

Rounding down wf->period_length_ns is fine, so this must be:

	if (wf->period_length_ns < cur_period)
		return 1;

> +
> +	return 0;
> +}
> +
> +
> +static int nct6775_pwm_write_waveform(struct pwm_chip *chip,
> +				      struct pwm_device *pwm,
> +				      const void *_wfhw)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +	const u8 *wfhw = _wfhw;
> +
> +	return write_pwm(data, pwm->hwpwm, 0, *wfhw);
> +}
> +
> +static int nct6775_pwm_read_waveform(struct pwm_chip *chip,
> +				     struct pwm_device *pwm,
> +				     void *_wfhw)
> +{
> +	struct nct6775_data *data = nct6775_update_device(pwmchip_parent(chip));
> +	u8 *wfhw = _wfhw;
> +	int val;
> +
> +	val = read_pwm(data, pwm->hwpwm, 0);
> +	if (val < 0)
> +		return val;
> +
> +	*wfhw = (u8)val;
> +
> +	return 0;
> +}
> +
> +static int nct6775_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +
> +	if (data->pwm_enable[pwm->hwpwm] > manual)
> +		return -EBUSY;
> +
> +	data->pwm_exported[pwm->hwpwm] = true;
> +
> +	return 0;
> +}
> +
> +static void nct6775_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> +
> +	data->pwm_exported[pwm->hwpwm] = false;
> +}
> +
> +static const struct pwm_ops nct6775_pwm_ops = {
> +	.sizeof_wfhw = sizeof(u8),
> +	.request = nct6775_pwm_request,
> +	.free = nct6775_pwm_free,
> +	.round_waveform_fromhw = nct6775_pwm_round_waveform_fromhw,
> +	.round_waveform_tohw = nct6775_pwm_round_waveform_tohw,
> +	.write_waveform = nct6775_pwm_write_waveform,
> +	.read_waveform = nct6775_pwm_read_waveform,
> +};
> +
> +static int nct6775_register_pwm_chip(struct device *dev, struct nct6775_data *data)
> +{
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (data->pwm_num < 1)
> +		return 0;
> +
> +	chip = devm_pwmchip_alloc(dev, data->pwm_num, 0);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->ops = &nct6775_pwm_ops;
> +	pwmchip_set_drvdata(chip, data);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not add PWM chip\n");
> +
> +

s/\n\n/\n/

> +	return 0;
> +}
> +
>  int nct6775_probe(struct device *dev, struct nct6775_data *data,
>  		  const struct regmap_config *regmapcfg)
>  {

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Richard Weinberger]

Hello Uwe,

----- Ursprüngliche Mail -----
> Von: "Uwe Kleine-König" <ukleinek@kernel.org>
>> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
>> +	const u8 *wfhw = _wfhw;
>> +
>> +	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
>> +		return 1;
> 
> That looks wrong. In principle nct6775_pwm_round_waveform_fromhw()
> doesn't depend on hardware state. It's supposed to just convert the
> settings stored in _wfhw to wf. If you know that some things are
> constant during the lifetime of the PWM and you read those from
> hardware, return a proper error code, not 1.

I see. Since the frequency is never changed by the driver we could
read it while probing and use here the cached value.

> Rounding down wf->period_length_ns is fine, so this must be:
> 
>	if (wf->period_length_ns < cur_period)
>		return 1;

But then the period is no longer fixed and something larger than supported
can get configured. Smaller values get caught, though.

e.g.
root@fedora:/sys/class/pwm/pwmchip0/pwm2# cat period 
43243
root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 43200 > period 
bash: echo: write error: Invalid argument
root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 50000 > period 
root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo $?
0

Thanks,
//richard

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Guenter Roeck]

On 2/25/26 04:51, Richard Weinberger wrote:
> The Nuvoton 6775 chip family also offers PWM functionality to control
> various fans. Depending on the hardware design, the PWM can also be
> used for other purposes, such as controlling a display backlight.
> 
> Historically, the driver implemented its own sysfs-based interface to
> control the PWMs. This made it impossible to use the PWM as a backend
> for other drivers, such as pwm-backlight.
> 
> This patch registers the PWM functionality as a PWM chip within the
> PWM subsystem. Since the Nuvoton chip has non-trivial control logic,
> the following constraints exist:
> 
>   - Exporting a PWM for external use is only allowed when the fan mode
>     is set to manual or off.
>   - As soon as a PWM is exported, changing its configuration is no
>     longer possible through the hwmon sysfs interface, reading is
>     still allowed.
>   - Changing the PWM period is not supported. IMHO, it is too risky
>     since the PWMs usually control system fans and similar components.
>   - Reading and decoding the current PWM period is only supported for
>     one chip variant so far, for all other chips, a fixed period of
>     100ms is assumed.
> 

This is a good start, but I'll want to see stronger safeguards.
- Creating a pwmchip entry for a pwm channel must be triggered by
   device property data, obtained either from devicetree or through
   DMI or through device properties embedded in ACPI data. For each
   channel, this must be confirmed by checking that the channel is
   not associated with a fan control channel.
- If a pwm channel is configured as pwmchip, it must not be used for fan
   control, meaning the hwmon properties associated with that channel
   must not be created.

This will retain current functionality while enabling support for using
pwm channels for purposes other than fan control.

Thanks,
Guenter

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Richard Weinberger]

Hello Guenter,

----- Ursprüngliche Mail -----
> Von: "Guenter Roeck" <linux@roeck-us.net>
>>   - Exporting a PWM for external use is only allowed when the fan mode
>>     is set to manual or off.
>>   - As soon as a PWM is exported, changing its configuration is no
>>     longer possible through the hwmon sysfs interface, reading is
>>     still allowed.
>>   - Changing the PWM period is not supported. IMHO, it is too risky
>>     since the PWMs usually control system fans and similar components.
>>   - Reading and decoding the current PWM period is only supported for
>>     one chip variant so far, for all other chips, a fixed period of
>>     100ms is assumed.
>> 
> 
> This is a good start, but I'll want to see stronger safeguards.
> - Creating a pwmchip entry for a pwm channel must be triggered by
>   device property data, obtained either from devicetree or through
>   DMI or through device properties embedded in ACPI data. For each
>   channel, this must be confirmed by checking that the channel is
>   not associated with a fan control channel.

In my case it's a x86 based industrial PC with direct access.
What safeguard do you suggest in this case? A module parameter?

Also for ACPI data, what exactly do you have in mind?

> - If a pwm channel is configured as pwmchip, it must not be used for fan
>   control, meaning the hwmon properties associated with that channel
>   must not be created.

Ok.
 
> This will retain current functionality while enabling support for using
> pwm channels for purposes other than fan control.
> 
> Thanks,
> Guenter

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Guenter Roeck]

Hi Richard,

On 2/26/26 23:46, Richard Weinberger wrote:
> Hello Guenter,
> 
> ----- Ursprüngliche Mail -----
>> Von: "Guenter Roeck" <linux@roeck-us.net>
>>>    - Exporting a PWM for external use is only allowed when the fan mode
>>>      is set to manual or off.
>>>    - As soon as a PWM is exported, changing its configuration is no
>>>      longer possible through the hwmon sysfs interface, reading is
>>>      still allowed.
>>>    - Changing the PWM period is not supported. IMHO, it is too risky
>>>      since the PWMs usually control system fans and similar components.
>>>    - Reading and decoding the current PWM period is only supported for
>>>      one chip variant so far, for all other chips, a fixed period of
>>>      100ms is assumed.
>>>
>>
>> This is a good start, but I'll want to see stronger safeguards.
>> - Creating a pwmchip entry for a pwm channel must be triggered by
>>    device property data, obtained either from devicetree or through
>>    DMI or through device properties embedded in ACPI data. For each
>>    channel, this must be confirmed by checking that the channel is
>>    not associated with a fan control channel.
> 
> In my case it's a x86 based industrial PC with direct access.
> What safeguard do you suggest in this case? A module parameter?
> 

Presumably it has DMI information or some other means to identify the system.
That information can be used to set device properties, which would then be used
in the probe function to determine if a channel is modeled as pwm channel.
See device_add_software_node() and friends to get an idea how that works.

How exactly those properties would look like needs to be documented in
nuvoton,nct6775.yaml. I'd assume that the pwm channels would be described
in there just like for any other pwm chips.

> Also for ACPI data, what exactly do you have in mind?
> 
ACPI can be used to provide devicetree properties. The information is embedded
in the DSDT table. Conceptually that is identical to devicetree data. That is
not something you need to be concerned about unless you are responsible for that
system and in control of the firmware. Technically the company selling that
industrial PC should provide the information in the DSDT table, but of course
that needs to be standardized first (and then they would have to actually use it).

Hope this helps,

Guenter

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

Hello Richard,

On Thu, Feb 26, 2026 at 07:36:44PM +0100, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Uwe Kleine-König" <ukleinek@kernel.org>
> >> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> >> +	const u8 *wfhw = _wfhw;
> >> +
> >> +	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
> >> +		return 1;
> > 
> > That looks wrong. In principle nct6775_pwm_round_waveform_fromhw()
> > doesn't depend on hardware state. It's supposed to just convert the
> > settings stored in _wfhw to wf. If you know that some things are
> > constant during the lifetime of the PWM and you read those from
> > hardware, return a proper error code, not 1.
> 
> I see. Since the frequency is never changed by the driver we could
> read it while probing and use here the cached value.

I don't care much why the period length is constant. With the idiom from
this patch a comment would be nice, reading once is also fine.

> > Rounding down wf->period_length_ns is fine, so this must be:
> > 
> >	if (wf->period_length_ns < cur_period)
> >		return 1;
> 
> But then the period is no longer fixed and something larger than supported
> can get configured. Smaller values get caught, though.

that's wrong. The period is still fixed.

> e.g.
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# cat period 
> 43243
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 43200 > period 
> bash: echo: write error: Invalid argument
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 50000 > period 
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo $?
> 0

Yes, you can request 50000, but the driver will round that down to
43243. If you have commit aa12c7e70319c9746e55e5b00a215119ba838dad, a
look into /sys/kernel/debug/pwm should confirm that.

(That's a weakness of the sysfs API, you cannot get feedback about how
far the driver has to modify your request to adapt it to the hardware.
Did I mention that you should better use libpwm tools for such tests?
pwm_round will tell you what the driver does with a certain request.)

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

Hello,

On Fri, Feb 27, 2026 at 12:17:00AM -0800, Guenter Roeck wrote:
> Hi Richard,
> 
> On 2/26/26 23:46, Richard Weinberger wrote:
> > Hello Guenter,
> > 
> > ----- Ursprüngliche Mail -----
> > > Von: "Guenter Roeck" <linux@roeck-us.net>
> > > >    - Exporting a PWM for external use is only allowed when the fan mode
> > > >      is set to manual or off.
> > > >    - As soon as a PWM is exported, changing its configuration is no
> > > >      longer possible through the hwmon sysfs interface, reading is
> > > >      still allowed.
> > > >    - Changing the PWM period is not supported. IMHO, it is too risky
> > > >      since the PWMs usually control system fans and similar components.
> > > >    - Reading and decoding the current PWM period is only supported for
> > > >      one chip variant so far, for all other chips, a fixed period of
> > > >      100ms is assumed.
> > > > 
> > > 
> > > This is a good start, but I'll want to see stronger safeguards.
> > > - Creating a pwmchip entry for a pwm channel must be triggered by
> > >    device property data, obtained either from devicetree or through
> > >    DMI or through device properties embedded in ACPI data. For each
> > >    channel, this must be confirmed by checking that the channel is
> > >    not associated with a fan control channel.
> > 
> > In my case it's a x86 based industrial PC with direct access.
> > What safeguard do you suggest in this case? A module parameter?
> > 
> 
> Presumably it has DMI information or some other means to identify the system.
> That information can be used to set device properties, which would then be used
> in the probe function to determine if a channel is modeled as pwm channel.
> See device_add_software_node() and friends to get an idea how that works.
> 
> How exactly those properties would look like needs to be documented in
> nuvoton,nct6775.yaml. I'd assume that the pwm channels would be described
> in there just like for any other pwm chips.
> 
> > Also for ACPI data, what exactly do you have in mind?
> > 
> ACPI can be used to provide devicetree properties. The information is embedded
> in the DSDT table. Conceptually that is identical to devicetree data. That is
> not something you need to be concerned about unless you are responsible for that
> system and in control of the firmware. Technically the company selling that
> industrial PC should provide the information in the DSDT table, but of course
> that needs to be standardized first (and then they would have to actually use it).

That would imply that derRichard has to update the BIOS, or at least
fake some ACPI tables, right?

For me it would be good enough if the first consumer of a channel "wins"
and others get a -EBUSY. Compared to describing that in dt or acpi this
has the advantage that the use can be changed without a reboot.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Guenter Roeck]

On 2/27/26 12:02, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Feb 27, 2026 at 12:17:00AM -0800, Guenter Roeck wrote:
>> Hi Richard,
>>
>> On 2/26/26 23:46, Richard Weinberger wrote:
>>> Hello Guenter,
>>>
>>> ----- Ursprüngliche Mail -----
>>>> Von: "Guenter Roeck" <linux@roeck-us.net>
>>>>>     - Exporting a PWM for external use is only allowed when the fan mode
>>>>>       is set to manual or off.
>>>>>     - As soon as a PWM is exported, changing its configuration is no
>>>>>       longer possible through the hwmon sysfs interface, reading is
>>>>>       still allowed.
>>>>>     - Changing the PWM period is not supported. IMHO, it is too risky
>>>>>       since the PWMs usually control system fans and similar components.
>>>>>     - Reading and decoding the current PWM period is only supported for
>>>>>       one chip variant so far, for all other chips, a fixed period of
>>>>>       100ms is assumed.
>>>>>
>>>>
>>>> This is a good start, but I'll want to see stronger safeguards.
>>>> - Creating a pwmchip entry for a pwm channel must be triggered by
>>>>     device property data, obtained either from devicetree or through
>>>>     DMI or through device properties embedded in ACPI data. For each
>>>>     channel, this must be confirmed by checking that the channel is
>>>>     not associated with a fan control channel.
>>>
>>> In my case it's a x86 based industrial PC with direct access.
>>> What safeguard do you suggest in this case? A module parameter?
>>>
>>
>> Presumably it has DMI information or some other means to identify the system.
>> That information can be used to set device properties, which would then be used
>> in the probe function to determine if a channel is modeled as pwm channel.
>> See device_add_software_node() and friends to get an idea how that works.
>>
>> How exactly those properties would look like needs to be documented in
>> nuvoton,nct6775.yaml. I'd assume that the pwm channels would be described
>> in there just like for any other pwm chips.
>>
>>> Also for ACPI data, what exactly do you have in mind?
>>>
>> ACPI can be used to provide devicetree properties. The information is embedded
>> in the DSDT table. Conceptually that is identical to devicetree data. That is
>> not something you need to be concerned about unless you are responsible for that
>> system and in control of the firmware. Technically the company selling that
>> industrial PC should provide the information in the DSDT table, but of course
>> that needs to be standardized first (and then they would have to actually use it).
> 
> That would imply that derRichard has to update the BIOS, or at least
> fake some ACPI tables, right?
> 
No, I didn't say that. The board manufacturer could do that, but checking
the DMI data in the init function and using software nodes to pass properties
to the driver would do the trick.

> For me it would be good enough if the first consumer of a channel "wins"
> and others get a -EBUSY. Compared to describing that in dt or acpi this
> has the advantage that the use can be changed without a reboot.
> 

No, this is not acceptable. In almost all cases the pwm output will be
connected to a fan, it will be configured by the BIOS, and it may (and
likely will) be configured for DC output instead of pwm output. Changing
that randomly with "first consumer wins" is simply not acceptable.
This will have to be static and well defined.

Guenter