Re: [PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
Hi! On Mon, 2023-07-10 at 16:04 +0200, Thomas Zimmermann wrote: > > > I won't argue with that, but the flag itself is wrong. > > > FBINFO_FLAG_DEFAULT is/was for struct fb_info.flags. You have struct > > > fb_videomode.flag. The valid flags for this field are at [1]. If > > > anything, the field could be initialized to FB_MODE_IS_UNKNOWN, which > > > has the same value. > > > > > > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L681 > > > > FB_MODE_IS_UNKNOWN sounds very reasonable to me. Would you agree using that > > instead? > > Sure, I'll update the patch accordingly. Thanks! I'll ack the updated patch. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
Hi Am 10.07.23 um 15:59 schrieb John Paul Adrian Glaubitz: Hi Thomas! On Mon, 2023-07-10 at 15:52 +0200, Thomas Zimmermann wrote: I would argue that the current code is more readable that your proposed change. I agree that it's a no-op, but code is not just about functionality but also readability, isn't it? I won't argue with that, but the flag itself is wrong. FBINFO_FLAG_DEFAULT is/was for struct fb_info.flags. You have struct fb_videomode.flag. The valid flags for this field are at [1]. If anything, the field could be initialized to FB_MODE_IS_UNKNOWN, which has the same value. [1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L681 FB_MODE_IS_UNKNOWN sounds very reasonable to me. Would you agree using that instead? Sure, I'll update the patch accordingly. Best regards Thomas Also, I prefer "sh:" as the architecture prefix, not "arch/sh:". Ok. Thanks. Adrian -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
Hi Thomas! On Mon, 2023-07-10 at 15:52 +0200, Thomas Zimmermann wrote: > > I would argue that the current code is more readable that your proposed > > change. > > > > I agree that it's a no-op, but code is not just about functionality but also > > readability, isn't it? > > I won't argue with that, but the flag itself is wrong. > FBINFO_FLAG_DEFAULT is/was for struct fb_info.flags. You have struct > fb_videomode.flag. The valid flags for this field are at [1]. If > anything, the field could be initialized to FB_MODE_IS_UNKNOWN, which > has the same value. > > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L681 FB_MODE_IS_UNKNOWN sounds very reasonable to me. Would you agree using that instead? > > > > Also, I prefer "sh:" as the architecture prefix, not "arch/sh:". > > Ok. Thanks. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
Hi Am 10.07.23 um 15:42 schrieb John Paul Adrian Glaubitz: Hi Thomas! On Mon, 2023-07-10 at 14:50 +0200, Thomas Zimmermann wrote: FBINFO_FLAG_DEFAULT is a flag for a framebuffer in struct fb_info. Flags for videomodes are prefixed with FB_MODE_. FBINFO_FLAG_DEFAULT is 0 and the static declaration already clears the memory area of sh7763fb_videomode. So remove the assignment. Signed-off-by: Thomas Zimmermann Cc: Yoshinori Sato Cc: Rich Felker Cc: John Paul Adrian Glaubitz --- arch/sh/boards/mach-sh7763rdp/setup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/sh/boards/mach-sh7763rdp/setup.c b/arch/sh/boards/mach-sh7763rdp/setup.c index 97e715e4e9b3..345f2b76c85a 100644 --- a/arch/sh/boards/mach-sh7763rdp/setup.c +++ b/arch/sh/boards/mach-sh7763rdp/setup.c @@ -119,7 +119,6 @@ static struct fb_videomode sh7763fb_videomode = { .vsync_len = 1, .sync = 0, .vmode = FB_VMODE_NONINTERLACED, - .flag = FBINFO_FLAG_DEFAULT, }; static struct sh7760fb_platdata sh7763fb_def_pdata = { I would argue that the current code is more readable that your proposed change. I agree that it's a no-op, but code is not just about functionality but also readability, isn't it? I won't argue with that, but the flag itself is wrong. FBINFO_FLAG_DEFAULT is/was for struct fb_info.flags. You have struct fb_videomode.flag. The valid flags for this field are at [1]. If anything, the field could be initialized to FB_MODE_IS_UNKNOWN, which has the same value. [1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L681 Also, I prefer "sh:" as the architecture prefix, not "arch/sh:". Ok. Best regards Thomas Thanks, Adrian -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
Hi Thomas! On Mon, 2023-07-10 at 14:50 +0200, Thomas Zimmermann wrote: > FBINFO_FLAG_DEFAULT is a flag for a framebuffer in struct fb_info. > Flags for videomodes are prefixed with FB_MODE_. FBINFO_FLAG_DEFAULT > is 0 and the static declaration already clears the memory area of > sh7763fb_videomode. So remove the assignment. > > Signed-off-by: Thomas Zimmermann > Cc: Yoshinori Sato > Cc: Rich Felker > Cc: John Paul Adrian Glaubitz > --- > arch/sh/boards/mach-sh7763rdp/setup.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/sh/boards/mach-sh7763rdp/setup.c > b/arch/sh/boards/mach-sh7763rdp/setup.c > index 97e715e4e9b3..345f2b76c85a 100644 > --- a/arch/sh/boards/mach-sh7763rdp/setup.c > +++ b/arch/sh/boards/mach-sh7763rdp/setup.c > @@ -119,7 +119,6 @@ static struct fb_videomode sh7763fb_videomode = { > .vsync_len = 1, > .sync = 0, > .vmode = FB_VMODE_NONINTERLACED, > - .flag = FBINFO_FLAG_DEFAULT, > }; > > static struct sh7760fb_platdata sh7763fb_def_pdata = { I would argue that the current code is more readable that your proposed change. I agree that it's a no-op, but code is not just about functionality but also readability, isn't it? Also, I prefer "sh:" as the architecture prefix, not "arch/sh:". Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
[PATCH 08/17] arch/sh: Do not assign FBINFO_FLAG_DEFAULT to fb_videomode.flag
FBINFO_FLAG_DEFAULT is a flag for a framebuffer in struct fb_info. Flags for videomodes are prefixed with FB_MODE_. FBINFO_FLAG_DEFAULT is 0 and the static declaration already clears the memory area of sh7763fb_videomode. So remove the assignment. Signed-off-by: Thomas Zimmermann Cc: Yoshinori Sato Cc: Rich Felker Cc: John Paul Adrian Glaubitz --- arch/sh/boards/mach-sh7763rdp/setup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/sh/boards/mach-sh7763rdp/setup.c b/arch/sh/boards/mach-sh7763rdp/setup.c index 97e715e4e9b3..345f2b76c85a 100644 --- a/arch/sh/boards/mach-sh7763rdp/setup.c +++ b/arch/sh/boards/mach-sh7763rdp/setup.c @@ -119,7 +119,6 @@ static struct fb_videomode sh7763fb_videomode = { .vsync_len = 1, .sync = 0, .vmode = FB_VMODE_NONINTERLACED, - .flag = FBINFO_FLAG_DEFAULT, }; static struct sh7760fb_platdata sh7763fb_def_pdata = { -- 2.41.0