My team has created a simple driver to control the Activity LED on Dell
laptops intended for the Education market. The Activity LED is visible
externally in the lid so Teachers can observe it from their desks. This
driver works on the shipping Latitude 2100 series platforms as well as
others to be released in the future. The driver follows the existing LED
class driver API (leds-class.txt), so it will easily allow anybody to
write an application to control the LED. Attached is dell_led.c
This has been internally reviewed, and we are ready for outside review
and feedback. My colleagues have identified the dell-wmi module as a
suitable container in lieu of a stand-alone module specifically for this
driver, which makes sense, but we welcome advice. We are submitting it
as a stand-alone module for now because that is how we developed and
tested it. We would like this to be included upstream after it has been
reviewed.
We look forward to your feedback. Thanks in advance.
Regards,
Bob Rodgers
Engineering Lead, Dell LED Control Project
Direct Tel: (512) 725-0665
Direct FAX: (512) 283-8994
On Mon, Feb 01, 2010 at 04:44:36PM -0600, Bob Rodgers wrote:
> This has been internally reviewed, and we are ready for outside review
> and feedback. My colleagues have identified the dell-wmi module as a
> suitable container in lieu of a stand-alone module specifically for this
> driver, which makes sense, but we welcome advice. We are submitting it
> as a stand-alone module for now because that is how we developed and
> tested it. We would like this to be included upstream after it has been
> reviewed.
It uses a different GUID to the event interface used by dell-wmi, so
right now there's no inherent reason to integrate it into that rather
than keeping it as a separate driver. On the other hand, if the GUID is
useful for other kinds of system control rather than just the LED then
dell-wmi may want to make use of that functionality in the future. In
that case we'd need it to be incorporated into the dell-wmi driver.
So, really, it depends on the interface. If this GUID is specific to
LEDs, then keep it separate. Otherwise it should be integrated.
I've got a few comments on the code...
> // Error Result Codes:
C99 style comments are usually discouraged in the kernel.
> // Devide ID
Typo?
> // LED Commands
> #define CMD_LED_ON 16
> #define CMD_LED_OFF 17
> #define CMD_LED_BLINK 18
Use of whitespace isn't very consistent here.
> struct bios_args {
> unsigned char Length;
> unsigned char ResultCode;
> unsigned char DeviceId;
> unsigned char Command;
> unsigned char OnTime;
> unsigned char OffTime;
> unsigned char Reserved[122];
> };
Mm. We're also not usually big on CamelCasing in variable names - it'd
be preferable to use underscores. That's true for the rest of this, too.
> // free the output ACPI object allocated by ACPI driver
Probably don't need this comment.
> static void led_on(void)
> {
> dell_led_perform_fn(3, // Length of command
> INTERFACE_ERROR, // Init result code to INTERFACE_ERROR
> DEVICE_ID_PANEL_BACK, // Device ID
> CMD_LED_ON, // Command
> 0, // not used
> 0); // not used
> }
Whitespace is a bit odd here, again.
Other than that, it looks good. You probably want to run it through
Scripts/checkpatch.pl in the kernel tree to perform further style
checks, but I can't see any functional issues.
--
Matthew Garrett | [email protected]
On Mon, Feb 01, 2010 at 04:44:36PM -0600, Bob Rodgers wrote:
> My team has created a simple driver to control the Activity LED on Dell
> laptops intended for the Education market. The Activity LED is visible
> externally in the lid so Teachers can observe it from their desks. This
> driver works on the shipping Latitude 2100 series platforms as well as
> others to be released in the future. The driver follows the existing LED
> class driver API (leds-class.txt), so it will easily allow anybody to
> write an application to control the LED. Attached is dell_led.c
>
> This has been internally reviewed, and we are ready for outside review
> and feedback. My colleagues have identified the dell-wmi module as a
> suitable container in lieu of a stand-alone module specifically for this
> driver, which makes sense, but we welcome advice. We are submitting it
> as a stand-alone module for now because that is how we developed and
> tested it. We would like this to be included upstream after it has been
> reviewed.
>
> We look forward to your feedback. Thanks in advance.
>
> Regards,
> Bob Rodgers
> Engineering Lead, Dell LED Control Project
> Direct Tel: (512) 725-0665
> Direct FAX: (512) 283-8994
>
> /*
> * dell_led.c - Dell LED Driver
> *
> * Copyright (C) 2010 Dell Inc.
> * Louis Davis <[email protected]>
> * Jim Dailey <[email protected]>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as
> * published by the Free Software Foundation.
> *
> */
>
> #include <linux/platform_device.h>
> #include <linux/acpi.h>
> #include <linux/leds.h>
>
> MODULE_AUTHOR("Louis Davis/Jim Dailey");
> MODULE_DESCRIPTION("Dell LED Control Driver");
> MODULE_LICENSE("GPL");
>
> #define DELL_LED_BIOS_GUID "F6E4FE6E-909D-47cb-8BAB-C9F6F2F8D396"
> MODULE_ALIAS("wmi:" DELL_LED_BIOS_GUID);
>
> // Error Result Codes:
> #define INVALID_DEVICE_ID 250
> #define INVALID_PARAMETER 251
> #define INVALID_BUFFER 252
> #define INTERFACE_ERROR 253
> #define UNSUPPORTED_COMMAND 254
> #define UNSPECIFIED_ERROR 255
>
> // Devide ID
> #define DEVICE_ID_PANEL_BACK 1
>
> // LED Commands
> #define CMD_LED_ON 16
> #define CMD_LED_OFF 17
> #define CMD_LED_BLINK 18
>
> struct bios_args {
> unsigned char Length;
> unsigned char ResultCode;
> unsigned char DeviceId;
> unsigned char Command;
> unsigned char OnTime;
> unsigned char OffTime;
> unsigned char Reserved[122];
> };
>
> static int dell_led_perform_fn(u8 Length, u8 ResultCode, u8 DeviceId, u8 Command, u8 OnTime, u8 OffTime)
> {
> struct bios_args bios_return;
It would be better to not put the bios_return struct on the stack. Make it a
pointer and use kmalloc().
It's a pity the Makefile bits weren't included.
regards,
dan carpenter
Looks good to me. I guess this could go through either the ACPI or LED
maintainers, so probably best to submit it to both of them
([email protected] and [email protected]). Feel free to add
Acked-by: Matthew Garrett <[email protected]>
--
Matthew Garrett | [email protected]
On Mon, Feb 01, 2010 at 04:44:36PM -0600, Bob Rodgers wrote:
> This has been internally reviewed, and we are ready for outside review
> and feedback. My colleagues have identified the dell-wmi module as a
> suitable container in lieu of a stand-alone module specifically for
> this driver, which makes sense, but we welcome advice. We are
> submitting it as a stand-alone module for now because that is how we
> developed and tested it. We would like this to be included upstream
> after it has been reviewed.
On Mon, Feb 01, 2010 at 5:02 PM, Matthew Garrett wrote:
> It uses a different GUID to the event interface used by dell-wmi,
> so right now there's no inherent reason to integrate it into that
> rather than keeping it as a separate driver. On the other hand,
> if the GUID is useful for other kinds of system control rather
> than just the LED then dell-wmi may want to make use of that
> functionality in the future. In that case we'd need it to be
> incorporated into the dell-wmi driver.
>
> So, really, it depends on the interface. If this GUID is specific to
LEDs,
> then keep it separate. Otherwise it should be integrated.
>
> I've got a few comments on the code...
>
> > // Error Result Codes:
>
> C99 style comments are usually discouraged in the kernel.
>
> > // Devide ID
>
> Typo?
>
> > // LED Commands
> > #define CMD_LED_ON 16
> > #define CMD_LED_OFF 17
> > #define CMD_LED_BLINK 18
>
> Use of whitespace isn't very consistent here.
>
> > struct bios_args {
> > unsigned char Length;
> > unsigned char ResultCode;
> > unsigned char DeviceId;
> > unsigned char Command;
> > unsigned char OnTime;
> > unsigned char OffTime;
> > unsigned char Reserved[122];
> > };
> Mm. We're also not usually big on CamelCasing in variable names -
> it'd be preferable to use underscores. That's true for the rest of
this, too.
>
> > // free the output ACPI object allocated by ACPI driver
>
> Probably don't need this comment.
>
> > static void led_on(void)
> > {
> > dell_led_perform_fn(3, // Length of command
> > INTERFACE_ERROR, // Init result code to INTERFACE_ERROR
> > DEVICE_ID_PANEL_BACK, // Device ID
> > CMD_LED_ON, // Command
> > 0, // not used
> > 0); // not used
> > }
>
> Whitespace is a bit odd here, again.
>
> Other than that, it looks good. You probably want to run it
> through Scripts/checkpatch.pl in the kernel tree to perform
> further style checks, but I can't see any functional issues.
> --
On Tue, Feb 02, 2010 at 6:15 AM, Dan Carpenter wrote:
> It would be better to not put the bios_return struct on the stack.
> Make it a pointer and use kmalloc().
>
> It's a pity the Makefile bits weren't included.
Thank you for all the feedback. We have reviewed the feedback and made
the recommended changes/corrections.
> So, really, it depends on the interface. If this GUID is specific to
LEDs,
> then keep it separate. Otherwise it should be integrated.
Since the GUID is specific to LEDs, we will keep the driver separate
rather than integrate it into the dell-wmi module.
> C99 style comments are usually discouraged in the kernel.
Removed.
> > // Devide ID
>
> Typo?
Yes. Fixed.
> Use of whitespace isn't very consistent here.
Fixed.
> Mm. We're also not usually big on CamelCasing in variable names -
> it'd be preferable to use underscores. That's true for the rest of
this, too.
Corrected. Changed to underscores.
> > // free the output ACPI object allocated by ACPI driver
>
> Probably don't need this comment.
Removed.
> > CMD_LED_ON, // Command
> > 0, // not used
> > 0); // not used
> > }
>
> Whitespace is a bit odd here, again.
Fixed.
> Other than that, it looks good. You probably want to run it
> through Scripts/checkpatch.pl in the kernel tree to perform
> further style checks, but I can't see any functional issues.
We ran the file through Scripts/checkpatch.pl and the script reported 0
errors and 0 warnings.
> It would be better to not put the bios_return struct on the stack.
> Make it a pointer and use kmalloc().
Changed to a pointer.
> It's a pity the Makefile bits weren't included.
The Makefile is now included.
The updated dell_led.c file and the Makefile are attached.
Regards,
Bob Rodgers
Louis Davis
Hi Bob,
On Tue, Feb 02, 2010 at 03:27:11PM -0600, Bob Rodgers wrote:
>
> static int __init dell_led_probe(struct platform_device *pdev)
> {
This should either be changed to __devinit or you need to call
platform_device_probe() or, even better, use platform_create_bundle()
that is in next.
But isn't it a bit wasteful to create a brand new platform device only
to attach a single led device to it? I think that, even thourgh LED GUID
is separate, it would be better to keep all this functionality in
dell-wmi driver.
Thanks.
--
Dmitry
On Tue, Feb 02, 2010 at 03:27:11PM -0600, Bob Rodgers wrote:
> On Mon, Feb 01, 2010 at 04:44:36PM -0600, Bob Rodgers wrote:
>
> > This has been internally reviewed, and we are ready for outside review
>
Looks good to me.
regards,
dan carpenter