--- David Brownell <[email protected]> wrote:
> Sorry for the delay getting back to these comments; I wanted to
> give them proper attention, which kept not arriving.
>
I'm glad I'm not the only one :)
>
> > Date: Sun, 18 Sep 2005 15:45:20 +0100 (BST)
> > From: Mark Underwood <[email protected]>
>
> First comments for <linux/spi.h>:
>
> > > +struct spi_device { /* this proxies the device through a master */
> > > + struct device dev;
> > > + struct spi_master *master;
> > > + u32 max_speed_hz;
> > > + u8 chip_select;
> > > + u8 mode;
> > > +#define SPI_CPHA 0x01 /* clock phase */
> > > +#define SPI_CPOL 0x02 /* clock polarity */
> > > +#define SPI_MODE_0 (0|0)
> > > +#define SPI_MODE_1 (0|SPI_CPHA)
> > > +#define SPI_MODE_2 (SPI_CPOL|0)
> > > +#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> >
> > Would be more flexable to have this in the message or even
> > the spi_transfer structure. Although I
> > don't know who would need this flexability.
>
> In this case, I don't see a benefit. The chips support only one
> signaling method at a time. It can be changed between requests,
> by calling spi_setup(...), but even that will be rare. I don't
> think there's any point to encouraging finer grained changes.
>
>
> > > +struct spi_master {
> > > + ...
> > > +};
> >
> > 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 as it would not be fair for a PIO driver to transfer several KB in what might be
interrupt context.
>
> > > +struct spi_transfer {
> > > + /* it's ok if tx_buf == rx_buf (right?)
> > > + * for MicroWire, one buffer must be null
> > > + * buffers must work with dma_*map_single() calls
> > > + */
> > > + void *tx_buf, *rx_buf;
> > > + unsigned len;
> > > +};
> >
> > I would like more flexability. I might want to toggle the CS line within
> > a message or another CS line which is really a GPO pin used for register
> > select. For example a char LCD with SPI interface
> > would require this and yes, they do exist! I've used one :).
>
> I've been persuaded that at least the "toggle chipselect" thing
> is needed, because of chips like the CS8415A (or ISTR some EEPROMs)
> that read by starting a write (to set a data pointer), dropping
> chipselect temporarily, then issuing the read. Those all need
> to be treated as single "spi_message".
>
More convergence, good :)
>
> > > +
> > > + /* Optionally leave this chipselect active afterward */
> > > + unsigned csrel_disable:1;
> >
> > This would be a disaster as anther SPI device driver might have
> > put a transfer straight after this one, in which case that message
> > would be sent to both devices :(, or has the driver that did this
> > take a lock on the bus? If so what lock?.
>
> That's not how it works. No spi_message starts unless _only_ that
> device's chipselect is active. If some other chipselect is still
> active, it must first be turned off. No lock needed, beyond the fact
> that the controller has only one queue and driver ... that driver
> ensures many correctness issues, not just this one.
I see. Sorry I'm mixing up our subsytems :(.
>
> The point of that option is to minimize the overhead of starting a
> new transaction to a "favored" device. I understand that's needed
> with some of the CORGI (Zaurus) touchscreen support. (Along with
> some other funky stuff like vertical retrace synchronization!)
>
Yes that is something that I have started to think about with respect to adding messages in
callback context (e.g. a network device which you have to to a write/read combination to get the
amount of data in the buffer and then just contine reading to get the data). But I want to get
what we have at the moment sorted before moving onto things like that.
>
> > > + /* completion is reported this way */
> > > + void (*complete)(void *context);
> > > + void *context;
> > > + unsigned actual_length;
> > > + int status;
> > > +
> > > + /* for optional controller driver use */
> > > + struct list_head queue;
> >
> > If your putting this here wouldn't it make sense to also add
> > a list_head to the adapter structure?
>
> That's only the first of many chunks of driver-private data
> they'll need. And as I've commented before, there's no business
> any other software has touching that queue ... assuming the
> controller driver is even written to use a queue.
>
> (Many current SPI drivers just spin using PIO to complete requests,
> and they could be pretty easily converted to this framework without
> forcing that character to change right away ...)
>
>
> > > +};
> >
> > Clock speed should also be in this structure as a SPI device might
> > want to change the speed it's clocked at for each message.
> > For example MMC cards are probed at 400KHz but can be read/written to
> > at up to 25MHz.
>
> The way I see that being done is by just calling spi_setup() to
> update the device speed. That's a direct mirror of how it's
> done in the MMC (or PCMCIA) stacks: a separate call to set
> the I/O mode parameters.
>
>
> > A priv pointer would be very usefull as I could allocate enough
> > memory for my message structure plus the transfer items and any
> > other thing(s) that I need to store and then set priv to point to
> > my area of memory (like you can for skb's).
>
> Yes, the latest version has spi_message.state, a void *pointer
> for use by whoever currently owns that message.
>
Thank you :)
>
> > > +static inline int
> > > +spi_setup(struct spi_device *spi)
> > > +{
> > > + return spi->master->setup(spi);
> > > +}
> > > +
> >
> > Where would this be used? Surely only the adapter could do this
> > as the SPI device driver and core only knows when it sends the
> > request for a transfer, not when the transfer actually happens.
>
> See above ... that's how the clock speed would be changed, or
> how various other long-lasting SPI protocol tweaks would kick in.
>
> This doesn't _need_ to touch chip registers, though it can.
> It just changes i/o characterics for that specific device.
>
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)?
>
> > > +static inline int
> > > +spi_async(struct spi_device *spi, struct spi_message *message)
> > > +{
> > > + message->dev = spi;
> > > + return spi->master->transfer(spi, message);
> > > +}
> >
> >
> > Couldn't/shouldn't this be in the core, otherwise it looks like
> > you can only do sync transfers (or
> > maybe some comment to say that it's in the header file).
>
> The header file IS part of the core, and that's where that little
> routine is declared (so no need for a comment saying that). The
> headerdefines the interface to the core. Code running in an IRQ
> (or BH) will use spi_async(), while code running in a sleeping
> context could use spi_sync() if it likes.
>
> And spi_sync() is sort-of-core; really it's just a veneer over
> that core async I/O primitive, but one that's so small (and easy
> to use) that it's worth paying the price to have it "everywhere".
>
> (Remember, we're still talking about 2 KBytes ARM object code...)
>
>
> > > +static inline void
> > > +spi_unregister_device(struct spi_device *spi)
> > > +{
> > > + if (spi)
> > > + device_unregister(&spi->dev);
> > > +}
> > > +
> >
> > Couldn't/shouldn't this be in the core, otherwise it looks like
> > you can only register a device and
> > not unregister (or maybe some comment to say that it's in the header file).
>
> That immediately follows the spi_new_device() declaration;
> the device would have been registered using that call.
>
> Really, I don't see much need for either function except to
> handle the sort of "hotplug an SPI adapter" code you were
> talking about. The reason they're inlined there is not
> because they're "not core"; it's because they'll be used
> so infrequenty that nobody else should pay the cost for
> them to exist.
>
OK.
>
> And now stuff from "spi.c":
>
> > > +static int spi_suspend(struct device *dev, pm_message_t message)
> > > +{
> > > + if (dev->driver && dev->driver->suspend)
> > > + return dev->driver->suspend(dev, message, SUSPEND_POWER_DOWN);
> > > + else
> > > + return 0;
> > > +}
>
> Actually those aren't quite right; the dev->power.power_state fields
> need to be updated. Otherwise only sysfs will be doing it.
>
> > > +static int spi_resume(struct device *dev)
> > > +{
> > > + if (dev->driver && dev->driver->resume)
> > > + return dev->driver->resume(dev, RESUME_POWER_ON);
> > > + else
> > > + return 0;
> > > +}
> >
> > What happens about all the devices sitting on the adapter?
>
> That's the suspend routine for those devices. The adapter
> would have a separate suspend routine ...
>
> > Does the driver core suspend them for you? If so could you
> > show me where because I missed it.
>
> Good point. It's arguably a weakness in the driver core.
> Meanwhile, what I've done elsewhere is basically
>
> device_for_each_driver(... fail_if_not_suspended);
>
> The invariant for the spi_master would be that it needs to
> ensure that its children (the spi_device objects) are all
> suspended -- if they have a driver, that is.
>
OK. Thats what I did. I guess the reason that the driver core can't do it is because some busses
may have to do it differently from others.
>
> > > +struct spi_device *__init_or_module
> > > +spi_new_device(struct spi_master *master, struct spi_board_info *chip)
> > > +{
> > > + ...
> > > +
> > > + /* drivers may modify this default i/o setup */
> > > + status = master->setup(proxy);
> >
> > How would this work if two devices work in a different mode?
> >
> > Example:
> > SPI device A works in mode 0 and so the adapter is setup to mode 0.
> > SPI device B works in mode 1 and so the adapter is setup to mode 1.
>
> That's the wrong starting point. It's not the adapter that's set
> to a given mode ... it's the interactions with a given device.
>
> > Device A does a transfer, which it should be done in mode 0, but
> > the transfer is actually done in
> > mode 1 as the last call to setup was for mode 1.
>
> No, device A would never be used in the wrong mode. That's
> a constraint that the spi_master must implement.
>
>
> > Setting up of the mode and clock should only be done in the context
> > of a message (and I mean when a message is transfered, not when it's
> > queued) as then and only then are the settings relevant and
> > you can guaranty that your not interfering with the settings for
> > other devices on the bus.
>
> Not exactly. Think of different kinds of SPI controller:
>
> * Like the PXA SSP. An spi_master for that controller will either
> implement its own chipselects using GPIOs, manually bank-switching
> the registers ... or it won't use chipselects, so it'll never
> need to change the registers. Either way, the register settings
> will be associated with the device, not the controller.
>
> So master->setup() can just update the copy of the registers
> used for that device, and they'll be used to set up the controller
> the next time a transfer to that device is started.
Right, so the answer to one of my questions above is yes, the adapter is expected to store a
'personality' for each device on the bus for adapters that don't support it in hardware.
This does mean that if a SPI driver wants to send messages at different speeds in the callback of
the current message it would have to change the speed for the next message.
>
> * Like the AT91rm9200 SPI. Each chipselect has a dedicated
> register covering mode, clock, and some delays.
>
> So master->setup() can just update the registers directly,
> it won't need to copy them when it starts a transfer, and
> starting a transfer involves fewer register writes.
>
> If the driver for an SPI controller gets the settings wrong, that'd
> be a bug just like reading or writing the wrong data.
>
>
> > > +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?
>
> > > +int __init_or_module // would be __init except for SPI_EXAMPLE
> > > +spi_register_board_info(struct spi_board_info const *info, unsigned n)
> > > +...
> >
> > This function should call scan_boardinfo as there may be devices in this
> > list that sit on adapters that have been registered already.
>
> Not easily. Remember, this is called from the board init code,
> normally in arch_initcall() which is before drivers are expected
> to start registering...
>
>
> > Please can we have a 'undo' version (the general rule being you
> > should be able to undo what you have done ;), i.e.
>
> That rule isn't really followed for board init code though. There's
> no point, since it's not like the board could transmogrify itself!
> The parts registered there can't physically vanish.
>
>
> > spi_unregister_board_info as I might have two different parallel port
> > boards (one with EEPROM and one with Ethernet for example) and I
> > don't want to have to reset my PC to switch between the two.
>
> 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.
>
> Then those devices would automatically vanish (in the latest code)
> when when the adapter calls the spi_unregister_master() routine.
>
>
> > > +int __init_or_module
> > > +spi_register_master(struct device *dev, struct spi_master *master)
> > > +{
> > > + static atomic_t dyn_bus_id = ATOMIC_INIT(0);
> > > + int status = -ENODEV;
> > > +
> > > + if (list_empty(&board_list)) {
> > > + dev_dbg(dev, "spi board info is missing\n");
> > > + goto done;
> > > + }
> >
> > Why is the fact the there is no board information registered at the moment
> > a reason to fail?
> > I thought I could register adapters and board/platform information in any
> > order I wanted.
>
> It's not; I recenty ripped that code out. For your case of a
> parallel port adapter, there would never be one. Only for
> "normal" situations would "nothing declared" be fishy, and
> it's not really worth even a warning.
>
>
> > > +void spi_unregister_master(struct spi_master *master)
> > > +{
> > > +/* REVISIT when do children get deleted? */
> > > + class_device_unregister(&master->cdev);
> > > +
> > > + put_device(master->cdev.dev);
> > > + master->cdev.dev = NULL;
> > > +
> > > +}
> > > +EXPORT_SYMBOL_GPL(spi_unregister_master);
> > > +
> >
> > Does this work? Adding a child device will cause the parent devices
> > ref count to be incremented so
> > surely you have to release all the children first.
>
> I finally revisited that and added the code to unregister the
> children (right there).
>
OK. There is an example of how to do this in my code :)
>
> > > +int spi_sync(struct spi_device *spi, struct spi_message *message)
> > > +{
> > > + DECLARE_COMPLETION(done);
> > > + int status;
> > > +
> > > + message->complete = spi_sync_complete;
> > > + message->context = &done;
> > > + status = spi_async(spi, message);
> > > + if (status == 0)
> > > + wait_for_completion(&done);
> > > + message->context = NULL;
> > > + return status;
> > > +}
> > > +EXPORT_SYMBOL(spi_sync);
> >
> > Why not combine spi_sync and spi_async and just check for a NULL pointer
> > in callback? If the callback/complete pointer is NULL then it's a sync
> > transfer else it's an async transfer.
>
> No, there is only ** one ** way to report completion and that's
> through the callback. All transfers are async at the low level.
> This small wrapper just uses the async notification callback to
> wake up a thread, so that thread has a synchronous model.
>
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 be prone to errors as people
who want an async transfer might forget/not need to set the complete element and would get a sync
transfer instead :(.
>
> > > +/**
> > > + * spi_w8r8 - SPI synchronous 8 bit write followed by 8 bit read
> > > + * @spi: device with which data will be exchanged
> > > + * @cmd: command to be written before data is read back
> > > + *
> > > + * This returns the (unsigned) eight bit number returned by the
> > > + * device, or else a negative error code.
> > > + */
>
> > > +/**
> > > + * spi_w8r16 - SPI synchronous 8 bit write followed by 16 bit read
> > > + * @spi: device with which data will be exchanged
> > > + * @cmd: command to be written before data is read back
> > > + *
> > > + * This returns the (unsigned) sixteen bit number returned by the
> > > + * device, or else a negative error code.
> > > + */
> >
> > Should these live in the core? I know they don't take up much space
> > but if I don't need them why should I have to have them?
> > What about putting these as inline functions in spi.h?
>
> Agreed. The latest version does just that ... but it also has
> a new helper function to call (write X bytes, read Y bytes back)
> to help keep the nonsharable/inlined parts small.
>
>
> > 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?
>
> But if we ever get a version where that's an issue -- or someone
> feels compelled to clean up that little issue, despite the fact that
> doing that creates a performance hit! -- the write_then_read() call
> could get some minor tweaks.
>
>
> > and either do a non DMA transfer or copy the data into a kmalloc'ed
> > area of memory to do the DMA from/to. It would make the adapter drivers
> > life easier if we stipulated that all messages must be kmalloc'ed.
>
> The true requirement is already documented: that all buffers must
> work with dma_{map,unmap}_single(). That's less restrictive than
> saying they've got to come from kmalloc().
>
OK. That's what I meant :)
>
> The "maybe-nice" thing that's not supported there is letting
> drivers provide their own DMA addresses, already mapped. If we
> ever need such a thing, it can be done; but IMO there's not a
> lot of point to it quite yet. Wait until we have the block
> layer handing scatterlists down to some SPI device; then the
> dma_map_sg() stuff will make us want that
I agree.
Mark
>
> - Dave
>
>
> > Mark
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
___________________________________________________________
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
