| Message ID | 1348652877-25816-2-git-send-email-javier.martin@vista-silicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
This is going to have to be quick, sorry... On Wed, 26 Sep 2012 11:47:53 +0200 Javier Martin <javier.martin@vista-silicon.com> wrote: > +static struct ov7670_win_size ov7670_win_sizes[2][4] = { > + /* ov7670 */ I must confess I don't like this; now we've got constants in an array that was automatically sized before and ov7670_win_sizes[info->model] everywhere. I'd suggest a separate array for each device and an ov7670_get_wsizes(model) function. > + /* CIF - WARNING: not tested for ov7675 */ > + { ...and this is part of why I don't like it. My experience with this particular sensor says that, if it's not tested, it hasn't yet seen the magic-number tweaking required to actually make it work. Please don't claim to support formats that you don't know actually work, or I'll get stuck with the bug reports :) > + .width = CIF_WIDTH, > + .height = CIF_HEIGHT, > + .com7_bit = COM7_FMT_CIF, > + .hstart = 170, /* Empirically determined */ > + .hstop = 90, > + .vstart = 14, > + .vstop = 494, > + .regs = NULL, > + }, jon -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, thank you for your time. On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote: > This is going to have to be quick, sorry... > > On Wed, 26 Sep 2012 11:47:53 +0200 > Javier Martin <javier.martin@vista-silicon.com> wrote: > >> +static struct ov7670_win_size ov7670_win_sizes[2][4] = { >> + /* ov7670 */ > > I must confess I don't like this; now we've got constants in an array that > was automatically sized before and ov7670_win_sizes[info->model] > everywhere. I'd suggest a separate array for each device and an > ov7670_get_wsizes(model) function. > >> + /* CIF - WARNING: not tested for ov7675 */ >> + { > > ...and this is part of why I don't like it. My experience with this > particular sensor says that, if it's not tested, it hasn't yet seen the > magic-number tweaking required to actually make it work. Please don't > claim to support formats that you don't know actually work, or I'll get > stuck with the bug reports :) Your concern makes a lot of sense. In fact, that was one of my doubts whether to 'support' not tested formats or not. Let me fix that in a new version.
Em Thu, 27 Sep 2012 08:58:33 +0200 javier Martin <javier.martin@vista-silicon.com> escreveu: > Hi Jonathan, > thank you for your time. > > On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote: > > This is going to have to be quick, sorry... > > > > On Wed, 26 Sep 2012 11:47:53 +0200 > > Javier Martin <javier.martin@vista-silicon.com> wrote: > > > >> +static struct ov7670_win_size ov7670_win_sizes[2][4] = { > >> + /* ov7670 */ > > > > I must confess I don't like this; now we've got constants in an array that > > was automatically sized before and ov7670_win_sizes[info->model] > > everywhere. I'd suggest a separate array for each device and an > > ov7670_get_wsizes(model) function. > > > >> + /* CIF - WARNING: not tested for ov7675 */ > >> + { > > > > ...and this is part of why I don't like it. My experience with this > > particular sensor says that, if it's not tested, it hasn't yet seen the > > magic-number tweaking required to actually make it work. Please don't > > claim to support formats that you don't know actually work, or I'll get > > stuck with the bug reports :) > > Your concern makes a lot of sense. In fact, that was one of my doubts > whether to 'support' not tested formats or not. > > Let me fix that in a new version. Hi Javier, I'm assuming that you'll be sending a new version of this entire changeset. So, I'll just mark this entire series as changes_requested. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 October 2012 17:19, Mauro Carvalho Chehab <mchehab@infradead.org> wrote: > Em Thu, 27 Sep 2012 08:58:33 +0200 > javier Martin <javier.martin@vista-silicon.com> escreveu: > >> Hi Jonathan, >> thank you for your time. >> >> On 26 September 2012 18:40, Jonathan Corbet <corbet@lwn.net> wrote: >> > This is going to have to be quick, sorry... >> > >> > On Wed, 26 Sep 2012 11:47:53 +0200 >> > Javier Martin <javier.martin@vista-silicon.com> wrote: >> > >> >> +static struct ov7670_win_size ov7670_win_sizes[2][4] = { >> >> + /* ov7670 */ >> > >> > I must confess I don't like this; now we've got constants in an array that >> > was automatically sized before and ov7670_win_sizes[info->model] >> > everywhere. I'd suggest a separate array for each device and an >> > ov7670_get_wsizes(model) function. >> > >> >> + /* CIF - WARNING: not tested for ov7675 */ >> >> + { >> > >> > ...and this is part of why I don't like it. My experience with this >> > particular sensor says that, if it's not tested, it hasn't yet seen the >> > magic-number tweaking required to actually make it work. Please don't >> > claim to support formats that you don't know actually work, or I'll get >> > stuck with the bug reports :) >> >> Your concern makes a lot of sense. In fact, that was one of my doubts >> whether to 'support' not tested formats or not. >> >> Let me fix that in a new version. > > Hi Javier, > > I'm assuming that you'll be sending a new version of this entire changeset. > So, I'll just mark this entire series as changes_requested. Hi Mauro, v2 of this changeset has already been sent with Jon Corbet's ack: http://www.mail-archive.com/linux-media@vger.kernel.org/msg52767.html https://patchwork.kernel.org/patch/1515001/ https://patchwork.kernel.org/patch/1515021/ https://patchwork.kernel.org/patch/1515011/ https://patchwork.kernel.org/patch/1515031/ https://patchwork.kernel.org/patch/1515041/ Regards.
diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c index e7c82b2..0478a7b 100644 --- a/drivers/media/i2c/ov7670.c +++ b/drivers/media/i2c/ov7670.c @@ -183,6 +183,10 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)"); #define REG_HAECC7 0xaa /* Hist AEC/AGC control 7 */ #define REG_BD60MAX 0xab /* 60hz banding step limit */ +enum ov7670_model { + MODEL_OV7670 = 0, + MODEL_OV7675, +}; /* * Information we maintain about a known sensor. @@ -198,6 +202,7 @@ struct ov7670_info { int clock_speed; /* External clock speed (MHz) */ u8 clkrc; /* Clock divider value */ bool use_smbus; /* Use smbus I/O instead of I2C */ + enum ov7670_model model; }; static inline struct ov7670_info *to_state(struct v4l2_subdev *sd) @@ -652,7 +657,7 @@ static struct regval_list ov7670_qcif_regs[] = { { 0xff, 0xff }, }; -static struct ov7670_win_size { +struct ov7670_win_size { int width; int height; unsigned char com7_bit; @@ -661,57 +666,105 @@ static struct ov7670_win_size { int vstart; /* sense to humans, but evidently the sensor */ int vstop; /* will do the right thing... */ struct regval_list *regs; /* Regs to tweak */ -/* h/vref stuff */ -} ov7670_win_sizes[] = { - /* VGA */ - { - .width = VGA_WIDTH, - .height = VGA_HEIGHT, - .com7_bit = COM7_FMT_VGA, - .hstart = 158, /* These values from */ - .hstop = 14, /* Omnivision */ - .vstart = 10, - .vstop = 490, - .regs = NULL, - }, - /* CIF */ - { - .width = CIF_WIDTH, - .height = CIF_HEIGHT, - .com7_bit = COM7_FMT_CIF, - .hstart = 170, /* Empirically determined */ - .hstop = 90, - .vstart = 14, - .vstop = 494, - .regs = NULL, - }, - /* QVGA */ +}; + +static struct ov7670_win_size ov7670_win_sizes[2][4] = { + /* ov7670 */ { - .width = QVGA_WIDTH, - .height = QVGA_HEIGHT, - .com7_bit = COM7_FMT_QVGA, - .hstart = 168, /* Empirically determined */ - .hstop = 24, - .vstart = 12, - .vstop = 492, - .regs = NULL, + /* VGA */ + { + .width = VGA_WIDTH, + .height = VGA_HEIGHT, + .com7_bit = COM7_FMT_VGA, + .hstart = 158, /* These values from */ + .hstop = 14, /* Omnivision */ + .vstart = 10, + .vstop = 490, + .regs = NULL, + }, + /* CIF */ + { + .width = CIF_WIDTH, + .height = CIF_HEIGHT, + .com7_bit = COM7_FMT_CIF, + .hstart = 170, /* Empirically determined */ + .hstop = 90, + .vstart = 14, + .vstop = 494, + .regs = NULL, + }, + /* QVGA */ + { + .width = QVGA_WIDTH, + .height = QVGA_HEIGHT, + .com7_bit = COM7_FMT_QVGA, + .hstart = 168, /* Empirically determined */ + .hstop = 24, + .vstart = 12, + .vstop = 492, + .regs = NULL, + }, + /* QCIF */ + { + .width = QCIF_WIDTH, + .height = QCIF_HEIGHT, + .com7_bit = COM7_FMT_VGA, /* see comment above */ + .hstart = 456, /* Empirically determined */ + .hstop = 24, + .vstart = 14, + .vstop = 494, + .regs = ov7670_qcif_regs, + } }, - /* QCIF */ + /* ov7675 */ { - .width = QCIF_WIDTH, - .height = QCIF_HEIGHT, - .com7_bit = COM7_FMT_VGA, /* see comment above */ - .hstart = 456, /* Empirically determined */ - .hstop = 24, - .vstart = 14, - .vstop = 494, - .regs = ov7670_qcif_regs, - }, + /* VGA */ + { + .width = VGA_WIDTH, + .height = VGA_HEIGHT, + .com7_bit = COM7_FMT_VGA, + .hstart = 158, /* These values from */ + .hstop = 14, /* Omnivision */ + .vstart = 14, + .vstop = 494, + .regs = NULL, + }, + /* CIF - WARNING: not tested for ov7675 */ + { + .width = CIF_WIDTH, + .height = CIF_HEIGHT, + .com7_bit = COM7_FMT_CIF, + .hstart = 170, /* Empirically determined */ + .hstop = 90, + .vstart = 14, + .vstop = 494, + .regs = NULL, + }, + /* QVGA - WARNING: not tested for ov7675 */ + { + .width = QVGA_WIDTH, + .height = QVGA_HEIGHT, + .com7_bit = COM7_FMT_QVGA, + .hstart = 168, /* Empirically determined */ + .hstop = 24, + .vstart = 12, + .vstop = 492, + .regs = NULL, + }, + /* QCIF - WARNING: not tested for ov7675 */ + { + .width = QCIF_WIDTH, + .height = QCIF_HEIGHT, + .com7_bit = COM7_FMT_VGA, /* see comment above */ + .hstart = 456, /* Empirically determined */ + .hstop = 24, + .vstart = 14, + .vstop = 494, + .regs = ov7670_qcif_regs, + } + } }; -#define N_WIN_SIZES (ARRAY_SIZE(ov7670_win_sizes)) - - /* * Store a set of start/stop values into the camera. */ @@ -761,6 +814,8 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd, { int index; struct ov7670_win_size *wsize; + struct ov7670_info *info = to_state(sd); + int n_win_sizes = ARRAY_SIZE(ov7670_win_sizes[info->model]); for (index = 0; index < N_OV7670_FMTS; index++) if (ov7670_formats[index].mbus_code == fmt->code) @@ -780,11 +835,11 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd, * 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++) + for (wsize = ov7670_win_sizes[info->model]; + wsize < ov7670_win_sizes[info->model] + n_win_sizes; wsize++) if (fmt->width >= wsize->width && fmt->height >= wsize->height) break; - if (wsize >= ov7670_win_sizes + N_WIN_SIZES) + if (wsize >= ov7670_win_sizes[info->model] + n_win_sizes) wsize--; /* Take the smallest one */ if (ret_wsize != NULL) *ret_wsize = wsize; @@ -931,13 +986,14 @@ static int ov7670_enum_framesizes(struct v4l2_subdev *sd, int i; int num_valid = -1; __u32 index = fsize->index; + int n_win_sizes = ARRAY_SIZE(ov7670_win_sizes[info->model]); /* * If a minimum width/height was requested, filter out the capture * windows that fall outside that. */ - for (i = 0; i < N_WIN_SIZES; i++) { - struct ov7670_win_size *win = &ov7670_win_sizes[index]; + for (i = 0; i < n_win_sizes; i++) { + struct ov7670_win_size *win = &ov7670_win_sizes[info->model][index]; if (info->min_width && win->width < info->min_width) continue; if (info->min_height && win->height < info->min_height) @@ -1551,6 +1607,7 @@ static int ov7670_probe(struct i2c_client *client, v4l_info(client, "chip found @ 0x%02x (%s)\n", client->addr << 1, client->adapter->name); + info->model = id->driver_data; info->fmt = &ov7670_formats[0]; info->sat = 128; /* Review this */ info->clkrc = info->clock_speed / 30; @@ -1568,7 +1625,8 @@ static int ov7670_remove(struct i2c_client *client) } static const struct i2c_device_id ov7670_id[] = { - { "ov7670", 0 }, + { "ov7670", MODEL_OV7670 }, + { "ov7675", MODEL_OV7675 }, { } }; MODULE_DEVICE_TABLE(i2c, ov7670_id);
ov7675 and ov7670 share the same registers but there is no way to distinguish them at runtime. However, they require different tweaks to achieve the desired resolution. For this reason this patch adds a new ov7675 entry to the ov7670_id table. Signed-off-by: Javier Martin <javier.martin@vista-silicon.com> --- drivers/media/i2c/ov7670.c | 164 ++++++++++++++++++++++++++++++-------------- 1 file changed, 111 insertions(+), 53 deletions(-)