| Message ID | 1357909986-9262-5-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2013-01-11 at 13:12 +0000, Lee Jones wrote: > Doing so provides a greater degree of accuracy when dealing with > time-frames between 1us and 20ms. msleep() is only accurate for > wake-ups greater than 20ms. [] > diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c [] > @@ -956,7 +956,7 @@ static int ab8500_fg_load_comp_volt_to_capacity(struct ab8500_fg *di) > do { > vbat += ab8500_fg_bat_voltage(di); > i++; > - msleep(5); > + usleep_range(5000, 5001); If you're going to give a range that small you might as well use usleep instead. Otherwise, add some tolerance to allow any other coalesced wakeup to occur.
On Mon, 14 Jan 2013, Joe Perches wrote: > On Fri, 2013-01-11 at 13:12 +0000, Lee Jones wrote: > > Doing so provides a greater degree of accuracy when dealing with > > time-frames between 1us and 20ms. msleep() is only accurate for > > wake-ups greater than 20ms. > [] > > diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c > [] > > @@ -956,7 +956,7 @@ static int ab8500_fg_load_comp_volt_to_capacity(struct ab8500_fg *di) > > do { > > vbat += ab8500_fg_bat_voltage(di); > > i++; > > - msleep(5); > > + usleep_range(5000, 5001); > > If you're going to give a range that small > you might as well use usleep instead. > > Otherwise, add some tolerance to allow any > other coalesced wakeup to occur. I can't increase the tolerance, as I don't know how that would effect the running of the system, and the person who would know is off on parental leave. What I can tell you is we're only using usleep_range() because there is no usleep in the kernel. At least that's what we've been led to believe: Documentation/timers/timers-howto.txt: - Why is there no "usleep" / What is a good range? Since usleep_range is built on top of hrtimers, the wakeup will be very precise (ish), thus a simple usleep function would likely introduce a large number of undesired interrupts.
On Tue, Jan 15, 2013 at 9:48 AM, Lee Jones <lee.jones@linaro.org> wrote: > On Mon, 14 Jan 2013, Joe Perches wrote: >> > - msleep(5); >> > + usleep_range(5000, 5001); >> >> If you're going to give a range that small >> you might as well use usleep instead. >> >> Otherwise, add some tolerance to allow any >> other coalesced wakeup to occur. > > I can't increase the tolerance, as I don't know how that would > effect the running of the system, and the person who would know > is off on parental leave. > > What I can tell you is we're only using usleep_range() because > there is no usleep in the kernel. At least that's what we've > been led to believe: > > Documentation/timers/timers-howto.txt: > > - Why is there no "usleep" / What is a good range? > Since usleep_range is built on top of hrtimers, the > wakeup will be very precise (ish), thus a simple > usleep function would likely introduce a large number > of undesired interrupts. And I think the above is why we have this in the kernel: arch/arm/mach-davinci/board-da850-evm.c: usleep_range(1000, 1000); arch/arm/mach-tegra/pcie.c: usleep_range(1000, 1000); drivers/clk/clk-wm831x.c: usleep_range(2000, 2000); drivers/media/i2c/m5mols/m5mols_core.c: usleep_range(200, 200); drivers/media/i2c/s5k6aa.c: usleep_range(4000, 4000); drivers/media/i2c/smiapp/smiapp-core.c: usleep_range(1000, 1000); drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c: usleep_range(1000, 1000); There are quite a few of these. Let's ping John Stultz for some clarification ... Yours, Linus Walleij
On Tuesday 15 January 2013, Lee Jones wrote: > > > @@ -956,7 +956,7 @@ static int ab8500_fg_load_comp_volt_to_capacity(struct ab8500_fg *di) > > > do { > > > vbat += ab8500_fg_bat_voltage(di); > > > i++; > > > - msleep(5); > > > + usleep_range(5000, 5001); > > > > If you're going to give a range that small > > you might as well use usleep instead. > > > > Otherwise, add some tolerance to allow any > > other coalesced wakeup to occur. > > I can't increase the tolerance, as I don't know how that would > effect the running of the system, and the person who would know > is off on parental leave. The function only averages the voltage between a couple of readings. It won't change much if those register reads are slightly more uniformly timed. Note that the thread can still be preempted for a much longer time if anything else is running, and the entire interrupt handling in this driver looks so fragile that I would not rely on the interrupt actually happening at the right time anyway. I think it should first be debugged properly to remove the need for the enable_irq/disable_irq calls. Arnd
> > > > @@ -956,7 +956,7 @@ static int ab8500_fg_load_comp_volt_to_capacity(struct ab8500_fg *di) > > > > do { > > > > vbat += ab8500_fg_bat_voltage(di); > > > > i++; > > > > - msleep(5); > > > > + usleep_range(5000, 5001); > > > > > > If you're going to give a range that small > > > you might as well use usleep instead. > > > > > > Otherwise, add some tolerance to allow any > > > other coalesced wakeup to occur. > > > > I can't increase the tolerance, as I don't know how that would > > effect the running of the system, and the person who would know > > is off on parental leave. > > The function only averages the voltage between a couple of readings. > It won't change much if those register reads are slightly more > uniformly timed. Note that the thread can still be preempted for > a much longer time if anything else is running, and Okay, I'll fixup to have a more sensible range. > the entire > interrupt handling in this driver looks so fragile that I would > not rely on the interrupt actually happening at the right time > anyway. I think it should first be debugged properly to remove > the need for the enable_irq/disable_irq calls. Yes, I remember discussing this with you before and I've since placed it on my TODO list. However, I'm really shying away from it for the moment, as this patch-set only applies <20 out of the 70+ outstanding patches left in the internal kernel's delta. To avoid any unnecessary fixups, I'll apply those kinds of fixes at the end of the set.
diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c index 828529e..ca3ba88c 100644 --- a/drivers/power/ab8500_fg.c +++ b/drivers/power/ab8500_fg.c @@ -956,7 +956,7 @@ static int ab8500_fg_load_comp_volt_to_capacity(struct ab8500_fg *di) do { vbat += ab8500_fg_bat_voltage(di); i++; - msleep(5); + usleep_range(5000, 5001); } while (!ab8500_fg_inst_curr_done(di)); ab8500_fg_inst_curr_finalize(di, &di->inst_curr);