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

Uwe Kleine-König
2025-04-17T06:42:00Z

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

Hello David,

On Wed, Apr 16, 2025 at 11:29:50AM -0500, David Lechner wrote:
> the passed wf according to hardware constraints. All parameters are
> > + * rounded down to the next possible value, unless there is no such value, then
> > + * values are rounded up. Note that zero isn't considered for rounding down
> > + * period_length_ns.
> > + */
> > +#define PWM_IOCTL_ROUNDWF _IOWR(0x75, 3, struct pwmchip_waveform)
>
> Should this return 1 if exact could not be met to match the other functions?

Currently only PWM_IOCTL_SETEXACTWF has that semantic. Adding that
information to PWM_IOCTL_SETROUNDEDWF has to involve calling
.read_waveform + .round_waveform_fromhw which makes it a bit more
expensive, maybe for little gain.

> > +/* Get the currently implemented waveform */
> > +#define PWM_IOCTL_GETWF _IOWR(0x75, 4, struct pwmchip_waveform)
> > +
> > +/* Like PWM_IOCTL_ROUNDWF + PWM_IOCTL_SETEXACTWF in one go. */
> > +#define PWM_IOCTL_SETROUNDEDWF _IOW(0x75, 5, struct pwmchip_waveform)
> > +
> > +/*
> > + * Program the PWM to emit exactly the passed waveform, subject only to rounding
> > + * down each value less than 1 ns. Returns 0 on success, 1 if the waveform
> > + * cannot be implemented exactly, or other negative error codes.
>
> It doesn't make sense to me that PWM_IOCTL_SETEXACTWF could return 1 meaning
> that the exact request could not be met. Isn't that the point of
> PWM_IOCTL_SETEXACTWF?

Yes it is. Returning 1 is an error indicator and then the HW state isn't
modified.

> To either do exactly as requested (with 1 ns precision)
> or return negative error code without changing the output state?

Returning 1 was my conclusion of our discussion that -EINVAL and -ERANGE
are both bad. The former because there could be other reasons for
-EINVAL during hw register writing and -ERANGE (also) because the
semantic doesn't fit nicely.

So I gave up finding a -ESOMETHING that has both the right semantic and
could be returned without being confused with a different problem during
hardware access. Inventing a new error code seems wrong to me, so only
the positive number range is left.

> It seems like only PWM_IOCTL_SETROUNDEDWF should be able to return 1. My natural
> expectation is that negative error would mean that the hardware output was not
> modified and non-negative value means that hardware output was modified.

I'm also not entirely happy with the semantics, but still I have no idea
for an improvement.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]