Re: Re: [RFC] Dell activity led WMI driver

Re: Re: [RFC] Dell activity led WMI driver

[Bob Rodgers]

[-- Attachment #1: Type: text/plain, Size: 4384 bytes --]

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




[-- Attachment #2: Makefile --]
[-- Type: text/plain, Size: 304 bytes --]

ifneq ($(KERNELRELEASE),)
# call from kernel build system

obj-m := dell_led.o

else

KERNELDIR ?= /lib/modules/$(shell uname -r)/build
PWD := $(shell pwd)

default:
	$(MAKE) -C $(KERNELDIR) M=$(PWD) modules

endif

clean:
	rm -rf *.o *~ core .depend .*.cmd *.ko *.mod.c .tmp_versions modules.* Module.*

[-- Attachment #3: dell_led.c --]
[-- Type: text/plain, Size: 5115 bytes --]

/*
 * 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

/* 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);

	obj = output.pointer;

	if (!obj || obj->type != ACPI_TYPE_BUFFER)
		return -EINVAL;

	bios_return = ((struct bios_args *)obj->buffer.pointer);
	return_code = bios_return->result_code;

	kfree(obj);

	return return_code;
}

static u8 led_on(void)
{
	return dell_led_perform_fn(3,	/* Length of command */
		INTERFACE_ERROR,	/* Init to  INTERFACE_ERROR */
		DEVICE_ID_PANEL_BACK,	/* Device ID */
		CMD_LED_ON,		/* Command */
		0,			/* not used */
		0);			/* not used */
}

static u8 led_off(void)
{
	return dell_led_perform_fn(3,	/* Length of command */
		INTERFACE_ERROR,	/* Init to  INTERFACE_ERROR */
		DEVICE_ID_PANEL_BACK,	/* Device ID */
		CMD_LED_OFF,		/* Command */
		0,			/* not used */
		0);			/* not used */
}

static u8 led_blink(unsigned char on_eighths,
		unsigned char off_eighths)
{
	return dell_led_perform_fn(5,	/* Length of command */
		INTERFACE_ERROR,	/* Init to  INTERFACE_ERROR */
		DEVICE_ID_PANEL_BACK,	/* Device ID */
		CMD_LED_BLINK,		/* Command */
		on_eighths,		/* blink on in eigths of a second */
		off_eighths);		/* blink off in eights of a second */
}

static void dell_led_set(struct led_classdev *led_cdev,
		enum led_brightness value)
{
	if (value == LED_OFF)
		led_off();
	else
		led_on();
}

static int dell_led_blink(struct led_classdev *led_cdev,
		unsigned long *delay_on,
		unsigned long *delay_off)
{
	unsigned long on_eighths;
	unsigned long off_eighths;

	/* The Dell LED delay is based on 125ms intervals.
	   Need to round up to next interval. */

	on_eighths = (*delay_on + 124) / 125;
	if (0 == on_eighths)
		on_eighths = 1;
	if (on_eighths > 255)
		on_eighths = 255;
	*delay_on = on_eighths * 125;

	off_eighths = (*delay_off + 124) / 125;
	if (0 == off_eighths)
		off_eighths = 1;
	if (off_eighths > 255)
		off_eighths = 255;
	*delay_off = off_eighths * 125;

	led_blink(on_eighths, off_eighths);

	return 0;
}

static struct led_classdev dell_led = {
	.name		= "dell::lid",
	.brightness	= LED_OFF,
	.max_brightness = 1,
	.brightness_set = dell_led_set,
	.blink_set	= dell_led_blink,
	.flags		= LED_CORE_SUSPENDRESUME,
};

static int __init dell_led_probe(struct platform_device *pdev)
{
	return led_classdev_register(&pdev->dev, &dell_led);
}

static int dell_led_remove(struct platform_device *pdev)
{
	led_classdev_unregister(&dell_led);
	return 0;
}

static struct platform_driver dell_led_driver = {
	.probe		= dell_led_probe,
	.remove		= dell_led_remove,
	.driver		= {
		.name	= KBUILD_MODNAME,
		.owner	= THIS_MODULE,
	},
};

static struct platform_device *pdev;

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");
		return -ENODEV;
	}

	error = led_off();
	if (error != 0) {
		printk(KERN_DEBUG KBUILD_MODNAME
			": could not communicate with LED"
			": error %d\n", error);
		return -ENODEV;
	}

	error = platform_driver_register(&dell_led_driver);
	if (error < 0)
		return error;

	pdev = platform_device_register_simple(KBUILD_MODNAME, -1, NULL, 0);
	if (IS_ERR(pdev)) {
		error = PTR_ERR(pdev);
		platform_driver_unregister(&dell_led_driver);
	}

	return error;
}

static void __exit dell_led_exit(void)
{
	platform_driver_unregister(&dell_led_driver);
	platform_device_unregister(pdev);

	led_off();
}

module_init(dell_led_init);
module_exit(dell_led_exit);

Re: Re: [RFC] Dell activity led WMI driver

[Dmitry Torokhov]

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

Re: Re: [RFC] Dell activity led WMI driver

[Dan Carpenter]

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

Re: Re: [RFC] Dell activity led WMI driver

[Matthew Garrett]

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

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org