Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

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?

>  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.

> +                        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!
Mark