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

[David Lechner]

On 4/16/25 4:43 AM, Uwe Kleine-König wrote:
> With this change each pwmchip defining the new-style waveform callbacks
> can be accessed from userspace via a character device. Compared to the
> sysfs-API this is faster and allows to pass the whole configuration in a
> single ioctl allowing atomic application and thus reducing glitches.

Didn't do a full review yet, but I have some confusion on when 1 should actually
be returned from ioctl calls...

> +/*
> + * Modifies 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?

> +
> +/* 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? To either do exactly as requested (with 1 ns precision)
or return negative error code without changing the output state?

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.

> + */
> +#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
> +
> +#endif /* _UAPI_PWM_H_ */
> 
> base-commit: bde5547f2e87e6c71db79dc41e56aff3061e39a9