Re: [coreboot] [PATCH] flashrom: Add external programmer delay functions

[Carl-Daniel Hailfinger]

On 05.06.2009 19:23, Uwe Hermann wrote:
> On Fri, Jun 05, 2009 at 06:28:41PM +0200, Carl-Daniel Hailfinger wrote:
> > Add external programmer delay functions so external programmers can
> > handle the delay on their own if needed.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Uwe Hermann
>
> ...but see below.

Thanks for the review!
> > Index: flashrom-programmer_delay/flash.h
> >
> > --- flashrom-programmer_delay/flash.h	(Revision 576)
> > +++ flashrom-programmer_delay/flash.h	(Arbeitskopie)
> > @@ -118,6 +120,7 @@
> >  uint8_t chip_readb(const chipaddr addr);
> >  uint16_t chip_readw(const chipaddr addr);
> >  uint32_t chip_readl(const chipaddr addr);
> > +void programmer_delay(uint32_t usecs);
>
> Why uint32_t here and int above?

Leftover from an earlier version. Should be int.
> > Index: flashrom-programmer_delay/spi.c
> >
> > --- flashrom-programmer_delay/spi.c	(Revision 576)
> > +++ flashrom-programmer_delay/spi.c	(Arbeitskopie)
> > @@ -425,7 +425,7 @@
> >     */
> >    /* FIXME: We assume spi_read_status_register will never fail. */
> >    while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> > sleep(1);
> > programmer_delay(1000000);
>
> I'd also make this 100 * 1000 as below, easier on the eyes.

1000 * 1000, sure.

Changed for all other calls as well.
> > Index: flashrom-programmer_delay/internal.c
> >
> > --- flashrom-programmer_delay/internal.c	(Revision 576)
> > +++ flashrom-programmer_delay/internal.c	(Arbeitskopie)
> > @@ -195,6 +195,18 @@
> >    return *(volatile uint32_t *) addr;
> >  }
> >
> > +void internal_delay(int usecs)
> > +{
> > /* If the delay is >1 s, use usleep because timing does not need to
> > * be so precise.
>
> Hm, but why not use myusec_delay() always? Does it have drawbacks?

Yes. myusec_delay is a busy-wait to get some precision. Hogging the CPU
for more than 1 second is bad style.

Committed with your suggestions in r578.

Regards,
Carl-Daniel