On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Many of the values in ov2680_global_setting[] match the default/reset
> register value for the ov2680 sensor (verified with both datasheet
> and actual hw) so they are no-ops.
>
> And there are also a coupe of others which are later overwritten
> by ctrls or by ov2680_set_mode().
>
> Remove all the unnecessary entries and add annotations to
> the remaining entries documenting what they change
> (in sofar as things are documented in the datasheet).
My spell checker suggests either "in so far as" or "insofar as".
> This also removes the double writing of OV2680_REG_SOFT_RESET in
> ov2680_init_registers() (one direct write, one in ov2680_global_setting[])
> instead add a short sleep after the first write to give the sensor
> time to reset.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../media/atomisp/i2c/atomisp-ov2680.c | 1 +
> drivers/staging/media/atomisp/i2c/ov2680.h | 117 ++++++++----------
> 2 files changed, 56 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 1db2eb9f9e25..dcc06c725544 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -175,6 +175,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
> int ret;
>
> ret = ov_write_reg8(client, OV2680_SW_RESET, 0x01);
> + usleep_range(1000, 2000);
Perhaps surround this with blank lines and s little comment?
Also does this sleep is anyhow required by a datasheet, or is it
purely empirical?
> ret |= ov2680_write_reg_array(client, ov2680_global_setting);
Side note: this is weird for an int returned variable.
> return ret;
> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
> index b6c0ef591c69..6a71de55600b 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> @@ -172,82 +172,75 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> }
>
> static struct ov2680_reg const ov2680_global_setting[] = {
> - {0x0103, 0x01},
> - {0x3002, 0x00},
> + /* MIPI PHY, 0x10 -> 0x1c enable bp_c_hs_en_lat and bp_d_hs_en_lat */
> {0x3016, 0x1c},
> - {0x3018, 0x44},
> - {0x3020, 0x00},
> - {0x3080, 0x02},
> +
> + /* PLL MULT bits 0-7, datasheet default 0x37 for 24MHz extclk, use 0x45 for 19.2 Mhz extclk */
> {0x3082, 0x45},
> - {0x3084, 0x09},
> - {0x3085, 0x04},
> - {0x3086, 0x00},
> +
> + /* R MANUAL set exposure (0x01) and gain (0x02) to manual (hw does not do auto) */
> {0x3503, 0x03},
> - {0x350b, 0x36},
> - {0x3600, 0xb4},
> - {0x3603, 0x39},
> - {0x3604, 0x24},
> - {0x3605, 0x00},
> - {0x3620, 0x26},
> - {0x3621, 0x37},
> - {0x3622, 0x04},
> - {0x3628, 0x00},
> - {0x3705, 0x3c},
> - {0x370c, 0x50},
> - {0x370d, 0xc0},
> - {0x3718, 0x88},
> - {0x3720, 0x00},
> - {0x3721, 0x00},
> - {0x3722, 0x00},
> - {0x3723, 0x00},
> - {0x3738, 0x00},
> - {0x3717, 0x58},
> - {0x3781, 0x80},
> - {0x3789, 0x60},
> - {0x3800, 0x00},
> - {0x3819, 0x04},
> +
> + /* Analog control register tweaks */
> + {0x3603, 0x39}, /* Reset value 0x99 */
> + {0x3604, 0x24}, /* Reset value 0x74 */
> + {0x3621, 0x37}, /* Reset value 0x44 */
> +
> + /* Sensor control register tweaks */
> + {0x3701, 0x64}, /* Reset value 0x61 */
> + {0x3705, 0x3c}, /* Reset value 0x21 */
> + {0x370c, 0x50}, /* Reset value 0x10 */
> + {0x370d, 0xc0}, /* Reset value 0x00 */
> + {0x3718, 0x88}, /* Reset value 0x80 */
> +
> + /* PSRAM tweaks */
> + {0x3781, 0x80}, /* Reset value 0x00 */
> + {0x3784, 0x0c}, /* Reset value 0x00, based on OV2680_R1A_AM10.ovt */
> + {0x3789, 0x60}, /* Reset value 0x50 */
> +
> + /* BLC CTRL00 0x01 -> 0x81 set avg_weight to 8 */
> {0x4000, 0x81},
> - {0x4001, 0x40},
> +
> + /* Set black level compensation range to 0 - 3 (default 0 - 11) */
> {0x4008, 0x00},
> {0x4009, 0x03},
> +
> + /* VFIFO R2 0x00 -> 0x02 set Frame reset enable */
> {0x4602, 0x02},
> +
> + /* MIPI ctrl CLK PREPARE MIN change from 0x26 (38) -> 0x36 (54) */
> {0x481f, 0x36},
> +
> + /* MIPI ctrl CLK LPX P MIN change from 0x32 (50) -> 0x36 (54) */
> {0x4825, 0x36},
> - {0x4837, 0x18},
> +
> + /* R ISP CTRL2 0x20 -> 0x30, set sof_sel bit */
> {0x5002, 0x30},
> - {0x5004, 0x04},//manual awb 1x
> - {0x5005, 0x00},
> - {0x5006, 0x04},
> - {0x5007, 0x00},
> - {0x5008, 0x04},
> - {0x5009, 0x00},
> - {0x5080, 0x00},
> - {0x5081, 0x41},
> - {0x5708, 0x01}, /* add for full size flip off and mirror off 2014/09/11 */
> - {0x3701, 0x64}, //add on 14/05/13
> - {0x3784, 0x0c}, //based OV2680_R1A_AM10.ovt add on 14/06/13
> - {0x5780, 0x3e}, //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
> - {0x5781, 0x0f},
> - {0x5782, 0x04},
> - {0x5783, 0x02},
> - {0x5784, 0x01},
> - {0x5785, 0x01},
> - {0x5786, 0x00},
> - {0x5787, 0x04},
> +
> + /*
> + * Window CONTROL 0x00 -> 0x01, enable manual window control,
> + * this is necessary for full size flip and mirror support.
> + */
> + {0x5708, 0x01},
> +
> + /*
> + * DPC CTRL0 0x14 -> 0x3e, set enable_tail, enable_3x3_cluster
> + * and enable_general_tail bits based OV2680_R1A_AM10.ovt.
> + */
> + {0x5780, 0x3e},
> +
> + /* DPC MORE CONNECTION CASE THRE 0x0c (12) -> 0x02 (2) */
> {0x5788, 0x02},
> - {0x5789, 0x00},
> - {0x578a, 0x01},
> - {0x578b, 0x02},
> - {0x578c, 0x03},
> - {0x578d, 0x03},
> +
> + /* DPC GAIN LIST1 0x0f (15) -> 0x08 (8) */
> {0x578e, 0x08},
> +
> + /* DPC GAIN LIST2 0x3f (63) -> 0x0c (12) */
> {0x578f, 0x0c},
> - {0x5790, 0x08},
> - {0x5791, 0x04},
> +
> + /* DPC THRE RATIO 0x04 (4) -> 0x00 (0) */
> {0x5792, 0x00},
> - {0x5793, 0x00},
> - {0x5794, 0x03}, //based OV2680_R1A_AM10.ovt,Adjust DPC setting (57xx) on 14/06/13
> - {0x0100, 0x00}, //stream off
> +
> {}
> };
>
> --
> 2.40.1
>
--
With Best Regards,
Andy Shevchenko
