[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <200709140053.l8E0rPb4003095@po-mbox303.hop.2iij.net>
Date: Fri, 14 Sep 2007 09:53:25 +0900
From: Yoichi Yuasa <yoichi_yuasa@...peaks.co.jp>
To: Richard Purdie <rpurdie@...ys.net>
Cc: yoichi_yuasa@...peaks.co.jp, linux-kernel@...r.kernel.org
Subject: Re: [PATCH][1/2] led: add Cobalt Raq LEDs support
On Thu, 13 Sep 2007 22:32:12 +0100
Richard Purdie <rpurdie@...ys.net> wrote:
> On Thu, 2007-09-13 at 23:54 +0900, Yoichi Yuasa wrote:
> > Add Cobalt Raq LEDs support.
> >
> > Signed-off-by: Yoichi Yuasa <yoichi_yuasa@...peaks.co.jp>
>
> 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...
OK, I'll split it.
> > 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?
It lights when the Cobalt Raq is possible to turn off power.
>
> > +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?
This driver cares power-off LED.
Therefore, this driver is not tristate.
> Some of these comments also apply to you second patch. All minor tweaks
> really though :).
Thank you for your comments.
Yoichi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists