| Message ID | 20250313160220.6410-5-sergeantsagara@protonmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Initial work for Rust abstraction for HID device driver development | expand |
On Mar 13 2025, Rahul Rameshbabu wrote: > This code will eventually contain the logic needed to drive the backlight > of displays that implement the USB Monitor Control Class specification. > Examples include the Apple Studio Display and Apple Pro Display XDR > monitors. USB Monitor Control Class encompasses more than just backlight > control, so the driver could be further extended as monitors support more > functionality in the specification. > > This code is a skeleton currently, where the focus right now is on the core > Rust API. The driver skeleton was written before approaching the Rust API > and C binding work. This was done to provide a guide for what the Rust API > should look like and avoid any rough C binding work from being exposed to > Rust HID device drivers. skeletons are good for documentation, but not really for code review as they can not compile. You should make this patch part of a documentation in Documentation/hid/, and squash it with the next one (having a minimal full driver instead of skeleton+fill in the voids). Cheers, Benjamin > > To go forward with this driver for the purpose of external monitor > backlight control, a new DRM backlight API that is scoped per connector > will be required. I am currently in the process of developing this new API. > I document the details in my related blog posts. The issue with the current > backlight API is it was designed on the assumption that only internal > panels have controllable backlights. Using this assumption combined with > another that there can only ever be a single internal panel, having more > than one device register with the backlight interface would confuse > userspace applications. > > Julius Zint originally tried to implement such a driver a bit more than a > year ago with a C driver but was blocked by the limitations of the > backlight API. I asked him for permission to continue the work in Rust > while accrediting him for the HID report parsing logic for the backlight > support in the USB Monitor Control Class specification. > > Cc: Julius Zint <julius@zint.sh> > Link: https://lore.kernel.org/lkml/20230820094118.20521-1-julius@zint.sh/ > Link: https://binary-eater.github.io/posts/linux_usb_monitor_control/ > Link: https://www.usb.org/sites/default/files/usbmon11.pdf > Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com> > --- > drivers/hid/Kconfig | 8 +++++++ > drivers/hid/Makefile | 1 + > drivers/hid/hid_monitor_control.rs | 37 ++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > create mode 100644 drivers/hid/hid_monitor_control.rs > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index e085964c7ffc..92be13acb956 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -722,6 +722,14 @@ config RUST_HID_ABSTRACTIONS > Adds support needed for HID drivers written in Rust. It provides a > wrapper around the C hid core. > > +config HID_MONITOR_CONTROL > + tristate "USB Monitor Control Class support" > + depends on USB_HID > + depends on RUST_HID_ABSTRACTIONS > + help > + Say Y here if you want to enable control over a monitor that uses USB > + Monitor Control Class. > + > config HID_REDRAGON > tristate "Redragon keyboards" > default !EXPERT > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 482b096eea28..bf8b096bcf23 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -86,6 +86,7 @@ obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o > obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o > +obj-$(CONFIG_HID_MONITOR_CONTROL) += hid_monitor_control.o > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o > obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o > obj-$(CONFIG_HID_NINTENDO) += hid-nintendo.o > diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs > new file mode 100644 > index 000000000000..18afd69a56d5 > --- /dev/null > +++ b/drivers/hid/hid_monitor_control.rs > @@ -0,0 +1,37 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com> > + > +use kernel::prelude::*; > +use kernel::hid::{ > + self, > + Driver, > +}; > + > +struct HidMonitorControl; > + > +#[vtable] > +impl Driver for HidMonitorControl { > + fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> { > + /* TODO implement */ > + Ok(()) > + } > + > + fn remove(dev: &mut hid::Device) { > + /* TODO implement */ > + } > +} > + > +kernel::module_hid_driver! { > + driver: HidMonitorControl, > + id_table: [ > + kernel::usb_device! { > + vendor: /* TODO fill in */, > + product: /* TODO fill in */, > + }, > + ], > + name: "monitor_control", > + author: "Rahul Rameshbabu <sergeantsagara@protonmail.com>", > + description: "Driver for the USB Monitor Control Class", > + license: "GPL", > +} > -- > 2.47.2 > >
On Thu, Mar 13, 2025 at 5:58 PM Benjamin Tissoires <bentiss@kernel.org> wrote: > > skeletons are good for documentation, but not really for code review as > they can not compile. > > You should make this patch part of a documentation in > Documentation/hid/, and squash it with the next one (having a minimal > full driver instead of skeleton+fill in the voids). It could be part of the documentation of the `module_hid_driver!` for instance, like we have done for other of those macros. (In general, we try to use `Documentation/` for things that do not fit as documentation for any of the code "items".) Cheers, Miguel
On Fri, 14 Mar, 2025 15:41:02 +0100 "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> wrote: > On Thu, Mar 13, 2025 at 5:58 PM Benjamin Tissoires <bentiss@kernel.org> wrote: >> >> skeletons are good for documentation, but not really for code review as >> they can not compile. >> >> You should make this patch part of a documentation in >> Documentation/hid/, and squash it with the next one (having a minimal >> full driver instead of skeleton+fill in the voids). > > It could be part of the documentation of the `module_hid_driver!` for > instance, like we have done for other of those macros. > > (In general, we try to use `Documentation/` for things that do not fit > as documentation for any of the code "items".) > > Cheers, > Miguel In general, I will add a lot more documentation in my next revision. For example, I have a bunch of unsafe usage right now without any SAFETY comments. Thanks for the review, Rahul Rameshbabu
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index e085964c7ffc..92be13acb956 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -722,6 +722,14 @@ config RUST_HID_ABSTRACTIONS Adds support needed for HID drivers written in Rust. It provides a wrapper around the C hid core. +config HID_MONITOR_CONTROL + tristate "USB Monitor Control Class support" + depends on USB_HID + depends on RUST_HID_ABSTRACTIONS + help + Say Y here if you want to enable control over a monitor that uses USB + Monitor Control Class. + config HID_REDRAGON tristate "Redragon keyboards" default !EXPERT diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index 482b096eea28..bf8b096bcf23 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -86,6 +86,7 @@ obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o +obj-$(CONFIG_HID_MONITOR_CONTROL) += hid_monitor_control.o obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o obj-$(CONFIG_HID_NINTENDO) += hid-nintendo.o diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs new file mode 100644 index 000000000000..18afd69a56d5 --- /dev/null +++ b/drivers/hid/hid_monitor_control.rs @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com> + +use kernel::prelude::*; +use kernel::hid::{ + self, + Driver, +}; + +struct HidMonitorControl; + +#[vtable] +impl Driver for HidMonitorControl { + fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> { + /* TODO implement */ + Ok(()) + } + + fn remove(dev: &mut hid::Device) { + /* TODO implement */ + } +} + +kernel::module_hid_driver! { + driver: HidMonitorControl, + id_table: [ + kernel::usb_device! { + vendor: /* TODO fill in */, + product: /* TODO fill in */, + }, + ], + name: "monitor_control", + author: "Rahul Rameshbabu <sergeantsagara@protonmail.com>", + description: "Driver for the USB Monitor Control Class", + license: "GPL", +}
This code will eventually contain the logic needed to drive the backlight of displays that implement the USB Monitor Control Class specification. Examples include the Apple Studio Display and Apple Pro Display XDR monitors. USB Monitor Control Class encompasses more than just backlight control, so the driver could be further extended as monitors support more functionality in the specification. This code is a skeleton currently, where the focus right now is on the core Rust API. The driver skeleton was written before approaching the Rust API and C binding work. This was done to provide a guide for what the Rust API should look like and avoid any rough C binding work from being exposed to Rust HID device drivers. To go forward with this driver for the purpose of external monitor backlight control, a new DRM backlight API that is scoped per connector will be required. I am currently in the process of developing this new API. I document the details in my related blog posts. The issue with the current backlight API is it was designed on the assumption that only internal panels have controllable backlights. Using this assumption combined with another that there can only ever be a single internal panel, having more than one device register with the backlight interface would confuse userspace applications. Julius Zint originally tried to implement such a driver a bit more than a year ago with a C driver but was blocked by the limitations of the backlight API. I asked him for permission to continue the work in Rust while accrediting him for the HID report parsing logic for the backlight support in the USB Monitor Control Class specification. Cc: Julius Zint <julius@zint.sh> Link: https://lore.kernel.org/lkml/20230820094118.20521-1-julius@zint.sh/ Link: https://binary-eater.github.io/posts/linux_usb_monitor_control/ Link: https://www.usb.org/sites/default/files/usbmon11.pdf Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com> --- drivers/hid/Kconfig | 8 +++++++ drivers/hid/Makefile | 1 + drivers/hid/hid_monitor_control.rs | 37 ++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 drivers/hid/hid_monitor_control.rs