Message-Id: <DC79F6AXATLL.JWGLJC42CXAN@kernel.org>
Date: Wed, 20 Aug 2025 14:39:10 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Alexandre Courbot"
 <acourbot@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <kernel@...labora.com>,
 <linux-media@...r.kernel.org>
Subject: Re: [PATCH 7/7] rust: samples: add the v4l2 sample driver

On Mon Aug 18, 2025 at 7:49 AM CEST, Daniel Almeida wrote:
> +/// The private data associated with the V4L2 device.
> +#[pin_data]
> +struct Data {}

I think you don't need #[pin_data] on this and the other empty structs. There
should be a blanket implementation for PinInit that got you covered.

> +/// The private data associated with a V4L2 device node, i.e. `struct
> +/// video_device`.
> +#[pin_data]
> +struct VideoData {}
> +
> +/// The private data associated with a V4L2 file, i.e. `struct v4l2_fh`.
> +#[pin_data]
> +struct File {}
> +
> +impl v4l2::file::DriverFile for File {
> +    type Driver = SampleDriver;
> +
> +    const MODULE: &'static ThisModule = &THIS_MODULE;
> +
> +    fn open(_vdev: &v4l2::video::Device<Self::Driver>) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +struct SampleDriver {
> +    _pdev: ARef<platform::Device>,
> +    _v4l2_reg: v4l2::device::Registration<Self>,
> +    video_reg: video::Registration<Self>,

Is the drop order of those registrations relevant? (If so, it shouldn't be on
the driver to get that right.)

> +}
> +
> +impl v4l2::device::Driver for SampleDriver {
> +    type Data = Data;
> +}
> +
> +#[vtable]
> +impl video::Driver for SampleDriver {
> +    type Data = VideoData;
> +    type File = File;
> +
> +    const NODE_TYPE: video::NodeType = video::NodeType::Video;
> +    const DIRECTION: video::Direction = video::Direction::Rx;
> +    const NAME: &'static CStr = c_str!("rv4l2");
> +    const CAPS: DeviceCaps = caps::device_caps::VIDEO_CAPTURE;
> +
> +    fn querycap(
> +        _file: &v4l2::file::File<Self::File>,
> +        _data: &<Self as video::Driver>::Data,
> +        cap: &mut caps::Capabilities,
> +    ) -> Result {
> +        cap.set_driver(c_str!("rv4l2"))?;
> +        cap.set_card(c_str!("rv4l2"))?;
> +        cap.set_bus_info(c_str!("platform:rv4l2"))?;
> +
> +        cap.set_device_caps(Self::CAPS);
> +        Ok(())
> +    }
> +}
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <SampleDriver as platform::Driver>::IdInfo,
> +    [(of::DeviceId::new(c_str!("test, rust-v4l2")), ())]
> +);
> +
> +impl platform::Driver for SampleDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +
> +        let v4l2_reg =
> +            v4l2::device::Registration::<Self>::new(dev, try_pin_init!(Data {}), GFP_KERNEL)?;

As mentioned on the struct already, you shouldn't need try_pin_init!() here,
since there should be a blanket implementation for PinInit.

> +
> +        let video_reg = video::Registration::<Self>::new(
> +            v4l2_reg.device(),
> +            try_pin_init!(VideoData {}),
> +            GFP_KERNEL,
> +        )?;
> +
> +        let this = KBox::new(
> +            Self {
> +                _pdev: pdev.into(),
> +                _v4l2_reg: v4l2_reg,
> +                video_reg,
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        dev_info!(
> +            dev,
> +            "Registered /dev/video{}\n",
> +            this.video_reg.device().num()
> +        );
> +        Ok(this.into())
> +    }
> +
> +    fn unbind(pdev: &platform::Device<Core>, _this: Pin<&Self>) {
> +        dev_info!(pdev.as_ref(), "Unbinding Rust V4L2 sample driver\n");
> +    }
> +}
> +
> +impl Drop for SampleDriver {
> +    fn drop(&mut self) {
> +        dev_dbg!(self._pdev.as_ref(), "Rust V4L2 sample driver removed\n");

A driver being unbound / removed is the same thing.

unbind() is the correct callback for this. SampleDriver::drop() is for
additional cleanup needed for the driver's private data. It's just that this
cleanup also happens on driver unbind.

(I think I need to fix up the existing sample drivers that date from before we
had unbind() in place.)

I'd just remove this drop() implementation and the ARef<platform::Device> from
SampleDriver.

> +    }
> +}
> +
> +kernel::module_platform_driver! {
> +    type: SampleDriver,
> +    name: "rust_driver_v4l2",
> +    authors: ["Daniel Almeida"],
> +    description: "Rust V4L2 sample video driver",
> +    license: "GPL v2",
> +}
>
> -- 
> 2.50.1

Powered by blists - more mailing lists