Re: [[RFC] 4/5] An LED "dimmer" trigger, which uses the PWM API to vary the brightness of an LED according to system load

[Mike Frysinger <vapier.adi@gmail.com>]

On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -1,153 +1,167 @@
> -/*
> - * linux/drivers/leds-pwm.c
> - *
> - * simple PWM based LED control
> - *
> - * Copyright 2009 Luotao Fu @ Pengutronix (l.fu@pengutronix.de)
> - *
> - * based on leds-gpio.c by Raphael Assenat <raph@8d.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */

this should not be removed.  if you wanted to add your copyright line,
then that's fine, but the rest needs to stay.

>  #include <linux/pwm.h>
> +#include <linux/pwm-led.h>

if there's going to be a bunch of new pwm related headers, perhaps a
subdir makes more sense: linux/pwm/xxx

> +static void
> +led_pwm_brightness_set(struct led_classdev *c,
> +                      enum led_brightness b)
> +{
> +       struct led_pwm *led;
> +       int percent;
> +
> +       percent = (b * 100) / (LED_FULL - LED_OFF);
> +       led = container_of(c, struct led_pwm, led);

instead of using container_of() everywhere, why not add a helper macro
that turns a led_classev into a led_pwm

> +static int __init
> +led_pwm_probe(struct platform_device *pdev)

probe functions are typical __devinit

> +       if (!try_module_get(d->driver->owner))
> +               return -ENODEV;

is this really needed ?

> -static int __devexit led_pwm_remove(struct platform_device *pdev)
> +static int
> +led_pwm_remove(struct platform_device *pdev)

looks like you lost the __devexit markings

>  static struct platform_driver led_pwm_driver = {
> +       .driver = {
> +               .name =         "leds-pwm",
> +               .owner =        THIS_MODULE,
>        },

i dont think platform_drivers need to explicitly set their owner.  i
thought that was all handled for you.

> --- /dev/null
> +++ b/include/linux/pwm-led.h
> @@ -0,0 +1,34 @@
> +#ifndef __LINUX_PWM_LED_H
> +#define __LINUX_PWM_LED_H
> +
> +/*
> + * include/linux/pwm-led.h

the ifdef protection typically goes after the header comment blob, not before
-mike