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