From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753132AbdF3V1i (ORCPT ); Fri, 30 Jun 2017 17:27:38 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:35351 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753022AbdF3V1g (ORCPT ); Fri, 30 Jun 2017 17:27:36 -0400 Subject: Re: [PATCH] leds: lp55xx: make various arrays static const To: Pavel Machek References: <20170629175738.12467-1-colin.king@canonical.com> <20170629204831.GA763@amd> Cc: Colin King , Richard Purdie , linux-leds@vger.kernel.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org From: Jacek Anaszewski X-Enigmail-Draft-Status: N1110 Message-ID: <1d5d4c21-32db-447e-1715-54a7eb959ad0@gmail.com> Date: Fri, 30 Jun 2017 23:26:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170629204831.GA763@amd> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>> +++ 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