Re: [PATCH 2.6-git] SPI core refresh

[Vitaly Wool <vwool@ru.mvista.com>]

David Brownell wrote:

>On Monday 05 December 2005 10:01 am, Vitaly Wool wrote:
>  
>
>>Again, some advantages of our core compared to David's I'd like to mention
>>
>>- it can be compiled as a module (as opposed to basic David's core w/o Mark Underwood's patch)
>>- it is less priority inversion-exposed
>>    
>>
>
>These are actually minor issues, with almost trivial fixes.  (Pending.)
>
>And I'd argue about the priority inversion thing ... the inversions in
>your stuff come from the API, not the implementation.  Which makes the
>problems inherent, rather than fixable:  "more" exposed, not "less".
>  
>
I don't think it's  true any more (with this new patch).

>>2. We still think that thread-based async messages processing will be the most
>>commonly used option so we didn't remove it from core but made it a compication
>>option whether to include it or not. In any case it can be overridden by a
>>specific bus driver, so even when compiled in, thread-based handling won't
>>necessarily _start_ the threads, so the overhead is minimal even in that case.    
>>    
>>
>
>Whereas I've just said such threading policies don't belong in a "core" at
>all.  You may have noticed the bitbanging adapter I posted ... oddly, that
>implementation allocates a thread.  Hmm ...
>  
>
Please remember that using threads is just a default option which may be 
even turned off at the kernel configuration stage.
Using threads for async transfers is reasonable _default_ assumption. It 
can save time for controller driver dev'ers as well as reduce overall 
kernel footprint (as opposed to different controller drivers each 
implementing thread-based async messages handling).

>That is:  you're not talking about capabilities that aren't already in
>the SPI patches already circulating in 2.6.15-rc5-mm1 (from Sunday).
>They're just layered differently ... the core is _minimal_ and those
>implementation policies can be chosen without adding to the core.
>(Or impacting drivers that want different implementation policies.)
>  
>
Hmm, how are we impacting drivers that want different implementation 
policies? Please look at the latest core :)

>
>  
>
>>3. We still don't feel comportable with complicated structure of SPI message in
>>David's core being exposed to all over the world. On the other hand, chaining
>>SPI messages can really be helpful,
>>    
>>
>
>The point has been made that such chaining is actually "essential", since
>device interaction protocols have constraints like "chipselect must be
>asserted during all these transfers" or contrariwise "between these transfers,
>chipselect must be dropped for N microseconds".  If the async messages didn't
>cover such linked message, then two activities could interfere with each other
>quite badly ... they'd break hardware protocol requirements.
>
>  
>
I don't argue about the need of chaining.
But don't you want to agree that things like this

+	struct spi_transfer	x[1] = { { .tx_dma = 0, }, };
...more initialization follows, spread around the code...


are not well-readable and may shadow what's going on and even lead to 
errors during support/extension of the functionality?
Exposing SPI message structure doesn't look good to me also because 
you're making it a part of the interface (and thus unlikely to change).
We're hiding spi_msg internals what allows us to be more flexible in 
implementation (for instance, implement our own allocation technique for 
spi_msg (more lightweight as the size is always the same).

Yeah thus we don't have an ability to allocate SPI messages on stack as 
you do, that's what votes for your approach. Yours is thus a bit faster, 
though I suspect that this method is a possible *danger* for really 
high-speed devices with data bursts on the SPI bus like WiFi adapters: 
stack's gonna suffer from large amounts of data allocated.

>>so we added this option to our core (yeeeep, 
>>convergence is gong on :)) but
>>    
>>
>
>My preferred level of convergence would be changes to make your code look
>more like mine, especially where you're providing a mechanism that's been
>in mine all along ... hmm, like spi_driver is the same now.  I made one
>such change; maybe it's your turn now.  ;)
>  
>
Okay :)

Vitaly