On Wed, 08 Feb 2012 01:17:04 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2...@gmx.net> wrote:
> Am 07.02.2012 23:23 schrieb Stefan Tauner:
> > On Wed, 01 Feb 2012 00:03:34 +0100
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2...@gmx.net> wrote:
> >
> >> Am 31.01.2012 06:59 schrieb Stefan Tauner:
> >>> […]
> >>> todo:
> >>> - handle programmers which have a problem with the dummy bytes needed
> >> AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
> >> write and read, IIRC it is called DropOneClkOnRead or somthing like
> >> that. Quite a few other SPI masters have the one-dummy-byte
> >> functionality as well. This needs to be implemented in a generic way (I
> >> have a totally bitrotted patch for it), but it should not hold back this
> >> patch.
> > what about simulating the dummy byte by reading one additional byte in
> > the beginning instead of writing one? due to SPI's underlying principle
> > of shifting bits in and out of master and slave simultaneously this
> > should give us the same effect, but eases working around programmer
> > limitations.
>
> Right. The direction is a don't-care thing for dummy bytes. Maybe just
> add a FIXME comment that we should handle dummy bytes explicitly later.
/* FIXME: the following dummy byte explodes on some programmers.
* One possible workaround would be to read the dummy byte
* instead and discard its value.
*/
will post the whole reworked patch later, so it is probably better if
you would comment to that then.
> >>> […]
> >>> +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr,
> >>> unsigned int blocklen);
> >> I believe that typedef to be pretty unreadable, but I see that avoiding
> >> the typedef would probably be even worse.
> > yes :) see http://patchwork.coreboot.org/patch/3492/ ... *shiver*
>
> Now that you point me to the alternative, I have to say that I think the
> typedef is less readable (at least the typedef definition itself).
a few lines above that line is the definition of struct flash(ctx)
which defines the different function pointers almost identically...
this stuff is just not made to be read i think :) if you compare the
two versions of get_erasefn_from_opcode you will notice the
obvious benefit:
int (*get_erasefn_from_opcode(uint8_t opcode)) (struct flashctx *flash,
unsigned int blockaddr, unsigned int blocklen)
vs.
erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode)
yes, this is really equivalent and the first one is not intentionally
obfuscated. :)
> >>> […]
> >>> + .read = spi_chip_read,
> >>> + .page_size = 256, /* ? */
> >> Argh, page_size comes to bite us again. Did I already send my "kill most
> >> uses of page_size" patch?
> > afaics no
>
> Yes, it's still in beta. But I can send it anyway so people can review
> it mercilessly.
cant hurt to post it... we are not that ruthless usually ;)
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
