[flashrom] [PATCH 3/3] kill central list of SPI programmers
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri May 6 22:13:40 CEST 2011
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<c-d.hailfinger.devel.2006 at 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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list