[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <hgoario7a2x6zoyjy5frbjmxla5vzkbucos55gsjycvxudue65@xqdta7kezomc>
Date: Mon, 30 Sep 2024 08:01:45 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Kent Gibson <warthog618@...il.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
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
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists