Re: [PATCH v3 2/2] rust: platform: add irq accessors

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Danilo,

[…]

>> +
>> +    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
>> +    /// cannot be obtained.
>> +    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
>> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
>> +        let res = unsafe {
>> +            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
>> +        };
>> +
>> +        if res < 0 {
>> +            return Err(Error::from_errno(res));
>> +        }
>> +
>> +        Ok(res as u32)
>> +    }
> 
> I don't like the indirection of claiming a u32 representing the IRQ number from
> a bus device and stuffing it into an irq::Registration.
> 
> It would be better we we'd make it impossible (or at least harder) for a driver
> to pass the wrong number to irq::Registration.
> 
> I see two options:
> 
>  1) Make the platform::Device accessors themselves return an
>     irq::Registration.
> 
>  2) Make the platform::Device accessors return some kind of transparent cookie,
>     that drivers can't create themselves that can be fed into the
>     irq::Registration.
> 
> My preference would be 1) if there's no major ergonomic issue with that.

Isn’t 1 way more cluttered?

That's because the accessors would have to forward all of the arguments (i.e.:
currently 4) to register().

Going with approach 2 lets us keep the two APIs distinct, we'd only have to
take in the cookie in place of the u32, of course.

— Daniel