Re: [PATCH v12 2/3] rust: pwm: Add Kconfig and basic data structures

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

On 7/25/25 17:08, Daniel Almeida wrote:
> Hi Michal,
> 
> 
> Overall looks good, a few minor comments:

Hi,
Congratulations for getting your IoMem series merged. Thank you for your
work, as it will allow me to proceed and re-add the driver part for this
series.

> 
> […]
> 
>> +
>> +/// Wrapper for board-dependent PWM arguments [`struct pwm_args`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct Args(Opaque<bindings::pwm_args>);
>> +
>> +impl Args {
>> +    /// Creates an `Args` wrapper from a C struct pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `c_args_ptr` is a valid, non-null pointer
>> +    /// to `bindings::pwm_args` and that the pointed-to data is valid
>> +    /// for the duration of this function call (as data is copied).
>> +    unsafe fn from_c_ptr(c_args_ptr: *const bindings::pwm_args) -> Self {
>> +        // SAFETY: Caller guarantees `c_args_ptr` is valid. We dereference it to copy.
>> +        Args(Opaque::new(unsafe { *c_args_ptr }))
>> +    }
> 
> from_raw()
> 
> 
>> +
>> +    /// Returns the period of the PWM signal in nanoseconds.
>> +    pub fn period(&self) -> u64 {
>> +        // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>> +        // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>> +        // valid and aligned for the lifetime of `self` because `Opaque` owns a copy.
>> +        unsafe { (*self.0.get()).period }
>> +    }
>> +
>> +    /// Returns the polarity of the PWM signal.
>> +    pub fn polarity(&self) -> Result<Polarity, Error> {
>> +        // SAFETY: `self.0.get()` returns a pointer to the `bindings::pwm_args`
>> +        // managed by the `Opaque` wrapper. This pointer is guaranteed to be
>> +        // valid and aligned for the lifetime of `self`.
>> +        let raw_polarity = unsafe { (*self.0.get()).polarity };
>> +        Polarity::try_from(raw_polarity)
>> +    }
>> +}
>> +
>> +/// Wrapper for PWM state [`struct pwm_state`](srctree/include/linux/pwm.h).
>> +#[repr(transparent)]
>> +pub struct State(bindings::pwm_state);
> 
> No Opaque<T>?

Since this is a copy of the state it's fine. The Args above should
follow similar pattern, the divergence stemmed when iterating with this
series. So I would rather fix the Args to also not be Opaque as it
doesn't need to be, since it's also a copy of the original (since it's
so small and read only).

> 
>> +
>> +impl State {
>> +    /// Creates a `State` wrapper by taking ownership of a C `pwm_state` value.
>> +    pub(crate) fn from_c(c_state: bindings::pwm_state) -> Self {
>> +        State(c_state)
>> +    }
>> +
>> +    /// Returns `true` if the PWM signal is enabled.
>> +    pub fn enabled(&self) -> bool {
>> +        self.0.enabled
>> +    }
>> +}
>>
>> -- 
>> 2.34.1
>>
>>
> 
> If the lack of Opaque<T> is not a problem for whatever reason:
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> — Daniel
> 
> 

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