Re: [PATCH] leds: lp55xx: make various arrays static const

[Jacek Anaszewski]

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@xxxxxx>
> 
>>> +++ 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