Re: [PATCH 7/7] media: i2c: dw9719: Fix power on/off sequence

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi André,

On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4 Relay wrote:
> From: Val Packett <val@packett.cool>
> 
> The "jiggle" code was not actually expecting failure, which it should
> because that's what actually happens when the device wasn't already woken
> up by the regulator power-on (i.e. in the case of a shared regulator).
> 
> Also, do actually enter the internal suspend mode on shutdown, to save
> power in the case of a shared regulator.
> 
> Also, wait a bit longer (2x tOPR) on waking up, 1x is not enough at least
> on the DW9718S as found on the motorola-nora smartphone.
> 
> Signed-off-by: Val Packett <val@packett.cool>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/media/i2c/dw9719.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> index 63c7fd4ab70a0e02518252b23b89c45df4ba273d..dd28a0223d6ac980084b1f661bd029ea6b0be503 100644
> --- a/drivers/media/i2c/dw9719.c
> +++ b/drivers/media/i2c/dw9719.c
> @@ -95,12 +95,19 @@ struct dw9719_device {
>  
>  static int dw9719_power_down(struct dw9719_device *dw9719)
>  {
> +	u32 reg_pwr = (dw9719->model == DW9718S) ? DW9718S_PD : DW9719_CONTROL;

Extra parentheses.

> +
> +	/*
> +	 * Worth engaging the internal SHUTDOWN mode especially due to the
> +	 * regulator being potentially shared with other devices.
> +	 */
> +	cci_write(dw9719->regmap, reg_pwr, DW9719_SHUTDOWN, NULL);

I'd still complain if this fails as we don't return the error.

>  	return regulator_disable(dw9719->regulator);
>  }
>  
>  static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  {
> -	u32 reg_pwr;
> +	u32 reg_pwr = (dw9719->model == DW9718S) ? DW9718S_PD : DW9719_CONTROL;

Extra parentheses.

>  	u64 val;
>  	int ret;
>  	int err;
> @@ -109,13 +116,15 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  	if (ret)
>  		return ret;
>  
> -	/* Jiggle SCL pin to wake up device */
> -	reg_pwr = (dw9719->model == DW9718S) ? DW9718S_PD : DW9719_CONTROL;
> -	cci_write(dw9719->regmap, reg_pwr, DW9719_SHUTDOWN, &ret);
> -	fsleep(100);
> +	/*
> +	 * Need 100us to transition from SHUTDOWN to STANDBY.
> +	 * Jiggle the SCL pin to wake up the device (even when the regulator
> +	 * is shared) and wait double the time to be sure, then retry the write.

Why double? Isn't the datasheet correct when it comes to the power-on
sequence?

> +	 */
> +	cci_write(dw9719->regmap, reg_pwr, DW9719_STANDBY, &ret);
> +	ret = 0; /* the jiggle is expected to fail, don't even log that as error */
> +	fsleep(200);
>  	cci_write(dw9719->regmap, reg_pwr, DW9719_STANDBY, &ret);

Just pass NULL instead of ret as we don't check the value and the ret
assignment above becomes redundant. Please spare the comment though.

> -	/* Need 100us to transit from SHUTDOWN to STANDBY */
> -	fsleep(100);
>  
>  	if (detect) {
>  		/* This model does not have an INFO register */
> 

-- 
Regards,

Sakari Ailus