Re: [spi-devel-general] Re: SPI

[David B. <da...@pa...>]

On Monday 28 November 2005 9:14 pm, Vitaly Wool wrote:
> Hi David,
> 
> there're a few things I still dislike in your core. Namely,
> 
> - leaving too much for the controller driver. I am sure that the core 
> should provide default techniques for queue handling, it's a common 
> thing to do

Well, every driver needs _some_ sort of queue handling, but they
don't all need the same handling.  I said more earlier:

  http://marc.theaimsgroup.com/?l=linux-kernel&m=112684135722116&w=2

And in any case, this takes _maybe_ a couple dozen lines for one
simple driver style, which is IMO clearly on the "don't need
to share code" side of things.  It's a trivial price to pay for
that level of flexibility.


> - you call wait_for_completion() in the core

Not exactly.  Those synchronous calls are non-essential wrappers,
so I don't think of them as "core".  Though one could argue; it's
still true those calls could easily end up unused, and thus be
candidates for link-time dead code removal.


>	 and leave the complete()  
> call fot the controller driver. This is IMHO a good way to produce 
> erroneous code

The requirement is simple:  for each transfer() call that the controller
driver accepts, it will issue the relevant completion callback once when
that message has been completed.  It's the same model used with USB
(both host and peripheral sides), and it's the same model used in various
other systems too.

It's a simple rule, very easy to understand ... that's what makes it a
good way to **eliminate** coding errors.  There's no possible ambiguity
there; any time that rule isn't followed would clearly be a fundamental
driver bug that would cause extensive breakage ... nothing subtle or
hard to find.  And there are no special cases to confuse anyone.

In fact any sane controller driver is going to have one single call
to spi_message->complete(), so if there are more (or less) that'll
be something suspicious for any code reviewer to notice quickly.

- Dave



> I also think that callbacks for retrieving next message from queue is 
> better way to do it.
> 
> Greg, BTW, can you please share the review results?
> 
> Best regards,
>    Vitaly
> 
> David Brownell wrote:
> 
> >On Wednesday 23 November 2005 2:01 pm, David Brownell wrote:
> >  
> >
> >>On Friday 11 November 2005 1:32 am, dmitry pervushin wrote:
> >>    
> >>
> >>>This is an updated version of SPI framework from me, Dmitry Pervushin.
> >>>It seems that now it is good time to consolidate our SPI frameworks to
> >>>push it to kernel :)
> >>>      
> >>>
> >>Agreed.
> >>    
> >>
> >
> >And I see Andrew merged my framework into MM, and that it's already
> >gotten some patches.  I think that means the next MM kernel (rc2-mm2?)
> >will be a good base for that consolidation.  (Dunno about 2.6.16...)
> >
> >
> >What this suggests to me is that there may need to be a few more
> >patches on the way.  Drivers, especially ... but also a few in the
> >framework itself. I fully expect someone to want:
> >
> >  
> >
> >> - Yours has a "struct spi_driver", mine doesn't.  It's probably
> >>   inevitable, though not currently needed.  Likewise mine has
> >>   the struct driver.suspend() and resume() changes, yours doesn't.
> >>    
> >>
> >
> >That's in the "best done early" category.
> >
> >
> >Sometime after Stephen posts his updated pxa2xx_spi driver, I'll
> >be able to post the Lubbock board-specific patch ... that'll mean
> >one "reference" board (albeit an old one) has out-of-the-box support
> >on Linux.  Given the number of PXA25x systems with an ADS7846, I'd
> >hope to see some testers of that stack (and, surely, bugfixes).
> >
> >One of the things that would IMO help here is getting a bitbanging
> >adapter driver:  rub four GPIO pins together, make a fire.  ;)
> >
> >If anyone's at work on such a thing (Mark?) please drop a line;
> >it looks to me like this should be very simple, and incidentally
> >help nail down any semantics that aren't described in the docs.
> >
> >- Dave
> >
> >
>