| Message ID | 00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: New abstraction and userspace API | expand |
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;
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 >
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
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.
Hello David, On Mon, Sep 09, 2024 at 03:53:11PM -0500, David Lechner wrote: > On 9/8/24 9:59 AM, Uwe Kleine-König wrote: > > 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. Yeah, I understand what you mean. So the semantic isn't "take control over the PWM line" but "ensure control over the PWM line". This also is simpler to handle: Iff the request ioctl fails, you don't have control over the PWM line. That's (I think) what an application cares about. It wants to *have* control and doesn't care what is needed to reach that state. And the request routine returning an error if *taking* control fails because it was already taken before is an annoyance that makes error handling more complicated. But I agree and admit this is all very subjective. > >> // 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. I'd hope you as a user would use libpwm or at least use it as a template for your code. :-) And to argue honestly: I see an advantage in keeping usage simple and so I think it's nice that if an application doesn't care about requesting a PWM line, it doesn't need to and the kernel just cares enough about these details automatically to still make the operation safe. But again, this is subjective and somewhat expected that tastes differ. Best regards Uwe
On Tue, Sep 17, 2024 at 07:10:54PM +0200, Uwe Kleine-König wrote: > Hello David, > > On Mon, Sep 09, 2024 at 03:53:11PM -0500, David Lechner wrote: > > On 9/8/24 9:59 AM, Uwe Kleine-König wrote: > > > On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote: > > > > 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. > > I'd hope you as a user would use libpwm or at least use it as a template > for your code. :-) > > And to argue honestly: I see an advantage in keeping usage simple and so > I think it's nice that if an application doesn't care about requesting a > PWM line, it doesn't need to and the kernel just cares enough about > these details automatically to still make the operation safe. > > But again, this is subjective and somewhat expected that tastes differ. While working on v5 of this series (and still having your concern in mind) I found a good argument to make the REQUEST ioctl explicitly necessary: In v4, if a call to say SETEXACTWF fails after the pwm_device was auto-requested the error of pwm_set_waveform_might_sleep() was just forwarded to userspace without releasing the pwm_device. That semantic is ugly: While the ioctl failed, still something was changed (i.e. the pwm_device was requested). The obvious fix is to auto-free the pwm_device again if pwm_set_waveform_might_sleep() failed. But the semantic then is still not so nice IMHO. Then SETEXACTWF can fail for two different sets of reasons: a) requesting failed, and b) programming the hardware failed. A userspace program has to act differently on these two error classes, so it cannot be unaware of the whole request complication. This defeats the idea to make the API simple. From the userspace program's point of view it then is sensible to explicitly request the pwm_device to have a simple means to distinguish the two error classes. That together with your reasoning that TMTOWTDI isn't usually good for kernel interfaces convinces me that making the REQUEST ioctl obligatory is the right way to move forward. So that's what I will do in v5. Thanks for your input Uwe PS: I forgot my crypto token at home, so this mail is unsigned.
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_ */
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