[flashrom] [PATCH 1/5] Add opaque programmer registration infrastructure.

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Wed Nov 2 18:29:57 CET 2011


Am Freitag, den 21.10.2011, 00:39 +0200 schrieb Stefan Tauner:
> diff --git a/flash.h b/flash.h
> index 535c1b8..8ad2845 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -62,8 +62,9 @@ enum chipbustype {
>  	BUS_LPC		= 1 << 1,
>  	BUS_FWH		= 1 << 2,
>  	BUS_SPI		= 1 << 3,
> +	BUS_PROG	= 1 << 4,
>  	BUS_NONSPI	= BUS_PARALLEL | BUS_LPC | BUS_FWH,
> -	BUS_UNKNOWN	= BUS_PARALLEL | BUS_LPC | BUS_FWH | BUS_SPI,
> +	BUS_UNKNOWN	= BUS_PARALLEL | BUS_LPC | BUS_FWH | BUS_SPI | BUS_PROG,
We don't need BUS_UNKNOWN. It is referenced only in flashbuses_to_text.
Having a programmer being both opaque and non-opaque seems to be as
useless as defining that a chip "might also be accessed through an
opaque interface". In the case of opaque interfaces, we don't care about
the chip type - so I don't see any use case for BUS_UNKNOWN, even more
after adding BUS_PROG.

>  };
> +#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> +	{
> +		.vendor		= "Programmer",
> +		.name		= "Opaque flash chip",
> +		.bustype	= BUS_PROG,
> +		.manufacture_id	= PROGMANUF_ID,
> +		.model_id	= PROGDEV_ID,
> +		.total_size	= 0,
> +		.page_size	= 256,
> +		/* probe is assumed to work, rest will be filled in by probe */
> +		.tested		= TEST_OK_PROBE,
> +		.probe		= probe_opaque,
> +		/* eraseblock sizes will be set by the probing function */
> +		.block_erasers	=
> +		{
> +			{
> +				.block_erase = erase_opaque,
> +			}
> +		},
> +		.write		= write_opaque,
> +		.read		= read_opaque,
> +	},
> +#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
Why do we need any prerequisite for the opaque programmer? It should
work outside of x86(-64), too, depending on which opaque backend is
used. I guess this #if is there because at the moment, the only opaque
programmer is the ICH9 stuff, but wouldn't a define NEED_OPAQUE that is
set in the include file as soon as any programmer based on the opaque
programmer is selected be more sensible?

> @@ -9005,6 +9027,7 @@ const struct flashchip flashchips[] = {
>  		.probe		= probe_spi_rdid,
>  		.write		= NULL,
>  	},
> +
>  	{
>  		.vendor		= "Generic",
>  		.name		= "unknown SPI chip (REMS)",
Unrelated change - you might have it in accidentally.

> +/*
> + * Contains the opaque programmer framework.
> + * An opaque programmer is a programmer which does not provide direct access
> + * to the flash chip and which abstracts all flash chip properties into a
> + * programmer specific interface.
> + */
> +
> +#include <strings.h>
> +#include <string.h>
> +#include "flash.h"
> +#include "flashchips.h"
> +#include "chipdrivers.h"
> +#include "programmer.h"
> +
> +const struct opaque_programmer opaque_programmer_none = {
> +	.max_data_read = MAX_DATA_UNSPECIFIED,
> +	.max_data_write = MAX_DATA_UNSPECIFIED,
> +	.probe = NULL,
> +	.read = NULL,
You are missing ".erase = NULL" here.

> +	.write = NULL,
> +};
> +
> +const struct opaque_programmer *opaque_programmer = &opaque_programmer_none;
> +
> +int probe_opaque(struct flashchip *flash)
> +{
> +	if (!opaque_programmer->probe) {
> +		msg_perr("%s called, but this is not an opaque programmer. "
> +			 "Please report a bug at flashrom at flashrom.org\n",
> +			 __func__);
I don't really like the error message, I would prefer "%s called without
register_opaque" or something like that.

> +void register_opaque_programmer(const struct opaque_programmer *pgm)
> +{
> +	opaque_programmer = pgm;
> +	buses_supported |= BUS_PROG;
> +}
You might want to check that the function pointers in pgm are not NULL,
to make the suggested error message "without register_opaque" more apt.

Otherwise, the patch looks good to me. This is
Acked-By: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

If you feel confident you didn't mess up anything, feel free to re-use
the ack after minor edits.

Regards,
  Michael Karcher





More information about the flashrom mailing list