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 <[email protected]>
---
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 <[email protected]>
*
- * Author: Richard Purdie <[email protected]>
+ * Copyright (C) 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <[email protected]>
*
* 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
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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.
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 <[email protected]>
>>
>> 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
---
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 <[email protected]>
*
- * Author: Richard Purdie <[email protected]>
+ * Copyright (C) 2009-2010 Pengutronix
+ * Uwe Kleine-Koenig <[email protected]>
*
* 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
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 <[email protected]>
Tested-by: Fabio Estevam <[email protected]>
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 <[email protected]>
> *
> - * Author: Richard Purdie <[email protected]>
> + * Copyright (C) 2009-2010 Pengutronix
> + * Uwe Kleine-Koenig <[email protected]>
> *
> * 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 */
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 <[email protected]>
> ---
> 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/
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 <[email protected]>
> > ---
> > 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/ |
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/
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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?
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/ |
On Tue, 5 Apr 2011 17:33:39 +0100
Russell King - ARM Linux <[email protected]> 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.
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 <[email protected]> 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...
On Wed, 20 Apr 2011 00:24:20 +0100
Russell King - ARM Linux <[email protected]> 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 <[email protected]> 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 <[email protected]>
AuthorDate: Mon Jul 10 04:44:51 2006 -0700
Commit: Linus Torvalds <[email protected]>
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 <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
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,
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 <[email protected]>
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 <[email protected]>
> + *
> + * 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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 <[email protected]>
> any comment by you?
ping
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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 <[email protected]>
Cheers,
Richard
--
Linux Foundation
http://www.yoctoproject.org/
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 <[email protected]>
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/ |
On Mon, 11 Apr 2011 22:35:57 +0200
Uwe Kleine-K__nig <[email protected]> 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 <[email protected]>
> + *
> + * 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.
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 <[email protected]> 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.
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 <[email protected]> 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/ |
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 <[email protected]> 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/ |
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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/ |