[v1] media: i2c: dw9719: add DT compatible and DW9718S support

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

Posted by André Apitzsch via B4 Relay 5 months, 3 weeks ago

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;
+
+	/*
+	 * 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);
 	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;
 	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.
+	 */
+	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);
-	/* Need 100us to transit from SHUTDOWN to STANDBY */
-	fsleep(100);
 
 	if (detect) {
 		/* This model does not have an INFO register */

-- 
2.50.1

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

Posted by Sakari Ailus 5 months, 3 weeks ago

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

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

Posted by André Apitzsch 4 months, 3 weeks ago

Hi Sakari,

@Val, please see below.

Am Montag, dem 18.08.2025 um 07:44 +0000 schrieb Sakari Ailus:
> 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..dd28a0223d6ac980084b1f661
> > bd029ea6b0be503 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?
> 
I haven't noticed any problems during power-up of DW9761. However,
according to the commit message, there seems be an issue with DW9718S.
But I don't own the device and cannot test it.

Maybe Val can provided some additional information.

Best regards,
André

> > +	 */
> > +	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 */
> >

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

Posted by Val Packett 4 months, 3 weeks ago

On 9/15/25 5:48 PM, André Apitzsch wrote:
> Hi Sakari,
>
> @Val, please see below.
>
> Am Montag, dem 18.08.2025 um 07:44 +0000 schrieb Sakari Ailus:
>> Hi André,
>>
>> On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4 Relay
>> wrote:
>>>   	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?
>>
> I haven't noticed any problems during power-up of DW9761. However,
> according to the commit message, there seems be an issue with DW9718S.
> But I don't own the device and cannot test it.
>
> Maybe Val can provided some additional information.

I haven't had access to the datasheet for the DW9718S, so this was all 
deduced experimentally. By "to be sure" I meant that I literally raised 
the timeout "just in case", not based on actual issues.

The actually important change was expecting the failure on the write and 
not erroring out.

Thanks,
~val

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

Posted by Sakari Ailus 4 months, 3 weeks ago

Hi André, Val,

On Tue, Sep 16, 2025 at 05:08:44PM -0300, Val Packett wrote:
> 
> On 9/15/25 5:48 PM, André Apitzsch wrote:
> > Hi Sakari,
> > 
> > @Val, please see below.
> > 
> > Am Montag, dem 18.08.2025 um 07:44 +0000 schrieb Sakari Ailus:
> > > Hi André,
> > > 
> > > On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4 Relay
> > > wrote:
> > > >   	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?
> > > 
> > I haven't noticed any problems during power-up of DW9761. However,
> > according to the commit message, there seems be an issue with DW9718S.
> > But I don't own the device and cannot test it.
> > 
> > Maybe Val can provided some additional information.
> 
> I haven't had access to the datasheet for the DW9718S, so this was all
> deduced experimentally. By "to be sure" I meant that I literally raised the
> timeout "just in case", not based on actual issues.
> 
> The actually important change was expecting the failure on the write and not
> erroring out.

Ack.

André: could you add this information to the comment as well?

-- 
Kind regards,

Sakari Ailus

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

Posted by André Apitzsch 4 months, 3 weeks ago

Hi Sakari,

Am Mittwoch, dem 17.09.2025 um 13:10 +0300 schrieb Sakari Ailus:
> Hi André, Val,
> 
> On Tue, Sep 16, 2025 at 05:08:44PM -0300, Val Packett wrote:
> > 
> > On 9/15/25 5:48 PM, André Apitzsch wrote:
> > > Hi Sakari,
> > > 
> > > @Val, please see below.
> > > 
> > > Am Montag, dem 18.08.2025 um 07:44 +0000 schrieb Sakari Ailus:
> > > > Hi André,
> > > > 
> > > > On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4
> > > > Relay wrote:
> > > > >   	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?
> > > > 
> > > I haven't noticed any problems during power-up of DW9761.
> > > However, according to the commit message, there seems be an issue
> > > with DW9718S. But I don't own the device and cannot test it.
> > > 
> > > Maybe Val can provided some additional information.
> > 
> > I haven't had access to the datasheet for the DW9718S, so this was
> > all deduced experimentally. By "to be sure" I meant that I
> > literally raised the timeout "just in case", not based on actual
> > issues.
> > 
> > The actually important change was expecting the failure on the
> > write and not erroring out.
> 
> Ack.
> 
> André: could you add this information to the comment as well?

Could you clarify which information.

Best regards,
André

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

Posted by Sakari Ailus 4 months, 3 weeks ago

On Thu, Sep 18, 2025 at 11:31:18PM +0200, André Apitzsch wrote:
> Hi Sakari,
> 
> Am Mittwoch, dem 17.09.2025 um 13:10 +0300 schrieb Sakari Ailus:
> > Hi André, Val,
> > 
> > On Tue, Sep 16, 2025 at 05:08:44PM -0300, Val Packett wrote:
> > > 
> > > On 9/15/25 5:48 PM, André Apitzsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > @Val, please see below.
> > > > 
> > > > Am Montag, dem 18.08.2025 um 07:44 +0000 schrieb Sakari Ailus:
> > > > > Hi André,
> > > > > 
> > > > > On Sun, Aug 17, 2025 at 07:09:26PM +0200, André Apitzsch via B4
> > > > > Relay wrote:
> > > > > >   	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?
> > > > > 
> > > > I haven't noticed any problems during power-up of DW9761.
> > > > However, according to the commit message, there seems be an issue
> > > > with DW9718S. But I don't own the device and cannot test it.
> > > > 
> > > > Maybe Val can provided some additional information.
> > > 
> > > I haven't had access to the datasheet for the DW9718S, so this was
> > > all deduced experimentally. By "to be sure" I meant that I
> > > literally raised the timeout "just in case", not based on actual
> > > issues.
> > > 
> > > The actually important change was expecting the failure on the
> > > write and not erroring out.
> > 
> > Ack.
> > 
> > André: could you add this information to the comment as well?
> 
> Could you clarify which information.

What you now have in the commit message. It's relevant for the driver.

-- 
Sakari Ailus