diff mbox series

[v2,5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

Message ID 7e50f9901d63c3aa27cdd02194f95b0ed79765f6.1721040875.git.u.kleine-koenig@baylibre.com
State Superseded
Headers show
Series pwm: New abstraction and userspace API | expand

Commit Message

Uwe Kleine-König July 15, 2024, 11:16 a.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       | 270 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  25 ++++
 3 files changed, 296 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

Comments

David Lechner July 15, 2024, 7:37 p.m. UTC | #1 
On 7/15/24 6:16 AM, Uwe Kleine-König wrote:

> +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)

What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
from sysfs, and it doesn't seem like there would be any performance consideration
for using an ioctl to get the same info.
Uwe Kleine-König July 15, 2024, 7:52 p.m. UTC | #2 
On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> 
> > +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
> 
> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
> from sysfs, and it doesn't seem like there would be any performance consideration
> for using an ioctl to get the same info.

This is provide all relevant information without looking at sysfs. Of
course "relevant" is somehow fuzzy. While it's nice not to have to look
at sysfs for basic operation---libpwm currently doesn't explore
there---you're right, the information duplication is somewhat ugly.

I'll think about it.

Thanks for your feedback,
Uwe
Uwe Kleine-König July 30, 2024, 10:26 a.m. UTC | #3 
On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> 
> > +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
> 
> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
> from sysfs, and it doesn't seem like there would be any performance consideration
> for using an ioctl to get the same info.

Thinking about that (and sending out a v3 that didn't change anything
here), I think it would be sensible to drop this. Maybe in favour of an
ioctl that gets the chip id. This way a consumer could find the
respective device directory below /sys/class/pwm without parsing the
chip id from the filename (assuming a sane udev configuration).

Would that make sense to you, too?

Best regards
Uwe
David Lechner July 30, 2024, 6:41 p.m. UTC | #4 
On 7/30/24 5:26 AM, Uwe Kleine-König wrote:
> On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
>> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
>>
>>> +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
>>
>> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
>> from sysfs, and it doesn't seem like there would be any performance consideration
>> for using an ioctl to get the same info.
> 
> Thinking about that (and sending out a v3 that didn't change anything
> here), I think it would be sensible to drop this. Maybe in favour of an
> ioctl that gets the chip id. This way a consumer could find the
> respective device directory below /sys/class/pwm without parsing the
> chip id from the filename (assuming a sane udev configuration).
> 
> Would that make sense to you, too?

How do we expect users to find the "right" PWM to use in the first
place? If libpwm is going to use libudev to enumerate all PWM devices
to find a match then we will already be able to get both the /sys/ and
/dev/ paths for the device from libudev.

And wouldn't the file name in both cases be "pwmchipX" (e.g.
/dev/pwm/pwmchip0 and /sys/class/pwm/pwmchip0) so we wouldn't need
to scrape the number out of the name if we wanted to do the matching
that way?

So I'm not convinced yet that having an IOCTL to get the device ID
it is especially useful.
Uwe Kleine-König July 31, 2024, 6:49 a.m. UTC | #5 
Hello David,

On Tue, Jul 30, 2024 at 01:41:32PM -0500, David Lechner wrote:
> > This way a consumer could find the
> > respective device directory below /sys/class/pwm without parsing the
> > chip id from the filename (assuming a sane udev configuration).
> > 
> > Would that make sense to you, too?
> 
> How do we expect users to find the "right" PWM to use in the first
> place? If libpwm is going to use libudev to enumerate all PWM devices
> to find a match then we will already be able to get both the /sys/ and
> /dev/ paths for the device from libudev.

I guess I have to take a deeper look into libudev. Until then just
dropping the ioctl for getting the number of pwm lines seems sensible.
Adding something later should be easy, when and if we see that an ioctl
for getting the chip number would be helpful.
 
> And wouldn't the file name in both cases be "pwmchipX" (e.g.
> /dev/pwm/pwmchip0 and /sys/class/pwm/pwmchip0) so we wouldn't need
> to scrape the number out of the name if we wanted to do the matching
> that way?

My train of thought was independent to libpwm. It was just: What if I
have a file descriptor for a pwm device (provided by some HAL lib maybe)
and I want to find out the /sys dir for it? But maybe that's not
something that will occur in practise. And if there is no such ioctl,
I just need more info than a plain file descriptor.
 
> So I'm not convinced yet that having an IOCTL to get the device ID
> it is especially useful.

Thanks for your input.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4dcb7ec4223f..918915563606 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>
 
@@ -1525,6 +1527,257 @@  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_GET_NUM_PWMS:
+		return chip->npwm;
+
+	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 = cwf.period_length,
+				.duty_length = cwf.duty_length,
+				.duty_offset = cwf.duty_offset,
+			};
+
+			ret = pwm_round_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf = (struct pwmchip_waveform) {
+				.hwpwm = cwf.hwpwm,
+				.period_length = wf.period_length,
+				.duty_length = wf.duty_length,
+				.duty_offset = wf.duty_offset,
+			};
+
+			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 = wf.period_length;
+			cwf.duty_length = wf.duty_length;
+			cwf.duty_offset = wf.duty_offset;
+
+			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 > 0 &&
+			    (cwf.duty_length > cwf.period_length ||
+			     cwf.duty_offset >= cwf.period_length))
+				return -EINVAL;
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			wf = (struct pwm_waveform){
+				.period_length = cwf.period_length,
+				.duty_length = cwf.duty_length,
+				.duty_offset = cwf.duty_offset,
+			};
+
+			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
@@ -1577,7 +1830,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;
 
@@ -1628,7 +1887,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);
 
@@ -2172,9 +2431,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 2d1f36a84f1e..fd100c27f109 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>
@@ -303,6 +304,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
@@ -317,6 +319,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..5038dca7c84c
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,25 @@ 
+/* 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 {
+	unsigned int hwpwm;
+	unsigned int __pad; /* padding, must be zero */
+	__u64 period_length;
+	__u64 duty_length;
+	__u64 duty_offset;
+};
+
+#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
+#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_ */