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@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?
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@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.
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@xxxxxxxxx>
Thanks a lot,
Fiona
>
> Thanks!
> Mark
