[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