Re: [PATCH 3/5] media: atomisp: ov2680: Remove unnecessary registers from ov2680_global_setting[]

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

 



Hi,

On 6/4/23 21:23, Andy Shevchenko wrote:
> 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?

The sleep is taken from the drivers/media/i2c/ov2680.c version
of the driver, to replace writing the reset reg twice.

The datasheet says the first i2c-transaction after a reset
needs to be delayed by 8192 cycles of the provided external clk,
which with a 19.2MHz clk is roughly 0.5 ms.  But that is for
a reset through the XSHUTDOWN pin of the sensor. How long
a sw initiated reset takes is not specified.

Regards,

Hans



> 
>>         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
>>
> 
> 





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux