Re: [flashrom] [PATCH] Bitbanging SPI driver

[Uwe Hermann]

On Thu, Sep 24, 2009 at 01:11:46AM +0200, Carl-Daniel Hailfinger wrote:
> This is the bitbanging SPI driver infrastructure.
>
> If you want support for a particular piece of hardware, just fill in
> a few functions in spi_bitbang_master_table. That's it.
> On top of this, the RayeR SPI flasher should be supportable in ~20 LOC.
>
> Tested, trace looks OK.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Uwe Hermann

But please see below.
> Index: flashrom-bitbang_spi/flash.h
>
> --- flashrom-bitbang_spi/flash.h	(Revision 731)
> +++ flashrom-bitbang_spi/flash.h	(Arbeitskopie)
> @@ -145,6 +145,21 @@
>  void chip_readn(uint8_t *buf, const chipaddr addr, size_t len);
>  void programmer_delay(int usecs);
>
> +enum spi_bitbang_master {
> SPI_BITBANG_INVALID /* This must always be the last entry. */
>
> +};
>
> +extern const int spi_bitbang_master_count;
>
> +extern enum spi_bitbang_master spi_bitbang_master;
>
> +struct spi_bitbang_master_entry {
> void (*set_cs) (int val);
> void (*set_sck) (int val);
> void (*set_mosi) (int val);
> int (*get_miso) (void);
>
> +};

You mix bitbang_spi_* and spi_bitbang_* naming in the code, please fix
to use only one of them (bitbang_spi_ IMHO).
Can be an extra patch, no need to fix in this one.
> +/* bitbang_spi.c */
> +extern int bitbang_half_period;

Also, I'd make all functions and global variables prefixed with
"bitbang_spi_" (not just "bitbang_" like this one) for consistency.
"bitbang_" alone is also a bit too generic, thus "bitbang_spi_".
> +extern const struct spi_bitbang_master_entry spi_bitbang_master_table[];
> +int bitbang_spi_init(void);
> +int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);
> +int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
> +int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf);
> +void bitbang_set_cs(int val)

bitbang_spi_set_cs
> +void bitbang_set_sck(int val)

bitbang_spi_set_sck
> +void bitbang_set_mosi(int val)

bitbang_spi_set_mosi
> +int bitbang_get_miso(void)

bitbang_spi_set_miso
> if ((r = spi_nbyte_program(i, &buf[i], l))) {
> 	fprintf(stderr, "%s: write fail %d\n", __FUNCTION__, r);

__func__ for consistency.

Uwe.