Am 11.05.2011 10:49 schrieb Michael Karcher:
Am Freitag, den 06.05.2011, 22:13 +0200 schrieb Carl-Daniel Hailfinger:
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.
MAX_DATA_WRITE_UNLIMITED happens to be 256 to "keep the 256 byte limit for now". So don't worry about that.
OK.
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.
This is wrong. The simulated flash chip accepts emu_max_byteprogram_size byte chunks, while the programmer accepts spi_write_256_chunksize. But this variable is indeed redundant, when we use a patched programmer info structure that contains the maximum write chunk size.
Right, I incorrectly remembered this part.
+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?
My current patch lets the bitbanging core register the SPI driver. I consider this advantageous, because this less redundant code and a more clean layer separation (spi core<-> bitbang core<-> bitbang driver) where each layer only communicates with the adjacent layers.
Given that we'll kill the programmer type variable anyway and all programmer instance settings will be handled with a private pointer in struct spi_programmer, this is pretty much irrelevant. Just go ahead with your version.
The first variant needs copying/patching,
Only if we really need to have the different bitbang adapter types in the spi_controller structure which I am still not convinced in.
Postponed this decision.
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 we are planning to provide a nice "private data" element soon, and controllers are going to use the private data structure for MMIO base address and the like,
Right, but I'd like to keep some data in local static variables if the programmer can't be instantiated more than once. That said, we'll decide about this issue in the next patch.
an opaque "controller type" in the public structure seems to make no sense. Probably we look at it from different points of views. From my point of view, namely the lower-level SPI drivers, I am envisioning the end of controller_type quite soon. In the low-level drivers, it seems to be used in ichspi only, so the discussion of the use of this field as private ingo for other low-level drivers (e.g. for deciding whether we need a single "bitbang" or many different types seems pointless. I am still tending towards a single "BITBANG" type. Low-level doesn't need more specific info and high-level probably doesn't care what type of bitbang host it is.
Agreed. A private discussion cleared this up.
Better to have feature flags instead like "SPI_MASTER_CAN_DO_SPI".
Sorry, -ENOPARSE.
Of course. I hope it parses with SPI_MASTER_CAN_DO_AAI.
Right.
void *private_data is the best choice. I retract my union suggestion.
Fine. Will not go into this patch, though, put in a later patch.
OK.
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel