[PATCH v5 1/9] rust: pwm: Add Kconfig and basic data structures

[Uwe Kleine-König]

Hello Michal,

On Sat, Jun 28, 2025 at 04:38:15PM +0200, Michal Wilczynski wrote:
> On 6/27/25 17:10, Uwe Kleine-König wrote:
> > On Mon, Jun 23, 2025 at 08:08:49PM +0200, Michal Wilczynski wrote:
> >> +/// From C: `#define WFHWSIZE 20`
> >> +pub const WFHW_MAX_SIZE: usize = 20;
> > 
> > Can we somehow enforce that this doesn't diverge if the C define is
> > increased?
> 
> You are absolutely right. The hardcoded value is a maintenance risk. The
> #define is in core.c, so bindgen cannot see it.
> 
> I can address this by submitting a patch to move the #define WFHWSIZE to
> include/linux/pwm.h. This will make it part of the public API, allow
> bindgen to generate a binding for it, and ensure the Rust code can never
> diverge. Is this fine ?

I wonder if that is the opportunity to create a file
include/linux/pwm-provider.h. In that file we could collect all the bits
that are only relevant for drivers (pwm_ops, pwm_chip, pwmchip_parent,
pwmchip_alloc ...). (Some inline functions depend on some of these, so
some might have to stay in pwm.h)

I can address that in parallel, don't add this quest to your series. So
yes, move WFHWSIZE to include/linux/pwm.h (and rename it to PWM_WFHWSIZE
to not pollute the global namespace).
 
> > Please don't expose these non-atomic callbacks. pwm_disable() would be
> > fine.
> > 
> > Otherwise I'd prefer if pwm_set_waveform_might_sleep() is the API
> > exposed to/by Rust.
> 
> 
> OK, I'll remove all the setters from the State, while will keep the
> getters, as they would be useful in apply callbacks.

How so? They might be useful for consumers, but my preferred idiom for
them is that they know at each point in time what they want completely
and don't make that dependant on the previous setting.

> Will implement additional functions for Device i.e set_waveform,
> round_waveform and get_waveform, and the new enum to expose the result
> of the round_waveform more idiomatically.
> 
> /// Describes the outcome of a `round_waveform` operation.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub enum RoundingOutcome {
>     /// The requested waveform was achievable exactly or by rounding values down.
>     ExactOrRoundedDown,
> 
>     /// The requested waveform could only be achieved by rounding up.
>     RoundedUp,
> }

Sounds good. Hoever I have some doubts about the C semantic here, too.
Is it really helpful to provide that info? A user of that return value
has to check anyhow which parameter got rounded up. If you have an
opinion here, please share.
 
Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20250629/dbe81303/attachment.sig>