Re: [RFC] Dell activity led WMI driver

[Dan Carpenter <error27@gmail.com>]

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 <louis_davis@dell.com>
>  * Jim Dailey <jim_dailey@dell.com>
>  *
>  * 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