Mike Frysinger wrote:
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@xxxxxxxxxxxxxx)
- *
- * based on leds-gpio.c by Raphael Assenat <raph@xxxxxx>
- *
- * 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.
For the record, the reason the file looks like it does is because I
wrote an original one that replaced the previous leds-pwm.c--- but
obviously git didn't see it that way when it produced the diff.
I certainly wasn't trying to write-out anyone's copyright! I don't have
a problem with their names appearing in the file, regardless, so I'll
put them back in. If they have problems with their names appearing
therein, they can let me know. :)
#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
In general I don't have a problem with this. However, in this specific
case would it make more sense to just fold pwm-led.h into pwm.h? There
really isn't much to pwm-led.h.
+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
That's just a personal style thing. As soon as I write a helper macro,
I tend to forget how it works and then start mis-using it everywhere.
But I don't have a problem with doing a helper.
For better type-safety, would a helper inline-function be a better
choice than a helper macro? Or would it make any difference?
+static int __init
+led_pwm_probe(struct platform_device *pdev)
probe functions are typical __devinit
Yep, my bad.
+ if (!try_module_get(d->driver->owner))
+ return -ENODEV;
is this really needed ?
Not sure.
-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
Yep.
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.
There are examples for both cases. I don't see anything assigning to
.owner in drivers/base/platform.c, though.
--- /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
Yep, my bad.
Thanks for the feedback!
b.g.
--
Bill Gatliff
bgat@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
