Message-ID: <20240930074707.GA57004@rigel>
Date: Mon, 30 Sep 2024 15:47:07 +0800
From: Kent Gibson <warthog618@...il.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: linux-kernel@...r.kernel.org, Trevor Gamblin <tgamblin@...libre.com>,
	David Lechner <dlechner@...libre.com>,
	Fabrice Gasnier <fabrice.gasnier@...s.st.com>
Subject: Re: [PATCH v5 8/8] pwm: Add support for pwmchip devices for faster
 and easier userspace access

On Mon, Sep 30, 2024 at 08:01:45AM +0200, Uwe Kleine-König wrote:
> 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?
>

Are we talking the PWM device or the consumer?  Or both?

Wrt the consumer, For GPIO we don't provide a useful default, though perhaps
we should. Instead we rely on the userspace library to provide one - if the
user hasn't.

I'd go with the pid option - that is more unique than the process name.

It could still be useful for the user to be able to provide a label,
especially if you are going to allow arbitrary labels via dt.  Though as
often as not the user doesn't know what name to provide and fills it with
something useless, so I don't think there is any clear winner here.

> > > +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.
>

That can always be a later refinement - better too much locking than
too little.

> > > +/**
> > > + * 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.
>

Yup, and as per the locking, the documentation can always be a later
refinement - so long as your intent is clear now before the ioctls get
chiselled in stone.

> > 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.
>

Depends on whether you want the interface to be standalone or an
addition to the sysfs - and that is your call.

> Thanks a lot for your feedback

No problem, and again sorry for not getting around to it earlier.

Cheers,
Kent.

Powered by blists - more mailing lists