| Message ID | 20201207211530.21180-18-sakari.ailus@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 51fc72e541b44d5ba52673b1e2b492ffc164b71c |
| Headers | show |
| Series | [01/24] ccs: Add digital gain support | expand |
Em Mon, 7 Dec 2020 23:15:23 +0200 Sakari Ailus <sakari.ailus@linux.intel.com> 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@linux.intel.com> > --- > 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. > + > + if (!reset) > + return -EIO; > } > > if (sensor->hwcfg.i2c_addr_alt) { Thanks, Mauro
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@linux.intel.com> 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@linux.intel.com> > > --- > > 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
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); + + if (!reset) + return -EIO; } if (sensor->hwcfg.i2c_addr_alt) {
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@linux.intel.com> --- drivers/media/i2c/ccs/ccs-core.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)