Am 06.05.2011 13:30 schrieb 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 Hailfingerc-d.hailfinger.devel.2006@gmx.net
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.
Right. However, generic write calls spi_write_chunked which calls spi_nbyte_program which will cause a stack overflow for writes larger than 256 bytes. That's why I'd like to keep the 256 byte limit for now.
.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.
The chunk size accepted by the simulated flash chip and the chunk size accepted by the dummy programmer are identical anyway, so killing that variable indeed makes sense.
+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.
And that's the interesting question. Should the bitbanging core register a SPI programmer or should each individual driver do it? The first variant needs copying/patching, the second variant creates "duplicated" (copy-pasted-modified) code. I don't have a strong preference in either direction.
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.
Limits, but also stuff like which MMIO address to use. It's controller private data and should be opaque to the spi registration core.
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.
Fully agreed.
Better to have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
Sorry, -ENOPARSE.
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,
void *private_data is the best choice. I retract my union suggestion.
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.
Which one is the "more OO like approach"? Pointer or derived struct casting? I prefer the pointer variant. And yes, union was a bad idea.
Thanks for your review,
Thank you for the patch and for the excellent response.
Regards, Carl-Daniel