Re: [PATCH 01/28] media: ov2680: Remove auto-gain and auto-exposure controls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/06/2023 08:44, Hans de Goede wrote:
Hi Dan,

On 6/12/23 08:53, Dan Scally wrote:
Morning Hans - thanks for the set

On 07/06/2023 17:46, Hans de Goede wrote:
Quoting the OV2680 datasheet:

"3.2 exposure and gain control

In the OV2680, the exposure time and gain are set manually from an external
controller. The OV2680 supports manual gain and exposure control only for
normal applications, no auto mode."

And indeed testing with the atomisp_ov2680 fork of ov2680.c has shown that
auto-exposure and auto-gain do not work.

Note that the code setting the auto-exposure flag was broken, callers
of ov2680_exposure_set() were directly passing !!ctrls->auto_exp->val as
"bool auto_exp" value, but ctrls->auto_exp is a menu control with:

enum  v4l2_exposure_auto_type {
          V4L2_EXPOSURE_AUTO = 0,
          V4L2_EXPOSURE_MANUAL = 1,
     ...

So instead of passing !!ctrls->auto_exp->val they should have been passing
ctrls->auto_exp->val == V4L2_EXPOSURE_AUTO, iow the passed value was
inverted of what it should have been.

Also remove ov2680_g_volatile_ctrl() since without auto support the gain
and exposure controls are not volatile.

This also fixes the control values not being properly applied in
ov2680_mode_set(). The 800x600 mode register-list also sets gain,
exposure and vflip overriding the last set ctrl values.

ov2680_mode_set() does call ov2680_gain_set() and ov2680_exposure_set()
but did this before writing the mode register-list, so these values
would still be overridden by the mode register-list.

Add a v4l2_ctrl_handler_setup() call after writing the mode register-list
to restore all ctrl values. Also remove the ctrls->gain->is_new check from
ov2680_gain_set() so that the gain always gets restored properly.

Last since ov2680_mode_set() now calls v4l2_ctrl_handler_setup(), remove
the v4l2_ctrl_handler_setup() call after ov2680_mode_restore() since
ov2680_mode_restore() calls ov2680_mode_set().

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
   drivers/media/i2c/ov2680.c | 162 ++++---------------------------------
   1 file changed, 17 insertions(+), 145 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 54153bf66bdd..02204e185e2f 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -85,15 +85,8 @@ struct ov2680_mode_info {
     struct ov2680_ctrls {
       struct v4l2_ctrl_handler handler;
-    struct {
-        struct v4l2_ctrl *auto_exp;
-        struct v4l2_ctrl *exposure;
-    };
-    struct {
-        struct v4l2_ctrl *auto_gain;
-        struct v4l2_ctrl *gain;
-    };
-
+    struct v4l2_ctrl *exposure;
+    struct v4l2_ctrl *gain;
       struct v4l2_ctrl *hflip;
       struct v4l2_ctrl *vflip;
       struct v4l2_ctrl *test_pattern;
@@ -143,6 +136,7 @@ static const struct reg_value ov2680_setting_30fps_QUXGA_800_600[] = {
       {0x380e, 0x02}, {0x380f, 0x84}, {0x3811, 0x04}, {0x3813, 0x04},
       {0x3814, 0x31}, {0x3815, 0x31}, {0x3820, 0xc0}, {0x4008, 0x00},
       {0x4009, 0x03}, {0x4837, 0x1e}, {0x3501, 0x4e}, {0x3502, 0xe0},
+    {0x3503, 0x03},

I'm confused why this was added to this mode but not the other two; according to the datasheet it's setting the AGC/AEC into manual but I can't see why it's only needed here?
The original driver uses the 800x600 mode to also store "init_mode" registers,
no matter what mode is set, first all 800x600 registers are written and then
the other mode's registers are written over that.


Ah yeah I see it now. Looks good to me then: Reviewed-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>


This ugliness goes away later in the series when this gets renamed to
ov2680_global_setting[] and all the mode specific registers are dropped
because they are calculated based on crop + binning.

When switching to calculating the mode registers the register poke also gets some docs :)

         /* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
         {0x3503, 0x03},


That sounds like it will be much better


Regards,

Hans




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux