diff mbox series

[v4,4/7] pwm: Add support for pwmchip devices for faster and easier userspace access

Message ID 00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com
State New
Headers show
Series pwm: New abstraction and userspace API | expand

Commit Message

Uwe Kleine-König Sept. 6, 2024, 3:43 p.m. UTC 
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       | 267 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  24 ++++
 3 files changed, 292 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

Comments

David Lechner Sept. 6, 2024, 10:26 p.m. UTC | #1 
On 9/6/24 10:43 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>
> ---
>  drivers/pwm/core.c       | 267 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/pwm.h      |   3 +
>  include/uapi/linux/pwm.h |  24 ++++
>  3 files changed, 292 insertions(+), 2 deletions(-)
>  create mode 100644 include/uapi/linux/pwm.h
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c7f39f9f4bcf..16615a4673ef 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>
>  
> @@ -1594,6 +1596,254 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
>  	return true;
>  }
>  
> +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;
> +}

Maybe a potential problem here with multiple requests.

Suppose an applications does:

// main thread
fd = open("/dev/pwm0", ...);

// start some threads

// thread A

ioctl(fd, PWM_IOCTL_REQUEST, 0);
// in kernel, pwm_device_request() is called and
// cdata->pwm[0] is assigned

// does some stuff - OK

	// thread B

	ioctl(fd, PWM_IOCTL_REQUEST, 0);
	// succeeds since cdata->pwm[0] is assigned

	// does some stuff - messes up thread A

// does some stuff - messes up thread B

	// cleans up after itself
	ioctl(fd, PWM_IOCTL_FREE, 0);
	// pwm_put() is called and
	// cdata->pwm[0] is set to NULL
	
// does some stuff - kernel has to call pwm_device_request()
// again, which may fail? e.g. if sysfs stole it in the meantime

// cleans up after itself
ioctl(fd, PWM_IOCTL_FREE, 0);

Maybe we should be more strict and only allow one requester at a time?

Or is it a documentation problem that this isn't the intended way to
use these ioctls?

Or maybe we just don't need the REQUEST and FREE ioctls?

To me, the most intuitive is that REQUEST would gain exclusive
access until FREE is called and other ioctls should fail if
you don't have exclusive access.

> +
> +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;
> +
> +			ret = get_user(hwpwm, (unsigned int __user *)arg);
> +			if (ret)
> +				return ret;
> +
> +			return pwm_cdev_request(cdata, hwpwm);
> +		}
> +		break;
> +
> +	case PWM_IOCTL_FREE:
> +		{
> +			unsigned int hwpwm;
> +
> +			ret = get_user(hwpwm, (unsigned int __user *)arg);
> +			if (ret)
> +				return ret;
> +
> +			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;
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			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) {
> +				pr_warn("huh\n");

This is what I will say when I see this in the kernel log. :-p

> +				return -EINVAL;
> +			}
> +
> +			ret = pwm_cdev_request(cdata, cwf.hwpwm);
> +			if (ret)
> +				return ret;
> +
> +			pwm = cdata->pwm[cwf.hwpwm];
> +
> +			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;
Kent Gibson Sept. 7, 2024, 1:58 a.m. UTC | #2 
On Fri, Sep 06, 2024 at 05:43:00PM +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>
> ---

> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,24 @@
> +/* 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 {
> +	__u32 hwpwm;
> +	__u32 __pad; /* padding, must be zero */
> +	__u64 period_length_ns;
> +	__u64 duty_length_ns;
> +	__u64 duty_offset_ns;
> +};
> +

Some documentation?

> +#define PWM_IOCTL_REQUEST	_IOW(0x75, 1, unsigned int)
> +#define PWM_IOCTL_FREE		_IOW(0x75, 2, unsigned int)
> +#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
>
Uwe Kleine-König Sept. 8, 2024, 2:59 p.m. UTC | #3 
Hello David,

On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
> Maybe a potential problem here with multiple requests.
> 
> Suppose an applications does:
> 
> // main thread
> fd = open("/dev/pwm0", ...);
> 
> // start some threads
> 
> // thread A
> 
> ioctl(fd, PWM_IOCTL_REQUEST, 0);
> // in kernel, pwm_device_request() is called and
> // cdata->pwm[0] is assigned
> 
> // does some stuff - OK
> 
> 	// thread B
> 
> 	ioctl(fd, PWM_IOCTL_REQUEST, 0);
> 	// succeeds since cdata->pwm[0] is assigned
> 
> 	// does some stuff - messes up thread A
> 
> // does some stuff - messes up thread B

If two threads share a single fd for a given pwmchip char device, it's
in the responsibility of the application to not shoot in its own foot.
There are similar problems if two threads write to the same fd
corresponding to an ordinary file or directory. I think behaving
differently here isn't a good idea.
 
> 	// cleans up after itself
> 	ioctl(fd, PWM_IOCTL_FREE, 0);
> 	// pwm_put() is called and
> 	// cdata->pwm[0] is set to NULL
> 	
> // does some stuff - kernel has to call pwm_device_request()
> // again, which may fail? e.g. if sysfs stole it in the meantime
> 
> // cleans up after itself
> ioctl(fd, PWM_IOCTL_FREE, 0);
> 
> Maybe we should be more strict and only allow one requester at a time?

From the POV of the kernel code, there is only one requestor, identified
by the opened file. Handling that in a different way isn't a good idea I
think.

> Or maybe we just don't need the REQUEST and FREE ioctls?

The idea of the REQUEST ioctl is that an application can make sure it
can access all PWMs it needs before starting to change the
configuration.

> > +			if (cwf.__pad != 0) {
> > +				pr_warn("huh\n");
> 
> This is what I will say when I see this in the kernel log. :-p

So the message seems to be chosen correctly :-)

Of course will drop that, thanks for finding that.

Best regards
Uwe
David Lechner Sept. 9, 2024, 8:53 p.m. UTC | #4 
On 9/8/24 9:59 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
>> Maybe a potential problem here with multiple requests.
>>
>> Suppose an applications does:
>>
>> // main thread
>> fd = open("/dev/pwm0", ...);
>>
>> // start some threads
>>
>> // thread A
>>
>> ioctl(fd, PWM_IOCTL_REQUEST, 0);
>> // in kernel, pwm_device_request() is called and
>> // cdata->pwm[0] is assigned
>>
>> // does some stuff - OK
>>
>> 	// thread B
>>
>> 	ioctl(fd, PWM_IOCTL_REQUEST, 0);
>> 	// succeeds since cdata->pwm[0] is assigned
>>
>> 	// does some stuff - messes up thread A
>>
>> // does some stuff - messes up thread B
> 
> If two threads share a single fd for a given pwmchip char device, it's
> in the responsibility of the application to not shoot in its own foot.
> There are similar problems if two threads write to the same fd
> corresponding to an ordinary file or directory. I think behaving
> differently here isn't a good idea.

Yes, applications should absolutely not be doing what I did in this
bad example. :-) So, that is why it would make more sense to me if a
second call of the REQUEST ioctl using the same fd would return an
error instead of succeeding without actually doing anything.

>  
>> 	// cleans up after itself
>> 	ioctl(fd, PWM_IOCTL_FREE, 0);
>> 	// pwm_put() is called and
>> 	// cdata->pwm[0] is set to NULL
>> 	
>> // does some stuff - kernel has to call pwm_device_request()
>> // again, which may fail? e.g. if sysfs stole it in the meantime
>>
>> // cleans up after itself
>> ioctl(fd, PWM_IOCTL_FREE, 0);
>>
>> Maybe we should be more strict and only allow one requester at a time?
> 
> From the POV of the kernel code, there is only one requestor, identified
> by the opened file. Handling that in a different way isn't a good idea I
> think.
> 
>> Or maybe we just don't need the REQUEST and FREE ioctls?
> 
> The idea of the REQUEST ioctl is that an application can make sure it
> can access all PWMs it needs before starting to change the
> configuration.
> 
Ah, I had not considered that case.

But if it is required in some cases, I feel like it would be better
to just make it required in all cases. Otherwise, it feels like there
are too many ways to do the same thing if all of the other GET/SET
ioctls implicitly call the equivalent of REQUEST if it was not
explicitly called. It is one less decision to make for me as a user
if there is only one "right" way to use this interface.
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c7f39f9f4bcf..16615a4673ef 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>
 
@@ -1594,6 +1596,254 @@  static bool pwm_ops_check(const struct pwm_chip *chip)
 	return true;
 }
 
+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 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;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				return ret;
+
+			return pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				return ret;
+
+			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;
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			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) {
+				pr_warn("huh\n");
+				return -EINVAL;
+			}
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			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) {
+				pr_warn("huh\n");
+				return -EINVAL;
+			}
+
+			if (cwf.period_length_ns > 0 &&
+			    (cwf.duty_length_ns > cwf.period_length_ns ||
+			     cwf.duty_offset_ns >= cwf.period_length_ns))
+				return -EINVAL;
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			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,
+			};
+
+			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
@@ -1646,7 +1896,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;
 
@@ -1697,7 +1953,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);
 
@@ -2241,9 +2497,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 40cef0bc0de7..b12ca9531e32 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..ed127a80afbf
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,24 @@ 
+/* 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 {
+	__u32 hwpwm;
+	__u32 __pad; /* padding, must be zero */
+	__u64 period_length_ns;
+	__u64 duty_length_ns;
+	__u64 duty_offset_ns;
+};
+
+#define PWM_IOCTL_REQUEST	_IOW(0x75, 1, unsigned int)
+#define PWM_IOCTL_FREE		_IOW(0x75, 2, unsigned int)
+#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_ */