* [PATCH][next] iio: adc: make read-only const array config static @ 2026-07-14 16:50 Colin Ian King 2026-07-14 17:08 ` Andy Shevchenko 2026-07-14 17:40 ` David Lechner 0 siblings, 2 replies; 8+ messages in thread From: Colin Ian King @ 2026-07-14 16:50 UTC (permalink / raw) To: Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio Cc: kernel-janitors, linux-kernel Don't populate the read-only const array config on the stack at run time, instead make it static. Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/iio/adc/88pm886-gpadc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iio/adc/88pm886-gpadc.c b/drivers/iio/adc/88pm886-gpadc.c index ff9bc5f06c18..938ba43c2c35 100644 --- a/drivers/iio/adc/88pm886-gpadc.c +++ b/drivers/iio/adc/88pm886-gpadc.c @@ -279,7 +279,7 @@ static int pm886_gpadc_read_raw(struct iio_dev *iio, struct iio_chan_spec const static int pm886_gpadc_hw_enable(struct regmap *map) { - const u8 config[] = { + static const u8 config[] = { PM886_GPADC_CONFIG1_EN_ALL, PM886_GPADC_CONFIG2_EN_ALL, PM886_GPADC_GND_DET2_EN, -- 2.53.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-14 16:50 [PATCH][next] iio: adc: make read-only const array config static Colin Ian King @ 2026-07-14 17:08 ` Andy Shevchenko 2026-07-15 10:07 ` David Laight 2026-07-15 10:22 ` Dan Carpenter 2026-07-14 17:40 ` David Lechner 1 sibling, 2 replies; 8+ messages in thread From: Andy Shevchenko @ 2026-07-14 17:08 UTC (permalink / raw) To: Colin Ian King Cc: Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: > Don't populate the read-only const array config on the stack at run > time, instead make it static. Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> ... In all patches like this it's always a bikeshedding possible of moving static data outside of a function. I have no strong opinion in these cases (when the data solely used by a single function), but in general it might give different readability experience (it's harder to notice static data in the local function definition block). So I leave this exercise to the maintainers of the respective pieces of the code. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-14 17:08 ` Andy Shevchenko @ 2026-07-15 10:07 ` David Laight 2026-07-15 10:22 ` Dan Carpenter 1 sibling, 0 replies; 8+ messages in thread From: David Laight @ 2026-07-15 10:07 UTC (permalink / raw) To: Andy Shevchenko Cc: Colin Ian King, Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel On Tue, 14 Jul 2026 20:08:10 +0300 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: > > Don't populate the read-only const array config on the stack at run > > time, instead make it static. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > > ... > > In all patches like this it's always a bikeshedding possible of moving static > data outside of a function. I have no strong opinion in these cases (when the > data solely used by a single function), but in general it might give different > readability experience (it's harder to notice static data in the local function > definition block). So I leave this exercise to the maintainers of the respective > pieces of the code. > If global you start wondering where it is used.... There is also the fact that run-time initialisation of a 3 byte array on stack is very likely to generate faster code than using static data. Mostly because the access to the static data is likely to be a cache miss. Then there are the architectures where just generating the address of the static data takes multiple instructions. In that case the on-stack version may even be smaller. David ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-14 17:08 ` Andy Shevchenko 2026-07-15 10:07 ` David Laight @ 2026-07-15 10:22 ` Dan Carpenter 2026-07-15 13:32 ` Andy Shevchenko 1 sibling, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2026-07-15 10:22 UTC (permalink / raw) To: Andy Shevchenko Cc: Colin Ian King, Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel On Tue, Jul 14, 2026 at 08:08:10PM +0300, Andy Shevchenko wrote: > On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: > > Don't populate the read-only const array config on the stack at run > > time, instead make it static. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > > ... > > In all patches like this it's always a bikeshedding possible of moving static > data outside of a function. I have no strong opinion in these cases (when the > data solely used by a single function), but in general it might give different > readability experience (it's harder to notice static data in the local function > definition block). So I leave this exercise to the maintainers of the respective > pieces of the code. > It's an interesting point... At one point Smatch didn't track static variables and it used to generate occasional false positives. And it's like you say, those little "static" qualifiers are hard to spot in a wall of declaration text. I've never considered moving the declarations out of the function scope but it might be a good idea? But in this case since this data is const, the static vs not-static doesn't affect flow analysis or readability. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-15 10:22 ` Dan Carpenter @ 2026-07-15 13:32 ` Andy Shevchenko 2026-07-15 13:34 ` Colin King (gmail) 0 siblings, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2026-07-15 13:32 UTC (permalink / raw) To: Dan Carpenter Cc: Colin Ian King, Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel On Wed, Jul 15, 2026 at 01:22:32PM +0300, Dan Carpenter wrote: > On Tue, Jul 14, 2026 at 08:08:10PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: ... > > In all patches like this it's always a bikeshedding possible of moving static > > data outside of a function. I have no strong opinion in these cases (when the > > data solely used by a single function), but in general it might give different > > readability experience (it's harder to notice static data in the local function > > definition block). So I leave this exercise to the maintainers of the respective > > pieces of the code. > > > > It's an interesting point... > > At one point Smatch didn't track static variables and it used to > generate occasional false positives. And it's like you say, those > little "static" qualifiers are hard to spot in a wall of declaration > text. I've never considered moving the declarations out of the > function scope but it might be a good idea? Maybe, as I said, I have no strong opinion here. I am all ears to hear what others think. > But in this case since this data is const, the static vs not-static > doesn't affect flow analysis or readability. It doesn't affect flow analysis, but we also have David's point about amount of data to be static may affect the code generation. ... The main point I have is that stumbling over the 'static' in the definition block might rise some additional questions and slow down the understanding of the code (by reading). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-15 13:32 ` Andy Shevchenko @ 2026-07-15 13:34 ` Colin King (gmail) 2026-07-15 13:54 ` Andy Shevchenko 0 siblings, 1 reply; 8+ messages in thread From: Colin King (gmail) @ 2026-07-15 13:34 UTC (permalink / raw) To: Andy Shevchenko, Dan Carpenter Cc: Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel [-- Attachment #1.1.1: Type: text/plain, Size: 1800 bytes --] On 15/07/2026 14:32, Andy Shevchenko wrote: > On Wed, Jul 15, 2026 at 01:22:32PM +0300, Dan Carpenter wrote: >> On Tue, Jul 14, 2026 at 08:08:10PM +0300, Andy Shevchenko wrote: >>> On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: > > ... > >>> In all patches like this it's always a bikeshedding possible of moving static >>> data outside of a function. I have no strong opinion in these cases (when the >>> data solely used by a single function), but in general it might give different >>> readability experience (it's harder to notice static data in the local function >>> definition block). So I leave this exercise to the maintainers of the respective >>> pieces of the code. >>> >> >> It's an interesting point... >> >> At one point Smatch didn't track static variables and it used to >> generate occasional false positives. And it's like you say, those >> little "static" qualifiers are hard to spot in a wall of declaration >> text. I've never considered moving the declarations out of the >> function scope but it might be a good idea? > > Maybe, as I said, I have no strong opinion here. I am all ears to hear > what others think. For small arrays it's a moot point if this type of change is useful, so I'm OK if it's not accepted. I appreciate the feedback. Colin > >> But in this case since this data is const, the static vs not-static >> doesn't affect flow analysis or readability. > > It doesn't affect flow analysis, but we also have David's point about > amount of data to be static may affect the code generation. > > ... > > The main point I have is that stumbling over the 'static' in the definition > block might rise some additional questions and slow down the understanding of > the code (by reading). > [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 4901 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-15 13:34 ` Colin King (gmail) @ 2026-07-15 13:54 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2026-07-15 13:54 UTC (permalink / raw) To: Colin King (gmail) Cc: Dan Carpenter, Duje Mihanović, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-iio, kernel-janitors, linux-kernel On Wed, Jul 15, 2026 at 02:34:50PM +0100, Colin King (gmail) wrote: > On 15/07/2026 14:32, Andy Shevchenko wrote: > > On Wed, Jul 15, 2026 at 01:22:32PM +0300, Dan Carpenter wrote: > > > On Tue, Jul 14, 2026 at 08:08:10PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 14, 2026 at 05:50:12PM +0100, Colin Ian King wrote: ... > > > > In all patches like this it's always a bikeshedding possible of moving static > > > > data outside of a function. I have no strong opinion in these cases (when the > > > > data solely used by a single function), but in general it might give different > > > > readability experience (it's harder to notice static data in the local function > > > > definition block). So I leave this exercise to the maintainers of the respective > > > > pieces of the code. > > > > > > It's an interesting point... > > > > > > At one point Smatch didn't track static variables and it used to > > > generate occasional false positives. And it's like you say, those > > > little "static" qualifiers are hard to spot in a wall of declaration > > > text. I've never considered moving the declarations out of the > > > function scope but it might be a good idea? > > > > Maybe, as I said, I have no strong opinion here. I am all ears to hear > > what others think. > > For small arrays it's a moot point if this type of change is useful, so Yep, it actually becomes a trade-off between generated code and existing data. With a small difference that static const may be located in ROM (important for the embedded devices with minimum available memory). > I'm OK if it's not accepted. I appreciate the feedback. Me too. > > > But in this case since this data is const, the static vs not-static > > > doesn't affect flow analysis or readability. > > > > It doesn't affect flow analysis, but we also have David's point about > > amount of data to be static may affect the code generation. ... > > The main point I have is that stumbling over the 'static' in the definition > > block might rise some additional questions and slow down the understanding of > > the code (by reading). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] iio: adc: make read-only const array config static 2026-07-14 16:50 [PATCH][next] iio: adc: make read-only const array config static Colin Ian King 2026-07-14 17:08 ` Andy Shevchenko @ 2026-07-14 17:40 ` David Lechner 1 sibling, 0 replies; 8+ messages in thread From: David Lechner @ 2026-07-14 17:40 UTC (permalink / raw) To: Colin Ian King, Duje Mihanović, Jonathan Cameron, Nuno Sá, Andy Shevchenko, linux-iio Cc: kernel-janitors, linux-kernel On 7/14/26 11:50 AM, Colin Ian King wrote: > Don't populate the read-only const array config on the stack at run > time, instead make it static. > Subject line should have `88pm886-gpadc` in it. Can leave out `config` to keep it shorter. iio: adc: 88pm886-gpadc: make read-only const array static ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-07-15 13:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-14 16:50 [PATCH][next] iio: adc: make read-only const array config static Colin Ian King 2026-07-14 17:08 ` Andy Shevchenko 2026-07-15 10:07 ` David Laight 2026-07-15 10:22 ` Dan Carpenter 2026-07-15 13:32 ` Andy Shevchenko 2026-07-15 13:34 ` Colin King (gmail) 2026-07-15 13:54 ` Andy Shevchenko 2026-07-14 17:40 ` David Lechner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox