On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori
wrote:> On Mon, 27 May 2024 21:22:47 +0200
> Danilo Krummrich <dakr at redhat.com> wrote:
>
> >> > +/// Abstraction around a C firmware struct.
> >> > +///
> >> > +/// This is a simple abstraction around the C firmware API.
Just like with the C API, firmware can
> >> > +/// be requested. Once requested the abstraction provides
direct access to the firmware buffer as
> >> > +/// `&[u8]`. Alternatively, the firmware can be copied
to a new buffer using `Firmware::copy`. The
> >> > +/// firmware is released once [`Firmware`] is dropped.
> >> > +///
> >> > +/// # Examples
> >> > +///
> >> > +/// ```
> >> > +/// let fw =
Firmware::request("path/to/firmware.bin", dev.as_ref())?;
> >> > +/// driver_load_firmware(fw.data());
> >> > +/// ```
> >> > +pub struct Firmware(Opaque<*const
bindings::firmware>);
> >>
> >> Wrapping a raw pointer is not the intended use of Qpaque type?
> >>
> >
> > Indeed, will fix this in v2 and use NonNull instead. I'll also
offload most of
> > the boilerplate in the 'request' functions to some common
'request_internal' one.
>
> You might need to add 'Invariants' comment on Firmware struct.
Which ones do you think should be documented?
>
> BTW, what merge window are you aiming for? As I wrote before, I have a
> driver that needs the firmware abstractions (the minimum device
> abstractions is enough; Device::as_raw() and as_ref()). So the sooner,
> the better for me.
I'm not aiming this on a specific merge window.
However, if you have a driver that needs the firmware abstractions, I would be
surprised if there were any hesitations to already merge the minimum device
abstractions [1] and this one (once reviewed) without the rest. At least there
aren't any from my side.
[1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at
redhat.com/
>