Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1164488 > unrolled thread

Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus  driver Greg KH <gregkh@linuxfoundation.org> - 2015-06-13 02:20 +0200
    doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc: New  functionality to the MC bus driver) Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-06-13 10:50 +0200
      Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc:  New functionality to the MC bus driver) Greg KH <gregkh@linuxfoundation.org> - 2015-06-13 18:30 +0200
        Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc:  New functionality to the MC bus driver) Joe Perches <joe@perches.com> - 2015-06-13 19:00 +0200
          Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc:  New functionality to the MC bus driver) Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-06-15 07:20 +0200
            Re: doubt about sm7xxfb (was: Re: [PATCH v4 0/7] staging: fsl-mc:  New functionality to the MC bus driver) Joe Perches <joe@perches.com> - 2015-06-15 08:40 +0200

#1164488 — Re: [PATCH v4 0/7] staging: fsl-mc: New functionality to the MC bus driver

On Tue, Jun 09, 2015 at 04:59:01PM -0500, J. German Rivera wrote:
> 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.

So I really don't want to take this series, sorry, someone better start
working on getting this out of drivers/staging/ or I'm going to have to
drop the driver entirely from the tree.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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:
> > 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
   
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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:
> > On Tue, Jun 09, 2015 at 04:59:01PM -0500, J. German Rivera wrote:
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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:
[]
> > 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);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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:
> > 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web