* [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller
@ 2020-10-20 14:46 Colin King
2020-10-20 15:29 ` David Laight
2020-10-20 16:23 ` Dafna Hirschfeld
0 siblings, 2 replies; 5+ messages in thread
From: Colin King @ 2020-10-20 14:46 UTC (permalink / raw)
To: Helen Koike, Dafna Hirschfeld, Mauro Carvalho Chehab,
Greg Kroah-Hartman, linux-media, devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Don't populate the const array dev_names on the stack but instead it
static. Makes the object code smaller by 15 bytes.
Before:
text data bss dec hex filename
17091 2648 64 19803 4d5b media/rkisp1/rkisp1-resizer.o
After:
text data bss dec hex filename
17012 2712 64 19788 4d4c media/rkisp1/rkisp1-resizer.o
(gcc version 10.2.0)
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/staging/media/rkisp1/rkisp1-resizer.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 1687d82e6c68..7ca5b47c5bf5 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -763,8 +763,10 @@ static void rkisp1_rsz_unregister(struct rkisp1_resizer *rsz)
static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
{
- const char * const dev_names[] = {RKISP1_RSZ_MP_DEV_NAME,
- RKISP1_RSZ_SP_DEV_NAME};
+ static const char * const dev_names[] = {
+ RKISP1_RSZ_MP_DEV_NAME,
+ RKISP1_RSZ_SP_DEV_NAME
+ };
struct media_pad *pads = rsz->pads;
struct v4l2_subdev *sd = &rsz->sd;
int ret;
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller
2020-10-20 14:46 [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller Colin King
@ 2020-10-20 15:29 ` David Laight
2020-10-20 17:10 ` Dafna Hirschfeld
2020-10-20 16:23 ` Dafna Hirschfeld
1 sibling, 1 reply; 5+ messages in thread
From: David Laight @ 2020-10-20 15:29 UTC (permalink / raw)
To: 'Colin King',
Helen Koike, Dafna Hirschfeld, Mauro Carvalho Chehab,
Greg Kroah-Hartman, linux-media, devel
Cc: kernel-janitors, linux-kernel
From: Colin King
> Sent: 20 October 2020 15:47
>
> From: Colin Ian King <colin.king@canonical.com>
>
> Don't populate the const array dev_names on the stack but instead it
> static. Makes the object code smaller by 15 bytes.
>
> Before:
> text data bss dec hex filename
> 17091 2648 64 19803 4d5b media/rkisp1/rkisp1-resizer.o
>
> After:
> text data bss dec hex filename
> 17012 2712 64 19788 4d4c media/rkisp1/rkisp1-resizer.o
>
> (gcc version 10.2.0)
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/staging/media/rkisp1/rkisp1-resizer.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-
> resizer.c
> index 1687d82e6c68..7ca5b47c5bf5 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -763,8 +763,10 @@ static void rkisp1_rsz_unregister(struct rkisp1_resizer *rsz)
>
> static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
> {
> - const char * const dev_names[] = {RKISP1_RSZ_MP_DEV_NAME,
> - RKISP1_RSZ_SP_DEV_NAME};
> + static const char * const dev_names[] = {
> + RKISP1_RSZ_MP_DEV_NAME,
> + RKISP1_RSZ_SP_DEV_NAME
> + };
> struct media_pad *pads = rsz->pads;
> struct v4l2_subdev *sd = &rsz->sd;
> int ret;
Don't look at what that code is actually doing....
It is rather horrid.
rkisp1_rsz_register() is called for each entry in an array (twice).
The array index is written into rsz->id.
The value is then used to select one of the two strings.
But rsz->id is actually an enum type.
rkisp1_rsz_register() should probably just be called twice with some
extra parameters.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller
2020-10-20 15:29 ` David Laight
@ 2020-10-20 17:10 ` Dafna Hirschfeld
2020-10-20 21:53 ` David Laight
0 siblings, 1 reply; 5+ messages in thread
From: Dafna Hirschfeld @ 2020-10-20 17:10 UTC (permalink / raw)
To: David Laight, 'Colin King',
Helen Koike, Mauro Carvalho Chehab, Greg Kroah-Hartman,
linux-media, devel
Cc: kernel-janitors, linux-kernel
Am 20.10.20 um 17:29 schrieb David Laight:
> From: Colin King
>> Sent: 20 October 2020 15:47
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Don't populate the const array dev_names on the stack but instead it
>> static. Makes the object code smaller by 15 bytes.
>>
>> Before:
>> text data bss dec hex filename
>> 17091 2648 64 19803 4d5b media/rkisp1/rkisp1-resizer.o
>>
>> After:
>> text data bss dec hex filename
>> 17012 2712 64 19788 4d4c media/rkisp1/rkisp1-resizer.o
>>
>> (gcc version 10.2.0)
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/staging/media/rkisp1/rkisp1-resizer.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-
>> resizer.c
>> index 1687d82e6c68..7ca5b47c5bf5 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -763,8 +763,10 @@ static void rkisp1_rsz_unregister(struct rkisp1_resizer *rsz)
>>
>> static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
>> {
>> - const char * const dev_names[] = {RKISP1_RSZ_MP_DEV_NAME,
>> - RKISP1_RSZ_SP_DEV_NAME};
>> + static const char * const dev_names[] = {
>> + RKISP1_RSZ_MP_DEV_NAME,
>> + RKISP1_RSZ_SP_DEV_NAME
>> + };
>> struct media_pad *pads = rsz->pads;
>> struct v4l2_subdev *sd = &rsz->sd;
>> int ret;
>
> Don't look at what that code is actually doing....
> It is rather horrid.
> rkisp1_rsz_register() is called for each entry in an array (twice).
> The array index is written into rsz->id.
> The value is then used to select one of the two strings.
> But rsz->id is actually an enum type.
Hi,
Is it that bad to use enum as an array index?
we use it in many places in the driver.
Thanks,
Dafna
>
> rkisp1_rsz_register() should probably just be called twice with some
> extra parameters.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller
2020-10-20 17:10 ` Dafna Hirschfeld
@ 2020-10-20 21:53 ` David Laight
0 siblings, 0 replies; 5+ messages in thread
From: David Laight @ 2020-10-20 21:53 UTC (permalink / raw)
To: 'Dafna Hirschfeld', 'Colin King',
Helen Koike, Mauro Carvalho Chehab, Greg Kroah-Hartman,
linux-media, devel
Cc: kernel-janitors, linux-kernel
From: Dafna Hirschfeld
> Sent: 20 October 2020 18:10
>
> Am 20.10.20 um 17:29 schrieb David Laight:
> > From: Colin King
> >> Sent: 20 October 2020 15:47
> >>
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
...
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-
> >> resizer.c
> >> index 1687d82e6c68..7ca5b47c5bf5 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> @@ -763,8 +763,10 @@ static void rkisp1_rsz_unregister(struct rkisp1_resizer *rsz)
> >>
> >> static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
> >> {
> >> - const char * const dev_names[] = {RKISP1_RSZ_MP_DEV_NAME,
> >> - RKISP1_RSZ_SP_DEV_NAME};
> >> + static const char * const dev_names[] = {
> >> + RKISP1_RSZ_MP_DEV_NAME,
> >> + RKISP1_RSZ_SP_DEV_NAME
> >> + };
> >> struct media_pad *pads = rsz->pads;
> >> struct v4l2_subdev *sd = &rsz->sd;
> >> int ret;
> >
> > Don't look at what that code is actually doing....
> > It is rather horrid.
> > rkisp1_rsz_register() is called for each entry in an array (twice).
> > The array index is written into rsz->id.
> > The value is then used to select one of the two strings.
> > But rsz->id is actually an enum type.
>
> Hi,
> Is it that bad to use enum as an array index?
> we use it in many places in the driver.
You'd normally use a constant from the enum to size the array definition.
In this case you've an enum, an array [2] and the dev_names[]
all of which have to match 'by magic'.
There is a loop that half implies you might add a third type.
but then the following code only works for the two types.
You've complex error recovery in case one of the calls to
rkisp1_rsz_register() fails - but since there can only ever
be two calls the code could be much simpler.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller
2020-10-20 14:46 [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller Colin King
2020-10-20 15:29 ` David Laight
@ 2020-10-20 16:23 ` Dafna Hirschfeld
1 sibling, 0 replies; 5+ messages in thread
From: Dafna Hirschfeld @ 2020-10-20 16:23 UTC (permalink / raw)
To: Colin King, Helen Koike, Mauro Carvalho Chehab,
Greg Kroah-Hartman, linux-media, devel
Cc: kernel-janitors, linux-kernel
Hi, thanks,
Am 20.10.20 um 16:46 schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
>
> Don't populate the const array dev_names on the stack but instead it
> static. Makes the object code smaller by 15 bytes.
>
> Before:
> text data bss dec hex filename
> 17091 2648 64 19803 4d5b media/rkisp1/rkisp1-resizer.o
>
> After:
> text data bss dec hex filename
> 17012 2712 64 19788 4d4c media/rkisp1/rkisp1-resizer.o
On the other hand the data segment is 64 bytes bigger.
I don't know what is more important to save.
Anyway, the rkisp1-capture.c does the same with the names
so it is better to be consistent.
Thanks,
Dafna
>
> (gcc version 10.2.0)
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/staging/media/rkisp1/rkisp1-resizer.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 1687d82e6c68..7ca5b47c5bf5 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -763,8 +763,10 @@ static void rkisp1_rsz_unregister(struct rkisp1_resizer *rsz)
>
> static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
> {
> - const char * const dev_names[] = {RKISP1_RSZ_MP_DEV_NAME,
> - RKISP1_RSZ_SP_DEV_NAME};
> + static const char * const dev_names[] = {
> + RKISP1_RSZ_MP_DEV_NAME,
> + RKISP1_RSZ_SP_DEV_NAME
> + };
> struct media_pad *pads = rsz->pads;
> struct v4l2_subdev *sd = &rsz->sd;
> int ret;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-20 21:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20 14:46 [PATCH] media: staging: rkisp1: rsz: make const array static, makes object smaller Colin King
2020-10-20 15:29 ` David Laight
2020-10-20 17:10 ` Dafna Hirschfeld
2020-10-20 21:53 ` David Laight
2020-10-20 16:23 ` Dafna Hirschfeld
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).