* [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings
@ 2012-09-23 18:28 Frank Schäfer
2012-09-23 18:28 ` [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE Frank Schäfer
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Frank Schäfer @ 2012-09-23 18:28 UTC (permalink / raw)
To: g.liakhovetski; +Cc: maramaopercheseimorto, linux-media, Frank Schäfer
We currently don't select the register bank in ov2640_s_ctrl, so we can end up
writing to DSP register 0x04 instead of sensor register 0x04.
This happens for example when calling ov2640_s_ctrl after ov2640_s_fmt.
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
Cc: stable@kernel.org
---
drivers/media/i2c/soc_camera/ov2640.c | 8 ++++++++
1 Datei geändert, 8 Zeilen hinzugefügt(+)
diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
index 78ac574..e4fc79e 100644
--- a/drivers/media/i2c/soc_camera/ov2640.c
+++ b/drivers/media/i2c/soc_camera/ov2640.c
@@ -683,8 +683,16 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
struct v4l2_subdev *sd =
&container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev;
struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct regval_list regval;
+ int ret;
u8 val;
+ regval.reg_num = BANK_SEL;
+ regval.value = BANK_SEL_SENS;
+ ret = ov2640_write_array(client, ®val);
+ if (ret < 0)
+ return ret;
+
switch (ctrl->id) {
case V4L2_CID_VFLIP:
val = ctrl->val ? REG04_VFLIP_IMG : 0x00;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE 2012-09-23 18:28 [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Frank Schäfer @ 2012-09-23 18:28 ` Frank Schäfer 2012-09-23 20:36 ` Guennadi Liakhovetski 2012-09-23 18:28 ` [PATCH v2 3/3] ov2640: simplify single register writes Frank Schäfer 2012-09-23 20:21 ` [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Guennadi Liakhovetski 2 siblings, 1 reply; 11+ messages in thread From: Frank Schäfer @ 2012-09-23 18:28 UTC (permalink / raw) To: g.liakhovetski; +Cc: maramaopercheseimorto, linux-media, Frank Schäfer This is the result of experimenting with the SpeedLink VAD Laplace webcam. The register sequence for V4L2_MBUS_FMT_YUYV8_2X8 has been identified by analyzing USB-logs of this device running on MS Windows. Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> --- drivers/media/i2c/soc_camera/ov2640.c | 49 ++++++++++++++++++++++++++++----- 1 Datei geändert, 42 Zeilen hinzugefügt(+), 7 Zeilen entfernt(-) diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c index e4fc79e..182d5a1 100644 --- a/drivers/media/i2c/soc_camera/ov2640.c +++ b/drivers/media/i2c/soc_camera/ov2640.c @@ -586,9 +586,20 @@ static const struct regval_list ov2640_format_change_preamble_regs[] = { ENDMARKER, }; -static const struct regval_list ov2640_yuv422_regs[] = { +static const struct regval_list ov2640_yuyv_regs[] = { + { IMAGE_MODE, IMAGE_MODE_YUV422 }, + { 0xd7, 0x03 }, + { 0x33, 0xa0 }, + { 0xe5, 0x1f }, + { 0xe1, 0x67 }, + { RESET, 0x00 }, + { R_BYPASS, R_BYPASS_USE_DSP }, + ENDMARKER, +}; + +static const struct regval_list ov2640_uyvy_regs[] = { { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_YUV422 }, - { 0xD7, 0x01 }, + { 0xd7, 0x01 }, { 0x33, 0xa0 }, { 0xe1, 0x67 }, { RESET, 0x00 }, @@ -596,7 +607,15 @@ static const struct regval_list ov2640_yuv422_regs[] = { ENDMARKER, }; -static const struct regval_list ov2640_rgb565_regs[] = { +static const struct regval_list ov2640_rgb565_be_regs[] = { + { IMAGE_MODE, IMAGE_MODE_RGB565 }, + { 0xd7, 0x03 }, + { RESET, 0x00 }, + { R_BYPASS, R_BYPASS_USE_DSP }, + ENDMARKER, +}; + +static const struct regval_list ov2640_rgb565_le_regs[] = { { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_RGB565 }, { 0xd7, 0x03 }, { RESET, 0x00 }, @@ -605,7 +624,9 @@ static const struct regval_list ov2640_rgb565_regs[] = { }; static enum v4l2_mbus_pixelcode ov2640_codes[] = { + V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_UYVY8_2X8, + V4L2_MBUS_FMT_RGB565_2X8_BE, V4L2_MBUS_FMT_RGB565_2X8_LE, }; @@ -790,14 +811,22 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height, /* select format */ priv->cfmt_code = 0; switch (code) { + case V4L2_MBUS_FMT_RGB565_2X8_BE: + dev_dbg(&client->dev, "%s: Selected cfmt RGB565 BE", __func__); + selected_cfmt_regs = ov2640_rgb565_be_regs; + break; case V4L2_MBUS_FMT_RGB565_2X8_LE: - dev_dbg(&client->dev, "%s: Selected cfmt RGB565", __func__); - selected_cfmt_regs = ov2640_rgb565_regs; + dev_dbg(&client->dev, "%s: Selected cfmt RGB565 LE", __func__); + selected_cfmt_regs = ov2640_rgb565_le_regs; + break; + case V4L2_MBUS_FMT_YUYV8_2X8: + dev_dbg(&client->dev, "%s: Selected cfmt YUYV (YUV422)", __func__); + selected_cfmt_regs = ov2640_yuyv_regs; break; default: case V4L2_MBUS_FMT_UYVY8_2X8: - dev_dbg(&client->dev, "%s: Selected cfmt YUV422", __func__); - selected_cfmt_regs = ov2640_yuv422_regs; + dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__); + selected_cfmt_regs = ov2640_uyvy_regs; } /* reset hardware */ @@ -862,10 +891,12 @@ static int ov2640_g_fmt(struct v4l2_subdev *sd, mf->code = priv->cfmt_code; switch (mf->code) { + case V4L2_MBUS_FMT_RGB565_2X8_BE: case V4L2_MBUS_FMT_RGB565_2X8_LE: mf->colorspace = V4L2_COLORSPACE_SRGB; break; default: + case V4L2_MBUS_FMT_YUYV8_2X8: case V4L2_MBUS_FMT_UYVY8_2X8: mf->colorspace = V4L2_COLORSPACE_JPEG; } @@ -882,11 +913,13 @@ static int ov2640_s_fmt(struct v4l2_subdev *sd, switch (mf->code) { + case V4L2_MBUS_FMT_RGB565_2X8_BE: case V4L2_MBUS_FMT_RGB565_2X8_LE: mf->colorspace = V4L2_COLORSPACE_SRGB; break; default: mf->code = V4L2_MBUS_FMT_UYVY8_2X8; + case V4L2_MBUS_FMT_YUYV8_2X8: case V4L2_MBUS_FMT_UYVY8_2X8: mf->colorspace = V4L2_COLORSPACE_JPEG; } @@ -909,11 +942,13 @@ static int ov2640_try_fmt(struct v4l2_subdev *sd, mf->field = V4L2_FIELD_NONE; switch (mf->code) { + case V4L2_MBUS_FMT_RGB565_2X8_BE: case V4L2_MBUS_FMT_RGB565_2X8_LE: mf->colorspace = V4L2_COLORSPACE_SRGB; break; default: mf->code = V4L2_MBUS_FMT_UYVY8_2X8; + case V4L2_MBUS_FMT_YUYV8_2X8: case V4L2_MBUS_FMT_UYVY8_2X8: mf->colorspace = V4L2_COLORSPACE_JPEG; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE 2012-09-23 18:28 ` [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE Frank Schäfer @ 2012-09-23 20:36 ` Guennadi Liakhovetski 0 siblings, 0 replies; 11+ messages in thread From: Guennadi Liakhovetski @ 2012-09-23 20:36 UTC (permalink / raw) To: Frank Schäfer; +Cc: maramaopercheseimorto, linux-media On Sun, 23 Sep 2012, Frank SchÀfer wrote: > This is the result of experimenting with the SpeedLink VAD Laplace webcam. > The register sequence for V4L2_MBUS_FMT_YUYV8_2X8 has been identified by > analyzing USB-logs of this device running on MS Windows. > > Signed-off-by: Frank SchÀfer <fschaefer.oss@googlemail.com> Looks good to me, thanks, will queue for 3.7. Thanks Guennadi > --- > drivers/media/i2c/soc_camera/ov2640.c | 49 ++++++++++++++++++++++++++++----- > 1 Datei geÀndert, 42 Zeilen hinzugefÃŒgt(+), 7 Zeilen entfernt(-) > > diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c > index e4fc79e..182d5a1 100644 > --- a/drivers/media/i2c/soc_camera/ov2640.c > +++ b/drivers/media/i2c/soc_camera/ov2640.c > @@ -586,9 +586,20 @@ static const struct regval_list ov2640_format_change_preamble_regs[] = { > ENDMARKER, > }; > > -static const struct regval_list ov2640_yuv422_regs[] = { > +static const struct regval_list ov2640_yuyv_regs[] = { > + { IMAGE_MODE, IMAGE_MODE_YUV422 }, > + { 0xd7, 0x03 }, > + { 0x33, 0xa0 }, > + { 0xe5, 0x1f }, > + { 0xe1, 0x67 }, > + { RESET, 0x00 }, > + { R_BYPASS, R_BYPASS_USE_DSP }, > + ENDMARKER, > +}; > + > +static const struct regval_list ov2640_uyvy_regs[] = { > { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_YUV422 }, > - { 0xD7, 0x01 }, > + { 0xd7, 0x01 }, > { 0x33, 0xa0 }, > { 0xe1, 0x67 }, > { RESET, 0x00 }, > @@ -596,7 +607,15 @@ static const struct regval_list ov2640_yuv422_regs[] = { > ENDMARKER, > }; > > -static const struct regval_list ov2640_rgb565_regs[] = { > +static const struct regval_list ov2640_rgb565_be_regs[] = { > + { IMAGE_MODE, IMAGE_MODE_RGB565 }, > + { 0xd7, 0x03 }, > + { RESET, 0x00 }, > + { R_BYPASS, R_BYPASS_USE_DSP }, > + ENDMARKER, > +}; > + > +static const struct regval_list ov2640_rgb565_le_regs[] = { > { IMAGE_MODE, IMAGE_MODE_LBYTE_FIRST | IMAGE_MODE_RGB565 }, > { 0xd7, 0x03 }, > { RESET, 0x00 }, > @@ -605,7 +624,9 @@ static const struct regval_list ov2640_rgb565_regs[] = { > }; > > static enum v4l2_mbus_pixelcode ov2640_codes[] = { > + V4L2_MBUS_FMT_YUYV8_2X8, > V4L2_MBUS_FMT_UYVY8_2X8, > + V4L2_MBUS_FMT_RGB565_2X8_BE, > V4L2_MBUS_FMT_RGB565_2X8_LE, > }; > > @@ -790,14 +811,22 @@ static int ov2640_set_params(struct i2c_client *client, u32 *width, u32 *height, > /* select format */ > priv->cfmt_code = 0; > switch (code) { > + case V4L2_MBUS_FMT_RGB565_2X8_BE: > + dev_dbg(&client->dev, "%s: Selected cfmt RGB565 BE", __func__); > + selected_cfmt_regs = ov2640_rgb565_be_regs; > + break; > case V4L2_MBUS_FMT_RGB565_2X8_LE: > - dev_dbg(&client->dev, "%s: Selected cfmt RGB565", __func__); > - selected_cfmt_regs = ov2640_rgb565_regs; > + dev_dbg(&client->dev, "%s: Selected cfmt RGB565 LE", __func__); > + selected_cfmt_regs = ov2640_rgb565_le_regs; > + break; > + case V4L2_MBUS_FMT_YUYV8_2X8: > + dev_dbg(&client->dev, "%s: Selected cfmt YUYV (YUV422)", __func__); > + selected_cfmt_regs = ov2640_yuyv_regs; > break; > default: > case V4L2_MBUS_FMT_UYVY8_2X8: > - dev_dbg(&client->dev, "%s: Selected cfmt YUV422", __func__); > - selected_cfmt_regs = ov2640_yuv422_regs; > + dev_dbg(&client->dev, "%s: Selected cfmt UYVY", __func__); > + selected_cfmt_regs = ov2640_uyvy_regs; > } > > /* reset hardware */ > @@ -862,10 +891,12 @@ static int ov2640_g_fmt(struct v4l2_subdev *sd, > mf->code = priv->cfmt_code; > > switch (mf->code) { > + case V4L2_MBUS_FMT_RGB565_2X8_BE: > case V4L2_MBUS_FMT_RGB565_2X8_LE: > mf->colorspace = V4L2_COLORSPACE_SRGB; > break; > default: > + case V4L2_MBUS_FMT_YUYV8_2X8: > case V4L2_MBUS_FMT_UYVY8_2X8: > mf->colorspace = V4L2_COLORSPACE_JPEG; > } > @@ -882,11 +913,13 @@ static int ov2640_s_fmt(struct v4l2_subdev *sd, > > > switch (mf->code) { > + case V4L2_MBUS_FMT_RGB565_2X8_BE: > case V4L2_MBUS_FMT_RGB565_2X8_LE: > mf->colorspace = V4L2_COLORSPACE_SRGB; > break; > default: > mf->code = V4L2_MBUS_FMT_UYVY8_2X8; > + case V4L2_MBUS_FMT_YUYV8_2X8: > case V4L2_MBUS_FMT_UYVY8_2X8: > mf->colorspace = V4L2_COLORSPACE_JPEG; > } > @@ -909,11 +942,13 @@ static int ov2640_try_fmt(struct v4l2_subdev *sd, > mf->field = V4L2_FIELD_NONE; > > switch (mf->code) { > + case V4L2_MBUS_FMT_RGB565_2X8_BE: > case V4L2_MBUS_FMT_RGB565_2X8_LE: > mf->colorspace = V4L2_COLORSPACE_SRGB; > break; > default: > mf->code = V4L2_MBUS_FMT_UYVY8_2X8; > + case V4L2_MBUS_FMT_YUYV8_2X8: > case V4L2_MBUS_FMT_UYVY8_2X8: > mf->colorspace = V4L2_COLORSPACE_JPEG; > } > -- > 1.7.10.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] ov2640: simplify single register writes 2012-09-23 18:28 [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Frank Schäfer 2012-09-23 18:28 ` [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE Frank Schäfer @ 2012-09-23 18:28 ` Frank Schäfer 2012-09-23 20:43 ` Guennadi Liakhovetski 2012-09-23 20:21 ` [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Guennadi Liakhovetski 2 siblings, 1 reply; 11+ messages in thread From: Frank Schäfer @ 2012-09-23 18:28 UTC (permalink / raw) To: g.liakhovetski; +Cc: maramaopercheseimorto, linux-media, Frank Schäfer Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> --- drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++--------- 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-) diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c index 182d5a1..e71bf4c 100644 --- a/drivers/media/i2c/soc_camera/ov2640.c +++ b/drivers/media/i2c/soc_camera/ov2640.c @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client) subdev); } +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val) +{ + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val); + return i2c_smbus_write_byte_data(client, reg, val); +} + static int ov2640_write_array(struct i2c_client *client, const struct regval_list *vals) { int ret; while ((vals->reg_num != 0xff) || (vals->value != 0xff)) { - ret = i2c_smbus_write_byte_data(client, - vals->reg_num, vals->value); - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x", - vals->reg_num, vals->value); - + ret = ov2640_write_single(client, vals->reg_num, vals->value); if (ret < 0) return ret; vals++; @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) struct v4l2_subdev *sd = &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; struct i2c_client *client = v4l2_get_subdevdata(sd); - struct regval_list regval; int ret; u8 val; - regval.reg_num = BANK_SEL; - regval.value = BANK_SEL_SENS; - ret = ov2640_write_array(client, ®val); + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS); if (ret < 0) return ret; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ov2640: simplify single register writes 2012-09-23 18:28 ` [PATCH v2 3/3] ov2640: simplify single register writes Frank Schäfer @ 2012-09-23 20:43 ` Guennadi Liakhovetski 2012-09-23 20:06 ` Frank Schäfer 0 siblings, 1 reply; 11+ messages in thread From: Guennadi Liakhovetski @ 2012-09-23 20:43 UTC (permalink / raw) To: Frank Schäfer; +Cc: maramaopercheseimorto, linux-media On Sun, 23 Sep 2012, Frank Schäfer wrote: > Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > --- > drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++--------- > 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-) > > diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c > index 182d5a1..e71bf4c 100644 > --- a/drivers/media/i2c/soc_camera/ov2640.c > +++ b/drivers/media/i2c/soc_camera/ov2640.c > @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client) > subdev); > } > > +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val) > +{ > + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val); > + return i2c_smbus_write_byte_data(client, reg, val); > +} Well, I'm not convinced. I don't necessarily see it as a simplification. You replace one perfectly ok function with another one with exactly the same parameters. Ok, you also hide a debug printk() in your wrapper, but that's not too useful either, IMHO. Besides, you're missing more calls to i2c_smbus_write_byte_data() in ov2640_mask_set(), ov2640_s_register() and ov2640_video_probe(). So, I'd just drop it. Thanks Guennadi > + > static int ov2640_write_array(struct i2c_client *client, > const struct regval_list *vals) > { > int ret; > > while ((vals->reg_num != 0xff) || (vals->value != 0xff)) { > - ret = i2c_smbus_write_byte_data(client, > - vals->reg_num, vals->value); > - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x", > - vals->reg_num, vals->value); > - > + ret = ov2640_write_single(client, vals->reg_num, vals->value); > if (ret < 0) > return ret; > vals++; > @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) > struct v4l2_subdev *sd = > &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; > struct i2c_client *client = v4l2_get_subdevdata(sd); > - struct regval_list regval; > int ret; > u8 val; > > - regval.reg_num = BANK_SEL; > - regval.value = BANK_SEL_SENS; > - ret = ov2640_write_array(client, ®val); > + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS); > if (ret < 0) > return ret; > > -- > 1.7.10.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ov2640: simplify single register writes 2012-09-23 20:43 ` Guennadi Liakhovetski @ 2012-09-23 20:06 ` Frank Schäfer 2012-09-23 21:23 ` Guennadi Liakhovetski 0 siblings, 1 reply; 11+ messages in thread From: Frank Schäfer @ 2012-09-23 20:06 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: maramaopercheseimorto, linux-media Am 23.09.2012 23:43, schrieb Guennadi Liakhovetski: > On Sun, 23 Sep 2012, Frank Schäfer wrote: > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >> --- >> drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++--------- >> 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-) >> >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c >> index 182d5a1..e71bf4c 100644 >> --- a/drivers/media/i2c/soc_camera/ov2640.c >> +++ b/drivers/media/i2c/soc_camera/ov2640.c >> @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client) >> subdev); >> } >> >> +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val) >> +{ >> + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val); >> + return i2c_smbus_write_byte_data(client, reg, val); >> +} > Well, I'm not convinced. I don't necessarily see it as a simplification. > You replace one perfectly ok function with another one with exactly the > same parameters. Ok, you also hide a debug printk() in your wrapper, but > that's not too useful either, IMHO. Sure, at the moment this is not really needed. But that will change in the future, when we need to do more single writes / can't use static register sequences. A good example is the powerline frequency filter control, which I'm currently experimenting with. But if you don't want to take it at the moment, it's ok for me. > Besides, you're missing more calls to > i2c_smbus_write_byte_data() in ov2640_mask_set(), ov2640_s_register() and > ov2640_video_probe(). So, I'd just drop it. I skipped that because of the different debug output (which could of course be improved). Regrads, Frank > Thanks > Guennadi > >> + >> static int ov2640_write_array(struct i2c_client *client, >> const struct regval_list *vals) >> { >> int ret; >> >> while ((vals->reg_num != 0xff) || (vals->value != 0xff)) { >> - ret = i2c_smbus_write_byte_data(client, >> - vals->reg_num, vals->value); >> - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x", >> - vals->reg_num, vals->value); >> - >> + ret = ov2640_write_single(client, vals->reg_num, vals->value); >> if (ret < 0) >> return ret; >> vals++; >> @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) >> struct v4l2_subdev *sd = >> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; >> struct i2c_client *client = v4l2_get_subdevdata(sd); >> - struct regval_list regval; >> int ret; >> u8 val; >> >> - regval.reg_num = BANK_SEL; >> - regval.value = BANK_SEL_SENS; >> - ret = ov2640_write_array(client, ®val); >> + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS); >> if (ret < 0) >> return ret; >> >> -- >> 1.7.10.4 >> > --- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ov2640: simplify single register writes 2012-09-23 20:06 ` Frank Schäfer @ 2012-09-23 21:23 ` Guennadi Liakhovetski 2012-09-23 20:37 ` Frank Schäfer 0 siblings, 1 reply; 11+ messages in thread From: Guennadi Liakhovetski @ 2012-09-23 21:23 UTC (permalink / raw) To: Frank Schäfer; +Cc: maramaopercheseimorto, linux-media On Sun, 23 Sep 2012, Frank Schäfer wrote: > Am 23.09.2012 23:43, schrieb Guennadi Liakhovetski: > > On Sun, 23 Sep 2012, Frank Schäfer wrote: > > > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > >> --- > >> drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++--------- > >> 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-) > >> > >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c > >> index 182d5a1..e71bf4c 100644 > >> --- a/drivers/media/i2c/soc_camera/ov2640.c > >> +++ b/drivers/media/i2c/soc_camera/ov2640.c > >> @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client) > >> subdev); > >> } > >> > >> +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val) > >> +{ > >> + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val); > >> + return i2c_smbus_write_byte_data(client, reg, val); > >> +} > > Well, I'm not convinced. I don't necessarily see it as a simplification. > > You replace one perfectly ok function with another one with exactly the > > same parameters. Ok, you also hide a debug printk() in your wrapper, but > > that's not too useful either, IMHO. > > Sure, at the moment this is not really needed. But that will change in > the future, when we need to do more single writes / can't use static > register sequences. Why won't you be able to just use i2c_smbus_write_byte_data() directly with those your sequences? Ok, if you just dislike the long name, and if you have a number of them, I might buy that as a valid reason:-) And yes, it'd be good to add such a helper function in a separate patch, preceding the actual functional changes. But then I'd probably suggest to name, that offers an even greater saving of your monitor real estate and is more similar to what other drivers use, something like ov2640_reg_write() and also add an ov2640_reg_read() for symmetry. Thanks Guennadi > A good example is the powerline frequency filter control, which I'm > currently experimenting with. > But if you don't want to take it at the moment, it's ok for me. > > > > Besides, you're missing more calls to > > i2c_smbus_write_byte_data() in ov2640_mask_set(), ov2640_s_register() and > > ov2640_video_probe(). So, I'd just drop it. > > I skipped that because of the different debug output (which could of > course be improved). > > Regrads, > Frank > > > Thanks > > Guennadi > > > >> + > >> static int ov2640_write_array(struct i2c_client *client, > >> const struct regval_list *vals) > >> { > >> int ret; > >> > >> while ((vals->reg_num != 0xff) || (vals->value != 0xff)) { > >> - ret = i2c_smbus_write_byte_data(client, > >> - vals->reg_num, vals->value); > >> - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x", > >> - vals->reg_num, vals->value); > >> - > >> + ret = ov2640_write_single(client, vals->reg_num, vals->value); > >> if (ret < 0) > >> return ret; > >> vals++; > >> @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) > >> struct v4l2_subdev *sd = > >> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; > >> struct i2c_client *client = v4l2_get_subdevdata(sd); > >> - struct regval_list regval; > >> int ret; > >> u8 val; > >> > >> - regval.reg_num = BANK_SEL; > >> - regval.value = BANK_SEL_SENS; > >> - ret = ov2640_write_array(client, ®val); > >> + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS); > >> if (ret < 0) > >> return ret; > >> > >> -- > >> 1.7.10.4 > >> > > --- > > 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 majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ov2640: simplify single register writes 2012-09-23 21:23 ` Guennadi Liakhovetski @ 2012-09-23 20:37 ` Frank Schäfer 0 siblings, 0 replies; 11+ messages in thread From: Frank Schäfer @ 2012-09-23 20:37 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: maramaopercheseimorto, linux-media Am 24.09.2012 00:23, schrieb Guennadi Liakhovetski: > On Sun, 23 Sep 2012, Frank Schäfer wrote: > >> Am 23.09.2012 23:43, schrieb Guennadi Liakhovetski: >>> On Sun, 23 Sep 2012, Frank Schäfer wrote: >>> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >>>> --- >>>> drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++--------- >>>> 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-) >>>> >>>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c >>>> index 182d5a1..e71bf4c 100644 >>>> --- a/drivers/media/i2c/soc_camera/ov2640.c >>>> +++ b/drivers/media/i2c/soc_camera/ov2640.c >>>> @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client) >>>> subdev); >>>> } >>>> >>>> +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val) >>>> +{ >>>> + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val); >>>> + return i2c_smbus_write_byte_data(client, reg, val); >>>> +} >>> Well, I'm not convinced. I don't necessarily see it as a simplification. >>> You replace one perfectly ok function with another one with exactly the >>> same parameters. Ok, you also hide a debug printk() in your wrapper, but >>> that's not too useful either, IMHO. >> Sure, at the moment this is not really needed. But that will change in >> the future, when we need to do more single writes / can't use static >> register sequences. > Why won't you be able to just use i2c_smbus_write_byte_data() directly > with those your sequences? Ok, if you just dislike the long name, and if > you have a number of them, I might buy that as a valid reason:-) The suggest helper function also prints a debugging message, so we are talking about two lines for each single write. > And yes, > it'd be good to add such a helper function in a separate patch, preceding > the actual functional changes. But then I'd probably suggest to name, that > offers an even greater saving of your monitor real estate and is more > similar to what other drivers use, something like ov2640_reg_write() and > also add an ov2640_reg_read() for symmetry. Ok, thats a matter of taste ;). ov2640_write_single seemed to be the logocal counterpart to ov2640_write_array, but I don't care. I will come back to this when the next feature patch(es) are ready. Regards, Frank > > Thanks > Guennadi > >> A good example is the powerline frequency filter control, which I'm >> currently experimenting with. >> But if you don't want to take it at the moment, it's ok for me. >> >> >>> Besides, you're missing more calls to >>> i2c_smbus_write_byte_data() in ov2640_mask_set(), ov2640_s_register() and >>> ov2640_video_probe(). So, I'd just drop it. >> I skipped that because of the different debug output (which could of >> course be improved). >> >> Regrads, >> Frank >> >>> Thanks >>> Guennadi >>> >>>> + >>>> static int ov2640_write_array(struct i2c_client *client, >>>> const struct regval_list *vals) >>>> { >>>> int ret; >>>> >>>> while ((vals->reg_num != 0xff) || (vals->value != 0xff)) { >>>> - ret = i2c_smbus_write_byte_data(client, >>>> - vals->reg_num, vals->value); >>>> - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x", >>>> - vals->reg_num, vals->value); >>>> - >>>> + ret = ov2640_write_single(client, vals->reg_num, vals->value); >>>> if (ret < 0) >>>> return ret; >>>> vals++; >>>> @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) >>>> struct v4l2_subdev *sd = >>>> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; >>>> struct i2c_client *client = v4l2_get_subdevdata(sd); >>>> - struct regval_list regval; >>>> int ret; >>>> u8 val; >>>> >>>> - regval.reg_num = BANK_SEL; >>>> - regval.value = BANK_SEL_SENS; >>>> - ret = ov2640_write_array(client, ®val); >>>> + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS); >>>> if (ret < 0) >>>> return ret; >>>> >>>> -- >>>> 1.7.10.4 >>>> >>> --- >>> 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 majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > --- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings 2012-09-23 18:28 [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Frank Schäfer 2012-09-23 18:28 ` [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE Frank Schäfer 2012-09-23 18:28 ` [PATCH v2 3/3] ov2640: simplify single register writes Frank Schäfer @ 2012-09-23 20:21 ` Guennadi Liakhovetski 2012-09-23 19:41 ` Frank Schäfer 2 siblings, 1 reply; 11+ messages in thread From: Guennadi Liakhovetski @ 2012-09-23 20:21 UTC (permalink / raw) To: Frank Schäfer; +Cc: maramaopercheseimorto, linux-media Hi Frank On Sun, 23 Sep 2012, Frank Schäfer wrote: > We currently don't select the register bank in ov2640_s_ctrl, so we can end up > writing to DSP register 0x04 instead of sensor register 0x04. > This happens for example when calling ov2640_s_ctrl after ov2640_s_fmt. Yes, in principle, I agree, bank switching in the driver is not very... consistent and also this specific case looks buggy. But, we have to fix your fix. > > Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > Cc: stable@kernel.org > --- > drivers/media/i2c/soc_camera/ov2640.c | 8 ++++++++ > 1 Datei geändert, 8 Zeilen hinzugefügt(+) > > diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c > index 78ac574..e4fc79e 100644 > --- a/drivers/media/i2c/soc_camera/ov2640.c > +++ b/drivers/media/i2c/soc_camera/ov2640.c > @@ -683,8 +683,16 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) > struct v4l2_subdev *sd = > &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; > struct i2c_client *client = v4l2_get_subdevdata(sd); > + struct regval_list regval; > + int ret; > u8 val; > > + regval.reg_num = BANK_SEL; > + regval.value = BANK_SEL_SENS; > + ret = ov2640_write_array(client, ®val); This doesn't look right to me. ov2640_write_array() keeps writing register address - value pairs to the hardware until it encounters an "ENDMARKER," which you don't have here, so, it's hard to say what will be written to the sensor... Secondly, you only have to write a single register here, for this the driver is already using i2c_smbus_write_byte_data() directly, please, do the same. Thanks Guennadi > + if (ret < 0) > + return ret; > + > switch (ctrl->id) { > case V4L2_CID_VFLIP: > val = ctrl->val ? REG04_VFLIP_IMG : 0x00; > -- > 1.7.10.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings 2012-09-23 20:21 ` [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Guennadi Liakhovetski @ 2012-09-23 19:41 ` Frank Schäfer 2012-09-23 20:47 ` Guennadi Liakhovetski 0 siblings, 1 reply; 11+ messages in thread From: Frank Schäfer @ 2012-09-23 19:41 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: maramaopercheseimorto, linux-media Am 23.09.2012 23:21, schrieb Guennadi Liakhovetski: > Hi Frank > > On Sun, 23 Sep 2012, Frank Schäfer wrote: > >> We currently don't select the register bank in ov2640_s_ctrl, so we can end up >> writing to DSP register 0x04 instead of sensor register 0x04. >> This happens for example when calling ov2640_s_ctrl after ov2640_s_fmt. > Yes, in principle, I agree, bank switching in the driver is not very... > consistent and also this specific case looks buggy. But, we have to fix > your fix. > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> >> Cc: stable@kernel.org >> --- >> drivers/media/i2c/soc_camera/ov2640.c | 8 ++++++++ >> 1 Datei geändert, 8 Zeilen hinzugefügt(+) >> >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c >> index 78ac574..e4fc79e 100644 >> --- a/drivers/media/i2c/soc_camera/ov2640.c >> +++ b/drivers/media/i2c/soc_camera/ov2640.c >> @@ -683,8 +683,16 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) >> struct v4l2_subdev *sd = >> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; >> struct i2c_client *client = v4l2_get_subdevdata(sd); >> + struct regval_list regval; >> + int ret; >> u8 val; >> >> + regval.reg_num = BANK_SEL; >> + regval.value = BANK_SEL_SENS; >> + ret = ov2640_write_array(client, ®val); > This doesn't look right to me. ov2640_write_array() keeps writing register > address - value pairs to the hardware until it encounters an "ENDMARKER," > which you don't have here, so, it's hard to say what will be written to > the sensor... Secondly, you only have to write a single register here, for > this the driver is already using i2c_smbus_write_byte_data() directly, > please, do the same. Argh, yes, you're right. The mistake was to split this off from patch 3 to reduce changes for stable... I will combine both patches and resend the series. Regards, Frank > > Thanks > Guennadi > >> + if (ret < 0) >> + return ret; >> + >> switch (ctrl->id) { >> case V4L2_CID_VFLIP: >> val = ctrl->val ? REG04_VFLIP_IMG : 0x00; >> -- >> 1.7.10.4 >> > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings 2012-09-23 19:41 ` Frank Schäfer @ 2012-09-23 20:47 ` Guennadi Liakhovetski 0 siblings, 0 replies; 11+ messages in thread From: Guennadi Liakhovetski @ 2012-09-23 20:47 UTC (permalink / raw) To: Frank Schäfer; +Cc: maramaopercheseimorto, linux-media On Sun, 23 Sep 2012, Frank Schäfer wrote: > Am 23.09.2012 23:21, schrieb Guennadi Liakhovetski: > > Hi Frank > > > > On Sun, 23 Sep 2012, Frank Schäfer wrote: > > > >> We currently don't select the register bank in ov2640_s_ctrl, so we can end up > >> writing to DSP register 0x04 instead of sensor register 0x04. > >> This happens for example when calling ov2640_s_ctrl after ov2640_s_fmt. > > Yes, in principle, I agree, bank switching in the driver is not very... > > consistent and also this specific case looks buggy. But, we have to fix > > your fix. > > > >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com> > >> Cc: stable@kernel.org > >> --- > >> drivers/media/i2c/soc_camera/ov2640.c | 8 ++++++++ > >> 1 Datei geändert, 8 Zeilen hinzugefügt(+) > >> > >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c > >> index 78ac574..e4fc79e 100644 > >> --- a/drivers/media/i2c/soc_camera/ov2640.c > >> +++ b/drivers/media/i2c/soc_camera/ov2640.c > >> @@ -683,8 +683,16 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl) > >> struct v4l2_subdev *sd = > >> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev; > >> struct i2c_client *client = v4l2_get_subdevdata(sd); > >> + struct regval_list regval; > >> + int ret; > >> u8 val; > >> > >> + regval.reg_num = BANK_SEL; > >> + regval.value = BANK_SEL_SENS; > >> + ret = ov2640_write_array(client, ®val); > > This doesn't look right to me. ov2640_write_array() keeps writing register > > address - value pairs to the hardware until it encounters an "ENDMARKER," > > which you don't have here, so, it's hard to say what will be written to > > the sensor... Secondly, you only have to write a single register here, for > > this the driver is already using i2c_smbus_write_byte_data() directly, > > please, do the same. > > Argh, yes, you're right. > The mistake was to split this off from patch 3 to reduce changes for > stable... > I will combine both patches and resend the series. No, please, don't. Just use i2c_smbus_write_byte_data() at this one location for the fix patch. Thanks Guennadi > > Regards, > Frank > > > > > Thanks > > Guennadi > > > >> + if (ret < 0) > >> + return ret; > >> + > >> switch (ctrl->id) { > >> case V4L2_CID_VFLIP: > >> val = ctrl->val ? REG04_VFLIP_IMG : 0x00; > >> -- > >> 1.7.10.4 > >> > > --- > > Guennadi Liakhovetski, Ph.D. > > Freelance Open-Source Software Developer > > http://www.open-technology.de/ > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-09-23 21:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-23 18:28 [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Frank Schäfer 2012-09-23 18:28 ` [PATCH v2 2/3] ov2640: add support for V4L2_MBUS_FMT_YUYV8_2X8, V4L2_MBUS_FMT_RGB565_2X8_BE Frank Schäfer 2012-09-23 20:36 ` Guennadi Liakhovetski 2012-09-23 18:28 ` [PATCH v2 3/3] ov2640: simplify single register writes Frank Schäfer 2012-09-23 20:43 ` Guennadi Liakhovetski 2012-09-23 20:06 ` Frank Schäfer 2012-09-23 21:23 ` Guennadi Liakhovetski 2012-09-23 20:37 ` Frank Schäfer 2012-09-23 20:21 ` [PATCH v2 1/3] ov2640: select sensor register bank before applying h/v-flip settings Guennadi Liakhovetski 2012-09-23 19:41 ` Frank Schäfer 2012-09-23 20:47 ` Guennadi Liakhovetski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).