Dell activity led WMI driver [Kernel]

Prev: [RFC] Dell activity led WMI driver
Next: linux-next: manual merge of the arm tree with the arm-current tree

From: Matthew Garrett on 1 Feb 2010 18:10

From: Dan Carpenter on 2 Feb 2010 07:20

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(a)dell.com>
> * Jim Dailey <jim_dailey(a)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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Matthew Garrett on 2 Feb 2010 16:10

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
(lenb(a)kernel.org and rpurdie(a)rpsys.net). Feel free to add

Acked-by: Matthew Garrett <mjg(a)redhat.com>

--
Matthew Garrett | mjg59(a)srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Bob Rodgers on 2 Feb 2010 16:30

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

From: Dmitry Torokhov on 2 Feb 2010 21:00

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

| Next | Last
Pages: 1 2
Prev: [RFC] Dell activity led WMI driver
Next: linux-next: manual merge of the arm tree with the arm-current tree