[PATCH 1/3] staging: sm7xxfb: use module init and exit

[PATCH 1/3] staging: sm7xxfb: use module init and exit

[Sudip Mukherjee]

use module_init() and module_exit() instead of module_pci_driver
and at the same time make way for use of kernel boot parameters.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/sm7xxfb/sm7xxfb.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index ebd9536..17b3917 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -113,6 +113,8 @@ static struct vesa_mode vesa_mode_table[] = {
 
 static struct screen_info smtc_scr_info;
 
+static char *mode_option;
+
 /* process command line options, get vga parameter */
 static int __init sm7xx_vga_setup(char *options)
 {
@@ -1017,7 +1019,29 @@ static struct pci_driver smtcfb_driver = {
 	.driver.pm  = SM7XX_PM_OPS,
 };
 
-module_pci_driver(smtcfb_driver);
+static int __init sm712fb_init(void)
+{
+#ifndef MODULE
+	char *option = NULL;
+
+	if (fb_get_options("sm712fb", &option))
+		return -ENODEV;
+	if (option && *option)
+		mode_option = option;
+#endif
+	sm7xx_vga_setup(mode_option);
+
+	return pci_register_driver(&smtcfb_driver);
+}
+
+module_init(sm712fb_init);
+
+static void __exit sm712fb_exit(void)
+{
+	pci_unregister_driver(&smtcfb_driver);
+}
+
+module_exit(sm712fb_exit);
 
 MODULE_AUTHOR("Siliconmotion ");
 MODULE_DESCRIPTION("Framebuffer driver for SMI Graphic Cards");
-- 
1.9.1

[PATCH 2/3] staging: sm7xxfb: remove unneeded __setup

[Sudip Mukherjee]

as we start to use kernel boot parameters and fb_get_options()
we donot need to use __setup() any more.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/sm7xxfb/sm7xxfb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 17b3917..e93563f 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -141,7 +141,6 @@ static int __init sm7xx_vga_setup(char *options)
 
 	return -1;
 }
-__setup("vga=", sm7xx_vga_setup);
 
 static void sm712_setpalette(int regno, unsigned red, unsigned green,
 			     unsigned blue, struct fb_info *info)
-- 
1.9.1

[PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Sudip Mukherjee]

change return type of sm7xx_vga_setup() to void as we are not using
the return type anywhere. if this function fails we fall back to the
default configuration.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/staging/sm7xxfb/sm7xxfb.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index e93563f..abdb021 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -116,12 +116,12 @@ static struct screen_info smtc_scr_info;
 static char *mode_option;
 
 /* process command line options, get vga parameter */
-static int __init sm7xx_vga_setup(char *options)
+static void __init sm7xx_vga_setup(char *options)
 {
 	int i;
 
 	if (!options || !*options)
-		return -EINVAL;
+		return;
 
 	smtc_scr_info.lfb_width = 0;
 	smtc_scr_info.lfb_height = 0;
@@ -135,11 +135,9 @@ static int __init sm7xx_vga_setup(char *options)
 			smtc_scr_info.lfb_height =
 						vesa_mode_table[i].lfb_height;
 			smtc_scr_info.lfb_depth  = vesa_mode_table[i].lfb_depth;
-			return 0;
+			return;
 		}
 	}
-
-	return -1;
 }
 
 static void sm712_setpalette(int regno, unsigned red, unsigned green,
-- 
1.9.1

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Dan Carpenter]

On Sun, Mar 08, 2015 at 11:10:11PM +0530, Sudip Mukherjee wrote:
> change return type of sm7xx_vga_setup() to void as we are not using
> the return type anywhere. if this function fails we fall back to the
> default configuration.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

This patch is fine but if you wanted you could send a follow on which
set it to SCREEN_X_RES, SCREEN_Y_RES, SCREEN_BPP if there were no
options.  Then remove the "if (smtc_scr_info.lfb_width != 0) " condition
in smtcfb_pci_probe().

regards,
dan carpenter

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Sudip Mukherjee]

On Mon, Mar 09, 2015 at 04:13:43PM +0300, Dan Carpenter wrote:
> On Sun, Mar 08, 2015 at 11:10:11PM +0530, Sudip Mukherjee wrote:
> > change return type of sm7xx_vga_setup() to void as we are not using
> > the return type anywhere. if this function fails we fall back to the
> > default configuration.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> This patch is fine but if you wanted you could send a follow on which
> set it to SCREEN_X_RES, SCREEN_Y_RES, SCREEN_BPP if there were no
> options.  Then remove the "if (smtc_scr_info.lfb_width != 0) " condition
> in smtcfb_pci_probe().

yes, i could have. But actually I am thinking to totally remove this function and use more conventional fb_get_mode() in its places. But if i do so then the boot parameters will change. Currently it is like vga=sm712fb:0x301 but if i change it to use fb_get_mode() then the boot parameters will change to vga=sm712fb:640x480@60-8 , and that is why I am hesitating. But it will be more conventional to what all framebuffer drivers follows.

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Dan Carpenter]

On Mon, Mar 09, 2015 at 07:00:33PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 04:13:43PM +0300, Dan Carpenter wrote:
> > On Sun, Mar 08, 2015 at 11:10:11PM +0530, Sudip Mukherjee wrote:
> > > change return type of sm7xx_vga_setup() to void as we are not using
> > > the return type anywhere. if this function fails we fall back to the
> > > default configuration.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > 
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > This patch is fine but if you wanted you could send a follow on which
> > set it to SCREEN_X_RES, SCREEN_Y_RES, SCREEN_BPP if there were no
> > options.  Then remove the "if (smtc_scr_info.lfb_width != 0) " condition
> > in smtcfb_pci_probe().
> 
> yes, i could have. But actually I am thinking to totally remove this function and use more conventional fb_get_mode() in its places. But if i do so then the boot parameters will change. Currently it is like vga=sm712fb:0x301 but if i change it to use fb_get_mode() then the boot parameters will change to vga=sm712fb:640x480@60-8 , and that is why I am hesitating. But it will be more conventional to what all framebuffer drivers follows.
> 

Hm...  That's a good question.

I suspect we should just go with fb_get_mode() and do it quickly.  This
driver is so new in the kernel that there are no users to get upset.

regards,
dan carpenter

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Sudip Mukherjee]

On Mon, Mar 09, 2015 at 04:38:05PM +0300, Dan Carpenter wrote:
> 
> 
> On Mon, Mar 09, 2015 at 07:00:33PM +0530, Sudip Mukherjee wrote:
> > On Mon, Mar 09, 2015 at 04:13:43PM +0300, Dan Carpenter wrote:
> > > On Sun, Mar 08, 2015 at 11:10:11PM +0530, Sudip Mukherjee wrote:
> > > > change return type of sm7xx_vga_setup() to void as we are not using
> > > > the return type anywhere. if this function fails we fall back to the
> > > > default configuration.
> > > > 
> > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > 
> > > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > This patch is fine but if you wanted you could send a follow on which
> > > set it to SCREEN_X_RES, SCREEN_Y_RES, SCREEN_BPP if there were no
> > > options.  Then remove the "if (smtc_scr_info.lfb_width != 0) " condition
> > > in smtcfb_pci_probe().
> > 
> > yes, i could have. But actually I am thinking to totally remove this function and use more conventional fb_get_mode() in its places. But if i do so then the boot parameters will change. Currently it is like vga=sm712fb:0x301 but if i change it to use fb_get_mode() then the boot parameters will change to vga=sm712fb:640x480@60-8 , and that is why I am hesitating. But it will be more conventional to what all framebuffer drivers follows.
> > 
> 
> Hm...  That's a good question.
> 
> I suspect we should just go with fb_get_mode() and do it quickly.  This
> driver is so new in the kernel that there are no users to get upset.
i am afraid that is not correct. this driver was in staging from long back, and Greg removed it from staging as no one was working with it. But after that removal Lemote Yeeloong laptop of Steven Rostedt stopped working. (https://lkml.org/lkml/2014/11/20/698) , so i guess there will be many such users.

regards
sudip

> 
> regards,
> dan carpenter
> 

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Dan Carpenter]

On Mon, Mar 09, 2015 at 07:23:43PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 04:38:05PM +0300, Dan Carpenter wrote:
> > 
> > 
> > On Mon, Mar 09, 2015 at 07:00:33PM +0530, Sudip Mukherjee wrote:
> > > On Mon, Mar 09, 2015 at 04:13:43PM +0300, Dan Carpenter wrote:
> > > > On Sun, Mar 08, 2015 at 11:10:11PM +0530, Sudip Mukherjee wrote:
> > > > > change return type of sm7xx_vga_setup() to void as we are not using
> > > > > the return type anywhere. if this function fails we fall back to the
> > > > > default configuration.
> > > > > 
> > > > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > > 
> > > > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > 
> > > > This patch is fine but if you wanted you could send a follow on which
> > > > set it to SCREEN_X_RES, SCREEN_Y_RES, SCREEN_BPP if there were no
> > > > options.  Then remove the "if (smtc_scr_info.lfb_width != 0) " condition
> > > > in smtcfb_pci_probe().
> > > 
> > > yes, i could have. But actually I am thinking to totally remove this function and use more conventional fb_get_mode() in its places. But if i do so then the boot parameters will change. Currently it is like vga=sm712fb:0x301 but if i change it to use fb_get_mode() then the boot parameters will change to vga=sm712fb:640x480@60-8 , and that is why I am hesitating. But it will be more conventional to what all framebuffer drivers follows.
> > > 
> > 
> > Hm...  That's a good question.
> > 
> > I suspect we should just go with fb_get_mode() and do it quickly.  This
> > driver is so new in the kernel that there are no users to get upset.
>
> i am afraid that is not correct. this driver was in staging from long
> back, and Greg removed it from staging as no one was working with it.
> But after that removal Lemote Yeeloong laptop of Steven Rostedt
> stopped working. (https://lkml.org/lkml/2014/11/20/698) , so i guess
> there will be many such users.

Hm...  I suspect we should still change it so it matches the other FB
drivers but I don't know.

Perhaps Greg has an opinion?

regards,
dan carpenter

Re: [PATCH 3/3] staging: sm7xxfb: change return of sm7xx_vga_setup

[Sudip Mukherjee]

On Mon, Mar 09, 2015 at 05:04:14PM +0300, Dan Carpenter wrote:
> On Mon, Mar 09, 2015 at 07:23:43PM +0530, Sudip Mukherjee wrote:
<snip>
> > > Hm...  That's a good question.
> > > 
> > > I suspect we should just go with fb_get_mode() and do it quickly.  This
> > > driver is so new in the kernel that there are no users to get upset.
> >
> > i am afraid that is not correct. this driver was in staging from long
> > back, and Greg removed it from staging as no one was working with it.
> > But after that removal Lemote Yeeloong laptop of Steven Rostedt
> > stopped working. (https://lkml.org/lkml/2014/11/20/698) , so i guess
> > there will be many such users.
> 
> Hm...  I suspect we should still change it so it matches the other FB
> drivers but I don't know.
> 
> Perhaps Greg has an opinion?
maybe, i should make the change and then we can request Steven Rostedt
to test it on his Lemote Yeeloong. if nothing breaks, then alright,
else revert the patch.

regards
sudip

> 
> regards,
> dan carpenter
>