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

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

Hi

On Fri, Jan 17, 2025, at 12:31 PM, Fiona Behrens wrote:
> 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

Ah - thanks for the clarification (and to Miguel in the follow up)

>
>>
>>>  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.
>
Ah - I should probably have known that :) All good.

Mark