Re: [RFC PATCH] spi: add at25 spi eeprom driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jun 20, 2011 at 09:23:40AM +0200, Hubert Feurstein wrote:
> Hi Sascha,
> 
> 2011/6/20 Sascha Hauer <s.hauer@pengutronix.de>:
> > Hi Hubert,
> >
> > On Thu, Jun 16, 2011 at 10:12:43AM +0200, Hubert Feurstein wrote:
> >>  obj-y        += mfd/
> >>  obj-$(CONFIG_LED) += led/
> >> +obj-y        += misc/
> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> >
> > I think we should move the driver up to drivers/eeprom and skip the
> > 'misc'. The kernel guys like to get rid of it also.
> >
> OK
> 
> >> +
> >> +             /* Wait for non-busy status */
> >> +             start_time = get_time_ns();
> >> +
> >> +             retries = 0;
> >> +             do {
> >> +
> >> +                     sr = spi_w8r8(at25->spi, AT25_RDSR);
> >> +                     if (sr < 0 || (sr & AT25_SR_nRDY)) {
> >> +                             dev_dbg(at25->cdev.dev,
> >> +                                     "rdsr --> %d (%02x)\n", sr, sr);
> >> +                             mdelay(1);

We can remove this mdelay. In the kernel it makes sense to use msleep to
give the cpu a chance to sleep. In barebox we are polling anyway, so we
can better poll the status register instead of the timer register. This
gives us the chance to bail out here after 1.1ms instead of 1, 2, ...

> >> +                             continue;
> >> +                     }
> >> +                     if (!(sr & AT25_SR_nRDY))
> >> +                             break;
> >> +             } while (retries++ < 3 || !is_timeout(start_time, EE_TIMEOUT));
> >
> > I don't understand this. The loop is limited by retries++ < 3. Why this
> > additional is_timeout? Is this the same in the kernel?
> >
> Hmm, I don't know why we have both here. I simply ported the kernel code.

Looking at it again it seems like 'poll for EE_TIMEOUT ms but at least
three times'. I think we can skip the retries and just poll for
EE_TIMEOUT ms. The most important thing about timeouts is that we bail
out after reasonable time to give the user a chance to continue without
this driver and to give him a clue where it hangs. The exact amount
of time we poll is not really important.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox