* [PATCH] leds: lp55xx: make various arrays static const
@ 2017-06-29 17:57 Colin King
2017-06-29 19:40 ` Jacek Anaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Colin King @ 2017-06-29 17:57 UTC (permalink / raw)
To: Richard Purdie, Jacek Anaszewski, Pavel Machek, linux-leds
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Several arrays are currently on-stack and instead should be made
static const.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/leds/leds-lp5523.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index e9ba8cd32d66..924e50aefb00 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -168,13 +168,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
static void lp5523_load_engine(struct lp55xx_chip *chip)
{
enum lp55xx_engine_index idx = chip->engine_idx;
- u8 mask[] = {
+ static const u8 mask[] = {
[LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
[LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
[LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
};
- u8 val[] = {
+ static const u8 val[] = {
[LP55XX_ENGINE_1] = LP5523_LOAD_ENG1,
[LP55XX_ENGINE_2] = LP5523_LOAD_ENG2,
[LP55XX_ENGINE_3] = LP5523_LOAD_ENG3,
@@ -188,7 +188,7 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
{
enum lp55xx_engine_index idx = chip->engine_idx;
- u8 page_sel[] = {
+ static const u8 page_sel[] = {
[LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
[LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
[LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
@@ -208,7 +208,7 @@ static void lp5523_stop_all_engines(struct lp55xx_chip *chip)
static void lp5523_stop_engine(struct lp55xx_chip *chip)
{
enum lp55xx_engine_index idx = chip->engine_idx;
- u8 mask[] = {
+ static const u8 mask[] = {
[LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
[LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
[LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
@@ -505,7 +505,7 @@ static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
{
struct lp55xx_engine *engine = &chip->engines[nr - 1];
int ret;
- u8 mux_page[] = {
+ static const u8 mux_page[] = {
[LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
[LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
[LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] leds: lp55xx: make various arrays static const
2017-06-29 17:57 [PATCH] leds: lp55xx: make various arrays static const Colin King
@ 2017-06-29 19:40 ` Jacek Anaszewski
2017-06-29 20:48 ` Pavel Machek
0 siblings, 1 reply; 4+ messages in thread
From: Jacek Anaszewski @ 2017-06-29 19:40 UTC (permalink / raw)
To: Colin King, Richard Purdie, Pavel Machek, linux-leds
Cc: kernel-janitors, linux-kernel
Hi Colin,
Thanks for the patch.
Do you have some profiling results showing the benefit of these changes?
It seems that these functions are called only on driver initialization.
Making the local arrays static will prevent releasing this memory even
though it will be no longer needed.
Anyway, the size of arrays is 3 bytes, so the impact of these changes
seems to be virtually negligible. I am not taking this patch unless
you are able to provide the numbers showing the gain.
Best regards,
Jacek Anaszewski
On 06/29/2017 07:57 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Several arrays are currently on-stack and instead should be made
> static const.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/leds/leds-lp5523.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index e9ba8cd32d66..924e50aefb00 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -168,13 +168,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
> static void lp5523_load_engine(struct lp55xx_chip *chip)
> {
> enum lp55xx_engine_index idx = chip->engine_idx;
> - u8 mask[] = {
> + static const u8 mask[] = {
> [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
> [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
> [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
> };
>
> - u8 val[] = {
> + static const u8 val[] = {
> [LP55XX_ENGINE_1] = LP5523_LOAD_ENG1,
> [LP55XX_ENGINE_2] = LP5523_LOAD_ENG2,
> [LP55XX_ENGINE_3] = LP5523_LOAD_ENG3,
> @@ -188,7 +188,7 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
> static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
> {
> enum lp55xx_engine_index idx = chip->engine_idx;
> - u8 page_sel[] = {
> + static const u8 page_sel[] = {
> [LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
> [LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
> [LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
> @@ -208,7 +208,7 @@ static void lp5523_stop_all_engines(struct lp55xx_chip *chip)
> static void lp5523_stop_engine(struct lp55xx_chip *chip)
> {
> enum lp55xx_engine_index idx = chip->engine_idx;
> - u8 mask[] = {
> + static const u8 mask[] = {
> [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
> [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
> [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
> @@ -505,7 +505,7 @@ static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
> {
> struct lp55xx_engine *engine = &chip->engines[nr - 1];
> int ret;
> - u8 mux_page[] = {
> + static const u8 mux_page[] = {
> [LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
> [LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
> [LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] leds: lp55xx: make various arrays static const
2017-06-29 19:40 ` Jacek Anaszewski
@ 2017-06-29 20:48 ` Pavel Machek
2017-06-30 21:26 ` Jacek Anaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Pavel Machek @ 2017-06-29 20:48 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Colin King, Richard Purdie, linux-leds, kernel-janitors,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]
Hi!
> Thanks for the patch.
>
> Do you have some profiling results showing the benefit of these changes?
> It seems that these functions are called only on driver initialization.
> Making the local arrays static will prevent releasing this memory even
> though it will be no longer needed.
>
> Anyway, the size of arrays is 3 bytes, so the impact of these changes
> seems to be virtually negligible. I am not taking this patch unless
> you are able to provide the numbers showing the gain.
C will have to initialize the array on stack, then copy values
there.. which is ugly. "static const" also tells the reader what is
going on there (nothing).
I'd say this is a good patch.
Acked-by: Pavel Machek <pavel@ucw.cz>
Pavel
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -168,13 +168,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
> > static void lp5523_load_engine(struct lp55xx_chip *chip)
> > {
> > enum lp55xx_engine_index idx = chip->engine_idx;
> > - u8 mask[] = {
> > + static const u8 mask[] = {
> > [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
> > [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
> > [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
> > };
> >
> > - u8 val[] = {
> > + static const u8 val[] = {
> > [LP55XX_ENGINE_1] = LP5523_LOAD_ENG1,
> > [LP55XX_ENGINE_2] = LP5523_LOAD_ENG2,
> > [LP55XX_ENGINE_3] = LP5523_LOAD_ENG3,
> > @@ -188,7 +188,7 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
> > static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
> > {
> > enum lp55xx_engine_index idx = chip->engine_idx;
> > - u8 page_sel[] = {
> > + static const u8 page_sel[] = {
> > [LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
> > [LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
> > [LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
> > @@ -208,7 +208,7 @@ static void lp5523_stop_all_engines(struct lp55xx_chip *chip)
> > static void lp5523_stop_engine(struct lp55xx_chip *chip)
> > {
> > enum lp55xx_engine_index idx = chip->engine_idx;
> > - u8 mask[] = {
> > + static const u8 mask[] = {
> > [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
> > [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
> > [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
> > @@ -505,7 +505,7 @@ static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
> > {
> > struct lp55xx_engine *engine = &chip->engines[nr - 1];
> > int ret;
> > - u8 mux_page[] = {
> > + static const u8 mux_page[] = {
> > [LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
> > [LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
> > [LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
> >
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] leds: lp55xx: make various arrays static const
2017-06-29 20:48 ` Pavel Machek
@ 2017-06-30 21:26 ` Jacek Anaszewski
0 siblings, 0 replies; 4+ messages in thread
From: Jacek Anaszewski @ 2017-06-30 21:26 UTC (permalink / raw)
To: Pavel Machek
Cc: Colin King, Richard Purdie, linux-leds, kernel-janitors,
linux-kernel
Hi,
On 06/29/2017 10:48 PM, Pavel Machek wrote:
> Hi!
>
>> Thanks for the patch.
>>
>> Do you have some profiling results showing the benefit of these changes?
>> It seems that these functions are called only on driver initialization.
>> Making the local arrays static will prevent releasing this memory even
>> though it will be no longer needed.
>>
>> Anyway, the size of arrays is 3 bytes, so the impact of these changes
>> seems to be virtually negligible. I am not taking this patch unless
>> you are able to provide the numbers showing the gain.
>
> C will have to initialize the array on stack, then copy values
OK, similar justification is used in case of few other similar patches.
Applied to the for-next branch of linux-leds.git. It's rather
cosmetic change so only a few days of testing before sending
upstream should be enough.
Thanks,
Jacek Anaszewski
> there.. which is ugly. "static const" also tells the reader what is
> going on there (nothing).
>
> I'd say this is a good patch.
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
>>> +++ b/drivers/leds/leds-lp5523.c
>>> @@ -168,13 +168,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
>>> static void lp5523_load_engine(struct lp55xx_chip *chip)
>>> {
>>> enum lp55xx_engine_index idx = chip->engine_idx;
>>> - u8 mask[] = {
>>> + static const u8 mask[] = {
>>> [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
>>> [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
>>> [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
>>> };
>>>
>>> - u8 val[] = {
>>> + static const u8 val[] = {
>>> [LP55XX_ENGINE_1] = LP5523_LOAD_ENG1,
>>> [LP55XX_ENGINE_2] = LP5523_LOAD_ENG2,
>>> [LP55XX_ENGINE_3] = LP5523_LOAD_ENG3,
>>> @@ -188,7 +188,7 @@ static void lp5523_load_engine(struct lp55xx_chip *chip)
>>> static void lp5523_load_engine_and_select_page(struct lp55xx_chip *chip)
>>> {
>>> enum lp55xx_engine_index idx = chip->engine_idx;
>>> - u8 page_sel[] = {
>>> + static const u8 page_sel[] = {
>>> [LP55XX_ENGINE_1] = LP5523_PAGE_ENG1,
>>> [LP55XX_ENGINE_2] = LP5523_PAGE_ENG2,
>>> [LP55XX_ENGINE_3] = LP5523_PAGE_ENG3,
>>> @@ -208,7 +208,7 @@ static void lp5523_stop_all_engines(struct lp55xx_chip *chip)
>>> static void lp5523_stop_engine(struct lp55xx_chip *chip)
>>> {
>>> enum lp55xx_engine_index idx = chip->engine_idx;
>>> - u8 mask[] = {
>>> + static const u8 mask[] = {
>>> [LP55XX_ENGINE_1] = LP5523_MODE_ENG1_M,
>>> [LP55XX_ENGINE_2] = LP5523_MODE_ENG2_M,
>>> [LP55XX_ENGINE_3] = LP5523_MODE_ENG3_M,
>>> @@ -505,7 +505,7 @@ static int lp5523_load_mux(struct lp55xx_chip *chip, u16 mux, int nr)
>>> {
>>> struct lp55xx_engine *engine = &chip->engines[nr - 1];
>>> int ret;
>>> - u8 mux_page[] = {
>>> + static const u8 mux_page[] = {
>>> [LP55XX_ENGINE_1] = LP5523_PAGE_MUX1,
>>> [LP55XX_ENGINE_2] = LP5523_PAGE_MUX2,
>>> [LP55XX_ENGINE_3] = LP5523_PAGE_MUX3,
>>>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-30 21:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-29 17:57 [PATCH] leds: lp55xx: make various arrays static const Colin King
2017-06-29 19:40 ` Jacek Anaszewski
2017-06-29 20:48 ` Pavel Machek
2017-06-30 21:26 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).