[PATCH v3] leds: provide helper to register "leds-gpio" devices

[Uwe Kleine-König]

On Mon, May 09, 2011 at 11:17:19PM +0100, Russell King - ARM Linux wrote:
> On Mon, May 09, 2011 at 03:02:54PM -0700, Andrew Morton wrote:
> > On Mon, 11 Apr 2011 22:35:57 +0200
> > Uwe Kleine-K__nig  <u.kleine-koenig at pengutronix.de> wrote:
> > > +#if defined(CONFIG_LED_REGISTER_GPIO)
> > > +struct platform_device *__init gpio_led_register_device(
> > > +		int id, const struct gpio_led_platform_data *pdata)
> > > +{
> > > +	struct platform_device *ret;
> > > +	struct gpio_led_platform_data _pdata = *pdata;
> > > +
> > > +	_pdata.leds = kmemdup(pdata->leds,
> > > +			pdata->num_leds * sizeof(*pdata->leds), GFP_KERNEL);
> > > +	if (!_pdata.leds)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	ret = platform_device_register_resndata(NULL, "leds-gpio", id,
> > > +			NULL, 0, &_pdata, sizeof(_pdata));
> > > +	if (IS_ERR(ret))
> > > +		kfree(_pdata.leds);
> > > +
> > > +	return ret;
> > > +}
> > > +#endif
> ...
> > The comment doesn't document return values.
> 
> Two further comments.
> 
> 1. Why is this .c file always built, but _all_ the containing code is
> wrapped up in an ifdef?  It seems a waste of resources to compile a .c
> file with all code #ifdef'd out.
This was done because I thought that the .c file could contain other
registration routines later. Richard requested to use

	obj-$(CONFIG_LED_REGISTER_GPIO) += ...

then the #ifdef can go away, too. (@Andrew: Richard's ack was only for a
patch that used that. You took the patch anyhow and added his ack.)

> 2. What is the point of returning the platform device structure?  You've
> already registered it, so you must _not_ modify any data in that structure
> which may be used by the driver.  The only thing which you can safely do
> with it is unregister it.
pdev->device can be used as parent for another device. (OK, probably not
an led device. The origin of this function is the imx device
registration stuff, and all these functions return a platform device).
The other case where I needed to have the device created was to fill a
struct regulator_consumer_supply, but nowadays this is done using just
the name.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |