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

Attachment: signature.asc
Description: Digital signature

-- 
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios

Reply via email to