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