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

[Carl-Daniel Hailfinger]

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 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/