doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

7 messages, 4 authors, 2015-06-20 · open the first message on its own page

doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Date: 2015-06-13 08:46:39 

On Fri, Jun 12, 2015 at 05:18:49PM -0700, Greg KH wrote:
On Tue, Jun 09, 2015 at 04:59:01PM -0500, J. German Rivera wrote:
quoted
This patch series includes new functionality for the Freescale fsl-mc
bus driver.
Why are people working on "new functionality" instead of working on
getting this out of the staging tree?  I really hate adding new
functions to staging drivers, as this is not the correct place for
drivers to be in the tree.  People should be working to get them out of
this location, and then you can add new functions to them.
Hi Greg,
Your reply above was for a different thread but my doubt started from
this. I was working on adding the Dual-Head support to sm7xxfb. So that
is also a new functionality which is not in the current driver as of now.
Then should i instead concentrate on getting that out of staging? If yes,
can you please have a look (when you are free) at it to see if anything
else needs to be done.

regards
sudip

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2015-06-13 16:28:47 

On Sat, Jun 13, 2015 at 02:16:18PM +0530, Sudip Mukherjee wrote:
On Fri, Jun 12, 2015 at 05:18:49PM -0700, Greg KH wrote:
quoted
On Tue, Jun 09, 2015 at 04:59:01PM -0500, J. German Rivera wrote:
quoted
This patch series includes new functionality for the Freescale fsl-mc
bus driver.
Why are people working on "new functionality" instead of working on
getting this out of the staging tree?  I really hate adding new
functions to staging drivers, as this is not the correct place for
drivers to be in the tree.  People should be working to get them out of
this location, and then you can add new functions to them.
Hi Greg,
Your reply above was for a different thread but my doubt started from
this. I was working on adding the Dual-Head support to sm7xxfb. So that
is also a new functionality which is not in the current driver as of now.
Then should i instead concentrate on getting that out of staging? If yes,
can you please have a look (when you are free) at it to see if anything
else needs to be done.
Yes, please work on getting it out of staging.

If you think it's ready, then submit a patch that moves it, copying the
proper maintainers, and I'll be glad to review it.

thanks,

greg k-h

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Joe Perches <joe@perches.com>
Date: 2015-06-13 16:57:18 

On Sat, 2015-06-13 at 09:28 -0700, Greg KH wrote:
On Sat, Jun 13, 2015 at 02:16:18PM +0530, Sudip Mukherjee wrote:
[]
quoted
Your reply above was for a different thread but my doubt started from
this. I was working on adding the Dual-Head support to sm7xxfb. So that
is also a new functionality which is not in the current driver as of now.
Then should i instead concentrate on getting that out of staging? If yes,
can you please have a look (when you are free) at it to see if anything
else needs to be done.
Yes, please work on getting it out of staging.

If you think it's ready, then submit a patch that moves it, copying the
proper maintainers, and I'll be glad to review it.
It seems ready to me.

As far as I can tell, there's just a few niggles
that could be done:

Something like (too lazy to separate them into multiple patches)

o reduce indentation a couple of places
o add newlines to logging messages
o add const to static array
o use consistent function declaration style

It's unfortunate there are so many #ifdef __BIG_ENDIAN uses.

---
 drivers/staging/sm7xxfb/sm7xxfb.c | 203 ++++++++++++++++++--------------------
 1 file changed, 96 insertions(+), 107 deletions(-)

diff --git a/drivers/staging/sm7xxfb/sm7xxfb.c b/drivers/staging/sm7xxfb/sm7xxfb.c
index 5db26f1..5be2560 100644
--- a/drivers/staging/sm7xxfb/sm7xxfb.c
+++ b/drivers/staging/sm7xxfb/sm7xxfb.c
@@ -94,7 +94,7 @@ struct vesa_mode {
 	u16  lfb_depth;
 };
 
-static struct vesa_mode vesa_mode_table[] = {
+static const struct vesa_mode vesa_mode_table[] = {
 	{"0x301", 640,  480,  8},
 	{"0x303", 800,  600,  8},
 	{"0x305", 1024, 768,  8},
@@ -255,37 +255,32 @@ static int smtc_setcolreg(unsigned regno, unsigned red, unsigned green,
 		/*
 		 * 16/32 bit true-colour, use pseudo-palette for 16 base color
 		 */
-		if (regno < 16) {
-			if (sfb->fb->var.bits_per_pixel = 16) {
-				u32 *pal = sfb->fb->pseudo_palette;
-
-				val = chan_to_field(red, &sfb->fb->var.red);
-				val |= chan_to_field(green,
-						     &sfb->fb->var.green);
-				val |= chan_to_field(blue, &sfb->fb->var.blue);
+		if (regno >= 16)
+			break;
+		if (sfb->fb->var.bits_per_pixel = 16) {
+			u32 *pal = sfb->fb->pseudo_palette;
+
+			val = chan_to_field(red, &sfb->fb->var.red);
+			val |= chan_to_field(green, &sfb->fb->var.green);
+			val |= chan_to_field(blue, &sfb->fb->var.blue);
 #ifdef __BIG_ENDIAN
-				pal[regno] -				    ((red & 0xf800) >> 8) |
-				    ((green & 0xe000) >> 13) |
-				    ((green & 0x1c00) << 3) |
-				    ((blue & 0xf800) >> 3);
+			pal[regno] = (((red & 0xf800) >> 8) |
+				      ((green & 0xe000) >> 13) |
+				      ((green & 0x1c00) << 3) |
+				      ((blue & 0xf800) >> 3));
 #else
-				pal[regno] = val;
+			pal[regno] = val;
 #endif
-			} else {
-				u32 *pal = sfb->fb->pseudo_palette;
+		} else {
+			u32 *pal = sfb->fb->pseudo_palette;
 
-				val = chan_to_field(red, &sfb->fb->var.red);
-				val |= chan_to_field(green,
-						     &sfb->fb->var.green);
-				val |= chan_to_field(blue, &sfb->fb->var.blue);
+			val = chan_to_field(red, &sfb->fb->var.red);
+			val |= chan_to_field(green, &sfb->fb->var.green);
+			val |= chan_to_field(blue, &sfb->fb->var.blue);
 #ifdef __BIG_ENDIAN
-				val -				    (val & 0xff00ff00 >> 8) |
-				    (val & 0x00ff00ff << 8);
+			val = (val & 0xff00ff00 >> 8) | (val & 0x00ff00ff << 8);
 #endif
-				pal[regno] = val;
-			}
+			pal[regno] = val;
 		}
 		break;
 
@@ -302,8 +297,8 @@ static int smtc_setcolreg(unsigned regno, unsigned red, unsigned green,
 }
 
 #ifdef __BIG_ENDIAN
-static ssize_t smtcfb_read(struct fb_info *info, char __user *buf, size_t
-				count, loff_t *ppos)
+static ssize_t smtcfb_read(struct fb_info *info, char __user *buf,
+			   size_t count, loff_t *ppos)
 {
 	unsigned long p = *ppos;
 
@@ -346,9 +341,8 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf, size_t
 		dst = buffer;
 		for (i = c >> 2; i--;) {
 			*dst = fb_readl(src++);
-			*dst -			    (*dst & 0xff00ff00 >> 8) |
-			    (*dst & 0x00ff00ff << 8);
+			*dst = (*dst & 0xff00ff00 >> 8) |
+			       (*dst & 0x00ff00ff << 8);
 			dst++;
 		}
 		if (c & 3) {
@@ -381,9 +375,8 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf, size_t
 	return (err) ? err : cnt;
 }
 
-static ssize_t
-smtcfb_write(struct fb_info *info, const char __user *buf, size_t count,
-	     loff_t *ppos)
+static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf,
+			    size_t count, loff_t *ppos)
 {
 	unsigned long p = *ppos;
 
@@ -478,72 +471,69 @@ static void sm7xx_set_timing(struct smtcfb_info *sfb)
 		sfb->width, sfb->height, sfb->fb->var.bits_per_pixel, sfb->hz);
 
 	for (j = 0; j < numvgamodes; j++) {
-		if (vgamode[j].mmsizex = sfb->width &&
-		    vgamode[j].mmsizey = sfb->height &&
-		    vgamode[j].bpp = sfb->fb->var.bits_per_pixel &&
-		    vgamode[j].hz = sfb->hz) {
-			dev_dbg(&sfb->pdev->dev,
-				"vgamode[j].mmsizex=%d vgamode[j].mmSizeY=%d vgamode[j].bpp=%d vgamode[j].hz=%d\n",
-				vgamode[j].mmsizex, vgamode[j].mmsizey,
-				vgamode[j].bpp, vgamode[j].hz);
-
-			dev_dbg(&sfb->pdev->dev, "vgamode index=%d\n", j);
-
-			smtc_mmiowb(0x0, 0x3c6);
-
-			smtc_seqw(0, 0x1);
-
-			smtc_mmiowb(vgamode[j].init_misc, 0x3c2);
-
-			/* init SEQ register SR00 - SR04 */
-			for (i = 0; i < SIZE_SR00_SR04; i++)
-				smtc_seqw(i, vgamode[j].init_sr00_sr04[i]);
-
-			/* init SEQ register SR10 - SR24 */
-			for (i = 0; i < SIZE_SR10_SR24; i++)
-				smtc_seqw(i + 0x10,
-					  vgamode[j].init_sr10_sr24[i]);
-
-			/* init SEQ register SR30 - SR75 */
-			for (i = 0; i < SIZE_SR30_SR75; i++)
-				if ((i + 0x30) != 0x62 &&
-				    (i + 0x30) != 0x6a &&
-				    (i + 0x30) != 0x6b)
-					smtc_seqw(i + 0x30,
-						  vgamode[j].init_sr30_sr75[i]);
-
-			/* init SEQ register SR80 - SR93 */
-			for (i = 0; i < SIZE_SR80_SR93; i++)
-				smtc_seqw(i + 0x80,
-					  vgamode[j].init_sr80_sr93[i]);
-
-			/* init SEQ register SRA0 - SRAF */
-			for (i = 0; i < SIZE_SRA0_SRAF; i++)
-				smtc_seqw(i + 0xa0,
-					  vgamode[j].init_sra0_sraf[i]);
-
-			/* init Graphic register GR00 - GR08 */
-			for (i = 0; i < SIZE_GR00_GR08; i++)
-				smtc_grphw(i, vgamode[j].init_gr00_gr08[i]);
-
-			/* init Attribute register AR00 - AR14 */
-			for (i = 0; i < SIZE_AR00_AR14; i++)
-				smtc_attrw(i, vgamode[j].init_ar00_ar14[i]);
-
-			/* init CRTC register CR00 - CR18 */
-			for (i = 0; i < SIZE_CR00_CR18; i++)
-				smtc_crtcw(i, vgamode[j].init_cr00_cr18[i]);
-
-			/* init CRTC register CR30 - CR4D */
-			for (i = 0; i < SIZE_CR30_CR4D; i++)
-				smtc_crtcw(i + 0x30,
-					   vgamode[j].init_cr30_cr4d[i]);
-
-			/* init CRTC register CR90 - CRA7 */
-			for (i = 0; i < SIZE_CR90_CRA7; i++)
-				smtc_crtcw(i + 0x90,
-					   vgamode[j].init_cr90_cra7[i]);
+		if (vgamode[j].mmsizex != sfb->width ||
+		    vgamode[j].mmsizey != sfb->height ||
+		    vgamode[j].bpp != sfb->fb->var.bits_per_pixel ||
+		    vgamode[j].hz != sfb->hz)
+			continue;
+
+		dev_dbg(&sfb->pdev->dev,
+			"vgamode[j].mmsizex=%d vgamode[j].mmSizeY=%d vgamode[j].bpp=%d vgamode[j].hz=%d\n",
+			vgamode[j].mmsizex, vgamode[j].mmsizey,
+			vgamode[j].bpp, vgamode[j].hz);
+
+		dev_dbg(&sfb->pdev->dev, "vgamode index=%d\n", j);
+
+		smtc_mmiowb(0x0, 0x3c6);
+
+		smtc_seqw(0, 0x1);
+
+		smtc_mmiowb(vgamode[j].init_misc, 0x3c2);
+
+		/* init SEQ register SR00 - SR04 */
+		for (i = 0; i < SIZE_SR00_SR04; i++)
+			smtc_seqw(i, vgamode[j].init_sr00_sr04[i]);
+
+		/* init SEQ register SR10 - SR24 */
+		for (i = 0; i < SIZE_SR10_SR24; i++)
+			smtc_seqw(i + 0x10, vgamode[j].init_sr10_sr24[i]);
+
+		/* init SEQ register SR30 - SR75 */
+		for (i = 0; i < SIZE_SR30_SR75; i++) {
+			if ((i + 0x30) != 0x62 &&
+			    (i + 0x30) != 0x6a &&
+			    (i + 0x30) != 0x6b)
+				smtc_seqw(i + 0x30,
+					  vgamode[j].init_sr30_sr75[i]);
 		}
+
+		/* init SEQ register SR80 - SR93 */
+		for (i = 0; i < SIZE_SR80_SR93; i++)
+			smtc_seqw(i + 0x80, vgamode[j].init_sr80_sr93[i]);
+
+		/* init SEQ register SRA0 - SRAF */
+		for (i = 0; i < SIZE_SRA0_SRAF; i++)
+			smtc_seqw(i + 0xa0, vgamode[j].init_sra0_sraf[i]);
+
+		/* init Graphic register GR00 - GR08 */
+		for (i = 0; i < SIZE_GR00_GR08; i++)
+			smtc_grphw(i, vgamode[j].init_gr00_gr08[i]);
+
+		/* init Attribute register AR00 - AR14 */
+		for (i = 0; i < SIZE_AR00_AR14; i++)
+			smtc_attrw(i, vgamode[j].init_ar00_ar14[i]);
+
+		/* init CRTC register CR00 - CR18 */
+		for (i = 0; i < SIZE_CR00_CR18; i++)
+			smtc_crtcw(i, vgamode[j].init_cr00_cr18[i]);
+
+		/* init CRTC register CR30 - CR4D */
+		for (i = 0; i < SIZE_CR30_CR4D; i++)
+			smtc_crtcw(i + 0x30, vgamode[j].init_cr30_cr4d[i]);
+
+		/* init CRTC register CR90 - CRA7 */
+		for (i = 0; i < SIZE_CR90_CRA7; i++)
+			smtc_crtcw(i + 0x90, vgamode[j].init_cr90_cra7[i]);
 	}
 	smtc_mmiowb(0x67, 0x3c2);
 
@@ -552,8 +542,7 @@ static void sm7xx_set_timing(struct smtcfb_info *sfb)
 	writel(0x0, sfb->vp_regs + 0x40);
 
 	/* set data width */
-	m_nscreenstride -		(sfb->width * sfb->fb->var.bits_per_pixel) / 64;
+	m_nscreenstride = (sfb->width * sfb->fb->var.bits_per_pixel) / 64;
 	switch (sfb->fb->var.bits_per_pixel) {
 	case 8:
 		writel(0x0, sfb->vp_regs + 0x0);
@@ -741,7 +730,7 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
 	int err;
 	unsigned long mmio_base;
 
-	dev_info(&pdev->dev, "Silicon Motion display driver.");
+	dev_info(&pdev->dev, "Silicon Motion display driver\n");
 
 	err = pci_enable_device(pdev);	/* enable SMTC chip */
 	if (err)
@@ -815,12 +804,12 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
 #ifdef __BIG_ENDIAN
 		if (sfb->fb->var.bits_per_pixel = 32) {
 			sfb->lfb += 0x800000;
-			dev_info(&pdev->dev, "sfb->lfb=%p", sfb->lfb);
+			dev_info(&pdev->dev, "sfb->lfb=%p\n", sfb->lfb);
 		}
 #endif
 		if (!smtc_regbaseaddress) {
 			dev_err(&pdev->dev,
-				"%s: unable to map memory mapped IO!",
+				"%s: unable to map memory mapped IO!\n",
 				sfb->fb->fix.id);
 			err = -ENOMEM;
 			goto failed_fb;
@@ -854,7 +843,7 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
 		break;
 	default:
 		dev_err(&pdev->dev,
-			"No valid Silicon Motion display chip was detected!");
+			"No valid Silicon Motion display chip was detected!\n");
 
 		goto failed_fb;
 	}
@@ -876,14 +865,14 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
 		goto failed;
 
 	dev_info(&pdev->dev,
-		 "Silicon Motion SM%X Rev%X primary display mode %dx%d-%d Init Complete.",
+		 "Silicon Motion SM%X Rev%X primary display mode %dx%d-%d Init Complete\n",
 		 sfb->chip_id, sfb->chip_rev_id, sfb->fb->var.xres,
 		 sfb->fb->var.yres, sfb->fb->var.bits_per_pixel);
 
 	return 0;
 
 failed:
-	dev_err(&pdev->dev, "Silicon Motion, Inc. primary display init fail.");
+	dev_err(&pdev->dev, "Silicon Motion, Inc. primary display init failed\n");
 
 	smtc_unmap_smem(sfb);
 	smtc_unmap_mmio(sfb);

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Date: 2015-06-15 05:17:37 

On Sat, Jun 13, 2015 at 09:57:05AM -0700, Joe Perches wrote:
It seems ready to me.

As far as I can tell, there's just a few niggles
that could be done:

Something like (too lazy to separate them into multiple patches)
Thanks.	I will break into patches. Call me lazy for not having it done
till now.
o reduce indentation a couple of places
o add newlines to logging messages
o add const to static array
o use consistent function declaration style

It's unfortunate there are so many #ifdef __BIG_ENDIAN uses.
instead of #ifdef __BIG_ENDIAN can i then use a bool flag to check by if-else?

regards
sudip

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Joe Perches <joe@perches.com>
Date: 2015-06-15 06:37:06 

On Mon, 2015-06-15 at 10:47 +0530, Sudip Mukherjee wrote:
On Sat, Jun 13, 2015 at 09:57:05AM -0700, Joe Perches wrote:
quoted
It's unfortunate there are so many #ifdef __BIG_ENDIAN uses.
instead of #ifdef __BIG_ENDIAN can i then use a bool flag to check by if-else?
I think that'd be worse.  Moving the #ifdef into the .h
may be better, but <shrug>, whatever works well enough.

Another thing may be to move the vgamode array declaration
in fb7xx.h to the .c file and make it const and remove the
#define numvgamodes and just use ARRAY_SIZE directly in
the one place it's used.

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Dan Carpenter <hidden>
Date: 2015-06-19 10:29:37 

On Sat, Jun 13, 2015 at 02:16:18PM +0530, Sudip Mukherjee wrote:
can you please have a look (when you are free) at it to see if anything
else needs to be done.
Remove any unused macros.
Cleanup indenting in the .h file.
drivers/staging/sm7xxfb/sm7xxfb.c:821 smtcfb_pci_probe() warn: 'smtc_regbaseaddress' can't be NULL.
move the BIG_ENDIAN ifdefs to the .h file.  I don't understand why only
big endian systems get a fb_read/write?  Cleanup casts in fb_read/write.
cleanup comments.  make sure they are up to date and make sense.
cleanup function declarations.  Make the style consistent.
run checkpatch.pl --strict
Remind me we need the #ifndef MODULE?

regards,
dan carpenter

Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver)

From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Date: 2015-06-20 11:20:23 

On Fri, Jun 19, 2015 at 01:29:13PM +0300, Dan Carpenter wrote:
On Sat, Jun 13, 2015 at 02:16:18PM +0530, Sudip Mukherjee wrote:
quoted
can you please have a look (when you are free) at it to see if anything
else needs to be done.
Remove any unused macros.
I will check.
Cleanup indenting in the .h file.
done.
drivers/staging/sm7xxfb/sm7xxfb.c:821 smtcfb_pci_probe() warn: 'smtc_regbaseaddress' can't be NULL.
i dont see this warning. Is it smatch?
move the BIG_ENDIAN ifdefs to the .h file.  I don't understand why only
big endian systems get a fb_read/write?
Greg asked to remove the BIG_ENDIAN ifdefs. for BIG_ENDIAN some
calculations are involved that is why the functions are defined.
LITTLE_ENDIAN will use the default read/write provided by fb core.
cleanup comments.  make sure they are up to date and make sense.
will do.
cleanup function declarations.  Make the style consistent.
i think its already done, with Joe's help.
run checkpatch.pl --strict
done.
Remind me we need the #ifndef MODULE?
This one I introduced with the plan that if it is built-in then it will
use the commandline to get the mode, but if it is a module then it will
use module parameters. But later Greg told that framebuffers should not
use module_param. I dont see any reason why a module can nor get the mode
values from the command line. I will test that during this period of the
merge window and prepare the final patch.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help