Hi Mauro,
On Tue, Jan 12, 2021 at 05:49:48PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 7 Dec 2020 23:15:23 +0200
> Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu:
>
> > Verify the software reset has been completed until proceeding.
> >
> > The spec does not guarantee a delay but presumably 100 ms should be
> > enough.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ccs/ccs-core.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> > index fdf2e83eeac3..e1b3c5693e01 100644
> > --- a/drivers/media/i2c/ccs/ccs-core.c
> > +++ b/drivers/media/i2c/ccs/ccs-core.c
> > @@ -1553,11 +1553,26 @@ static int ccs_power_on(struct device *dev)
> > */
> >
> > if (!sensor->reset && !sensor->xshutdown) {
> > + u8 retry = 100;
> > + u32 reset;
> > +
> > rval = ccs_write(sensor, SOFTWARE_RESET, CCS_SOFTWARE_RESET_ON);
> > if (rval < 0) {
> > dev_err(dev, "software reset failed\n");
> > goto out_cci_addr_fail;
> > }
> > +
> > + do {
> > + rval = ccs_read(sensor, SOFTWARE_RESET, &reset);
> > + reset = !rval && reset == CCS_SOFTWARE_RESET_OFF;
> > + if (reset)
> > + break;
> > +
> > + usleep_range(1000, 2000);
> > + } while (--retry);
>
> Hmm... the way it is, the loop will wait for some time between
> 100ms and 200ms. Based on past experiences with non-deterministic
> sleep times, this can be hard to debug if some device would require
> to wait for a value between 100ms and 200ms.
>
> So, I would, instead, make the retry time more deterministic, by
> using time_is_after_jiffies(), e. g.:
>
> end = jiffies + msecs_to_jiffies(retry);
> do {
> rval = ccs_read(sensor, SOFTWARE_RESET, &reset);
> reset = !rval && reset == CCS_SOFTWARE_RESET_OFF;
> if (reset)
> break;
>
> usleep_range(1000, 2000);
> } while (time_is_after_jiffies(end));
>
> In any case, I'm taking this patch, but it seems worth to change
> to something like the above on a followup patch.
Thanks for the suggestion. I agree.
I'll address this soon.
--
Sakari Ailus
