[PATCH 0/5] pixel format handling in camera host drivers - part 2

[PATCH 0/5] pixel format handling in camera host drivers - part 2

[Guennadi Liakhovetski]

These patches should finish the necessary preparations for the pxa-camera 
driver to finally correctly present its planar YUV format and to be able 
to select camera formats, it actually can support, and perform further 
format conversions as they emerge.

In fact, not all these patches address this issue, some are not really 
related, some minor fixes that just occurred to me while working on the 
formats, but they should be applied in a specific order, so I put them in 
a series.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 1/5] soc-camera: simplify naming

[Guennadi Liakhovetski]

We anyway don't follow the s_fmt_vid_cap / g_fmt_vid_cap / try_fmt_vid_cap
naming, and soc-camera is so far only about video capture, let's simplify
operation names a bit further. set_fmt_cap / try_fmt_cap wasn't a very good
choice too.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/mt9m001.c              |   14 +++++++-------
 drivers/media/video/mt9m111.c              |   12 ++++++------
 drivers/media/video/mt9v022.c              |   14 +++++++-------
 drivers/media/video/ov772x.c               |   14 +++++++-------
 drivers/media/video/pxa_camera.c           |   16 ++++++++--------
 drivers/media/video/sh_mobile_ceu_camera.c |   16 ++++++++--------
 drivers/media/video/soc_camera.c           |    6 +++---
 drivers/media/video/soc_camera_platform.c  |   12 ++++++------
 include/media/soc_camera.h                 |   10 ++++------
 9 files changed, 56 insertions(+), 58 deletions(-)

diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index 0c52437..0bcfef7 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -285,8 +285,8 @@ static unsigned long mt9m001_query_bus_param(struct soc_camera_device *icd)
 		width_flag;
 }
 
-static int mt9m001_set_fmt_cap(struct soc_camera_device *icd,
-		__u32 pixfmt, struct v4l2_rect *rect)
+static int mt9m001_set_fmt(struct soc_camera_device *icd,
+			   __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
 	int ret;
@@ -298,7 +298,7 @@ static int mt9m001_set_fmt_cap(struct soc_camera_device *icd,
 		ret = reg_write(icd, MT9M001_VERTICAL_BLANKING, vblank);
 
 	/* The caller provides a supported format, as verified per
-	 * call to icd->try_fmt_cap() */
+	 * call to icd->try_fmt() */
 	if (!ret)
 		ret = reg_write(icd, MT9M001_COLUMN_START, rect->left);
 	if (!ret)
@@ -325,8 +325,8 @@ static int mt9m001_set_fmt_cap(struct soc_camera_device *icd,
 	return ret;
 }
 
-static int mt9m001_try_fmt_cap(struct soc_camera_device *icd,
-			       struct v4l2_format *f)
+static int mt9m001_try_fmt(struct soc_camera_device *icd,
+			   struct v4l2_format *f)
 {
 	if (f->fmt.pix.height < 32 + icd->y_skip_top)
 		f->fmt.pix.height = 32 + icd->y_skip_top;
@@ -447,8 +447,8 @@ static struct soc_camera_ops mt9m001_ops = {
 	.release		= mt9m001_release,
 	.start_capture		= mt9m001_start_capture,
 	.stop_capture		= mt9m001_stop_capture,
-	.set_fmt_cap		= mt9m001_set_fmt_cap,
-	.try_fmt_cap		= mt9m001_try_fmt_cap,
+	.set_fmt		= mt9m001_set_fmt,
+	.try_fmt		= mt9m001_try_fmt,
 	.set_bus_param		= mt9m001_set_bus_param,
 	.query_bus_param	= mt9m001_query_bus_param,
 	.controls		= mt9m001_controls,
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index da0b2d5..63c52c0 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -452,8 +452,8 @@ static int mt9m111_set_pixfmt(struct soc_camera_device *icd, u32 pixfmt)
 	return ret;
 }
 
-static int mt9m111_set_fmt_cap(struct soc_camera_device *icd,
-			       __u32 pixfmt, struct v4l2_rect *rect)
+static int mt9m111_set_fmt(struct soc_camera_device *icd,
+			   __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
 	int ret;
@@ -473,8 +473,8 @@ static int mt9m111_set_fmt_cap(struct soc_camera_device *icd,
 	return ret;
 }
 
-static int mt9m111_try_fmt_cap(struct soc_camera_device *icd,
-			       struct v4l2_format *f)
+static int mt9m111_try_fmt(struct soc_camera_device *icd,
+			   struct v4l2_format *f)
 {
 	if (f->fmt.pix.height > MT9M111_MAX_HEIGHT)
 		f->fmt.pix.height = MT9M111_MAX_HEIGHT;
@@ -597,8 +597,8 @@ static struct soc_camera_ops mt9m111_ops = {
 	.release		= mt9m111_release,
 	.start_capture		= mt9m111_start_capture,
 	.stop_capture		= mt9m111_stop_capture,
-	.set_fmt_cap		= mt9m111_set_fmt_cap,
-	.try_fmt_cap		= mt9m111_try_fmt_cap,
+	.set_fmt		= mt9m111_set_fmt,
+	.try_fmt		= mt9m111_try_fmt,
 	.query_bus_param	= mt9m111_query_bus_param,
 	.set_bus_param		= mt9m111_set_bus_param,
 	.controls		= mt9m111_controls,
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 2584201..3a39f02 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -337,14 +337,14 @@ static unsigned long mt9v022_query_bus_param(struct soc_camera_device *icd)
 		width_flag;
 }
 
-static int mt9v022_set_fmt_cap(struct soc_camera_device *icd,
-		__u32 pixfmt, struct v4l2_rect *rect)
+static int mt9v022_set_fmt(struct soc_camera_device *icd,
+			   __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
 	int ret;
 
 	/* The caller provides a supported format, as verified per call to
-	 * icd->try_fmt_cap(), datawidth is from our supported format list */
+	 * icd->try_fmt(), datawidth is from our supported format list */
 	switch (pixfmt) {
 	case V4L2_PIX_FMT_GREY:
 	case V4L2_PIX_FMT_Y16:
@@ -400,8 +400,8 @@ static int mt9v022_set_fmt_cap(struct soc_camera_device *icd,
 	return 0;
 }
 
-static int mt9v022_try_fmt_cap(struct soc_camera_device *icd,
-			       struct v4l2_format *f)
+static int mt9v022_try_fmt(struct soc_camera_device *icd,
+			   struct v4l2_format *f)
 {
 	if (f->fmt.pix.height < 32 + icd->y_skip_top)
 		f->fmt.pix.height = 32 + icd->y_skip_top;
@@ -538,8 +538,8 @@ static struct soc_camera_ops mt9v022_ops = {
 	.release		= mt9v022_release,
 	.start_capture		= mt9v022_start_capture,
 	.stop_capture		= mt9v022_stop_capture,
-	.set_fmt_cap		= mt9v022_set_fmt_cap,
-	.try_fmt_cap		= mt9v022_try_fmt_cap,
+	.set_fmt		= mt9v022_set_fmt,
+	.try_fmt		= mt9v022_try_fmt,
 	.set_bus_param		= mt9v022_set_bus_param,
 	.query_bus_param	= mt9v022_query_bus_param,
 	.controls		= mt9v022_controls,
diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 6206dff..b1b4cd2 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -737,9 +737,9 @@ static int ov772x_set_register(struct soc_camera_device *icd,
 }
 #endif
 
-static int ov772x_set_fmt_cap(struct soc_camera_device *icd,
-			      __u32                     pixfmt,
-			      struct v4l2_rect         *rect)
+static int ov772x_set_fmt(struct soc_camera_device *icd,
+			  __u32                     pixfmt,
+			  struct v4l2_rect         *rect)
 {
 	struct ov772x_priv *priv = container_of(icd, struct ov772x_priv, icd);
 	int ret = -EINVAL;
@@ -760,8 +760,8 @@ static int ov772x_set_fmt_cap(struct soc_camera_device *icd,
 	return ret;
 }
 
-static int ov772x_try_fmt_cap(struct soc_camera_device *icd,
-			      struct v4l2_format       *f)
+static int ov772x_try_fmt(struct soc_camera_device *icd,
+			  struct v4l2_format       *f)
 {
 	struct v4l2_pix_format *pix  = &f->fmt.pix;
 	struct ov772x_priv     *priv;
@@ -859,8 +859,8 @@ static struct soc_camera_ops ov772x_ops = {
 	.release		= ov772x_release,
 	.start_capture		= ov772x_start_capture,
 	.stop_capture		= ov772x_stop_capture,
-	.set_fmt_cap		= ov772x_set_fmt_cap,
-	.try_fmt_cap		= ov772x_try_fmt_cap,
+	.set_fmt		= ov772x_set_fmt,
+	.try_fmt		= ov772x_try_fmt,
 	.set_bus_param		= ov772x_set_bus_param,
 	.query_bus_param	= ov772x_query_bus_param,
 	.get_chip_id		= ov772x_get_chip_id,
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index a375872..665eef2 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -904,8 +904,8 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	return soc_camera_bus_param_compatible(camera_flags, bus_flags) ? 0 : -EINVAL;
 }
 
-static int pxa_camera_set_fmt_cap(struct soc_camera_device *icd,
-				  __u32 pixfmt, struct v4l2_rect *rect)
+static int pxa_camera_set_fmt(struct soc_camera_device *icd,
+			      __u32 pixfmt, struct v4l2_rect *rect)
 {
 	const struct soc_camera_data_format *cam_fmt;
 	int ret;
@@ -920,15 +920,15 @@ static int pxa_camera_set_fmt_cap(struct soc_camera_device *icd,
 			return -EINVAL;
 	}
 
-	ret = icd->ops->set_fmt_cap(icd, pixfmt, rect);
+	ret = icd->ops->set_fmt(icd, pixfmt, rect);
 	if (pixfmt && !ret)
 		icd->current_fmt = cam_fmt;
 
 	return ret;
 }
 
-static int pxa_camera_try_fmt_cap(struct soc_camera_device *icd,
-				  struct v4l2_format *f)
+static int pxa_camera_try_fmt(struct soc_camera_device *icd,
+			      struct v4l2_format *f)
 {
 	const struct soc_camera_data_format *cam_fmt;
 	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
@@ -960,7 +960,7 @@ static int pxa_camera_try_fmt_cap(struct soc_camera_device *icd,
 	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
 
 	/* limit to sensor capabilities */
-	return icd->ops->try_fmt_cap(icd, f);
+	return icd->ops->try_fmt(icd, f);
 }
 
 static int pxa_camera_reqbufs(struct soc_camera_file *icf,
@@ -1068,8 +1068,8 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.remove		= pxa_camera_remove_device,
 	.suspend	= pxa_camera_suspend,
 	.resume		= pxa_camera_resume,
-	.set_fmt_cap	= pxa_camera_set_fmt_cap,
-	.try_fmt_cap	= pxa_camera_try_fmt_cap,
+	.set_fmt	= pxa_camera_set_fmt,
+	.try_fmt	= pxa_camera_try_fmt,
 	.init_videobuf	= pxa_camera_init_videobuf,
 	.reqbufs	= pxa_camera_reqbufs,
 	.poll		= pxa_camera_poll,
diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index 1bacfc7..367c4eb 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -444,8 +444,8 @@ static int sh_mobile_ceu_try_bus_param(struct soc_camera_device *icd,
 	return 0;
 }
 
-static int sh_mobile_ceu_set_fmt_cap(struct soc_camera_device *icd,
-				     __u32 pixfmt, struct v4l2_rect *rect)
+static int sh_mobile_ceu_set_fmt(struct soc_camera_device *icd,
+				 __u32 pixfmt, struct v4l2_rect *rect)
 {
 	const struct soc_camera_data_format *cam_fmt;
 	int ret;
@@ -460,15 +460,15 @@ static int sh_mobile_ceu_set_fmt_cap(struct soc_camera_device *icd,
 			return -EINVAL;
 	}
 
-	ret = icd->ops->set_fmt_cap(icd, pixfmt, rect);
+	ret = icd->ops->set_fmt(icd, pixfmt, rect);
 	if (pixfmt && !ret)
 		icd->current_fmt = cam_fmt;
 
 	return ret;
 }
 
-static int sh_mobile_ceu_try_fmt_cap(struct soc_camera_device *icd,
-				     struct v4l2_format *f)
+static int sh_mobile_ceu_try_fmt(struct soc_camera_device *icd,
+				 struct v4l2_format *f)
 {
 	const struct soc_camera_data_format *cam_fmt;
 	int ret = sh_mobile_ceu_try_bus_param(icd, f->fmt.pix.pixelformat);
@@ -502,7 +502,7 @@ static int sh_mobile_ceu_try_fmt_cap(struct soc_camera_device *icd,
 	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
 
 	/* limit to sensor capabilities */
-	return icd->ops->try_fmt_cap(icd, f);
+	return icd->ops->try_fmt(icd, f);
 }
 
 static int sh_mobile_ceu_reqbufs(struct soc_camera_file *icf,
@@ -570,8 +570,8 @@ static struct soc_camera_host_ops sh_mobile_ceu_host_ops = {
 	.owner		= THIS_MODULE,
 	.add		= sh_mobile_ceu_add_device,
 	.remove		= sh_mobile_ceu_remove_device,
-	.set_fmt_cap	= sh_mobile_ceu_set_fmt_cap,
-	.try_fmt_cap	= sh_mobile_ceu_try_fmt_cap,
+	.set_fmt	= sh_mobile_ceu_set_fmt,
+	.try_fmt	= sh_mobile_ceu_try_fmt,
 	.reqbufs	= sh_mobile_ceu_reqbufs,
 	.poll		= sh_mobile_ceu_poll,
 	.querycap	= sh_mobile_ceu_querycap,
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index afadb33..7304e73 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -73,7 +73,7 @@ static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 	}
 
 	/* limit format to hardware capabilities */
-	ret = ici->ops->try_fmt_cap(icd, f);
+	ret = ici->ops->try_fmt(icd, f);
 
 	return ret;
 }
@@ -324,7 +324,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 	rect.top	= icd->y_current;
 	rect.width	= f->fmt.pix.width;
 	rect.height	= f->fmt.pix.height;
-	ret = ici->ops->set_fmt_cap(icd, f->fmt.pix.pixelformat, &rect);
+	ret = ici->ops->set_fmt(icd, f->fmt.pix.pixelformat, &rect);
 	if (ret < 0) {
 		return ret;
 	} else if (!icd->current_fmt ||
@@ -553,7 +553,7 @@ static int soc_camera_s_crop(struct file *file, void *fh,
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
-	ret = ici->ops->set_fmt_cap(icd, 0, &a->c);
+	ret = ici->ops->set_fmt(icd, 0, &a->c);
 	if (!ret) {
 		icd->width	= a->c.width;
 		icd->height	= a->c.height;
diff --git a/drivers/media/video/soc_camera_platform.c b/drivers/media/video/soc_camera_platform.c
index bb7a9d4..c23871e 100644
--- a/drivers/media/video/soc_camera_platform.c
+++ b/drivers/media/video/soc_camera_platform.c
@@ -79,14 +79,14 @@ soc_camera_platform_query_bus_param(struct soc_camera_device *icd)
 	return p->bus_param;
 }
 
-static int soc_camera_platform_set_fmt_cap(struct soc_camera_device *icd,
-					   __u32 pixfmt, struct v4l2_rect *rect)
+static int soc_camera_platform_set_fmt(struct soc_camera_device *icd,
+				       __u32 pixfmt, struct v4l2_rect *rect)
 {
 	return 0;
 }
 
-static int soc_camera_platform_try_fmt_cap(struct soc_camera_device *icd,
-					   struct v4l2_format *f)
+static int soc_camera_platform_try_fmt(struct soc_camera_device *icd,
+				       struct v4l2_format *f)
 {
 	struct soc_camera_platform_info *p = soc_camera_platform_get_info(icd);
 
@@ -124,8 +124,8 @@ static struct soc_camera_ops soc_camera_platform_ops = {
 	.release		= soc_camera_platform_release,
 	.start_capture		= soc_camera_platform_start_capture,
 	.stop_capture		= soc_camera_platform_stop_capture,
-	.set_fmt_cap		= soc_camera_platform_set_fmt_cap,
-	.try_fmt_cap		= soc_camera_platform_try_fmt_cap,
+	.set_fmt		= soc_camera_platform_set_fmt,
+	.try_fmt		= soc_camera_platform_try_fmt,
 	.set_bus_param		= soc_camera_platform_set_bus_param,
 	.query_bus_param	= soc_camera_platform_query_bus_param,
 };
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index c9b2b7f..64eb4b5 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -66,9 +66,8 @@ struct soc_camera_host_ops {
 	void (*remove)(struct soc_camera_device *);
 	int (*suspend)(struct soc_camera_device *, pm_message_t state);
 	int (*resume)(struct soc_camera_device *);
-	int (*set_fmt_cap)(struct soc_camera_device *, __u32,
-			   struct v4l2_rect *);
-	int (*try_fmt_cap)(struct soc_camera_device *, struct v4l2_format *);
+	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
+	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	void (*init_videobuf)(struct videobuf_queue *,
 			      struct soc_camera_device *);
 	int (*reqbufs)(struct soc_camera_file *, struct v4l2_requestbuffers *);
@@ -125,9 +124,8 @@ struct soc_camera_ops {
 	int (*release)(struct soc_camera_device *);
 	int (*start_capture)(struct soc_camera_device *);
 	int (*stop_capture)(struct soc_camera_device *);
-	int (*set_fmt_cap)(struct soc_camera_device *, __u32,
-			   struct v4l2_rect *);
-	int (*try_fmt_cap)(struct soc_camera_device *, struct v4l2_format *);
+	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
+	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
 	unsigned long (*query_bus_param)(struct soc_camera_device *);
 	int (*set_bus_param)(struct soc_camera_device *, unsigned long);
 	int (*get_chip_id)(struct soc_camera_device *,
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 2/5] soc-camera: move pixel format handling to host drivers (part 2)

[Guennadi Liakhovetski]

Just letting camera host drivers handle the choice of a pixel format in
.vidioc_s_fmt_vid_cap() is not enough, pixel format enumeration should be
handled by host drivers too.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/pxa_camera.c           |   17 +++++++++++++++++
 drivers/media/video/sh_mobile_ceu_camera.c |   17 +++++++++++++++++
 drivers/media/video/soc_camera.c           |   12 +++---------
 include/media/soc_camera.h                 |    1 +
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 665eef2..f7f621c 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -963,6 +963,22 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 	return icd->ops->try_fmt(icd, f);
 }
 
+static int pxa_camera_enum_fmt(struct soc_camera_device *icd,
+			       struct v4l2_fmtdesc *f)
+{
+	const struct soc_camera_data_format *format;
+
+	if (f->index >= icd->num_formats)
+		return -EINVAL;
+
+	format = &icd->formats[f->index];
+
+	strlcpy(f->description, format->name, sizeof(f->description));
+	f->pixelformat = format->fourcc;
+
+	return 0;
+}
+
 static int pxa_camera_reqbufs(struct soc_camera_file *icf,
 			      struct v4l2_requestbuffers *p)
 {
@@ -1070,6 +1086,7 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.resume		= pxa_camera_resume,
 	.set_fmt	= pxa_camera_set_fmt,
 	.try_fmt	= pxa_camera_try_fmt,
+	.enum_fmt	= pxa_camera_enum_fmt,
 	.init_videobuf	= pxa_camera_init_videobuf,
 	.reqbufs	= pxa_camera_reqbufs,
 	.poll		= pxa_camera_poll,
diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index 367c4eb..fdfe04f 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -505,6 +505,22 @@ static int sh_mobile_ceu_try_fmt(struct soc_camera_device *icd,
 	return icd->ops->try_fmt(icd, f);
 }
 
+static int sh_mobile_ceu_enum_fmt(struct soc_camera_device *icd,
+				  struct v4l2_fmtdesc *f)
+{
+	const struct soc_camera_data_format *format;
+
+	if (f->index >= icd->num_formats)
+		return -EINVAL;
+
+	format = &icd->formats[f->index];
+
+	strlcpy(f->description, format->name, sizeof(f->description));
+	f->pixelformat = format->fourcc;
+
+	return 0;
+}
+
 static int sh_mobile_ceu_reqbufs(struct soc_camera_file *icf,
 				 struct v4l2_requestbuffers *p)
 {
@@ -572,6 +588,7 @@ static struct soc_camera_host_ops sh_mobile_ceu_host_ops = {
 	.remove		= sh_mobile_ceu_remove_device,
 	.set_fmt	= sh_mobile_ceu_set_fmt,
 	.try_fmt	= sh_mobile_ceu_try_fmt,
+	.enum_fmt	= sh_mobile_ceu_enum_fmt,
 	.reqbufs	= sh_mobile_ceu_reqbufs,
 	.poll		= sh_mobile_ceu_poll,
 	.querycap	= sh_mobile_ceu_querycap,
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 7304e73..f5a1a86 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -355,18 +355,12 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	const struct soc_camera_data_format *format;
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icd->dev.parent);
 
 	WARN_ON(priv != file->private_data);
 
-	if (f->index >= icd->num_formats)
-		return -EINVAL;
-
-	format = &icd->formats[f->index];
-
-	strlcpy(f->description, format->name, sizeof(f->description));
-	f->pixelformat = format->fourcc;
-	return 0;
+	return ici->ops->enum_fmt(icd, f);
 }
 
 static int soc_camera_g_fmt_vid_cap(struct file *file, void *priv,
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 64eb4b5..9c3734c 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -68,6 +68,7 @@ struct soc_camera_host_ops {
 	int (*resume)(struct soc_camera_device *);
 	int (*set_fmt)(struct soc_camera_device *, __u32, struct v4l2_rect *);
 	int (*try_fmt)(struct soc_camera_device *, struct v4l2_format *);
+	int (*enum_fmt)(struct soc_camera_device *, struct v4l2_fmtdesc *);
 	void (*init_videobuf)(struct videobuf_queue *,
 			      struct soc_camera_device *);
 	int (*reqbufs)(struct soc_camera_file *, struct v4l2_requestbuffers *);
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 4/5] soc-camera: initialise fields on device-registration, more formatting cleanup

[Guennadi Liakhovetski]

Camera devices typically allocate their struct soc_camera_device objects per
kzalloc(), but soc_camera.c shall not rely on that and shall initialise
host_priv and use_count explicitly. Many camera and host methods are called
without being checked for NULL, let's check them at registration. Also merge
broken lines, that no longer exceed 80 characters.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/pxa_camera.c |   53 ++++++++++++++++---------------------
 drivers/media/video/soc_camera.c |   54 ++++++++++++++++++++++---------------
 2 files changed, 55 insertions(+), 52 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index f7f621c..56aeb07 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -143,8 +143,7 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
 			      unsigned int *size)
 {
 	struct soc_camera_device *icd = vq->priv_data;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 
 	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
@@ -170,8 +169,7 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
 static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf)
 {
 	struct soc_camera_device *icd = vq->priv_data;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb);
 	int i;
@@ -247,8 +245,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 		struct videobuf_buffer *vb, enum v4l2_field field)
 {
 	struct soc_camera_device *icd = vq->priv_data;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
 	int ret;
@@ -367,8 +364,7 @@ static void pxa_videobuf_queue(struct videobuf_queue *vq,
 			       struct videobuf_buffer *vb)
 {
 	struct soc_camera_device *icd = vq->priv_data;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
 	struct pxa_buffer *active;
@@ -772,8 +768,7 @@ static int test_platform_param(struct pxa_camera_dev *pcdev,
 
 static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long dw, bpp, bus_flags, camera_flags, common_flags;
 	u32 cicr0, cicr1, cicr4 = 0;
@@ -890,8 +885,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 
 static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
 	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);
@@ -931,7 +925,8 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
 	const struct soc_camera_data_format *cam_fmt;
-	int ret = pxa_camera_try_bus_param(icd, f->fmt.pix.pixelformat);
+	struct v4l2_pix_format *pix = &f->fmt.pix;
+	int ret = pxa_camera_try_bus_param(icd, pix->pixelformat);
 
 	if (ret < 0)
 		return ret;
@@ -940,24 +935,24 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 	 * TODO: find a suitable supported by the SoC output format, check
 	 * whether the sensor supports one of acceptable input formats.
 	 */
-	cam_fmt = soc_camera_format_by_fourcc(icd, f->fmt.pix.pixelformat);
+	cam_fmt = soc_camera_format_by_fourcc(icd, pix->pixelformat);
 	if (!cam_fmt)
 		return -EINVAL;
 
 	/* limit to pxa hardware capabilities */
-	if (f->fmt.pix.height < 32)
-		f->fmt.pix.height = 32;
-	if (f->fmt.pix.height > 2048)
-		f->fmt.pix.height = 2048;
-	if (f->fmt.pix.width < 48)
-		f->fmt.pix.width = 48;
-	if (f->fmt.pix.width > 2048)
-		f->fmt.pix.width = 2048;
-	f->fmt.pix.width &= ~0x01;
-
-	f->fmt.pix.bytesperline = f->fmt.pix.width *
+	if (pix->height < 32)
+		pix->height = 32;
+	if (pix->height > 2048)
+		pix->height = 2048;
+	if (pix->width < 48)
+		pix->width = 48;
+	if (pix->width > 2048)
+		pix->width = 2048;
+	pix->width &= ~0x01;
+
+	pix->bytesperline = pix->width *
 		DIV_ROUND_UP(cam_fmt->depth, 8);
-	f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
+	pix->sizeimage = pix->height * pix->bytesperline;
 
 	/* limit to sensor capabilities */
 	return icd->ops->try_fmt(icd, f);
@@ -1028,8 +1023,7 @@ static int pxa_camera_querycap(struct soc_camera_host *ici,
 
 static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	int i = 0, ret = 0;
 
@@ -1047,8 +1041,7 @@ static int pxa_camera_suspend(struct soc_camera_device *icd, pm_message_t state)
 
 static int pxa_camera_resume(struct soc_camera_device *icd)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	int i = 0, ret = 0;
 
diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index f5a1a86..a63738c 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -52,8 +52,7 @@ static int soc_camera_try_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	enum v4l2_field field;
 	int ret;
 
@@ -117,8 +116,7 @@ static int soc_camera_reqbufs(struct file *file, void *priv,
 	int ret;
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 
 	WARN_ON(priv != file->private_data);
 
@@ -282,8 +280,7 @@ static unsigned int soc_camera_poll(struct file *file, poll_table *pt)
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 
 	if (list_empty(&icf->vb_vidq.stream)) {
 		dev_err(&icd->dev, "Trying to poll with no queued buffers!\n");
@@ -309,8 +306,7 @@ static int soc_camera_s_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	int ret;
 	struct v4l2_rect rect;
 
@@ -355,8 +351,7 @@ static int soc_camera_enum_fmt_vid_cap(struct file *file, void  *priv,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 
 	WARN_ON(priv != file->private_data);
 
@@ -388,8 +383,7 @@ static int soc_camera_querycap(struct file *file, void  *priv,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 
 	WARN_ON(priv != file->private_data);
 
@@ -540,8 +534,7 @@ static int soc_camera_s_crop(struct file *file, void *fh,
 {
 	struct soc_camera_file *icf = file->private_data;
 	struct soc_camera_device *icd = icf->icd;
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	int ret;
 
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
@@ -665,13 +658,9 @@ static int scan_add_device(struct soc_camera_device *icd)
 static int soc_camera_probe(struct device *dev)
 {
 	struct soc_camera_device *icd = to_soc_camera_dev(dev);
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	int ret;
 
-	if (!icd->ops->probe)
-		return -ENODEV;
-
 	/* We only call ->add() here to activate and probe the camera.
 	 * We shall ->remove() and deactivate it immediately afterwards. */
 	ret = ici->ops->add(icd);
@@ -752,7 +741,17 @@ int soc_camera_host_register(struct soc_camera_host *ici)
 	int ret;
 	struct soc_camera_host *ix;
 
-	if (!ici->ops->init_videobuf || !ici->ops->add || !ici->ops->remove)
+	if (!ici || !ici->ops ||
+	    !ici->ops->try_fmt ||
+	    !ici->ops->set_fmt ||
+	    !ici->ops->enum_fmt ||
+	    !ici->ops->set_bus_param ||
+	    !ici->ops->querycap ||
+	    !ici->ops->init_videobuf ||
+	    !ici->ops->reqbufs ||
+	    !ici->ops->add ||
+	    !ici->ops->remove ||
+	    !ici->ops->poll)
 		return -EINVAL;
 
 	/* Number might be equal to the platform device ID */
@@ -820,7 +819,16 @@ int soc_camera_device_register(struct soc_camera_device *icd)
 	struct soc_camera_device *ix;
 	int num = -1, i;
 
-	if (!icd)
+	if (!icd || !icd->ops ||
+	    !icd->ops->probe ||
+	    !icd->ops->init ||
+	    !icd->ops->release ||
+	    !icd->ops->start_capture ||
+	    !icd->ops->stop_capture ||
+	    !icd->ops->set_fmt ||
+	    !icd->ops->try_fmt ||
+	    !icd->ops->query_bus_param ||
+	    !icd->ops->set_bus_param)
 		return -EINVAL;
 
 	for (i = 0; i < 256 && num < 0; i++) {
@@ -843,7 +851,9 @@ int soc_camera_device_register(struct soc_camera_device *icd)
 	snprintf(icd->dev.bus_id, sizeof(icd->dev.bus_id),
 		 "%u-%u", icd->iface, icd->devnum);
 
-	icd->dev.release = dummy_release;
+	icd->dev.release	= dummy_release;
+	icd->use_count		= 0;
+	icd->host_priv		= NULL;
 
 	return scan_add_device(icd);
 }
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

This creates the framework, necessary to handle pixel formats, provided by the
camera, and formats, synthesized by the Quick Capture Interface. Now we just
have to handle each specific format individually.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/pxa_camera.c |  211 +++++++++++++++++++++++++++++++++++---
 1 files changed, 194 insertions(+), 17 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 56aeb07..00eebf1 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -27,6 +27,7 @@
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/clk.h>
+#include <linux/vmalloc.h>
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-dev.h>
@@ -132,6 +133,21 @@ struct pxa_camera_dev {
 	u32			save_cicr[5];
 };
 
+/**
+ * @camera_formats:	pointer to an array of links to supported camera
+ *			formats.
+ * @camera_formats_num:	size of the above array.
+ * @extra_formats:	formats we support by converting a different camera
+ *			format.
+ * @extra_formats_num:	size of the above array.
+ */
+struct camera_data {
+	int					camera_formats_num;
+	const struct soc_camera_data_format	**camera_formats;
+	int					extra_formats_num;
+	struct soc_camera_data_format		extra_formats[];
+};
+
 static const char *pxa_cam_driver_description = "PXA_Camera";
 
 static unsigned int vid_limit = 16;	/* Video memory limit, in Mb */
@@ -673,6 +689,104 @@ static irqreturn_t pxa_camera_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static bool depth_supported(struct soc_camera_device *icd, int i)
+{
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
+
+	switch (icd->formats[i].depth) {
+	case 8:
+		if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)
+			return true;
+		return false;
+	case 9:
+		if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_9)
+			return true;
+		return false;
+	case 10:
+		if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10)
+			return true;
+		return false;
+	}
+	return false;
+}
+
+static bool format_supported(struct soc_camera_device *icd, int i)
+{
+	if (!depth_supported(icd, i))
+		return false;
+
+	/* To be implemented - verify fourcc */
+	return true;
+}
+
+/**
+ * @icd:	camera device
+ * @i:		index of the camera format to be considered
+ * @fmt:	if (fmt != NULL) it points to the beginning of an area,
+ *		sufficient for all extra formats, that the i's camera format
+ *		can be converted to, and this area has to be filled in
+ * @return:	number of extra formats found for this camera format
+ */
+static int format_extras(struct soc_camera_device *icd, int i,
+			 const struct soc_camera_data_format *fmt)
+{
+	if (!depth_supported(icd, i))
+		return 0;
+
+	/* To be implemented */
+	return 0;
+}
+
+static int init_formats(struct soc_camera_device *icd)
+{
+	int cam_fmts = 0, extra_fmts = 0, i;
+	int cam_idx = 0, extra_idx = 0;
+	struct camera_data *cam_data;
+
+	for (i = 0; i < icd->num_formats; i++) {
+		if (format_supported(icd, i))
+			cam_fmts++;
+
+		/*
+		 * Even formats we cannot pass to the user might be suitable
+		 * for conversion
+		 */
+		extra_fmts += format_extras(icd, i, NULL);
+	}
+
+	dev_dbg(&icd->dev, "Found %d native and %d synthesized formats\n",
+		cam_fmts, extra_fmts);
+
+	icd->host_priv = vmalloc(sizeof(struct camera_data) +
+				 sizeof(struct soc_camera_data_format) *
+				 extra_fmts + sizeof(void *) * cam_fmts);
+	if (!icd->host_priv)
+		return -ENOMEM;
+
+	/*
+	 * Layout:
+	 * struct camera_data
+	 * extra_formats[extra_fmts]
+	 * camera_formats[cam_fmts]
+	 */
+	cam_data = icd->host_priv;
+
+	cam_data->camera_formats_num	= cam_fmts;
+	cam_data->camera_formats	= icd->host_priv + sizeof(struct camera_data) +
+		sizeof(struct soc_camera_data_format) * extra_fmts;
+	cam_data->extra_formats_num	= extra_fmts;
+
+	for (i = 0; i < icd->num_formats; i++) {
+		if (format_supported(icd, i))
+			cam_data->camera_formats[cam_idx++] = icd->formats + i;
+
+		extra_idx += format_extras(icd, i, cam_data->extra_formats + extra_idx);
+	}
+
+	return 0;
+}
+
 /* The following two functions absolutely depend on the fact, that
  * there can be only one camera on PXA quick capture interface */
 static int pxa_camera_add_device(struct soc_camera_device *icd)
@@ -693,10 +807,30 @@ static int pxa_camera_add_device(struct soc_camera_device *icd)
 
 	pxa_camera_activate(pcdev);
 	ret = icd->ops->init(icd);
+	if (ret < 0)
+		goto einit;
+
+	pcdev->icd = icd;
+
+	/*
+	 * We're either called during probing or during open under video_lock.
+	 * During probe we don't need to initialise the formats, and during
+	 * open video_lock is held, so, it is safe to use the use_count
+	 */
+	if (icd->use_count) {
+		ret = init_formats(icd);
+		if (ret < 0)
+			goto eifmt;
+	}
 
-	if (!ret)
-		pcdev->icd = icd;
+	mutex_unlock(&camera_lock);
+
+	return 0;
 
+eifmt:
+	icd->ops->release(icd);
+einit:
+	pxa_camera_deactivate(pcdev);
 ebusy:
 	mutex_unlock(&camera_lock);
 
@@ -713,6 +847,8 @@ static void pxa_camera_remove_device(struct soc_camera_device *icd)
 	dev_info(&icd->dev, "PXA Camera driver detached from camera %d\n",
 		 icd->devnum);
 
+	mutex_lock(&camera_lock);
+
 	/* disable capture, disable interrupts */
 	CICR0 = 0x3ff;
 
@@ -725,7 +861,12 @@ static void pxa_camera_remove_device(struct soc_camera_device *icd)
 
 	pxa_camera_deactivate(pcdev);
 
+	vfree(icd->host_priv);
+	icd->host_priv = NULL;
+
 	pcdev->icd = NULL;
+
+	mutex_unlock(&camera_lock);
 }
 
 static int test_platform_param(struct pxa_camera_dev *pcdev,
@@ -901,15 +1042,33 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
-	const struct soc_camera_data_format *cam_fmt;
+	const struct soc_camera_data_format *cam_fmt = NULL;
 	int ret;
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
 	if (pixfmt) {
-		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
+		struct camera_data *cam_data = icd->host_priv;
+		int i;
+
+		/* First check camera native formats */
+		for (i = 0; i < cam_data->camera_formats_num; i++)
+			if (cam_data->camera_formats[i]->fourcc == pixfmt) {
+				cam_fmt = cam_data->camera_formats[i];
+				break;
+			}
+
+		/* Next, if failed, check synthesized formats */
+		if (!cam_fmt)
+			for (i = 0; i < cam_data->extra_formats_num; i++)
+				if (cam_data->extra_formats[i].fourcc ==
+				    pixfmt) {
+					cam_fmt = cam_data->extra_formats + i;
+					/* TODO: synthesize... */
+					dev_err(&icd->dev,
+						"Cannot provide format 0x%x\n",
+						pixfmt);
+					return -EOPNOTSUPP;
+				}
+
 		if (!cam_fmt)
 			return -EINVAL;
 	}
@@ -924,18 +1083,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
-	const struct soc_camera_data_format *cam_fmt;
+	struct camera_data *cam_data = icd->host_priv;
+	const struct soc_camera_data_format *cam_fmt = NULL;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	int ret = pxa_camera_try_bus_param(icd, pix->pixelformat);
+	__u32 pixfmt = pix->pixelformat;
+	int ret = pxa_camera_try_bus_param(icd, pixfmt);
+	int i;
 
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * TODO: find a suitable supported by the SoC output format, check
-	 * whether the sensor supports one of acceptable input formats.
-	 */
-	cam_fmt = soc_camera_format_by_fourcc(icd, pix->pixelformat);
+	/* First check camera native formats */
+	for (i = 0; i < cam_data->camera_formats_num; i++)
+		if (cam_data->camera_formats[i]->fourcc == pixfmt) {
+			cam_fmt = cam_data->camera_formats[i];
+			break;
+		}
+
+	/* Next, if failed, check synthesized formats */
+	if (!cam_fmt)
+		for (i = 0; i < cam_data->extra_formats_num; i++)
+			if (cam_data->extra_formats[i].fourcc == pixfmt) {
+				cam_fmt = cam_data->extra_formats + i;
+				break;
+			}
+
 	if (!cam_fmt)
 		return -EINVAL;
 
@@ -962,11 +1134,16 @@ static int pxa_camera_enum_fmt(struct soc_camera_device *icd,
 			       struct v4l2_fmtdesc *f)
 {
 	const struct soc_camera_data_format *format;
+	struct camera_data *cam_data = icd->host_priv;
 
-	if (f->index >= icd->num_formats)
+	if (f->index >= cam_data->camera_formats_num + cam_data->extra_formats_num)
 		return -EINVAL;
 
-	format = &icd->formats[f->index];
+	if (f->index < cam_data->camera_formats_num)
+		format = cam_data->camera_formats[f->index];
+	else
+		format = &cam_data->extra_formats[f->index -
+						  cam_data->camera_formats_num];
 
 	strlcpy(f->description, format->name, sizeof(f->description));
 	f->pixelformat = format->fourcc;
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[David Ellingsworth]

[snip]
> +static bool depth_supported(struct soc_camera_device *icd, int i)
> +{
> +       struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> +       struct pxa_camera_dev *pcdev = ici->priv;
> +
> +       switch (icd->formats[i].depth) {
> +       case 8:
> +               if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)
> +                       return true;
> +               return false;
I'm not sure what the linux kernel development docs might say about
this, but the if statement here might be unnecessary. Couldn't you
write the following instead?

return pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8;

> +       case 9:
> +               if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_9)
> +                       return true;
> +               return false;
> +       case 10:
> +               if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10)
> +                       return true;
> +               return false;
> +       }
> +       return false;
> +}

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

On Mon, 10 Nov 2008, David Ellingsworth wrote:

> [snip]
> > +static bool depth_supported(struct soc_camera_device *icd, int i)
> > +{
> > +       struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > +       struct pxa_camera_dev *pcdev = ici->priv;
> > +
> > +       switch (icd->formats[i].depth) {
> > +       case 8:
> > +               if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8)
> > +                       return true;
> > +               return false;
> I'm not sure what the linux kernel development docs might say about
> this, but the if statement here might be unnecessary. Couldn't you
> write the following instead?
> 
> return pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8;

Indeed, a good idea, thanks, only I would do this like

	return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Indeed, a good idea, thanks, only I would do this like
>
> 	return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);
You're using it 2 times, and with a if (!depth_supported()), that's overkill.
Wouldn't it be better for that function to return 0 for false, and "not 0" for
true ? That's what was done for gpio API (check gpio_get_value()) ...

I would definitely drop the purely boolean part, I don't think it brings
anything here (the function name is already very clear, isn't it ? :))

JM2P.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

On Mon, 10 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Indeed, a good idea, thanks, only I would do this like
> >
> > 	return !!(pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_8);
> You're using it 2 times, and with a if (!depth_supported()), that's overkill.
> Wouldn't it be better for that function to return 0 for false, and "not 0" for
> true ? That's what was done for gpio API (check gpio_get_value()) ...
> 
> I would definitely drop the purely boolean part, I don't think it brings
> anything here (the function name is already very clear, isn't it ? :))

The idea behind "!!x" is, that the compiler should be smart enough to drop 
this and to just convert this to "x!=0", whereas "!!x" has the advantage 
of pointing out to treating "x" as boolean, and in "x!=0" it is compared 
against a number, or some such. In any case, I haven't invented it, I 
remember it being discussed on lkml and the opinion about it was pretty 
positive. So, if this hasn't changed since then, it should be fine:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> @@ -901,15 +1042,33 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
>  			      __u32 pixfmt, struct v4l2_rect *rect)
snip
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
>  	if (pixfmt) {
> -		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> +		struct camera_data *cam_data = icd->host_priv;
> +		int i;
> +
> +		/* First check camera native formats */
> +		for (i = 0; i < cam_data->camera_formats_num; i++)
> +			if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> +				cam_fmt = cam_data->camera_formats[i];
> +				break;
> +			}
> +
> +		/* Next, if failed, check synthesized formats */
> +		if (!cam_fmt)
> +			for (i = 0; i < cam_data->extra_formats_num; i++)
> +				if (cam_data->extra_formats[i].fourcc ==
> +				    pixfmt) {
> +					cam_fmt = cam_data->extra_formats + i;
> +					/* TODO: synthesize... */
> +					dev_err(&icd->dev,
> +						"Cannot provide format 0x%x\n",
> +						pixfmt);
> +					return -EOPNOTSUPP;
> +				}
> +
>  		if (!cam_fmt)
>  			return -EINVAL;
>  	}

> @@ -924,18 +1083,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
snip
> -	/*
> -	 * TODO: find a suitable supported by the SoC output format, check
> -	 * whether the sensor supports one of acceptable input formats.
> -	 */
> -	cam_fmt = soc_camera_format_by_fourcc(icd, pix->pixelformat);
> +	/* First check camera native formats */
> +	for (i = 0; i < cam_data->camera_formats_num; i++)
> +		if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> +			cam_fmt = cam_data->camera_formats[i];
> +			break;
> +		}
> +
> +	/* Next, if failed, check synthesized formats */
> +	if (!cam_fmt)
> +		for (i = 0; i < cam_data->extra_formats_num; i++)
> +			if (cam_data->extra_formats[i].fourcc == pixfmt) {
> +				cam_fmt = cam_data->extra_formats + i;
> +				break;
> +			}
> +
>  	if (!cam_fmt)
>  		return -EINVAL;
Isn't that the second time you're looking for a format the same way, with only a
printk making a difference ? Shouldn't that be grouped in a function
(pxa_camera_find_format(icd, pixfmt) ?) ...


More globally, all camera hosts will implement the creation of this formats
table. Why did you choose to put that in pxa_camera, and not in soc_camera to
make available to all host drivers ?
I had thought you would design something like :

 - soc_camera provides a format like :

struct soc_camera_format_translate {
       u32 host_pixfmt;
       u32 sensor_pixfmt;
};

 - camera host provide a static table like this :
struct soc_camera_format_translate pxa_pixfmt_translations[] = {
       { V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_YUYV },
       { V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_UYVY },
       ...
       { V4L2_PIX_FMT_YUV422P, V4L2_PIX_FMT_UYVY},
};

 - soc_camera provides functions like :
  - soc_camera_compute_formats(struct soc_camera_format_translate trans,
                               struct soc_camera_data_format sensor_formats)
    => that creates the formats table

 - camera host either :
  - call the generic soc_camera_compute_formats()
  - or make the computation themself if it is way too specific.

Is there a reason you chose to fully export the formats computation to hosts ?

Otherwise, the other 4 patches look fine to me, I'll make some tests tomorrow.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

On Tue, 11 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > @@ -901,15 +1042,33 @@ static int pxa_camera_try_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> >  static int pxa_camera_set_fmt(struct soc_camera_device *icd,
> >  			      __u32 pixfmt, struct v4l2_rect *rect)
> snip
> > -	/*
> > -	 * TODO: find a suitable supported by the SoC output format, check
> > -	 * whether the sensor supports one of acceptable input formats.
> > -	 */
> >  	if (pixfmt) {
> > -		cam_fmt = soc_camera_format_by_fourcc(icd, pixfmt);
> > +		struct camera_data *cam_data = icd->host_priv;
> > +		int i;
> > +
> > +		/* First check camera native formats */
> > +		for (i = 0; i < cam_data->camera_formats_num; i++)
> > +			if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> > +				cam_fmt = cam_data->camera_formats[i];
> > +				break;
> > +			}
> > +
> > +		/* Next, if failed, check synthesized formats */
> > +		if (!cam_fmt)
> > +			for (i = 0; i < cam_data->extra_formats_num; i++)
> > +				if (cam_data->extra_formats[i].fourcc ==
> > +				    pixfmt) {
> > +					cam_fmt = cam_data->extra_formats + i;
> > +					/* TODO: synthesize... */
> > +					dev_err(&icd->dev,
> > +						"Cannot provide format 0x%x\n",
> > +						pixfmt);
> > +					return -EOPNOTSUPP;
> > +				}
> > +
> >  		if (!cam_fmt)
> >  			return -EINVAL;
> >  	}
> 
> > @@ -924,18 +1083,31 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
> snip
> > -	/*
> > -	 * TODO: find a suitable supported by the SoC output format, check
> > -	 * whether the sensor supports one of acceptable input formats.
> > -	 */
> > -	cam_fmt = soc_camera_format_by_fourcc(icd, pix->pixelformat);
> > +	/* First check camera native formats */
> > +	for (i = 0; i < cam_data->camera_formats_num; i++)
> > +		if (cam_data->camera_formats[i]->fourcc == pixfmt) {
> > +			cam_fmt = cam_data->camera_formats[i];
> > +			break;
> > +		}
> > +
> > +	/* Next, if failed, check synthesized formats */
> > +	if (!cam_fmt)
> > +		for (i = 0; i < cam_data->extra_formats_num; i++)
> > +			if (cam_data->extra_formats[i].fourcc == pixfmt) {
> > +				cam_fmt = cam_data->extra_formats + i;
> > +				break;
> > +			}
> > +
> >  	if (!cam_fmt)
> >  		return -EINVAL;
> Isn't that the second time you're looking for a format the same way, with only a
> printk making a difference ? Shouldn't that be grouped in a function
> (pxa_camera_find_format(icd, pixfmt) ?) ...

No, the real difference between them is the comment:

+					/* TODO: synthesize... */

that's where your code is supposed to go, and that's what makes them 
different - one only tries to see if it would work, the other one actually 
does. But depending on how complex your synthesis is going to be, maybe 
we'll be able to abstract parts of it further, yes.

> More globally, all camera hosts will implement the creation of this formats
> table.

That's what I am not sure about

> Why did you choose to put that in pxa_camera, and not in soc_camera to
> make available to all host drivers ?
> I had thought you would design something like :
> 
>  - soc_camera provides a format like :
> 
> struct soc_camera_format_translate {
>        u32 host_pixfmt;
>        u32 sensor_pixfmt;
> };
> 
>  - camera host provide a static table like this :
> struct soc_camera_format_translate pxa_pixfmt_translations[] = {
>        { V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_YUYV },
>        { V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_UYVY },
>        ...
>        { V4L2_PIX_FMT_YUV422P, V4L2_PIX_FMT_UYVY},
> };

Hm, I don't think you want to list all possible formats you can pull 
through this camera host. AFAIU, camera hosts can transfer data from 
cameras to destination (memory / framebuffer / output device...) in three 
ways:

1. generic: just pack what appears on the camera bus in output buffers. 
Only restrictions here are bus-width, frame-size...

2. 1-to-1: like pxa packed support for YUV / RGB. You get the same format 
on the output as on input, but re-packed, maybe scaled / rotated / 
otherwise transformed.

3. translated: like pxa UYUV to YUV422P - a different format on output 
than on input.

So far we only supported 1 and 2. For which we just used pixel format 
tables provided by the camera-sensor. But the easiest case is 1, this is 
what we currently use for Bayer and monochrome formats. And you do not 
want to create a table like above of all possible formats for each host 
that supports it. That's why I create two tables per device - one for 
sensor-native formats we just pass 1-to-1, and one list for synthesised 
formats.

For 1 and 2 we now export soc_camera_format_by_fourcc() (see 
sh_mobile_ceu_camera.c). For hosts only supporting these two modes, we can 
provide a default .enum_fmt(), maybe .set_fmt().

For 3 - as I wrote, camera supported pixel formats seem to be statis, and 
I just think, that SoC designers are a little bit more creative than CMOS 
camera designers, so that creating a generic approach might be too 
difficult.

In any case, in the beginning I put quite a lot of functionality in 
soc_camera.c. Now we notice, that we need more and more special-casing, 
and the functionality is migrating into respective camera or host drivers. 
Now I'd like to avoid this. Instead of guessing how to support "all" 
hosts, I would first implement functionality inside host drivers, and 
then, if there is too much copy-paste, extract it into common code. Yes, 
this approach has its disadvantages too...

Also, a probably better approach than what you suggested above (if I 
understood it right) would be not to use a static translation table, but 
to generate one dynamically during .add() and have them per-device, not 
per-host.

>  - soc_camera provides functions like :
>   - soc_camera_compute_formats(struct soc_camera_format_translate trans,
>                                struct soc_camera_data_format sensor_formats)
>     => that creates the formats table
> 
>  - camera host either :
>   - call the generic soc_camera_compute_formats()
>   - or make the computation themself if it is way too specific.
> 
> Is there a reason you chose to fully export the formats computation to hosts ?

In short: I'd prefer to first keep this in pxa-camera, and then see as new 
host drivers arrive, whether we can make portions of the code generic. 
Makes sense?

> Otherwise, the other 4 patches look fine to me, I'll make some tests tomorrow.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> Isn't that the second time you're looking for a format the same way, with only a
>> printk making a difference ? Shouldn't that be grouped in a function
>> (pxa_camera_find_format(icd, pixfmt) ?) ...
>
> No, the real difference between them is the comment:
>
> +					/* TODO: synthesize... */
True. But the 30 or so lines of code are the same. I still think this should be
in a unique function, doing translation or not (think function parameter here).

>> More globally, all camera hosts will implement the creation of this formats
>> table.
>
> That's what I am not sure about
>
>> Why did you choose to put that in pxa_camera, and not in soc_camera to
>> make available to all host drivers ?
>> I had thought you would design something like :
>> 
>>  - soc_camera provides a format like :
>> 
>> struct soc_camera_format_translate {
>>        u32 host_pixfmt;
>>        u32 sensor_pixfmt;
>> };
>> 
>>  - camera host provide a static table like this :
>> struct soc_camera_format_translate pxa_pixfmt_translations[] = {
>>        { V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_YUYV },
>>        { V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_UYVY },
>>        ...
>>        { V4L2_PIX_FMT_YUV422P, V4L2_PIX_FMT_UYVY},
>> };
>
> Hm, I don't think you want to list all possible formats you can pull 
> through this camera host. AFAIU, camera hosts can transfer data from 
> cameras to destination (memory / framebuffer / output device...) in three 
> ways:
Oh yes, you should list them all : that's what you'll end up doing once the
function format_supported() is fully implemented anyway, wouldn't you ?
format_supported() will compare a list of known formats to the sensor output
formats, and keep only known ones (ie. drop RGB32, or YUV 4:2:0, etc ...)

> 1. generic: just pack what appears on the camera bus in output buffers. 
> Only restrictions here are bus-width, frame-size...
>
> 2. 1-to-1: like pxa packed support for YUV / RGB. You get the same format 
> on the output as on input, but re-packed, maybe scaled / rotated / 
> otherwise transformed.
>
> 3. translated: like pxa UYUV to YUV422P - a different format on output 
> than on input.
>
> So far we only supported 1 and 2. For which we just used pixel format 
> tables provided by the camera-sensor. But the easiest case is 1, this is 
> what we currently use for Bayer and monochrome formats. And you do not 
> want to create a table like above of all possible formats for each host 
> that supports it. That's why I create two tables per device - one for 
> sensor-native formats we just pass 1-to-1, and one list for synthesised 
> formats.
>
> For 1 and 2 we now export soc_camera_format_by_fourcc() (see 
> sh_mobile_ceu_camera.c). For hosts only supporting these two modes, we can 
> provide a default .enum_fmt(), maybe .set_fmt().
>
> For 3 - as I wrote, camera supported pixel formats seem to be statis, and 
> I just think, that SoC designers are a little bit more creative than CMOS 
> camera designers, so that creating a generic approach might be too 
> difficult.
>
> In any case, in the beginning I put quite a lot of functionality in 
> soc_camera.c. Now we notice, that we need more and more special-casing, 
> and the functionality is migrating into respective camera or host drivers. 
> Now I'd like to avoid this. Instead of guessing how to support "all" 
> hosts, I would first implement functionality inside host drivers, and 
> then, if there is too much copy-paste, extract it into common code. Yes, 
> this approach has its disadvantages too...
Yes, amongst them let loose coders of each host ...
>
> Also, a probably better approach than what you suggested above (if I 
> understood it right) would be not to use a static translation table, but 
> to generate one dynamically during .add() and have them per-device, not 
> per-host.
The translation table will be per-host static (it's hardwired in the chip), but
the generated supported formats will be dynamically generated by the host on
sensor attachment (ie. computed), and for each device.

>> Is there a reason you chose to fully export the formats computation to hosts ?
>
> In short: I'd prefer to first keep this in pxa-camera, and then see as new 
> host drivers arrive, whether we can make portions of the code generic. 
> Makes sense?
I understand your thinking. I don't think it's the good way to go, but you're
the maintainer, you decide. We'll see soon enough, once TI and Qualcomm embedded
devices will be enough documented, who was right.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

On Tue, 11 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> >> };
> >> 
> >>  - camera host provide a static table like this :
> >> struct soc_camera_format_translate pxa_pixfmt_translations[] = {
> >>        { V4L2_PIX_FMT_YUYV, V4L2_PIX_FMT_YUYV },
> >>        { V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_UYVY },
> >>        ...
> >>        { V4L2_PIX_FMT_YUV422P, V4L2_PIX_FMT_UYVY},
> >> };
> >
> > Hm, I don't think you want to list all possible formats you can pull 
> > through this camera host. AFAIU, camera hosts can transfer data from 
> > cameras to destination (memory / framebuffer / output device...) in three 
> > ways:
> Oh yes, you should list them all : that's what you'll end up doing once the
> function format_supported() is fully implemented anyway, wouldn't you ?
> format_supported() will compare a list of known formats to the sensor output
> formats, and keep only known ones (ie. drop RGB32, or YUV 4:2:0, etc ...)

Don't you think we can have a default case? If the format requested by the 
user is provided by the camera and we "can trandfer it 1-to-1" (bus-width 
== depth or some such) then just switch on the pass-through mode?

> > Also, a probably better approach than what you suggested above (if I 
> > understood it right) would be not to use a static translation table, but 
> > to generate one dynamically during .add() and have them per-device, not 
> > per-host.
> The translation table will be per-host static (it's hardwired in the chip), but
> the generated supported formats will be dynamically generated by the host on
> sensor attachment (ie. computed), and for each device.
> 
> >> Is there a reason you chose to fully export the formats computation to hosts ?
> >
> > In short: I'd prefer to first keep this in pxa-camera, and then see as new 
> > host drivers arrive, whether we can make portions of the code generic. 
> > Makes sense?
> I understand your thinking. I don't think it's the good way to go, but you're
> the maintainer, you decide. We'll see soon enough, once TI and Qualcomm embedded
> devices will be enough documented, who was right.

Wow, I'm scared...:-) Ok, let's try it your way, I don't want to play the 
"maintainer" card, and you seem to be strongly in favour of a central 
solution, whereas I just slightly incline towards local... So, either give 
me a few days, or feel free to code off - whichever you prefer.

If you do this, I think, best do something like

int soc_camera_host_register(struct soc_camera_host *ici)
{
...
	if (!ici->ops->enum_fmt)
		ici->ops->enum_fmt = soc_camera_enum_fmt;
...

etc. for any other methods you want to provide defaults for. Instead of 
exporting more functions and letting hosts do

int x_do_something(...)
{
	return soc_camera_do_something(...);
}

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Don't you think we can have a default case? If the format requested by the 
> user is provided by the camera and we "can trandfer it 1-to-1" (bus-width 
> == depth or some such) then just switch on the pass-through mode?
Yes, we should, you're perfectly right.
>
> Wow, I'm scared...:-) Ok, let's try it your way, I don't want to play the 
> "maintainer" card, and you seem to be strongly in favour of a central 
> solution, whereas I just slightly incline towards local... So, either give 
> me a few days, or feel free to code off - whichever you prefer.

Well, since I bring the burden, I'll bring in the solution too, that's fair.

What I can do is make the translation in pxa_camera, but generic enough so that
it's transplantation to soc_camera is only a matter of copy/paste (with a bit of
renaming).
That way, we'll have :
 - the local model holding : no centralization in soc_camera
 - if you see a wave of incoming host cameras copy pasting that bit of code,
 transplant that to soc_camera.
 - tradeoff between your model and my model.

Do you like that approach ? If you prefer the soc_camera one (translation code
in soc_camera), I'll put it in there, just tell me.

> If you do this, I think, best do something like
>
> int soc_camera_host_register(struct soc_camera_host *ici)
> {
> ...
> 	if (!ici->ops->enum_fmt)
> 		ici->ops->enum_fmt = soc_camera_enum_fmt;
> ...
>
> etc. for any other methods you want to provide defaults for. Instead of 
> exporting more functions and letting hosts do
>
> int x_do_something(...)
> {
> 	return soc_camera_do_something(...);
> }
Yep. Sounds less poluting to kernel namespace.

OK, now I'm on the coding path, let's see what futur brings :)

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 5/5] pxa-camera: framework to handle camera-native and synthesized formats

[Guennadi Liakhovetski]

On Tue, 11 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Don't you think we can have a default case? If the format requested by the 
> > user is provided by the camera and we "can trandfer it 1-to-1" (bus-width 
> > == depth or some such) then just switch on the pass-through mode?
> Yes, we should, you're perfectly right.

Which means we cannot have your suggested static list of host-camera 
formats, right?

> > Wow, I'm scared...:-) Ok, let's try it your way, I don't want to play the 
> > "maintainer" card, and you seem to be strongly in favour of a central 
> > solution, whereas I just slightly incline towards local... So, either give 
> > me a few days, or feel free to code off - whichever you prefer.
> 
> Well, since I bring the burden, I'll bring in the solution too, that's fair.
> 
> What I can do is make the translation in pxa_camera, but generic enough so that
> it's transplantation to soc_camera is only a matter of copy/paste (with a bit of
> renaming).
> That way, we'll have :
>  - the local model holding : no centralization in soc_camera
>  - if you see a wave of incoming host cameras copy pasting that bit of code,
>  transplant that to soc_camera.
>  - tradeoff between your model and my model.
> 
> Do you like that approach ? If you prefer the soc_camera one (translation code
> in soc_camera), I'll put it in there, just tell me.

Emn, no, what you're describing is what I have done in my patch and what 
you didn't like - a solution local to pxa-camera but generic enough to be 
easily converted to a central one... My understanding was, that you wanted 
a central solution straight away, I agreed, and now you're looking for 
compromises?...:-) hm, no, please don't. You win, I lose, please code it 
for soc_camera.c.

> > If you do this, I think, best do something like
> >
> > int soc_camera_host_register(struct soc_camera_host *ici)
> > {
> > ...
> > 	if (!ici->ops->enum_fmt)
> > 		ici->ops->enum_fmt = soc_camera_enum_fmt;
> > ...
> >
> > etc. for any other methods you want to provide defaults for. Instead of 
> > exporting more functions and letting hosts do
> >
> > int x_do_something(...)
> > {
> > 	return soc_camera_do_something(...);
> > }
> Yep. Sounds less poluting to kernel namespace.
> 
> OK, now I'm on the coding path, let's see what futur brings :)

Looking forward:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 0/5] pixel format handling in camera host drivers - part 2

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> These patches should finish the necessary preparations for the pxa-camera 
> driver to finally correctly present its planar YUV format and to be able 
> to select camera formats, it actually can support, and perform further 
> format conversions as they emerge.

Hi Guennadi,

Would you tell me against what tree you're based (a git URL would be wonderful)
?  Because I got rejects, having not the ov7272.c file in my git tree
(mainline), which would mean you're ahead and I'm late ...

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH 0/5] pixel format handling in camera host drivers - part 2

[Guennadi Liakhovetski]

Hi Robert,

On Mon, 10 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > These patches should finish the necessary preparations for the pxa-camera 
> > driver to finally correctly present its planar YUV format and to be able 
> > to select camera formats, it actually can support, and perform further 
> > format conversions as they emerge.
> 
> Hi Guennadi,
> 
> Would you tell me against what tree you're based (a git URL would be wonderful)
> ?  Because I got rejects, having not the ov7272.c file in my git tree
> (mainline), which would mean you're ahead and I'm late ...

My tree is still based on commit 67d112842586aa11506b7a8afec29391bf8f3cca 
in linux-next of 9 days ago, but ov772x wasn't there yet, so, I 
cherry-picked it. But you can try a more recent linux-next, ov722x is 
already there and even with two commits!:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Moderators: please act (was Re: [PATCH 3/5] soc-camera: add a per-camera...)

[Guennadi Liakhovetski]

This message "awaits moderator approval" since 34 hours already, can it be 
posted, please?

Thanks
Guennadi

On Mon, 10 Nov 2008, Guennadi Liakhovetski wrote:

> This pointer will be used by pxa_camera.c to point to its pixel format 
> data.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  include/media/soc_camera.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 9c3734c..a63f7fb 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -42,6 +42,7 @@ struct soc_camera_device {
>  	const struct soc_camera_data_format *formats;
>  	int num_formats;
>  	struct module *owner;
> +	void *host_priv;		/* per-device host private data */
>  	/* soc_camera.c private count. Only accessed with video_lock held */
>  	int use_count;
>  };
> -- 
> 1.5.4
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list