| Message ID | 20190111154345.29145-1-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [1/2] media: i2c: adv7482: Fix wait procedure usleep_range from msleep | expand |
Hi Matsuoka-san, Thank you for the patch, On 11/01/2019 15:43, Kieran Bingham wrote:> > From: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > > By Documentation/timers/timers-howto.txt, when waiting 20ms from 10us, > it is correct to use usleep_range. this patch corrects it. > > Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com> > (cherry picked from horms/renesas-bsp commit af0cdba377bc8a784cdae6a77fb7a822cebc7083) > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index 64eb1bfda581..097e5c3a8e7e 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -273,7 +273,8 @@ static int adv748x_write_regs(struct adv748x_state *state, > > while (regs->page != ADV748X_PAGE_EOR) { > if (regs->page == ADV748X_PAGE_WAIT) { > - msleep(regs->value); > + usleep_range(regs->value * 1000, > + (regs->value * 1000) + 1000); I might propose simplifying this by moving the duplicated multiplication to a local variable: if (regs->page == ADV748X_PAGE_WAIT) { unsigned int usec = regs->value * 1000; usleep_range(usec, usec + 1000); } ... There are three other usages of usleep_range in the driver which utilise a 500 uSec range rather than 1000. However I don't see any real reason to object to a range of 1000 and it will help the scheduling to be a bit more relaxed. In fact, It seems there is only a single usage of ADV748X_PAGE_WAIT ... to perform a sw-reset, and that itself only has 3 register writes. As part of the ongoing aim to remove the register tables from this code - I think I will drop this patch and submit an alternative which implements a function to handle the sw_reset, and removes all references to ADV748X_PAGE_WAIT. The PAGE_WAIT feels like a bit of an ugly hack due to the tables and I think we can let it go. > } else { > ret = adv748x_write(state, regs->page, regs->reg, > regs->value); > Regards Kieran
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c index 64eb1bfda581..097e5c3a8e7e 100644 --- a/drivers/media/i2c/adv748x/adv748x-core.c +++ b/drivers/media/i2c/adv748x/adv748x-core.c @@ -273,7 +273,8 @@ static int adv748x_write_regs(struct adv748x_state *state, while (regs->page != ADV748X_PAGE_EOR) { if (regs->page == ADV748X_PAGE_WAIT) { - msleep(regs->value); + usleep_range(regs->value * 1000, + (regs->value * 1000) + 1000); } else { ret = adv748x_write(state, regs->page, regs->reg, regs->value);