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

[Michael Karcher]

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 [e-mail]\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

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

Regards,
  Michael Karcher