Re: [PATCH][1/2] led: add Cobalt Raq LEDs support

[Richard Purdie]

On Thu, 2007-09-13 at 23:54 +0900, Yoichi Yuasa wrote:
> Add Cobalt Raq LEDs support.
> 
> Signed-off-by: Yoichi Yuasa <yoichi_yuasa@xxxxxxxxxxxxxx>

Not the clearest patch I've ever seen or the most helpful patch
description. The rename could probably be split from the additional new
driver at least...

> diff -pruN -X mips/Documentation/dontdiff mips-orig/drivers/leds/leds-cobalt-raq.c mips/drivers/leds/leds-cobalt-raq.c
> --- mips-orig/drivers/leds/leds-cobalt-raq.c	1970-01-01 09:00:00.000000000 +0900
> +++ mips/drivers/leds/leds-cobalt-raq.c	2007-09-12 12:04:34.021000750 +0900
[...]
> +static void raq_web_led_set(struct led_classdev *led_cdev,
> +                            enum led_brightness brightness)
> +{
> +	spin_lock_irq(&raq_led_lock);
> +
> +	if (brightness)
> +		led_value |= LED_WEB;
> +	else
> +		led_value &= ~LED_WEB;
> +	writeb(led_value, led_port);
> +
> +	spin_unlock_irq(&raq_led_lock);
> +}

The _irq spinlock variants are not safe there or in the other brightness
set function...

+static struct led_classdev raq_web_led = {
> +	.name		= "raq-web-led",
> +	.brightness_set	= raq_web_led_set,
> +};
> +
[...]
> +static struct led_classdev raq_power_off_led = {
> +	.name			= "raq-power-off-led",
> +	.brightness_set		= raq_power_off_led_set,
> +	.default_trigger	= "power-off",
> +};

Do these really need "led" in the name?

What does a power-off trigger do with an LED out of interest?

> +static int __devexit cobalt_raq_led_remove(struct platform_device *pdev)
> +{
> +	if (led_port) {
> +		iounmap(led_port);
> +		led_port = NULL;
> +	}
> +
> +	led_classdev_unregister(&raq_power_off_led);
> +	led_classdev_unregister(&raq_web_led);
> +
> +	return 0;
> +}

You want to unregister, then iounmap...

> +static int __init cobalt_raq_led_init(void)
> +{
> +	return platform_driver_register(&cobalt_raq_led_driver);
> +}
> +
> +module_init(cobalt_raq_led_init);

No module_exit in one driver for any particular reason?

Some of these comments also apply to you second patch. All minor tweaks
really though :).

Cheers,

Richard

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