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

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Mon Dec 19 22:57:50 CET 2011


Am Montag, den 19.12.2011, 02:38 +0100 schrieb Carl-Daniel Hailfinger:
> > A patch that is rebased to that revision and fixes a compiler warning
> > (which gets fatal due to -Werror) is attached.
> Used those changes as basis for my reworked patch. Did you introduce any
> functional changes except the compiler warning fix?
If I remember correctly, apart from that warning fix I just shuffled
whitespace around to fix the 80-column-wrap-mismatches.

> I don't see a good way to move the max_data_read check in
> default_spi_read to an init function, though, because you can never be
> sure at init time if default_spi_read will be called.
Currently, default_spi_read is only called directly from the spi
programmer structure, but we can't be sure a programmer decides to use
the default implementation only sometimes, so agreed, we have to keep
the checks.

> > 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.
> noop_chip_readb dropped.
> noop_chip_writeb may be useful if we merge a programmer driver which
> does not support write.
> What do you think?
If we find a way to revive the "blind read via force", good idea. On the
other hand, I think a opaque mmap dumper would be more apt for the blind
read. It should just get the chip size and the optionally start or end
address of the mapping as parameter.

> >>  	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.
> I beg to differ. This is a message to the end user, and the end user
> does not think of a mainboard as multiple programmers.
As on IRC: "The following protocols are supported".

> ichspi.c:
> Use ich_generation instead of spi_programmer->type (or
> flash->pgm->spi.type) in run_opcode to determine if the programmer was
> initialized correctly.
> This change is debatable because it uses a local static variable instead
> of the info available in flash->pgm, but OTOH now the internal usage is
> consistent.
consistency first - we can eliminate the local variable later anyway
(and we are going to, I expect)

> Another question is whether we should really check for
> correct initialization at this point.
I guess the checking makes no sense anymore, as there is no way to enter
this function without going through the init function.

> The programmer registration functions now return error/success instead
> of void, but their callers don't care. Fix now or later?
Fix later. We already inform the user if we are losing programmers,
that's enough for now.

> TODO?
> - max_decode is a programmer property, add it to the
> register_*_programmer parameters
> - programmer_may_write is a programmer property, add it to the
> register_*_programmer parameters
Yes, both of them should be added to the programmer structure. I am
unsure whether we want rarely used options (like may-not-write) as
parameters though. Having the size there is good - it makes the
programmer think about the maximum supported size.

> All programmer types (Parallel, SPI, Opaque) now register themselves
> into a generic programmer list and probing is now programmer-centric
> instead of chip-centric.
TODO: split the flash chip list into SPI and non-SPI lists. No need to
iterate over the other type and skip those chips because of bus
mismatches. Do it later.

> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Acked-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

 
>  int bitbang_spi_shutdown(const struct bitbang_spi_master *master)
>  {
> -	if (!bitbang_spi_master) {
> +	if (!master) {
>  		msg_perr("Shutting down an uninitialized SPI bitbang master!\n"
>  			 "Please report a bug at flashrom at flashrom.org\n");
>  		return 1;
As I just found out, bitbang_spi_shutdown is never called at all, but if
it will be called, it will be a callback registered in bitbang_spi_init
which already checked master for nullness. Also the function could be
made static in that case.

>  	msg_pdbg("This programmer supports the following protocols: %s.\n",
>  		 tempstr);
Message change: see intro.

>  	if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) {
>  		msg_perr("%s called with one of probe/read/write/erase being "
>  			 "NULL. Please report a bug at flashrom at flashrom.org\n",
>  			 __func__);
> -		return;
> +		return ERROR_FLASHROM_BUG;
Later, we could add a feature that allows read-only "programmers" that
have write and erase NULL, setting programmer_may_write to FALSE.

> -void register_spi_programmer(const struct spi_programmer *pgm)
> +int register_spi_programmer(const struct spi_programmer *pgm)
>  {
> -	spi_programmer = pgm;
> -	buses_supported |= BUS_SPI;
> +	struct registered_programmer rpgm;
> +
> +	if (!pgm->write_256 || !pgm->read || !pgm->command ||
> +	    !pgm->multicommand ||
> +	    ((pgm->command == default_spi_send_command) &&
> +	     (pgm->multicommand == default_spi_send_multicommand))) {
> +		msg_perr("%s called with inconsistent settings. "
s/inconsistent settings/incomplete programmer definition/

> +	if (!chip_writeb || !chip_writew || !chip_writel || !chip_writen ||
> +	    !chip_readb || !chip_readw || !chip_readl || !chip_readn) {
> +		msg_perr("%s called with one of probe/read/write/erase being "
dito.

Regards,
  Michael Karcher





More information about the flashrom mailing list