On Mon, 14 Dec 2009, Jonathan Cameron wrote:
> Hi All,
>
> >> 3) it would be interesting to patch the other sensor drivers to be
> >> compatible
> >> with soc_camera (mt9v011/ov7670);
> >
> > Well, this could be done, yes, but does it make sense to do this blindly
> > without any hardware to test? I would rather add such conversions on a
> > one-by-one basis as need arises.
> >
> I'm working on the ov7670. I've added the media bus stuff to what I've
> previously posted but I haven't tested as yet. For reference, a quick and
> dirty
> cut of the patch is below. Some thought is needed on how to deal with the
> sections
> that currently need to be different for the soc-camera and non soc camera
> uses.
> (mainly the registration code in probe).
Look at my patch for mt9t031. First you do not have to fail if
client->dev.platform_data == NULL. You have to look at the bridge driver,
that's currently working with ov7670. If it is not setting platform_data,
you can use it to detect whether you're in soc-camera environment or not.
Otherwise, if the bridge driver were also using that pointer, we would
have to come up with a ov7670-specific struct to be linked from that
pointer. I was actually thinking about a way to pass soc-camera data to
client drivers in a way, that would not imped non soc-camera use. So, how
about:
I soc-camera environment
1. platform defines a
static struct ov7670_platform_data ov7670 = {
.link = &ov7670_link,
};
2. and a
static struct soc_camera_link ov7670_link = {
.client_data = &ov7670,
};
3. soc-camera core gets &ov7670_link in its probe, same as this is done
now, then it does
icl->icd = icd;
icl->board_info->platform_data = icl->client_data;
subdev = v4l2_i2c_new_subdev_board(&ici->v4l2_dev, adap,
icl->module_name, icl->board_info, NULL);
4. ov7670 probe is called with client platform data pointing to its own
data:
static int ov7670_probe(struct i2c_client *client,
const struct i2c_device_id *did)
{
struct ov7670_platform_data *pdata = client->dev.platform_data;
struct soc_camera_link *icl = pdata->link;
struct soc_camera_device *icd;
if (icl)
icd = icl->icd;
II Non soc-camera environment
1. the bridge driver fills in sensor data and does
struct ov7670_platform_data *ov7670_pdata = kzalloc(sizeof(*pdata));
ov7670_pdata->... = ...;
board_info->platform_data = ov7670_pdata;
subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adap,
module_name, board_info, NULL);
2. ov7670_probe() gets called and finds its data as above, but without an
soc-camera link this time.
The advantage of the above is, that each client driver is free to define
its own platform data struct, and for soc-camera it only has to provide
one pointer in it. The ugly side is the cross-referencing between I.1. and
I.2. above.
Alternatively we could agree, that all client drivers get in their i2c
client platform data a standard struct with only pointers in it to client
data and to various bridging models:
struct v4l2_client {
void *client;
struct soc_camera_link *icl;
struct int_device_link *int;
...
};
or a bit more hidden
struct v4l2_client {
void *client;
void *bridge_data;
enum V4L2_BRIDGE_TYPE bridge_type;
};
> On another note, does anyone have an objection to making the set_bus_param
> function optional? At the moment we are adding null functions for those
> devices that can't actually change anything which seems a little pointless.
Yes, it can.
>
> Jonathan
>
>
> diff --git a/drivers/media/video/ov7670.c b/drivers/media/video/ov7670.c
> index 0e2184e..e57c3b5 100644
> --- a/drivers/media/video/ov7670.c
> +++ b/drivers/media/video/ov7670.c
> @@ -18,6 +18,7 @@
> #include <media/v4l2-device.h>
> #include <media/v4l2-chip-ident.h>
> #include <media/v4l2-i2c-drv.h>
> +#include <media/soc_camera.h>
>
>
> MODULE_AUTHOR("Jonathan Corbet <cor...@lwn.net>");
> @@ -545,7 +546,7 @@ static struct ov7670_format_struct {
> .bpp = 1
> },
> };
> -#define N_OV7670_FMTS ARRAY_SIZE(ov7670_formats)
> +
>
>
> /*
> @@ -668,52 +669,37 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int
> hstart, int hstop,
> return ret;
> }
>
> +static enum v4l2_mbus_pixelcode ov7670_codes[] = {
> + V4L2_MBUS_FMT_YUYV8_2X8,
> + V4L2_MBUS_FMT_RGB565_2X8_LE,
> +};
>
> -static int ov7670_enum_fmt(struct v4l2_subdev *sd, struct v4l2_fmtdesc *fmt)
> +static int ov7670_enum_fmt(struct v4l2_subdev *sd, int index, enum
> v4l2_mbus_pixelcode *code)
> {
> - struct ov7670_format_struct *ofmt;
> -
> - if (fmt->index >= N_OV7670_FMTS)
> + if (index >= ARRAY_SIZE(ov7670_codes))
> return -EINVAL;
>
> - ofmt = ov7670_formats + fmt->index;
> - fmt->flags = 0;
> - strcpy(fmt->description, ofmt->desc);
> - fmt->pixelformat = ofmt->pixelformat;
> + *code = ov7670_codes[index];
> return 0;
> }
>
>
> static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
> - struct v4l2_format *fmt,
> - struct ov7670_format_struct **ret_fmt,
> - struct ov7670_win_size **ret_wsize)
> + struct v4l2_mbus_framefmt *mf,
> + struct ov7670_format_struct **ret_fmt,
> + struct ov7670_win_size **ret_wsize)
> {
> int index;
> struct ov7670_win_size *wsize;
> - struct v4l2_pix_format *pix = &fmt->fmt.pix;
>
> - for (index = 0; index < N_OV7670_FMTS; index++)
> - if (ov7670_formats[index].pixelformat == pix->pixelformat)
> - break;
> - if (index >= N_OV7670_FMTS) {
> - /* default to first format */
> - index = 0;
> - pix->pixelformat = ov7670_formats[0].pixelformat;
> - }
> - if (ret_fmt != NULL)
> - *ret_fmt = ov7670_formats + index;
> - /*
> - * Fields: the OV devices claim to be progressive.
> - */
> - pix->field = V4L2_FIELD_NONE;
> + mf->field = V4L2_FIELD_NONE;
> /*
> * Round requested image size down to the nearest
> * we support, but not below the smallest.
> */
> for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes + N_WIN_SIZES;
> wsize++)
> - if (pix->width >= wsize->width && pix->height >= wsize->height)
> + if ( mf->width >= wsize->width && mf->height >= wsize->height)
> break;
> if (wsize >= ov7670_win_sizes + N_WIN_SIZES)
> wsize--; /* Take the smallest one */
> @@ -722,22 +708,34 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev
> *sd,
> /*
> * Note the size we'll actually handle.
> */
> - pix->width = wsize->width;
> - pix->height = wsize->height;
> - pix->bytesperline = pix->width*ov7670_formats[index].bpp;
> - pix->sizeimage = pix->height*pix->bytesperline;
> + mf->width = wsize->width;
> + mf->height = wsize->height;
> + switch (mf->code) {
> + case V4L2_MBUS_FMT_RGB565_2X8_LE:
> + mf->colorspace = V4L2_COLORSPACE_SRGB;
> + if (ret_fmt != NULL)
> + *ret_fmt = ov7670_formats + 2;
> + break;
> + default:
> + mf->code = V4L2_MBUS_FMT_YUYV8_2X8;
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + mf->colorspace = V4L2_COLORSPACE_JPEG;
> + if (ret_fmt != NULL)
> + *ret_fmt = ov7670_formats;
> + break;
> + }
> return 0;
> }
>
> -static int ov7670_try_fmt(struct v4l2_subdev *sd, struct v4l2_format *fmt)
> +static int ov7670_try_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt
> *mf)
> {
> - return ov7670_try_fmt_internal(sd, fmt, NULL, NULL);
> + return ov7670_try_fmt_internal(sd, mf, NULL, NULL);
> }
>
> /*
> * Set a format.
> */
> -static int ov7670_s_fmt(struct v4l2_subdev *sd, struct v4l2_format *fmt)
> +static int ov7670_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt
> *mf)
> {
> int ret;
> struct ov7670_format_struct *ovfmt;
> @@ -745,7 +743,11 @@ static int ov7670_s_fmt(struct v4l2_subdev *sd, struct
> v4l2_format *fmt)
> struct ov7670_info *info = to_state(sd);
> unsigned char com7, clkrc = 0;
>
> - ret = ov7670_try_fmt_internal(sd, fmt, &ovfmt, &wsize);
> + ret = ov7670_init(sd, 0);
> + if (ret)
> + return ret;
> +
> + ret = ov7670_try_fmt_internal(sd, mf, &ovfmt, &wsize);
> if (ret)
> return ret;
> /*
> @@ -753,7 +755,7 @@ static int ov7670_s_fmt(struct v4l2_subdev *sd, struct
> v4l2_format *fmt)
> * CLKRC. If we're *not*, however, then rewriting clkrc hoses
> * the colors.
> */
> - if (fmt->fmt.pix.pixelformat == V4L2_PIX_FMT_RGB565) {
> + if (mf->code == V4L2_MBUS_FMT_RGB565_2X8_LE) {
> ret = ov7670_read(sd, REG_CLKRC, &clkrc);
> if (ret)
> return ret;
> @@ -778,7 +780,7 @@ static int ov7670_s_fmt(struct v4l2_subdev *sd, struct
> v4l2_format *fmt)
> ret = ov7670_write_array(sd, wsize->regs);
> info->fmt = ovfmt;
>
> - if (fmt->fmt.pix.pixelformat == V4L2_PIX_FMT_RGB565 && ret == 0)
> + if (mf->code == V4L2_MBUS_FMT_RGB565_2X8_LE && ret == 0)
> ret = ov7670_write(sd, REG_CLKRC, clkrc);
> return ret;
> }
> @@ -1226,11 +1228,11 @@ static const struct v4l2_subdev_core_ops
> ov7670_core_ops = {
> };
>
> static const struct v4l2_subdev_video_ops ov7670_video_ops = {
> - .enum_fmt = ov7670_enum_fmt,
> - .try_fmt = ov7670_try_fmt,
> - .s_fmt = ov7670_s_fmt,
> .s_parm = ov7670_s_parm,
> .g_parm = ov7670_g_parm,
> + .s_mbus_fmt = ov7670_s_fmt,
> + .try_mbus_fmt = ov7670_try_fmt,
> + .enum_mbus_fmt = ov7670_enum_fmt,
> };
>
> static const struct v4l2_subdev_ops ov7670_ops = {
> @@ -1239,6 +1241,28 @@ static const struct v4l2_subdev_ops ov7670_ops = {
> };
>
> /* ----------------------------------------------------------------------- */
> +static unsigned long ov7670_soc_query_bus_param(struct soc_camera_device
> *icd)
> +{
> + struct soc_camera_link *icl = to_soc_camera_link(icd);
> +
> + unsigned long flags = SOCAM_PCLK_SAMPLE_RISING | SOCAM_MASTER |
> + SOCAM_VSYNC_ACTIVE_HIGH | SOCAM_HSYNC_ACTIVE_HIGH |
> + SOCAM_DATAWIDTH_8 | SOCAM_DATA_ACTIVE_HIGH;
> +
> + return soc_camera_apply_sensor_flags(icl, flags);
> +}
> +
> +/* This device only supports one bus option */
> +static int ov7670_soc_set_bus_param(struct soc_camera_device *icd,
> + unsigned long flags)
> +{
> + return 0;
> +}
> +
> +static struct soc_camera_ops ov7670_soc_ops = {
> + .set_bus_param = ov7670_soc_set_bus_param,
> + .query_bus_param = ov7670_soc_query_bus_param,
> +};
>
> static int ov7670_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> @@ -1246,6 +1270,12 @@ static int ov7670_probe(struct i2c_client *client,
> struct v4l2_subdev *sd;
> struct ov7670_info *info;
> int ret;
> + struct soc_camera_device *icd = client->dev.platform_data;
> + if (!icd) {
> + dev_err(&client->dev, "OV7670: missing soc-camera data!\n");
> + return -EINVAL;
> + }
> + icd->ops = &ov7670_soc_ops;
>
> info = kzalloc(sizeof(struct ov7670_info), GFP_KERNEL);
> if (info == NULL)
>
---
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
