Re: [PATCH 2/2 v2] soc-camera: convert to the new mediabus API
On Monday 30 November 2009 15:48:24 Guennadi Liakhovetski wrote:
On Mon, 30 Nov 2009, Hans Verkuil wrote:
On Fri, 27 Nov 2009, Guennadi Liakhovetski wrote:
On Fri, 27 Nov 2009, Hans Verkuil wrote:
Hi Guennadi,
Convert soc-camera core and all soc-camera drivers to the new
mediabus API. This also takes soc-camera client drivers one step
closer to also be
usable with generic v4l2-subdev host drivers.
Just a quick question:
@@ -323,28 +309,39 @@ static int mt9m001_s_fmt(struct v4l2_subdev
*sd, struct v4l2_format *f)
/* No support for scaling so far, just crop. TODO: use skipping
*/ ret = mt9m001_s_crop(sd, a);
if (!ret) {
- pix-width = mt9m001-rect.width;
- pix-height = mt9m001-rect.height;
- mt9m001-fourcc = pix-pixelformat;
+ mf-width = mt9m001-rect.width;
+ mf-height = mt9m001-rect.height;
+ mt9m001-fmt= soc_mbus_find_datafmt(mf-code,
+ mt9m001-fmts,
mt9m001-num_fmts);
+ mf-colorspace = mt9m001-fmt-colorspace;
}
return ret;
}
-static int mt9m001_try_fmt(struct v4l2_subdev *sd, struct
v4l2_format *f)
+static int mt9m001_try_fmt(struct v4l2_subdev *sd,
+struct v4l2_mbus_framefmt *mf)
{
struct i2c_client *client = sd-priv;
struct mt9m001 *mt9m001 = to_mt9m001(client);
- struct v4l2_pix_format *pix = f-fmt.pix;
+ const struct soc_mbus_datafmt *fmt;
- v4l_bound_align_image(pix-width, MT9M001_MIN_WIDTH,
+ v4l_bound_align_image(mf-width, MT9M001_MIN_WIDTH,
MT9M001_MAX_WIDTH, 1,
- pix-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
+ mf-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
MT9M001_MAX_HEIGHT + mt9m001-y_skip_top, 0, 0);
- if (pix-pixelformat == V4L2_PIX_FMT_SBGGR8 ||
- pix-pixelformat == V4L2_PIX_FMT_SBGGR16)
- pix-height = ALIGN(pix-height - 1, 2);
+ if (mt9m001-fmts == mt9m001_colour_fmts)
+ mf-height = ALIGN(mf-height - 1, 2);
+
+ fmt = soc_mbus_find_datafmt(mf-code, mt9m001-fmts,
+ mt9m001-num_fmts);
+ if (!fmt) {
+ fmt = mt9m001-fmt;
+ mf-code = fmt-code;
+ }
+
+ mf-colorspace = fmt-colorspace;
return 0;
}
Why do the sensor drivers use soc_mbus_find_datafmt? They only seem
to be interested in the colorspace field, but I don't see the reason
for that. Most if not all sensors have a fixed colorspace depending
on the pixelcode, so they can just ignore the colorspace that the
caller requested and replace it with their own.
Right, that's exactly what's done here. mt9m001 and mt9v022 drivers
support different formats, depending on the exact detected or specified
by the user model. That's why they have to search for the requested
format in supported list. and then - yes, they just put the found
format into user
request:
+ mf-colorspace = fmt-colorspace;
I didn't have time for a full review, so I might have missed
something.
I looked at this more closely and I realized that I did indeed miss that
soc_mbus_find_datafmt just searched in the pixelcode - colorspace
mapping array.
I also realized that there is no need for that data structure and
function to be soc-camera specific. I believe I said otherwise in an
earlier review. My apologies for that, all I can say is that I had very
little time to do the reviews...
No, you did not say otherwise about _these_ struct and function - they
only appeared in v2 of the mediabus API, after you'd suggested to move
colorspace into struct v4l2_mbus_framefmt.
That said, there is no need for both the soc_mbus_datafmt struct and the
soc_mbus_find_datafmt function. These can easily be replaced by something
like this as a local function in each subdev:
static enum v4l2_colorspace mt9m111_g_colorspace(enum v4l2_mbus_pixelcode
code)
{
switch (code) {
case V4L2_MBUS_FMT_YUYV:
case V4L2_MBUS_FMT_YVYU:
case V4L2_MBUS_FMT_UYVY:
case V4L2_MBUS_FMT_VYUY:
return V4L2_COLORSPACE_JPEG;
case V4L2_MBUS_FMT_RGB555:
case V4L2_MBUS_FMT_RGB565:
case V4L2_MBUS_FMT_SBGGR8:
case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
return V4L2_COLORSPACE_SRGB;
default:
return 0;
}
}
So if mt9m111_g_colorspace() returns 0, then the format wasn't found.
(Note that the compiler might give a warning for the return 0, so that
might need some editing)
Much simpler and much easier to understand.
Drivers are
Re: [PATCH 2/2 v2] soc-camera: convert to the new mediabus API
On Tue, 1 Dec 2009, Hans Verkuil wrote:
On Monday 30 November 2009 15:48:24 Guennadi Liakhovetski wrote:
On Mon, 30 Nov 2009, Hans Verkuil wrote:
On Fri, 27 Nov 2009, Guennadi Liakhovetski wrote:
On Fri, 27 Nov 2009, Hans Verkuil wrote:
Hi Guennadi,
Convert soc-camera core and all soc-camera drivers to the new
mediabus API. This also takes soc-camera client drivers one step
closer to also be
usable with generic v4l2-subdev host drivers.
Just a quick question:
@@ -323,28 +309,39 @@ static int mt9m001_s_fmt(struct v4l2_subdev
*sd, struct v4l2_format *f)
/* No support for scaling so far, just crop. TODO: use skipping
*/ ret = mt9m001_s_crop(sd, a);
if (!ret) {
- pix-width = mt9m001-rect.width;
- pix-height = mt9m001-rect.height;
- mt9m001-fourcc = pix-pixelformat;
+ mf-width = mt9m001-rect.width;
+ mf-height = mt9m001-rect.height;
+ mt9m001-fmt= soc_mbus_find_datafmt(mf-code,
+ mt9m001-fmts,
mt9m001-num_fmts);
+ mf-colorspace = mt9m001-fmt-colorspace;
}
return ret;
}
-static int mt9m001_try_fmt(struct v4l2_subdev *sd, struct
v4l2_format *f)
+static int mt9m001_try_fmt(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *mf)
{
struct i2c_client *client = sd-priv;
struct mt9m001 *mt9m001 = to_mt9m001(client);
- struct v4l2_pix_format *pix = f-fmt.pix;
+ const struct soc_mbus_datafmt *fmt;
- v4l_bound_align_image(pix-width, MT9M001_MIN_WIDTH,
+ v4l_bound_align_image(mf-width, MT9M001_MIN_WIDTH,
MT9M001_MAX_WIDTH, 1,
- pix-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
+ mf-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
MT9M001_MAX_HEIGHT + mt9m001-y_skip_top, 0, 0);
- if (pix-pixelformat == V4L2_PIX_FMT_SBGGR8 ||
- pix-pixelformat == V4L2_PIX_FMT_SBGGR16)
- pix-height = ALIGN(pix-height - 1, 2);
+ if (mt9m001-fmts == mt9m001_colour_fmts)
+ mf-height = ALIGN(mf-height - 1, 2);
+
+ fmt = soc_mbus_find_datafmt(mf-code, mt9m001-fmts,
+ mt9m001-num_fmts);
+ if (!fmt) {
+ fmt = mt9m001-fmt;
+ mf-code = fmt-code;
+ }
+
+ mf-colorspace = fmt-colorspace;
return 0;
}
Why do the sensor drivers use soc_mbus_find_datafmt? They only seem
to be interested in the colorspace field, but I don't see the reason
for that. Most if not all sensors have a fixed colorspace depending
on the pixelcode, so they can just ignore the colorspace that the
caller requested and replace it with their own.
Right, that's exactly what's done here. mt9m001 and mt9v022 drivers
support different formats, depending on the exact detected or specified
by the user model. That's why they have to search for the requested
format in supported list. and then - yes, they just put the found
format into user
request:
+ mf-colorspace = fmt-colorspace;
I didn't have time for a full review, so I might have missed
something.
I looked at this more closely and I realized that I did indeed miss that
soc_mbus_find_datafmt just searched in the pixelcode - colorspace
mapping array.
I also realized that there is no need for that data structure and
function to be soc-camera specific. I believe I said otherwise in an
earlier review. My apologies for that, all I can say is that I had very
little time to do the reviews...
No, you did not say otherwise about _these_ struct and function - they
only appeared in v2 of the mediabus API, after you'd suggested to move
colorspace into struct v4l2_mbus_framefmt.
That said, there is no need for both the soc_mbus_datafmt struct and the
soc_mbus_find_datafmt function. These can easily be replaced by something
like this as a local function in each subdev:
static enum v4l2_colorspace mt9m111_g_colorspace(enum v4l2_mbus_pixelcode
code)
{
switch (code) {
case V4L2_MBUS_FMT_YUYV:
case V4L2_MBUS_FMT_YVYU:
case V4L2_MBUS_FMT_UYVY:
case V4L2_MBUS_FMT_VYUY:
return V4L2_COLORSPACE_JPEG;
case V4L2_MBUS_FMT_RGB555:
case V4L2_MBUS_FMT_RGB565:
case V4L2_MBUS_FMT_SBGGR8:
case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
return V4L2_COLORSPACE_SRGB;
default:
return 0;
}
}
So if mt9m111_g_colorspace() returns 0, then the format wasn't found.
(Note that the compiler might give a warning for the return 0, so that
might need some
Re: [PATCH 2/2 v2] soc-camera: convert to the new mediabus API
Hi Guennadi,
Convert soc-camera core and all soc-camera drivers to the new mediabus
API. This also takes soc-camera client drivers one step closer to also be
usable with generic v4l2-subdev host drivers.
Just a quick question:
@@ -323,28 +309,39 @@ static int mt9m001_s_fmt(struct v4l2_subdev *sd,
struct v4l2_format *f)
/* No support for scaling so far, just crop. TODO: use skipping */
ret = mt9m001_s_crop(sd, a);
if (!ret) {
- pix-width = mt9m001-rect.width;
- pix-height = mt9m001-rect.height;
- mt9m001-fourcc = pix-pixelformat;
+ mf-width = mt9m001-rect.width;
+ mf-height = mt9m001-rect.height;
+ mt9m001-fmt= soc_mbus_find_datafmt(mf-code,
+ mt9m001-fmts, mt9m001-num_fmts);
+ mf-colorspace = mt9m001-fmt-colorspace;
}
return ret;
}
-static int mt9m001_try_fmt(struct v4l2_subdev *sd, struct v4l2_format *f)
+static int mt9m001_try_fmt(struct v4l2_subdev *sd,
+struct v4l2_mbus_framefmt *mf)
{
struct i2c_client *client = sd-priv;
struct mt9m001 *mt9m001 = to_mt9m001(client);
- struct v4l2_pix_format *pix = f-fmt.pix;
+ const struct soc_mbus_datafmt *fmt;
- v4l_bound_align_image(pix-width, MT9M001_MIN_WIDTH,
+ v4l_bound_align_image(mf-width, MT9M001_MIN_WIDTH,
MT9M001_MAX_WIDTH, 1,
- pix-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
+ mf-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
MT9M001_MAX_HEIGHT + mt9m001-y_skip_top, 0, 0);
- if (pix-pixelformat == V4L2_PIX_FMT_SBGGR8 ||
- pix-pixelformat == V4L2_PIX_FMT_SBGGR16)
- pix-height = ALIGN(pix-height - 1, 2);
+ if (mt9m001-fmts == mt9m001_colour_fmts)
+ mf-height = ALIGN(mf-height - 1, 2);
+
+ fmt = soc_mbus_find_datafmt(mf-code, mt9m001-fmts,
+ mt9m001-num_fmts);
+ if (!fmt) {
+ fmt = mt9m001-fmt;
+ mf-code = fmt-code;
+ }
+
+ mf-colorspace = fmt-colorspace;
return 0;
}
Why do the sensor drivers use soc_mbus_find_datafmt? They only seem to be
interested in the colorspace field, but I don't see the reason for that.
Most if not all sensors have a fixed colorspace depending on the
pixelcode, so they can just ignore the colorspace that the caller
requested and replace it with their own.
I didn't have time for a full review, so I might have missed something.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] soc-camera: convert to the new mediabus API
On Fri, 27 Nov 2009, Hans Verkuil wrote:
Hi Guennadi,
Convert soc-camera core and all soc-camera drivers to the new mediabus
API. This also takes soc-camera client drivers one step closer to also be
usable with generic v4l2-subdev host drivers.
Just a quick question:
@@ -323,28 +309,39 @@ static int mt9m001_s_fmt(struct v4l2_subdev *sd,
struct v4l2_format *f)
/* No support for scaling so far, just crop. TODO: use skipping */
ret = mt9m001_s_crop(sd, a);
if (!ret) {
- pix-width = mt9m001-rect.width;
- pix-height = mt9m001-rect.height;
- mt9m001-fourcc = pix-pixelformat;
+ mf-width = mt9m001-rect.width;
+ mf-height = mt9m001-rect.height;
+ mt9m001-fmt= soc_mbus_find_datafmt(mf-code,
+ mt9m001-fmts, mt9m001-num_fmts);
+ mf-colorspace = mt9m001-fmt-colorspace;
}
return ret;
}
-static int mt9m001_try_fmt(struct v4l2_subdev *sd, struct v4l2_format *f)
+static int mt9m001_try_fmt(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *mf)
{
struct i2c_client *client = sd-priv;
struct mt9m001 *mt9m001 = to_mt9m001(client);
- struct v4l2_pix_format *pix = f-fmt.pix;
+ const struct soc_mbus_datafmt *fmt;
- v4l_bound_align_image(pix-width, MT9M001_MIN_WIDTH,
+ v4l_bound_align_image(mf-width, MT9M001_MIN_WIDTH,
MT9M001_MAX_WIDTH, 1,
- pix-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
+ mf-height, MT9M001_MIN_HEIGHT + mt9m001-y_skip_top,
MT9M001_MAX_HEIGHT + mt9m001-y_skip_top, 0, 0);
- if (pix-pixelformat == V4L2_PIX_FMT_SBGGR8 ||
- pix-pixelformat == V4L2_PIX_FMT_SBGGR16)
- pix-height = ALIGN(pix-height - 1, 2);
+ if (mt9m001-fmts == mt9m001_colour_fmts)
+ mf-height = ALIGN(mf-height - 1, 2);
+
+ fmt = soc_mbus_find_datafmt(mf-code, mt9m001-fmts,
+ mt9m001-num_fmts);
+ if (!fmt) {
+ fmt = mt9m001-fmt;
+ mf-code = fmt-code;
+ }
+
+ mf-colorspace = fmt-colorspace;
return 0;
}
Why do the sensor drivers use soc_mbus_find_datafmt? They only seem to be
interested in the colorspace field, but I don't see the reason for that.
Most if not all sensors have a fixed colorspace depending on the
pixelcode, so they can just ignore the colorspace that the caller
requested and replace it with their own.
Right, that's exactly what's done here. mt9m001 and mt9v022 drivers
support different formats, depending on the exact detected or specified by
the user model. That's why they have to search for the requested format in
supported list. and then - yes, they just put the found format into user
request:
+ mf-colorspace = fmt-colorspace;
I didn't have time for a full review, so I might have missed something.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
