| Message ID | 20230604161406.69369-1-hdegoede@redhat.com |
|---|---|
| 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 >
On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi All, > > Here is some more ov2680 sensor driver work. This work is the result > of trying to get the main drivers/media/i2c/ov2680.c driver in a shape > where it is good enough to replace the atomisp specific version. > > The plan is to port recent improvements to atomisp-ov2680.c over > to the main driver. While working on this I noticed some issues which > need fixing before copying them over to the "main" driver. > > Besides that this also adds a small patch to make testing with > gstreamer easier and this adds testing instruction to the TODO file. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> (a nit-pick in one patch commented separately) Thank you! > Regards, > > Hans > > > Hans de Goede (5): > media: atomisp: Stop resetting selected input to 0 between /dev/video# > opens > media: atomisp: ov2680: Stop using half pixelclock for binned modes > media: atomisp: ov2680: Remove unnecessary registers from > ov2680_global_setting[] > media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a > media: atomisp: Add testing instructions to TODO file > > drivers/staging/media/atomisp/TODO | 33 +++++ > .../media/atomisp/i2c/atomisp-ov2680.c | 15 +-- > drivers/staging/media/atomisp/i2c/ov2680.h | 118 +++++++++--------- > .../staging/media/atomisp/pci/atomisp_fops.c | 3 - > 4 files changed, 95 insertions(+), 74 deletions(-) > > -- > 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 >> > >
Hi Andy, On 6/4/23 21:28, Andy Shevchenko wrote: > On Sun, Jun 4, 2023 at 7:14 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> Here is some more ov2680 sensor driver work. This work is the result >> of trying to get the main drivers/media/i2c/ov2680.c driver in a shape >> where it is good enough to replace the atomisp specific version. >> >> The plan is to port recent improvements to atomisp-ov2680.c over >> to the main driver. While working on this I noticed some issues which >> need fixing before copying them over to the "main" driver. >> >> Besides that this also adds a small patch to make testing with >> gstreamer easier and this adds testing instruction to the TODO file. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > (a nit-pick in one patch commented separately) > > Thank you! Thank you for the reviews. I've pushed this series (with the nitpick addressed) as well as the previous 3 fixes you reviewed to my media-atomisp branch now. Regards, Hans >> Hans de Goede (5): >> media: atomisp: Stop resetting selected input to 0 between /dev/video# >> opens >> media: atomisp: ov2680: Stop using half pixelclock for binned modes >> media: atomisp: ov2680: Remove unnecessary registers from >> ov2680_global_setting[] >> media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a >> media: atomisp: Add testing instructions to TODO file >> >> drivers/staging/media/atomisp/TODO | 33 +++++ >> .../media/atomisp/i2c/atomisp-ov2680.c | 15 +-- >> drivers/staging/media/atomisp/i2c/ov2680.h | 118 +++++++++--------- >> .../staging/media/atomisp/pci/atomisp_fops.c | 3 - >> 4 files changed, 95 insertions(+), 74 deletions(-) >> >> -- >> 2.40.1 >> > >
Hi All, Here is some more ov2680 sensor driver work. This work is the result of trying to get the main drivers/media/i2c/ov2680.c driver in a shape where it is good enough to replace the atomisp specific version. The plan is to port recent improvements to atomisp-ov2680.c over to the main driver. While working on this I noticed some issues which need fixing before copying them over to the "main" driver. Besides that this also adds a small patch to make testing with gstreamer easier and this adds testing instruction to the TODO file. Regards, Hans Hans de Goede (5): media: atomisp: Stop resetting selected input to 0 between /dev/video# opens media: atomisp: ov2680: Stop using half pixelclock for binned modes media: atomisp: ov2680: Remove unnecessary registers from ov2680_global_setting[] media: atomisp: ov2680: Rename unknown/0x370a to sensor_ctrl_0a media: atomisp: Add testing instructions to TODO file drivers/staging/media/atomisp/TODO | 33 +++++ .../media/atomisp/i2c/atomisp-ov2680.c | 15 +-- drivers/staging/media/atomisp/i2c/ov2680.h | 118 +++++++++--------- .../staging/media/atomisp/pci/atomisp_fops.c | 3 - 4 files changed, 95 insertions(+), 74 deletions(-)