| Message ID | 1345799431-29426-2-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Anatolij On Fri, 24 Aug 2012, Anatolij Gustschin wrote: > Add controls for horizontal and vertical blanking, analog control > and control for undocumented register 32. Sorry, I don't think this is a good idea to export an undocumented register as a control. At most I would add a platform parameter for it, if really needed. Or do you have to switch it at run-time? If so, you would have some idea - at what time to switch it to what value and what effect that produces. Then you could define a _logical_ control. If you absolutely need to write random values to various registers, you can use VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER. > Also add an error message > for case that the control handler init failed. Since setting the > blanking registers is done by controls now, we should't change these > registers outside of the control function. Use v4l2_ctrl_s_ctrl() > to set them. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > drivers/media/i2c/soc_camera/mt9v022.c | 105 ++++++++++++++++++++++++++++++-- > 1 files changed, 100 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c > index 350d0d8..d13c8c4 100644 > --- a/drivers/media/i2c/soc_camera/mt9v022.c > +++ b/drivers/media/i2c/soc_camera/mt9v022.c > @@ -50,12 +50,14 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\""); > #define MT9V022_PIXEL_OPERATION_MODE 0x0f > #define MT9V022_LED_OUT_CONTROL 0x1b > #define MT9V022_ADC_MODE_CONTROL 0x1c > +#define MT9V022_REG32 0x20 > #define MT9V022_ANALOG_GAIN 0x35 > #define MT9V022_BLACK_LEVEL_CALIB_CTRL 0x47 > #define MT9V022_PIXCLK_FV_LV 0x74 > #define MT9V022_DIGITAL_TEST_PATTERN 0x7f > #define MT9V022_AEC_AGC_ENABLE 0xAF > #define MT9V022_MAX_TOTAL_SHUTTER_WIDTH 0xBD > +#define MT9V022_ANALOG_CONTROL 0xC2 > > /* mt9v024 partial list register addresses changes with respect to mt9v022 */ > #define MT9V024_PIXCLK_FV_LV 0x72 > @@ -71,6 +73,13 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\""); > #define MT9V022_COLUMN_SKIP 1 > #define MT9V022_ROW_SKIP 4 > > +#define MT9V022_HORIZONTAL_BLANKING_MIN 43 > +#define MT9V022_HORIZONTAL_BLANKING_MAX 1023 > +#define MT9V022_HORIZONTAL_BLANKING_DEF 94 > +#define MT9V022_VERTICAL_BLANKING_MIN 2 Interesting, in my datasheet min is 4. Maybe 4 would be a safer bet then. > +#define MT9V022_VERTICAL_BLANKING_MAX 3000 > +#define MT9V022_VERTICAL_BLANKING_DEF 45 > + > #define is_mt9v024(id) (id == 0x1324) > > /* MT9V022 has only one fixed colorspace per pixelcode */ > @@ -136,6 +145,8 @@ struct mt9v022 { > struct v4l2_ctrl *autogain; > struct v4l2_ctrl *gain; > }; > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vblank; > struct v4l2_rect rect; /* Sensor window */ > const struct mt9v022_datafmt *fmt; > const struct mt9v022_datafmt *fmts; > @@ -277,11 +288,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) > * Default 94, Phytec driver says: > * "width + horizontal blank >= 660" > */ > - ret = reg_write(client, MT9V022_HORIZONTAL_BLANKING, > - rect.width > 660 - 43 ? 43 : > - 660 - rect.width); > + ret = v4l2_ctrl_s_ctrl(mt9v022->hblank, > + rect.width > 660 - 43 ? 43 : 660 - rect.width); > if (!ret) > - ret = reg_write(client, MT9V022_VERTICAL_BLANKING, 45); > + ret = v4l2_ctrl_s_ctrl(mt9v022->vblank, 45); > if (!ret) > ret = reg_write(client, MT9V022_WINDOW_WIDTH, rect.width); > if (!ret) > @@ -476,6 +486,9 @@ static int mt9v022_s_power(struct v4l2_subdev *sd, int on) > return soc_camera_set_power(&client->dev, icl, on); > } > > +#define V4L2_CID_REG32 (V4L2_CTRL_CLASS_CAMERA | 0x1001) > +#define V4L2_CID_ANALOG_CONTROLS (V4L2_CTRL_CLASS_CAMERA | 0x1002) Sorry, no again. The MT9V022_ANALOG_CONTROL register contains two fields: anti-eclipse and "anti-eclipse reference voltage control," don't think they should be set as a single control value. IIUC, controls are supposed to control logical parameters of the system. In this case you could introduce an "anti-eclipse reference voltage" control with values in the range between 0 and 2250mV, setting it to anything below 1900mV would turn the enable bit off. Would such a control make sense? Then you might want to ask on the ML, whether this control would make sense as a generic one, not mt9v022 specific. > + > static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > { > struct mt9v022 *mt9v022 = container_of(ctrl->handler, > @@ -504,6 +517,30 @@ static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > range = exp->maximum - exp->minimum; > exp->val = ((data - 1) * range + 239) / 479 + exp->minimum; > return 0; > + case V4L2_CID_HBLANK: > + data = reg_read(client, MT9V022_HORIZONTAL_BLANKING); > + if (data < 0) > + return -EIO; > + ctrl->val = data; > + return 0; > + case V4L2_CID_VBLANK: > + data = reg_read(client, MT9V022_VERTICAL_BLANKING); > + if (data < 0) > + return -EIO; > + ctrl->val = data; > + return 0; > + case V4L2_CID_REG32: > + data = reg_read(client, MT9V022_REG32); > + if (data < 0) > + return -EIO; > + ctrl->val = data; > + return 0; > + case V4L2_CID_ANALOG_CONTROLS: > + data = reg_read(client, MT9V022_ANALOG_CONTROL); > + if (data < 0) > + return -EIO; > + ctrl->val = data; > + return 0; > } > return -EINVAL; > } > @@ -585,6 +622,26 @@ static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl) > return -EIO; > } > return 0; > + case V4L2_CID_HBLANK: > + if (reg_write(client, MT9V022_HORIZONTAL_BLANKING, > + ctrl->val) < 0) > + return -EIO; > + return 0; > + case V4L2_CID_VBLANK: > + if (reg_write(client, MT9V022_VERTICAL_BLANKING, > + ctrl->val) < 0) > + return -EIO; > + return 0; > + case V4L2_CID_REG32: > + if (reg_write(client, MT9V022_REG32, > + ctrl->val) < 0) > + return -EIO; > + return 0; > + case V4L2_CID_ANALOG_CONTROLS: > + if (reg_write(client, MT9V022_ANALOG_CONTROL, > + ctrl->val) < 0) > + return -EIO; > + return 0; > } > return -EINVAL; > } > @@ -697,6 +754,30 @@ static const struct v4l2_ctrl_ops mt9v022_ctrl_ops = { > .s_ctrl = mt9v022_s_ctrl, > }; > > +static const struct v4l2_ctrl_config mt9v022_ctrls[] = { > + { > + .ops = &mt9v022_ctrl_ops, > + .id = V4L2_CID_REG32, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "reg32 (0x20)", > + .min = 0, > + .max = 0x0fff, > + .step = 1, > + .def = 0x01d1, > + .flags = V4L2_CTRL_FLAG_VOLATILE, > + }, { > + .ops = &mt9v022_ctrl_ops, > + .id = V4L2_CID_ANALOG_CONTROLS, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "analog controls", > + .min = 0, > + .max = 0xffff, > + .step = 1, > + .def = 0x1840, > + .flags = V4L2_CTRL_FLAG_VOLATILE, > + }, > +}; > + > static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = { > .g_chip_ident = mt9v022_g_chip_ident, > #ifdef CONFIG_VIDEO_ADV_DEBUG > @@ -832,7 +913,7 @@ static int mt9v022_probe(struct i2c_client *client, > return -ENOMEM; > > v4l2_i2c_subdev_init(&mt9v022->subdev, client, &mt9v022_subdev_ops); > - v4l2_ctrl_handler_init(&mt9v022->hdl, 6); > + v4l2_ctrl_handler_init(&mt9v022->hdl, 6 + ARRAY_SIZE(mt9v022_ctrls)); > v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, > V4L2_CID_VFLIP, 0, 1, 1, 0); > v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, > @@ -852,10 +933,24 @@ static int mt9v022_probe(struct i2c_client *client, > mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, > V4L2_CID_EXPOSURE, 1, 255, 1, 255); > > + mt9v022->hblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, > + V4L2_CID_HBLANK, MT9V022_HORIZONTAL_BLANKING_MIN, > + MT9V022_HORIZONTAL_BLANKING_MAX, 1, > + MT9V022_HORIZONTAL_BLANKING_DEF); > + > + mt9v022->vblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, > + V4L2_CID_VBLANK, MT9V022_VERTICAL_BLANKING_MIN, > + MT9V022_VERTICAL_BLANKING_MAX, 1, > + MT9V022_VERTICAL_BLANKING_DEF); > + > + v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[0], NULL); > + v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[1], NULL); > + > mt9v022->subdev.ctrl_handler = &mt9v022->hdl; > if (mt9v022->hdl.error) { > int err = mt9v022->hdl.error; > > + dev_err(&client->dev, "hdl init err %d\n", err); That's not very clear IMHO. "hdl" isn't too specific, just "control initialisation?" > kfree(mt9v022); > return err; > } > -- > 1.7.1 > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guennadi, > Hi Anatolij > > On Fri, 24 Aug 2012, Anatolij Gustschin wrote: > >> Add controls for horizontal and vertical blanking, analog control >> and control for undocumented register 32. > > Sorry, I don't think this is a good idea to export an undocumented > register as a control. Why exactly is that? Even though it is not documented, we need to fiddle with it to make our application work at all. So we tend to believe that other users of the chip will want to use it also. Furthermore I don't see that we fundamentally reject patches for other parts in the Linux kernel where people found out things not in the official datasheets. But I agree that better documentation would be valuable - we are trying to find out more information on the original > At most I would add a platform parameter for it, if really needed. Or > do you have to switch it at run-time? If so, you would have some idea > - at what time to switch it to what value and what effect that > produces. Then you could define a _logical_ control. If you absolutely > need to write random values to various registers, you can use > VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER. I made a mistake of using them once in an application before I noticed the comments immediately above in the header file[1]: /* * A D V A N C E D D E B U G G I N G * * NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS! * FOR DEBUGGING, TESTING AND INTERNAL USE ONLY! */ /* VIDIOC_DBG_G_REGISTER and VIDIOC_DBG_S_REGISTER */ At that time I was burned by the API changing (the undocumented register does not go away) and thus this time we want to follow the advice in the header file. So I really hope that you do not suggest that we use this in an application or do you? Best wishes Detlev [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=include/linux/videodev2.h
Hi Guennadi, On Fri, 24 Aug 2012 13:08:52 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: ... > > +#define MT9V022_HORIZONTAL_BLANKING_MIN 43 > > +#define MT9V022_HORIZONTAL_BLANKING_MAX 1023 > > +#define MT9V022_HORIZONTAL_BLANKING_DEF 94 > > +#define MT9V022_VERTICAL_BLANKING_MIN 2 > > Interesting, in my datasheet min is 4. Maybe 4 would be a safer bet then. The legal range in the datasheet here is 2-3000. The datasheet states that the minimal value must be 4 only if "show dark rows" control bit is set (it is unset by default). ... > > +#define V4L2_CID_REG32 (V4L2_CTRL_CLASS_CAMERA | 0x1001) > > +#define V4L2_CID_ANALOG_CONTROLS (V4L2_CTRL_CLASS_CAMERA | 0x1002) > > Sorry, no again. The MT9V022_ANALOG_CONTROL register contains two fields: > anti-eclipse and "anti-eclipse reference voltage control," don't think > they should be set as a single control value. IIUC, controls are supposed > to control logical parameters of the system. In this case you could > introduce an "anti-eclipse reference voltage" control with values in the > range between 0 and 2250mV, setting it to anything below 1900mV would turn > the enable bit off. Would such a control make sense? Then you might want > to ask on the ML, whether this control would make sense as a generic one, > not mt9v022 specific. It probably makes sense since other sensors also have anti-eclipse control registers. ... > > if (mt9v022->hdl.error) { > > int err = mt9v022->hdl.error; > > > > + dev_err(&client->dev, "hdl init err %d\n", err); > > That's not very clear IMHO. "hdl" isn't too specific, just "control > initialisation?" Ok, I'll fix it. Thanks, Anatolij -- 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 Detlev On Fri, 24 Aug 2012, Detlev Zundel wrote: > Hello Guennadi, > > > Hi Anatolij > > > > On Fri, 24 Aug 2012, Anatolij Gustschin wrote: > > > >> Add controls for horizontal and vertical blanking, analog control > >> and control for undocumented register 32. > > > > Sorry, I don't think this is a good idea to export an undocumented > > register as a control. > > Why exactly is that? Even though it is not documented, we need to > fiddle with it to make our application work at all. So we tend to > believe that other users of the chip will want to use it also. Below I asked to provide details about how you have to change this register value: toggle dynamically at run-time or just set once at initialisation? Even if toggle: are this certain moments, related to standard camera activities (e.g., starting and stopping streaming, changing geometry etc.) or you have to set this absolutely asynchronously at moments of time, that only your application knows about? > Furthermore I don't see that we fundamentally reject patches for other > parts in the Linux kernel where people found out things not in the > official datasheets. The problem is not, that this register is undocumented, the problem rather is, that IMHO exporting an API to user-space, setting an undocumented register to arbitrary values is, hm, at least pretty dubious. > But I agree that better documentation would be valuable - we are trying > to find out more information on the original That would be great, thanks! > > At most I would add a platform parameter for it, if really needed. Or > > do you have to switch it at run-time? If so, you would have some idea > > - at what time to switch it to what value and what effect that > > produces. Then you could define a _logical_ control. If you absolutely > > need to write random values to various registers, you can use > > VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER. > > I made a mistake of using them once in an application before I noticed > the comments immediately above in the header file[1]: > > /* > * A D V A N C E D D E B U G G I N G > * > * NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS! > * FOR DEBUGGING, TESTING AND INTERNAL USE ONLY! > */ > > /* VIDIOC_DBG_G_REGISTER and VIDIOC_DBG_S_REGISTER */ > > At that time I was burned by the API changing (the undocumented register > does not go away) and thus this time we want to follow the advice in the > header file. So I really hope that you do not suggest that we use this > in an application or do you? Not in a production application of course, no. That's why I asked to explain how and why this register has to be changed. 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, > Hi Detlev > > On Fri, 24 Aug 2012, Detlev Zundel wrote: > >> Hello Guennadi, >> >> > Hi Anatolij >> > >> > On Fri, 24 Aug 2012, Anatolij Gustschin wrote: >> > >> >> Add controls for horizontal and vertical blanking, analog control >> >> and control for undocumented register 32. >> > >> > Sorry, I don't think this is a good idea to export an undocumented >> > register as a control. >> >> Why exactly is that? Even though it is not documented, we need to >> fiddle with it to make our application work at all. So we tend to >> believe that other users of the chip will want to use it also. > > Below I asked to provide details about how you have to change this > register value: toggle dynamically at run-time or just set once at > initialisation? Even if toggle: are this certain moments, related to > standard camera activities (e.g., starting and stopping streaming, > changing geometry etc.) or you have to set this absolutely asynchronously > at moments of time, that only your application knows about? Anatolij can answer those detail questions, all I know is that without fiddling with the register we do not receive valid pictures at all. >> Furthermore I don't see that we fundamentally reject patches for other >> parts in the Linux kernel where people found out things not in the >> official datasheets. > > The problem is not, that this register is undocumented, the problem rather > is, that IMHO exporting an API to user-space, setting an undocumented > register to arbitrary values is, hm, at least pretty dubious. As I wrote above, without fiddling with the register, we do _not_ receive correct pictures at all. So this is not dubious but shown by experiment to be needed (at least in our setup). Best wishes Detlev
Hi Guennadi, On Fri, 24 Aug 2012 15:35:59 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: ... > Below I asked to provide details about how you have to change this > register value: toggle dynamically at run-time or just set once at > initialisation? Even if toggle: are this certain moments, related to > standard camera activities (e.g., starting and stopping streaming, > changing geometry etc.) or you have to set this absolutely asynchronously > at moments of time, that only your application knows about? Every time the sensor is reset, it resets this register. Without setting the register after sensor reset to the needed value I only get garbage data from the sensor. Since the possibility to reset the sensor is provided on the hardware and also used, the register has to be set after each sensor reset. Only the instance controlling the reset gpio pin "knows" the time, when the register should be initialized again, so it is asynchronously and not related to the standard camera activities. But since the stuff is _not_ documented, I can only speculate. Maybe it can be set to different values to achieve different things, currently I do not know. Thanks, Anatolij -- 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 Fri, 24 Aug 2012, Anatolij Gustschin wrote: > Hi Guennadi, > > On Fri, 24 Aug 2012 15:35:59 +0200 (CEST) > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > ... > > Below I asked to provide details about how you have to change this > > register value: toggle dynamically at run-time or just set once at > > initialisation? Even if toggle: are this certain moments, related to > > standard camera activities (e.g., starting and stopping streaming, > > changing geometry etc.) or you have to set this absolutely asynchronously > > at moments of time, that only your application knows about? > > Every time the sensor is reset, it resets this register. Without setting > the register after sensor reset to the needed value I only get garbage data > from the sensor. Since the possibility to reset the sensor is provided on > the hardware and also used, the register has to be set after each sensor > reset. Only the instance controlling the reset gpio pin "knows" the time, > when the register should be initialized again, so it is asynchronously and > not related to the standard camera activities. But since the stuff is _not_ > documented, I can only speculate. Maybe it can be set to different values > to achieve different things, currently I do not know. How about adding that register write (if required by platform data) to mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at least) on each first open(), would this suffice? 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Fri, 24 Aug 2012 23:23:37 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: ... > > Every time the sensor is reset, it resets this register. Without setting > > the register after sensor reset to the needed value I only get garbage data > > from the sensor. Since the possibility to reset the sensor is provided on > > the hardware and also used, the register has to be set after each sensor > > reset. Only the instance controlling the reset gpio pin "knows" the time, > > when the register should be initialized again, so it is asynchronously and > > not related to the standard camera activities. But since the stuff is _not_ > > documented, I can only speculate. Maybe it can be set to different values > > to achieve different things, currently I do not know. > > How about adding that register write (if required by platform data) to > mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at > least) on each first open(), would this suffice? This would suffice. But now I found some more info for this register. Rev3. errata mentions that the bit 9 of the register should be set when in snapshot mode (in my case this is the only mode that we can use). Additionally the errata recommends to set bit 2 when the high dynamic range mode is used. Now I'm not sure how to realise these settings. The bit 9 should be set/unset when configuring the master/snapshot mode in mt9v022_s_stream(), I think. For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control which primarily configures the HiDy mode in register 0x0f and additionally sets/unsets bit 2 in register 0x20. But setting this bit 2 seems to be needed also in linear mode when manual exposure control is used. With the recommended register value 0x03D1 in linear mode the image quality is really bad when manual exposure mode is used, independent of the configured exposure time. Using 0x03D5 in linear mode however gives good image quality here. So setting bit 2 should be independent of HiDy control. The errata states "the register setting recommendations are based on the characterization of the image sensor only and that camera module makers should test these recommendations on their module and evaluate the overall performance". These settings should be configurable independently of each other, I think. Thanks, Anatolij -- 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 Anatolij On Tue, 28 Aug 2012, Anatolij Gustschin wrote: > Hi Guennadi, > > On Fri, 24 Aug 2012 23:23:37 +0200 (CEST) > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > ... > > > Every time the sensor is reset, it resets this register. Without setting > > > the register after sensor reset to the needed value I only get garbage data > > > from the sensor. Since the possibility to reset the sensor is provided on > > > the hardware and also used, the register has to be set after each sensor > > > reset. Only the instance controlling the reset gpio pin "knows" the time, > > > when the register should be initialized again, so it is asynchronously and > > > not related to the standard camera activities. But since the stuff is _not_ > > > documented, I can only speculate. Maybe it can be set to different values > > > to achieve different things, currently I do not know. > > > > How about adding that register write (if required by platform data) to > > mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at > > least) on each first open(), would this suffice? > > This would suffice. But now I found some more info for this register. > Rev3. errata Yes, I've found that one too. But I don't understand the table they present there: have only default values of various registers changed, or have actually new features been added in Rev.3? If the latter we'd have to find out which revision we're running on. > mentions that the bit 9 of the register should be set when > in snapshot mode (in my case this is the only mode that we can use). Currently the snapshot mode is used in the mt9v022 driver to stop streaming. This means you've got more local changes in your driver to enable capture in the snapshot mode? > Additionally the errata recommends to set bit 2 when the high dynamic > range mode is used. The HiDy mode is also not used so far in the driver. > Now I'm not sure how to realise these settings. > > The bit 9 should be set/unset when configuring the master/snapshot > mode in mt9v022_s_stream(), I think. As mentioned above, currently the snapshot mode is used by the driver to stop continuous data read-out by the sensor, so, unless we begin supporting capture in the snapshot mode, setting any further configuration parameters seems pretty useless to me. > For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control > which primarily configures the HiDy mode in register 0x0f and > additionally sets/unsets bit 2 in register 0x20. But setting this > bit 2 seems to be needed also in linear mode when manual exposure > control is used. With the recommended register value 0x03D1 in linear > mode the image quality is really bad when manual exposure mode is > used, independent of the configured exposure time. Using 0x03D5 > in linear mode however gives good image quality here. Doesn't this switch on the HiDy mode? What happens if you also set 0x0f:6 to 1? > So setting > bit 2 should be independent of HiDy control. The errata states "the > register setting recommendations are based on the characterization of > the image sensor only and that camera module makers should test these > recommendations on their module and evaluate the overall performance". > These settings should be configurable independently of each other, > I think. Maybe we need some noise (PFN) related control? 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Tue, 11 Sep 2012 10:24:23 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Hi Anatolij > > On Tue, 28 Aug 2012, Anatolij Gustschin wrote: > > > Hi Guennadi, > > > > On Fri, 24 Aug 2012 23:23:37 +0200 (CEST) > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > ... > > > > Every time the sensor is reset, it resets this register. Without setting > > > > the register after sensor reset to the needed value I only get garbage data > > > > from the sensor. Since the possibility to reset the sensor is provided on > > > > the hardware and also used, the register has to be set after each sensor > > > > reset. Only the instance controlling the reset gpio pin "knows" the time, > > > > when the register should be initialized again, so it is asynchronously and > > > > not related to the standard camera activities. But since the stuff is _not_ > > > > documented, I can only speculate. Maybe it can be set to different values > > > > to achieve different things, currently I do not know. > > > > > > How about adding that register write (if required by platform data) to > > > mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at > > > least) on each first open(), would this suffice? > > > > This would suffice. But now I found some more info for this register. > > Rev3. errata > > Yes, I've found that one too. But I don't understand the table they > present there: have only default values of various registers changed, or > have actually new features been added in Rev.3? If the latter we'd have to > find out which revision we're running on. Some issues have been fixed in Rev3 so that these register settings can work. I've found the technical note describing the snapshot mode and it says that register 0x20 bit 2 and bit 9 must be set in the snapshot mode. This applies to mt9v022 rev3 and mt9v024, so it should be configured dependent on revision, yes. I'll remove register 0x20 setting control entirely and provide separate patch configuring required settings in snapshot mode. > > mentions that the bit 9 of the register should be set when > > in snapshot mode (in my case this is the only mode that we can use). > > Currently the snapshot mode is used in the mt9v022 driver to stop > streaming. This means you've got more local changes in your driver to > enable capture in the snapshot mode? Yes, in my case the capture driver sets this mode in its start_streaming function. I'll submit the driver, too. > > Additionally the errata recommends to set bit 2 when the high dynamic > > range mode is used. > > The HiDy mode is also not used so far in the driver. Yes. I think support for it could be added as V4L2_CID_WIDE_DYNAMIC_RANGE control. > > Now I'm not sure how to realise these settings. > > > > The bit 9 should be set/unset when configuring the master/snapshot > > mode in mt9v022_s_stream(), I think. > > As mentioned above, currently the snapshot mode is used by the driver to > stop continuous data read-out by the sensor, so, unless we begin > supporting capture in the snapshot mode, setting any further configuration > parameters seems pretty useless to me. We need the snapshot mode, our camera doesn't work in normal mode, only external triggering is supported on it. I'll add required configuration by a separate patch. > > For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control > > which primarily configures the HiDy mode in register 0x0f and > > additionally sets/unsets bit 2 in register 0x20. But setting this > > bit 2 seems to be needed also in linear mode when manual exposure > > control is used. With the recommended register value 0x03D1 in linear > > mode the image quality is really bad when manual exposure mode is > > used, independent of the configured exposure time. Using 0x03D5 > > in linear mode however gives good image quality here. > > Doesn't this switch on the HiDy mode? What happens if you also set 0x0f:6 > to 1? No, it doesn't. It is supposed to lower pixel-wise FPN. If I configure HiDy mode by setting 0x0f:6 to 1, I get better image quality, the loss of detail in very bright or dark areas of a picture is reduced, as expected. > > > So setting > > bit 2 should be independent of HiDy control. The errata states "the > > register setting recommendations are based on the characterization of > > the image sensor only and that camera module makers should test these > > recommendations on their module and evaluate the overall performance". > > These settings should be configurable independently of each other, > > I think. > > Maybe we need some noise (PFN) related control? Maybe. Thanks, Anatolij -- 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
diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c index 350d0d8..d13c8c4 100644 --- a/drivers/media/i2c/soc_camera/mt9v022.c +++ b/drivers/media/i2c/soc_camera/mt9v022.c @@ -50,12 +50,14 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\""); #define MT9V022_PIXEL_OPERATION_MODE 0x0f #define MT9V022_LED_OUT_CONTROL 0x1b #define MT9V022_ADC_MODE_CONTROL 0x1c +#define MT9V022_REG32 0x20 #define MT9V022_ANALOG_GAIN 0x35 #define MT9V022_BLACK_LEVEL_CALIB_CTRL 0x47 #define MT9V022_PIXCLK_FV_LV 0x74 #define MT9V022_DIGITAL_TEST_PATTERN 0x7f #define MT9V022_AEC_AGC_ENABLE 0xAF #define MT9V022_MAX_TOTAL_SHUTTER_WIDTH 0xBD +#define MT9V022_ANALOG_CONTROL 0xC2 /* mt9v024 partial list register addresses changes with respect to mt9v022 */ #define MT9V024_PIXCLK_FV_LV 0x72 @@ -71,6 +73,13 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\""); #define MT9V022_COLUMN_SKIP 1 #define MT9V022_ROW_SKIP 4 +#define MT9V022_HORIZONTAL_BLANKING_MIN 43 +#define MT9V022_HORIZONTAL_BLANKING_MAX 1023 +#define MT9V022_HORIZONTAL_BLANKING_DEF 94 +#define MT9V022_VERTICAL_BLANKING_MIN 2 +#define MT9V022_VERTICAL_BLANKING_MAX 3000 +#define MT9V022_VERTICAL_BLANKING_DEF 45 + #define is_mt9v024(id) (id == 0x1324) /* MT9V022 has only one fixed colorspace per pixelcode */ @@ -136,6 +145,8 @@ struct mt9v022 { struct v4l2_ctrl *autogain; struct v4l2_ctrl *gain; }; + struct v4l2_ctrl *hblank; + struct v4l2_ctrl *vblank; struct v4l2_rect rect; /* Sensor window */ const struct mt9v022_datafmt *fmt; const struct mt9v022_datafmt *fmts; @@ -277,11 +288,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a) * Default 94, Phytec driver says: * "width + horizontal blank >= 660" */ - ret = reg_write(client, MT9V022_HORIZONTAL_BLANKING, - rect.width > 660 - 43 ? 43 : - 660 - rect.width); + ret = v4l2_ctrl_s_ctrl(mt9v022->hblank, + rect.width > 660 - 43 ? 43 : 660 - rect.width); if (!ret) - ret = reg_write(client, MT9V022_VERTICAL_BLANKING, 45); + ret = v4l2_ctrl_s_ctrl(mt9v022->vblank, 45); if (!ret) ret = reg_write(client, MT9V022_WINDOW_WIDTH, rect.width); if (!ret) @@ -476,6 +486,9 @@ static int mt9v022_s_power(struct v4l2_subdev *sd, int on) return soc_camera_set_power(&client->dev, icl, on); } +#define V4L2_CID_REG32 (V4L2_CTRL_CLASS_CAMERA | 0x1001) +#define V4L2_CID_ANALOG_CONTROLS (V4L2_CTRL_CLASS_CAMERA | 0x1002) + static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl) { struct mt9v022 *mt9v022 = container_of(ctrl->handler, @@ -504,6 +517,30 @@ static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl) range = exp->maximum - exp->minimum; exp->val = ((data - 1) * range + 239) / 479 + exp->minimum; return 0; + case V4L2_CID_HBLANK: + data = reg_read(client, MT9V022_HORIZONTAL_BLANKING); + if (data < 0) + return -EIO; + ctrl->val = data; + return 0; + case V4L2_CID_VBLANK: + data = reg_read(client, MT9V022_VERTICAL_BLANKING); + if (data < 0) + return -EIO; + ctrl->val = data; + return 0; + case V4L2_CID_REG32: + data = reg_read(client, MT9V022_REG32); + if (data < 0) + return -EIO; + ctrl->val = data; + return 0; + case V4L2_CID_ANALOG_CONTROLS: + data = reg_read(client, MT9V022_ANALOG_CONTROL); + if (data < 0) + return -EIO; + ctrl->val = data; + return 0; } return -EINVAL; } @@ -585,6 +622,26 @@ static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl) return -EIO; } return 0; + case V4L2_CID_HBLANK: + if (reg_write(client, MT9V022_HORIZONTAL_BLANKING, + ctrl->val) < 0) + return -EIO; + return 0; + case V4L2_CID_VBLANK: + if (reg_write(client, MT9V022_VERTICAL_BLANKING, + ctrl->val) < 0) + return -EIO; + return 0; + case V4L2_CID_REG32: + if (reg_write(client, MT9V022_REG32, + ctrl->val) < 0) + return -EIO; + return 0; + case V4L2_CID_ANALOG_CONTROLS: + if (reg_write(client, MT9V022_ANALOG_CONTROL, + ctrl->val) < 0) + return -EIO; + return 0; } return -EINVAL; } @@ -697,6 +754,30 @@ static const struct v4l2_ctrl_ops mt9v022_ctrl_ops = { .s_ctrl = mt9v022_s_ctrl, }; +static const struct v4l2_ctrl_config mt9v022_ctrls[] = { + { + .ops = &mt9v022_ctrl_ops, + .id = V4L2_CID_REG32, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "reg32 (0x20)", + .min = 0, + .max = 0x0fff, + .step = 1, + .def = 0x01d1, + .flags = V4L2_CTRL_FLAG_VOLATILE, + }, { + .ops = &mt9v022_ctrl_ops, + .id = V4L2_CID_ANALOG_CONTROLS, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "analog controls", + .min = 0, + .max = 0xffff, + .step = 1, + .def = 0x1840, + .flags = V4L2_CTRL_FLAG_VOLATILE, + }, +}; + static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = { .g_chip_ident = mt9v022_g_chip_ident, #ifdef CONFIG_VIDEO_ADV_DEBUG @@ -832,7 +913,7 @@ static int mt9v022_probe(struct i2c_client *client, return -ENOMEM; v4l2_i2c_subdev_init(&mt9v022->subdev, client, &mt9v022_subdev_ops); - v4l2_ctrl_handler_init(&mt9v022->hdl, 6); + v4l2_ctrl_handler_init(&mt9v022->hdl, 6 + ARRAY_SIZE(mt9v022_ctrls)); v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, @@ -852,10 +933,24 @@ static int mt9v022_probe(struct i2c_client *client, mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, V4L2_CID_EXPOSURE, 1, 255, 1, 255); + mt9v022->hblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, + V4L2_CID_HBLANK, MT9V022_HORIZONTAL_BLANKING_MIN, + MT9V022_HORIZONTAL_BLANKING_MAX, 1, + MT9V022_HORIZONTAL_BLANKING_DEF); + + mt9v022->vblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops, + V4L2_CID_VBLANK, MT9V022_VERTICAL_BLANKING_MIN, + MT9V022_VERTICAL_BLANKING_MAX, 1, + MT9V022_VERTICAL_BLANKING_DEF); + + v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[0], NULL); + v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[1], NULL); + mt9v022->subdev.ctrl_handler = &mt9v022->hdl; if (mt9v022->hdl.error) { int err = mt9v022->hdl.error; + dev_err(&client->dev, "hdl init err %d\n", err); kfree(mt9v022); return err; }
Add controls for horizontal and vertical blanking, analog control and control for undocumented register 32. Also add an error message for case that the control handler init failed. Since setting the blanking registers is done by controls now, we should't change these registers outside of the control function. Use v4l2_ctrl_s_ctrl() to set them. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- drivers/media/i2c/soc_camera/mt9v022.c | 105 ++++++++++++++++++++++++++++++-- 1 files changed, 100 insertions(+), 5 deletions(-)