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 Hailfinger<[email protected]>
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
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
