Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

2013-08-26 Thread Sylwester Nawrocki

On 08/19/2013 12:58 PM, Shaik Ameer Basha wrote:

This patch adds support for M-Scaler (M2M Scaler) device which is a
new device for scaling, blending, color fill  and color space
conversion on EXYNOS5 SoCs.

This device supports the followings as key feature.
 input image format
 - YCbCr420 2P(UV/VU), 3P
 - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
 - YCbCr444 2P(UV,VU), 3P
 - RGB565, ARGB1555, ARGB, ARGB, RGBA
 - Pre-multiplexed ARGB, L8A8 and L8
 output image format
 - YCbCr420 2P(UV/VU), 3P
 - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
 - YCbCr444 2P(UV,VU), 3P
 - RGB565, ARGB1555, ARGB, ARGB, RGBA
 - Pre-multiplexed ARGB
 input rotation
 - 0/90/180/270 degree, X/Y/XY Flip
 scale ratio
 - 1/4 scale down to 16 scale up
 color space conversion
 - RGB to YUV / YUV to RGB
 Size
 - Input : 16x16 to 8192x8192
 - Output:   4x4 to 8192x8192
 alpha blending, color fill


We don't have good support for alpha blending in v4l2. For example, the
s5p-g2d v4l2 driver only supports a subset of features of the device.
The G2D IP is probably better utilized through the exynos drm driver.

It might be a matter of designing proper controls though for such devices
that generate image by blending data from the output and capture buffers.


Signed-off-by: Shaik Ameer Bashashaik.am...@samsung.com
---
  drivers/media/platform/exynos-mscl/mscl-regs.c |  318 
  drivers/media/platform/exynos-mscl/mscl-regs.h |  282 +


Shouldn't we call this driver exynos5-scaler and this files would be
renamed to drivers/media/platform/exynos5-scaler/scaler-regs.[ch].

Or maybe put maybe put the SCALER, G-SCALER drivers into common exynos5-is
directory ? I don't have strong preference. But we planned gscaler to be
moved into the exynos5-is directory.


  2 files changed, 600 insertions(+)
  create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.c
  create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.h

diff --git a/drivers/media/platform/exynos-mscl/mscl-regs.c 
b/drivers/media/platform/exynos-mscl/mscl-regs.c
new file mode 100644
index 000..9354afc
--- /dev/null
+++ b/drivers/media/platform/exynos-mscl/mscl-regs.c
@@ -0,0 +1,318 @@
+/*
+ * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.


Just make it 2013 ? I think we can manage to merge this diver this year :)


+ * http://www.samsung.com
+ *
+ * Samsung EXYNOS5 SoC series M-Scaler driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.


Are you sure you want to have this or (at your option) any later version
clause ?


+ */
+
+#includelinux/delay.h
+#includelinux/platform_device.h
+
+#include mscl-core.h


How about defining register access helpers like:

u32 mscl_read(struct mscl_dev *dev, u32 offset)
{
readl(dev-regs + offset);
}

void mscl_write(struct mscl_dev *dev, u32 offset, u32 value)
{
writel(value, dev-regs + offset);
}

perhaps as static inline in mscl-regs.h ?


+void mscl_hw_set_sw_reset(struct mscl_dev *dev)
+{
+   u32 cfg;
+
+   cfg = readl(dev-regs + MSCL_CFG);
+   cfg |= MSCL_CFG_SOFT_RESET;
+
+   writel(cfg, dev-regs + MSCL_CFG);
+}
+
+int mscl_wait_reset(struct mscl_dev *dev)
+{
+   unsigned long end = jiffies + msecs_to_jiffies(50);


Don't you want a #define for this timeout ?


+   u32 cfg, reset_done = 0;
+
+   while (time_before(jiffies, end)) {
+   cfg = readl(dev-regs + MSCL_CFG);
+   if (!(cfg  MSCL_CFG_SOFT_RESET)) {
+   reset_done = 1;
+   break;
+   }
+   usleep_range(10, 20);
+   }
+
+   /* write any value to r/w reg and read it back */
+   while (reset_done) {
+
+   /* [TBD] need to define number of tries before returning
+* -EBUSY to the caller
+*/


CodingStyle: wrong multi-line comment style


+   writel(MSCL_CFG_SOFT_RESET_CHECK_VAL,
+   dev-regs + MSCL_CFG_SOFT_RESET_CHECK_REG);
+   if (MSCL_CFG_SOFT_RESET_CHECK_VAL ==
+   readl(dev-regs + MSCL_CFG_SOFT_RESET_CHECK_REG))
+   return 0;
+   }
+
+   return -EBUSY;
+}
+
+void mscl_hw_set_irq_mask(struct mscl_dev *dev, int interrupt, bool mask)
+{
+   u32 cfg;
+
+   switch (interrupt) {
+   case MSCL_INT_TIMEOUT:
+   case MSCL_INT_ILLEGAL_BLEND:
+   case MSCL_INT_ILLEGAL_RATIO:
+   case MSCL_INT_ILLEGAL_DST_HEIGHT:
+   case MSCL_INT_ILLEGAL_DST_WIDTH:
+   case MSCL_INT_ILLEGAL_DST_V_POS:
+   case 

Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

2013-08-20 Thread Shaik Ameer Basha
Hi Inki Dae,

Thanks for the review.


On Mon, Aug 19, 2013 at 6:18 PM, Inki Dae inki@samsung.com wrote:
 Just quick review.

 -Original Message-
 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Shaik Ameer Basha
 Sent: Monday, August 19, 2013 7:59 PM
 To: linux-media@vger.kernel.org; linux-samsung-...@vger.kernel.org
 Cc: s.nawro...@samsung.com; posc...@google.com; arun...@samsung.com;
 shaik.am...@samsung.com
 Subject: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

 This patch adds support for M-Scaler (M2M Scaler) device which is a
 new device for scaling, blending, color fill  and color space
 conversion on EXYNOS5 SoCs.

 This device supports the followings as key feature.
 input image format
 - YCbCr420 2P(UV/VU), 3P
 - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
 - YCbCr444 2P(UV,VU), 3P
 - RGB565, ARGB1555, ARGB, ARGB, RGBA
 - Pre-multiplexed ARGB, L8A8 and L8
 output image format
 - YCbCr420 2P(UV/VU), 3P
 - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
 - YCbCr444 2P(UV,VU), 3P
 - RGB565, ARGB1555, ARGB, ARGB, RGBA
 - Pre-multiplexed ARGB
 input rotation
 - 0/90/180/270 degree, X/Y/XY Flip
 scale ratio
 - 1/4 scale down to 16 scale up
 color space conversion
 - RGB to YUV / YUV to RGB
 Size
 - Input : 16x16 to 8192x8192
 - Output:   4x4 to 8192x8192
 alpha blending, color fill

 Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
 ---
  drivers/media/platform/exynos-mscl/mscl-regs.c |  318
 
  drivers/media/platform/exynos-mscl/mscl-regs.h |  282
 +
  2 files changed, 600 insertions(+)
  create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.c
  create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.h

 diff --git a/drivers/media/platform/exynos-mscl/mscl-regs.c
 b/drivers/media/platform/exynos-mscl/mscl-regs.c
 new file mode 100644
 index 000..9354afc
 --- /dev/null
 +++ b/drivers/media/platform/exynos-mscl/mscl-regs.c
 @@ -0,0 +1,318 @@
 +/*
 + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.
 + *   http://www.samsung.com
 + *
 + * Samsung EXYNOS5 SoC series M-Scaler driver
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published
 + * by the Free Software Foundation, either version 2 of the License,
 + * or (at your option) any later version.
 + */
 +
 +#include linux/delay.h
 +#include linux/platform_device.h
 +
 +#include mscl-core.h
 +
 +void mscl_hw_set_sw_reset(struct mscl_dev *dev)
 +{
 + u32 cfg;
 +
 + cfg = readl(dev-regs + MSCL_CFG);
 + cfg |= MSCL_CFG_SOFT_RESET;
 +
 + writel(cfg, dev-regs + MSCL_CFG);
 +}
 +
 +int mscl_wait_reset(struct mscl_dev *dev)
 +{
 + unsigned long end = jiffies + msecs_to_jiffies(50);

 What does 50 mean?

 + u32 cfg, reset_done = 0;
 +

 Please describe why the below codes are needed.


As per the Documentation,

 SOFT RESET: Writing 1 to this bit generates software reset. To
check the completion of the reset, wait until this
field becomes zero, then wrie an arbitrary value to any of RW
registers and read it. If the read data matches the written data,
 it means SW reset succeeded. Otherwise, repeat write  read until matched.


Thie below code tries to do the same (as per user manual). and in the
above msec_to_jiffies(50), 50 is the 50msec wait. before
checking the SOFT RESET is really done.

Is it good to ignore this checks?




 + while (time_before(jiffies, end)) {
 + cfg = readl(dev-regs + MSCL_CFG);
 + if (!(cfg  MSCL_CFG_SOFT_RESET)) {
 + reset_done = 1;
 + break;
 + }
 + usleep_range(10, 20);
 + }
 +
 + /* write any value to r/w reg and read it back */
 + while (reset_done) {
 +
 + /* [TBD] need to define number of tries before returning
 +  * -EBUSY to the caller
 +  */
 +
 + writel(MSCL_CFG_SOFT_RESET_CHECK_VAL,
 + dev-regs + MSCL_CFG_SOFT_RESET_CHECK_REG);
 + if (MSCL_CFG_SOFT_RESET_CHECK_VAL ==
 + readl(dev-regs + MSCL_CFG_SOFT_RESET_CHECK_REG))
 + return 0;
 + }
 +
 + return -EBUSY;
 +}
 +
 +void mscl_hw_set_irq_mask(struct mscl_dev *dev, int interrupt, bool mask)
 +{
 + u32 cfg;
 +
 + switch (interrupt) {
 + case MSCL_INT_TIMEOUT:
 + case MSCL_INT_ILLEGAL_BLEND:
 + case MSCL_INT_ILLEGAL_RATIO:
 + case MSCL_INT_ILLEGAL_DST_HEIGHT:
 + case MSCL_INT_ILLEGAL_DST_WIDTH:
 + case MSCL_INT_ILLEGAL_DST_V_POS:
 + case MSCL_INT_ILLEGAL_DST_H_POS:
 + case MSCL_INT_ILLEGAL_DST_C_SPAN:
 + case MSCL_INT_ILLEGAL_DST_Y_SPAN:
 + 

RE: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

2013-08-20 Thread Inki Dae


 -Original Message-
 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Shaik Ameer Basha
 Sent: Tuesday, August 20, 2013 5:07 PM
 To: Inki Dae
 Cc: Shaik Ameer Basha; LMML; linux-samsung-...@vger.kernel.org;
 c...@samsung.com; Sylwester Nawrocki; posc...@google.com; Arun Kumar K
 Subject: Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-
 Scaler
 
 Hi Inki Dae,
 
 Thanks for the review.
 
 
 On Mon, Aug 19, 2013 at 6:18 PM, Inki Dae inki@samsung.com wrote:
  Just quick review.
 
  -Original Message-
  From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
  ow...@vger.kernel.org] On Behalf Of Shaik Ameer Basha
  Sent: Monday, August 19, 2013 7:59 PM
  To: linux-media@vger.kernel.org; linux-samsung-...@vger.kernel.org
  Cc: s.nawro...@samsung.com; posc...@google.com; arun...@samsung.com;
  shaik.am...@samsung.com
  Subject: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-
 Scaler
 
  This patch adds support for M-Scaler (M2M Scaler) device which is a
  new device for scaling, blending, color fill  and color space
  conversion on EXYNOS5 SoCs.
 
  This device supports the followings as key feature.
  input image format
  - YCbCr420 2P(UV/VU), 3P
  - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
  - YCbCr444 2P(UV,VU), 3P
  - RGB565, ARGB1555, ARGB, ARGB, RGBA
  - Pre-multiplexed ARGB, L8A8 and L8
  output image format
  - YCbCr420 2P(UV/VU), 3P
  - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
  - YCbCr444 2P(UV,VU), 3P
  - RGB565, ARGB1555, ARGB, ARGB, RGBA
  - Pre-multiplexed ARGB
  input rotation
  - 0/90/180/270 degree, X/Y/XY Flip
  scale ratio
  - 1/4 scale down to 16 scale up
  color space conversion
  - RGB to YUV / YUV to RGB
  Size
  - Input : 16x16 to 8192x8192
  - Output:   4x4 to 8192x8192
  alpha blending, color fill
 
  Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
  ---
   drivers/media/platform/exynos-mscl/mscl-regs.c |  318
  
   drivers/media/platform/exynos-mscl/mscl-regs.h |  282
  +
   2 files changed, 600 insertions(+)
   create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.c
   create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.h
 
  diff --git a/drivers/media/platform/exynos-mscl/mscl-regs.c
  b/drivers/media/platform/exynos-mscl/mscl-regs.c
  new file mode 100644
  index 000..9354afc
  --- /dev/null
  +++ b/drivers/media/platform/exynos-mscl/mscl-regs.c
  @@ -0,0 +1,318 @@
  +/*
  + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.
  + *   http://www.samsung.com
  + *
  + * Samsung EXYNOS5 SoC series M-Scaler driver
  + *
  + * This program is free software; you can redistribute it and/or
 modify
  + * it under the terms of the GNU General Public License as published
  + * by the Free Software Foundation, either version 2 of the License,
  + * or (at your option) any later version.
  + */
  +
  +#include linux/delay.h
  +#include linux/platform_device.h
  +
  +#include mscl-core.h
  +
  +void mscl_hw_set_sw_reset(struct mscl_dev *dev)
  +{
  + u32 cfg;
  +
  + cfg = readl(dev-regs + MSCL_CFG);
  + cfg |= MSCL_CFG_SOFT_RESET;
  +
  + writel(cfg, dev-regs + MSCL_CFG);
  +}
  +
  +int mscl_wait_reset(struct mscl_dev *dev)
  +{
  + unsigned long end = jiffies + msecs_to_jiffies(50);
 
  What does 50 mean?
 
  + u32 cfg, reset_done = 0;
  +
 
  Please describe why the below codes are needed.
 
 
 As per the Documentation,
 
  SOFT RESET: Writing 1 to this bit generates software reset. To
 check the completion of the reset, wait until this
 field becomes zero, then wrie an arbitrary value to any of RW
 registers and read it. If the read data matches the written data,
  it means SW reset succeeded. Otherwise, repeat write  read until
 matched.
 
 
 Thie below code tries to do the same (as per user manual). and in the
 above msec_to_jiffies(50), 50 is the 50msec wait. before
 checking the SOFT RESET is really done.
 
 Is it good to ignore this checks?
 

No, I mean that someone may want to understand your codes so leave comments 
enough for them.

Thanks,
Inki Dae

 
 
 
  + while (time_before(jiffies, end)) {
  + cfg = readl(dev-regs + MSCL_CFG);
  + if (!(cfg  MSCL_CFG_SOFT_RESET)) {
  + reset_done = 1;
  + break;
  + }
  + usleep_range(10, 20);
  + }
  +
  + /* write any value to r/w reg and read it back */
  + while (reset_done) {
  +
  + /* [TBD] need to define number of tries before returning
  +  * -EBUSY to the caller
  +  */
  +
  + writel(MSCL_CFG_SOFT_RESET_CHECK_VAL,
  + dev-regs + MSCL_CFG_SOFT_RESET_CHECK_REG

Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

2013-08-20 Thread Shaik Ameer Basha
On Tue, Aug 20, 2013 at 2:13 PM, Inki Dae inki@samsung.com wrote:


 -Original Message-
 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of Shaik Ameer Basha
 Sent: Tuesday, August 20, 2013 5:07 PM
 To: Inki Dae
 Cc: Shaik Ameer Basha; LMML; linux-samsung-...@vger.kernel.org;
 c...@samsung.com; Sylwester Nawrocki; posc...@google.com; Arun Kumar K
 Subject: Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-
 Scaler

 Hi Inki Dae,

 Thanks for the review.


 On Mon, Aug 19, 2013 at 6:18 PM, Inki Dae inki@samsung.com wrote:
  Just quick review.
 
  -Original Message-
  From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
  ow...@vger.kernel.org] On Behalf Of Shaik Ameer Basha
  Sent: Monday, August 19, 2013 7:59 PM
  To: linux-media@vger.kernel.org; linux-samsung-...@vger.kernel.org
  Cc: s.nawro...@samsung.com; posc...@google.com; arun...@samsung.com;
  shaik.am...@samsung.com
  Subject: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-
 Scaler
 
  This patch adds support for M-Scaler (M2M Scaler) device which is a
  new device for scaling, blending, color fill  and color space
  conversion on EXYNOS5 SoCs.
 
  This device supports the followings as key feature.
  input image format
  - YCbCr420 2P(UV/VU), 3P
  - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
  - YCbCr444 2P(UV,VU), 3P
  - RGB565, ARGB1555, ARGB, ARGB, RGBA
  - Pre-multiplexed ARGB, L8A8 and L8
  output image format
  - YCbCr420 2P(UV/VU), 3P
  - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P
  - YCbCr444 2P(UV,VU), 3P
  - RGB565, ARGB1555, ARGB, ARGB, RGBA
  - Pre-multiplexed ARGB
  input rotation
  - 0/90/180/270 degree, X/Y/XY Flip
  scale ratio
  - 1/4 scale down to 16 scale up
  color space conversion
  - RGB to YUV / YUV to RGB
  Size
  - Input : 16x16 to 8192x8192
  - Output:   4x4 to 8192x8192
  alpha blending, color fill
 
  Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
  ---
   drivers/media/platform/exynos-mscl/mscl-regs.c |  318
  
   drivers/media/platform/exynos-mscl/mscl-regs.h |  282
  +
   2 files changed, 600 insertions(+)
   create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.c
   create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.h
 
  diff --git a/drivers/media/platform/exynos-mscl/mscl-regs.c
  b/drivers/media/platform/exynos-mscl/mscl-regs.c
  new file mode 100644
  index 000..9354afc
  --- /dev/null
  +++ b/drivers/media/platform/exynos-mscl/mscl-regs.c
  @@ -0,0 +1,318 @@
  +/*
  + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.
  + *   http://www.samsung.com
  + *
  + * Samsung EXYNOS5 SoC series M-Scaler driver
  + *
  + * This program is free software; you can redistribute it and/or
 modify
  + * it under the terms of the GNU General Public License as published
  + * by the Free Software Foundation, either version 2 of the License,
  + * or (at your option) any later version.
  + */
  +
  +#include linux/delay.h
  +#include linux/platform_device.h
  +
  +#include mscl-core.h
  +
  +void mscl_hw_set_sw_reset(struct mscl_dev *dev)
  +{
  + u32 cfg;
  +
  + cfg = readl(dev-regs + MSCL_CFG);
  + cfg |= MSCL_CFG_SOFT_RESET;
  +
  + writel(cfg, dev-regs + MSCL_CFG);
  +}
  +
  +int mscl_wait_reset(struct mscl_dev *dev)
  +{
  + unsigned long end = jiffies + msecs_to_jiffies(50);
 
  What does 50 mean?
 
  + u32 cfg, reset_done = 0;
  +
 
  Please describe why the below codes are needed.


 As per the Documentation,

  SOFT RESET: Writing 1 to this bit generates software reset. To
 check the completion of the reset, wait until this
 field becomes zero, then wrie an arbitrary value to any of RW
 registers and read it. If the read data matches the written data,
  it means SW reset succeeded. Otherwise, repeat write  read until
 matched.


 Thie below code tries to do the same (as per user manual). and in the
 above msec_to_jiffies(50), 50 is the 50msec wait. before
 checking the SOFT RESET is really done.

 Is it good to ignore this checks?


 No, I mean that someone may want to understand your codes so leave comments 
 enough for them.


Ok. thanks. I will add more comments. :)

Regards,
Shaik Ameer Basha


 Thanks,
 Inki Dae



 
  + while (time_before(jiffies, end)) {
  + cfg = readl(dev-regs + MSCL_CFG);
  + if (!(cfg  MSCL_CFG_SOFT_RESET)) {
  + reset_done = 1;
  + break;
  + }
  + usleep_range(10, 20);
  + }
  +
  + /* write any value to r/w reg and read it back */
  + while (reset_done) {
  +
  + /* [TBD] need to define number of tries before returning
  +  * -EBUSY to the caller