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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 13, 2025 at 01:16:20PM +0100, 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 +++++++++++++++++++++++++++++++
>  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
>  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")),

There are currently two LED drivers utilizing the led_hw_trigger_type
mechanism to make certain triggers available only for certain LEDs:
- the leds-cros_ec.c driver, which registers the trigger under the name
  "chromeos-auto", to suggest that activating the trigger on this LED
  will make it blink automatically by hardware and that it is ChromeOS
  specific,
- the leds-turris-omnia.c driver, which registers the trigger under the
  name "omnia-mcu", to suggest that activating the trigger will make the
  LED blinking be controller by the MCU on Turris Omnia.

Using the name "wwan" for this trigger is too general. In the future
someone may want to create a software "wwan" trigger that will be
available for any LED class device, for example...

Please change the name of this LED-private trigger.

> +                        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)
> +    }

No deactivation method for the trigger? NACK.

The driver must implement the deactivation method, since LED core
always allows disabling LED triggers. See led-trigger.c function
led_trigger_write(): if "none" is written to the sysfs `trigger`
file, the trigger is removed and the `trigger` file will afterwards
report that no trigger is activated on the LED.

Since you did not implement the deactivation method, this will result
in the system thinking that no LED trigger is selected on the LED,
but in fact your LED's blinking will still be controlled by hardware.

Marek




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux