Messages in this thread | | | Date | Mon, 3 Oct 2005 11:57:48 +0100 (BST) | From | Mark Underwood <> | Subject | Re: [RFC][PATCH] SPI subsystem |
| |
--- David Brownell <david-b@pacbell.net> wrote:
> > > > I notice that there is no bus lock. Are you expecting the adapter > > > > driver to handle the fact that its transfer routine could be called > > > > before a previous call returns? > > > > > > Yes. The transfer routine is purely async, and its responsibility > > > is to append that spi_message to the current queue. (Assuming > > > the driver isn't a simple pure-PIO driver without a queue...) > > > > > > That's a simple matter of a spin_lock_irqsave/list_add_tail/unlock. > > > > > > > OK. Thought so. I think that in the documentation (when it gets written ;) > > we need to warn people that they can only do quick work (adding message > > to a queue or waking up a kthread) in the transfer routine > > The documented constraint -- right by the declaration of that > particular method!! -- is that it may not sleep. That suffices. >
Sorry, must have had my glasses on back to front ;)
> > > as it would > > not be fair for a PIO driver to transfer several KB in what might be > > interrupt context. > > That's a "quality of implementation" issue. There are a lot of > different SPI drivers floating around today that are pure PIO; > they're used for sensor access, and work in exactly that way. > (And without any buslock.) > > When the driver is only reading/writing a handful of bytes, PIO > can easily be "quick" ... and may well be quicker than going > through a queue manager. Example: if SPI is clocked at 8 MHz, > that's a microsecond per byte. Add a smidgeon of overhead, > and call it 5 usecs to read a sensor that way. > > One point of standardizing an API is to support a broad range > of different controller driver optimization points. They should > all work correctly of course. A DMA driver may be the ticket for > running from SPI flash ... but setting up DMA for just a couple > bytes is likely not a win. >
True. In our SPI adapter driver we check to see if the transfer is below is certain size in which case it is quicker to do PIO, otherwise we do DMA.
> > > So your asking the adapter to keep a 'personality' for each device on > > that bus (clock speed, cs & clock mode etc) and then just before the > > transfer to/from a device is started the adapter takes the 'personality' > > of that device (i.e. sets clock speed registers if needed etc)? > > As you noted later, yes. Most of the SPI controllers I've looked > at will do that in hardware, for that matter. PCI drivers don't > need to arbitrate bus access themselves; neither should SPI drivers. >
OK. Our hardware doesn't :(, so I'll have to emulate it. It's an interesting idea and as you say it is more optimal for devices that have this support :). To make it quicker for devices that don't have this support in hardware how would you feel about having a 'void *personality' pointer in the spi_device structure which the adapter could use for storing and accessing the register settings for clock etc for that SPI device?
> > > > > > +EXPORT_SYMBOL_GPL(spi_new_device); > > > > > > > > I think we should have a bus lock (in the adapter structure) for > > > > safety, and in the remove routine as well. > > > > > > Why? I don't see any need for one, at least in the "all drivers > > > must use this one" category. Persuade me; what problems would such > > > a lock solve? > > > > > > > Problems with parallel calls to register spi device/unregister > > spi device/transfer? > > Only an issue if the driver core had bugs ... bugs that would break > many more things than just SPI. :) > > > > > The parallel port adapter wouldn't use that interface. It would > > > instead be using spi_new_device() with board_info matching the > > > device (Ethernet, EEPROM, USB controller, etc) ... > > > > OK. So if I had an array of devices then I have to go though that array > > and call spi_new_device() for each one? Where do I get spi_master > > from? I need a function to which I can pass the name/bus number to and > > get a spi_master pointer in return. > > You're the one who's defining the "parallel port adapter with device" > thing ... so you've got the spi_master that you created. In fact you > probably used dev_set_drvdata(dev, master) to keep it handy. >
Ahh, but the spi_master structure is in /usr/src/linux/drivers/spi/busses/spi-parport.c and my array of devices is in ~/spi-work/parprt_adapter_1.c
> > > > Sorry I didn't make myself clear. I mean check the complete element in > > the spi_message structure when spi_transfer is called. So: > > > > int spi_transfer(struct spi_device *spi, struct spi_message *message) > > { > > if (message->complete) > > /* We have callback so transfer is async */ > > else > > /* We have no callback so transfer is sync */ > > } > > > > Although thinking about it this is probably a bad idea as it could b > > prone to errors > > That's a large part of why I would never support that model. :) > > > > > > Hmm, using local variables for messages, so DMA adapter drivers have > > > > to check if this is non-kmalloc'ed space (how?) > > > > > > They can't check that. It turns out that most current Linuxes > > > have no issues DMAing a few bytes from the stack. > > > > Will the DMA remapping calls work with data from the stack? > > On "most current Linuxes" yes. All I know about, in fact. > But it's not guaranteed.
OK. Thanks
Mark
> > - Dave > > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
___________________________________________________________ Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |