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

From: Kieran Bingham <kieran.bingham@ideasonboard.com>
To: Biju Das <biju.das.jz@bp.renesas.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Biju Das <biju.das.jz@bp.renesas.com>,
	linux-media@vger.kernel.org,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Wolfram Sang <wsa@kernel.org>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	Biju Das <biju.das.au@gmail.com>,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2] media: i2c: ov5645: Remove software reset entry from ov5645_global_init_setting
Date: Thu, 15 Feb 2024 16:33:08 +0000	[thread overview]
Message-ID: <170801478828.2629073.1256300005311782106@ping.linuxembedded.co.uk> (raw)
In-Reply-To: <20240215144333.82036-1-biju.das.jz@bp.renesas.com>

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
>