| Message ID | 20250113121620.21598-6-me@kloenk.dev |
|---|---|
| State | New |
| Headers | show |
| Series | Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support | expand |
Hi, On 17 Jan 2025, at 18:21, Mark Pearson wrote: > Hi Fiona, > > On Mon, Jan 13, 2025, at 7:16 AM, Fiona Behrens wrote: >> Add driver for the Lenovo ThinkEdge SE10 LED. >> >> This driver supports controlling the red LED located on the front panel of the >> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered >> functionality of the LED, which by default is tied to the WWAN trigger. >> >> The driver is written in Rust and adds basic LED support for the SE10 platform. >> >> Signed-off-by: Fiona Behrens <me@kloenk.dev> >> --- >> drivers/leds/Kconfig | 10 +++ >> drivers/leds/Makefile | 1 + >> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++ > > All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs? This does not work with rust, as the rust makefile converts this filename to a rust crate name, and those crate name cannot have dashes in them. Not sure if we should fix this to hold to the file name conventions, maybe something for @Miguel to decide > >> 3 files changed, 143 insertions(+) >> create mode 100644 drivers/leds/leds_lenovo_se10.rs >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index b784bb74a837..89d9e98189d6 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA >> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the >> front panel. >> >> +config LEDS_LENOVO_SE10 >> + tristate "LED support for Lenovo ThinkEdge SE10" >> + depends on RUST >> + depends on (X86 && DMI) || COMPILE_TEST >> + depends on HAS_IOPORT >> + imply LEDS_TRIGGERS >> + help >> + This option enables basic support for the LED found on the front of >> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the >> front panel. >> + >> config LEDS_LM3530 >> tristate "LCD Backlight driver for LM3530" >> depends on LEDS_CLASS >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 18afbb5a23ee..2cff22cbafcf 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o >> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o >> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o >> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o >> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o > > Same note above on name formatting. > >> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o >> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o >> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o >> diff --git a/drivers/leds/leds_lenovo_se10.rs >> b/drivers/leds/leds_lenovo_se10.rs >> new file mode 100644 >> index 000000000000..d704125610a4 >> --- /dev/null >> +++ b/drivers/leds/leds_lenovo_se10.rs >> @@ -0,0 +1,132 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +//! LED driver for Lenovo ThinkEdge SE10. >> + >> +use kernel::ioport::{Region, ResourceSize}; >> +#[cfg(CONFIG_LEDS_TRIGGERS)] >> +use kernel::leds::triggers; >> +use kernel::leds::{Led, LedConfig, Operations}; >> +use kernel::prelude::*; >> +use kernel::time::Delta; >> +use kernel::{c_str, dmi_device_table}; >> + >> +module! { >> + type: SE10, >> + name: "leds_lenovo_se10", >> + author: "Fiona Behrens <me@kloenk.dev>", >> + description: "LED driver for Lenovo ThinkEdge SE10", >> + license: "GPL", >> +} >> + >> +dmi_device_table!(5, SE10_DMI_TABLE, [ >> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"], >> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"], >> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"], >> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"], >> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"], >> +]); >> + >> +struct SE10 { >> + /// Led registration >> + _led: Pin<KBox<Led<LedSE10>>>, >> +} >> + >> +impl kernel::Module for SE10 { >> + fn init(_module: &'static ThisModule) -> Result<Self> { >> + if SE10_DMI_TABLE.check_system().is_none() { >> + return Err(ENODEV); >> + } >> + >> + let led = KBox::try_pin_init( >> + Led::register( >> + None, >> + LedConfig { >> + name: Some(c_str!("platform:red:user")), >> + #[cfg(CONFIG_LEDS_TRIGGERS)] >> + hardware_trigger: Some(kernel::sync::Arc::pin_init( >> + triggers::Hardware::register(c_str!("wwan")), > > I was curious as to why the "wwan" in here. This is the hardware trigger, as to the documentation I found from Lenovo the trigger mode gives hardware control to the wwan module if installed in the hardware. > >> + GFP_KERNEL, >> + )?), >> + ..LedConfig::new(kernel::leds::Color::Red, LedSE10) >> + }, >> + ), >> + GFP_KERNEL, >> + )?; >> + >> + Ok(Self { _led: led }) >> + } >> +} >> + >> +/// Valid led commands. >> +#[repr(u8)] >> +#[allow(missing_docs)] >> +enum LedCommand { >> + #[cfg(CONFIG_LEDS_TRIGGERS)] >> + Trigger = 0xB2, >> + Off = 0xB3, >> + On = 0xB4, >> + Blink = 0xB5, >> +} >> + >> +struct LedSE10; >> + >> +impl LedSE10 { >> + /// Base address of the command port. >> + const CMD_PORT: ResourceSize = 0x6C; >> + /// Length of the command port. >> + const CMD_LEN: ResourceSize = 1; >> + /// Blink duration the hardware supports. >> + const HW_DURATION: Delta = Delta::from_millis(1000); >> + >> + /// Request led region. >> + fn request_cmd_region(&self) -> Result<Region<'static>> { >> + Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN, >> c_str!("leds_lenovo_se10")) >> + .ok_or(EBUSY) >> + } >> + >> + /// Send command. >> + fn send_cmd(&self, cmd: LedCommand) -> Result { >> + let region = self.request_cmd_region()?; >> + region.outb(cmd as u8, 0); >> + Ok(()) >> + } >> +} >> + >> +#[vtable] >> +impl Operations for LedSE10 { >> + type This = Led<LedSE10>; >> + >> + const MAX_BRIGHTNESS: u8 = 1; >> + >> + fn brightness_set(this: &mut Self::This, brightness: u8) { >> + if let Err(e) = if brightness == 0 { >> + this.data.send_cmd(LedCommand::Off) >> + } else { >> + this.data.send_cmd(LedCommand::On) >> + } { >> + pr_warn!("Failed to set led: {e:?}\n)") >> + } >> + } >> + >> + fn blink_set( >> + this: &mut Self::This, >> + delay_on: Delta, >> + delay_off: Delta, >> + ) -> Result<(Delta, Delta)> { >> + if !(delay_on.is_zero() && delay_off.is_zero() >> + || delay_on == Self::HW_DURATION && delay_off == >> Self::HW_DURATION) >> + { >> + return Err(EINVAL); >> + } >> + >> + this.data.send_cmd(LedCommand::Blink)?; >> + Ok((Self::HW_DURATION, Self::HW_DURATION)) >> + } >> +} >> + >> +#[vtable] >> +#[cfg(CONFIG_LEDS_TRIGGERS)] >> +impl triggers::HardwareOperations for LedSE10 { >> + fn activate(this: &mut Self::This) -> Result { >> + this.data.send_cmd(LedCommand::Trigger) >> + } >> +} >> -- >> 2.47.0 > > I don't have the competence to review the rust code I'm afraid - so my limited feedback above is the best I can do. Not sure it's really worth a reviewed-by tag, but I did read the code and learnt a little about rust in the process (which was fun). > > I did test your changes on my SE10 system and it works well. > Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca> Thanks a lot, Fiona > > Thanks! > Mark
Hi On Fri, Jan 17, 2025, at 12:31 PM, Fiona Behrens wrote: > Hi, > > On 17 Jan 2025, at 18:21, Mark Pearson wrote: > >> Hi Fiona, >> >> On Mon, Jan 13, 2025, at 7:16 AM, Fiona Behrens wrote: >>> Add driver for the Lenovo ThinkEdge SE10 LED. >>> >>> This driver supports controlling the red LED located on the front panel of the >>> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered >>> functionality of the LED, which by default is tied to the WWAN trigger. >>> >>> The driver is written in Rust and adds basic LED support for the SE10 platform. >>> >>> Signed-off-by: Fiona Behrens <me@kloenk.dev> >>> --- >>> drivers/leds/Kconfig | 10 +++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++ >> >> All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs? > > This does not work with rust, as the rust makefile converts this > filename to a rust crate name, and those crate name cannot have dashes > in them. > Not sure if we should fix this to hold to the file name conventions, > maybe something for @Miguel to decide Ah - thanks for the clarification (and to Miguel in the follow up) > >> >>> 3 files changed, 143 insertions(+) >>> create mode 100644 drivers/leds/leds_lenovo_se10.rs >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index b784bb74a837..89d9e98189d6 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA >>> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the >>> front panel. >>> >>> +config LEDS_LENOVO_SE10 >>> + tristate "LED support for Lenovo ThinkEdge SE10" >>> + depends on RUST >>> + depends on (X86 && DMI) || COMPILE_TEST >>> + depends on HAS_IOPORT >>> + imply LEDS_TRIGGERS >>> + help >>> + This option enables basic support for the LED found on the front of >>> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the >>> front panel. >>> + >>> config LEDS_LM3530 >>> tristate "LCD Backlight driver for LM3530" >>> depends on LEDS_CLASS >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index 18afbb5a23ee..2cff22cbafcf 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o >>> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o >>> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o >>> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o >>> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o >> >> Same note above on name formatting. >> >>> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o >>> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o >>> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o >>> diff --git a/drivers/leds/leds_lenovo_se10.rs >>> b/drivers/leds/leds_lenovo_se10.rs >>> new file mode 100644 >>> index 000000000000..d704125610a4 >>> --- /dev/null >>> +++ b/drivers/leds/leds_lenovo_se10.rs >>> @@ -0,0 +1,132 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +//! LED driver for Lenovo ThinkEdge SE10. >>> + >>> +use kernel::ioport::{Region, ResourceSize}; >>> +#[cfg(CONFIG_LEDS_TRIGGERS)] >>> +use kernel::leds::triggers; >>> +use kernel::leds::{Led, LedConfig, Operations}; >>> +use kernel::prelude::*; >>> +use kernel::time::Delta; >>> +use kernel::{c_str, dmi_device_table}; >>> + >>> +module! { >>> + type: SE10, >>> + name: "leds_lenovo_se10", >>> + author: "Fiona Behrens <me@kloenk.dev>", >>> + description: "LED driver for Lenovo ThinkEdge SE10", >>> + license: "GPL", >>> +} >>> + >>> +dmi_device_table!(5, SE10_DMI_TABLE, [ >>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"], >>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"], >>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"], >>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"], >>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"], >>> +]); >>> + >>> +struct SE10 { >>> + /// Led registration >>> + _led: Pin<KBox<Led<LedSE10>>>, >>> +} >>> + >>> +impl kernel::Module for SE10 { >>> + fn init(_module: &'static ThisModule) -> Result<Self> { >>> + if SE10_DMI_TABLE.check_system().is_none() { >>> + return Err(ENODEV); >>> + } >>> + >>> + let led = KBox::try_pin_init( >>> + Led::register( >>> + None, >>> + LedConfig { >>> + name: Some(c_str!("platform:red:user")), >>> + #[cfg(CONFIG_LEDS_TRIGGERS)] >>> + hardware_trigger: Some(kernel::sync::Arc::pin_init( >>> + triggers::Hardware::register(c_str!("wwan")), >> >> I was curious as to why the "wwan" in here. > > This is the hardware trigger, as to the documentation I found from > Lenovo the trigger mode gives hardware control to the wwan module if > installed in the hardware. > Ah - I should probably have known that :) All good. Mark
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b784bb74a837..89d9e98189d6 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the front panel. +config LEDS_LENOVO_SE10 + tristate "LED support for Lenovo ThinkEdge SE10" + depends on RUST + depends on (X86 && DMI) || COMPILE_TEST + depends on HAS_IOPORT + imply LEDS_TRIGGERS + help + This option enables basic support for the LED found on the front of + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the front panel. + config LEDS_LM3530 tristate "LCD Backlight driver for LM3530" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 18afbb5a23ee..2cff22cbafcf 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o diff --git a/drivers/leds/leds_lenovo_se10.rs b/drivers/leds/leds_lenovo_se10.rs new file mode 100644 index 000000000000..d704125610a4 --- /dev/null +++ b/drivers/leds/leds_lenovo_se10.rs @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: GPL-2.0 +//! LED driver for Lenovo ThinkEdge SE10. + +use kernel::ioport::{Region, ResourceSize}; +#[cfg(CONFIG_LEDS_TRIGGERS)] +use kernel::leds::triggers; +use kernel::leds::{Led, LedConfig, Operations}; +use kernel::prelude::*; +use kernel::time::Delta; +use kernel::{c_str, dmi_device_table}; + +module! { + type: SE10, + name: "leds_lenovo_se10", + author: "Fiona Behrens <me@kloenk.dev>", + description: "LED driver for Lenovo ThinkEdge SE10", + license: "GPL", +} + +dmi_device_table!(5, SE10_DMI_TABLE, [ + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"], + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"], + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"], + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"], + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"], +]); + +struct SE10 { + /// Led registration + _led: Pin<KBox<Led<LedSE10>>>, +} + +impl kernel::Module for SE10 { + fn init(_module: &'static ThisModule) -> Result<Self> { + if SE10_DMI_TABLE.check_system().is_none() { + return Err(ENODEV); + } + + let led = KBox::try_pin_init( + Led::register( + None, + LedConfig { + name: Some(c_str!("platform:red:user")), + #[cfg(CONFIG_LEDS_TRIGGERS)] + hardware_trigger: Some(kernel::sync::Arc::pin_init( + triggers::Hardware::register(c_str!("wwan")), + GFP_KERNEL, + )?), + ..LedConfig::new(kernel::leds::Color::Red, LedSE10) + }, + ), + GFP_KERNEL, + )?; + + Ok(Self { _led: led }) + } +} + +/// Valid led commands. +#[repr(u8)] +#[allow(missing_docs)] +enum LedCommand { + #[cfg(CONFIG_LEDS_TRIGGERS)] + Trigger = 0xB2, + Off = 0xB3, + On = 0xB4, + Blink = 0xB5, +} + +struct LedSE10; + +impl LedSE10 { + /// Base address of the command port. + const CMD_PORT: ResourceSize = 0x6C; + /// Length of the command port. + const CMD_LEN: ResourceSize = 1; + /// Blink duration the hardware supports. + const HW_DURATION: Delta = Delta::from_millis(1000); + + /// Request led region. + fn request_cmd_region(&self) -> Result<Region<'static>> { + Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN, c_str!("leds_lenovo_se10")) + .ok_or(EBUSY) + } + + /// Send command. + fn send_cmd(&self, cmd: LedCommand) -> Result { + let region = self.request_cmd_region()?; + region.outb(cmd as u8, 0); + Ok(()) + } +} + +#[vtable] +impl Operations for LedSE10 { + type This = Led<LedSE10>; + + const MAX_BRIGHTNESS: u8 = 1; + + fn brightness_set(this: &mut Self::This, brightness: u8) { + if let Err(e) = if brightness == 0 { + this.data.send_cmd(LedCommand::Off) + } else { + this.data.send_cmd(LedCommand::On) + } { + pr_warn!("Failed to set led: {e:?}\n)") + } + } + + fn blink_set( + this: &mut Self::This, + delay_on: Delta, + delay_off: Delta, + ) -> Result<(Delta, Delta)> { + if !(delay_on.is_zero() && delay_off.is_zero() + || delay_on == Self::HW_DURATION && delay_off == Self::HW_DURATION) + { + return Err(EINVAL); + } + + this.data.send_cmd(LedCommand::Blink)?; + Ok((Self::HW_DURATION, Self::HW_DURATION)) + } +} + +#[vtable] +#[cfg(CONFIG_LEDS_TRIGGERS)] +impl triggers::HardwareOperations for LedSE10 { + fn activate(this: &mut Self::This) -> Result { + this.data.send_cmd(LedCommand::Trigger) + } +}
Add driver for the Lenovo ThinkEdge SE10 LED. This driver supports controlling the red LED located on the front panel of the Lenovo SE10 hardware. Additionally, it supports the hardware-triggered functionality of the LED, which by default is tied to the WWAN trigger. The driver is written in Rust and adds basic LED support for the SE10 platform. Signed-off-by: Fiona Behrens <me@kloenk.dev> --- drivers/leds/Kconfig | 10 +++ drivers/leds/Makefile | 1 + drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 drivers/leds/leds_lenovo_se10.rs