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       | 291 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  32 +++++
 3 files changed, 311 insertions(+), 15 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ed620e35db61..3c25e0ac682c 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>
 
@@ -1915,20 +1917,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.
@@ -1949,6 +1940,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)
@@ -2018,6 +2023,249 @@ 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) {
+		if (cdata->pwm[i])
+			pwm_put(cdata->pwm[i]);
+	}
+	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];
+		int ret;
+
+		ret = pwm_device_request(pwm, "pwm-cdev");
+		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];
+
+		__pwm_put(pwm);
+
+		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)
+				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,
+	.llseek = no_llseek,
+	.unlocked_ioctl = pwm_cdev_ioctl,
+};
+
+static dev_t pwm_devt;
+
 /**
  * __pwmchip_add() - register a new PWM chip
  * @chip: the PWM chip to add
@@ -2070,7 +2318,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;
 
@@ -2121,7 +2375,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);
 
@@ -2262,9 +2516,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 c3d9ddeafa65..f43dbfaef1a1 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>
@@ -304,6 +305,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
@@ -318,6 +320,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..0a9cdaa0fe37
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,32 @@
+/* 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
+ * @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;
+};
+
+#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
+#define PWM_IOCTL_FREE		_IO(0x75, 2)
+#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
+#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
+#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
+#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
+
+#endif /* _UAPI_PWM_H_ */
-- 
2.45.2

On Fri, Sep 20, 2024 at 10:58:04AM +0200, 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>
> ---
>  drivers/pwm/core.c       | 291 +++++++++++++++++++++++++++++++++++++--
>  include/linux/pwm.h      |   3 +
>  include/uapi/linux/pwm.h |  32 +++++
>  3 files changed, 311 insertions(+), 15 deletions(-)
>  create mode 100644 include/uapi/linux/pwm.h
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index ed620e35db61..3c25e0ac682c 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>
>
> @@ -1915,20 +1917,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.
> @@ -1949,6 +1940,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)
> @@ -2018,6 +2023,249 @@ 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) {
> +		if (cdata->pwm[i])
> +			pwm_put(cdata->pwm[i]);
> +	}
> +	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];
> +		int ret;
> +
> +		ret = pwm_device_request(pwm, "pwm-cdev");
> +		if (ret < 0)
> +			return ret;
> +
> +		cdata->pwm[hwpwm] = pwm;
> +	}
> +
> +	return 0;
> +}
> +

Allow the user to specify the consumer label as part of the request rather
than hard coding it to "pwm-cdev"?

> +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];
> +
> +		__pwm_put(pwm);
> +
> +		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);
> +

Coarse grain locking of the whole of pwm for the duration, where some
calls my sleep, feels excessive. Is it really necessary for all of the
ioctls?

> +	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)
> +				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,
> +	.llseek = no_llseek,
> +	.unlocked_ioctl = pwm_cdev_ioctl,
> +};
> +
> +static dev_t pwm_devt;
> +
>  /**
>   * __pwmchip_add() - register a new PWM chip
>   * @chip: the PWM chip to add
> @@ -2070,7 +2318,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;
>
> @@ -2121,7 +2375,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);
>
> @@ -2262,9 +2516,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 c3d9ddeafa65..f43dbfaef1a1 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>
> @@ -304,6 +305,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
> @@ -318,6 +320,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..0a9cdaa0fe37
> --- /dev/null
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,32 @@
> +/* 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
> + * @duty_length_ns: duration of the active part in each period
> + * @duty_offset_ns: offset of the rising edge from a period's start
> + */

While you have added some documentation, this is still lacking compared
to the corresponding in include/linux/pwm.h.  e.g. zeroing
period_length_ns to disable a PWM...

> +struct pwmchip_waveform {
> +	__u32 hwpwm;
> +	__u32 __pad;
> +	__u64 period_length_ns;
> +	__u64 duty_length_ns;
> +	__u64 duty_offset_ns;
> +};
> +
> +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> +

A brief description of the ioctls and their semantics would be handy,
either here or as full blown documentation in
linux/Documentation/userspace-api/pwm/...

PWMs are automatically released when the pwmchip file is closed?
And the state of the PWM line after release (or _FREE) is indeterminate?

Is it possible that the underlying PWM chip be removed while the user has
the pwmchip open and/or has pwm devices requested?

Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
npwms, module name.  For pwm devices the label, if the PWM is requested and
the consumer's label (similar to the GPIO chipinfo and lineinfo)?
Unless that information otherwise exposed to userspace?

Cheers,
Kent.

Hello Kent,

On Sun, Sep 29, 2024 at 12:48:28PM +0800, Kent Gibson wrote:
> On Fri, Sep 20, 2024 at 10:58:04AM +0200, Uwe Kleine-König wrote:
> > +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];
> > +		int ret;
> > +
> > +		ret = pwm_device_request(pwm, "pwm-cdev");
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		cdata->pwm[hwpwm] = pwm;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> Allow the user to specify the consumer label as part of the request rather
> than hard coding it to "pwm-cdev"?

I considered using the process name, or pwm-cdev:$(pidof
userspace-program). And I'd like to have a possibility to specify names
in dt (that then could be used to lookup a PWM, too). I think these two
are better than having userspace provide a name. What do you think?

> > +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];
> > +
> > +		__pwm_put(pwm);
> > +
> > +		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);
> > +
> 
> Coarse grain locking of the whole of pwm for the duration, where some
> calls my sleep, feels excessive. Is it really necessary for all of the
> ioctls?

This might be improvable a bit indeed. I think we won't come around
holding the pwmchip lock, but that would already be nice. I'll invest
some brain cycles here.
 
> > +/**
> > + * 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
> > + * @duty_length_ns: duration of the active part in each period
> > + * @duty_offset_ns: offset of the rising edge from a period's start
> > + */
> 
> While you have added some documentation, this is still lacking compared
> to the corresponding in include/linux/pwm.h.  e.g. zeroing
> period_length_ns to disable a PWM...

Ack.

> > +struct pwmchip_waveform {
> > +	__u32 hwpwm;
> > +	__u32 __pad;
> > +	__u64 period_length_ns;
> > +	__u64 duty_length_ns;
> > +	__u64 duty_offset_ns;
> > +};
> > +
> > +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> > +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> > +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> > +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> > +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> > +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> > +
> 
> A brief description of the ioctls and their semantics would be handy,
> either here or as full blown documentation in
> linux/Documentation/userspace-api/pwm/...

Ack.
 
> PWMs are automatically released when the pwmchip file is closed?
> And the state of the PWM line after release (or _FREE) is indeterminate?
> 
> Is it possible that the underlying PWM chip be removed while the user has
> the pwmchip open and/or has pwm devices requested?

I guess these questions are hints about what to describe in the docs to
be written and not actual questions. I fully agree that the
documentation front isn't optimal yet.

> Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
> npwms, module name.  For pwm devices the label, if the PWM is requested and
> the consumer's label (similar to the GPIO chipinfo and lineinfo)?
> Unless that information otherwise exposed to userspace?

Most of that is already in /sys/class/pwm. Duplicating that feels wrong.

Thanks a lot for your feedback
Uwe

On Mon, Sep 30, 2024 at 08:01:45AM +0200, Uwe Kleine-König wrote:
> Hello Kent,
>
> On Sun, Sep 29, 2024 at 12:48:28PM +0800, Kent Gibson wrote:
> > On Fri, Sep 20, 2024 at 10:58:04AM +0200, Uwe Kleine-König wrote:
> > > +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];
> > > +		int ret;
> > > +
> > > +		ret = pwm_device_request(pwm, "pwm-cdev");
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		cdata->pwm[hwpwm] = pwm;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> >
> > Allow the user to specify the consumer label as part of the request rather
> > than hard coding it to "pwm-cdev"?
>
> I considered using the process name, or pwm-cdev:$(pidof
> userspace-program). And I'd like to have a possibility to specify names
> in dt (that then could be used to lookup a PWM, too). I think these two
> are better than having userspace provide a name. What do you think?
>

Are we talking the PWM device or the consumer?  Or both?

Wrt the consumer, For GPIO we don't provide a useful default, though perhaps
we should. Instead we rely on the userspace library to provide one - if the
user hasn't.

I'd go with the pid option - that is more unique than the process name.

It could still be useful for the user to be able to provide a label,
especially if you are going to allow arbitrary labels via dt.  Though as
often as not the user doesn't know what name to provide and fills it with
something useless, so I don't think there is any clear winner here.

> > > +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];
> > > +
> > > +		__pwm_put(pwm);
> > > +
> > > +		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);
> > > +
> >
> > Coarse grain locking of the whole of pwm for the duration, where some
> > calls my sleep, feels excessive. Is it really necessary for all of the
> > ioctls?
>
> This might be improvable a bit indeed. I think we won't come around
> holding the pwmchip lock, but that would already be nice. I'll invest
> some brain cycles here.
>

That can always be a later refinement - better too much locking than
too little.

> > > +/**
> > > + * 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
> > > + * @duty_length_ns: duration of the active part in each period
> > > + * @duty_offset_ns: offset of the rising edge from a period's start
> > > + */
> >
> > While you have added some documentation, this is still lacking compared
> > to the corresponding in include/linux/pwm.h.  e.g. zeroing
> > period_length_ns to disable a PWM...
>
> Ack.
>
> > > +struct pwmchip_waveform {
> > > +	__u32 hwpwm;
> > > +	__u32 __pad;
> > > +	__u64 period_length_ns;
> > > +	__u64 duty_length_ns;
> > > +	__u64 duty_offset_ns;
> > > +};
> > > +
> > > +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> > > +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> > > +#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
> > > +#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
> > > +#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
> > > +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> > > +
> >
> > A brief description of the ioctls and their semantics would be handy,
> > either here or as full blown documentation in
> > linux/Documentation/userspace-api/pwm/...
>
> Ack.
>
> > PWMs are automatically released when the pwmchip file is closed?
> > And the state of the PWM line after release (or _FREE) is indeterminate?
> >
> > Is it possible that the underlying PWM chip be removed while the user has
> > the pwmchip open and/or has pwm devices requested?
>
> I guess these questions are hints about what to describe in the docs to
> be written and not actual questions. I fully agree that the
> documentation front isn't optimal yet.
>

Yup, and as per the locking, the documentation can always be a later
refinement - so long as your intent is clear now before the ioctls get
chiselled in stone.

> > Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
> > npwms, module name.  For pwm devices the label, if the PWM is requested and
> > the consumer's label (similar to the GPIO chipinfo and lineinfo)?
> > Unless that information otherwise exposed to userspace?
>
> Most of that is already in /sys/class/pwm. Duplicating that feels wrong.
>

Depends on whether you want the interface to be standalone or an
addition to the sysfs - and that is your call.

> Thanks a lot for your feedback

No problem, and again sorry for not getting around to it earlier.

Cheers,
Kent.