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@xxxxxxxxxx>
> ---
> 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?
> 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@xxxxxxxxxx>",
> + 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.
> + 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@xxxxxxxxx>
Thanks!
Mark
