Hi Mauro, thank you for your interest. On 13 September 2012 15:00, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> wrote: > Hi Javier, > > I'm not too familiar with soc_camera and ov7670 drivers, so my comments > reflects my understanding of the question, without taking into account > drivers specifics. > > Em 13-09-2012 06:48, javier Martin escreveu: >> Hi, >> our new i.MX27 based platform (Visstrim-SM20) uses an ov7675 sensor >> attached to the CSI interface. Apparently, this sensor is fully >> compatible with the old ov7670. For this reason, it seems rather >> sensible that they should share the same driver: ov7670.c >> One of the challenges we have to face is that capture video support >> for our platform is mx2_camera.c, which is a soc-camera host driver; >> while ov7670.c was developed for being used as part of a more complex >> video card. >> >> Here is the list of current users of ov7670: >> >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/ov519.c >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/sn9c20x.c >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/gspca/vc032x.c >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/via-camera.c >> http://lxr.linux.no/#linux+v3.5.3/drivers/media/video/marvell-ccic/mcam-core.c > > In order to avoid breakages on those drivers, we need to be sure that > none of the changes will alter the register settings used there. > > (C/C Hans de Goede, as he is the gspca maintainer) > >> >> These are basically the improvements we need to make to this driver in >> order to satisfy our needs: >> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a >> proper ctrl handler, etc... >> 2.- Add the possibility to bypass PLL and clkrc preescaler. >> 3.- Adjust vstart/vstop in order to remove an horizontal green line. >> 4.- Disable pixclk during horizontal blanking. >> 5.- min_height, min_width should be respected in try_fmt(). >> 6.- Pass platform data when used with a soc-camera host driver. >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl. > > Doing one patch per change helps to review the changes individually. > I suspect that it will needed to be tested with the above drivers, > anyway. > >> I will try to summarize below why we need to accomplish each of the >> previous tasks and what solution we propose for them: >> >> 1.- Adapt v4l2 controls to the subvevice control framework, with a >> proper ctrl handler, etc... >> >> Why? Because soc-camera needs to inherit v4l2 subdevice controls in >> order to expose them to user space. >> How? Something like the following, incomplete, patch: >> >> --- >> @@ -190,6 +196,7 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)"); >> struct ov7670_format_struct; /* coming later */ >> struct ov7670_info { >> struct v4l2_subdev sd; >> + struct v4l2_ctrl_handler hdl; >> struct ov7670_format_struct *fmt; /* Current format */ >> unsigned char sat; /* Saturation value */ >> int hue; /* Hue value */ >> >> >> @@ -1480,10 +1518,14 @@ static int ov7670_s_register(struct >> v4l2_subdev *sd, struct v4l2_dbg_register *r >> >> /* ----------------------------------------------------------------------- */ >> >> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = { >> + .s_ctrl = ov7670_s_ctrl, >> +}; >> + >> static const struct v4l2_subdev_core_ops ov7670_core_ops = { >> .g_chip_ident = ov7670_g_chip_ident, >> - .g_ctrl = ov7670_g_ctrl, >> - .s_ctrl = ov7670_s_ctrl, >> + .g_ctrl = v4l2_subdev_g_ctrl, >> + .s_ctrl = v4l2_subdev_s_ctrl, >> .queryctrl = ov7670_queryctrl, >> .reset = ov7670_reset, >> .init = ov7670_init, >> >> @@ -1551,6 +1600,16 @@ static int ov7670_probe(struct i2c_client *client, >> v4l_info(client, "chip found @ 0x%02x (%s)\n", >> client->addr << 1, client->adapter->name); >> >> + v4l2_ctrl_handler_init(&info->hdl, 1); >> + v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops, >> V4L2_CID_VFLIP, 0, 1, 1, 0); >> ... >> ... >> + sd->ctrl_handler = &info->hdl; >> + if (info->hdl.error) { >> + v4l2_ctrl_handler_free(&info->hdl); >> + kfree(info); >> + return info->hdl.error; >> + } >> + v4l2_ctrl_handler_setup(&info->hdl); >> + >> --- > > Tests are required here, but I don't think this would break anything. >> >> 2.- Add the possibility to bypass PLL and clkrc preescaler. >> >> Why? The formula to get the desired frame rate in this chip in YUV is >> the following: fps = fpclk / (2 * 510 * 784) This means that for a >> desired fps = 30 we need fpclk = 24MHz. For that reason we have a >> clean 24MHz xvclk input that comes from an oscillator. If we enable >> the PLL it internally transforms the 24MHz in 22MHz and thus fps is >> not 30 but 27. In order to get 30fps we need to bypass the PLL. >> How? Defining a platform flag 'direct_clk' or similar that allows >> xvclk being used directly as the pixel clock. > > As this should be a new platform data, provided that the old behavior > is to use the old formula, this shouldn't break anything. > >> >> 3.- Adjust vstart/vstop in order to remove an horizontal green line. >> >> Why? Currently, in the driver, for VGA, vstart = 10 and vstop = 490. >>>From our tests we found out that vstart = 14, vstop = 494 in order to >> remove a disgusting horizontal green line in ov7675. >> How? It seems these sensor aren't provided with a version register or >> anything similar so I can't think of a clean solution for this yet. >> Suggestions will be much appreciated. > > If it is not possible to differentiate between ov7670 and ov7675, just > add a platform data flag, in order to identify the ov7675 > >> 4.- Disable pixclk during horizontal blanking. >> >> Why? Otherwise i.MX27 will capture wrong pixels during blanking periods. >> How? Through a private V4L2 control. > > Hmm... I'm assuming that there is a register that controls pixclk disable > during horizontal blanking. In this case, the better is to add support for > it also via platform data. > >> >> 5.- min_height, min_width should be respected in try_fmt(). >> Why? Otherwise you are telling the user you are going to use a >> different size than the one you are going to use. >> How? With a patch similar to this: >> >> --- >> @@ -759,8 +772,10 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd, >> struct ov7670_format_struct **ret_fmt, >> struct ov7670_win_size **ret_wsize) >> { >> - int index; >> + int index, i; >> + int win_sizes_limit = N_WIN_SIZES; >> struct ov7670_win_size *wsize; >> + struct ov7670_info *info = to_state(sd); >> >> for (index = 0; index < N_OV7670_FMTS; index++) >> if (ov7670_formats[index].mbus_code == fmt->code) >> @@ -776,15 +791,30 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd, >> * Fields: the OV devices claim to be progressive. >> */ >> fmt->field = V4L2_FIELD_NONE; >> + >> + /* >> + * Don't consider values that don't match min_height and min_width >> + * constraints. >> + */ >> + if (info->min_width || info->min_height) >> + for (i=0; i < N_WIN_SIZES; i++) { >> + wsize = ov7670_win_sizes + i; >> + >> + if (wsize->width < info->min_width || >> + wsize->height < info->min_height) { >> + win_sizes_limit = i; >> + break; >> + } >> + } >> /* >> * 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; >> + for (wsize = ov7670_win_sizes; wsize < ov7670_win_sizes + >> win_sizes_limit; >> wsize++) >> if (fmt->width >= wsize->width && fmt->height >= wsize->height) >> break; >> - if (wsize >= ov7670_win_sizes + N_WIN_SIZES) >> + if (wsize >= ov7670_win_sizes + win_sizes_limit) >> wsize--; /* Take the smallest one */ >> if (ret_wsize != NULL) >> *ret_wsize = wsize; > > Of course, patch need to be tested, but that change looks fine on my eyes. > >> --- >> >> 6.- Pass platform data when used with a soc-camera host driver. >> Why? We need to set several platform data like 'min_height', >> 'min_width' and others. >> How? This is an old subject we discussed in January. We agreed that >> some soc-camera core changes were needed, but I couldn't find the time >> and I think nobody else has addressed it either. Please, correct me if >> I am wrong:http://patchwork.linuxtv.org/patch/8860/ > > Hmm... patch 8860 is related to s_input logic, and not to pass platform data. I know but in the end we discussed passing platform data to tvp5150 sensor using soc-camera. This is a similar case for the ov7670. Here is the specific e-mail in the thread: http://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg41437.html > For sure passing platform data is needed, as different boards may require > different sensor configurations. If soc_camera doesn't support it yet, > this needs to be added there. > >> >> 7.- Add V4L2_CID_POWER_LINE_FREQUENCY ctrl. >> Why? Because the platform will be used in several countries. >> How? As long as point 1 is solved this is quite trivial. >> >> >> The reason of this e-mail is to discuss whether you find these >> solution suitable or not and, more important, whether you think the >> suggested changes could break existing drivers. >> >> Regards. >> > > Regards, > Mauro > -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
