[PATCH 0/4] Miscellaneous OMAP3 ISP fixes and enhancements

[PATCH 0/4] Miscellaneous OMAP3 ISP fixes and enhancements

[Laurent Pinchart]

Hi everybody,

Here are 4 miscellaneous patches for the OMAP3 ISP driver.

Laurent Pinchart (4):
  omap3isp: preview: Rename last occurences of *_rgb_to_ycbcr to *_csc
  omap3isp: preview: Add support for greyscale input
  omap3isp: ccdc: Add crop support on output formatter source pad
  omap3isp: Mark probe and cleanup functions with __devinit and
    __devexit

 drivers/media/video/omap3isp/isp.c        |    6 +-
 drivers/media/video/omap3isp/ispccdc.c    |  147 ++++++++++++++++++++++++++---
 drivers/media/video/omap3isp/ispccdc.h    |    2 +
 drivers/media/video/omap3isp/isppreview.c |   58 +++++++-----
 4 files changed, 172 insertions(+), 41 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/4] omap3isp: preview: Rename last occurences of *_rgb_to_ycbcr to *_csc

[Laurent Pinchart]

For consistency reasons, rename the last remaining occurences of
rgb_to_ycbcr to csc (color space conversion).

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 5bdf9e7..f839cf8 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -608,12 +608,12 @@ preview_config_rgb_blending(struct isp_prev_device *prev, const void *rgb2rgb)
 }
 
 /*
- * Configures the RGB-YCbYCr conversion matrix
+ * Configures the color space conversion (RGB toYCbYCr) matrix
  * @prev_csc: Structure containing the RGB to YCbYCr matrix and the
  *            YCbCr offset.
  */
 static void
-preview_config_rgb_to_ycbcr(struct isp_prev_device *prev, const void *prev_csc)
+preview_config_csc(struct isp_prev_device *prev, const void *prev_csc)
 {
 	struct isp_device *isp = to_isp_device(prev);
 	const struct omap3isp_prev_csc *csc = prev_csc;
@@ -860,7 +860,7 @@ static const struct preview_update update_attrs[] = {
 		FIELD_SIZEOF(struct prev_params, rgb2rgb),
 		offsetof(struct omap3isp_prev_update_config, rgb2rgb),
 	}, /* OMAP3ISP_PREV_COLOR_CONV */ {
-		preview_config_rgb_to_ycbcr,
+		preview_config_csc,
 		NULL,
 		offsetof(struct prev_params, csc),
 		FIELD_SIZEOF(struct prev_params, csc),
-- 
1.7.3.4

[PATCH 2/4] omap3isp: preview: Add support for greyscale input

[Laurent Pinchart]

Configure CFA interpolation automatically based on the input format.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |   52 ++++++++++++++++------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index f839cf8..420b131 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -441,23 +441,6 @@ preview_enable_dcor(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_cfa - Enable/Disable the CFA Interpolation.
- * @enable: 1 - Enables the CFA.
- */
-static void
-preview_enable_cfa(struct isp_prev_device *prev, u8 enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_CFAEN);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_CFAEN);
-}
-
-/*
  * preview_enable_gammabypass - Enables/Disables the GammaByPass
  * @enable: 1 - Bypasses Gamma - 10bit input is cropped to 8MSB.
  *          0 - Goes through Gamma Correction. input and output is 10bit.
@@ -831,7 +814,7 @@ static const struct preview_update update_attrs[] = {
 		offsetof(struct omap3isp_prev_update_config, hmed),
 	}, /* OMAP3ISP_PREV_CFA */ {
 		preview_config_cfa,
-		preview_enable_cfa,
+		NULL,
 		offsetof(struct prev_params, cfa),
 		FIELD_SIZEOF(struct prev_params, cfa),
 		offsetof(struct omap3isp_prev_update_config, cfa),
@@ -1078,6 +1061,27 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 }
 
 /*
+ * preview_config_input_format - Configure the input format
+ * @prev: The preview engine
+ * @format: Format on the preview engine sink pad
+ *
+ * Enable CFA interpolation for Bayer formats and disable it for greyscale
+ * formats.
+ */
+static void preview_config_input_format(struct isp_prev_device *prev,
+					const struct v4l2_mbus_framefmt *format)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (format->code != V4L2_MBUS_FMT_Y10_1X10)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_CFAEN);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_CFAEN);
+}
+
+/*
  * preview_config_input_size - Configure the input frame size
  *
  * The preview engine crops several rows and columns internally depending on
@@ -1090,6 +1094,7 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
  */
 static void preview_config_input_size(struct isp_prev_device *prev, u32 active)
 {
+	const struct v4l2_mbus_framefmt *format = &prev->formats[PREV_PAD_SINK];
 	struct isp_device *isp = to_isp_device(prev);
 	unsigned int sph = prev->crop.left;
 	unsigned int eph = prev->crop.left + prev->crop.width - 1;
@@ -1097,15 +1102,16 @@ static void preview_config_input_size(struct isp_prev_device *prev, u32 active)
 	unsigned int elv = prev->crop.top + prev->crop.height - 1;
 	u32 features;
 
-	features = (prev->params.params[0].features & active)
-		 | (prev->params.params[1].features & ~active);
-
-	if (features & OMAP3ISP_PREV_CFA) {
+	if (format->code == V4L2_MBUS_FMT_Y10_1X10) {
 		sph -= 2;
 		eph += 2;
 		slv -= 2;
 		elv += 2;
 	}
+
+	features = (prev->params.params[0].features & active)
+		 | (prev->params.params[1].features & ~active);
+
 	if (features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
 		sph -= 2;
 		eph += 2;
@@ -1436,6 +1442,7 @@ static void preview_configure(struct isp_prev_device *prev)
 
 	preview_adjust_bandwidth(prev);
 
+	preview_config_input_format(prev, format);
 	preview_config_input_size(prev, active);
 
 	if (prev->input == PREVIEW_INPUT_CCDC)
@@ -1723,6 +1730,7 @@ __preview_get_crop(struct isp_prev_device *prev, struct v4l2_subdev_fh *fh,
 
 /* previewer format descriptions */
 static const unsigned int preview_input_fmts[] = {
+	V4L2_MBUS_FMT_Y10_1X10,
 	V4L2_MBUS_FMT_SGRBG10_1X10,
 	V4L2_MBUS_FMT_SRGGB10_1X10,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
-- 
1.7.3.4

Re: [PATCH 2/4] omap3isp: preview: Add support for greyscale input

[Sakari Ailus]

Hi Laurent,

Thanks for the patch!

On Mon, Apr 23, 2012 at 01:29:53PM +0200, Laurent Pinchart wrote:
> Configure CFA interpolation automatically based on the input format.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |   52 ++++++++++++++++------------
>  1 files changed, 30 insertions(+), 22 deletions(-)

Acked-by: <sakari.ailus@iki.fi>

(Same for 1/4, too!)

It looks like the OMAP 3 ISP will be functionally complete soon! 8-)

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

[PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/ispccdc.c |  147 +++++++++++++++++++++++++++++---
 drivers/media/video/omap3isp/ispccdc.h |    2 +
 2 files changed, 136 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 8d8d6f3..f7851fa 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -38,6 +38,9 @@
 #include "ispreg.h"
 #include "ispccdc.h"
 
+#define CCDC_MIN_WIDTH		32
+#define CCDC_MIN_HEIGHT		32
+
 static struct v4l2_mbus_framefmt *
 __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		  unsigned int pad, enum v4l2_subdev_format_whence which);
@@ -1118,6 +1121,7 @@ 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;
+	const struct v4l2_rect *crop;
 	const struct isp_format_info *fmt_info;
 	struct v4l2_subdev_format fmt_src;
 	unsigned int depth_out;
@@ -1211,14 +1215,14 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
 
 	/* CCDC_PAD_SOURCE_OF */
-	format = &ccdc->formats[CCDC_PAD_SOURCE_OF];
+	crop = &ccdc->crop;
 
-	isp_reg_writel(isp, (0 << ISPCCDC_HORZ_INFO_SPH_SHIFT) |
-		       ((format->width - 1) << ISPCCDC_HORZ_INFO_NPH_SHIFT),
+	isp_reg_writel(isp, (crop->left << ISPCCDC_HORZ_INFO_SPH_SHIFT) |
+		       ((crop->width - 1) << ISPCCDC_HORZ_INFO_NPH_SHIFT),
 		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_HORZ_INFO);
-	isp_reg_writel(isp, 0 << ISPCCDC_VERT_START_SLV0_SHIFT,
+	isp_reg_writel(isp, crop->top << ISPCCDC_VERT_START_SLV0_SHIFT,
 		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
-	isp_reg_writel(isp, (format->height - 1)
+	isp_reg_writel(isp, (crop->height - 1)
 			<< ISPCCDC_VERT_LINES_NLV_SHIFT,
 		       OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_LINES);
 
@@ -1793,6 +1797,16 @@ __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		return &ccdc->formats[pad];
 }
 
+static struct v4l2_rect *
+__ccdc_get_crop(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
+		enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_crop(fh, CCDC_PAD_SOURCE_OF);
+	else
+		return &ccdc->crop;
+}
+
 /*
  * ccdc_try_format - Try video format on a pad
  * @ccdc: ISP CCDC device
@@ -1809,6 +1823,7 @@ ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 	const struct isp_format_info *info;
 	unsigned int width = fmt->width;
 	unsigned int height = fmt->height;
+	struct v4l2_rect *crop;
 	unsigned int i;
 
 	switch (pad) {
@@ -1834,14 +1849,10 @@ ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 		format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SINK, which);
 		memcpy(fmt, format, sizeof(*fmt));
 
-		/* The data formatter truncates the number of horizontal output
-		 * pixels to a multiple of 16. To avoid clipping data, allow
-		 * callers to request an output size bigger than the input size
-		 * up to the nearest multiple of 16.
-		 */
-		fmt->width = clamp_t(u32, width, 32, fmt->width + 15);
-		fmt->width &= ~15;
-		fmt->height = clamp_t(u32, height, 32, fmt->height);
+		/* Hardcode the output size to the crop rectangle size. */
+		crop = __ccdc_get_crop(ccdc, fh, which);
+		fmt->width = crop->width;
+		fmt->height = crop->height;
 		break;
 
 	case CCDC_PAD_SOURCE_VP:
@@ -1869,6 +1880,49 @@ ccdc_try_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 }
 
 /*
+ * ccdc_try_crop - Validate a crop rectangle
+ * @ccdc: ISP CCDC device
+ * @sink: format on the sink pad
+ * @crop: crop rectangle to be validated
+ */
+static void ccdc_try_crop(struct isp_ccdc_device *ccdc,
+			  const struct v4l2_mbus_framefmt *sink,
+			  struct v4l2_rect *crop)
+{
+	const struct isp_format_info *info;
+	unsigned int max_width;
+
+	/* For Bayer formats, restrict left/top and width/height to even values
+	 * to keep the Bayer pattern.
+	 */
+	info = omap3isp_video_format_info(sink->code);
+	if (info->flavor != V4L2_MBUS_FMT_Y8_1X8) {
+		crop->left &= ~1;
+		crop->top &= ~1;
+	}
+
+	crop->left = clamp_t(u32, crop->left, 0, sink->width - CCDC_MIN_WIDTH);
+	crop->top = clamp_t(u32, crop->top, 0, sink->height - CCDC_MIN_HEIGHT);
+
+	/* The data formatter truncates the number of horizontal output pixels
+	 * to a multiple of 16. To avoid clipping data, allow callers to request
+	 * an output size bigger than the input size up to the nearest multiple
+	 * of 16.
+	 */
+	max_width = (sink->width - crop->left + 15) & ~15;
+	crop->width = clamp_t(u32, crop->width, CCDC_MIN_WIDTH, max_width)
+		    & ~15;
+	crop->height = clamp_t(u32, crop->height, CCDC_MIN_HEIGHT,
+			       sink->height - crop->top);
+
+	/* Odd width/height values don't make sense for Bayer formats. */
+	if (info->flavor != V4L2_MBUS_FMT_Y8_1X8) {
+		crop->width &= ~1;
+		crop->height &= ~1;
+	}
+}
+
+/*
  * ccdc_enum_mbus_code - Handle pixel format enumeration
  * @sd     : pointer to v4l2 subdev structure
  * @fh : V4L2 subdev file handle
@@ -1940,6 +1994,60 @@ static int ccdc_enum_frame_size(struct v4l2_subdev *sd,
 }
 
 /*
+ * ccdc_get_crop - Retrieve the crop rectangle on a pad
+ * @sd: ISP CCDC V4L2 subdevice
+ * @fh: V4L2 subdev file handle
+ * @crop: crop rectangle
+ *
+ * Return 0 on success or a negative error code otherwise.
+ */
+static int ccdc_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			    struct v4l2_subdev_crop *crop)
+{
+	struct isp_ccdc_device *ccdc = v4l2_get_subdevdata(sd);
+
+	/* Cropping is only supported on the output formatter source pad. */
+	if (crop->pad != CCDC_PAD_SOURCE_OF)
+		return -EINVAL;
+
+	crop->rect = *__ccdc_get_crop(ccdc, fh, crop->which);
+	return 0;
+}
+
+/*
+ * ccdc_set_crop - Retrieve the crop rectangle on a pad
+ * @sd: ISP CCDC V4L2 subdevice
+ * @fh: V4L2 subdev file handle
+ * @crop: crop rectangle
+ *
+ * Return 0 on success or a negative error code otherwise.
+ */
+static int ccdc_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			    struct v4l2_subdev_crop *crop)
+{
+	struct isp_ccdc_device *ccdc = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *format;
+
+	/* Cropping is only supported on the output formatter source pad. */
+	if (crop->pad != CCDC_PAD_SOURCE_OF)
+		return -EINVAL;
+
+	/* The crop rectangle can't be changed while streaming. */
+	if (ccdc->state != ISP_PIPELINE_STREAM_STOPPED)
+		return -EBUSY;
+
+	format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SINK, crop->which);
+	ccdc_try_crop(ccdc, format, &crop->rect);
+	*__ccdc_get_crop(ccdc, fh, crop->which) = crop->rect;
+
+	/* Update the source format. */
+	format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SOURCE_OF, crop->which);
+	ccdc_try_format(ccdc, fh, CCDC_PAD_SOURCE_OF, format, crop->which);
+
+	return 0;
+}
+
+/*
  * ccdc_get_format - Retrieve the video format on a pad
  * @sd : ISP CCDC V4L2 subdevice
  * @fh : V4L2 subdev file handle
@@ -1976,6 +2084,7 @@ static int ccdc_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 {
 	struct isp_ccdc_device *ccdc = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
 
 	format = __ccdc_get_format(ccdc, fh, fmt->pad, fmt->which);
 	if (format == NULL)
@@ -1986,6 +2095,16 @@ static int ccdc_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 
 	/* Propagate the format from sink to source */
 	if (fmt->pad == CCDC_PAD_SINK) {
+		/* Reset the crop rectangle. */
+		crop = __ccdc_get_crop(ccdc, fh, fmt->which);
+		crop->left = 0;
+		crop->top = 0;
+		crop->width = fmt->format.width;
+		crop->height = fmt->format.height;
+
+		ccdc_try_crop(ccdc, &fmt->format, crop);
+
+		/* Update the source formats. */
 		format = __ccdc_get_format(ccdc, fh, CCDC_PAD_SOURCE_OF,
 					   fmt->which);
 		*format = fmt->format;
@@ -2044,6 +2163,8 @@ static const struct v4l2_subdev_pad_ops ccdc_v4l2_pad_ops = {
 	.enum_frame_size = ccdc_enum_frame_size,
 	.get_fmt = ccdc_get_format,
 	.set_fmt = ccdc_set_format,
+	.get_crop = ccdc_get_crop,
+	.set_crop = ccdc_set_crop,
 };
 
 /* V4L2 subdev operations */
diff --git a/drivers/media/video/omap3isp/ispccdc.h b/drivers/media/video/omap3isp/ispccdc.h
index 6d0264b..966bbf8 100644
--- a/drivers/media/video/omap3isp/ispccdc.h
+++ b/drivers/media/video/omap3isp/ispccdc.h
@@ -147,6 +147,7 @@ struct ispccdc_lsc {
  * @subdev: V4L2 subdevice
  * @pads: Sink and source media entity pads
  * @formats: Active video formats
+ * @crop: Active crop rectangle on the OF source pad
  * @input: Active input
  * @output: Active outputs
  * @video_out: Output video node
@@ -173,6 +174,7 @@ struct isp_ccdc_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[CCDC_PADS_NUM];
 	struct v4l2_mbus_framefmt formats[CCDC_PADS_NUM];
+	struct v4l2_rect crop;
 
 	enum ccdc_input_entity input;
 	unsigned int output;
-- 
1.7.3.4

Re: [PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Sakari Ailus]

Hi Laurent,

The patch looks good as such on the first glance, but I have another 
question: why are you not using the selections API instead? It's in 
Mauro's tree already. Also, the old S_CROP IOCTL only has been defined 
for sink pads, not source.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Laurent Pinchart]

Hi Sakari,

On Tuesday 24 April 2012 01:23:06 Sakari Ailus wrote:
> Hi Laurent,
> 
> The patch looks good as such on the first glance, but I have another
> question: why are you not using the selections API instead? It's in
> Mauro's tree already.

You're totally right, we need to convert the selection API. The reason why 
I've implemented crop support at the CCDC output was simply that I needed it 
for a project and didn't have time to implement the selection API. As the code 
works, I considered it would be good to have it upstream until we switch to 
the selection API.

> Also, the old S_CROP IOCTL only has been defined for sink pads, not source.

We're already using crop on source pads on sensors ;-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Sakari Ailus]

Hi Laurent,

On Tue, Apr 24, 2012 at 11:08:12AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 24 April 2012 01:23:06 Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > The patch looks good as such on the first glance, but I have another
> > question: why are you not using the selections API instead? It's in
> > Mauro's tree already.
> 
> You're totally right, we need to convert the selection API. The reason why 
> I've implemented crop support at the CCDC output was simply that I needed it 
> for a project and didn't have time to implement the selection API. As the code 
> works, I considered it would be good to have it upstream until we switch to 
> the selection API.

"Until we switch to the selection API"? The subdev selection API is in
Mauro's tree already so I see no reason not to use it. Implementing new
functionality in a driver using API we've just marked obsolete is... not
pretty.

The compatibility code for the old crop ioctls exist, too, so you get
exactly the same functionality as well.

> > Also, the old S_CROP IOCTL only has been defined for sink pads, not source.
> 
> We're already using crop on source pads on sensors ;-)

Is that supposed to work? At the very least least it does not follow the
spec.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Laurent Pinchart]

Hi Sakari,

On Tuesday 24 April 2012 18:14:20 Sakari Ailus wrote:
> On Tue, Apr 24, 2012 at 11:08:12AM +0200, Laurent Pinchart wrote:
> > On Tuesday 24 April 2012 01:23:06 Sakari Ailus wrote:
> > > Hi Laurent,
> > > 
> > > The patch looks good as such on the first glance, but I have another
> > > question: why are you not using the selections API instead? It's in
> > > Mauro's tree already.
> > 
> > You're totally right, we need to convert the selection API. The reason why
> > I've implemented crop support at the CCDC output was simply that I needed
> > it for a project and didn't have time to implement the selection API. As
> > the code works, I considered it would be good to have it upstream until
> > we switch to the selection API.
> 
> "Until we switch to the selection API"? The subdev selection API is in
> Mauro's tree already so I see no reason not to use it. Implementing new
> functionality in a driver using API we've just marked obsolete is... not
> pretty.

You're of course totally right. I've pushed back on enough attemps similar to 
this one to know that I will have to give up here and implement selection 
support :-)

> The compatibility code for the old crop ioctls exist, too, so you get
> exactly the same functionality as well.
> 
> > > Also, the old S_CROP IOCTL only has been defined for sink pads, not
> > > source.
> > 
> > We're already using crop on source pads on sensors ;-)
> 
> Is that supposed to work? At the very least least it does not follow the
> spec.

It doesn't follow the spec. But it works :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] omap3isp: ccdc: Add crop support on output formatter source pad

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tuesday 24 April 2012 18:14:20 Sakari Ailus wrote:
>> On Tue, Apr 24, 2012 at 11:08:12AM +0200, Laurent Pinchart wrote:
>>> On Tuesday 24 April 2012 01:23:06 Sakari Ailus wrote:
>>>> Hi Laurent,
>>>>
>>>> The patch looks good as such on the first glance, but I have another
>>>> question: why are you not using the selections API instead? It's in
>>>> Mauro's tree already.
>>>
>>> You're totally right, we need to convert the selection API. The reason why
>>> I've implemented crop support at the CCDC output was simply that I needed
>>> it for a project and didn't have time to implement the selection API. As
>>> the code works, I considered it would be good to have it upstream until
>>> we switch to the selection API.
>>
>> "Until we switch to the selection API"? The subdev selection API is in
>> Mauro's tree already so I see no reason not to use it. Implementing new
>> functionality in a driver using API we've just marked obsolete is... not
>> pretty.
>
> You're of course totally right. I've pushed back on enough attemps similar to
> this one to know that I will have to give up here and implement selection
> support :-)

Thanks! :-) You should not need to make many changes to your existing 
patch, I believe. The additional functionality you should support is the 
bounds rectangle; other than that, it's primarily just renaming things.

>> The compatibility code for the old crop ioctls exist, too, so you get
>> exactly the same functionality as well.
>>
>>>> Also, the old S_CROP IOCTL only has been defined for sink pads, not
>>>> source.
>>>
>>> We're already using crop on source pads on sensors ;-)
>>
>> Is that supposed to work? At the very least least it does not follow the
>> spec.
>
> It doesn't follow the spec. But it works :-)

One of the reasons we implemented selections API was to get proper 
support for source pad crop. Well, at least it wouldn't work _with_ 
scaling in the same subdev.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

[PATCH 4/4] omap3isp: Mark probe and cleanup functions with __devinit and __devexit

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isp.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index df6416c..0307ac3 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -1978,7 +1978,7 @@ error_csiphy:
  *
  * Always returns 0.
  */
-static int isp_remove(struct platform_device *pdev)
+static int __devexit isp_remove(struct platform_device *pdev)
 {
 	struct isp_device *isp = platform_get_drvdata(pdev);
 	int i;
@@ -2059,7 +2059,7 @@ static int isp_map_mem_resource(struct platform_device *pdev,
  *   -EINVAL if couldn't install ISR,
  *   or clk_get return error value.
  */
-static int isp_probe(struct platform_device *pdev)
+static int __devinit isp_probe(struct platform_device *pdev)
 {
 	struct isp_platform_data *pdata = pdev->dev.platform_data;
 	struct isp_device *isp;
@@ -2227,7 +2227,7 @@ MODULE_DEVICE_TABLE(platform, omap3isp_id_table);
 
 static struct platform_driver omap3isp_driver = {
 	.probe = isp_probe,
-	.remove = isp_remove,
+	.remove = __devexit_p(isp_remove),
 	.id_table = omap3isp_id_table,
 	.driver = {
 		.owner = THIS_MODULE,
-- 
1.7.3.4

Re: [PATCH 4/4] omap3isp: Mark probe and cleanup functions with __devinit and __devexit

[Sakari Ailus]

On Mon, Apr 23, 2012 at 01:29:55PM +0200, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isp.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk