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 [email protected]\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 <[email protected]>
If you feel confident you didn't mess up anything, feel free to re-use
the ack after minor edits.
Regards,
Michael Karcher
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
