With this change each pwmchip defining the new-style waveform callbacks
can be accessed from userspace via a character device. Compared to the
sysfs-API this is faster (on a stm32mp157 applying a new configuration
takes approx 25% only) and allows to pass the whole configuration in a
single ioctl allowing atomic application.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c       | 304 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  51 +++++++
 3 files changed, 343 insertions(+), 15 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index cec325bdffa5..eed5ba16703d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -23,6 +23,8 @@
 
 #include <dt-bindings/pwm/pwm.h>
 
+#include <uapi/linux/pwm.h>
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/pwm.h>
 
@@ -1960,20 +1962,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL_GPL(pwm_get);
 
-/**
- * pwm_put() - release a PWM device
- * @pwm: PWM device
- */
-void pwm_put(struct pwm_device *pwm)
+static void __pwm_put(struct pwm_device *pwm)
 {
-	struct pwm_chip *chip;
-
-	if (!pwm)
-		return;
-
-	chip = pwm->chip;
-
-	guard(mutex)(&pwm_lock);
+	struct pwm_chip *chip = pwm->chip;
 
 	/*
 	 * Trigger a warning if a consumer called pwm_put() twice.
@@ -1994,6 +1985,20 @@ void pwm_put(struct pwm_device *pwm)
 
 	module_put(chip->owner);
 }
+
+/**
+ * pwm_put() - release a PWM device
+ * @pwm: PWM device
+ */
+void pwm_put(struct pwm_device *pwm)
+{
+	if (!pwm)
+		return;
+
+	guard(mutex)(&pwm_lock);
+
+	__pwm_put(pwm);
+}
 EXPORT_SYMBOL_GPL(pwm_put);
 
 static void devm_pwm_release(void *pwm)
@@ -2063,6 +2068,262 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
 
+struct pwm_cdev_data {
+	struct pwm_chip *chip;
+	struct pwm_device *pwm[];
+};
+
+static int pwm_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pwm_chip *chip = container_of(inode->i_cdev, struct pwm_chip, cdev);
+	struct pwm_cdev_data *cdata;
+
+	guard(mutex)(&pwm_lock);
+
+	if (!chip->operational)
+		return -ENXIO;
+
+	cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
+	if (!cdata)
+		return -ENOMEM;
+
+	cdata->chip = chip;
+
+	file->private_data = cdata;
+
+	return nonseekable_open(inode, file);
+}
+
+static int pwm_cdev_release(struct inode *inode, struct file *file)
+{
+	struct pwm_cdev_data *cdata = file->private_data;
+	unsigned int i;
+
+	for (i = 0; i < cdata->chip->npwm; ++i) {
+		struct pwm_device *pwm = cdata->pwm[i];
+
+		if (pwm) {
+			const char *label = pwm->label;
+
+			pwm_put(cdata->pwm[i]);
+			kfree(label);
+		}
+	}
+	kfree(cdata);
+
+	return 0;
+}
+
+static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (!cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = &chip->pwms[hwpwm];
+		const char *label;
+		int ret;
+
+		label = kasprintf(GFP_KERNEL, "pwm-cdev (pid=%d)", current->pid);
+		if (!label)
+			return -ENOMEM;
+
+		ret = pwm_device_request(pwm, label);
+		if (ret < 0)
+			return ret;
+
+		cdata->pwm[hwpwm] = pwm;
+	}
+
+	return 0;
+}
+
+static int pwm_cdev_free(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = cdata->pwm[hwpwm];
+		const char *label = pwm->label;
+
+		__pwm_put(pwm);
+
+		kfree(label);
+
+		cdata->pwm[hwpwm] = NULL;
+	}
+
+	return 0;
+}
+
+static struct pwm_device *pwm_cdev_get_requested_pwm(struct pwm_cdev_data *cdata,
+						     u32 hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	if (cdata->pwm[hwpwm])
+		return cdata->pwm[hwpwm];
+
+	return ERR_PTR(-EINVAL);
+}
+
+static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	struct pwm_cdev_data *cdata = file->private_data;
+	struct pwm_chip *chip = cdata->chip;
+
+	guard(mutex)(&pwm_lock);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	switch (cmd) {
+	case PWM_IOCTL_REQUEST:
+		{
+			unsigned int hwpwm = arg;
+
+			return pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm = arg;
+
+			return pwm_cdev_free(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_ROUNDWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+
+			if (cwf.__pad != 0)
+				return -EINVAL;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			wf = (struct pwm_waveform) {
+				.period_length_ns = cwf.period_length_ns,
+				.duty_length_ns = cwf.duty_length_ns,
+				.duty_offset_ns = cwf.duty_offset_ns,
+			};
+
+			ret = pwm_round_waveform_might_sleep(pwm, &wf);
+			if (ret < 0)
+				return ret;
+
+			cwf = (struct pwmchip_waveform) {
+				.hwpwm = cwf.hwpwm,
+				.period_length_ns = wf.period_length_ns,
+				.duty_length_ns = wf.duty_length_ns,
+				.duty_offset_ns = wf.duty_offset_ns,
+			};
+
+			return copy_to_user((struct pwmchip_waveform __user *)arg,
+					    &cwf, sizeof(cwf));
+		}
+		break;
+
+	case PWM_IOCTL_GETWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+
+			if (cwf.__pad != 0)
+				return -EINVAL;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			ret = pwm_get_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf.period_length_ns = wf.period_length_ns;
+			cwf.duty_length_ns = wf.duty_length_ns;
+			cwf.duty_offset_ns = wf.duty_offset_ns;
+
+			return copy_to_user((struct pwmchip_waveform __user *)arg,
+					    &cwf, sizeof(cwf));
+		}
+		break;
+
+	case PWM_IOCTL_SETROUNDEDWF:
+	case PWM_IOCTL_SETEXACTWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+
+			if (cwf.__pad != 0)
+				return -EINVAL;
+
+			wf = (struct pwm_waveform){
+				.period_length_ns = cwf.period_length_ns,
+				.duty_length_ns = cwf.duty_length_ns,
+				.duty_offset_ns = cwf.duty_offset_ns,
+			};
+
+			if (!pwm_wf_valid(&wf))
+				return -EINVAL;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			return pwm_set_waveform_might_sleep(pwm, &wf,
+							    cmd == PWM_IOCTL_SETEXACTWF);
+		}
+		break;
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static const struct file_operations pwm_cdev_fileops = {
+	.open = pwm_cdev_open,
+	.release = pwm_cdev_release,
+	.owner = THIS_MODULE,
+	.unlocked_ioctl = pwm_cdev_ioctl,
+};
+
+static dev_t pwm_devt;
+
 /**
  * __pwmchip_add() - register a new PWM chip
  * @chip: the PWM chip to add
@@ -2115,7 +2376,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	scoped_guard(pwmchip, chip)
 		chip->operational = true;
 
-	ret = device_add(&chip->dev);
+	if (chip->id < 256 && chip->ops->write_waveform)
+		chip->dev.devt = MKDEV(MAJOR(pwm_devt), chip->id);
+
+	cdev_init(&chip->cdev, &pwm_cdev_fileops);
+	chip->cdev.owner = owner;
+
+	ret = cdev_device_add(&chip->cdev, &chip->dev);
 	if (ret)
 		goto err_device_add;
 
@@ -2166,7 +2433,7 @@ void pwmchip_remove(struct pwm_chip *chip)
 		idr_remove(&pwm_chips, chip->id);
 	}
 
-	device_del(&chip->dev);
+	cdev_device_del(&chip->cdev, &chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
@@ -2310,9 +2577,16 @@ static int __init pwm_init(void)
 {
 	int ret;
 
+	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
+	if (ret) {
+		pr_warn("Failed to initialize chrdev region for PWM usage\n");
+		return ret;
+	}
+
 	ret = class_register(&pwm_class);
 	if (ret) {
 		pr_err("Failed to initialize PWM class (%pe)\n", ERR_PTR(ret));
+		unregister_chrdev_region(pwm_devt, 256);
 		return ret;
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index bf0469b2201d..d8817afe95dc 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/module.h>
@@ -309,6 +310,7 @@ struct pwm_ops {
 /**
  * struct pwm_chip - abstract a PWM controller
  * @dev: device providing the PWMs
+ * @cdev: &struct cdev for this device
  * @ops: callbacks for this PWM controller
  * @owner: module providing this chip
  * @id: unique number of this PWM chip
@@ -323,6 +325,7 @@ struct pwm_ops {
  */
 struct pwm_chip {
 	struct device dev;
+	struct cdev cdev;
 	const struct pwm_ops *ops;
 	struct module *owner;
 	unsigned int id;
diff --git a/include/uapi/linux/pwm.h b/include/uapi/linux/pwm.h
new file mode 100644
index 000000000000..3d2c3cefc090
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+
+#ifndef _UAPI_PWM_H_
+#define _UAPI_PWM_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/**
+ * struct pwmchip_waveform - Describe a PWM waveform for a pwm_chip's PWM channel
+ * @hwpwm: per-chip relative index of the PWM device
+ * @__pad: padding, must be zero
+ * @period_length_ns: duration of the repeating period.
+ *    A value of 0 represents a disabled PWM.
+ * @duty_length_ns: duration of the active part in each period
+ * @duty_offset_ns: offset of the rising edge from a period's start
+ */
+struct pwmchip_waveform {
+	__u32 hwpwm;
+	__u32 __pad;
+	__u64 period_length_ns;
+	__u64 duty_length_ns;
+	__u64 duty_offset_ns;
+};
+
+/* Reserves the passed hwpwm for exclusive control. */
+#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
+
+/* counter part to PWM_IOCTL_REQUEST */
+#define PWM_IOCTL_FREE		_IO(0x75, 2)
+
+/*
+ * Modifies the passed wf according to hardware constraints. All parameters are
+ * rounded down to the next possible value, unless there is no such value, then
+ * values are rounded up.
+ */
+#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
+
+/* Get the currently implemented waveform */
+#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
+
+/* Like PWM_IOCTL_GETWF + PWM_IOCTL_SETROUNDEDWF in one go. */
+#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
+
+/*
+ * Program the PWM to emit exactly the passed waveform, subject only to rounding
+ * down each value less than 1 ns.
+ */
+#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
+
+#endif /* _UAPI_PWM_H_ */
-- 
2.47.2

On 4/8/25 9:23 AM, Uwe Kleine-König wrote:
> With this change each pwmchip defining the new-style waveform callbacks
> can be accessed from userspace via a character device. Compared to the
> sysfs-API this is faster (on a stm32mp157 applying a new configuration
> takes approx 25% only) and allows to pass the whole configuration in a
> single ioctl allowing atomic application.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

...

> +static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
> +{
> +	struct pwm_chip *chip = cdata->chip;
> +
> +	if (hwpwm >= chip->npwm)
> +		return -EINVAL;
> +
> +	if (!cdata->pwm[hwpwm]) {
> +		struct pwm_device *pwm = &chip->pwms[hwpwm];
> +		const char *label;
> +		int ret;
> +
> +		label = kasprintf(GFP_KERNEL, "pwm-cdev (pid=%d)", current->pid);
> +		if (!label)
> +			return -ENOMEM;
> +
> +		ret = pwm_device_request(pwm, label);
> +		if (ret < 0)

Should kfree(label) before error return?

> +			return ret;
> +
> +		cdata->pwm[hwpwm] = pwm;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	int ret = 0;
> +	struct pwm_cdev_data *cdata = file->private_data;
> +	struct pwm_chip *chip = cdata->chip;
> +
> +	guard(mutex)(&pwm_lock);
> +
> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case PWM_IOCTL_REQUEST:
> +		{
> +			unsigned int hwpwm = arg;
> +
> +			return pwm_cdev_request(cdata, hwpwm);
> +		}
> +		break;

Unreachable code? Should be able to removed all of the breaks without any
compiler complaining - otherwise it would already be complaining about no
return at the end of the funtion where the break jumps to.

> +
> +	case PWM_IOCTL_FREE:
> +		{
> +			unsigned int hwpwm = arg;
> +
> +			return pwm_cdev_free(cdata, hwpwm);
> +		}
> +		break;
> +
> +	case PWM_IOCTL_ROUNDWF:
> +		{
> +			struct pwmchip_waveform cwf;
> +			struct pwm_waveform wf;
> +			struct pwm_device *pwm;
> +
> +			ret = copy_from_user(&cwf,
> +					     (struct pwmchip_waveform __user *)arg,
> +					     sizeof(cwf));
> +			if (ret)
> +				return -EFAULT;
> +
> +			if (cwf.__pad != 0)
> +				return -EINVAL;
> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			wf = (struct pwm_waveform) {
> +				.period_length_ns = cwf.period_length_ns,
> +				.duty_length_ns = cwf.duty_length_ns,
> +				.duty_offset_ns = cwf.duty_offset_ns,
> +			};
> +
> +			ret = pwm_round_waveform_might_sleep(pwm, &wf);
> +			if (ret < 0)
> +				return ret;
> +
> +			cwf = (struct pwmchip_waveform) {
> +				.hwpwm = cwf.hwpwm,
> +				.period_length_ns = wf.period_length_ns,
> +				.duty_length_ns = wf.duty_length_ns,
> +				.duty_offset_ns = wf.duty_offset_ns,
> +			};
> +
> +			return copy_to_user((struct pwmchip_waveform __user *)arg,
> +					    &cwf, sizeof(cwf));
> +		}
> +		break;
> +
> +	case PWM_IOCTL_GETWF:
> +		{
> +			struct pwmchip_waveform cwf;
> +			struct pwm_waveform wf;
> +			struct pwm_device *pwm;
> +
> +			ret = copy_from_user(&cwf,
> +					     (struct pwmchip_waveform __user *)arg,
> +					     sizeof(cwf));
> +			if (ret)
> +				return -EFAULT;
> +
> +			if (cwf.__pad != 0)
> +				return -EINVAL;

Since this is get-only (argument is purly output), should we not check this
to allow userspace to be able to pass an unintialized struct without error?

> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			ret = pwm_get_waveform_might_sleep(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf.period_length_ns = wf.period_length_ns;
> +			cwf.duty_length_ns = wf.duty_length_ns;
> +			cwf.duty_offset_ns = wf.duty_offset_ns;

Odd to use different style for setting struct here compared to the other cases.
(I prefer this one since it is less lines of code to read and less indent.)

> +
> +			return copy_to_user((struct pwmchip_waveform __user *)arg,
> +					    &cwf, sizeof(cwf));
> +		}
> +		break;
> +
> +	case PWM_IOCTL_SETROUNDEDWF:
> +	case PWM_IOCTL_SETEXACTWF:
> +		{
> +			struct pwmchip_waveform cwf;
> +			struct pwm_waveform wf;
> +			struct pwm_device *pwm;
> +
> +			ret = copy_from_user(&cwf,
> +					     (struct pwmchip_waveform __user *)arg,
> +					     sizeof(cwf));
> +			if (ret)
> +				return -EFAULT;
> +
> +			if (cwf.__pad != 0)
> +				return -EINVAL;
> +
> +			wf = (struct pwm_waveform){
> +				.period_length_ns = cwf.period_length_ns,
> +				.duty_length_ns = cwf.duty_length_ns,
> +				.duty_offset_ns = cwf.duty_offset_ns,
> +			};
> +
> +			if (!pwm_wf_valid(&wf))
> +				return -EINVAL;
> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			return pwm_set_waveform_might_sleep(pwm, &wf,
> +							    cmd == PWM_IOCTL_SETEXACTWF);

For PWM_IOCTL_SETROUNDEDWF case, should we be copying the modifed waveform back
to userspace so that it can know what rounding what actually applied without having
to call PWM_IOCTL_GETWF?

> +		}
> +		break;> +
> +	default:
> +		return -ENOTTY;
> +	}
> +}
> +
> +static const struct file_operations pwm_cdev_fileops = {
> +	.open = pwm_cdev_open,
> +	.release = pwm_cdev_release,
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = pwm_cdev_ioctl,
> +};
> +
> +static dev_t pwm_devt;
> +
>  /**
>   * __pwmchip_add() - register a new PWM chip
>   * @chip: the PWM chip to add
> @@ -2115,7 +2376,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	scoped_guard(pwmchip, chip)
>  		chip->operational = true;
>  
> -	ret = device_add(&chip->dev);
> +	if (chip->id < 256 && chip->ops->write_waveform)
> +		chip->dev.devt = MKDEV(MAJOR(pwm_devt), chip->id);

if (chip->id >= 256 && chip->ops->write_waveform)
dev_warn("too many PWM devices, chardev will not be created for ...") ?

> +
> +	cdev_init(&chip->cdev, &pwm_cdev_fileops);
> +	chip->cdev.owner = owner;
> +
> +	ret = cdev_device_add(&chip->cdev, &chip->dev);
>  	if (ret)
>  		goto err_device_add;
>  
> @@ -2166,7 +2433,7 @@ void pwmchip_remove(struct pwm_chip *chip)
>  		idr_remove(&pwm_chips, chip->id);
>  	}
>  
> -	device_del(&chip->dev);
> +	cdev_device_del(&chip->cdev, &chip->dev);
>  }
>  EXPORT_SYMBOL_GPL(pwmchip_remove);
>  
> @@ -2310,9 +2577,16 @@ static int __init pwm_init(void)
>  {
>  	int ret;
>  
> +	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
> +	if (ret) {
> +		pr_warn("Failed to initialize chrdev region for PWM usage\n");

Why warn and not err?

> +		return ret;
> +	}
> +
>  	ret = class_register(&pwm_class);
>  	if (ret) {
>  		pr_err("Failed to initialize PWM class (%pe)\n", ERR_PTR(ret));
> +		unregister_chrdev_region(pwm_devt, 256);
>  		return ret;
>  	}
>  
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index bf0469b2201d..d8817afe95dc 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -2,6 +2,7 @@
>  #ifndef __LINUX_PWM_H
>  #define __LINUX_PWM_H
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
> @@ -309,6 +310,7 @@ struct pwm_ops {
>  /**
>   * struct pwm_chip - abstract a PWM controller
>   * @dev: device providing the PWMs
> + * @cdev: &struct cdev for this device
>   * @ops: callbacks for this PWM controller
>   * @owner: module providing this chip
>   * @id: unique number of this PWM chip
> @@ -323,6 +325,7 @@ struct pwm_ops {
>   */
>  struct pwm_chip {
>  	struct device dev;
> +	struct cdev cdev;
>  	const struct pwm_ops *ops;
>  	struct module *owner;
>  	unsigned int id;
> diff --git a/include/uapi/linux/pwm.h b/include/uapi/linux/pwm.h
> new file mode 100644
> index 000000000000..3d2c3cefc090
> --- /dev/null
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +
> +#ifndef _UAPI_PWM_H_
> +#define _UAPI_PWM_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct pwmchip_waveform - Describe a PWM waveform for a pwm_chip's PWM channel
> + * @hwpwm: per-chip relative index of the PWM device
> + * @__pad: padding, must be zero
> + * @period_length_ns: duration of the repeating period.
> + *    A value of 0 represents a disabled PWM.
> + * @duty_length_ns: duration of the active part in each period
> + * @duty_offset_ns: offset of the rising edge from a period's start
> + */
> +struct pwmchip_waveform {
> +	__u32 hwpwm;
> +	__u32 __pad;
> +	__u64 period_length_ns;
> +	__u64 duty_length_ns;
> +	__u64 duty_offset_ns;
> +};
> +
> +/* Reserves the passed hwpwm for exclusive control. */
> +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> +
> +/* counter part to PWM_IOCTL_REQUEST */
> +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> +
> +/*
> + * Modifies the passed wf according to hardware constraints. All parameters are
> + * rounded down to the next possible value, unless there is no such value, then

Technically, isn't 0 a possible value (at least for duty length/offset)?

So maybe more clear to say that if the requested value is non-zero then the
value will be rounded down unless the result would be zero in which case
the resulting value will the be smallest possible non-zero value.

> + * values are rounded up.
> + */
> +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> +
> +/* Get the currently implemented waveform */
> +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> +
> +/* Like PWM_IOCTL_GETWF + PWM_IOCTL_SETROUNDEDWF in one go. */

Is this supposed to say "Like PWM_IOCTL_ROUNDWF + PWM_IOCTL_SETEXACTWF in one go"?

> +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> +
> +/*
> + * Program the PWM to emit exactly the passed waveform, subject only to rounding
> + * down each value less than 1 ns.

Otherwise returns and error? What error codes could we expect?

> + */
> +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> +
> +#endif /* _UAPI_PWM_H_ */

Hello David,

first of all thanks for your time and valuable feedback!

On Tue, Apr 08, 2025 at 11:20:19AM -0500, David Lechner wrote:
> On 4/8/25 9:23 AM, Uwe Kleine-König wrote:
> > +	case PWM_IOCTL_GETWF:
> > +		{
> > +			struct pwmchip_waveform cwf;
> > +			struct pwm_waveform wf;
> > +			struct pwm_device *pwm;
> > +
> > +			ret = copy_from_user(&cwf,
> > +					     (struct pwmchip_waveform __user *)arg,
> > +					     sizeof(cwf));
> > +			if (ret)
> > +				return -EFAULT;
> > +
> > +			if (cwf.__pad != 0)
> > +				return -EINVAL;
> 
> Since this is get-only (argument is purly output), should we not check this
> to allow userspace to be able to pass an unintialized struct without error?

No, cwf.hwpwm is an input. So I think it's reasonable to assume cwf is
properly initialized.

> > +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> > +			if (IS_ERR(pwm))
> > +				return PTR_ERR(pwm);
> > +
> > +			ret = pwm_get_waveform_might_sleep(pwm, &wf);
> > +			if (ret)
> > +				return ret;
> > +
> > +			cwf.period_length_ns = wf.period_length_ns;
> > +			cwf.duty_length_ns = wf.duty_length_ns;
> > +			cwf.duty_offset_ns = wf.duty_offset_ns;
> 
> Odd to use different style for setting struct here compared to the other cases.
> (I prefer this one since it is less lines of code to read and less indent.)

Note there is a semantic difference:

	cwf = (struct pwmchip_waveform) {
		.period_length_ns = wf.period_length_ns,
		.duty_length_ns = wf.duty_length_ns,
		.duty_offset_ns = wf.duty_offset_ns,
	};

initializes all unspecified members (here e.g. hwpwm) to zero. I used
that idiom for ROUNDWF ioctl with

		.hwpwm = cwf.hwpwm,

I guess I'll update to that variant here, too.

> > +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> > +			if (IS_ERR(pwm))
> > +				return PTR_ERR(pwm);
> > +
> > +			return pwm_set_waveform_might_sleep(pwm, &wf,
> > +							    cmd == PWM_IOCTL_SETEXACTWF);
> 
> For PWM_IOCTL_SETROUNDEDWF case, should we be copying the modifed waveform back
> to userspace so that it can know what rounding what actually applied without having
> to call PWM_IOCTL_GETWF?

Hmm, for pwm_set_waveform_might_sleep() and also pwm_apply_might_sleep()
the argument isn't modified. So while this might save an ioctl for
GETWF, but you might have to rewrite your state instead.

> > @@ -2115,7 +2376,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
> >  	scoped_guard(pwmchip, chip)
> >  		chip->operational = true;
> >  
> > -	ret = device_add(&chip->dev);
> > +	if (chip->id < 256 && chip->ops->write_waveform)
> > +		chip->dev.devt = MKDEV(MAJOR(pwm_devt), chip->id);
> 
> if (chip->id >= 256 && chip->ops->write_waveform)
> dev_warn("too many PWM devices, chardev will not be created for ...") ?

I would be surprised to hit that, but I guess it's wise to do that
before it happens for the first time.

> > +/*
> > + * Modifies the passed wf according to hardware constraints. All parameters are
> > + * rounded down to the next possible value, unless there is no such value, then
> 
> Technically, isn't 0 a possible value (at least for duty length/offset)?

Yes, but not all hardware's support duty_length == 0 or duty_offset ==
0. For those that do, it's expected that 1 is rounded down to 0 (unless
they support 1, too). period_length isn't supposed to be round down to
0.

> So maybe more clear to say that if the requested value is non-zero then the
> value will be rounded down unless the result would be zero in which case
> the resulting value will the be smallest possible non-zero value.

Yes, this applies only to period however.

All your remarks that I removed will be addressed in the next revision.

Best regards
Uwe