diff mbox series

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

Message ID 8d3acfc431ecd431d6cced032dcb58ad2579474c.1710670958.git.u.kleine-koenig@pengutronix.de
State Accepted
Headers show
Series pwm: Add support for character devices | expand

Commit Message

Uwe Kleine-König March 17, 2024, 10:40 a.m. UTC
With this change each pwmchip 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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c       | 310 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   2 +
 include/uapi/linux/pwm.h |  23 +++
 3 files changed, 322 insertions(+), 13 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

Comments

John Ernberg April 8, 2024, 3:42 p.m. UTC | #1
Hi Uwe,

Seeing this patch set made me excited.

Did you consider or try a pwm_line model structure, that is connected to a file
descriptor, more like request_line from gpio chardevs?

We have been using gpio chardevs for a while and really benefitted from that
over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
application design more streamlined.

With this patch set I do not see how we can name the PWMs in the device tree
nor during request which is a big benefit with GPIO when we need to support
multiple hardwares. Nor can I see how we would inspect which pins are
allocated or their names when debugging.

Thanks! // John Ernberg
Uwe Kleine-König April 9, 2024, 7:49 a.m. UTC | #2
Hallo,

On Mon, Apr 08, 2024 at 03:42:39PM +0000, John Ernberg wrote:
> Seeing this patch set made me excited.

\o/

> Did you consider or try a pwm_line model structure, that is connected to a file
> descriptor, more like request_line from gpio chardevs?

I'd be open for an extension that simplifies detection of which (chip,
hwpwm) pair corresponds to which usage. It could be fed from a device
tree like:

	&pwm0 {
		compatible = ...;
		reg = <...>;
		#pwm-cells = <3>;
		pwm-names = "motor-control", "backlight", "", "status-led";
	}

and provide a function in libpwm that allows to determine the chipid of
the chip corresponding to &pwm0 (or an open fd to that device) and 3
when "status-led" is searched for.

This could be done completely in userspace I think (by inspecting
sysfs), so I wonder if this could be a complete userspace
implementation. I like keeping the kernel API simple, i.e. let it only
work (as it is now) only using chipid and hwpwm. Hmm, maybe make pwm
names a kernel concept exposed to sysfs to not have to hardcode the dt
details into libpwm?!

> We have been using gpio chardevs for a while and really benefitted from that
> over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
> interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
> application design more streamlined.

libpwm can also fall back to sysfs when the character devices are
missing. If you want to compare the two implementation and create a best
of two merge from it, that would be great.

> With this patch set I do not see how we can name the PWMs in the device tree
> nor during request which is a big benefit with GPIO when we need to support
> multiple hardwares. Nor can I see how we would inspect which pins are
> allocated or their names when debugging.

Best regards
Uwe
John Ernberg April 15, 2024, 11:27 a.m. UTC | #3
Hi Uwe,

On 4/9/24 9:49 AM, Uwe Kleine-König wrote:
> Hallo,
> 
> On Mon, Apr 08, 2024 at 03:42:39PM +0000, John Ernberg wrote:
>> Seeing this patch set made me excited.
> 
> \o/
> 
>> Did you consider or try a pwm_line model structure, that is connected to a file
>> descriptor, more like request_line from gpio chardevs?
> 
> I'd be open for an extension that simplifies detection of which (chip,
> hwpwm) pair corresponds to which usage. It could be fed from a device
> tree like:
> 
> 	&pwm0 {
> 		compatible = ...;
> 		reg = <...>;
> 		#pwm-cells = <3>;
> 		pwm-names = "motor-control", "backlight", "", "status-led";
> 	}
> 
> and provide a function in libpwm that allows to determine the chipid of
> the chip corresponding to &pwm0 (or an open fd to that device) and 3
> when "status-led" is searched for.
> 
> This could be done completely in userspace I think (by inspecting
> sysfs), so I wonder if this could be a complete userspace
> implementation. I like keeping the kernel API simple, i.e. let it only
> work (as it is now) only using chipid and hwpwm. Hmm, maybe make pwm
> names a kernel concept exposed to sysfs to not have to hardcode the dt
> details into libpwm?!

Sounds like it could be workable that way. Thanks!

> 
>> We have been using gpio chardevs for a while and really benefitted from that
>> over the sysfs interface. I wrote a simple wrapper around the PWM sysfs
>> interface mimicing the way gpio chardevs work for PWM using dirfd, to make our
>> application design more streamlined.
> 
> libpwm can also fall back to sysfs when the character devices are
> missing. If you want to compare the two implementation and create a best
> of two merge from it, that would be great.
> 
>> With this patch set I do not see how we can name the PWMs in the device tree
>> nor during request which is a big benefit with GPIO when we need to support
>> multiple hardwares. Nor can I see how we would inspect which pins are
>> allocated or their names when debugging.
> 
> Best regards
> Uwe
> 

Best regards // John Ernberg
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2e08fcfbe138..7f41ab087b98 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -21,6 +21,8 @@ 
 
 #include <dt-bindings/pwm/pwm.h>
 
+#include <uapi/linux/pwm.h>
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/pwm.h>
 
@@ -249,6 +251,25 @@  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
 
+static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	int ret = -EOPNOTSUPP;
+
+	if (ops->get_state) {
+		pwmchip_lock(chip);
+
+		ret = ops->get_state(chip, pwm, state);
+
+		pwmchip_unlock(chip);
+
+		trace_pwm_get(pwm, state, ret);
+	}
+
+	return ret;
+}
+
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
  * @pwm: PWM device
@@ -411,14 +432,7 @@  static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
-		pwmchip_lock(chip);
-
-		err = ops->get_state(chip, pwm, &state);
-
-		pwmchip_unlock(chip);
-
-		trace_pwm_get(pwm, &state, err);
-
+		err = pwm_get_state_hw(pwm, &state);
 		if (!err)
 			pwm->state = state;
 
@@ -1120,6 +1134,250 @@  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;
+	int ret;
+
+	mutex_lock(&pwm_lock);
+
+	if (!chip->operational) {
+		ret = -ENXIO;
+		goto out_unlock;
+	}
+
+	cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
+	if (!cdata) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	cdata->chip = chip;
+
+	file->private_data = cdata;
+
+	ret = nonseekable_open(inode, file);
+
+out_unlock:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+
+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;
+
+	mutex_lock(&pwm_lock);
+
+	if (!chip->operational) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	switch (cmd) {
+	case PWM_IOCTL_GET_NUM_PWMS:
+		ret = chip->npwm;
+		break;
+
+	case PWM_IOCTL_REQUEST:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_free(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_GET:
+		{
+			struct pwmchip_state cstate;
+			struct pwm_state state;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cstate, (struct pwmchip_state __user *)arg, sizeof(cstate));
+			if (ret)
+				goto out_unlock;
+
+			ret = pwm_cdev_request(cdata, cstate.hwpwm);
+			if (ret)
+				goto out_unlock;
+
+			pwm = cdata->pwm[cstate.hwpwm];
+
+			ret = pwm_get_state_hw(pwm, &state);
+			if (ret)
+				goto out_unlock;
+
+			if (state.enabled) {
+				cstate.period = state.period;
+				if (state.polarity == PWM_POLARITY_NORMAL) {
+					cstate.duty_offset = 0;
+					cstate.duty_cycle = state.duty_cycle;
+				} else {
+					cstate.duty_offset = state.duty_cycle;
+					cstate.duty_cycle = state.period - state.duty_cycle;
+				}
+			} else {
+				cstate.period = 0;
+				cstate.duty_cycle = 0;
+				cstate.duty_offset = 0;
+			}
+			ret = copy_to_user((struct pwmchip_state __user *)arg, &cstate, sizeof(cstate));
+		}
+		break;
+
+	case PWM_IOCTL_APPLY:
+		{
+			struct pwmchip_state cstate;
+			struct pwm_state state = { };
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cstate, (struct pwmchip_state __user *)arg, sizeof(cstate));
+			if (ret)
+				goto out_unlock;
+
+			if (cstate.period > 0 &&
+			    (cstate.duty_cycle > cstate.period ||
+			     cstate.duty_offset >= cstate.period)) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
+
+			/*
+			 * While the API provides a duty_offset member
+			 * to describe (among others) also inversed
+			 * polarity wave forms, the translation into the
+			 * traditional representation with a (binary) polarity
+			 * isn't trivial because the lowlevel drivers round
+			 * duty_cycle down when applying a setting and so in the
+			 * representation with duty_offset the rounding is
+			 * inconsistent. I have no idea what's the best way to
+			 * fix that, so to not commit to a solution yet, just
+			 * refuse requests with .duty_offset that would yield
+			 * inversed polarity for now.
+			 */
+			if (cstate.duty_cycle < cstate.period &&
+			    cstate.duty_offset + cstate.duty_cycle >= cstate.period) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
+
+			ret = pwm_cdev_request(cdata, cstate.hwpwm);
+			if (ret)
+				goto out_unlock;
+
+			pwm = cdata->pwm[cstate.hwpwm];
+
+			if (cstate.period > 0) {
+				state.enabled = true;
+				state.period = cstate.period;
+				state.polarity = PWM_POLARITY_NORMAL;
+				state.duty_cycle = cstate.duty_cycle;
+			} else {
+				state.enabled = false;
+			}
+
+			ret = pwm_apply_might_sleep(pwm, &state);
+		}
+		break;
+
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+out_unlock:
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+
+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
@@ -1173,7 +1431,13 @@  int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	chip->operational = true;
 	pwmchip_unlock(chip);
 
-	ret = device_add(&chip->dev);
+	if (chip->id < 256)
+		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;
 
@@ -1232,7 +1496,7 @@  void pwmchip_remove(struct pwm_chip *chip)
 
 	mutex_unlock(&pwm_lock);
 
-	device_del(&chip->dev);
+	cdev_device_del(&chip->cdev, &chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
@@ -1785,9 +2049,29 @@  DEFINE_SEQ_ATTRIBUTE(pwm_debugfs);
 
 static int __init pwm_init(void)
 {
-	if (IS_ENABLED(CONFIG_DEBUG_FS))
-		debugfs_create_file("pwm", 0444, NULL, NULL, &pwm_debugfs_fops);
+	struct dentry *pwm_debugfs = NULL;
+	int ret;
 
-	return class_register(&pwm_class);
+	if (IS_ENABLED(CONFIG_DEBUG_FS))
+		pwm_debugfs = debugfs_create_file("pwm", 0444, NULL, NULL,
+						  &pwm_debugfs_fops);
+
+	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
+	if (ret) {
+		pr_warn("Failed to initialize chrdev region for PWM usage\n");
+		goto err_alloc_chrdev;
+	}
+
+	ret = class_register(&pwm_class);
+	if (ret) {
+		pr_warn("Failed to initialize PWM class\n");
+
+		unregister_chrdev_region(pwm_devt, 256);
+err_alloc_chrdev:
+
+		debugfs_remove(pwm_debugfs);
+	}
+
+	return ret;
 }
 subsys_initcall(pwm_init);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9c84e0ba81a4..d5fed23b48f0 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/mutex.h>
@@ -281,6 +282,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..ca765bfaa68d
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,23 @@ 
+/* 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_state {
+	unsigned int hwpwm;
+	__u64 period;
+	__u64 duty_cycle;
+	__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)
+/* reserve nr = 3 for rounding */
+#define PWM_IOCTL_GET		_IOWR(0x75, 4, struct pwmchip_state)
+#define PWM_IOCTL_APPLY		_IOW(0x75, 5, struct pwmchip_state)
+
+#endif /* _UAPI_PWM_H_ */