[PATCH 1/2] media: i2c: adv7482: Fix wait procedure usleep_range from msleep

[PATCH 1/2] media: i2c: adv7482: Fix wait procedure usleep_range from msleep

[Kieran Bingham]

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);
 		} else {
 			ret = adv748x_write(state, regs->page, regs->reg,
 				      regs->value);
-- 
2.19.2

Re: [PATCH 1/2] media: i2c: adv7482: Fix wait procedure usleep_range from msleep

[Kieran Bingham]

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