Hi Rahul, Some mostly doc-related comments below... On Sun, Jul 13, 2025 at 11:12 PM Rahul Rameshbabu <sergeantsagara@xxxxxxxxxxxxxx> wrote: > > Some points I did not address from the last review cycle: > > * Enabling CONFIG_HID=m with the Rust bindings > - This is a limitation due to the Makefile rules that currently exist, > compiling all the Rust bindings into kernel.o, which gets linked into the > core kernel image. > - I would be happy to work on support for module-level bindings, but this is > not a problem unique only to the Rust HID binding work. I am working on supporting that, so for the moment your approach is fine. > +L: rust-for-linux@xxxxxxxxxxxxxxx > +T: git https://github.com/Rust-for-Linux/linux.git hid-next Hmm... I think we discussed going through HID, no? Or did something change? > +/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf > +pub const MAIN_ITEM_CONSTANT: u8 = bindings::HID_MAIN_ITEM_CONSTANT as u8; > +/// Indicates the item represents data from a physical control. Please add new-lines between items. > + Generic = bindings::HID_GROUP_GENERIC as isize, > + /// Maps multitouch devices to hid-multitouch instead of hid-generic. Same between `enum` variants. > + /// Internal function used to convert Group variants into u16 [`Group`] [`u16`] > + const fn into(self) -> u16 { Could the `enum` be `repr(u16)` directly? Are those `as isize` for a reason? > + // variants assigned constants that can be represented as u16 Please format consistently comments as well. > + /// Returns the HID report group. > + pub fn group(&self) -> u16 { > + // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device` > + unsafe { *self.as_raw() }.group > + } Do you need to expose the group as a raw integer? Also, are these used currently? > +/// Abstraction for the HID device ID structure ([`struct hid_device_id`]). This intra-doc pointing is probably broken -- we don't have yet automatic support to link to C ones yet (please check `make ..... rustdoc`). > + bus: 0x3, /* BUS_USB */ Please use `//` for comments unless there is a reason that wouldn't work (e.g. to comment out within code). > +/// IdTable type for HID [`IdTable`] Period at the end. (I see others elsewhere are like this, so that is probably you had it like this -- they should be fixed too) > +/// # Example Please use the plural even if there is one example. > +/// // Perform some report descriptor fixup Period at the end. > +///``` Missing space (several). > + "Cannot fix report description due to length conversion failure: {}!\n", This could probably be a bit shorter using {e} > + // Build a mutable Rust slice from buf and size Markdown. > + // SAFETY: The HID subsystem only ever calls the report_fixup callback Markdown (several). > +/// fn report_fixup<'a, 'b: 'a>(_hdev: &hid::Device<device::Core>, rdesc: &'b mut [u8]) -> &'a [u8] { The example does not build -- please see how to test them here: https://docs.kernel.org/rust/testing.html#kunit-tests-are-documentation-tests Thanks! Cheers, Miguel
