Messages in this thread |  | | | Subject | Re: [PATCH v2] leds: provide helper to register "leds-gpio" devices | | From | Richard Purdie <> | | Date | Wed, 06 Apr 2011 06:38:17 -0700 |
| |
On Wed, 2011-04-06 at 14:33 +0200, Uwe Kleine-König wrote: > On Wed, Apr 06, 2011 at 04:52:18AM -0700, Richard Purdie wrote: > > I guess the motivation might be that if a given platform has many > > different LED configurations, you're trying to remove the ones you don't > > need from memory? Given all the above I'd suggest that this function > > should really be added to the platform device code if anywhere and > "platform device code" means e.g. arch/arm/plat-mxc or drivers/base > here?
Yes.
> > doesn't belong in the LED subsystem as its not an LED specific problem. > yeap, that's it. Note that this thread[1] started on the linux-arm-kernel > mailing list with a imx-specific version of this function. With the > background of Linus' recent rant against churn in arch/arm several > people pointed out that this can better go to a more generic place where > not only arm/imx, but also arm/omap and even powerpc can use the same > code. So the (IMHO) obvious place to put the code is near the driver. > > And yes, the problem is not LED specific, but a function that creates > and registers a leds-gpio device *is* LED specific. A while back I > thought about introducing something like drivers/register/*, but I'm > sure this won't scale. Either you need a header per device type (or at > subsystem) or a single header. Both look ugly in my eyes. Having the > prototype in include/linux/leds.h seems the right thing, because > platform code needs to include that anyhow to provide a struct > gpio_led_platform_data. > > I don't consider something wrong here, because the Linux device model > requires that you have a driver and a device. Both have to match and the > device (usually) is created at boot time. Because of the needed match > it's natural to have device use (aka driver) and device creation at the > same place. Because of the boot time thing the code needs to be > built-in.
It should only be built-in on platforms that both use leds-gpio and want to use this function at boot time. This is not the description of leds-core.c.
I understand the issue and the desire to move it into common code, I think that is good but I don't think you've found the most appropriate place yet.
I'm tempted to suggest making the function a static inline in leds.h (or create an leds-gpio.h and move the struct definition there too).
Cheers,
Richard -- Linux Foundation http://www.yoctoproject.org/
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |