Re: [PATCH] v2 - Add Dell Business Class Netbook LED driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Bob,

On Fri, Feb 12, 2010 at 08:11:07AM -0600, Bob Rodgers wrote:
> This patch adds an LED driver to support the Dell Activity LED on the 
> Dell Latitude 2100 netbook and future products to come. The Activity LED 
> is visible externally in the lid so classroom instructors can observe it 
> from a distance. The driver uses the sysfs led_class and provides a 
> standard LED interface. This driver is ready for submission upstream.
> 
> Signed-off by: Bob Rodgers <Robert_Rodgers@dell.com>
> Signed-off-by: Louis Davis <Louis_Davis@dell.com>
> Signed-off-by: Jim Dailey <Jim_Dailey@dell.com>, Developers
> Acked-by: Matthew Garrett <mjg@redhat.com>
> 
> ---
> Description of changes in v2:
> 1) Added X86 and ACPI_WMI dependencies to the Kconfig file.
> 2) Removed platform_driver and platform_device as they are not needed in this driver.
> 

With the platform device stiff gone it looks much better, still a few
things:

> +
> +#include <linux/platform_device.h>

Do you still need 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
> +
> +/* Device 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 result_code;
> +	unsigned char device_id;
> +	unsigned char command;
> +	unsigned char on_time;
> +	unsigned char off_time;
> +};
> +
> +static u8 dell_led_perform_fn(u8 length,
> +		u8 result_code,
> +		u8 device_id,
> +		u8 command,
> +		u8 on_time,
> +		u8 off_time)
> +{
> +	struct bios_args *bios_return;
> +	u8 return_code;
> +	union acpi_object *obj;
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	struct acpi_buffer input;
> +
> +	struct bios_args args;
> +	args.length = length;
> +	args.result_code = result_code;
> +	args.device_id = device_id;
> +	args.command = command;
> +	args.on_time = on_time;
> +	args.off_time = off_time;
> +
> +	input.length = sizeof(struct bios_args);
> +	input.pointer = &args;
> +
> +	wmi_evaluate_method(DELL_LED_BIOS_GUID,
> +		1,
> +		1,
> +		&input,
> +		&output);

This needs error handling.

> +
> +	obj = output.pointer;
> +
> +	if (!obj || obj->type != ACPI_TYPE_BUFFER)
> +		return -EINVAL;

You are leaking obj when obj is not NULL and is not a buffer.

> +
> +static int __init dell_led_init(void)
> +{
> +	int error = 0;
> +
> +	if (!wmi_has_guid(DELL_LED_BIOS_GUID)) {
> +		printk(KERN_DEBUG KBUILD_MODNAME
> +			": could not find: DELL_LED_BIOS_GUID\n");

Please ocnsider using pr_xxx() family.

> +		return -ENODEV;
> +	}
> +
> +	error = led_off();
> +	if (error != 0) {
> +		printk(KERN_DEBUG KBUILD_MODNAME

This is a warning or an error, not a debug.

Thank you.

-- 
Dmitry