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

[dmitry p. <dpe...@gm...>]

David,

some additional comments on your code:

- you've made spi_read_then_write DMA-capable... OK. But why you're
using global for buffer ? You need to create the semaphore to protect
the global 'buf', so if you have two controllers on the same board, you
would not perform two simultaneous transfers with these controllers,
right ? I's like to say that buf should be either controller data or
message data
- sometimes you're ignoring return codes of functions that return int,
e.g., bus_register and class_register

On Mon, 2005-11-28 at 23:36 -0800, David Brownell wrote:
> 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
> > >
> > >
> > 
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> _______________________________________________
> spi-devel-general mailing list
> spi...@li...
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
> 
>