RE: [PATCH 2/2] video: s3c-fb: Modify s3c-fb driver to support window positioning

Hi, Ajay.

Can you move this to separate function?
In my opinion, it would be better for readability and consistency.
Also, FBIO_WAITFORVSYNC has been implemented in this way.
		ret = s3c_fb_wait_for_vsync(sfb, crtc);
How about adding a function 's3c_fb_set_window_position' for this as belows?
+int s3c_fb_set_window_position(struct fb_info *info,
+				struct s3cfb_window_pos winpos)
		........
+	case S3CFB_WIN_POSITION:
+		if (copy_from_user(winpos, (u32 __user *)arg,
+					sizeof(struct s3cfb_window_pos))) {
+			ret = -EFAULT;
+			break;
+		}
+		ret = s3c_fb_set_window_position(info, winpos);
+		break;

 	default:
 		ret = -ENOTTY;
 	}
--
1.7.0.4

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±ıöİzÿâ�Ø^n‡r¡ö¦zË�ëh™¨è­Ú&£ûàz¿äz¹Ş—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿï�êÿ‘êçz_è®æj:+v‰¨ş)ߣøm

Hi Jingoo,

2011/8/26 JinGoo Han [off-list ref]:

Hi, Ajay.

quoted

-----Original Message-----
From: Ajay Kumar [mailto:ajaykumar.rs@samsung.com]
Sent: Friday, August 26, 2011 4:52 AM
To: linux-samsung-soc@vger.kernel.org; linux-fbdev@vger.kernel.org; linux-
arm-kernel@lists.infradead.org
Cc: FlorianSchandinat@gmx.de; lethal@linux-sh.org; jg1.han@samsung.com;
m.szyprowski@samsung.com; ben-linux@fluff.org; banajit.g@samsung.com
Subject: [PATCH 2/2] video: s3c-fb: Modify s3c-fb driver to support window
positioning

This patch modifies the existing s3c-fb driver to provide
flexibility to the user to reposition the framebuffer windows.

Signed-off-by: Ajay Kumar <redacted>
Signed-off-by: Banajit Goswami <redacted>
---
 drivers/video/s3c-fb.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0fda252..41179d7 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c

@@ -442,6 +442,7 @@ static int s3c_fb_set_par(struct fb_info *info)

      struct fb_var_screeninfo *var = &info->var;
      struct s3c_fb_win *win = info->par;
      struct s3c_fb *sfb = win->parent;
+     struct s3cfb_window_pos *winpos = &win->windata->winpos;
      void __iomem *regs = sfb->regs;
      void __iomem *buf = regs;
      int win_no = win->index;

@@ -539,12 +540,13 @@ static int s3c_fb_set_par(struct fb_info *info)

      /* write 'OSD' registers to control position of framebuffer */

-     data = VIDOSDxA_TOPLEFT_X(0) | VIDOSDxA_TOPLEFT_Y(0);
+     data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+            VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y);
      writel(data, regs + VIDOSD_A(win_no, sfb->variant));

-     data = VIDOSDxB_BOTRIGHT_X(s3c_fb_align_word(var->bits_per_pixel,
-                                                  var->xres - 1)) |
-            VIDOSDxB_BOTRIGHT_Y(var->yres - 1);
+     data = VIDOSDxB_BOTRIGHT_X((s3c_fb_align_word(var->bits_per_pixel,
+            (winpos->win_pos_x + var->xres - 1)))) |
+            VIDOSDxB_BOTRIGHT_Y((winpos->win_pos_y + var->yres - 1));

      writel(data, regs + VIDOSD_B(win_no, sfb->variant));

@@ -999,8 +1001,10 @@ static int s3c_fb_ioctl(struct fb_info *info,

unsigned int cmd,
 {
      struct s3c_fb_win *win = info->par;
      struct s3c_fb *sfb = win->parent;
-     int ret;
+     struct s3cfb_window_pos *winpos = &win->windata->winpos;
+     int ret = 0;
      u32 crtc;
+     u32 data;

      switch (cmd) {
      case FBIO_WAITFORVSYNC:

@@ -1011,6 +1015,29 @@ static int s3c_fb_ioctl(struct fb_info *info,

unsigned int cmd,

              ret = s3c_fb_wait_for_vsync(sfb, crtc);
              break;
+     case S3CFB_WIN_POSITION:
+             if (copy_from_user(winpos, (u32 __user *)arg,
+                                     sizeof(struct s3cfb_window_pos))) {
+                     ret = -EFAULT;
+                     break;
+             }
+
+             shadow_protect_win(win, 1);
+
+             /* write 'OSD' registers to set position of the window */
+             data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+                    VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y);
+             writel(data, sfb->regs + VIDOSD_A(win->index, sfb->variant));
+
+             data = VIDOSDxB_BOTRIGHT_X(
+                             s3c_fb_align_word(info->var.bits_per_pixel,
+                             (winpos->win_pos_x + info->var.xres - 1)));
+             data |= VIDOSDxB_BOTRIGHT_Y(winpos->win_pos_y +
+                             info->var.yres - 1);
+             writel(data, sfb->regs + VIDOSD_B(win->index, sfb->variant));
+
+             shadow_protect_win(win, 0);
+             break;

Can you move this to separate function?
In my opinion, it would be better for readability and consistency.
Also, FBIO_WAITFORVSYNC has been implemented in this way.
               ret = s3c_fb_wait_for_vsync(sfb, crtc);
How about adding a function 's3c_fb_set_window_position' for this as belows?
+int s3c_fb_set_window_position(struct fb_info *info,
+                               struct s3cfb_window_pos winpos)

Thanks for your suggestion.
I have considered it, and now I have written a separate function for this.
I will be sending it in V2 version.

               ........
+       case S3CFB_WIN_POSITION:
+               if (copy_from_user(winpos, (u32 __user *)arg,
+                                       sizeof(struct s3cfb_window_pos))) {
+                       ret = -EFAULT;
+                       break;
+               }
+               ret = s3c_fb_set_window_position(info, winpos);
+               break;

quoted

      default:
              ret = -ENOTTY;
      }
--
1.7.0.4