[PATCH v9 2/6] rust: pwm: Add complete abstraction layer

[Michal Wilczynski]

On 7/7/25 08:57, Uwe Kleine-König wrote:
> Hello Danilo,
> 
> On Sun, Jul 06, 2025 at 02:23:04PM +0200, Danilo Krummrich wrote:
>> On Sun, Jul 06, 2025 at 01:45:13PM +0200, Michal Wilczynski wrote:
>>> +    /// # Safety
>>> +    ///
>>> +    /// `dev` must be a valid pointer to a `bindings::device` embedded within a
>>> +    /// `bindings::pwm_chip`. This function is called by the device core when the
>>> +    /// last reference to the device is dropped.
>>> +    unsafe extern "C" fn release_callback(dev: *mut bindings::device) {
>>> +        // SAFETY: The function's contract guarantees that `dev` points to a `device`
>>> +        // field embedded within a valid `pwm_chip`. `container_of!` can therefore
>>> +        // safely calculate the address of the containing struct.
>>> +        let c_chip_ptr = unsafe { container_of!(dev, bindings::pwm_chip, dev) };
>>> +
>>> +        // SAFETY: `c_chip_ptr` is a valid pointer to a `pwm_chip` as established
>>> +        // above. Calling this FFI function is safe.
>>> +        let drvdata_ptr = unsafe { bindings::pwmchip_get_drvdata(c_chip_ptr) };
>>> +
>>> +        if !drvdata_ptr.is_null() {
>>
>> Is this check needed? I think one can't create a pwm::Chip instance without
>> providing a T, so this pointer can't be NULL I think.
> 
> There are currently a few C drivers, that don't use a private data
> struct that is managed by the pwmchip. One of them doesn't make use of
> the pwmchip's drvdata at all. The latter is drivers/pwm/pwm-twl-led.c.

Thank you both for the feedback on this point.

My interpretation aligns with Danilo's: the null check is unnecessary
within the context of this Rust abstraction.

The pwm::Chip::new() API as designed guarantees that driver data is
always provided. For the cases Uwe mentioned, where a C driver might not
use private data, the idiomatic Rust solution would be to use a
zero sized type (e.g., an empty struct or ()) for the PwmOps::DrvData.
This approach still results in a valid, non null pointer being passed to
the C core.

Given that all paths within this abstraction lead to a non null drvdata
pointer, I believe removing the redundant check is the correct approach.

> 
> Best regards
> Uwe

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