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

[Uwe Kleine-König]

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