[PATCH v2 0/4] OMAP3-ISP lane shifter support

[PATCH v2 0/4] OMAP3-ISP lane shifter support

[Michael Jones]

Add support for the ISP's lane shifter.  To use the shifter, set different
pixel formats at each end of the link at the CCDC input.

This has only been tested shifting Y12 and SBGGR12 from a parallel sensor to Y8
and SBGGR8 (respectively) at the CCDC input.  Support has also been added for
other formats and other shifting values, but is untested.  Shifting data coming
from one of the serial sensor interfaces (CSI2a, etc) is also untested.

As before, ccdc_try_format() does not check that the format at its input is
compatible with the format coming from the sensor interface. This consistency
check is first done when activating the pipeline.

These patches apply to Laurent's media-0005-omap3isp branch, based on 2.6.38-rc5

Changes since v1:
-added support for remaining 8-bit Bayer formats (SGBRG8_1X8 & SRGGB8_1X8)
-moved omap3isp_is_shiftable() from isp.c to ispvideo.c and return bool
-moved 'shift' calculation from omap3isp_configure_bridge() to ccdc_configure()
-added 'shift' arg to omap3isp_configure_bridge()
-misc minor changes according to feedback (removed unnecessary tests, etc.)

Michael Jones (4):
  v4l: add V4L2_PIX_FMT_Y12 format
  media: add 8-bit bayer formats and Y12
  omap3isp: ccdc: support Y10/12, 8-bit bayer fmts
  omap3isp: lane shifter support

 drivers/media/video/omap3-isp/isp.c      |    6 +-
 drivers/media/video/omap3-isp/isp.h      |    5 +-
 drivers/media/video/omap3-isp/ispccdc.c  |   32 +++++++++-
 drivers/media/video/omap3-isp/ispvideo.c |   97 +++++++++++++++++++++++++----
 drivers/media/video/omap3-isp/ispvideo.h |    3 +
 include/linux/v4l2-mediabus.h            |    7 ++-
 include/linux/videodev2.h                |    1 +
 7 files changed, 126 insertions(+), 25 deletions(-)

-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[PATCH v2 1/4] v4l: add V4L2_PIX_FMT_Y12 format

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/videodev2.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 02da9e7..6fac463 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -288,6 +288,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_Y4      v4l2_fourcc('Y', '0', '4', ' ') /*  4  Greyscale     */
 #define V4L2_PIX_FMT_Y6      v4l2_fourcc('Y', '0', '6', ' ') /*  6  Greyscale     */
 #define V4L2_PIX_FMT_Y10     v4l2_fourcc('Y', '1', '0', ' ') /* 10  Greyscale     */
+#define V4L2_PIX_FMT_Y12     v4l2_fourcc('Y', '1', '2', ' ') /* 12  Greyscale     */
 #define V4L2_PIX_FMT_Y16     v4l2_fourcc('Y', '1', '6', ' ') /* 16  Greyscale     */
 
 /* Palette formats */
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[PATCH v2 2/4] media: add 8-bit bayer formats and Y12

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/v4l2-mediabus.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 7054a7a..46caecd 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -47,8 +47,9 @@ enum v4l2_mbus_pixelcode {
 	V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
 	V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
 
-	/* YUV (including grey) - next is 0x2013 */
+	/* YUV (including grey) - next is 0x2014 */
 	V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
+	V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
 	V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
 	V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
 	V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
@@ -67,9 +68,11 @@ enum v4l2_mbus_pixelcode {
 	V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
 	V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
 
-	/* Bayer - next is 0x3013 */
+	/* Bayer - next is 0x3015 */
 	V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
+	V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
 	V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
+	V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
 	V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
 	V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
 	V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[PATCH v2 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts

[Michael Jones]

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3-isp/ispccdc.c  |    6 ++++++
 drivers/media/video/omap3-isp/ispvideo.c |   12 ++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c
index e4d04ce..23000b6 100644
--- a/drivers/media/video/omap3-isp/ispccdc.c
+++ b/drivers/media/video/omap3-isp/ispccdc.c
@@ -43,6 +43,12 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 
 static const unsigned int ccdc_fmts[] = {
 	V4L2_MBUS_FMT_Y8_1X8,
+	V4L2_MBUS_FMT_Y10_1X10,
+	V4L2_MBUS_FMT_Y12_1X12,
+	V4L2_MBUS_FMT_SGRBG8_1X8,
+	V4L2_MBUS_FMT_SRGGB8_1X8,
+	V4L2_MBUS_FMT_SBGGR8_1X8,
+	V4L2_MBUS_FMT_SGBRG8_1X8,
 	V4L2_MBUS_FMT_SGRBG10_1X10,
 	V4L2_MBUS_FMT_SRGGB10_1X10,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c
index f16d787..3c3b3c4 100644
--- a/drivers/media/video/omap3-isp/ispvideo.c
+++ b/drivers/media/video/omap3-isp/ispvideo.c
@@ -48,6 +48,18 @@
 static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
 	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
+	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
+	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
+	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
+	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
+	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
+	{ V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, },
+	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
+	{ V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
 	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

[PATCH v2 4/4] omap3isp: lane shifter support

[Michael Jones]

To use the lane shifter, set different pixel formats at each end of
the link at the CCDC input.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 drivers/media/video/omap3-isp/isp.c      |    6 +-
 drivers/media/video/omap3-isp/isp.h      |    5 +-
 drivers/media/video/omap3-isp/ispccdc.c  |   26 +++++++-
 drivers/media/video/omap3-isp/ispvideo.c |   97 +++++++++++++++++++++++------
 drivers/media/video/omap3-isp/ispvideo.h |    3 +
 5 files changed, 108 insertions(+), 29 deletions(-)

diff --git a/drivers/media/video/omap3-isp/isp.c b/drivers/media/video/omap3-isp/isp.c
index 08d90fe..68c6bcd 100644
--- a/drivers/media/video/omap3-isp/isp.c
+++ b/drivers/media/video/omap3-isp/isp.c
@@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device *isp, int idle)
  */
 void omap3isp_configure_bridge(struct isp_device *isp,
 			       enum ccdc_input_entity input,
-			       const struct isp_parallel_platform_data *pdata)
+			       const struct isp_parallel_platform_data *pdata,
+			       int shift)
 {
 	u32 ispctrl_val;
 
@@ -298,7 +299,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 	switch (input) {
 	case CCDC_INPUT_PARALLEL:
 		ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL;
-		ispctrl_val |= pdata->data_lane_shift << ISPCTRL_SHIFT_SHIFT;
 		ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT;
 		ispctrl_val |= pdata->bridge << ISPCTRL_PAR_BRIDGE_SHIFT;
 		break;
@@ -319,6 +319,8 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 		return;
 	}
 
+	ispctrl_val |= ((shift/2) << ISPCTRL_SHIFT_SHIFT) & ISPCTRL_SHIFT_MASK;
+
 	ispctrl_val &= ~ISPCTRL_SYNC_DETECT_MASK;
 	ispctrl_val |= ISPCTRL_SYNC_DETECT_VSRISE;
 
diff --git a/drivers/media/video/omap3-isp/isp.h b/drivers/media/video/omap3-isp/isp.h
index 21fa88b..3d13f8b 100644
--- a/drivers/media/video/omap3-isp/isp.h
+++ b/drivers/media/video/omap3-isp/isp.h
@@ -144,8 +144,6 @@ struct isp_reg {
  *		ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian
  */
 struct isp_parallel_platform_data {
-	unsigned int width;
-	unsigned int data_lane_shift:2;
 	unsigned int clk_pol:1;
 	unsigned int bridge:4;
 };
@@ -322,7 +320,8 @@ int omap3isp_pipeline_set_stream(struct isp_pipeline *pipe,
 				 enum isp_pipeline_stream_state state);
 void omap3isp_configure_bridge(struct isp_device *isp,
 			       enum ccdc_input_entity input,
-			       const struct isp_parallel_platform_data *pdata);
+			       const struct isp_parallel_platform_data *pdata,
+			       int shift);
 
 #define ISP_XCLK_NONE			-1
 #define ISP_XCLK_A			0
diff --git a/drivers/media/video/omap3-isp/ispccdc.c b/drivers/media/video/omap3-isp/ispccdc.c
index 23000b6..923a08f 100644
--- a/drivers/media/video/omap3-isp/ispccdc.c
+++ b/drivers/media/video/omap3-isp/ispccdc.c
@@ -1120,21 +1120,39 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	struct isp_parallel_platform_data *pdata = NULL;
 	struct v4l2_subdev *sensor;
 	struct v4l2_mbus_framefmt *format;
+	int depth_in = 0, depth_out = 0;
+	int shift;
+	const struct isp_format_info *fmt_info;
+	struct v4l2_subdev_format fmt_src;
 	struct media_pad *pad;
 	unsigned long flags;
 	u32 syn_mode;
 	u32 ccdc_pattern;
 
+	pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
+	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
-		sensor = media_entity_to_v4l2_subdev(pad->entity);
 		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
 			->bus.parallel;
 	}
 
-	omap3isp_configure_bridge(isp, ccdc->input, pdata);
+	/* set syncif.datsz */
+	fmt_src.pad = pad->index;
+	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
+		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
+		depth_in = fmt_info ? fmt_info->bpp : 0;
+	}
+
+	/* find CCDC input format */
+	fmt_info = omap3isp_video_format_info
+		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
+	depth_out = fmt_info ? fmt_info->bpp : 0;
+
+	shift = depth_in - depth_out;
+	omap3isp_configure_bridge(isp, ccdc->input, pdata, shift);
 
-	ccdc->syncif.datsz = pdata ? pdata->width : 10;
+	ccdc->syncif.datsz = depth_out;
 	ccdc_config_sync_if(ccdc, &ccdc->syncif);
 
 	/* CCDC_PAD_SINK */
diff --git a/drivers/media/video/omap3-isp/ispvideo.c b/drivers/media/video/omap3-isp/ispvideo.c
index 3c3b3c4..decc744 100644
--- a/drivers/media/video/omap3-isp/ispvideo.c
+++ b/drivers/media/video/omap3-isp/ispvideo.c
@@ -47,41 +47,59 @@
 
 static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
-	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
+	  V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_GREY, 8, },
 	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
-	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
+	  V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_Y10, 10, },
 	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
-	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
+	  V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y8_1X8,
+	  V4L2_PIX_FMT_Y12, 12, },
 	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
-	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
+	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR8, 8, },
 	{ V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
-	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, },
+	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_PIX_FMT_SGBRG8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
-	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
+	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG8, 8, },
 	{ V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
-	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, },
+	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_PIX_FMT_SRGGB8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
-	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
+	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
+	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
-	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 10, },
+	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR10, 10, },
 	{ V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG10_1X10,
-	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 10, },
+	  V4L2_MBUS_FMT_SGBRG10_1X10, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_PIX_FMT_SGBRG10, 10, },
 	{ V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_1X10,
-	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 10, },
+	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG10, 10, },
 	{ V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB10_1X10,
-	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 10, },
+	  V4L2_MBUS_FMT_SRGGB10_1X10, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_PIX_FMT_SRGGB10, 10, },
 	{ V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR10_1X10,
-	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_PIX_FMT_SBGGR12, 12, },
+	  V4L2_MBUS_FMT_SBGGR12_1X12, V4L2_MBUS_FMT_SBGGR8_1X8,
+	  V4L2_PIX_FMT_SBGGR12, 12, },
 	{ V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG10_1X10,
-	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_PIX_FMT_SGBRG12, 12, },
+	  V4L2_MBUS_FMT_SGBRG12_1X12, V4L2_MBUS_FMT_SGBRG8_1X8,
+	  V4L2_PIX_FMT_SGBRG12, 12, },
 	{ V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG10_1X10,
-	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_PIX_FMT_SGRBG12, 12, },
+	  V4L2_MBUS_FMT_SGRBG12_1X12, V4L2_MBUS_FMT_SGRBG8_1X8,
+	  V4L2_PIX_FMT_SGRBG12, 12, },
 	{ V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB10_1X10,
-	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_PIX_FMT_SRGGB12, 12, },
+	  V4L2_MBUS_FMT_SRGGB12_1X12, V4L2_MBUS_FMT_SRGGB8_1X8,
+	  V4L2_PIX_FMT_SRGGB12, 12, },
 	{ V4L2_MBUS_FMT_UYVY8_1X16, V4L2_MBUS_FMT_UYVY8_1X16,
-	  V4L2_MBUS_FMT_UYVY8_1X16, V4L2_PIX_FMT_UYVY, 16, },
+	  V4L2_MBUS_FMT_UYVY8_1X16, 0,
+	  V4L2_PIX_FMT_UYVY, 16, },
 	{ V4L2_MBUS_FMT_YUYV8_1X16, V4L2_MBUS_FMT_YUYV8_1X16,
-	  V4L2_MBUS_FMT_YUYV8_1X16, V4L2_PIX_FMT_YUYV, 16, },
+	  V4L2_MBUS_FMT_YUYV8_1X16, 0,
+	  V4L2_PIX_FMT_YUYV, 16, },
 };
 
 const struct isp_format_info *
@@ -98,6 +116,32 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode code)
 }
 
 /*
+ * Decide whether desired output pixel code can be obtained with
+ * the lane shifter by shifting the input pixel code.
+ * return 1 if the combination is possible
+ * return 0 otherwise
+ */
+static bool omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
+		enum v4l2_mbus_pixelcode out)
+{
+	const struct isp_format_info *in_info, *out_info;
+
+	if (in == out)
+		return 1;
+
+	in_info = omap3isp_video_format_info(in);
+	out_info = omap3isp_video_format_info(out);
+
+	if ((in_info->flavor == 0) || (out_info->flavor == 0))
+		return 0;
+
+	if (in_info->flavor != out_info->flavor)
+		return 0;
+
+	return (in_info->bpp - out_info->bpp <= 6);
+}
+
+/*
  * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
  * @video: ISP video instance
  * @mbus: v4l2_mbus_framefmt format (input)
@@ -247,6 +291,7 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 		return -EPIPE;
 
 	while (1) {
+		unsigned int link_has_shifter;
 		/* Retrieve the sink format */
 		pad = &subdev->entity.pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
@@ -275,6 +320,10 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 				return -ENOSPC;
 		}
 
+		/* if sink pad is on CCDC, the link has the lane shifter
+		 * in the middle of it. */
+		link_has_shifter = (subdev == &isp->isp_ccdc.subdev);
+
 		/* Retrieve the source format */
 		pad = media_entity_remote_source(pad);
 		if (pad == NULL ||
@@ -290,10 +339,18 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 			return -EPIPE;
 
 		/* Check if the two ends match */
-		if (fmt_source.format.code != fmt_sink.format.code ||
-		    fmt_source.format.width != fmt_sink.format.width ||
+		if (fmt_source.format.width != fmt_sink.format.width ||
 		    fmt_source.format.height != fmt_sink.format.height)
 			return -EPIPE;
+
+		if (link_has_shifter) {
+			if (!omap3isp_is_shiftable(fmt_source.format.code,
+						fmt_sink.format.code)) {
+				pr_debug("%s not shiftable.\n", __func__);
+				return -EPIPE;
+			}
+		} else if (fmt_source.format.code != fmt_sink.format.code)
+			return -EPIPE;
 	}
 
 	return 0;
diff --git a/drivers/media/video/omap3-isp/ispvideo.h b/drivers/media/video/omap3-isp/ispvideo.h
index 524a1ac..911bea6 100644
--- a/drivers/media/video/omap3-isp/ispvideo.h
+++ b/drivers/media/video/omap3-isp/ispvideo.h
@@ -49,6 +49,8 @@ struct v4l2_pix_format;
  *	bits. Identical to @code if the format is 10 bits wide or less.
  * @uncompressed: V4L2 media bus format code for the corresponding uncompressed
  *	format. Identical to @code if the format is not DPCM compressed.
+ * @flavor: V4L2 media bus format code for the same pixel layout but
+ *	shifted to be 8 bits per pixel. =0 if format is not shiftable.
  * @pixelformat: V4L2 pixel format FCC identifier
  * @bpp: Bits per pixel
  */
@@ -56,6 +58,7 @@ struct isp_format_info {
 	enum v4l2_mbus_pixelcode code;
 	enum v4l2_mbus_pixelcode truncated;
 	enum v4l2_mbus_pixelcode uncompressed;
+	enum v4l2_mbus_pixelcode flavor;
 	u32 pixelformat;
 	unsigned int bpp;
 };
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH v2 1/4] v4l: add V4L2_PIX_FMT_Y12 format

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Wednesday 09 March 2011 17:07:40 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/linux/videodev2.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 02da9e7..6fac463 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -288,6 +288,7 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_Y4      v4l2_fourcc('Y', '0', '4', ' ') /*  4  Greyscale     */
>  #define V4L2_PIX_FMT_Y6      v4l2_fourcc('Y', '0', '6', ' ') /*  6  Greyscale     */
>  #define V4L2_PIX_FMT_Y10     v4l2_fourcc('Y', '1', '0', ' ') /* 10  Greyscale     */
> +#define V4L2_PIX_FMT_Y12     v4l2_fourcc('Y', '1', '2', ' ') /* 12  Greyscale     */
>  #define V4L2_PIX_FMT_Y16     v4l2_fourcc('Y', '1', '6', ' ') /* 16  Greyscale     */
> 
>  /* Palette formats */

Could you please also document the format in Documentation/DocBook/v4l ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] media: add 8-bit bayer formats and Y12

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Wednesday 09 March 2011 17:07:41 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/linux/v4l2-mediabus.h |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 7054a7a..46caecd 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -47,8 +47,9 @@ enum v4l2_mbus_pixelcode {
>  	V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
>  	V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
> 
> -	/* YUV (including grey) - next is 0x2013 */
> +	/* YUV (including grey) - next is 0x2014 */
>  	V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
> +	V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
>  	V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
>  	V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
>  	V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
> @@ -67,9 +68,11 @@ enum v4l2_mbus_pixelcode {
>  	V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
>  	V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
> 
> -	/* Bayer - next is 0x3013 */
> +	/* Bayer - next is 0x3015 */
>  	V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
> +	V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
>  	V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
> +	V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
>  	V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
>  	V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
>  	V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,

Could you please also document those formats in 
Documentation/DocBook/v4l/subdev-formats.xml ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] omap3isp: ccdc: support Y10/12, 8-bit bayer fmts

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Wednesday 09 March 2011 17:07:42 Michael Jones wrote:
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/video/omap3-isp/ispccdc.c  |    6 ++++++
>  drivers/media/video/omap3-isp/ispvideo.c |   12 ++++++++++++
>  2 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
> b/drivers/media/video/omap3-isp/ispccdc.c index e4d04ce..23000b6 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -43,6 +43,12 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct
> v4l2_subdev_fh *fh,
> 
>  static const unsigned int ccdc_fmts[] = {
>  	V4L2_MBUS_FMT_Y8_1X8,
> +	V4L2_MBUS_FMT_Y10_1X10,
> +	V4L2_MBUS_FMT_Y12_1X12,
> +	V4L2_MBUS_FMT_SGRBG8_1X8,
> +	V4L2_MBUS_FMT_SRGGB8_1X8,
> +	V4L2_MBUS_FMT_SBGGR8_1X8,
> +	V4L2_MBUS_FMT_SGBRG8_1X8,
>  	V4L2_MBUS_FMT_SGRBG10_1X10,
>  	V4L2_MBUS_FMT_SRGGB10_1X10,
>  	V4L2_MBUS_FMT_SBGGR10_1X10,
> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
> b/drivers/media/video/omap3-isp/ispvideo.c index f16d787..3c3b3c4 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.c
> +++ b/drivers/media/video/omap3-isp/ispvideo.c
> @@ -48,6 +48,18 @@
>  static struct isp_format_info formats[] = {
>  	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
>  	  V4L2_MBUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 8, },
> +	{ V4L2_MBUS_FMT_Y10_1X10, V4L2_MBUS_FMT_Y10_1X10,
> +	  V4L2_MBUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10, 10, },
> +	{ V4L2_MBUS_FMT_Y12_1X12, V4L2_MBUS_FMT_Y10_1X10,
> +	  V4L2_MBUS_FMT_Y12_1X12, V4L2_PIX_FMT_Y12, 12, },
> +	{ V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_MBUS_FMT_SBGGR8_1X8,
> +	  V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_PIX_FMT_SBGGR8, 8, },
> +	{ V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_MBUS_FMT_SGBRG8_1X8,
> +	  V4L2_MBUS_FMT_SGBRG8_1X8, V4L2_PIX_FMT_SGBRG8, 8, },
> +	{ V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_MBUS_FMT_SGRBG8_1X8,
> +	  V4L2_MBUS_FMT_SGRBG8_1X8, V4L2_PIX_FMT_SGRBG8, 8, },
> +	{ V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
> +	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_PIX_FMT_SRGGB8, 8, },
>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
>  	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>  	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] omap3isp: lane shifter support

[Laurent Pinchart]

Hi Michael,

Thanks for the patch.

On Wednesday 09 March 2011 17:07:43 Michael Jones wrote:
> To use the lane shifter, set different pixel formats at each end of
> the link at the CCDC input.
> 
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>

[snip]

> diff --git a/drivers/media/video/omap3-isp/isp.h
> b/drivers/media/video/omap3-isp/isp.h index 21fa88b..3d13f8b 100644
> --- a/drivers/media/video/omap3-isp/isp.h
> +++ b/drivers/media/video/omap3-isp/isp.h
> @@ -144,8 +144,6 @@ struct isp_reg {
>   *		ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian
>   */
>  struct isp_parallel_platform_data {
> -	unsigned int width;
> -	unsigned int data_lane_shift:2;

I'm afraid you can't remove the data_lane_shift field completely. Board can 
wire a 8 bits sensor to DATA[9:2] :-/ That needs to be taken into account as 
well when computing the total shift value.

Hardware configuration can be done by adding the requested shift value to 
data_lane_shift for parallel sensors in omap3isp_configure_bridge(), but we 
also need to take it into account when validating the pipeline.

I'm not aware of any board requiring data_lane_shift at the moment though, so 
we could just drop it now and add it back later when needed. This will avoid 
making this patch more complex.

>  	unsigned int clk_pol:1;
>  	unsigned int bridge:4;
>  };

[snip]

> diff --git a/drivers/media/video/omap3-isp/ispccdc.c
> b/drivers/media/video/omap3-isp/ispccdc.c index 23000b6..923a08f 100644
> --- a/drivers/media/video/omap3-isp/ispccdc.c
> +++ b/drivers/media/video/omap3-isp/ispccdc.c
> @@ -1120,21 +1120,39 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) struct isp_parallel_platform_data *pdata = NULL;
>  	struct v4l2_subdev *sensor;
>  	struct v4l2_mbus_framefmt *format;
> +	int depth_in = 0, depth_out = 0;

Linux discourages the declaration of multiple variables on a single line. 
Could you split this ? The depths should also be unsigned int's (as well as 
the shift value below).

> +	int shift;
> +	const struct isp_format_info *fmt_info;
> +	struct v4l2_subdev_format fmt_src;
>  	struct media_pad *pad;
>  	unsigned long flags;
>  	u32 syn_mode;
>  	u32 ccdc_pattern;

Could you keep variable declaration lines (mostly) sorted by line length ?

> 
> +	pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> +	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		pad = media_entity_remote_source(&ccdc->pads[CCDC_PAD_SINK]);
> -		sensor = media_entity_to_v4l2_subdev(pad->entity);
>  		pdata = &((struct isp_v4l2_subdevs_group *)sensor->host_priv)
>  			->bus.parallel;
>  	}

You can remove the curly braces as the 'if' body now contains a single 
statement.

> 
> -	omap3isp_configure_bridge(isp, ccdc->input, pdata);
> +	/* set syncif.datsz */

The following lines don't set syncif.datsz, so the comment is a bit 
misleading. I think you can replace it with

	/* Compute the lane shifter shift value to configure the bridge. */

or something similar, and remove the "find CCDC input format" comment below.

> +	fmt_src.pad = pad->index;
> +	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
> +		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
> +		depth_in = fmt_info ? fmt_info->bpp : 0;

omap3isp_video_format_info() won't return NULL, as it supports all formats 
supported by the CCDC. You can thus skip the NULL check.

> +	}
> +
> +	/* find CCDC input format */
> +	fmt_info = omap3isp_video_format_info
> +		(isp->isp_ccdc.formats[CCDC_PAD_SINK].code);
> +	depth_out = fmt_info ? fmt_info->bpp : 0;

And here too.

> +
> +	shift = depth_in - depth_out;
> +	omap3isp_configure_bridge(isp, ccdc->input, pdata, shift);
> 
> -	ccdc->syncif.datsz = pdata ? pdata->width : 10;
> +	ccdc->syncif.datsz = depth_out;
>  	ccdc_config_sync_if(ccdc, &ccdc->syncif);
> 
>  	/* CCDC_PAD_SINK */
> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
> b/drivers/media/video/omap3-isp/ispvideo.c index 3c3b3c4..decc744 100644
> --- a/drivers/media/video/omap3-isp/ispvideo.c
> +++ b/drivers/media/video/omap3-isp/ispvideo.c
> @@ -47,41 +47,59 @@
> 
>  static struct isp_format_info formats[] = {

[snip]

>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
> -	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
> +	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,

Should this be

 V4L2_MBUS_FMT_SGRBG10_1X10, 0,

instead ? DPCM formats are not shiftable. It won't make any difference in 
practice, as the format is already 8-bit wide, but you state in the flavor 
field documentation that "=0 if format is not shiftable".

> +	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },

[snip]

> @@ -98,6 +116,32 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode
> code) }
> 
>  /*
> + * Decide whether desired output pixel code can be obtained with
> + * the lane shifter by shifting the input pixel code.
> + * return 1 if the combination is possible
> + * return 0 otherwise

Return true if ... or false otherwise.

> + */
> +static bool omap3isp_is_shiftable(enum v4l2_mbus_pixelcode in,
> +		enum v4l2_mbus_pixelcode out)
> +{
> +	const struct isp_format_info *in_info, *out_info;
> +
> +	if (in == out)
> +		return 1;

return true;

(and false below).

> +
> +	in_info = omap3isp_video_format_info(in);
> +	out_info = omap3isp_video_format_info(out);
> +
> +	if ((in_info->flavor == 0) || (out_info->flavor == 0))
> +		return 0;
> +
> +	if (in_info->flavor != out_info->flavor)
> +		return 0;
> +
> +	return (in_info->bpp - out_info->bpp <= 6);

No need for brackets.

> +}
> +
> +/*
>   * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
>   * @video: ISP video instance
>   * @mbus: v4l2_mbus_framefmt format (input)
> @@ -247,6 +291,7 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -EPIPE;
> 
>  	while (1) {
> +		unsigned int link_has_shifter;
>  		/* Retrieve the sink format */
>  		pad = &subdev->entity.pads[0];
>  		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> @@ -275,6 +320,10 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -ENOSPC;
>  		}
> 
> +		/* if sink pad is on CCDC, the link has the lane shifter
> +		 * in the middle of it. */

As you'll need to resubmit the patch, please capitalize the 'If'.

> +		link_has_shifter = (subdev == &isp->isp_ccdc.subdev);

And there's no need for brackets.

> +
>  		/* Retrieve the source format */
>  		pad = media_entity_remote_source(pad);
>  		if (pad == NULL ||
> @@ -290,10 +339,18 @@ static int isp_video_validate_pipeline(struct
> isp_pipeline *pipe) return -EPIPE;
> 
>  		/* Check if the two ends match */
> -		if (fmt_source.format.code != fmt_sink.format.code ||
> -		    fmt_source.format.width != fmt_sink.format.width ||
> +		if (fmt_source.format.width != fmt_sink.format.width ||
>  		    fmt_source.format.height != fmt_sink.format.height)
>  			return -EPIPE;
> +
> +		if (link_has_shifter) {
> +			if (!omap3isp_is_shiftable(fmt_source.format.code,
> +						fmt_sink.format.code)) {
> +				pr_debug("%s not shiftable.\n", __func__);

Do we need the pr_debug call ?

> +				return -EPIPE;
> +			}
> +		} else if (fmt_source.format.code != fmt_sink.format.code)
> +			return -EPIPE;
>  	}
> 
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] omap3isp: lane shifter support

[Michael Jones]

Hi Laurent,

Thanks for the review.  Most of your suggestions didn't warrant
discussion and I will incorporate those changes.  The others are below.

On 03/10/2011 01:13 AM, Laurent Pinchart wrote:
> Hi Michael,
> 
> Thanks for the patch.
> 
> On Wednesday 09 March 2011 17:07:43 Michael Jones wrote:
>> To use the lane shifter, set different pixel formats at each end of
>> the link at the CCDC input.
>>
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> 
> [snip]
> 
>> diff --git a/drivers/media/video/omap3-isp/isp.h
>> b/drivers/media/video/omap3-isp/isp.h index 21fa88b..3d13f8b 100644
>> --- a/drivers/media/video/omap3-isp/isp.h
>> +++ b/drivers/media/video/omap3-isp/isp.h
>> @@ -144,8 +144,6 @@ struct isp_reg {
>>   *		ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian
>>   */
>>  struct isp_parallel_platform_data {
>> -	unsigned int width;
>> -	unsigned int data_lane_shift:2;
> 
> I'm afraid you can't remove the data_lane_shift field completely. Board can 
> wire a 8 bits sensor to DATA[9:2] :-/ That needs to be taken into account as 
> well when computing the total shift value.
> 
> Hardware configuration can be done by adding the requested shift value to 
> data_lane_shift for parallel sensors in omap3isp_configure_bridge(), but we 
> also need to take it into account when validating the pipeline.
> 
> I'm not aware of any board requiring data_lane_shift at the moment though, so 
> we could just drop it now and add it back later when needed. This will avoid 
> making this patch more complex.
> 

I'm in favor of leaving it as is for now and adding it back when needed.
 It's a good point, and I do think it should be supported in the long
run, but it'd be nice to not have to worry about it for this patch.  Is
it OK with you to leave 'data_lane_shift' out for now?

>>  	unsigned int clk_pol:1;
>>  	unsigned int bridge:4;
>>  };
> 

[snip]

>>  	/* CCDC_PAD_SINK */
>> diff --git a/drivers/media/video/omap3-isp/ispvideo.c
>> b/drivers/media/video/omap3-isp/ispvideo.c index 3c3b3c4..decc744 100644
>> --- a/drivers/media/video/omap3-isp/ispvideo.c
>> +++ b/drivers/media/video/omap3-isp/ispvideo.c
>> @@ -47,41 +47,59 @@
>>
>>  static struct isp_format_info formats[] = {
> 
> [snip]
> 
>>  	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
>> -	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
>> +	  V4L2_MBUS_FMT_SGRBG10_1X10, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
> 
> Should this be
> 
>  V4L2_MBUS_FMT_SGRBG10_1X10, 0,
> 
> instead ? DPCM formats are not shiftable. It won't make any difference in 
> practice, as the format is already 8-bit wide, but you state in the flavor 
> field documentation that "=0 if format is not shiftable".

I went back and forth on that since this format is already 8 bits wide
and no non-8-bit format will declare this as its flavor, since it really
is non-shiftable.  Although- now that I explicitly say '=0 if format is
not shiftable', I suppose it does make sense to have flavor=0 here.  I
will change that.

> 
>> +	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
> 
> [snip]
> 

[snip]

>> +		if (link_has_shifter) {
>> +			if (!omap3isp_is_shiftable(fmt_source.format.code,
>> +						fmt_sink.format.code)) {
>> +				pr_debug("%s not shiftable.\n", __func__);
> 
> Do we need the pr_debug call ?

No, that was an oversight.

> 
>> +				return -EPIPE;
>> +			}
>> +		} else if (fmt_source.format.code != fmt_sink.format.code)
>> +			return -EPIPE;
>>  	}
>>
>>  	return 0;
> 

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH v2 4/4] omap3isp: lane shifter support

[Laurent Pinchart]

Hi Michael,

On Thursday 10 March 2011 11:10:24 Michael Jones wrote:
> Hi Laurent,
> 
> Thanks for the review.  Most of your suggestions didn't warrant
> discussion and I will incorporate those changes.  The others are below.
> 
> On 03/10/2011 01:13 AM, Laurent Pinchart wrote:
> > On Wednesday 09 March 2011 17:07:43 Michael Jones wrote:
> >> To use the lane shifter, set different pixel formats at each end of
> >> the link at the CCDC input.
> >> 
> >> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/video/omap3-isp/isp.h
> >> b/drivers/media/video/omap3-isp/isp.h index 21fa88b..3d13f8b 100644
> >> --- a/drivers/media/video/omap3-isp/isp.h
> >> +++ b/drivers/media/video/omap3-isp/isp.h
> >> @@ -144,8 +144,6 @@ struct isp_reg {
> >> 
> >>   *		ISPCTRL_PAR_BRIDGE_BENDIAN - Big endian
> >>   */
> >>  
> >>  struct isp_parallel_platform_data {
> >> 
> >> -	unsigned int width;
> >> -	unsigned int data_lane_shift:2;
> > 
> > I'm afraid you can't remove the data_lane_shift field completely. Board
> > can wire a 8 bits sensor to DATA[9:2] :-/ That needs to be taken into
> > account as well when computing the total shift value.
> > 
> > Hardware configuration can be done by adding the requested shift value to
> > data_lane_shift for parallel sensors in omap3isp_configure_bridge(), but
> > we also need to take it into account when validating the pipeline.
> > 
> > I'm not aware of any board requiring data_lane_shift at the moment
> > though, so we could just drop it now and add it back later when needed.
> > This will avoid making this patch more complex.
> 
> I'm in favor of leaving it as is for now and adding it back when needed.
>  It's a good point, and I do think it should be supported in the long
> run, but it'd be nice to not have to worry about it for this patch.  Is
> it OK with you to leave 'data_lane_shift' out for now?

I've had a closer look at the boards I have here, and it turns out one of them 
connects a 10-bit sensor to DATA[11:2] :-/ data_lane_shift is thus needed for 
it.

I'm fine with leaving data_lane_shift out of this patch, but can you submit a 
second patch to add it back ? I'd rather avoid applying a patch that breaks 
one of my boards and then have to fix it myself :-)

> >>  	unsigned int clk_pol:1;
> >>  	unsigned int bridge:4;
> >>  
> >>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] omap3isp: lane shifter support

[Michael Jones]

Hi Laurent,

On 03/10/2011 11:21 AM, Laurent Pinchart wrote:
> Hi Michael,
> 
[snip]
> I've had a closer look at the boards I have here, and it turns out one of them 
> connects a 10-bit sensor to DATA[11:2] :-/ data_lane_shift is thus needed for 
> it.
> 
> I'm fine with leaving data_lane_shift out of this patch, but can you submit a 
> second patch to add it back ? I'd rather avoid applying a patch that breaks 
> one of my boards and then have to fix it myself :-)

OK, but in that case I'd rather incorporate it into this last patch than
introduce a new patch for it.  I don't think it will be very complex and
they logically belong together.  I had just been hoping to avoid
implementing it altogether.

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH v2 4/4] omap3isp: lane shifter support

[Laurent Pinchart]

Hi Michael,

On Thursday 10 March 2011 16:30:48 Michael Jones wrote:
> On 03/10/2011 11:21 AM, Laurent Pinchart wrote:
> > Hi Michael,
> 
> [snip]
> 
> > I've had a closer look at the boards I have here, and it turns out one of
> > them connects a 10-bit sensor to DATA[11:2] :-/ data_lane_shift is thus
> > needed for it.
> > 
> > I'm fine with leaving data_lane_shift out of this patch, but can you
> > submit a second patch to add it back ? I'd rather avoid applying a patch
> > that breaks one of my boards and then have to fix it myself :-)
> 
> OK, but in that case I'd rather incorporate it into this last patch than
> introduce a new patch for it.  I don't think it will be very complex and
> they logically belong together.  I had just been hoping to avoid
> implementing it altogether.

As you wish. You can also submit two patches and then squash them together if 
it makes it simpler to develop/review the code.

-- 
Regards,

Laurent Pinchart