[flashrom] [PATCH] Generic programmer registration: final part

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Sun Dec 18 14:22:34 CET 2011


Am Samstag, den 17.12.2011, 15:23 +0100 schrieb Carl-Daniel Hailfinger:
> This depends on [PATCH] Add struct flashctx * everywhere
But not on the revision you posted on December 16, 14:41 CET.

A patch that is rebased to that revision and fixes a compiler warning
(which gets fatal due to -Werror) is attached.

> - mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI)
ICH7 SPI works for reading. Currently I don't have no hardware at hand
to test other stuff.

>  int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt,
>  		const unsigned char *writearr, unsigned char *readarr)
>  {
> -	if (!spi_programmer->command) {
> +	if (!flash->pgm->spi.command) {
>  		msg_perr("%s called, but SPI is unsupported on this "
>  			 "hardware. Please report a bug at "
>  			 "flashrom at flashrom.org\n", __func__);
>  		return 1;
>  	}
I suggest to drop the NULL pointer check here, and just do it once in
register_spi_programmer (refusing to call register_programmer if any
function pointer is NULL)
Same for the other functions in here.


> -static const struct par_programmer par_programmer_none = {
> -		.chip_readb		= noop_chip_readb,
> -		.chip_readw		= fallback_chip_readw,
> -		.chip_readl		= fallback_chip_readl,
> -		.chip_readn		= fallback_chip_readn,
> -		.chip_writeb		= noop_chip_writeb,
> -		.chip_writew		= fallback_chip_writew,
> -		.chip_writel		= fallback_chip_writel,
> -		.chip_writen		= fallback_chip_writen,
> -};
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as
we drop par_programmer_none, as these functions are no longer used.


> +enum chipbustype get_buses_supported(void)
> +{
> +	int i;
> +	enum chipbustype ret = BUS_NONE;
> +
> +	for (i = 0; i < registered_programmer_count; i++)
> +		ret |= registered_programmers[i].buses_supported;
> +
> +	return ret;
>  }
Looking at this function, just keep in mind it returns a list of busses
supported by all the programmers found.

> +#if 0 // Does not really make sense anymore if we use a programmer-centric walk.
>  			msg_gspew("Probing for %s %s, %d kB: skipped. ",
>  			         flash->vendor, flash->name, flash->total_size);
> -			tmp = flashbuses_to_text(buses_supported);
> +			tmp = flashbuses_to_text(get_buses_supported());
>  			msg_gspew("Host bus type %s ", tmp);
If we reinstate this code (i.e. decide to not #if 0 it), don't use
get_buses_supported(), but pgm->buses_supported. Also don't call it
"Host bus type", but "programmer bus type"

> --- flashrom-register_all_programmers_register_generic/ogp_spi.c	(Revision 1473)
> +++ flashrom-register_all_programmers_register_generic/ogp_spi.c	(Arbeitskopie)
> @@ -91,6 +91,8 @@
>  	.get_miso = ogp_bitbang_get_miso,
>  	.request_bus = ogp_request_spibus,
>  	.release_bus = ogp_release_spibus,
> +	/* no delay for now. */
> +	.half_period = 0,
Is this comment really useful? Opposed to where it was used before, in
this place, the name of the setting "half_period" is explicitly
mentioned.

> +	tempstr = flashbuses_to_text(get_buses_supported());
>  	msg_pdbg("This programmer supports the following protocols: %s.\n",
>  		 tempstr);
It's definitely not "this programmer", as should be obvious if you "kept
in mind" as I told you that get_buses_supportet returns the union of all
programmers. So either move this into the loop over all the programmers
and print programmer specific bus types, or call it something like "The
programmers in this systems provide the following protocols: %s.\n"

> +	/* 1 usec halfperiod delay for now. */
> +	.half_period = 1,
Same comment as above. If you want to have the unit visible, rename the
variable to half_period_usecs, instead of adding comments of
questionable information content.

No ack yet because of the non-appliacation of your patch and the use of
get_buses_supported where a specific programmer bus list would make more
sense.
The "superflous comment" issue, the "NULL pointer on each spi call"
issue, and the "orphaned noop" issue are not something that blocks
acking the patch.

Regards,
  Michael Karcher
-------------- next part --------------
A non-text attachment was scrubbed...
Name: generic_pgm_final.diff
Type: text/x-patch
Size: 33500 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20111218/dfa516f4/attachment.diff>


More information about the flashrom mailing list