Re: [PATCH v3] leds: implement OpenFirmare GPIO LED driver

On Thu, Jul 17, 2008 at 01:18:18PM -0700, Trent Piepho wrote:

On Thu, 17 Jul 2008, Grant Likely wrote:

quoted

Alternately, I would also be okay with a scheme where all LED nodes
have a common parent and an of_platform driver would bind against the
parent node; not the individual children.  Then the leds-gpio driver
could be refactored to have both platform and of_platform bus
bindings.

Basically what I did then in my patch then, refactor leds-gpio so most of
it is shared and there is a block of code that does platform binding and
another block that does of_platform binding.

Ok. I must admit I'm quite burned out with OF gpio-leds. I was posting the
bindings since April, probably four or five times. Last time a week ago,
IIRC.

During the months I received just a few replies, one from Grant ("Looks
good to me."), few from Segher (with a lot of criticism, that I much
appreciated and tried to fix all spotted issues), and one from Laurent
(about active-low LEDs).

Of course, I know we're all busy etc and we don't always read or reply to
RFCs, so don't get me wrong, I'm not blaming or something. It just would be
great if all these ideas would be spoken up a bit earlier, i.e. not in the
middle of the merge window... But yes, we have what we have.

And the true thing is that I tend to agree with all your comments, and I
strongly believe that the issues you pointed out should be fixed prior to
merging. But I'm tired of these leds, they aren't _that_ interesting,
btw. ;-)

So..

Trent, I encourage you to collect your patch snippets into complete patch,
and post it so it could slip into 2.6.27 (the bad thing is that your
approach involves changes to the existing drivers, not just adding new
driver that could slip even into -rc9).

That is, I have a bunch of patches in my stg queue on which I can work
with more fun, so I'm somewhat glad that somebody will take care of the
notorious OF GPIO LEDs. ;-)


For what it's worth:

I didn't change the OF platform binding syntax so as not to complicate the
example, but that's easy to do.  Something like:

leds {
 	compatible = "gpio-led";
 	gpios = <&mpc8572 6 0
 	         &mpc8572 7 0>;
 	labels = "red", "green";
};

I don't like this.

Or like this, which needs a little more code to parse:

leds {
 	compatible = "gpio-led";
 	led@6 {
 		gpios = <&mpc8572 6 0>;
 		label = "red";
 	};
 	led@7 {
 		gpios = <&mpc8572 7 0>;
 		label = "green";
 	};
};

I like this. Or better what Grant suggested, i.e. move label to node
name.

I like the first better.  It follows the example from the docs about how
devices with multiple gpios should work too.

IMO, each LED is a device. So, I would rather define compatible = <> for
each led.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2