[flashrom] [PATCH] split spi.c and fix include headers
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Feb 25 11:43:39 CET 2010
On 25.02.2010 08:55, Sean Nelson wrote:
> Split spi.c into programmer and chip drivers.
> Make it so flash.h doesn't include any other flashrom-related headers.
> Fix includes in files.
>
> Signed-off-by: Sean Nelson <audiohacked at gmail.com>
>
> ---
>
> Made as minimal changes as I could, and split sli.c where it was
> clearly programmer code where the rest was moved to spi25.c. Names and
> Ideas as suggested by carldani.
Thanks for preparing this patch. It brings us a step closer to a good
separation and abstraction.
> --- a/flash.h
> +++ b/flash.h
> @@ -17,32 +17,34 @@
> * GNU General Public License for more details.
> *
> * You should have received a copy of the GNU General Public License
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> #ifndef __FLASH_H__
> #define __FLASH_H__ 1
>
> #include <unistd.h>
> #include <stdint.h>
> #include <stdio.h>
> -#include "hwaccess.h"
> #ifdef _WIN32
> #include <windows.h>
> #undef min
> #undef max
> #endif
> +#if NEED_PCI == 1
> +#include <pci/pci.h>
> +#endif
>
Hmmm... PCI accesses are hardware as well. Can you move them to
hwaccess.h or include them where needed?
Side note: In a perfect abstraction, the SPI programmer drivers should
not know anything about flash chips. Unfortunately, this still needs a
lot of work. I have a preliminary patch, but it's not ready for
consumption yet. It's not something you need to address, though.
> --- a/spi.c
> +++ b/spi.c
> @@ -139,191 +139,26 @@ int spi_send_command(unsigned int writecnt, unsigned int readcnt,
> }
>
> int spi_send_multicommand(struct spi_command *cmds)
> {
> if (!spi_programmer[spi_controller].multicommand) {
> fprintf(stderr, "%s called, but SPI is unsupported on this "
> "hardware. Please report a bug.\n", __func__);
> return 1;
> }
>
> return spi_programmer[spi_controller].multicommand(cmds);
> }
>
> -int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> - const unsigned char *writearr, unsigned char *readarr)
> -{
> - struct spi_command cmd[] = {
> - {
> - .writecnt = writecnt,
> - .readcnt = readcnt,
> - .writearr = writearr,
> - .readarr = readarr,
> - }, {
> - .writecnt = 0,
> - .writearr = NULL,
> - .readcnt = 0,
> - .readarr = NULL,
> - }};
> -
> - return spi_send_multicommand(cmd);
> -}
> -
> -int default_spi_send_multicommand(struct spi_command *cmds)
> -{
> - int result = 0;
> - for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
> - result = spi_send_command(cmds->writecnt, cmds->readcnt,
> - cmds->writearr, cmds->readarr);
> - }
> - return result;
> -}
> -
>
The two functions above need to stay in spi.c because they are generic
programmer code, not chip driver code.
> /* support 4 bytes flash ID */
> int probe_spi_rdid4(struct flashchip *flash)
> {
> /* only some SPI chipsets support 4 bytes commands */
> switch (spi_controller) {
> #if INTERNAL_SUPPORT == 1
> case SPI_CONTROLLER_ICH7:
> case SPI_CONTROLLER_ICH9:
> case SPI_CONTROLLER_VIA:
> case SPI_CONTROLLER_SB600:
> case SPI_CONTROLLER_WBSIO:
> #endif
> #if FT2232_SPI_SUPPORT == 1
> @@ -336,811 +171,46 @@ int probe_spi_rdid4(struct flashchip *flash)
> case SPI_CONTROLLER_BUSPIRATE:
> #endif
> #if DEDIPROG_SUPPORT == 1
> case SPI_CONTROLLER_DEDIPROG:
> #endif
> return probe_spi_rdid_generic(flash, 4);
> default:
> printf_debug("4b ID not supported on this SPI controller\n");
> }
>
> return 0;
> }
>
And even if this function looks programmer specific, it is chip driver
code. The only reason we perform any programmer differentiation is that
we don't have a generic can_run_this_command() function which would
check if the hardware supports the command in question. I have some code
to restructure this function from back when I attempted to extend the
SPI abstraction, but the code has bitrotted quite a bit, so it might be
faster to just rewrite it.
> int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len)
> {
> if (!spi_programmer[spi_controller].read) {
> fprintf(stderr, "%s called, but SPI read is unsupported on this"
> " hardware. Please report a bug.\n", __func__);
> return 1;
> }
>
> return spi_programmer[spi_controller].read(flash, buf, start, len);
> }
>
Hm yes, this one is very special. I can see arguments for declaring it
both as programmer code and as chip code. IMHO it is chip code because
it implies a certain read command. OTOH, it is a very specific
programmer operation which programmers can short-circuit. It would
definitely benefit from a can_run_this_command() function.
> +uint32_t spi_get_valid_read_addr(void)
> +{
> + /* Need to return BBAR for ICH chipsets. */
> return 0;
> }
>
Why did you move this function a few lines up? Just a cosmetic question
because the move makes tracking code history harder.
> new file mode 100644
> index 0000000..624e22d
> --- /dev/null
> +++ b/spi25.c
>
Please make sure this file is created with "svn cp spi.c spi25.c" and
then modified as needed before committing. git-svn may or may not do
that. I just want to make sure the code history stays intact.
> @@ -0,0 +1,964 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2007, 2008, 2009 Carl-Daniel Hailfinger
> + * Copyright (C) 2008 coresystems GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/*
> + * Contains the generic SPI framework
>
Contains the common SPI chip driver functions
> + */
> +
> +#include <string.h>
> +#include "flash.h"
> +#include "flashchips.h"
> +#include "chipdrivers.h"
> +#include "spi.h"
> +
> +void spi_prettyprint_status_register(struct flashchip *flash);
> +
> +int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> + const unsigned char *writearr, unsigned char *readarr)
> +{
> + struct spi_command cmd[] = {
> + {
> + .writecnt = writecnt,
> + .readcnt = readcnt,
> + .writearr = writearr,
> + .readarr = readarr,
> + }, {
> + .writecnt = 0,
> + .writearr = NULL,
> + .readcnt = 0,
> + .readarr = NULL,
> + }};
> +
> + return spi_send_multicommand(cmd);
> +}
> +
> +int default_spi_send_multicommand(struct spi_command *cmds)
> +{
> + int result = 0;
> + for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
> + result = spi_send_command(cmds->writecnt, cmds->readcnt,
> + cmds->writearr, cmds->readarr);
> + }
> + return result;
> +}
> +
>
The two functions above belong in spi.c (see above).
> +
> +/* support 4 bytes flash ID */
> +int probe_spi_rems(struct flashchip *flash)
>
Comment doesn't belong to function. It seems something got lost in the
shuffle.
What I could not check was function ordering. Did you keep the old order
while moving code from spi.c to spi25.c? If not, please do so because it
makes tracking history a lot easier.
Looks good otherwise.
Regards,
Carl-Daniel
--
"I do consider assignment statements and pointer variables to be among
computer science's most valuable treasures."
-- Donald E. Knuth
More information about the flashrom
mailing list