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

[Carl-Daniel Hailfinger]

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/