[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Fabio Estevam]

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/plat-mxc/devices/Kconfig               |    3 +++
 arch/arm/plat-mxc/devices/Makefile              |    1 +
 arch/arm/plat-mxc/devices/platform-gpio_leds.c  |   22 ++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/devices-common.h |    4 ++++
 4 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-mxc/devices/platform-gpio_leds.c

diff --git a/arch/arm/plat-mxc/devices/Kconfig b/arch/arm/plat-mxc/devices/Kconfig
index b9ab1d5..3d4f63c 100644
--- a/arch/arm/plat-mxc/devices/Kconfig
+++ b/arch/arm/plat-mxc/devices/Kconfig
@@ -13,6 +13,9 @@ config IMX_HAVE_PLATFORM_GPIO_KEYS
 	bool
 	default y if SOC_IMX51
 
+config IMX_HAVE_PLATFORM_GPIO_LEDS
+	bool
+
 config IMX_HAVE_PLATFORM_IMX21_HCD
 	bool
 	
diff --git a/arch/arm/plat-mxc/devices/Makefile b/arch/arm/plat-mxc/devices/Makefile
index 75cd2ec..ceefca6 100644
--- a/arch/arm/plat-mxc/devices/Makefile
+++ b/arch/arm/plat-mxc/devices/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_IMX_HAVE_PLATFORM_FEC) += platform-fec.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_FSL_USB2_UDC) += platform-fsl-usb2-udc.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_KEYS) += platform-gpio_keys.o
+obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_LEDS) += platform-gpio_leds.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX21_HCD) += platform-imx21-hcd.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX2_WDT) += platform-imx2-wdt.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_IMXDI_RTC) += platform-imxdi_rtc.o
diff --git a/arch/arm/plat-mxc/devices/platform-gpio_leds.c b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
new file mode 100644
index 0000000..a430213
--- /dev/null
+++ b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <asm/sizes.h>
+#include <mach/hardware.h>
+#include <mach/devices-common.h>
+
+struct platform_device *__init imx_add_gpio_leds(
+		 const struct gpio_led_platform_data *pdata)
+{
+	return imx_add_platform_device("leds-gpio", -1, NULL,
+		 0, pdata, sizeof(*pdata));
+}
diff --git a/arch/arm/plat-mxc/include/mach/devices-common.h b/arch/arm/plat-mxc/include/mach/devices-common.h
index 8658c9c..94ee936 100644
--- a/arch/arm/plat-mxc/include/mach/devices-common.h
+++ b/arch/arm/plat-mxc/include/mach/devices-common.h
@@ -57,6 +57,10 @@ struct platform_device *__init imx_add_fsl_usb2_udc(
 struct platform_device *__init imx_add_gpio_keys(
 		const struct gpio_keys_platform_data *pdata);
 
+#include <linux/leds.h>
+struct platform_device *__init imx_add_gpio_leds(
+		const struct gpio_led_platform_data *pdata);
+
 #include <mach/mx21-usbhost.h>
 struct imx_imx21_hcd_data {
 	resource_size_t iobase;
-- 
1.6.0.4

[PATCH v2 2/2] ARM: mx5/mx53_evk.c: Add LED support

[Fabio Estevam]

On the mx53evk board there is an LED connected to GPIO7_7.

Add support for it.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
-Changes since v1:

Use imx_add_gpio_leds to register gpio_leds

 arch/arm/mach-mx5/Kconfig          |    1 +
 arch/arm/mach-mx5/board-mx53_evk.c |   18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mx5/Kconfig b/arch/arm/mach-mx5/Kconfig
index 159340d..74b4e92 100644
--- a/arch/arm/mach-mx5/Kconfig
+++ b/arch/arm/mach-mx5/Kconfig
@@ -139,6 +139,7 @@ config MACH_MX51_EFIKASB
 config MACH_MX53_EVK
 	bool "Support MX53 EVK platforms"
 	select SOC_IMX53
+	select IMX_HAVE_PLATFORM_GPIO_LEDS
 	select IMX_HAVE_PLATFORM_IMX2_WDT
 	select IMX_HAVE_PLATFORM_IMX_UART
 	select IMX_HAVE_PLATFORM_IMX_I2C
diff --git a/arch/arm/mach-mx5/board-mx53_evk.c b/arch/arm/mach-mx5/board-mx53_evk.c
index 2af3f43..6ffeb42 100644
--- a/arch/arm/mach-mx5/board-mx53_evk.c
+++ b/arch/arm/mach-mx5/board-mx53_evk.c
@@ -37,6 +37,7 @@
 #define MX53_EVK_FEC_PHY_RST	IMX_GPIO_NR(7, 6)
 #define EVK_ECSPI1_CS0		IMX_GPIO_NR(2, 30)
 #define EVK_ECSPI1_CS1		IMX_GPIO_NR(3, 19)
+#define MX53EVK_LED		IMX_GPIO_NR(7, 7)
 
 #include "crm_regs.h"
 #include "devices-imx53.h"
@@ -60,6 +61,8 @@ static iomux_v3_cfg_t mx53_evk_pads[] = {
 	/* ecspi chip select lines */
 	MX53_PAD_EIM_EB2__GPIO2_30,
 	MX53_PAD_EIM_D19__GPIO3_19,
+	/* LED */
+	MX53_PAD_PATA_DA_1__GPIO7_7,
 };
 
 static const struct imxuart_platform_data mx53_evk_uart_pdata __initconst = {
@@ -117,6 +120,20 @@ static const struct spi_imx_master mx53_evk_spi_data __initconst = {
 	.num_chipselect = ARRAY_SIZE(mx53_evk_spi_cs),
 };
 
+static struct gpio_led mx53_evk_leds[] = {
+	{
+		.name			= "led-green",
+		.default_trigger	= "heartbeat",
+		.active_low		= 0,
+		.gpio			= MX53EVK_LED,
+	},
+};
+
+static struct gpio_led_platform_data mx53_evk_leds_data = {
+	.leds = mx53_evk_leds,
+	.num_leds = ARRAY_SIZE(mx53_evk_leds),
+};
+
 static void __init mx53_evk_board_init(void)
 {
 	mxc_iomux_v3_setup_multiple_pads(mx53_evk_pads,
@@ -135,6 +152,7 @@ static void __init mx53_evk_board_init(void)
 		ARRAY_SIZE(mx53_evk_spi_board_info));
 	imx53_add_ecspi(0, &mx53_evk_spi_data);
 	imx53_add_imx2_wdt(0, NULL);
+	imx_add_gpio_leds(&mx53_evk_leds_data);
 }
 
 static void __init mx53_evk_timer_init(void)
-- 
1.6.0.4

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Sascha Hauer]

On Mon, Apr 04, 2011 at 02:06:45PM -0300, Fabio Estevam wrote:
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/plat-mxc/devices/Kconfig               |    3 +++
>  arch/arm/plat-mxc/devices/Makefile              |    1 +
>  arch/arm/plat-mxc/devices/platform-gpio_leds.c  |   22 ++++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/devices-common.h |    4 ++++
>  4 files changed, 30 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/plat-mxc/devices/platform-gpio_leds.c
> 
> diff --git a/arch/arm/plat-mxc/devices/Kconfig b/arch/arm/plat-mxc/devices/Kconfig
> index b9ab1d5..3d4f63c 100644
> --- a/arch/arm/plat-mxc/devices/Kconfig
> +++ b/arch/arm/plat-mxc/devices/Kconfig
> @@ -13,6 +13,9 @@ config IMX_HAVE_PLATFORM_GPIO_KEYS
>  	bool
>  	default y if SOC_IMX51
>  
> +config IMX_HAVE_PLATFORM_GPIO_LEDS
> +	bool
> +
>  config IMX_HAVE_PLATFORM_IMX21_HCD
>  	bool
>  	
> diff --git a/arch/arm/plat-mxc/devices/Makefile b/arch/arm/plat-mxc/devices/Makefile
> index 75cd2ec..ceefca6 100644
> --- a/arch/arm/plat-mxc/devices/Makefile
> +++ b/arch/arm/plat-mxc/devices/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_IMX_HAVE_PLATFORM_FEC) += platform-fec.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_FSL_USB2_UDC) += platform-fsl-usb2-udc.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_KEYS) += platform-gpio_keys.o
> +obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_LEDS) += platform-gpio_leds.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX21_HCD) += platform-imx21-hcd.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX2_WDT) += platform-imx2-wdt.o
>  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMXDI_RTC) += platform-imxdi_rtc.o
> diff --git a/arch/arm/plat-mxc/devices/platform-gpio_leds.c b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
> new file mode 100644
> index 0000000..a430213
> --- /dev/null
> +++ b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <asm/sizes.h>
> +#include <mach/hardware.h>
> +#include <mach/devices-common.h>
> +
> +struct platform_device *__init imx_add_gpio_leds(
> +		 const struct gpio_led_platform_data *pdata)
> +{
> +	return imx_add_platform_device("leds-gpio", -1, NULL,
> +		 0, pdata, sizeof(*pdata));
> +}

Does this really make sense? There's nothing imx specific in this
function, so it shouldn't be named imx_* and it shouldn't be under
plat-mxc. Also, any user of imx_add_gpio_leds could call
platform_device_register_resndata instead.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

On Mon, Apr 04, 2011 at 07:19:27PM +0200, Sascha Hauer wrote:
> On Mon, Apr 04, 2011 at 02:06:45PM -0300, Fabio Estevam wrote:
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  arch/arm/plat-mxc/devices/Kconfig               |    3 +++
> >  arch/arm/plat-mxc/devices/Makefile              |    1 +
> >  arch/arm/plat-mxc/devices/platform-gpio_leds.c  |   22 ++++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/devices-common.h |    4 ++++
> >  4 files changed, 30 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/plat-mxc/devices/platform-gpio_leds.c
> > 
> > diff --git a/arch/arm/plat-mxc/devices/Kconfig b/arch/arm/plat-mxc/devices/Kconfig
> > index b9ab1d5..3d4f63c 100644
> > --- a/arch/arm/plat-mxc/devices/Kconfig
> > +++ b/arch/arm/plat-mxc/devices/Kconfig
> > @@ -13,6 +13,9 @@ config IMX_HAVE_PLATFORM_GPIO_KEYS
> >  	bool
> >  	default y if SOC_IMX51
> >  
> > +config IMX_HAVE_PLATFORM_GPIO_LEDS
> > +	bool
> > +
> >  config IMX_HAVE_PLATFORM_IMX21_HCD
> >  	bool
> >  	
> > diff --git a/arch/arm/plat-mxc/devices/Makefile b/arch/arm/plat-mxc/devices/Makefile
> > index 75cd2ec..ceefca6 100644
> > --- a/arch/arm/plat-mxc/devices/Makefile
> > +++ b/arch/arm/plat-mxc/devices/Makefile
> > @@ -2,6 +2,7 @@ obj-$(CONFIG_IMX_HAVE_PLATFORM_FEC) += platform-fec.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_FSL_USB2_UDC) += platform-fsl-usb2-udc.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_KEYS) += platform-gpio_keys.o
> > +obj-$(CONFIG_IMX_HAVE_PLATFORM_GPIO_LEDS) += platform-gpio_leds.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX21_HCD) += platform-imx21-hcd.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX2_WDT) += platform-imx2-wdt.o
> >  obj-$(CONFIG_IMX_HAVE_PLATFORM_IMXDI_RTC) += platform-imxdi_rtc.o
> > diff --git a/arch/arm/plat-mxc/devices/platform-gpio_leds.c b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
> > new file mode 100644
> > index 0000000..a430213
> > --- /dev/null
> > +++ b/arch/arm/plat-mxc/devices/platform-gpio_leds.c
> > @@ -0,0 +1,22 @@
> > +/*
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +#include <asm/sizes.h>
> > +#include <mach/hardware.h>
> > +#include <mach/devices-common.h>
> > +
> > +struct platform_device *__init imx_add_gpio_leds(
> > +		 const struct gpio_led_platform_data *pdata)
> > +{
> > +	return imx_add_platform_device("leds-gpio", -1, NULL,
> > +		 0, pdata, sizeof(*pdata));
> > +}
> 
> Does this really make sense? There's nothing imx specific in this
> function, so it shouldn't be named imx_* and it shouldn't be under
> plat-mxc. Also, any user of imx_add_gpio_leds could call
> platform_device_register_resndata instead.
hmm, as long as the function lives in plat-mxc it's IMHO even required
to be named imx_*. But other than that, you're obivously right.
(Well, the central approach would assert that only one instance of
"leds-gpio" is used. But that alone probably isn't worth the
optimisation.)

The mechanims could be made a bit more aggressive by something like
that:

	struct gpio_led_platform_data *_pdata = *pdata;
	_pdata->leds = kmemdup(pdata->leds, pdata->num_leds * sizeof(*pdata->leds), GFP_KERNEL)
	if (!_pdata->leds)
		return ...

	ret = imx_add_platform_device("leds-gpio", -1, NULL, 0, _pdata, sizeof(_pdata));
	if (IS_ERR(ret))
		kfree(_pdata->leds);

	return PTR_RET(ret);

This would allow to have the struct gpio_led array in init memory, too.

Is it worth the effort?

Best regards
Uwe

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

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Mon, Apr 04, 2011 at 07:42:44PM +0200, Uwe Kleine-K?nig wrote:
> The mechanims could be made a bit more aggressive by something like
> that:
> 
> 	struct gpio_led_platform_data *_pdata = *pdata;
> 	_pdata->leds = kmemdup(pdata->leds, pdata->num_leds * sizeof(*pdata->leds), GFP_KERNEL)
> 	if (!_pdata->leds)
> 		return ...
> 
> 	ret = imx_add_platform_device("leds-gpio", -1, NULL, 0, _pdata, sizeof(_pdata));
> 	if (IS_ERR(ret))
> 		kfree(_pdata->leds);
> 
> 	return PTR_RET(ret);
> 
> This would allow to have the struct gpio_led array in init memory, too.

If you go to the effort of making it _that_ generic, then it shouldn't
even be in plat-mxc, but somewhere *everyone* can benefit from it.  See
Linus' complaints in the OMAP pull request thread about the amount of
largely similar arch/arm code.

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[H Hartley Sweeten]

On Monday, April 04, 2011 10:53 AM, Russell King wrote:
> On Mon, Apr 04, 2011 at 07:42:44PM +0200, Uwe Kleine-K?nig wrote:
>> The mechanims could be made a bit more aggressive by something like
>> that:
>> 
>> 	struct gpio_led_platform_data *_pdata = *pdata;
>> 	_pdata->leds = kmemdup(pdata->leds, pdata->num_leds * sizeof(*pdata->leds), GFP_KERNEL)
>> 	if (!_pdata->leds)
>> 		return ...
>> 
>> 	ret = imx_add_platform_device("leds-gpio", -1, NULL, 0, _pdata, sizeof(_pdata));
>> 	if (IS_ERR(ret))
>> 		kfree(_pdata->leds);
>> 
>> 	return PTR_RET(ret);
>> 
>> This would allow to have the struct gpio_led array in init memory, too.
>
> If you go to the effort of making it _that_ generic, then it shouldn't
> even be in plat-mxc, but somewhere *everyone* can benefit from it.  See
> Linus' complaints in the OMAP pull request thread about the amount of
> largely similar arch/arm code.

If it's made generic, mach-ep93xx/core.c could use it to register it's two
platform leds.  Please keep me in the loop if this is done.

Regards,
Hartley

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Mon, Apr 04, 2011 at 01:06:52PM -0500, H Hartley Sweeten wrote:
> On Monday, April 04, 2011 10:53 AM, Russell King wrote:
> > On Mon, Apr 04, 2011 at 07:42:44PM +0200, Uwe Kleine-K?nig wrote:
> >> The mechanims could be made a bit more aggressive by something like
> >> that:
> >> 
> >> 	struct gpio_led_platform_data *_pdata = *pdata;
> >> 	_pdata->leds = kmemdup(pdata->leds, pdata->num_leds * sizeof(*pdata->leds), GFP_KERNEL)
> >> 	if (!_pdata->leds)
> >> 		return ...
> >> 
> >> 	ret = imx_add_platform_device("leds-gpio", -1, NULL, 0, _pdata, sizeof(_pdata));
> >> 	if (IS_ERR(ret))
> >> 		kfree(_pdata->leds);
> >> 
> >> 	return PTR_RET(ret);
> >> 
> >> This would allow to have the struct gpio_led array in init memory, too.
> >
> > If you go to the effort of making it _that_ generic, then it shouldn't
> > even be in plat-mxc, but somewhere *everyone* can benefit from it.  See
> > Linus' complaints in the OMAP pull request thread about the amount of
> > largely similar arch/arm code.
> 
> If it's made generic, mach-ep93xx/core.c could use it to register it's two
> platform leds.  Please keep me in the loop if this is done.

In which case we don't want "if it's made generic", we *require* it to be
generic.

As I say, we *must* get away from the idea that something in mach-xyz has
nothing to do with our own development in mach-abc, and start taking an
interest in consolidating some of this stuff.  Otherwise we may get to
the point where further arch/arm development is barred from the mainline
kernel.

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[H Hartley Sweeten]

On Monday, April 04, 2011 11:18 AM, Russell King wrote:
>>> If you go to the effort of making it _that_ generic, then it shouldn't
>>> even be in plat-mxc, but somewhere *everyone* can benefit from it.  See
>>> Linus' complaints in the OMAP pull request thread about the amount of
>>> largely similar arch/arm code.
>> 
>> If it's made generic, mach-ep93xx/core.c could use it to register it's two
>> platform leds.  Please keep me in the loop if this is done.
>
> In which case we don't want "if it's made generic", we *require* it to be
> generic.
>
> As I say, we *must* get away from the idea that something in mach-xyz has
> nothing to do with our own development in mach-abc, and start taking an
> interest in consolidating some of this stuff.  Otherwise we may get to
> the point where further arch/arm development is barred from the mainline
> kernel.

How about something like this:

int __init gpio_led_platform_register(int id, struct gpio_led *leds, int nr)
{
	struct gpio_led_platform_data *pdata;
	struct platform_device *pdev;
	int ret;

	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
	if (!pdata)
		return -ENOMEM;

	pdev = platform_device_alloc("leds-gpio", id);
	if (!pdev) {
		ret = -ENOMEM;
		goto out_free;
	}

	pdata->leds = leds;
	pdata->num_leds = nr;
	pdev->dev.platform_data = pdata;
	ret = platform_device_add(pdev);
	if (ret)
		goto out_pdev;
	return 0;

out_pdev:
	platform_device_put(pdev);
out_free:
	kfree(pdata);
	return ret;
}

The the mach-xyz init code just has to:

static struct gpio_led gpio_leds[] = {
	{
		.name	= "foo:fooclr",
		.gpio	= FOO_GPIO_NUM,
	}, {
		.name	= "bar:barclr",
		.gpio	= BAR_LED_NUM,
	},
	/* etc. */
};


	gpio_led_platform_register(-1, gpio_leds, ARRAY_SIZE(gpio_leds));

If some one could suggest a good place for the gpio_led_platform_register function
I can create a proper patch.  Maybe just add it to the end of the leds-gpio.c driver?

Regards,
Hartley

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

Hello,

On Mon, Apr 04, 2011 at 04:52:43PM -0500, H Hartley Sweeten wrote:
> On Monday, April 04, 2011 11:18 AM, Russell King wrote:
> >>> If you go to the effort of making it _that_ generic, then it shouldn't
> >>> even be in plat-mxc, but somewhere *everyone* can benefit from it.  See
> >>> Linus' complaints in the OMAP pull request thread about the amount of
> >>> largely similar arch/arm code.
> >> 
> >> If it's made generic, mach-ep93xx/core.c could use it to register it's two
> >> platform leds.  Please keep me in the loop if this is done.
> >
> > In which case we don't want "if it's made generic", we *require* it to be
> > generic.

Yeah, definitely. I already tried once to make the device registration
that we use on mxc globally available.

> > As I say, we *must* get away from the idea that something in mach-xyz has
> > nothing to do with our own development in mach-abc, and start taking an
> > interest in consolidating some of this stuff.  Otherwise we may get to
> > the point where further arch/arm development is barred from the mainline
> > kernel.
> 
> How about something like this:
> 
> int __init gpio_led_platform_register(int id, struct gpio_led *leds, int nr)
> {
> 	struct gpio_led_platform_data *pdata;
> 	struct platform_device *pdev;
> 	int ret;
> 
> 	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> 	if (!pdata)
> 		return -ENOMEM;
> 
> 	pdev = platform_device_alloc("leds-gpio", id);
> 	if (!pdev) {
> 		ret = -ENOMEM;
> 		goto out_free;
> 	}
> 
> 	pdata->leds = leds;
> 	pdata->num_leds = nr;
> 	pdev->dev.platform_data = pdata;
> 	ret = platform_device_add(pdev);
> 	if (ret)
> 		goto out_pdev;
> 	return 0;
> 
> out_pdev:
> 	platform_device_put(pdev);
> out_free:
> 	kfree(pdata);
> 	return ret;
> }
I like my approach better, without using mxc specific functions it would
look as follows:

struct platform_device *__init gpio_led_register_device(
		const gpio_led_platform_data *pdata)
{
	struct platform_device *ret = ERR_PTR(-ENOMEM);
	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", -1,
		NULL, 0, _pdata, sizeof(_pdata));

	if (IS_ERR(ret))
		kfree(_pdata->leds);

	return ret;
}

> The the mach-xyz init code just has to:
> 
> static struct gpio_led gpio_leds[] = {
> 	{
> 		.name	= "foo:fooclr",
> 		.gpio	= FOO_GPIO_NUM,
> 	}, {
> 		.name	= "bar:barclr",
> 		.gpio	= BAR_LED_NUM,
> 	},
> 	/* etc. */
> };
> 
> 
> 	gpio_led_platform_register(-1, gpio_leds, ARRAY_SIZE(gpio_leds));
Compared to your approach with mine
 - gpio_leds could be const __initconst;
 - uses only a single parameter which AFAIK saves a few bytes to call
   the function;
 - you don't have control about the .id member (which IMHO is OK but
   might be changed easily);
 - you have to call a function with a different name;

(Note all of these are no hard requirements for me, just my thoughts.)

> If some one could suggest a good place for the gpio_led_platform_register function
> I can create a proper patch.  Maybe just add it to the end of the leds-gpio.c driver?
mxc uses a central place to register all devices
(arch/arm/plat-mxc/devices).  I think for a global approach this won't
scale (e.g. because a single header file is used for all devices), so a
place near the driver is the right thing to do. leds-gpio.c doesn't work
though because then it might end in a module.

I'll send a patch as a reply to this mail.

Best regards
Uwe

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

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 09:30:30AM +0200, Uwe Kleine-K?nig wrote:
> I like my approach better, without using mxc specific functions it would
> look as follows:
> 
> struct platform_device *__init gpio_led_register_device(
> 		const gpio_led_platform_data *pdata)
> {
> 	struct platform_device *ret = ERR_PTR(-ENOMEM);
> 	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", -1,
> 		NULL, 0, _pdata, sizeof(_pdata));
> 
> 	if (IS_ERR(ret))
> 		kfree(_pdata->leds);
> 
> 	return ret;
> }

But this is inconsistent.  The gpio_led_platform_data isn't copied,
but a sub-structure is - which makes this a candidate for people adding
__initdata to the wrong thing.  Also marking the LEDs array as __initdata
but leaving the gpio_led_platform_data without __initdata will lead to
warnings.

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

On Tue, Apr 05, 2011 at 08:40:17AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 05, 2011 at 09:30:30AM +0200, Uwe Kleine-K?nig wrote:
> > I like my approach better, without using mxc specific functions it would
> > look as follows:
> > 
> > struct platform_device *__init gpio_led_register_device(
> > 		const gpio_led_platform_data *pdata)
> > {
> > 	struct platform_device *ret = ERR_PTR(-ENOMEM);
> > 	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", -1,
> > 		NULL, 0, _pdata, sizeof(_pdata));
> > 
> > 	if (IS_ERR(ret))
> > 		kfree(_pdata->leds);
> > 
> > 	return ret;
> > }
> 
> But this is inconsistent.  The gpio_led_platform_data isn't copied,
> but a sub-structure is - which makes this a candidate for people adding
> __initdata to the wrong thing.  Also marking the LEDs array as __initdata
> but leaving the gpio_led_platform_data without __initdata will lead to
> warnings.
platform_device_register_resndata does the copying.

Best regards
Uwe

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

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 09:43:15AM +0200, Uwe Kleine-K?nig wrote:
> On Tue, Apr 05, 2011 at 08:40:17AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Apr 05, 2011 at 09:30:30AM +0200, Uwe Kleine-K?nig wrote:
> > > I like my approach better, without using mxc specific functions it would
> > > look as follows:
> > > 
> > > struct platform_device *__init gpio_led_register_device(
> > > 		const gpio_led_platform_data *pdata)
> > > {
> > > 	struct platform_device *ret = ERR_PTR(-ENOMEM);
> > > 	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", -1,
> > > 		NULL, 0, _pdata, sizeof(_pdata));
> > > 
> > > 	if (IS_ERR(ret))
> > > 		kfree(_pdata->leds);
> > > 
> > > 	return ret;
> > > }
> > 
> > But this is inconsistent.  The gpio_led_platform_data isn't copied,
> > but a sub-structure is - which makes this a candidate for people adding
> > __initdata to the wrong thing.  Also marking the LEDs array as __initdata
> > but leaving the gpio_led_platform_data without __initdata will lead to
> > warnings.
> platform_device_register_resndata does the copying.

Ok, but you're still modifying the leds pointer in the platform data,
which you've also marked as const.  That's an error.

If you want gpio_led_platform_data to be const, you also have to make
a copy of it here, even if you have to free it after
platform_device_register_resndata().

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

On Tue, Apr 05, 2011 at 08:47:54AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 05, 2011 at 09:43:15AM +0200, Uwe Kleine-K?nig wrote:
> > On Tue, Apr 05, 2011 at 08:40:17AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Apr 05, 2011 at 09:30:30AM +0200, Uwe Kleine-K?nig wrote:
> > > > I like my approach better, without using mxc specific functions it would
> > > > look as follows:
> > > > 
> > > > struct platform_device *__init gpio_led_register_device(
> > > > 		const gpio_led_platform_data *pdata)
> > > > {
> > > > 	struct platform_device *ret = ERR_PTR(-ENOMEM);
> > > > 	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", -1,
> > > > 		NULL, 0, _pdata, sizeof(_pdata));
> > > > 
> > > > 	if (IS_ERR(ret))
> > > > 		kfree(_pdata->leds);
> > > > 
> > > > 	return ret;
> > > > }
> > > 
> > > But this is inconsistent.  The gpio_led_platform_data isn't copied,
> > > but a sub-structure is - which makes this a candidate for people adding
> > > __initdata to the wrong thing.  Also marking the LEDs array as __initdata
> > > but leaving the gpio_led_platform_data without __initdata will lead to
> > > warnings.
> > platform_device_register_resndata does the copying.
> 
> Ok, but you're still modifying the leds pointer in the platform data,
> which you've also marked as const.  That's an error.
> 
> If you want gpio_led_platform_data to be const, you also have to make
> a copy of it here, even if you have to free it after
> platform_device_register_resndata().
This was intended by

	struct gpio_led_platform_data *_pdata = *pdata;

I'm just fighting with the compiler at this line because it doesn't like
_pdata being initialized by a const pointer.

Best regards
Uwe

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

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 09:51:18AM +0200, Uwe Kleine-K?nig wrote:
> This was intended by
> 
> 	struct gpio_led_platform_data *_pdata = *pdata;
> 
> I'm just fighting with the compiler at this line because it doesn't like
> _pdata being initialized by a const pointer.

That's idiotic and revolting.  Casting away const means you're writing
broken code, which can fail for XIP users.  If you're going to play
such games, there's *NO* point in marking it const in the first place.

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

On Tue, Apr 05, 2011 at 08:59:32AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 05, 2011 at 09:51:18AM +0200, Uwe Kleine-K?nig wrote:
> > This was intended by
> > 
> > 	struct gpio_led_platform_data *_pdata = *pdata;
> > 
> > I'm just fighting with the compiler at this line because it doesn't like
> > _pdata being initialized by a const pointer.
> 
> That's idiotic and revolting.  Casting away const means you're writing
> broken code, which can fail for XIP users.  If you're going to play
> such games, there's *NO* point in marking it const in the first place.
No I won the fight with the compiler, the result is

	struct gpio_led_platform_data _pdata = *pdata

which AFAIK is valid C and even works for XIP users.

Best regards
Uwe

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

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 10:32:59AM +0200, Uwe Kleine-K?nig wrote:
> On Tue, Apr 05, 2011 at 08:59:32AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Apr 05, 2011 at 09:51:18AM +0200, Uwe Kleine-K?nig wrote:
> > > This was intended by
> > > 
> > > 	struct gpio_led_platform_data *_pdata = *pdata;
> > > 
> > > I'm just fighting with the compiler at this line because it doesn't like
> > > _pdata being initialized by a const pointer.
> > 
> > That's idiotic and revolting.  Casting away const means you're writing
> > broken code, which can fail for XIP users.  If you're going to play
> > such games, there's *NO* point in marking it const in the first place.
> No I won the fight with the compiler, the result is
> 
> 	struct gpio_led_platform_data _pdata = *pdata
> 
> which AFAIK is valid C and even works for XIP users.

Which will work, but that's not what you wrote in the first two posts
of the code.

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[Uwe Kleine-König]

On Tue, Apr 05, 2011 at 09:43:48AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 05, 2011 at 10:32:59AM +0200, Uwe Kleine-K?nig wrote:
> > On Tue, Apr 05, 2011 at 08:59:32AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Apr 05, 2011 at 09:51:18AM +0200, Uwe Kleine-K?nig wrote:
> > > > This was intended by
> > > > 
> > > > 	struct gpio_led_platform_data *_pdata = *pdata;
> > > > 
> > > > I'm just fighting with the compiler at this line because it doesn't like
> > > > _pdata being initialized by a const pointer.
> > > 
> > > That's idiotic and revolting.  Casting away const means you're writing
> > > broken code, which can fail for XIP users.  If you're going to play
> > > such games, there's *NO* point in marking it const in the first place.
> > No I won the fight with the compiler, the result is
> > 
> > 	struct gpio_led_platform_data _pdata = *pdata
> > 
> > which AFAIK is valid C and even works for XIP users.
> 
> Which will work, but that's not what you wrote in the first two posts
> of the code.
Yeah, this was completely untested and apart from the const issue didn't
even matched the correct types.

Best regards
Uwe

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

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

[Uwe Kleine-König]

This function makes a deep copy of the platform data to allow it to live
in init memory.
The definition cannot go into leds-gpio.c because it needs to be builtin
to be usable by platforms.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

I choosed to put this into drivers/leds/led-core.c because I consider
drivers/leds/leds-register.c or even something leds-gpio specific overkill.

But I'm open to discuss where to put it best.

Best regards
Uwe

 drivers/leds/led-core.c |   27 ++++++++++++++++++++++++++-
 include/linux/leds.h    |   12 ++++++++++++
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 016d19f..a9a762e 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -2,8 +2,10 @@
  * LED Class Core
  *
  * Copyright 2005-2006 Openedhand Ltd.
+ * Richard Purdie <rpurdie@openedhand.com>
  *
- * Author: Richard Purdie <rpurdie@openedhand.com>
+ * Copyright (C) 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
  *
  * 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
@@ -16,6 +18,9 @@
 #include <linux/module.h>
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 #include "leds.h"
 
 DECLARE_RWSEM(leds_list_lock);
@@ -23,3 +28,23 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
+
+struct platform_device *__init gpio_led_register_device(
+                const struct gpio_led_platform_data *pdata)
+{
+	struct platform_device *ret = ERR_PTR(-ENOMEM);
+	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", -1,
+			NULL, 0, &_pdata, sizeof(_pdata));
+
+	if (IS_ERR(ret))
+		kfree(_pdata.leds);
+
+	return ret;
+}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 61e0340..e6897d9 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -207,5 +207,17 @@ struct gpio_led_platform_data {
 					unsigned long *delay_off);
 };
 
+/**
+ * gpio_led_register_device - register a gpio-led device
+ * @pdata: the platform data used for the new device
+ *
+ * Use this function instead of platform_device_add()ing a static struct
+ * platform_device.
+ *
+ * Note: as pdata and pdata->leds is copied these usually can and should be
+ * __initdata.
+ */
+struct platform_device *__init gpio_led_register_device(
+		const struct gpio_led_platform_data *pdata);
 
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.2.3

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

[Fabio Estevam]

Hi Uwe,

On 4/5/2011 5:37 AM, Uwe Kleine-K?nig wrote:
> This function makes a deep copy of the platform data to allow it to live
> in init memory.
> The definition cannot go into leds-gpio.c because it needs to be builtin
> to be usable by platforms.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

I tested your patch on a MX53EVK board, but I could only build it after unselecting the mmc driver.

This is the error I got when mmc was selected:

  CC      drivers/mmc/card/mmc_test.o
  LD      drivers/mmc/card/built-in.o
  CC      drivers/mmc/core/sdio_io.o
In file included from include/linux/mmc/host.h:13,
                 from drivers/mmc/core/sdio_io.c:12:
include/linux/leds.h:220: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'gpio_led_register_device'
make[3]: *** [drivers/mmc/core/sdio_io.o] Error 1
make[2]: *** [drivers/mmc/core] Error 2
make[1]: *** [drivers/mmc] Error 2
make: *** [drivers] Error 2

Regards,

Fabio Estevam

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

[Fabio Estevam]

On 4/5/2011 1:13 PM, Fabio Estevam wrote:
> Hi Uwe,
> 
> On 4/5/2011 5:37 AM, Uwe Kleine-K?nig wrote:
>> This function makes a deep copy of the platform data to allow it to live
>> in init memory.
>> The definition cannot go into leds-gpio.c because it needs to be builtin
>> to be usable by platforms.
>>
>> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> 
> I tested your patch on a MX53EVK board, but I could only build it after unselecting the mmc driver.
> 
> This is the error I got when mmc was selected:
> 
>   CC      drivers/mmc/card/mmc_test.o
>   LD      drivers/mmc/card/built-in.o
>   CC      drivers/mmc/core/sdio_io.o
> In file included from include/linux/mmc/host.h:13,
>                  from drivers/mmc/core/sdio_io.c:12:
> include/linux/leds.h:220: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'gpio_led_register_device'
> make[3]: *** [drivers/mmc/core/sdio_io.o] Error 1
> make[2]: *** [drivers/mmc/core] Error 2
> make[1]: *** [drivers/mmc] Error 2
> make: *** [drivers] Error 2

If I declare it as "platform_device *gpio_led_register_device" then it builds fine.

Regards,

Fabio Estevam

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

[H Hartley Sweeten]

On Tuesday, April 05, 2011 9:29 AM, Fabio Estevam wrote:
> On 4/5/2011 1:13 PM, Fabio Estevam wrote:
>> Hi Uwe,
>> 
>> On 4/5/2011 5:37 AM, Uwe Kleine-K?nig wrote:
>>> This function makes a deep copy of the platform data to allow it to live
>>> in init memory.
>>> The definition cannot go into leds-gpio.c because it needs to be builtin
>>> to be usable by platforms.
>>>
>>> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> 
>> I tested your patch on a MX53EVK board, but I could only build it after unselecting the mmc driver.
>> 
>> This is the error I got when mmc was selected:
>> 
>>   CC      drivers/mmc/card/mmc_test.o
>>   LD      drivers/mmc/card/built-in.o
>>   CC      drivers/mmc/core/sdio_io.o
>> In file included from include/linux/mmc/host.h:13,
>>                  from drivers/mmc/core/sdio_io.c:12:
>> include/linux/leds.h:220: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'gpio_led_register_device'
>> make[3]: *** [drivers/mmc/core/sdio_io.o] Error 1
>> make[2]: *** [drivers/mmc/core] Error 2
>> make[1]: *** [drivers/mmc] Error 2
>> make: *** [drivers] Error 2
>
> If I declare it as "platform_device *gpio_led_register_device" then it builds fine.

With the change Fabio pointed out this builds and works fine on the ep93xx.

Hartley

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

[Russell King - ARM Linux]

On Tue, Apr 05, 2011 at 10:37:35AM +0200, Uwe Kleine-K?nig wrote:
> +struct platform_device *__init gpio_led_register_device(
> +		const struct gpio_led_platform_data *pdata);

Please don't add __init annotations to declarations.

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

[Uwe Kleine-König]

This function makes a deep copy of the platform data to allow it to live
in init memory.
The definition cannot go into leds-gpio.c because it needs to be builtin
to be usable by platforms.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
changes since v1:
 - don't add __init to the declaration of gpio_led_register_device

 drivers/leds/led-core.c |   27 ++++++++++++++++++++++++++-
 include/linux/leds.h    |   12 ++++++++++++
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 016d19f..a9a762e 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -2,8 +2,10 @@
  * LED Class Core
  *
  * Copyright 2005-2006 Openedhand Ltd.
+ * Richard Purdie <rpurdie@openedhand.com>
  *
- * Author: Richard Purdie <rpurdie@openedhand.com>
+ * Copyright (C) 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
  *
  * 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
@@ -16,6 +18,9 @@
 #include <linux/module.h>
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
 #include "leds.h"
 
 DECLARE_RWSEM(leds_list_lock);
@@ -23,3 +28,23 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
+
+struct platform_device *__init gpio_led_register_device(
+                const struct gpio_led_platform_data *pdata)
+{
+	struct platform_device *ret = ERR_PTR(-ENOMEM);
+	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", -1,
+			NULL, 0, &_pdata, sizeof(_pdata));
+
+	if (IS_ERR(ret))
+		kfree(_pdata.leds);
+
+	return ret;
+}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 61e0340..1c82006 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -207,5 +207,17 @@ struct gpio_led_platform_data {
 					unsigned long *delay_off);
 };
 
+/**
+ * gpio_led_register_device - register a gpio-led device
+ * @pdata: the platform data used for the new device
+ *
+ * Use this function instead of platform_device_add()ing a static struct
+ * platform_device.
+ *
+ * Note: as pdata and pdata->leds is copied these usually can and should be
+ * __initdata.
+ */
+struct platform_device *gpio_led_register_device(
+		const struct gpio_led_platform_data *pdata);
 
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.2.3

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

[Fabio Estevam]

Hi Uwe,

On 4/5/2011 5:24 PM, Uwe Kleine-K?nig wrote:
> This function makes a deep copy of the platform data to allow it to live
> in init memory.
> The definition cannot go into leds-gpio.c because it needs to be builtin
> to be usable by platforms.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks,

Fabio Estevam

> ---
> changes since v1:
>  - don't add __init to the declaration of gpio_led_register_device
> 
>  drivers/leds/led-core.c |   27 ++++++++++++++++++++++++++-
>  include/linux/leds.h    |   12 ++++++++++++
>  2 files changed, 38 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 016d19f..a9a762e 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -2,8 +2,10 @@
>   * LED Class Core
>   *
>   * Copyright 2005-2006 Openedhand Ltd.
> + * Richard Purdie <rpurdie@openedhand.com>
>   *
> - * Author: Richard Purdie <rpurdie@openedhand.com>
> + * Copyright (C) 2009-2010 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
>   *
>   * 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
> @@ -16,6 +18,9 @@
>  #include <linux/module.h>
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include "leds.h"
>  
>  DECLARE_RWSEM(leds_list_lock);
> @@ -23,3 +28,23 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>  
>  LIST_HEAD(leds_list);
>  EXPORT_SYMBOL_GPL(leds_list);
> +
> +struct platform_device *__init gpio_led_register_device(
> +                const struct gpio_led_platform_data *pdata)
> +{
> +	struct platform_device *ret = ERR_PTR(-ENOMEM);
> +	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", -1,
> +			NULL, 0, &_pdata, sizeof(_pdata));
> +
> +	if (IS_ERR(ret))
> +		kfree(_pdata.leds);
> +
> +	return ret;
> +}
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 61e0340..1c82006 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -207,5 +207,17 @@ struct gpio_led_platform_data {
>  					unsigned long *delay_off);
>  };
>  
> +/**
> + * gpio_led_register_device - register a gpio-led device
> + * @pdata: the platform data used for the new device
> + *
> + * Use this function instead of platform_device_add()ing a static struct
> + * platform_device.
> + *
> + * Note: as pdata and pdata->leds is copied these usually can and should be
> + * __initdata.
> + */
> +struct platform_device *gpio_led_register_device(
> +		const struct gpio_led_platform_data *pdata);
>  
>  #endif		/* __LINUX_LEDS_H_INCLUDED */

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

[Richard Purdie]

On Tue, 2011-04-05 at 22:24 +0200, Uwe Kleine-K?nig wrote:
> This function makes a deep copy of the platform data to allow it to live
> in init memory.
> The definition cannot go into leds-gpio.c because it needs to be builtin
> to be usable by platforms.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> changes since v1:
>  - don't add __init to the declaration of gpio_led_register_device
> 
>  drivers/leds/led-core.c |   27 ++++++++++++++++++++++++++-
>  include/linux/leds.h    |   12 ++++++++++++
>  2 files changed, 38 insertions(+), 1 deletions(-)

Can you explain the reasoning for this a little further please? It
sounds like instead of leaving the platform data in memory (which is
reasonable since we need it), we're now adding code to make a copy of
this data so we can remove the original. Why?

I have a rather strong dislike of adding "always builtin" functions as
they suggest something is wrong with the approach. led-core.c has always
been intentionally as minimal as we could get it.

In times when things like boot time are important it also seems like bad
form to be copying data around just so we can throw one copy away during
the boot process. What memory savings (which I assume is the
motivation?) is this giving us at what cost?

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
doesn't belong in the LED subsystem as its not an LED specific problem.

Cheers,

Richard

-- 
Linux Foundation
http://www.yoctoproject.org/

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

[Uwe Kleine-König]

Hello Richard,

On Wed, Apr 06, 2011 at 04:52:18AM -0700, Richard Purdie wrote:
> On Tue, 2011-04-05 at 22:24 +0200, Uwe Kleine-K?nig wrote:
> > This function makes a deep copy of the platform data to allow it to live
> > in init memory.
> > The definition cannot go into leds-gpio.c because it needs to be builtin
> > to be usable by platforms.
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> > changes since v1:
> >  - don't add __init to the declaration of gpio_led_register_device
> > 
> >  drivers/leds/led-core.c |   27 ++++++++++++++++++++++++++-
> >  include/linux/leds.h    |   12 ++++++++++++
> >  2 files changed, 38 insertions(+), 1 deletions(-)
> 
> Can you explain the reasoning for this a little further please? It
> sounds like instead of leaving the platform data in memory (which is
> reasonable since we need it), we're now adding code to make a copy of
> this data so we can remove the original. Why?
> 
> I have a rather strong dislike of adding "always builtin" functions as
> they suggest something is wrong with the approach. led-core.c has always
> been intentionally as minimal as we could get it.
> 
> In times when things like boot time are important it also seems like bad
> form to be copying data around just so we can throw one copy away during
> the boot process. What memory savings (which I assume is the
> motivation?) is this giving us at what cost?
> 
> 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?

> 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.

Best regards
Uwe

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/112485

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

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

[Richard Purdie]

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/

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

[Uwe Kleine-König]

This function makes a deep copy of the platform data to allow it to live
in init memory.
The definition cannot go into leds-gpio.c because it needs to be builtin
to be usable by platforms.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

changes since v2:
 - define gpio_led_register_device in a new file (led-register.c)
 - new parameter id for gpio_led_register_device
 - change drivers/Makefile to unconditionally include
   drivers/leds/Makefile

On Wed, Apr 06, 2011 at 06:38:17AM -0700, Richard Purdie wrote:
> 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).
I don't like your static inline suggestion but prefer a "select
SOMETHING" to make the function available. So what about this patch?

I choosed a generic name led-register.c, maybe in the future some more
leds want a similar function.

Best regards
Uwe

 drivers/Makefile            |    2 +-
 drivers/leds/Kconfig        |    5 +++++
 drivers/leds/Makefile       |    1 +
 drivers/leds/led-register.c |   33 +++++++++++++++++++++++++++++++++
 include/linux/leds.h        |   12 ++++++++++++
 5 files changed, 52 insertions(+), 1 deletions(-)
 create mode 100644 drivers/leds/led-register.c

diff --git a/drivers/Makefile b/drivers/Makefile
index 3f135b6..4a4f425 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -95,7 +95,7 @@ obj-$(CONFIG_CPU_IDLE)		+= cpuidle/
 obj-$(CONFIG_DMA_ENGINE)	+= dma/
 obj-$(CONFIG_MMC)		+= mmc/
 obj-$(CONFIG_MEMSTICK)		+= memstick/
-obj-$(CONFIG_NEW_LEDS)		+= leds/
+obj-y				+= leds/
 obj-$(CONFIG_INFINIBAND)	+= infiniband/
 obj-$(CONFIG_SGI_SN)		+= sn/
 obj-y				+= firmware/
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9bec869..e8e101e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -14,6 +14,11 @@ config LEDS_CLASS
 	  This option enables the led sysfs class in /sys/class/leds.  You'll
 	  need this to do anything useful with LEDs.  If unsure, say N.
 
+config LED_REGISTER_GPIO
+	bool
+	help
+	  This option provides the function gpio_led_register_device.
+
 if NEW_LEDS
 
 comment "LED drivers"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 39c80fc..ca428bd 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -3,6 +3,7 @@
 obj-$(CONFIG_NEW_LEDS)			+= led-core.o
 obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
+obj-y					+= led-register.o
 
 # LED Platform Drivers
 obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
diff --git a/drivers/leds/led-register.c b/drivers/leds/led-register.c
new file mode 100644
index 0000000..d3731ea
--- /dev/null
+++ b/drivers/leds/led-register.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
+ *
+ * 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.
+ */
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+
+#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
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 61e0340..b20474d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -207,5 +207,17 @@ struct gpio_led_platform_data {
 					unsigned long *delay_off);
 };
 
+/**
+ * gpio_led_register_device - register a gpio-led device
+ * @pdata: the platform data used for the new device
+ *
+ * Use this function instead of platform_device_add()ing a static struct
+ * platform_device.
+ *
+ * Note: as pdata and pdata->leds is copied these usually can and should be
+ * __initdata.
+ */
+struct platform_device *gpio_led_register_device(
+		int id, const struct gpio_led_platform_data *pdata);
 
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.2.3

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

[Russell King - ARM Linux]

On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9bec869..e8e101e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -14,6 +14,11 @@ config LEDS_CLASS
>  	  This option enables the led sysfs class in /sys/class/leds.  You'll
>  	  need this to do anything useful with LEDs.  If unsure, say N.
>  
> +config LED_REGISTER_GPIO
> +	bool
> +	help
> +	  This option provides the function gpio_led_register_device.
> +
>  if NEW_LEDS
>  
>  comment "LED drivers"
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 39c80fc..ca428bd 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -3,6 +3,7 @@
>  obj-$(CONFIG_NEW_LEDS)			+= led-core.o
>  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> +obj-y					+= led-register.o

Why not obj-$(CONFIG_LED_REGISTER_GPIO) += led-register.o

rather than wrapping the code of led-register.c with a #ifdef for the
same symbol?

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

[Uwe Kleine-König]

Hello,

On Tue, Apr 12, 2011 at 10:48:48PM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 9bec869..e8e101e 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -14,6 +14,11 @@ config LEDS_CLASS
> >  	  This option enables the led sysfs class in /sys/class/leds.  You'll
> >  	  need this to do anything useful with LEDs.  If unsure, say N.
> >  
> > +config LED_REGISTER_GPIO
> > +	bool
> > +	help
> > +	  This option provides the function gpio_led_register_device.
> > +
> >  if NEW_LEDS
> >  
> >  comment "LED drivers"
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 39c80fc..ca428bd 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -3,6 +3,7 @@
> >  obj-$(CONFIG_NEW_LEDS)			+= led-core.o
> >  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
> >  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> > +obj-y					+= led-register.o
> 
> Why not obj-$(CONFIG_LED_REGISTER_GPIO) += led-register.o
> 
> rather than wrapping the code of led-register.c with a #ifdef for the
> same symbol?
I thought that the registration for other led-devices could go into that
file, too. That's why I choosed the name led-register and not
leds-gpio-register.c. Agreed? I don't insist on that.

Best regards
Uwe

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

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

[Richard Purdie]

On Wed, 2011-04-13 at 08:23 +0200, Uwe Kleine-K?nig wrote:
> On Tue, Apr 12, 2011 at 10:48:48PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 9bec869..e8e101e 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -14,6 +14,11 @@ config LEDS_CLASS
> > >  	  This option enables the led sysfs class in /sys/class/leds.  You'll
> > >  	  need this to do anything useful with LEDs.  If unsure, say N.
> > >  
> > > +config LED_REGISTER_GPIO
> > > +	bool
> > > +	help
> > > +	  This option provides the function gpio_led_register_device.
> > > +
> > >  if NEW_LEDS
> > >  
> > >  comment "LED drivers"
> > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > index 39c80fc..ca428bd 100644
> > > --- a/drivers/leds/Makefile
> > > +++ b/drivers/leds/Makefile
> > > @@ -3,6 +3,7 @@
> > >  obj-$(CONFIG_NEW_LEDS)			+= led-core.o
> > >  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
> > >  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> > > +obj-y					+= led-register.o
> > 
> > Why not obj-$(CONFIG_LED_REGISTER_GPIO) += led-register.o
> > 
> > rather than wrapping the code of led-register.c with a #ifdef for the
> > same symbol?
> I thought that the registration for other led-devices could go into that
> file, too. That's why I choosed the name led-register and not
> leds-gpio-register.c. Agreed? I don't insist on that.

I'm not sure we want/need to put other registration functions in this
file? obj-$(CONFIG_LED_REGISTER_GPIO) probably therefore makes sense
until some other registration need arises.

Regardless, I'm happier with this patch than the previous ones. If you
change it to use obj-$(CONFIG_LED_REGISTER_GPIO), 

Acked-by: Richard Purdie <richard.purdie@linuxfoundation.org>

Cheers,

Richard

-- 
Linux Foundation
http://www.yoctoproject.org/

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

[Uwe Kleine-König]

Hello Richard,

On Fri, May 06, 2011 at 10:03:22PM +0100, Richard Purdie wrote:
> On Wed, 2011-04-13 at 08:23 +0200, Uwe Kleine-K?nig wrote:
> > On Tue, Apr 12, 2011 at 10:48:48PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> > > > +obj-y					+= led-register.o
> > > 
> > > Why not obj-$(CONFIG_LED_REGISTER_GPIO) += led-register.o
> > > 
> > > rather than wrapping the code of led-register.c with a #ifdef for the
> > > same symbol?
> > I thought that the registration for other led-devices could go into that
> > file, too. That's why I choosed the name led-register and not
> > leds-gpio-register.c. Agreed? I don't insist on that.
> 
> I'm not sure we want/need to put other registration functions in this
> file? obj-$(CONFIG_LED_REGISTER_GPIO) probably therefore makes sense
> until some other registration need arises.
OK, then I will name the file leds-gpio-register.c. This can be renamed
to led-register.c when/if other registrations follow.

> Regardless, I'm happier with this patch than the previous ones. If you
> change it to use obj-$(CONFIG_LED_REGISTER_GPIO), 
> 
> Acked-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Who will take the patch then? Andrew? Or is there someone else I need to
Cc?

Thanks
Uwe

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

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

[Uwe Kleine-König]

Hi Richard,

On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> This function makes a deep copy of the platform data to allow it to live
> in init memory.
> The definition cannot go into leds-gpio.c because it needs to be builtin
> to be usable by platforms.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
any comment by you?

Best regards
Uwe

> ---
> Hello,
> 
> changes since v2:
>  - define gpio_led_register_device in a new file (led-register.c)
>  - new parameter id for gpio_led_register_device
>  - change drivers/Makefile to unconditionally include
>    drivers/leds/Makefile
> 
> On Wed, Apr 06, 2011 at 06:38:17AM -0700, Richard Purdie wrote:
> > 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).
> I don't like your static inline suggestion but prefer a "select
> SOMETHING" to make the function available. So what about this patch?
> 
> I choosed a generic name led-register.c, maybe in the future some more
> leds want a similar function.
> 
> Best regards
> Uwe
> 
>  drivers/Makefile            |    2 +-
>  drivers/leds/Kconfig        |    5 +++++
>  drivers/leds/Makefile       |    1 +
>  drivers/leds/led-register.c |   33 +++++++++++++++++++++++++++++++++
>  include/linux/leds.h        |   12 ++++++++++++
>  5 files changed, 52 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/leds/led-register.c
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 3f135b6..4a4f425 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -95,7 +95,7 @@ obj-$(CONFIG_CPU_IDLE)		+= cpuidle/
>  obj-$(CONFIG_DMA_ENGINE)	+= dma/
>  obj-$(CONFIG_MMC)		+= mmc/
>  obj-$(CONFIG_MEMSTICK)		+= memstick/
> -obj-$(CONFIG_NEW_LEDS)		+= leds/
> +obj-y				+= leds/
>  obj-$(CONFIG_INFINIBAND)	+= infiniband/
>  obj-$(CONFIG_SGI_SN)		+= sn/
>  obj-y				+= firmware/
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9bec869..e8e101e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -14,6 +14,11 @@ config LEDS_CLASS
>  	  This option enables the led sysfs class in /sys/class/leds.  You'll
>  	  need this to do anything useful with LEDs.  If unsure, say N.
>  
> +config LED_REGISTER_GPIO
> +	bool
> +	help
> +	  This option provides the function gpio_led_register_device.
> +
>  if NEW_LEDS
>  
>  comment "LED drivers"
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 39c80fc..ca428bd 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -3,6 +3,7 @@
>  obj-$(CONFIG_NEW_LEDS)			+= led-core.o
>  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> +obj-y					+= led-register.o
>  
>  # LED Platform Drivers
>  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
> diff --git a/drivers/leds/led-register.c b/drivers/leds/led-register.c
> new file mode 100644
> index 0000000..d3731ea
> --- /dev/null
> +++ b/drivers/leds/led-register.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2011 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
> + *
> + * 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.
> + */
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/leds.h>
> +
> +#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
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 61e0340..b20474d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -207,5 +207,17 @@ struct gpio_led_platform_data {
>  					unsigned long *delay_off);
>  };
>  
> +/**
> + * gpio_led_register_device - register a gpio-led device
> + * @pdata: the platform data used for the new device
> + *
> + * Use this function instead of platform_device_add()ing a static struct
> + * platform_device.
> + *
> + * Note: as pdata and pdata->leds is copied these usually can and should be
> + * __initdata.
> + */
> +struct platform_device *gpio_led_register_device(
> +		int id, const struct gpio_led_platform_data *pdata);
>  
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Uwe Kleine-König]

Hi Richard,

On Tue, Apr 26, 2011 at 05:08:15PM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Apr 11, 2011 at 10:35:57PM +0200, Uwe Kleine-K?nig wrote:
> > This function makes a deep copy of the platform data to allow it to live
> > in init memory.
> > The definition cannot go into leds-gpio.c because it needs to be builtin
> > to be usable by platforms.
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> any comment by you?
ping

Best regards
Uwe

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

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

[Andrew Morton]

On Mon, 11 Apr 2011 22:35:57 +0200
Uwe Kleine-K__nig  <u.kleine-koenig@pengutronix.de> wrote:

> This function makes a deep copy of the platform data to allow it to live
> in init memory.
> The definition cannot go into leds-gpio.c because it needs to be builtin
> to be usable by platforms.
> 

Well...  why?  The changelog tells us what the function does but
provides no information explaining why you think it's needed, nor how
it is expected to be used, etc.

Please send a complete and useful changelog!

> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -14,6 +14,11 @@ config LEDS_CLASS
>  	  This option enables the led sysfs class in /sys/class/leds.  You'll
>  	  need this to do anything useful with LEDs.  If unsure, say N.
>  
> +config LED_REGISTER_GPIO
> +	bool
> +	help
> +	  This option provides the function gpio_led_register_device.
> +

Every other LEDS Kconfig symbol uses "LEDS", not "LED".  I'll make that
change.

>  if NEW_LEDS
>  
>  comment "LED drivers"
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 39c80fc..ca428bd 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -3,6 +3,7 @@
>  obj-$(CONFIG_NEW_LEDS)			+= led-core.o
>  obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
> +obj-y					+= led-register.o
>  
>  # LED Platform Drivers
>  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
> diff --git a/drivers/leds/led-register.c b/drivers/leds/led-register.c
> new file mode 100644
> index 0000000..d3731ea
> --- /dev/null
> +++ b/drivers/leds/led-register.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2011 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
> + *
> + * 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.
> + */
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/leds.h>
> +
> +#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
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 61e0340..b20474d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -207,5 +207,17 @@ struct gpio_led_platform_data {
>  					unsigned long *delay_off);
>  };
>  
> +/**
> + * gpio_led_register_device - register a gpio-led device
> + * @pdata: the platform data used for the new device
> + *
> + * Use this function instead of platform_device_add()ing a static struct
> + * platform_device.
> + *
> + * Note: as pdata and pdata->leds is copied these usually can and should be
> + * __initdata.
> + */
> +struct platform_device *gpio_led_register_device(
> +		int id, const struct gpio_led_platform_data *pdata);

It's unusual to document functions in the .h file.  There's a bit of
precedent for that in leds.h, but there is more precedent in
drivers/leds/*.c

Personally I think that putting the documentation in .h is rather sucky
- it happens so rarely that one just doesn't think of looking in there.

The description isn't terribly useful if the reader doesn't know what
"platform_device_add()ing a static struct platform_device" is for.  Can
we describe this in some manner whcih doesn't refer to something else
which is probably undocumented?

The comment doesn't document return values.

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

[Russell King - ARM Linux]

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@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.

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.

[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@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/  |

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

[Uwe Kleine-König]

Hi Andrew,

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@pengutronix.de> wrote:
> 
> > This function makes a deep copy of the platform data to allow it to live
> > in init memory.
> > The definition cannot go into leds-gpio.c because it needs to be builtin
> > to be usable by platforms.
> > 
> 
> Well...  why?  The changelog tells us what the function does but
> provides no information explaining why you think it's needed, nor how
> it is expected to be used, etc.
> 
> Please send a complete and useful changelog!
OK, will try to do better for v4.

> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -14,6 +14,11 @@ config LEDS_CLASS
> >  	  This option enables the led sysfs class in /sys/class/leds.  You'll
> >  	  need this to do anything useful with LEDs.  If unsure, say N.
> >  
> > +config LED_REGISTER_GPIO
> > +	bool
> > +	help
> > +	  This option provides the function gpio_led_register_device.
> > +
> 
> Every other LEDS Kconfig symbol uses "LEDS", not "LED".  I'll make that
> change.
I used LED only to match led-register.c. There led- seemed reasonable to
me because it's only the drivers that use leds-, but there is
led-core.o, led-class.o and led-triggers.o.
Hmm, I don't care much.
 
> >  if NEW_LEDS
> >  
> >  comment "LED drivers"
> > [...]
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -207,5 +207,17 @@ struct gpio_led_platform_data {
> >  					unsigned long *delay_off);
> >  };
> >  
> > +/**
> > + * gpio_led_register_device - register a gpio-led device
> > + * @pdata: the platform data used for the new device
> > + *
> > + * Use this function instead of platform_device_add()ing a static struct
> > + * platform_device.
> > + *
> > + * Note: as pdata and pdata->leds is copied these usually can and should be
> > + * __initdata.
> > + */
> > +struct platform_device *gpio_led_register_device(
> > +		int id, const struct gpio_led_platform_data *pdata);
> 
> It's unusual to document functions in the .h file.  There's a bit of
> precedent for that in leds.h, but there is more precedent in
> drivers/leds/*.c
> 
> Personally I think that putting the documentation in .h is rather sucky
> - it happens so rarely that one just doesn't think of looking in there.
> 
> The description isn't terribly useful if the reader doesn't know what
> "platform_device_add()ing a static struct platform_device" is for.  Can
> we describe this in some manner whcih doesn't refer to something else
> which is probably undocumented?
> 
> The comment doesn't document return values.
OK, will fix in v4.

Best regards
Uwe

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

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

[Uwe Kleine-König]

This function makes a deep copy of the platform data to allow it to live
in init memory. For a kernel that supports several machines and so
includes the definition for several leds-gpio devices this saves quite
some memory because all but one definition can be free'd after boot.

As the function is used by arch code it must be builtin and so cannot go
into leds-gpio.c.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Changes since v3:
- new commit log
- improved documentation and moved it to .c file
- used obj-$(CONFIG..) and renamed source file
- provide an example usage (in a seperate patch)

 drivers/Makefile                  |    2 +-
 drivers/leds/Kconfig              |    7 ++++++
 drivers/leds/Makefile             |    1 +
 drivers/leds/leds-gpio-register.c |   42 +++++++++++++++++++++++++++++++++++++
 include/linux/leds.h              |    2 +
 5 files changed, 53 insertions(+), 1 deletions(-)
 create mode 100644 drivers/leds/leds-gpio-register.c

diff --git a/drivers/Makefile b/drivers/Makefile
index 3f135b6..4a4f425 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -95,7 +95,7 @@ obj-$(CONFIG_CPU_IDLE)		+= cpuidle/
 obj-$(CONFIG_DMA_ENGINE)	+= dma/
 obj-$(CONFIG_MMC)		+= mmc/
 obj-$(CONFIG_MEMSTICK)		+= memstick/
-obj-$(CONFIG_NEW_LEDS)		+= leds/
+obj-y				+= leds/
 obj-$(CONFIG_INFINIBAND)	+= infiniband/
 obj-$(CONFIG_SGI_SN)		+= sn/
 obj-y				+= firmware/
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9bec869..eb832dc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -14,6 +14,13 @@ config LEDS_CLASS
 	  This option enables the led sysfs class in /sys/class/leds.  You'll
 	  need this to do anything useful with LEDs.  If unsure, say N.
 
+config LEDS_GPIO_REGISTER
+	bool
+	help
+	  This option provides the function gpio_led_register_device.
+	  As this function is used by arch code it must not be compiled as a
+	  module.
+
 if NEW_LEDS
 
 comment "LED drivers"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 39c80fc..8439ce5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
 obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
 obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
 obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
+obj-$(CONFIG_LEDS_GPIO_REGISTER)	+= leds-gpio-register.o
 obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
 obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
 obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
diff --git a/drivers/leds/leds-gpio-register.c b/drivers/leds/leds-gpio-register.c
new file mode 100644
index 0000000..1c4ed55
--- /dev/null
+++ b/drivers/leds/leds-gpio-register.c
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2011 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
+ *
+ * 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.
+ */
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+
+/**
+ * gpio_led_register_device - register a gpio-led device
+ * @pdata: the platform data used for the new device
+ *
+ * Makes a copy of pdata and pdata->leds and registers a new leds-gpio device
+ * with the result. This allows to have pdata and pdata-leds in .init.rodata
+ * and so saves some bytes compared to a static struct platform_device with
+ * static platform data.
+ *
+ * Returns the registered device or an error pointer.
+ */
+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;
+}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 61e0340..5884def 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -207,5 +207,7 @@ struct gpio_led_platform_data {
 					unsigned long *delay_off);
 };
 
+struct platform_device *gpio_led_register_device(
+		int id, const struct gpio_led_platform_data *pdata);
 
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.4.1

[PATCH] [wip] ARM: imx: register "leds-gpio" device using new helper function

[Uwe Kleine-König]

This converts eukrea_mbimx27-baseboard to the new helper function.

bloat-o-meter reports for this change:

	add/remove: 1/1 grow/shrink: 0/1 up/down: 128/-220 (-92)
	function                                     old     new   delta
	gpio_led_register_device                       -     128    +128
	platform_devices                              28      24      -4
	leds_gpio                                    216       -    -216

Additionally gpio_led_info (12 bytes) and gpio_leds (32 Bytes) are
initdata now as is gpio_led_register_device.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
This is just an example and probably doesn't apply to the imx tree as
is. I will convert all imx machines when there's an agreement for the
patch providing the helper function.

 arch/arm/mach-imx/Kconfig                    |    1 +
 arch/arm/mach-imx/eukrea_mbimx27-baseboard.c |   18 +++---------------
 2 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 56b930a..ef16471 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -222,6 +222,7 @@ choice
 
 config MACH_EUKREA_MBIMX27_BASEBOARD
 	bool "Eukrea MBIMX27 development board"
+	select LEDS_GPIO_REGISTER
 	select IMX_HAVE_PLATFORM_IMX_FB
 	select IMX_HAVE_PLATFORM_IMX_KEYPAD
 	select IMX_HAVE_PLATFORM_IMX_SSI
diff --git a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
index fa5288018..3479f66 100644
--- a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
+++ b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
@@ -113,7 +113,7 @@ eukrea_mbimx27_keymap_data __initconst = {
 	.keymap_size    = ARRAY_SIZE(eukrea_mbimx27_keymap),
 };
 
-static struct gpio_led gpio_leds[] = {
+static const struct gpio_led gpio_leds[] __initconst = {
 	{
 		.name			= "led1",
 		.default_trigger	= "heartbeat",
@@ -128,19 +128,11 @@ static struct gpio_led gpio_leds[] = {
 	},
 };
 
-static struct gpio_led_platform_data gpio_led_info = {
+static const struct gpio_led_platform_data gpio_led_info __initconst = {
 	.leds		= gpio_leds,
 	.num_leds	= ARRAY_SIZE(gpio_leds),
 };
 
-static struct platform_device leds_gpio = {
-	.name	= "leds-gpio",
-	.id	= -1,
-	.dev	= {
-		.platform_data	= &gpio_led_info,
-	},
-};
-
 static struct imx_fb_videomode eukrea_mbimx27_modes[] = {
 	{
 		.mode = {
@@ -294,10 +286,6 @@ static struct i2c_board_info eukrea_mbimx27_i2c_devices[] = {
 	},
 };
 
-static struct platform_device *platform_devices[] __initdata = {
-	&leds_gpio,
-};
-
 static const struct imxmmc_platform_data sdhc_pdata __initconst = {
 	.dat3_card_detect = 1,
 };
@@ -378,5 +366,5 @@ void __init eukrea_mbimx27_baseboard_init(void)
 
 	imx27_add_imx_keypad(&eukrea_mbimx27_keymap_data);
 
-	platform_add_devices(platform_devices, ARRAY_SIZE(platform_devices));
+	gpio_led_register_device(-1, &gpio_led_info);
 }
-- 
1.7.4.1

[PATCH] [wip] ARM: imx: register "leds-gpio" device using new helper function

[H Hartley Sweeten]

On Tuesday, May 10, 2011 1:51 AM, Uwe Kleine-K?nig wrote:
> This converts eukrea_mbimx27-baseboard to the new helper function.
>
> bloat-o-meter reports for this change:
>
>	add/remove: 1/1 grow/shrink: 0/1 up/down: 128/-220 (-92)
>	function                                     old     new   delta
>	gpio_led_register_device                       -     128    +128
>	platform_devices                              28      24      -4
>	leds_gpio                                    216       -    -216
>
> Additionally gpio_led_info (12 bytes) and gpio_leds (32 Bytes) are
> initdata now as is gpio_led_register_device.
>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> This is just an example and probably doesn't apply to the imx tree as
> is. I will convert all imx machines when there's an agreement for the
> patch providing the helper function.
>
>  arch/arm/mach-imx/Kconfig                    |    1 +
>  arch/arm/mach-imx/eukrea_mbimx27-baseboard.c |   18 +++---------------
>  2 files changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 56b930a..ef16471 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -222,6 +222,7 @@ choice
> 
>  config MACH_EUKREA_MBIMX27_BASEBOARD
>  	bool "Eukrea MBIMX27 development board"
> +	select LEDS_GPIO_REGISTER
>  	select IMX_HAVE_PLATFORM_IMX_FB
>  	select IMX_HAVE_PLATFORM_IMX_KEYPAD
>  	select IMX_HAVE_PLATFORM_IMX_SSI
> diff --git a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> index fa5288018..3479f66 100644
> --- a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> +++ b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> @@ -113,7 +113,7 @@ eukrea_mbimx27_keymap_data __initconst = {
>  	.keymap_size    = ARRAY_SIZE(eukrea_mbimx27_keymap),
>  };
>  
> -static struct gpio_led gpio_leds[] = {
> +static const struct gpio_led gpio_leds[] __initconst = {
>  	{
>  		.name			= "led1",
>  		.default_trigger	= "heartbeat",
> @@ -128,19 +128,11 @@ static struct gpio_led gpio_leds[] = {
>  	},
>  };
>  
> -static struct gpio_led_platform_data gpio_led_info = {
> +static const struct gpio_led_platform_data gpio_led_info __initconst = {
>  	.leds		= gpio_leds,
>  	.num_leds	= ARRAY_SIZE(gpio_leds),
>  };

Uwe,

Just a note that the 'const' you added to struct gpio_led above will be
discarded in struct gpio_led_platform_data.  You will get something like:

arch/arm/mach-imx/eukrea_mbimx27-baseboard.c:132: warning: initialization discards qualifiers from pointer target type

Regards,
Hartley

[PATCH] [wip] ARM: imx: register "leds-gpio" device using new helper function

[Uwe Kleine-König]

Hello Hartley,

On Tue, May 10, 2011 at 05:26:18PM -0500, H Hartley Sweeten wrote:
> On Tuesday, May 10, 2011 1:51 AM, Uwe Kleine-K?nig wrote:
> > diff --git a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> > index fa5288018..3479f66 100644
> > --- a/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> > +++ b/arch/arm/mach-imx/eukrea_mbimx27-baseboard.c
> > @@ -113,7 +113,7 @@ eukrea_mbimx27_keymap_data __initconst = {
> >  	.keymap_size    = ARRAY_SIZE(eukrea_mbimx27_keymap),
> >  };
> >  
> > -static struct gpio_led gpio_leds[] = {
> > +static const struct gpio_led gpio_leds[] __initconst = {
> >  	{
> >  		.name			= "led1",
> >  		.default_trigger	= "heartbeat",
> > @@ -128,19 +128,11 @@ static struct gpio_led gpio_leds[] = {
> >  	},
> >  };
> >  
> > -static struct gpio_led_platform_data gpio_led_info = {
> > +static const struct gpio_led_platform_data gpio_led_info __initconst = {
> >  	.leds		= gpio_leds,
> >  	.num_leds	= ARRAY_SIZE(gpio_leds),
> >  };
> 
> Just a note that the 'const' you added to struct gpio_led above will be
> discarded in struct gpio_led_platform_data.  You will get something like:
> 
> arch/arm/mach-imx/eukrea_mbimx27-baseboard.c:132: warning: initialization discards qualifiers from pointer target type
It seems you don't have

	9517f92 (leds: make *struct gpio_led_platform_data.leds const)

(contained in .39-rc1) in your tree :-)

Best regards
Uwe

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

[PATCH] [wip] ARM: imx: register "leds-gpio" device using new helper function

[H Hartley Sweeten]

On Tuesday, May 10, 2011 1:51 AM, Uwe Kleine-K?nig wrote:
>
> This converts eukrea_mbimx27-baseboard to the new helper function.
>
> bloat-o-meter reports for this change:
>
>	add/remove: 1/1 grow/shrink: 0/1 up/down: 128/-220 (-92)
>	function                                     old     new   delta
>	gpio_led_register_device                       -     128    +128
>	platform_devices                              28      24      -4
>	leds_gpio                                    216       -    -216
>
> Additionally gpio_led_info (12 bytes) and gpio_leds (32 Bytes) are
> initdata now as is gpio_led_register_device.

FWIW, similar results with ep93xx:

add/remove: 1/1 grow/shrink: 2/0 up/down: 137/-240 (-103)
function                                     old     new   delta
gpio_led_register_device                       -     128    +128
kernel_config_data                         10474   10479      +5
ep93xx_init_devices                          116     120      +4
ep93xx_leds                                  240       -    -240

Regards,
Hartley

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

[Andrew Morton]

On Tue, 5 Apr 2011 17:33:39 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Apr 05, 2011 at 10:37:35AM +0200, Uwe Kleine-K__nig wrote:
> > +struct platform_device *__init gpio_led_register_device(
> > +		const struct gpio_led_platform_data *pdata);
> 
> Please don't add __init annotations to declarations.

Reasons?

A year or so ago we had to *add* an __init to a declaration, because
one architecture was generating a short-mode-addressing relative branch
to the callee, assuming the target was in the same section as the call
site.  When the linker went to resolve the branch, it discovered that
the target was in fact in a different section and was too far away to
be able to use the short-mode addressing.  IIRC, that architecture was
arm.

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

[Russell King - ARM Linux]

On Tue, Apr 19, 2011 at 04:19:13PM -0700, Andrew Morton wrote:
> On Tue, 5 Apr 2011 17:33:39 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Tue, Apr 05, 2011 at 10:37:35AM +0200, Uwe Kleine-K__nig wrote:
> > > +struct platform_device *__init gpio_led_register_device(
> > > +		const struct gpio_led_platform_data *pdata);
> > 
> > Please don't add __init annotations to declarations.
> 
> Reasons?

It's noise.

> A year or so ago we had to *add* an __init to a declaration, because
> one architecture was generating a short-mode-addressing relative branch
> to the callee, assuming the target was in the same section as the call
> site.  When the linker went to resolve the branch, it discovered that
> the target was in fact in a different section and was too far away to
> be able to use the short-mode addressing.  IIRC, that architecture was
> arm.

I'm not aware of ARM ever requiring that.  If it was, we'd have to add
a heck of a lot of those annotations.  And those annotations don't tell
the compiler that it might be far away.

We _do_ have a problem if someone decides to include a big ramdisk or
initramfs image in the discarded section, which is something I have a
patch kicking around to completely change the vmlinux layout to resolve.
That's taking something of a low priority at the moment though as we've
been turned upside down by Linus...

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

[Andrew Morton]

On Wed, 20 Apr 2011 00:24:20 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Apr 19, 2011 at 04:19:13PM -0700, Andrew Morton wrote:
> > On Tue, 5 Apr 2011 17:33:39 +0100
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 
> > > On Tue, Apr 05, 2011 at 10:37:35AM +0200, Uwe Kleine-K__nig wrote:
> > > > +struct platform_device *__init gpio_led_register_device(
> > > > +		const struct gpio_led_platform_data *pdata);
> > > 
> > > Please don't add __init annotations to declarations.
> > 
> > Reasons?
> 
> It's noise.
> 
> > A year or so ago we had to *add* an __init to a declaration, because
> > one architecture was generating a short-mode-addressing relative branch
> > to the callee, assuming the target was in the same section as the call
> > site.  When the linker went to resolve the branch, it discovered that
> > the target was in fact in a different section and was too far away to
> > be able to use the short-mode addressing.  IIRC, that architecture was
> > arm.
> 
> I'm not aware of ARM ever requiring that.  If it was, we'd have to add
> a heck of a lot of those annotations.  And those annotations don't tell
> the compiler that it might be far away.
> 
> We _do_ have a problem if someone decides to include a big ramdisk or
> initramfs image in the discarded section, which is something I have a
> patch kicking around to completely change the vmlinux layout to resolve.
> That's taking something of a low priority at the moment though as we've
> been turned upside down by Linus...

<spends 20 minutes hunting>

OK, maybe it was the below patch in which case I misremembered - this
patch is FRV and __meminitdata.

But I do have a feeling that we had a similar problem with .text ->
.init.text references.


commit 9dec17eb577169f78d642c5424e4264186d27115
Author:     David Howells <dhowells@redhat.com>
AuthorDate: Mon Jul 10 04:44:51 2006 -0700
Commit:     Linus Torvalds <torvalds@g5.osdl.org>
CommitDate: Mon Jul 10 13:24:21 2006 -0700

    [PATCH] FRV: Fix FRV arch compile errors
    
    Fix some FRV arch compile errors, including:
    
     (*) Marking nr_kernel_pages as __meminitdata so that references to it end up
         being properly calculated rather than being assumed to be in the small
         data section (and thus calculated wrt the GP register).  Not doing this
         causes the linker to emit errors as the offset is too big to fit into the
         load instruction.
    
     (*) Move pm_power_off into an unconditionally compiled .c file as it's now
         unconditionally accessed.
    
     (*) Declare frv_change_cmode() in a header file rather than in a .c file, and
         declare it asmlinkage.
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/arch/frv/kernel/local.h b/arch/frv/kernel/local.h
index e947176..76606d1 100644
--- a/arch/frv/kernel/local.h
+++ b/arch/frv/kernel/local.h
@@ -51,6 +51,9 @@ extern void (*__power_switch_wake_cleanup)(void);
 /* time.c */
 extern void time_divisor_init(void);
 
+/* cmode.S */
+extern asmlinkage void frv_change_cmode(int);
+
 
 #endif /* __ASSEMBLY__ */
 #endif /* _FRV_LOCAL_H */
diff --git a/arch/frv/kernel/pm.c b/arch/frv/kernel/pm.c
index e65a9f1..c1d9fc8 100644
--- a/arch/frv/kernel/pm.c
+++ b/arch/frv/kernel/pm.c
@@ -26,11 +26,6 @@
 
 #include "local.h"
 
-void (*pm_power_off)(void);
-EXPORT_SYMBOL(pm_power_off);
-
-extern void frv_change_cmode(int);
-
 /*
  * Debug macros
  */
diff --git a/arch/frv/kernel/process.c b/arch/frv/kernel/process.c
index eeeb1e2..ecdeafb 100644
--- a/arch/frv/kernel/process.c
+++ b/arch/frv/kernel/process.c
@@ -10,6 +10,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/sched.h>
 #include <linux/kernel.h>
@@ -38,6 +39,9 @@ asmlinkage void ret_from_fork(void);
 
 #include <asm/pgalloc.h>
 
+void (*pm_power_off)(void);
+EXPORT_SYMBOL(pm_power_off);
+
 struct task_struct *alloc_task_struct(void)
 {
 	struct task_struct *p = kmalloc(THREAD_SIZE, GFP_KERNEL);
diff --git a/arch/frv/mb93090-mb00/pci-vdk.c b/arch/frv/mb93090-mb00/pci-vdk.c
index fb98e90..f7279d7 100644
--- a/arch/frv/mb93090-mb00/pci-vdk.c
+++ b/arch/frv/mb93090-mb00/pci-vdk.c
@@ -406,7 +406,9 @@ int __init pcibios_init(void)
 	ioport_resource.end	= (__reg_MB86943_sl_pci_io_range << 9) | 0x3ff;
 	ioport_resource.end	+= ioport_resource.start;
 
-	printk("PCI IO window:  %08lx-%08lx\n", ioport_resource.start, ioport_resource.end);
+	printk("PCI IO window:  %08llx-%08llx\n",
+	       (unsigned long long) ioport_resource.start,
+	       (unsigned long long) ioport_resource.end);
 
 	iomem_resource.start	= (__reg_MB86943_sl_pci_mem_base << 9) & 0xfffffc00;
 
@@ -416,8 +418,11 @@ int __init pcibios_init(void)
 	iomem_resource.end	= (__reg_MB86943_sl_pci_mem_range << 9) | 0x3ff;
 	iomem_resource.end	+= iomem_resource.start;
 
-	printk("PCI MEM window: %08lx-%08lx\n", iomem_resource.start, iomem_resource.end);
-	printk("PCI DMA memory: %08lx-%08lx\n", dma_coherent_mem_start, dma_coherent_mem_end);
+	printk("PCI MEM window: %08llx-%08llx\n",
+	       (unsigned long long) iomem_resource.start,
+	       (unsigned long long) iomem_resource.end);
+	printk("PCI DMA memory: %08lx-%08lx\n",
+	       dma_coherent_mem_start, dma_coherent_mem_end);
 
 	if (!pci_probe)
 		return -ENXIO;
diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
index 22866fa..1021f50 100644
--- a/include/linux/bootmem.h
+++ b/include/linux/bootmem.h
@@ -91,7 +91,7 @@ static inline void *alloc_remap(int nid, unsigned long size)
 }
 #endif
 
-extern unsigned long nr_kernel_pages;
+extern unsigned long __meminitdata nr_kernel_pages;
 extern unsigned long nr_all_pages;
 
 extern void *__init alloc_large_system_hash(const char *tablename,

[PATCH v2 1/2] ARM: mxc: Introduce imx_add_gpio_leds

[H Hartley Sweeten]

On Tuesday, April 05, 2011 12:31 AM, Uwe Kleine-K?nig wrote:
> I like my approach better, without using mxc specific functions it would
> look as follows:
>
> struct platform_device *__init gpio_led_register_device(
> 		const gpio_led_platform_data *pdata)
> {
> 	struct platform_device *ret = ERR_PTR(-ENOMEM);
> 	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", -1,
> 		NULL, 0, _pdata, sizeof(_pdata));
> 
> 	if (IS_ERR(ret))
> 		kfree(_pdata->leds);
> 
> 	return ret;
> }
>
> Compared to your approach with mine
>  - gpio_leds could be const __initconst;
>  - uses only a single parameter which AFAIK saves a few bytes to call
>    the function;
>  - you don't have control about the .id member (which IMHO is OK but
>    might be changed easily);
>  - you have to call a function with a different name;

Your approach is fine with me.

I think you should pass the .id number.  I noticed a couple platforms that
register more than one leds-gpio platform device and some that currently
use and id of 0.

Regards,
Hartley