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

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 6/29/25 12:29, Uwe Kleine-König wrote:
> On Sat, Jun 28, 2025 at 09:47:19PM +0200, Michal Wilczynski wrote:

>>>>> +    /// Sets the polarity of the PWM signal.
>>>>> +    pub fn set_polarity(&mut self, polarity: Polarity) {
>>>>> +        self.0.polarity = polarity.into();
>>>>> +    }
>>>>
>>>> Please don't expose these non-atomic callbacks. pwm_disable() would be
>>>> fine.
>>
>> Hmm, I've just realized that without those setters it would most likely
>> impossible to correctly implement the get_state callback.
> 
> You shouldn't implement the get_state callback for a waveform driver.

You're right that a new driver using the waveform API shouldn't
implement .get_state.

My goal for the abstraction layer, however, is to be flexible enough to
support writing both modern waveform drivers and legacy style drivers
that use the .apply and .get_state callbacks.

To implement the .get_state callback, a driver needs the ability to
construct a State struct and populate its fields from hardware values
before returning it to the PWM core. Without this ability there is no
way to implement get_state callback.

I think the cleaner way, without the setters would be to update the
`new` like so:
    pub fn new(
        period: u64,
        duty_cycle: u64,
        polarity: Polarity,
        enabled: bool,
        usage_power: bool,
    ) -> Self {
        let raw_c_state = bindings::pwm_state {
            period,
            duty_cycle,
            polarity: polarity.into(),
            enabled,
            usage_power,
        };

        State(raw_c_state)
    }

This way the get_state callback would be responsible for creating new
state and initializing it, instead of passing the mutable State to
get_state.


> 
> Best regards
> Uwe

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>