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

From: Uwe Kleine-König
Date: Mon Sep 30 2024 - 02:02:04 EST


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

Attachment: signature.asc
Description: PGP signature