diff mbox series

[v2] media: i2c: ov5645: Remove software reset entry from ov5645_global_init_setting

Message ID 20240215144333.82036-1-biju.das.jz@bp.renesas.com (mailing list archive)
State Accepted
Delegated to: Kieran Bingham
Headers show
Series [v2] media: i2c: ov5645: Remove software reset entry from ov5645_global_init_setting | expand

Commit Message

Biju Das Feb. 15, 2024, 2:43 p.m. UTC
The SYSTEM_CTRL0 register in ov5645 is almost similar to ov5640. But the
hard reset control is mandatory for the ov5645 device, so there is no
need for soft reset in the driver.

Add a 1msec delay for the software power up (OV5645_SYSTEM_CTRL0_START)
register in ov5645_global_init_setting. Without this delay sometimes the
image is not captured at all when the i2c frequency is 400kHz.

The changes are fixing both the greenish issue and image capture at 400kHz
i2c frequency on Renesas RZ/G2L SMARC EVK platforms.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Dropped patch#2
 * Updated commit header and description.
 * Dropped unneeded software reset from ov5645_global_init_setting
 * Added 1 msec delay for software power up from ov5645_*_init_setting.
---
 drivers/media/i2c/ov5645.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Wolfram Sang Feb. 15, 2024, 2:51 p.m. UTC | #1
On Thu, Feb 15, 2024 at 02:43:33PM +0000, Biju Das wrote:
> The SYSTEM_CTRL0 register in ov5645 is almost similar to ov5640. But the
> hard reset control is mandatory for the ov5645 device, so there is no
> need for soft reset in the driver.
> 
> Add a 1msec delay for the software power up (OV5645_SYSTEM_CTRL0_START)
> register in ov5645_global_init_setting. Without this delay sometimes the
> image is not captured at all when the i2c frequency is 400kHz.
> 
> The changes are fixing both the greenish issue and image capture at 400kHz
> i2c frequency on Renesas RZ/G2L SMARC EVK platforms.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Much better IMO.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Kieran Bingham Feb. 15, 2024, 4:33 p.m. UTC | #2
Quoting Biju Das (2024-02-15 14:43:33)
> The SYSTEM_CTRL0 register in ov5645 is almost similar to ov5640. But the
> hard reset control is mandatory for the ov5645 device, so there is no
> need for soft reset in the driver.
> 
> Add a 1msec delay for the software power up (OV5645_SYSTEM_CTRL0_START)
> register in ov5645_global_init_setting. Without this delay sometimes the
> image is not captured at all when the i2c frequency is 400kHz.
> 
> The changes are fixing both the greenish issue and image capture at 400kHz
> i2c frequency on Renesas RZ/G2L SMARC EVK platforms.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Dropped patch#2
>  * Updated commit header and description.
>  * Dropped unneeded software reset from ov5645_global_init_setting
>  * Added 1 msec delay for software power up from ov5645_*_init_setting.

As with all drivers, I'd love to see more cleanup to move the reset/init
sequences away from opaque tables. But for now this seems like a
reasonable improvement.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5645.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a26ac11c989d..df86b951b9f3 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -118,7 +118,6 @@ static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
>  
>  static const struct reg_value ov5645_global_init_setting[] = {
>         { 0x3103, 0x11 },
> -       { 0x3008, 0x82 },
>         { 0x3008, 0x42 },
>         { 0x3103, 0x03 },
>         { 0x3503, 0x07 },
> @@ -627,6 +626,10 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
>                 ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
>                 if (ret < 0)
>                         return ret;
> +
> +               if (settings->reg == OV5645_SYSTEM_CTRL0 &&
> +                   settings->val == OV5645_SYSTEM_CTRL0_START)
> +                       usleep_range(1000, 2000);
>         }
>  
>         return 0;
> -- 
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a26ac11c989d..df86b951b9f3 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -118,7 +118,6 @@  static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
 
 static const struct reg_value ov5645_global_init_setting[] = {
 	{ 0x3103, 0x11 },
-	{ 0x3008, 0x82 },
 	{ 0x3008, 0x42 },
 	{ 0x3103, 0x03 },
 	{ 0x3503, 0x07 },
@@ -627,6 +626,10 @@  static int ov5645_set_register_array(struct ov5645 *ov5645,
 		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
 		if (ret < 0)
 			return ret;
+
+		if (settings->reg == OV5645_SYSTEM_CTRL0 &&
+		    settings->val == OV5645_SYSTEM_CTRL0_START)
+			usleep_range(1000, 2000);
 	}
 
 	return 0;