On Sat, Sep 15, 2012 at 11:29:58PM -0400, Parag Warudkar wrote:
>
>
> On Sat, 15 Sep 2012, Parag Warudkar wrote:
>
> >
> >
> > On Sat, 15 Sep 2012, Guenter Roeck wrote:
> > >
> > > Also, since the delay time can get quite large, would it make sense to
> > > replace
> > > udelay with usleep_range() ?
> > >
> >
> > Yes, I think that would be a good thing to do. We could sleep in range of
> > us<<=1 and us<<1 and if usleep_range() returns actual sleep time we can
> > factor that in for next loop iteration if necessary. Gotta think a bit on
> > that one.
> >
> > I will rework the patch to fix the loop termination and keep the bump to
> > 0x10000 in place and possibly also experiment with usleep_range().
> >
>
> Below is what I am experimenting with right now. I chose to keep the
> APPLESMC_MAX_WAIT to current 0x8000 (32ms). With the fixed loop
> termination and use of usleep_range() instead of udelay - I will try to
> run this a couple days and see if I can recreate the failure within 32ms.
>
> Does this look ok? (I haven't yet changed send_byte to use usleep_range -
> if this approach looks ok I can change that as well.)
>
> Signed-off-by: Parag Warudkar <parag.l...@gmail.com>
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 2827088..6610037 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -168,15 +168,20 @@ static struct workqueue_struct *applesmc_led_wq;
> static int wait_read(void)
> {
> u8 status;
> - int us;
> - for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> - udelay(us);
> + unsigned long r1_us = APPLESMC_MIN_WAIT;
> + unsigned long r2_us;
> + do {
> status = inb(APPLESMC_CMD_PORT);
> /* read: wait for smc to settle */
> if (status & 0x01)
> return 0;
> - }
> -
> + r2_us = r1_us << 2;
> + if (r2_us > APPLESMC_MAX_WAIT)
> + goto fail;
> + usleep_range(r1_us, r2_us);
> + r1_us = r2_us;
That looks terribly complicated. Better keep the loop, and just replace
udelay(us);
with something like
usleep_range(us, us << 1);
Alternatively, just use a constant such as
usleep_range(us, us + APPLESMC_MIN_WAIT);
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
