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

[Michael Karcher]

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

Acked-by: Michael Karcher
> 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 [e-mail]\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 [e-mail]\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