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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: André Apitzsch <[email protected]> > > --- > > 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 */ > >
