[LinuxBIOS] [PATCH] flashrom: add MX25L4005 erase/write support
Uwe Hermann
uwe at hermann-uwe.de
Wed Oct 17 19:21:15 CEST 2007
On Wed, Oct 17, 2007 at 02:14:17PM +0200, Carl-Daniel Hailfinger wrote:
> > distclean: clean
> > rm -f $(PROGRAM) .dependencies
> >
>
> Skip the make clean changes. We have make distclean.
Yep.
> > diff -ubrN ../../flashrom.original/mx25l4005.c flashrom.new/mx25l4005.c
> > --- ../../flashrom.original/mx25l4005.c 1970-01-01 01:00:00.000000000 +0100
> > +++ flashrom.new/mx25l4005.c 2007-10-15 01:17:56.000000000 +0200
> > @@ -0,0 +1,57 @@
No license header?
> > +int write_25l4005(struct flashchip *flash, uint8_t *buf) {
> > + int total_size=1024*flash->total_size;
> > + int i;
> > + for (i=0;i<total_size/256;++i) {
> > + write256b(i, buf, (uint8_t*)flash->virtual_memory);
> > + }
> > +return 0;
> > +}
> >
>
> OK
Yes, but the coding style if broken (in several places). Please fix that.
> My version of the write patch follows. Please test.
Missing Signed-off-by. May be per design, but please always add it, even
if the patch is not (yet) meant to be committed. In such a case just
mention that it's WIP and shouldn't be commited (but always add the
Signed-off-by).
> Index: flash.h
> ===================================================================
> --- flash.h (Revision 2864)
> +++ flash.h (Arbeitskopie)
> @@ -296,4 +296,10 @@
> /* w49f002u.c */
> int write_49f002(struct flashchip *flash, uint8_t *buf);
>
> +/* mx25l4005.c */
> +// probe
What's the "// probe" for?
> +void generic_spi_write_enable()
> +{
> + const unsigned char cmd[] = JEDEC_WREN;
Why does JEDEC_WREN contain the braces and not do this?
const unsigned char cmd[] = { JEDEC_WREN };
Is there a technical reason for that? The version without braces in the
#define is preferrable IMO.
> + unsigned char result[] = {0, 0, 0};
uint8_t ?
> Index: spi.h
> ===================================================================
> --- spi.h (Revision 0)
> +++ spi.h (Revision 0)
> @@ -0,0 +1,9 @@
Missing license header.
Do we need this file at all? I'd rather merge it into an existing header
file, especially since it's almost empty anyway.
> +#ifndef __SPI_H__
> +#define __SPI_H__ 1
> +
> +uint16_t it8716f_flashport;
Missing "extern"?
> +int generic_spi_command(unsigned char writecnt, unsigned char readcnt, const unsigned char *writearr, unsigned char *readarr);
> +void generic_spi_write_enable();
> +void generic_spi_write_disable();
> +
> +#endif
> Index: mx25l4005.c
> ===================================================================
> --- mx25l4005.c (Revision 0)
> +++ mx25l4005.c (Revision 0)
Missing license header?
Uwe.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071017/e282bdb9/attachment.sig>
More information about the coreboot
mailing list