Re: [flashrom] [PATCH 3/3] kill central list of SPI programmers

[Michael Karcher]

Am Freitag, den 06.05.2011, 00:40 +0200 schrieb Carl-Daniel Hailfinger:
> Thanks for your patch!
> I have a few minor nitpicks and design questions, but the code changes 
> seem to be bug-free, so you get an
> Acked-by: Carl-Daniel Hailfinger

Thanks for the Ack, but I think we need one iteration of discussion.
> Your patch is very readable and got me thinking about future directions 
> of that code, especially multiple registrations... so take the review 
> with a grain of salt.

OK, I keep thinking of multiple registrations...
> > +static const struct spi_programmer spi_programmer_dummyflasher = {
> >    .type = SPI_CONTROLLER_DUMMY,
> >    .max_data_read = MAX_DATA_READ_UNLIMITED,
> >    .max_data_write = MAX_DATA_UNSPECIFIED,
>
> 256 instead?

I used UNSPECIFIED because I didn't use the generic write function (for
whatever reason). I am inclined to use "MAX_DATA_WRITE_UNLIMITED"
though, as the dummy flasher does not have any hardware limits on
maximal write size.
> >    .command = dummy_spi_send_command,
> >    .multicommand = default_spi_send_multicommand,
> >    .read = default_spi_read,
> >    .write_256 = dummy_spi_write_256,
>
> Same comment as for patch 1 of the series. AFAICS dummy_spi_write_256 
> should be replaced by the default one.

If we patch the structure to avoid the global variable
"spi_write_256_chunksize", you are right.
> Please fix the whitespace (spaces instead of tabs) in it87spi.c and 
> dummyflasher.c.

Argh, I am going to kill my "smart" editor. It tries to guess
white-space type from the file contents, and usually fails.
> > +static const struct spi_programmer spi_programmer_bitbang = {
>
> Can you make this one non-const to allow changing .type?

No, I can't. If I think of multiple registrations, I also think of
multiple bit-banging adapter. I can of course have bitbang_spi_init()
make a copy and patch type in that.
> Could you extend this function signature to have an additional enum 
> spi_controller parameter? If each bitbang-using programmer calls 
> bitbang_spi_init(spi_controller, master, halfperiod) we can keep the 
> spi_controller enums unchanged for now which would help us keep things 
> consistent (for example, ichspi.c uses 3 different spi_controller enums 
> for the same functions).

The controller type enum is used as far as I can see to take care of
limits of certains SPI hosts. As the limits (number of read/written
bytes, whether there is a lowest accessible address and so on) are the
same for all bitbang programmers, so I consider having one type for all
bitbang programmers to be superior to having different types.

In the long run, I suggest to get rid of the type alltogether. Better to
have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
> I plan to add a union private_data{} to struct spi_programmer to allow 
> storing stuff like struct bitbang_spi_master, avoiding the need for 
> programmer-internal static configuration variables.

I thought of either "void *private_data", so the generic does not have
to know about the types of private_data, or having derived structures
that start with a struct spi_programmer and cast them in the programmer
implementation to the derived type while registering them as the first
object, like this:

struct ich_spi_programmer {
    struct spi_programmer spipgm;
    unsigned int spi_bar;
}

int ich_spi_init()
{
    [...]
    register_spi_programmer(&ich_spi_programmer.spipgm);
    [...]
}

int ich_spi_send_command(struct spi_programmer * spipgm, ...)
{
    struct ich_spi_programmer * pgm = (struct ich_spi_programmer*)spipgm;
    [...]
}

Of course your idea gets rid of the cast, and the size of the flashrom
project is small enough that we don't have to care about the generic
code depend on all the specific programmer types. While I still prefer
the "more OO like" approach I pointed out, I have no problem with the
union approach, too.

Thanks for your review,
  Michael Karcher