On Thu, 2007-09-13 at 23:54 +0900, Yoichi Yuasa wrote:
> Add Cobalt Raq LEDs support.
>
> Signed-off-by: Yoichi Yuasa <[EMAIL PROTECTED]>
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
