| Message ID | 20230604161406.69369-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: ov2680 work + add testing instructions | expand |
On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> > --- > .../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 >
Hi, On 6/4/23 21:23, Andy Shevchenko wrote: > On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> >> --- >> .../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 >> > >
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); ret |= ov2680_write_reg_array(client, ov2680_global_setting); 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 + {} };
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). 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@redhat.com> --- .../media/atomisp/i2c/atomisp-ov2680.c | 1 + drivers/staging/media/atomisp/i2c/ov2680.h | 117 ++++++++---------- 2 files changed, 56 insertions(+), 62 deletions(-)