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

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2024-07-08 at 12:52 +0200, Uwe Kleine-König wrote:
> With this change each pwmchip can be accessed from userspace via a
> character device. Compared to the sysfs-API this is faster (on a
> stm32mp157 applying a new configuration takes approx 25% only) and
> allows to pass the whole configuration in a single ioctl allowing atomic
> application.
> 
> Thanks to Randy Dunlap for pointing out a missing kernel-doc
> description.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

I didn't looked very carefully at the patch but one thing did caught my
attention

...

> +
> +struct pwmchip_waveform {
> +	unsigned int hwpwm;
> +	__u64 period_length;
> +	__u64 duty_length;
> +	__u64 duty_offset;
> +};
> +

I do not think we should have holes in the struct given this is an userspace
interface.

One other thing is how likely is this struct to grow? If that is expected we
should probably think in adding some __reserved__ parameters or maybe to modify
the interface so we could make use of:

https://elixir.bootlin.com/linux/latest/source/include/linux/uaccess.h#L348

Like wrapping struct pwmchip_waveform in another struct with an extra member
forcing userspace to specify pwmchip_waveform size. But I agree it's a bit
awkward and ugly (but it could be hidden in libpwm).

Anyways, if this is not likely to change disregard all the compatibility
thingy...

- Nuno Sá