Re: [PATCH] led: simplify led_trigger_register_simple

From: Felipe Balbi
Date: Thu Nov 20 2008 - 08:11:29 EST

On Thu, Nov 13, 2008 at 09:10:50PM +0200, ext Felipe Balbi wrote:
> On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote:
> > On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote:
> > > The simple triggers were designed to cause minimum interference to the
> > > usually external subsystem code they were added into. As an example this
> > > meant things like errors were just handled gracefully with a printk
> > > warning and did not take down the whole subsystem. I therefore don't
> > > regard this patch as a simplification, more a complication.
> >
> > That's a matter of changing the return ERR_PTR(err); back to a printk.
>
> And here you are. I still think we should at least kfree(trigger) in
> case of error, though.

Any comments here ??

> ======= cut here ===========
>
> >From 4a96a53109f1edc3277ffb2d22641330930e60cd Mon Sep 17 00:00:00 2001
> From: Felipe Balbi <felipe.balbi@xxxxxxxxx>
> Date: Thu, 13 Nov 2008 03:28:22 +0200
> Subject: [PATCH] led: simplify led_trigger_register_simple
>
> We can make led_trigger_register_simple by returning a
> struct led_trigger *, instead of passing a struct led_trigger **
> as a parameter and changing it inside the function.
>
> Cc: Anton Vorontsov <cbou@xxxxxxx>
> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> Cc: Greg KH <greg@xxxxxxxxx>
> Cc: Pierre Ossman <drzeus@xxxxxxxxx>
> Cc: Richard Purdie <rpurdie@xxxxxxxxxxxxxxx>
> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxx>
> ---
> drivers/leds/led-triggers.c | 4 ++--
> drivers/leds/ledtrig-ide-disk.c | 2 +-
> drivers/mmc/core/host.c | 2 +-
> drivers/mtd/nand/nand_base.c | 2 +-
> drivers/power/power_supply_leds.c | 13 ++++++-------
> drivers/staging/at76_usb/at76_usb.c | 2 +-
> include/linux/leds.h | 5 ++---
> 7 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index f910eaf..4304278 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -222,7 +222,7 @@ void led_trigger_event(struct led_trigger *trigger,
> }
> EXPORT_SYMBOL_GPL(led_trigger_event);
>
> -void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +struct led_trigger *led_trigger_register_simple(const char *name)
> {
> struct led_trigger *trigger;
> int err;
> @@ -239,7 +239,7 @@ void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> printk(KERN_WARNING "LED trigger %s failed to register"
> " (no memory)\n", name);
>
> - *tp = trigger;
> + return trigger;
> }
> EXPORT_SYMBOL_GPL(led_trigger_register_simple);
>
> diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
> index 883a577..750e166 100644
> --- a/drivers/leds/ledtrig-ide-disk.c
> +++ b/drivers/leds/ledtrig-ide-disk.c
> @@ -46,7 +46,7 @@ static void ledtrig_ide_timerfunc(unsigned long data)
>
> static int __init ledtrig_ide_init(void)
> {
> - led_trigger_register_simple("ide-disk", &ledtrig_ide);
> + ledtrig_ide = led_trigger_register_simple("ide-disk");
> return 0;
> }
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 5e945e6..8b337de 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -120,7 +120,7 @@ int mmc_add_host(struct mmc_host *host)
> WARN_ON((host->caps & MMC_CAP_SDIO_IRQ) &&
> !host->ops->enable_sdio_irq);
>
> - led_trigger_register_simple(dev_name(&host->class_dev), &host->led);
> + host->led = led_trigger_register_simple(dev_name(&host->class_dev));
>
> err = device_add(&host->class_dev);
> if (err)
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 0a9c9cd..9c5f0e2 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2781,7 +2781,7 @@ EXPORT_SYMBOL_GPL(nand_release);
>
> static int __init nand_base_init(void)
> {
> - led_trigger_register_simple("nand-disk", &nand_led_trigger);
> + nand_led_trigger = led_trigger_register_simple("nand-disk");
> return 0;
> }
>
> diff --git a/drivers/power/power_supply_leds.c b/drivers/power/power_supply_leds.c
> index 2dece40..da03990 100644
> --- a/drivers/power/power_supply_leds.c
> +++ b/drivers/power/power_supply_leds.c
> @@ -63,12 +63,11 @@ static int power_supply_create_bat_triggers(struct power_supply *psy)
> if (!psy->full_trig_name)
> goto full_failed;
>
> - led_trigger_register_simple(psy->charging_full_trig_name,
> - &psy->charging_full_trig);
> - led_trigger_register_simple(psy->charging_trig_name,
> - &psy->charging_trig);
> - led_trigger_register_simple(psy->full_trig_name,
> - &psy->full_trig);
> + psy->charging_full_trig = led_trigger_register_simple(
> + psy->charging_full_trig_name);
> + psy->charging_trig = led_trigger_register_simple(
> + psy->charging_trig_name);
> + psy->full_trig = led_trigger_register_simple(psy->full_trig_name);
>
> goto success;
>
> @@ -117,7 +116,7 @@ static int power_supply_create_gen_triggers(struct power_supply *psy)
> if (!psy->online_trig_name)
> goto online_failed;
>
> - led_trigger_register_simple(psy->online_trig_name, &psy->online_trig);
> + psy->online_trig = led_trigger_register_simple(psy->online_trig_name);
>
> goto success;
>
> diff --git a/drivers/staging/at76_usb/at76_usb.c b/drivers/staging/at76_usb/at76_usb.c
> index 174e2be..ae6dc6e 100644
> --- a/drivers/staging/at76_usb/at76_usb.c
> +++ b/drivers/staging/at76_usb/at76_usb.c
> @@ -5528,7 +5528,7 @@ static int __init at76_mod_init(void)
> printk(KERN_ERR DRIVER_NAME
> ": usb_register failed (status %d)\n", result);
>
> - led_trigger_register_simple("at76_usb-tx", &ledtrig_tx);
> + ledtrig_tx = led_trigger_register_simple("at76_usb-tx");
> return result;
> }
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index d3a73f5..b796ec0 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -94,8 +94,7 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
> /* Registration functions for simple triggers */
> #define DEFINE_LED_TRIGGER(x) static struct led_trigger *x;
> #define DEFINE_LED_TRIGGER_GLOBAL(x) struct led_trigger *x;
> -extern void led_trigger_register_simple(const char *name,
> - struct led_trigger **trigger);
> +extern struct led_trigger *led_trigger_register_simple(const char *name);
> extern void led_trigger_unregister_simple(struct led_trigger *trigger);
> extern void led_trigger_event(struct led_trigger *trigger,
> enum led_brightness event);
> @@ -105,7 +104,7 @@ extern void led_trigger_event(struct led_trigger *trigger,
> /* Triggers aren't active - null macros */
> #define DEFINE_LED_TRIGGER(x)
> #define DEFINE_LED_TRIGGER_GLOBAL(x)
> -#define led_trigger_register_simple(x, y) do {} while(0)
> +#define led_trigger_register_simple(x) NULL
> #define led_trigger_unregister_simple(x) do {} while(0)
> #define led_trigger_event(x, y) do {} while(0)
>
> --
> 1.6.0.4.617.g2baf1
>
> --
> balbi

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