| Message ID | 20220707153952.32264-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | fbdev: Maintain device ownership with aperture helpers | expand |
On 7/7/22 17:39, Thomas Zimmermann wrote: > Move vgag16fb's option parsing into the driver's probe function and > generate the rest of the module's init/exit functions from macros. > Keep the options code, although there are no options defined. > Ah, I see now why you wanted to move the check to the probe function. If is to allow this cleanup then discard that comment from previous patch and I'm OK with the move. Maybe you could comment in patch 02/11 commit message that the check is moved to the probe handler to allow this cleanup as a follow-up patch ? > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- > 1 file changed, 10 insertions(+), 25 deletions(-) > > diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c > index f7c1bb018843..e7767ed50c5b 100644 > --- a/drivers/video/fbdev/vga16fb.c > +++ b/drivers/video/fbdev/vga16fb.c > @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) > > static int vga16fb_probe(struct platform_device *dev) > { > +#ifndef MODULE > + char *option = NULL; > +#endif > struct screen_info *si; > struct fb_info *info; > struct vga16fb_par *par; > int i; > int ret = 0; > > +#ifndef MODULE > + if (fb_get_options("vga16fb", &option)) > + return -ENODEV; > + vga16fb_setup(option); > +#endif > + I would just drop these ifdefery and have the option unconditionally. It seems that's what most fbdev drivers do AFAICT. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: > On 7/7/22 17:39, Thomas Zimmermann wrote: >> Move vgag16fb's option parsing into the driver's probe function and >> generate the rest of the module's init/exit functions from macros. >> Keep the options code, although there are no options defined. >> > > Ah, I see now why you wanted to move the check to the probe function. If > is to allow this cleanup then discard that comment from previous patch > and I'm OK with the move. > > Maybe you could comment in patch 02/11 commit message that the check is > moved to the probe handler to allow this cleanup as a follow-up patch ? Sure. I mostly wanted to use module_platform_driver(). The options handling is in the way. > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- >> 1 file changed, 10 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >> index f7c1bb018843..e7767ed50c5b 100644 >> --- a/drivers/video/fbdev/vga16fb.c >> +++ b/drivers/video/fbdev/vga16fb.c >> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) >> >> static int vga16fb_probe(struct platform_device *dev) >> { >> +#ifndef MODULE >> + char *option = NULL; >> +#endif >> struct screen_info *si; >> struct fb_info *info; >> struct vga16fb_par *par; >> int i; >> int ret = 0; >> >> +#ifndef MODULE >> + if (fb_get_options("vga16fb", &option)) >> + return -ENODEV; >> + vga16fb_setup(option); >> +#endif >> + > > I would just drop these ifdefery and have the option unconditionally. > It seems that's what most fbdev drivers do AFAICT. Or can we kill it entirely? There are no actual options. Best regards Thomas > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >
On 7/11/22 10:01, Thomas Zimmermann wrote: > Hi > > Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: >> On 7/7/22 17:39, Thomas Zimmermann wrote: >>> Move vgag16fb's option parsing into the driver's probe function and >>> generate the rest of the module's init/exit functions from macros. >>> Keep the options code, although there are no options defined. >>> >> >> Ah, I see now why you wanted to move the check to the probe function. If >> is to allow this cleanup then discard that comment from previous patch >> and I'm OK with the move. >> >> Maybe you could comment in patch 02/11 commit message that the check is >> moved to the probe handler to allow this cleanup as a follow-up patch ? > > Sure. > > I mostly wanted to use module_platform_driver(). The options handling is > in the way. > Yes, I got it when looked at this patch. >> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- >>> 1 file changed, 10 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >>> index f7c1bb018843..e7767ed50c5b 100644 >>> --- a/drivers/video/fbdev/vga16fb.c >>> +++ b/drivers/video/fbdev/vga16fb.c >>> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) >>> >>> static int vga16fb_probe(struct platform_device *dev) >>> { >>> +#ifndef MODULE >>> + char *option = NULL; >>> +#endif >>> struct screen_info *si; >>> struct fb_info *info; >>> struct vga16fb_par *par; >>> int i; >>> int ret = 0; >>> >>> +#ifndef MODULE >>> + if (fb_get_options("vga16fb", &option)) >>> + return -ENODEV; >>> + vga16fb_setup(option); >>> +#endif >>> + >> >> I would just drop these ifdefery and have the option unconditionally. >> It seems that's what most fbdev drivers do AFAICT. > > Or can we kill it entirely? There are no actual options. > That sounds good to me as well.
diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c index f7c1bb018843..e7767ed50c5b 100644 --- a/drivers/video/fbdev/vga16fb.c +++ b/drivers/video/fbdev/vga16fb.c @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) static int vga16fb_probe(struct platform_device *dev) { +#ifndef MODULE + char *option = NULL; +#endif struct screen_info *si; struct fb_info *info; struct vga16fb_par *par; int i; int ret = 0; +#ifndef MODULE + if (fb_get_options("vga16fb", &option)) + return -ENODEV; + vga16fb_setup(option); +#endif + si = dev_get_platdata(&dev->dev); if (!si) return -ENODEV; @@ -1449,31 +1458,7 @@ static struct platform_driver vga16fb_driver = { .id_table = vga16fb_driver_id_table, }; -static int __init vga16fb_init(void) -{ - int ret; -#ifndef MODULE - char *option = NULL; - - if (fb_get_options("vga16fb", &option)) - return -ENODEV; - - vga16fb_setup(option); -#endif - - ret = platform_driver_register(&vga16fb_driver); - if (ret) - return ret; - - return 0; -} - -static void __exit vga16fb_exit(void) -{ - platform_driver_unregister(&vga16fb_driver); -} +module_platform_driver(vga16fb_driver); MODULE_DESCRIPTION("Legacy VGA framebuffer device driver"); MODULE_LICENSE("GPL"); -module_init(vga16fb_init); -module_exit(vga16fb_exit);
Move vgag16fb's option parsing into the driver's probe function and generate the rest of the module's init/exit functions from macros. Keep the options code, although there are no options defined. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/fbdev/vga16fb.c | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-)