[PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Zhi Mao]

GT9769 VCM power-on default setting is PD=0,
so it is not necessary to set again in dw9768_init function,
and it also has no requirement of setting PD=1
before power-off in dw9768_release function.
For GT9769 VCM, PD mode control will add extra time
when switching between suspend and resume.
e.g. chrome camera AP can switch between video and photo mode,
the behavior corresponding to VCM is suspend and resume,
it will cause camera preview is not smooth.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/media/i2c/dw9768.c | 65 ++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
index 18ef2b35c9aa..88d96165a805 100644
--- a/drivers/media/i2c/dw9768.c
+++ b/drivers/media/i2c/dw9768.c
@@ -97,12 +97,17 @@ static const char * const dw9768_supply_names[] = {
 	"vdd",	/* Digital core power */
 };
 
+struct dw9768_vcm_data {
+	bool pd_mode_ctrl;
+};
+
 /* dw9768 device structure */
 struct dw9768 {
 	struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *focus;
 	struct v4l2_subdev sd;
+	const struct dw9768_vcm_data *data;
 
 	u32 aac_mode;
 	u32 aac_timing;
@@ -221,18 +226,20 @@ static int dw9768_init(struct dw9768 *dw9768)
 	struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
 	int ret, val;
 
-	/* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
-	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
-					DW9768_PD_MODE_OFF);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * DW9769 requires waiting delay time of t_OPR
-	 * after PD reset takes place.
-	 */
-	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+	if (dw9768->data->pd_mode_ctrl) {
+		/* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
+		ret = i2c_smbus_write_byte_data(client,
+						DW9768_RING_PD_CONTROL_REG,
+						DW9768_PD_MODE_OFF);
+		if (ret < 0)
+			return ret;
 
+		/*
+		 * DW9769 requires waiting delay time of t_OPR
+		 * after PD reset takes place.
+		 */
+		usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+	}
 	/* Set DW9768_RING_PD_CONTROL_REG to DW9768_AAC_MODE_EN(0x01) */
 	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
 					DW9768_AAC_MODE_EN);
@@ -294,17 +301,19 @@ static int dw9768_release(struct dw9768 *dw9768)
 			     dw9768->move_delay_us + 1000);
 	}
 
-	ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
-					DW9768_PD_MODE_EN);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * DW9769 requires waiting delay time of t_OPR
-	 * after PD reset takes place.
-	 */
-	usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+	if (dw9768->data->pd_mode_ctrl) {
+		ret = i2c_smbus_write_byte_data(client,
+						DW9768_RING_PD_CONTROL_REG,
+						DW9768_PD_MODE_EN);
+		if (ret < 0)
+			return ret;
 
+		/*
+		 * DW9769 requires waiting delay time of t_OPR
+		 * after PD reset takes place.
+		 */
+		usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+	}
 	return 0;
 }
 
@@ -440,6 +449,8 @@ static int dw9768_probe(struct i2c_client *client)
 						      dw9768->clock_presc,
 						      dw9768->aac_timing);
 
+	dw9768->data = device_get_match_data(dev);
+
 	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
 		dw9768->supplies[i].supply = dw9768_supply_names[i];
 
@@ -525,9 +536,17 @@ static void dw9768_remove(struct i2c_client *client)
 	pm_runtime_disable(dev);
 }
 
+static const struct dw9768_vcm_data dw9768_data = {
+	.pd_mode_ctrl = true,
+};
+
+static const struct dw9768_vcm_data gt9769_data = {
+	.pd_mode_ctrl = false,
+};
+
 static const struct of_device_id dw9768_of_table[] = {
-	{ .compatible = "dongwoon,dw9768" },
-	{ .compatible = "giantec,gt9769" },
+	{ .compatible = "dongwoon,dw9768", .data = &dw9768_data },
+	{ .compatible = "giantec,gt9769", .data = &gt9769_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dw9768_of_table);
-- 
2.46.0

Re: [PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Sakari Ailus]

Hi Zhi,

Thanks for the patch.

On Sat, Aug 17, 2024 at 03:34:02PM +0800, Zhi Mao wrote:
> GT9769 VCM power-on default setting is PD=0,
> so it is not necessary to set again in dw9768_init function,
> and it also has no requirement of setting PD=1
> before power-off in dw9768_release function.
> For GT9769 VCM, PD mode control will add extra time
> when switching between suspend and resume.
> e.g. chrome camera AP can switch between video and photo mode,
> the behavior corresponding to VCM is suspend and resume,
> it will cause camera preview is not smooth.

If this is the problem, wouldn't either of these two be a better option:

- keep the file handle open in the user space, to avoid powering off the
  VCM or

- add autosuspend support to the driver.

I also wouldn't differentiate driver behaviour between the chips. If the
hardware default really is different (is it, this is rare for
register-compatible parts), then the driver needs to reprogram it (at least
on the one with a different default).

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Zhi Mao (毛智)]

Hi Sakari,

Thanks for your review.


On Wed, 2024-08-28 at 07:51 +0000, Sakari Ailus wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Zhi,
> 
> Thanks for the patch.
> 
> On Sat, Aug 17, 2024 at 03:34:02PM +0800, Zhi Mao wrote:
> > GT9769 VCM power-on default setting is PD=0,
> > so it is not necessary to set again in dw9768_init function,
> > and it also has no requirement of setting PD=1
> > before power-off in dw9768_release function.
> > For GT9769 VCM, PD mode control will add extra time
> > when switching between suspend and resume.
> > e.g. chrome camera AP can switch between video and photo mode,
> > the behavior corresponding to VCM is suspend and resume,
> > it will cause camera preview is not smooth.
> 
> If this is the problem, wouldn't either of these two be a better
> option:
> 
> - keep the file handle open in the user space, to avoid powering off
> the
>   VCM or
> 
> - add autosuspend support to the driver.
We use autosuspend fucntion can fix this issue.
please review: 
https://lore.kernel.org/all/20240831055328.22482-1-zhi.mao@mediatek.com/

> 
> I also wouldn't differentiate driver behaviour between the chips. If
> the
> hardware default really is different (is it, this is rare for
> register-compatible parts), then the driver needs to reprogram it (at
> least
> on the one with a different default).
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

[PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Zhi Mao]

Chromebook camera AP can switch between video and photo mode,
the behavior corresponding to VCM is suspend and resume,
it will cause camera preview is not smooth during switching operation.
We use autosuspend function can fix this issue.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/media/i2c/dw9768.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
index 18ef2b35c9aa..7449d29df901 100644
--- a/drivers/media/i2c/dw9768.c
+++ b/drivers/media/i2c/dw9768.c
@@ -374,7 +374,7 @@ static int dw9768_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 static int dw9768_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
-	pm_runtime_put(sd->dev);
+	pm_runtime_put_sync_autosuspend(sd->dev);
 
 	return 0;
 }
@@ -490,6 +490,7 @@ static int dw9768_probe(struct i2c_client *client)
 		goto err_power_off;
 	}
 
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_idle(dev);
 
 	return 0;
-- 
2.46.0

Re: [PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Zhi Mao (毛智)]

ping...

On Sat, 2024-08-31 at 13:52 +0800, Zhi Mao wrote:
> Chromebook camera AP can switch between video and photo mode,
> the behavior corresponding to VCM is suspend and resume,
> it will cause camera preview is not smooth during switching
> operation.
> We use autosuspend function can fix this issue.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/media/i2c/dw9768.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
> index 18ef2b35c9aa..7449d29df901 100644
> --- a/drivers/media/i2c/dw9768.c
> +++ b/drivers/media/i2c/dw9768.c
> @@ -374,7 +374,7 @@ static int dw9768_open(struct v4l2_subdev *sd,
> struct v4l2_subdev_fh *fh)
>  
>  static int dw9768_close(struct v4l2_subdev *sd, struct
> v4l2_subdev_fh *fh)
>  {
> -	pm_runtime_put(sd->dev);
> +	pm_runtime_put_sync_autosuspend(sd->dev);
>  
>  	return 0;
>  }
> @@ -490,6 +490,7 @@ static int dw9768_probe(struct i2c_client
> *client)
>  		goto err_power_off;
>  	}
>  
> +	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_idle(dev);
>  
>  	return 0;

Re: [PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Fei Shao]

Hi Zhi,

On Sat, Aug 31, 2024 at 1:54 PM Zhi Mao <zhi.mao@mediatek.com> wrote:

Suggestion for the new patch title:
  media: i2c: dw9768: Use runtime PM autosuspend

to better describe "what" this patch does to the DW9768 driver, not "why".
Also there's nothing to do with GT9769 in this patch.

>
> Chromebook camera AP can switch between video and photo mode,
> the behavior corresponding to VCM is suspend and resume,
> it will cause camera preview is not smooth during switching operation.
> We use autosuspend function can fix this issue.
>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/media/i2c/dw9768.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
> index 18ef2b35c9aa..7449d29df901 100644
> --- a/drivers/media/i2c/dw9768.c
> +++ b/drivers/media/i2c/dw9768.c
> @@ -374,7 +374,7 @@ static int dw9768_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>
>  static int dw9768_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  {
> -       pm_runtime_put(sd->dev);

Call pm_runtime_mark_last_busy() to update the power.last_busy field
for autosuspend to work properly.

> +       pm_runtime_put_sync_autosuspend(sd->dev);

Use pm_runtime_put_autosuspend().
Don't switch to the synchronized variant without reasons.

>
>         return 0;
>  }
> @@ -490,6 +490,7 @@ static int dw9768_probe(struct i2c_client *client)
>                 goto err_power_off;
>         }
>
> +       pm_runtime_use_autosuspend(dev);

Add pm_runtime_set_autosuspend_delay() before calling
pm_runtime_use_autosuspend() to avoid rapid power state bouncing.

Regards,
Fei



>         pm_runtime_idle(dev);
>
>         return 0;
> --
> 2.46.0
>
>

Re: [PATCH] media: i2c: improve suspend/resume switch performance for GT9769 VCM driver

[Zhi Mao (毛智)]

Hi Fei,

Thanks for your review.

On Mon, 2024-09-23 at 17:07 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Zhi,
> 
> On Sat, Aug 31, 2024 at 1:54 PM Zhi Mao <zhi.mao@mediatek.com> wrote:
> 
> Suggestion for the new patch title:
>   media: i2c: dw9768: Use runtime PM autosuspend
> 
> to better describe "what" this patch does to the DW9768 driver, not
> "why".
> Also there's nothing to do with GT9769 in this patch.
> 
> >
> > Chromebook camera AP can switch between video and photo mode,
> > the behavior corresponding to VCM is suspend and resume,
> > it will cause camera preview is not smooth during switching
> operation.
> > We use autosuspend function can fix this issue.
> >
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  drivers/media/i2c/dw9768.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/dw9768.c
> b/drivers/media/i2c/dw9768.c
> > index 18ef2b35c9aa..7449d29df901 100644
> > --- a/drivers/media/i2c/dw9768.c
> > +++ b/drivers/media/i2c/dw9768.c
> > @@ -374,7 +374,7 @@ static int dw9768_open(struct v4l2_subdev *sd,
> struct v4l2_subdev_fh *fh)
> >
> >  static int dw9768_close(struct v4l2_subdev *sd, struct
> v4l2_subdev_fh *fh)
> >  {
> > -       pm_runtime_put(sd->dev);
> 
> Call pm_runtime_mark_last_busy() to update the power.last_busy field
> for autosuspend to work properly.
> 
> > +       pm_runtime_put_sync_autosuspend(sd->dev);
> 
> Use pm_runtime_put_autosuspend().
> Don't switch to the synchronized variant without reasons.
> 
> >
> >         return 0;
> >  }
> > @@ -490,6 +490,7 @@ static int dw9768_probe(struct i2c_client
> *client)
> >                 goto err_power_off;
> >         }
> >
> > +       pm_runtime_use_autosuspend(dev);
> 
> Add pm_runtime_set_autosuspend_delay() before calling
> pm_runtime_use_autosuspend() to avoid rapid power state bouncing.
> 

I have fixed these suggestion above.
please review patch:

https://lore.kernel.org/all/20241012032805.23545-1-zhi.mao@mediatek.com/


> Regards,
> Fei
> 
> 
> 
> >         pm_runtime_idle(dev);
> >
> >         return 0;
> > --
> > 2.46.0
> >
> >