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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aKLZ39IzI_azrDIu@kekkonen.localdomain>
Date: Mon, 18 Aug 2025 07:44:31 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: git@...tzsch.eu
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	Daniel Scally <djrscally@...il.com>,
	~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Val Packett <val@...kett.cool>
Subject: Re: [PATCH 7/7] media: i2c: dw9719: Fix power on/off sequence

Hi André,

On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4 Relay wrote:
> From: Val Packett <val@...kett.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@...kett.cool>
> Signed-off-by: André Apitzsch <git@...tzsch.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