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.
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},
Regards,
Hans
