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

[Fiona Behrens <me@kloenk.dev>]

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