2005-09-03 10:13:50

by Mark Underwood

[permalink] [raw]
Subject: [RFC][PATCH] SPI subsystem

Here is a SPI framework which tries to use the driver
framework provided by the 2.6 kernel which you can
play around with on any platform, even on your PC :).

This patch only contains a core layer that handles
un/registering of SPI adapters SPI devices and SPI
drivers. It is a work in progress and as yet has no
transfer methods, these will by moved from my old I2C
like core layer in the beginning of next work and I
will send another patch then.
This framework removes all the platform knowledge from
the SPI adapter and all SPI knowledge (chip selects,
which adapter, etc) from the SPI driver.

This patch still doesn’t address a couple of
platform abstraction problems.

1) How to handle SPI devices that require an interrupt
line.
This isn’t a problem on embedded systems as most
of these will have interrupt pins which are connected
directly to the interrupt controller, but what about
PCI cards which will have one interrupt line to the
interrupt controller and a interrupt status register.
In this case the adapter will have to take the
interrupt and workout if it was for it or for the SPI
device. One idea as to have function pointers for
registering, enabling and disabling interrupts in the
adapter structure. The core layer could fill in the
normal functions if an adapter driver doesn’t
fill them in, that way most adapter drivers
don’t have to do any extra work.

2) Extra GPO lines used for reset, etc.
These could be treated like extra CS lines which can
be changed in a spi_msg.

Mark

drivers/Kconfig | 2
drivers/Makefile | 1
drivers/spi/Kconfig | 58 +++
drivers/spi/Makefile | 10
drivers/spi/algos/Kconfig | 19 +
drivers/spi/algos/Makefile | 9
drivers/spi/busses/Kconfig | 17 +
drivers/spi/busses/Makefile | 9
drivers/spi/busses/spi-bus-test.c | 103 ++++++
drivers/spi/chips/Kconfig | 26 +
drivers/spi/chips/Makefile | 12
drivers/spi/chips/spi-bar.c | 75 ++++
drivers/spi/chips/spi-foo.c | 75 ++++
drivers/spi/spi-core.c | 613
++++++++++++++++++++++++++++++++++++++
drivers/spi/test/Kconfig | 19 +
drivers/spi/test/Makefile | 5
drivers/spi/test/my_platform.c | 133 ++++++++
include/linux/spi.h | 259
++++++++++++++++
18 files changed, 1445 insertions(+)





___________________________________________________________
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com


Attachments:
spi-v1.patch (40.97 kB)
509471718-spi-v1.patch

2005-09-09 21:40:05

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> Here is a SPI framework which tries to use the driver
> framework provided by the 2.6 kernel which you can
> play around with on any platform, even on your PC :).

An idea I've used in the refresh of my simpler patch.
More complete examples (often) help. :)


> This patch only contains a core layer that handles
> un/registering of SPI adapters SPI devices and SPI
> drivers.

It's got more than that:

- An "algorithm" layer, an I2C leftover that IMO should be removed.

- A "workqueue/task per driver" assumption. This is interesting, because
it might be easier to port existing drivers into this new framework if
there were always a "free" task context available. But on the other
hand ... not all drivers will need or want a workqueue, and those which
do can always create their own if keventd isn't good enough.

- Bits and pieces of some sort of procfs API, which I'm guessing you
haven't finished removing.

And as for that core:

- The "spi_cs_table" (entry) is very similar to the "spi_board_info" in
my patch, but it has some stuff I expect to live in platform_data.
And it's managed quite differently; it's not something early boot
code would just hand to the SPI core before it vanishes.

- You set global policy on what "platform_data" should hold. That makes it
not be platform-specific data any more!! All that info should be moved
someplace else. (E.g. in my patch, some of it is held by the SPI core.)

- It has code to wrap "struct device_driver". I'd rather it work more
like the platform bus -- no wrapper at all!! That's simpler, and easier
to understand. It eliminates public API functions to manage "struct
spi_driver", as well as that struct itself; and also a bunch of internal
code that's more or less just mirroring what the driver model does.
Fewer parallel concepts means smaller and simpler code all around.

- There's stuff I don't think should be in the device, like a semaphore
(in addition to the driver model one!) and an "spi_driver" pointer
(mirroring driver model dev->driver).

- And there's code to register and unregister a "spi_device". I've been
thinking of that as the responsibility of the "SPI core", and that things
should work like most other bus frameworks on Linux (other than I2C!!).
That is, few folk (basically just subsystem maintainers) ever need to muck
with those particular "innards". (That's the case in the code I sent.)
For better or worse, such code has proven quite error prone.

The I/O layer stub is less minimal than what I sent, but it's very similar
in structure, names, and roles. A few things live in different places.
I see convergence there!

As for I/O fast paths, I was thinking more about "write word then read word"
stuff than bulk transfers, since so many SPI devices seem to use that sort
of protocol convention (including MicroWire hardware). And the message
structure that's there now is prety bulk-oriented ... I modeled it on one
from a DataFlash driver, not a sensor driver.



> This framework removes all the platform knowledge from
> the SPI adapter and all SPI knowledge (chip selects,
> which adapter, etc) from the SPI driver.

It does that in a somewhat different way than mine did, and doesn't
really decouple board descriptions (early boot stuff) from the
driver load sequence (late boot, or even managed by userspace).


> This patch still doesn’t address a couple of
> platform abstraction problems.
>
> 1) How to handle SPI devices that require an interrupt
> line. This isn’t a problem on embedded systems as most
> of these will have interrupt pins which are connected
> directly to the interrupt controller,

As noted in comments, I expect IRQs to be passed with the similar sorts
of board-specific platform_data, as one more "int" (likely it's a GPIO
number, though the numbers request_irq takes can be useful too).

I considered passing a "struct resource" with the board description data,
but decided against that ... "int" is smaller, cheaper, more obvious.



> but what about
> PCI cards which will have one interrupt line to the
> interrupt controller and a interrupt status register.
> In this case the adapter will have to take the
> interrupt and workout if it was for it or for the SPI
> device.

Are you implying the standard interrupt framework would NOT work here?
It already handles shared IRQs, whether for PCI or GPIO or whatever.
(Or are you getting at something else?)


> One idea as to have function pointers for
> registering, enabling and disabling interrupts in the
> adapter structure. The core layer could fill in the
> normal functions if an adapter driver doesn’t
> fill them in, that way most adapter drivers
> don’t have to do any extra work.

The normal thing is just to pass an IRQ number into the driver (maybe
through platform_data) which then gets passed to request_irq() as usual.

The place that falls down is when that IRQ source hasn't been merged
into the platform IRQ framework ... for example, the IRQ is reported
indirectly, through some I2C based GPIO expander. Those cases are
not usually mainstream. So I'm happy to leave them as platform-specific
hacks for a very long time, rather than write code that handles the
normal clean request_irq() path AND something less clean. ;)


> 2) Extra GPO lines used for reset, etc.
> These could be treated like extra CS lines which can
> be changed in a spi_msg.

Same as with IRQs: just pass them in with the chip's platform_data.

What's problematic is the fact that all GPIO code is platform specific.
What works on PXA250 won't work on x86 or OMAP, and it may not even work
on PXA270. We'll still have limits to how portable drivers can be.

- Dave

2005-09-10 10:55:40

by Vitaly Wool

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

Mark Underwood wrote:

<snip>

>+static int spi_test_probe(struct device *dev)
>+{
>+ struct spi_adapter *sadap;
>+ struct platform_device *pdev = to_platform_device(dev);
>+ int stat;
>+
>+ printk("spi_test_probe\n");
>+
>+ sadap = kmalloc(sizeof(struct spi_adapter), GFP_KERNEL);
>+ if(!sadap)
>+ {
>+ return -ENOMEM;
>+ }
>+ memset(sadap, 0, sizeof(struct spi_adapter));
>+
>+ sadap->spi_adap_cs = NULL;
>+ sadap->cs_table = ((struct spi_platform_data*) (pdev->dev.platform_data))->cs_table;
>+ sadap->max_cs = ((struct spi_platform_data*) (pdev->dev.platform_data))->max_cs;
>
>
What if pdev->dev.platform_data is NULL?

<snip>

>diff -uprN -X dontdiff linux-2.6.10.orig/drivers/spi/chips/spi-bar.c linux-2.6.10/drivers/spi/chips/spi-bar.c
>--- linux-2.6.10.orig/drivers/spi/chips/spi-bar.c 1970-01-01 01:00:00.000000000 +0100
>+++ linux-2.6.10/drivers/spi/chips/spi-bar.c 2005-09-02 17:29:16.000000000 +0100
>
>
I'm afraid the example provided can not give any idea on the intended usage.

>diff -uprN -X dontdiff linux-2.6.10.orig/drivers/spi/spi-core.c linux-2.6.10/drivers/spi/spi-core.c
>--- linux-2.6.10.orig/drivers/spi/spi-core.c 1970-01-01 01:00:00.000000000 +0100
>+++ linux-2.6.10/drivers/spi/spi-core.c 2005-09-02 17:29:50.000000000 +0100
>@@ -0,0 +1,613 @@
>+/*
>+ * linux/driver/spi/spi-core.c - The spi subsystem core layer
>+ *
>+ * Copyright (C) 2005 Philips Semicondutors, All Rights Reserved.
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License version 2 as
>+ * published by the Free Software Foundation.
>+ *
>+ * Authors:
>+ * Mark Underwood
>+ */
>+
>+#include <linux/device.h>
>+#include <linux/module.h>
>+#include <linux/init.h>
>+#include <linux/err.h>
>+#include <linux/spi.h>
>+#include <linux/idr.h>
>+
>+#define SPI_CORE_NONE 0
>+#define SPI_CORE_BUS 1
>+#define SPI_CORE_DRIVER 2
>+#define SPI_CORE_CLASS 3
>+#define SPI_CORE_DONE 4
>+
>+static DEFINE_IDR(spi_adapter_idr);
>+
>+void __spi_device_unregister(struct spi_device * sdev);
>+int __spi_device_register(struct spi_device * sdev);
>
>
Shouldn't those be static?

<snip>

>+
>+ memset(new_device,0,sizeof(struct spi_device));
>
>
Spaces after semicolon please.

<snip>

>+fail1:
>+ for (j=0;j<(i-1);j++)
>
>
Spaces again. How about using lindent?

<snip>

>+ flush_workqueue(adap->work_queue);
>
>
Indentation/tabs.

<snip>

>+static int spi_suspend(struct device * dev, u32 state)
>+{
>+ struct device *child;
>+ int ret = 0;
>+
>+ /* First suspend all the children */
>+ list_for_each_entry(child, &dev->children, node) {
>+ if (child->driver && child->driver->suspend) {
>+ ret = child->driver->suspend(child, state, SUSPEND_DISABLE);
>+ if (ret == 0)
>+ ret = child->driver->suspend(child, state, SUSPEND_SAVE_STATE);
>+ if (ret == 0)
>+ ret = child->driver->suspend(child, state, SUSPEND_POWER_DOWN);
>+ }
>+ }
>
>
Oh my God. It will be called 3 times for each child entry, isn't it?!

>+
>+ if (ret)
>+ return ret;
>+
>+ /* Then the adapter */
>+ if (dev->driver && dev->driver->suspend) {
>+ ret = dev->driver->suspend(dev, state, SUSPEND_DISABLE);
>+ if (ret == 0)
>+ ret = dev->driver->suspend(dev, state, SUSPEND_SAVE_STATE);
>+ if (ret == 0)
>+ ret = dev->driver->suspend(dev, state, SUSPEND_POWER_DOWN);
>+ }
>+
>+ return ret;
>+}
>
>
Can you please clarify what is 'adapter' here: is it a bus or what?

>+
>+static int spi_resume(struct device * dev)
>+{
>+ int ret = 0;
>+ struct device *child;
>+
>+ /* First resume the adapter */
>+ if (dev->driver && dev->driver->resume) {
>+ ret = dev->driver->resume(dev, RESUME_POWER_ON);
>+ if (ret == 0)
>+ ret = dev->driver->resume(dev, RESUME_RESTORE_STATE);
>+ if (ret == 0)
>+ ret = dev->driver->resume(dev, RESUME_ENABLE);
>+ }
>+
>+ if (ret)
>+ return ret;
>+
>+ /* Then all the children */
>+ list_for_each_entry(child, &dev->children, node) {
>+ if (child->driver && child->driver->resume) {
>+ ret = child->driver->resume(child, RESUME_POWER_ON);
>+ if (ret == 0)
>+ ret = child->driver->resume(child, RESUME_RESTORE_STATE);
>+ if (ret == 0)
>+ ret = child->driver->resume(child, RESUME_ENABLE);
>+ if (ret)
>+ break;
>+ }
>+ }
>+
>
>
Same as for suspend.

And the basic idea anyway looks wrong and not LDM'ish.
What if your driver

+static struct device_driver spi_adapter_driver = {
+ .name = "spi_adapter",
+ .bus = &spi_bus_type,
+ .probe = spi_adapter_probe,
+ .remove = spi_adapter_remove,
+};

presents also suspend/resume functions (what it should have done anyway).

Won't it be in a clash with your suspend/resume technique?

Also:

Can you please specify what is the difference between 'bus' and 'chip'
in your model?
It's not clear to me how the following situation is handled. Suppose you
have two SPI 'busses' with same devices (for instance, 2 SD card
adapters) attached to different busses.
The work queues approach is also not quite clear.

But the main thing that seems to be not thought about at all is power
management for the SPI busses/devices.
I'm afraid your approach needs serious rework in this area.

Best regards,
Vitaly

2005-09-10 11:17:13

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> wrote:

> > Here is a SPI framework which tries to use the
> driver
> > framework provided by the 2.6 kernel which you can
> > play around with on any platform, even on your PC
> :).
>
> An idea I've used in the refresh of my simpler
> patch.
> More complete examples (often) help. :)
>
>
> > This patch only contains a core layer that handles
> > un/registering of SPI adapters SPI devices and SPI
> > drivers.
>
> It's got more than that:
>
> - An "algorithm" layer, an I2C leftover that IMO
> should be removed.

Will be removed in the next patch set.

>
> - A "workqueue/task per driver" assumption. This
> is interesting, because
> it might be easier to port existing drivers into
> this new framework if
> there were always a "free" task context
> available. But on the other
> hand ... not all drivers will need or want a
> workqueue, and those which
> do can always create their own if keventd isn't
> good enough.



Without the transfer routines I know it is hard to see
why I have used a workqueue so let me explain.
The workqueue is used as a scheduler for the adapter.
What happens is that a SPI device driver calls
transfer which creates a work item on the workqueue to
do the transfer. An adapter is given one SPI message
to process at a time and when it has finished it will
notify the core and the core will execute the next
item on the work list (the next transfer). You have to
have such a mechanism as you may have several SPI
devices on an adapter doing asynchronous IO at the
same time (i.e. giving an adapter that is in the
middle of doing work more work).
Hopefully its use will become clearer in the next
patch set I send.

>
> - Bits and pieces of some sort of procfs API,
> which I'm guessing you
> haven't finished removing.

Yep, still removing I2C stuff!

>
> And as for that core:
>
> - The "spi_cs_table" (entry) is very similar to
> the "spi_board_info" in
> my patch, but it has some stuff I expect to live
> in platform_data.
> And it's managed quite differently; it's not
> something early boot
> code would just hand to the SPI core before it
> vanishes.
>
> - You set global policy on what "platform_data"
> should hold. That makes it
> not be platform-specific data any more!! All
> that info should be moved
> someplace else. (E.g. in my patch, some of it
> is held by the SPI core.)

No, but don't the serial port drivers do that with the
serial_struct structure?
Anyway surely what SPI devices are on an adapter is
platform data! Why give the core work more to do?

>
> - It has code to wrap "struct device_driver". I'd
> rather it work more
> like the platform bus -- no wrapper at all!!
> That's simpler, and easier
> to understand. It eliminates public API
> functions to manage "struct
> spi_driver", as well as that struct itself; and
> also a bunch of internal
> code that's more or less just mirroring what the
> driver model does.
> Fewer parallel concepts means smaller and
> simpler code all around.

Yes. I was thinking the same thing (as you can see by
the comments in spi.h). I have now removed the
spi_driver structure. Yay, less code!

>
> - There's stuff I don't think should be in the
> device, like a semaphore
> (in addition to the driver model one!) and an
> "spi_driver" pointer
> (mirroring driver model dev->driver).

As the comment says, it is used to sleep on for
blocking transfers, it's not a structure lock.

>
> - And there's code to register and unregister a
> "spi_device". I've been
> thinking of that as the responsibility of the
> "SPI core", and that things
> should work like most other bus frameworks on
> Linux (other than I2C!!).
> That is, few folk (basically just subsystem
> maintainers) ever need to muck
> with those particular "innards". (That's the
> case in the code I sent.)
> For better or worse, such code has proven quite
> error prone.

But the idea is that it's the same style as the
platform bus in which you register and unregister a
platform device. How else is the core going to know
about the device? My idea is that most people will
pass in a cs_table which has the SPI devices on that
adapter in it, but as you have pointed out this will
not always be what people want (for more hotplug'ish
devices) in which case they register those SPI devices
in the same manor as platform devices (I need to do a
bit work more work on this).

>
> The I/O layer stub is less minimal than what I sent,
> but it's very similar
> in structure, names, and roles. A few things live
> in different places.
> I see convergence there!

Good :). Although that patch didn't have the transfer
method in it :(.

>
> As for I/O fast paths, I was thinking more about
> "write word then read word"
> stuff than bulk transfers, since so many SPI devices
> seem to use that sort
> of protocol convention (including MicroWire
> hardware). And the message
> structure that's there now is prety bulk-oriented
> ... I modeled it on one
> from a DataFlash driver, not a sensor driver.
>

The problem is that the "write word then read word"
model is not universal and the msg structure is. We
have a SPI network device which requires both sort
messages (reading status) and large packets for data
and firmware download (upto 32K chunks). You may well
have a SPI network device on the same bus as an eeprom
(which we do) and so both have to be handled in the
same way.

>
>
> > This framework removes all the platform knowledge
> from
> > the SPI adapter and all SPI knowledge (chip
> selects,
> > which adapter, etc) from the SPI driver.
>
> It does that in a somewhat different way than mine
> did, and doesn't
> really decouple board descriptions (early boot
> stuff) from the
> driver load sequence (late boot, or even managed by
> userspace).
>

It gives you the option to decouple board descriptions
form the adapter declaration if that's what you mean.
SPI devices and drivers can be loaded in any order.


>
> > This patch still doesn&#8217;t address a couple of
> > platform abstraction problems.
> >
> > 1) How to handle SPI devices that require an
> interrupt
> > line. This isn&#8217;t a problem on embedded
> systems as most
> > of these will have interrupt pins which are
> connected
> > directly to the interrupt controller,
>
> As noted in comments, I expect IRQs to be passed
> with the similar sorts
> of board-specific platform_data, as one more "int"
> (likely it's a GPIO
> number, though the numbers request_irq takes can be
> useful too).
>
> I considered passing a "struct resource" with the
> board description data,
> but decided against that ... "int" is smaller,
> cheaper, more obvious.
>
>
>
> > but what about
> > PCI cards which will have one interrupt line to
> the
> > interrupt controller and a interrupt status
> register.
> > In this case the adapter will have to take the
> > interrupt and workout if it was for it or for the
> SPI
> > device.
>
> Are you implying the standard interrupt framework
> would NOT work here?

Yes.

> It already handles shared IRQs, whether for PCI or
> GPIO or whatever.
> (Or are you getting at something else?)
>

But if a SPI device generates an interrupt on a PCI
card which has only one interrupt line, how does the
SPI device enable/disable/clear that interrupt given
that the register is in the PCI device and the SPI
device has no knowledge of that?

>
> > One idea as to have function pointers for
> > registering, enabling and disabling interrupts in
> the
> > adapter structure. The core layer could fill in
> the
> > normal functions if an adapter driver
> doesn&#8217;t
> > fill them in, that way most adapter drivers
> > don&#8217;t have to do any extra work.
>
> The normal thing is just to pass an IRQ number into
> the driver (maybe
> through platform_data) which then gets passed to
> request_irq() as usual.
>
> The place that falls down is when that IRQ source
> hasn't been merged
> into the platform IRQ framework ... for example, the
> IRQ is reported
> indirectly, through some I2C based GPIO expander.
> Those cases are
> not usually mainstream. So I'm happy to leave them
> as platform-specific
> hacks for a very long time, rather than write code
> that handles the
> normal clean request_irq() path AND something less
> clean. ;)

But if you can do it easily why not :)?

>
>
> > 2) Extra GPO lines used for reset, etc.
> > These could be treated like extra CS lines which
> can
> > be changed in a spi_msg.
>
> Same as with IRQs: just pass them in with the chip's
> platform_data.
>
> What's problematic is the fact that all GPIO code is
> platform specific.
> What works on PXA250 won't work on x86 or OMAP, and
> it may not even work
> on PXA270. We'll still have limits to how portable
> drivers can be.
>

Not if you treat GPO pins as chip selects and let the
adapter abstract them for you :).

Mark

> - Dave
>
> -
> To unsubscribe from this list: send the line
>"unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at
>http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/



___________________________________________________________
How much free photo storage do you get? Store your holiday
snaps for FREE with Yahoo! Photos http://uk.photos.yahoo.com

2005-09-10 11:54:51

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- Vital <[email protected]> wrote:

> Mark Underwood wrote:
>
> <snip>
>
> >+static int spi_test_probe(struct device *dev)
> >+{
> >+ struct spi_adapter *sadap;
> >+ struct platform_device *pdev =
> to_platform_device(dev);
> >+ int stat;
> >+
> >+ printk("spi_test_probe\n");
> >+
> >+ sadap = kmalloc(sizeof(struct spi_adapter),
> GFP_KERNEL);
> >+ if(!sadap)
> >+ {
> >+ return -ENOMEM;
> >+ }
> >+ memset(sadap, 0, sizeof(struct spi_adapter));
> >+
> >+ sadap->spi_adap_cs = NULL;
> >+ sadap->cs_table = ((struct spi_platform_data*)
> (pdev->dev.platform_data))->cs_table;
> >+ sadap->max_cs = ((struct spi_platform_data*)
> (pdev->dev.platform_data))->max_cs;
> >
> >
> What if pdev->dev.platform_data is NULL?

Thanks for pointing this one out:).

>
> <snip>
>
> >diff -uprN -X dontdiff
> linux-2.6.10.orig/drivers/spi/chips/spi-bar.c
> linux-2.6.10/drivers/spi/chips/spi-bar.c
> >--- linux-2.6.10.orig/drivers/spi/chips/spi-bar.c
> 1970-01-01 01:00:00.000000000 +0100
> >+++ linux-2.6.10/drivers/spi/chips/spi-bar.c
> 2005-09-02 17:29:16.000000000 +0100
> >
> >
> I'm afraid the example provided can not give any
> idea on the intended usage.

I know! I said that this only shows the registration &
unregistration of SPI devices and adapters, the
transfer routines will be coming next!

>
> >diff -uprN -X dontdiff
> linux-2.6.10.orig/drivers/spi/spi-core.c
> linux-2.6.10/drivers/spi/spi-core.c
> >--- linux-2.6.10.orig/drivers/spi/spi-core.c
> 1970-01-01 01:00:00.000000000 +0100
> >+++ linux-2.6.10/drivers/spi/spi-core.c 2005-09-02
> 17:29:50.000000000 +0100
> >@@ -0,0 +1,613 @@
> >+/*
> >+ * linux/driver/spi/spi-core.c - The spi
> subsystem core layer
> >+ *
> >+ * Copyright (C) 2005 Philips Semicondutors, All
> Rights Reserved.
> >+ *
> >+ * This program is free software; you can
> redistribute it and/or modify
> >+ * it under the terms of the GNU General Public
> License version 2 as
> >+ * published by the Free Software Foundation.
> >+ *
> >+ * Authors:
> >+ * Mark Underwood
> >+ */
> >+
> >+#include <linux/device.h>
> >+#include <linux/module.h>
> >+#include <linux/init.h>
> >+#include <linux/err.h>
> >+#include <linux/spi.h>
> >+#include <linux/idr.h>
> >+
> >+#define SPI_CORE_NONE 0
> >+#define SPI_CORE_BUS 1
> >+#define SPI_CORE_DRIVER 2
> >+#define SPI_CORE_CLASS 3
> >+#define SPI_CORE_DONE 4
> >+
> >+static DEFINE_IDR(spi_adapter_idr);
> >+
> >+void __spi_device_unregister(struct spi_device *
> sdev);
> >+int __spi_device_register(struct spi_device *
> sdev);
> >
> >
> Shouldn't those be static?

Yep.

>
> <snip>
>
> >+
> >+ memset(new_device,0,sizeof(struct spi_device));
> >
> >
> Spaces after semicolon please.
>
> <snip>
>
> >+fail1:
> >+ for (j=0;j<(i-1);j++)
> >
> >
> Spaces again. How about using lindent?

Cheers, this is my first patch posted on the LKML, I
still don't know about all the tools.

>
> <snip>
>
> >+ flush_workqueue(adap->work_queue);
> >
> >
> Indentation/tabs.
>
> <snip>
>
> >+static int spi_suspend(struct device * dev, u32
> state)
> >+{
> >+ struct device *child;
> >+ int ret = 0;
> >+
> >+ /* First suspend all the children */
> >+ list_for_each_entry(child, &dev->children, node)
> {
> >+ if (child->driver && child->driver->suspend) {
> >+ ret = child->driver->suspend(child, state,
> SUSPEND_DISABLE);
> >+ if (ret == 0)
> >+ ret = child->driver->suspend(child, state,
> SUSPEND_SAVE_STATE);
> >+ if (ret == 0)
> >+ ret = child->driver->suspend(child, state,
> SUSPEND_POWER_DOWN);
> >+ }
> >+ }
> >
> >
> Oh my God. It will be called 3 times for each child
> entry, isn't it?!

OK. This stuff is probably wrong. I used the platform
subsystem as an example. I need to do more research
into how much work the driver core does for you.

>
> >+
> >+ if (ret)
> >+ return ret;
> >+
> >+ /* Then the adapter */
> >+ if (dev->driver && dev->driver->suspend) {
> >+ ret = dev->driver->suspend(dev, state,
> SUSPEND_DISABLE);
> >+ if (ret == 0)
> >+ ret = dev->driver->suspend(dev, state,
> SUSPEND_SAVE_STATE);
> >+ if (ret == 0)
> >+ ret = dev->driver->suspend(dev, state,
> SUSPEND_POWER_DOWN);
> >+ }
> >+
> >+ return ret;
> >+}
> >
> >
> Can you please clarify what is 'adapter' here: is it
> a bus or what?

Yes, it is a bus master and/or slave.

>
> >+
> >+static int spi_resume(struct device * dev)
> >+{
> >+ int ret = 0;
> >+ struct device *child;
> >+
> >+ /* First resume the adapter */
> >+ if (dev->driver && dev->driver->resume) {
> >+ ret = dev->driver->resume(dev, RESUME_POWER_ON);
> >+ if (ret == 0)
> >+ ret = dev->driver->resume(dev,
> RESUME_RESTORE_STATE);
> >+ if (ret == 0)
> >+ ret = dev->driver->resume(dev, RESUME_ENABLE);
> >+ }
> >+
> >+ if (ret)
> >+ return ret;
> >+
> >+ /* Then all the children */
> >+ list_for_each_entry(child, &dev->children, node)
> {
> >+ if (child->driver && child->driver->resume) {
> >+ ret = child->driver->resume(child,
> RESUME_POWER_ON);
> >+ if (ret == 0)
> >+ ret = child->driver->resume(child,
> RESUME_RESTORE_STATE);
> >+ if (ret == 0)
> >+ ret = child->driver->resume(child,
> RESUME_ENABLE);
> >+ if (ret)
> >+ break;
> >+ }
> >+ }
> >+
> >
> >
> Same as for suspend.
>
> And the basic idea anyway looks wrong and not
> LDM'ish.
> What if your driver
>
> +static struct device_driver spi_adapter_driver = {
> + .name = "spi_adapter",
> + .bus = &spi_bus_type,
> + .probe = spi_adapter_probe,
> + .remove = spi_adapter_remove,
> +};
>
> presents also suspend/resume functions (what it
> should have done anyway).
>
> Won't it be in a clash with your suspend/resume
> technique?

Probably as I said above I don't know enough about
this area yet. Maybe you could help me to do this
correctly?

>
> Also:
>
> Can you please specify what is the difference
> between 'bus' and 'chip'
> in your model?

My current terminology is:

spi adapter: A device that sits on an bus (platform,
PCI, USB etc) and is a SPI master and/or slave (I
haven't done any work on the slave part yet).

spi device: A SPI device (e.g. a SPI eeprom) which one
or more of are connected to a spi adapter.

> It's not clear to me how the following situation is
> handled. Suppose you
> have two SPI 'busses' with same devices (for
> instance, 2 SD card
> adapters) attached to different busses.

I don't understand your question here. You would have
two instances of a SD card adapter. In sysfs it would
look like:

SD card 0
/sys/devices/platform/spi-0/0-0000

SD card 1
/sys/devices/platform/spi-1/1-0002

As far as the SD card driver is concernedit doesn't
need to know which one its driving or even how many of
them they are. The big advantage of using the LDM.

> The work queues approach is also not quite clear.

This will become clearer in the next patch set which
will show how to do transfers.

>
> But the main thing that seems to be not thought
> about at all is power
> management for the SPI busses/devices.
> I'm afraid your approach needs serious rework in
> this area.

I know, any help would be great :). At the moment I'm
trying to get the registration and transfer model
sorted and then I'll have a look at PM.

Mark

>
> Best regards,
> Vitaly
>
> -
> 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/
>






___________________________________________________________
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com

2005-09-10 21:20:29

by Vitaly Wool

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

Mark Underwood wrote:

>>Same as for suspend.
>>
>>And the basic idea anyway looks wrong and not
>>LDM'ish.
>>What if your driver
>>
>>+static struct device_driver spi_adapter_driver = {
>>+ .name = "spi_adapter",
>>+ .bus = &spi_bus_type,
>>+ .probe = spi_adapter_probe,
>>+ .remove = spi_adapter_remove,
>>+};
>>
>>presents also suspend/resume functions (what it
>>should have done anyway).
>>
>>Won't it be in a clash with your suspend/resume
>>technique?
>>
>>
>
>Probably as I said above I don't know enough about
>this area yet. Maybe you could help me to do this
>correctly?
>
>
>
I guess that it's basically wrong to use platform_device here. My POV is
that a specific spi_device structure should be introduced here, just
like pcidevice, for instance.

>>Also:
>>
>>Can you please specify what is the difference
>>between 'bus' and 'chip'
>>in your model?
>>
>>
>
>My current terminology is:
>
>spi adapter: A device that sits on an bus (platform,
>PCI, USB etc) and is a SPI master and/or slave (I
>haven't done any work on the slave part yet).
>
>spi device: A SPI device (e.g. a SPI eeprom) which one
>or more of are connected to a spi adapter.
>
>
>
>>It's not clear to me how the following situation is
>>handled. Suppose you
>>have two SPI 'busses' with same devices (for
>>instance, 2 SD card
>>adapters) attached to different busses.
>>
>>
>
>I don't understand your question here. You would have
>two instances of a SD card adapter. In sysfs it would
>look like:
>
>SD card 0
>/sys/devices/platform/spi-0/0-0000
>
>SD card 1
>/sys/devices/platform/spi-1/1-0002
>
>As far as the SD card driver is concernedit doesn't
>need to know which one its driving or even how many of
>them they are. The big advantage of using the LDM.
>
>
Not sure if I get you right.
If we suspend bus 0, how does the SD driver get aware of that?

Vitaly

2005-09-11 08:36:06

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- Vital <[email protected]> wrote:

> Mark Underwood wrote:
>
> >>Same as for suspend.
> >>
> >>And the basic idea anyway looks wrong and not
> >>LDM'ish.
> >>What if your driver
> >>
> >>+static struct device_driver spi_adapter_driver =
> {
> >>+ .name = "spi_adapter",
> >>+ .bus = &spi_bus_type,
> >>+ .probe = spi_adapter_probe,
> >>+ .remove = spi_adapter_remove,
> >>+};
> >>
> >>presents also suspend/resume functions (what it
> >>should have done anyway).
> >>
> >>Won't it be in a clash with your suspend/resume
> >>technique?
> >>
> >>
> >
> >Probably as I said above I don't know enough about
> >this area yet. Maybe you could help me to do this
> >correctly?
> >
> >
> >
> I guess that it's basically wrong to use
> platform_device here. My POV is
> that a specific spi_device structure should be
> introduced here, just
> like pcidevice, for instance.

If you look at spi.h you will see the spi_device
structure :). My comment about suspend/resume was that
I based my suspend/resume functions on those in
platform.c.

>
> >>Also:
> >>
> >>Can you please specify what is the difference
> >>between 'bus' and 'chip'
> >>in your model?
> >>
> >>
> >
> >My current terminology is:
> >
> >spi adapter: A device that sits on an bus
> (platform,
> >PCI, USB etc) and is a SPI master and/or slave (I
> >haven't done any work on the slave part yet).
> >
> >spi device: A SPI device (e.g. a SPI eeprom) which
> one
> >or more of are connected to a spi adapter.
> >
> >
> >
> >>It's not clear to me how the following situation
> is
> >>handled. Suppose you
> >>have two SPI 'busses' with same devices (for
> >>instance, 2 SD card
> >>adapters) attached to different busses.
> >>
> >>
> >
> >I don't understand your question here. You would
> have
> >two instances of a SD card adapter. In sysfs it
> would
> >look like:
> >
> >SD card 0
> >/sys/devices/platform/spi-0/0-0000
> >
> >SD card 1
> >/sys/devices/platform/spi-1/1-0002
> >
> >As far as the SD card driver is concernedit doesn't
> >need to know which one its driving or even how many
> of
> >them they are. The big advantage of using the LDM.
> >
> >
> Not sure if I get you right.
> If we suspend bus 0, how does the SD driver get
> aware of that?

That is what my suspend/resume functions are doing
(maybe incorrectly). My current understanding is that
it is the job of the bus layer (e.g. PCI, USB and in
this case SPI) to handle the fact that a bus has been
suspended or resumed. This means that when an adapter
gets a suspend/resume call the core layer must then
ask all the devices on that adapter to suspend/resume.
In platform.c there are 3 calls made to the suspend
resume function which I assumed was the standard why
of doing this as some devices might have to be called
3 times inorder to be suspended/resumed correctly.
I can see that you could have a situation where a SPI
device has been suspended, and then the bus gets
suspended and then resumed, in which case I guess that
SPI device should probably still be suspended but at
the moment would get resumed.

Mark

>
> Vitaly
>




___________________________________________________________
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com

2005-09-15 00:08:29

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> Date: Sat, 10 Sep 2005 12:17:06 +0100 (BST)
> From: Mark Underwood <[email protected]>
>
> >
> > - A "workqueue/task per driver" assumption. This
> > is interesting, because
> > it might be easier to port existing drivers into
> > this new framework if
> > there were always a "free" task context
> > available. But on the other
> > hand ... not all drivers will need or want a
> > workqueue, and those which
> > do can always create their own if keventd isn't
> > good enough.
>
>
>
> Without the transfer routines I know it is hard to see
> why I have used a workqueue so let me explain.
> The workqueue is used as a scheduler for the adapter.
> What happens is that a SPI device driver calls
> transfer which creates a work item on the workqueue to
> do the transfer. ...

In short, it's an artifact of how some controller drivers are
implemented. So it looks even more as if the right thing to do
there is to NOT require all controller drivers to do that.

Plus, since you've made it public, there's no way to ensure
that nobody else uses that work queue. You're not following
the basic "information hiding" rule for interface design.


> You have to
> have such a mechanism as you may have several SPI
> devices on an adapter doing asynchronous IO at the
> same time (i.e. giving an adapter that is in the
> middle of doing work more work).

No, you do not "have" to have such a mechanism. You can look at the USB
stack for a counterexample. Both host and peripheral/gadget side adapter
drivers manage request queues just fine without requiring such a task.
And I assure you the queueing structure inside any USB Host Controller is
orders of magnitude more complex than any SPI adapter needs.


> > - You set global policy on what "platform_data"
> > should hold. That makes it
> > not be platform-specific data any more!! All
> > that info should be moved
> > someplace else. (E.g. in my patch, some of it
> > is held by the SPI core.)
>
> No, but don't the serial port drivers do that with the
> serial_struct structure?
> Anyway surely what SPI devices are on an adapter is
> platform data! Why give the core work more to do?

Well, I've repeatedly given examples of cases where that is the
wrong model ... where the devices are associated with an expansion
card, so any reliance on platform data (that's set up by and coupled
to the baseboard) causes lots of problems.


> > - There's stuff I don't think should be in the
> > device, like a semaphore
> > (in addition to the driver model one!) and an
> > "spi_driver" pointer
> > (mirroring driver model dev->driver).
>
> As the comment says, it is used to sleep on for
> blocking transfers, it's not a structure lock.

The traditional way to do this is more like what I
did in the refresh of my patch:

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

The first of those shows how to do it without that sort
of pre-allocation; see the synchronous calls.


> > - And there's code to register and unregister a
> > "spi_device". I've been
> > thinking of that as the responsibility of the
> > "SPI core", and that things
> > should work like most other bus frameworks on
> > Linux (other than I2C!!).
> > That is, few folk (basically just subsystem
> > maintainers) ever need to muck
> > with those particular "innards". (That's the
> > case in the code I sent.)
> > For better or worse, such code has proven quite
> > error prone.
>
> But the idea is that it's the same style as the
> platform bus in which you register and unregister a
> platform device.

For SPI, there needs to be a layer between the platform device and the
SPI devices. That's the sort of code that I was pointing out tends to
be error prone.


> How else is the core going to know about the device?

See the patches above; I've explained that before.


> My idea is that most people will
> pass in a cs_table which has the SPI devices on that
> adapter in it, but as you have pointed out this will
> not always be what people want (for more hotplug'ish
> devices) in which case they register those SPI devices
> in the same manor as platform devices (I need to do a
> bit work more work on this).

I've already done this work. You could just reuse my code. :)


> > The I/O layer stub is less minimal than what I sent,
> > but it's very similar
> > in structure, names, and roles. A few things live
> > in different places.
> > I see convergence there!
>
> Good :). Although that patch didn't have the transfer
> method in it :(.

I'm not sure how to parse that. I remember seeing a transfer
method of some kind; otherwise I'd not have been able to
make such a comment. (And the "different places" seemed
like a real issue ... per-request flags in the device,
per-device flags in the request, etc.)


> > As for I/O fast paths, I was thinking more about
> > "write word then read word"
> > stuff than bulk transfers, since so many SPI devices
> > seem to use that sort
> > of protocol convention (including MicroWire
> > hardware). And the message
> > structure that's there now is prety bulk-oriented
> > ... I modeled it on one
> > from a DataFlash driver, not a sensor driver.
> >
>
> The problem is that the "write word then read word"
> model is not universal and the msg structure is.

You can express "write word then read word" in terms of those messages;
necessarily so. The controller method accepting a message structure
would be be a "must implement" (otherwise the rest couldn't just be
optional "fast path" support).

OR ... for the very common case that the hardware supports such things
directly, you could also add some fast paths. So folk wanting quick
audio control, or sensor access, could get that. (Unless the controller
were busy with some other request, in which case the request would need
to get queued up as a "message". Not a fastpath.)


> > > but what about
> > > PCI cards which will have one interrupt line to
> > > the interrupt controller and a interrupt status
> > > register.
> > > In this case the adapter will have to take the
> > > interrupt and workout if it was for it or for the
> > > SPI device.
> >
> > Are you implying the standard interrupt framework
> > would NOT work here?
>
> Yes.

You should maybe explain why it fails, in some other thread.
I assure you the IRQ framework will not be changed just to
make one exotic SPI implementation happier.


> > It already handles shared IRQs, whether for PCI or
> > GPIO or whatever.
> > (Or are you getting at something else?)
>
> But if a SPI device generates an interrupt on a PCI
> card which has only one interrupt line, how does the
> SPI device enable/disable/clear that interrupt given
> that the register is in the PCI device and the SPI
> device has no knowledge of that?

This sounds like exactly the same problem lots of device drivers
have: they may have one PCI interrupt, but internally the device
has its own IRQ status, enable, and disable masks. Then they do
the dispatch to different parts of the driver internally, based
on the status which triggered the IRQ.

SPI over PCI is reasonably exotic. Seems to me as if the best way
to solve that kind of problem would involve specialized SPI-over-PCI
code, matching that one board. Don't force every SPI implementations
to cope with the quirks of that one PCI card, especially when most
SPI hardware tres to be dead simple, to save costs.


> > > 2) Extra GPO lines used for reset, etc.
> > > These could be treated like extra CS lines which
> > > can be changed in a spi_msg.
> >
> > Same as with IRQs: just pass them in with the chip's
> > platform_data.
> >
> > What's problematic is the fact that all GPIO code is
> > platform specific.
> > What works on PXA250 won't work on x86 or OMAP, and
> > it may not even work
> > on PXA270. We'll still have limits to how portable
> > drivers can be.
> >
>
> Not if you treat GPO pins as chip selects and let the
> adapter abstract them for you :).

That assumes the GPIO pin is being used as a chip select,
not as an input or output. :)

- Dave

2005-09-15 00:14:48

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> > >+static int spi_suspend(struct device * dev, u32 state)
> > >+{
> > >...
> > >+ list_for_each_entry(child, &dev->children, node) {

That should probably use device_for_each_child() if you're
going to do it that way ...


> > >+ if (child->driver && child->driver->suspend) {
> > >+ ret = child->driver->suspend(child, state, SUSPEND_DISABLE);
> > >+ if (ret == 0)
> > >+ ret = child->driver->suspend(child, state, SUSPEND_SAVE_STATE);
> > >+ if (ret == 0)
> > >+ ret = child->driver->suspend(child, state, SUSPEND_POWER_DOWN);
> > >+ }
> > >+ }
> > >
> > >
> > Oh my God. It will be called 3 times for each child
> > entry, isn't it?!
>
> OK. This stuff is probably wrong. I used the platform
> subsystem as an example. I need to do more research
> into how much work the driver core does for you.

Most platform drivers I've seen just handle the power on/off
requests. I think there's some historical reason that the
"reason" stuff exists ... but I suspect not many folk would
get unhappy if that were removed, and those calls got simplified.

- Dave

2005-09-15 19:24:24

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> wrote:

> > Date: Sat, 10 Sep 2005 12:17:06 +0100 (BST)
> > From: Mark Underwood <[email protected]>
> >
> > >
> > > - A "workqueue/task per driver" assumption.
> This
> > > is interesting, because
> > > it might be easier to port existing drivers
> into
> > > this new framework if
> > > there were always a "free" task context
> > > available. But on the other
> > > hand ... not all drivers will need or want a
> > > workqueue, and those which
> > > do can always create their own if keventd
> isn't
> > > good enough.
> >
> >
> >
> > Without the transfer routines I know it is hard to
> see
> > why I have used a workqueue so let me explain.
> > The workqueue is used as a scheduler for the
> adapter.
> > What happens is that a SPI device driver calls
> > transfer which creates a work item on the
> workqueue to
> > do the transfer. ...
>
> In short, it's an artifact of how some controller
> drivers are
> implemented. So it looks even more as if the right
> thing to do
> there is to NOT require all controller drivers to do
> that.
>
> Plus, since you've made it public, there's no way to
> ensure
> that nobody else uses that work queue. You're not
> following
> the basic "information hiding" rule for interface
> design.
>

Sorry I must not be explaining this very well :(.
_Only_ the core uses the work queue. This is what
happens:

1) SPI device driver creates message.
2) SPI device passes this message to the core.
3) The core puts this message on the work queue.
4) When the controller is ready to receive the next
message the message is passed to the adapter driver.

The work queue is used to isolate and serializes the
transfer calls from the SPI driver to the adapter.

>
> > You have to
> > have such a mechanism as you may have several SPI
> > devices on an adapter doing asynchronous IO at the
> > same time (i.e. giving an adapter that is in the
> > middle of doing work more work).
>
> No, you do not "have" to have such a mechanism. You
> can look at the USB
> stack for a counterexample. Both host and
> peripheral/gadget side adapter
> drivers manage request queues just fine without
> requiring such a task.
> And I assure you the queueing structure inside any
> USB Host Controller is
> orders of magnitude more complex than any SPI
> adapter needs.
>

First, quoting from urb.c
"Host Controller Drivers (HCDs) place all the URBs for
a particular endpoint in a queue."
So USB does use queues.

Here's that situation that shows why you need to queue
transfers.

A SPI adapter has two devices, a networking device and
an EEPROM.
The SPI network device doing asynchronous transfers
with call back the packets might be up to 1.5K. During
one of these transfers another device (in this example
an EEPROM) has to be read and thus the EEPROM driver
does a transfer. The transfer request from the EEPROM
gets put in the queue. When the adapter has finished
transmitting the message it notifies the core and the
core passes the EEPROM transfer request to the
adapter.

What could be even worse is that there might be two
(or more!) networking devices on one SPI adapter so a
message from one will have to be queued while a
message from the other is being transfered.


>
> > > - You set global policy on what
> "platform_data"
> > > should hold. That makes it
> > > not be platform-specific data any more!!
> All
> > > that info should be moved
> > > someplace else. (E.g. in my patch, some of
> it
> > > is held by the SPI core.)
> >
> > No, but don't the serial port drivers do that with
> the
> > serial_struct structure?
> > Anyway surely what SPI devices are on an adapter
> is
> > platform data! Why give the core work more to do?
>
> Well, I've repeatedly given examples of cases where
> that is the
> wrong model ... where the devices are associated
> with an expansion
> card, so any reliance on platform data (that's set
> up by and coupled
> to the baseboard) causes lots of problems.
>

I know! I agree that you can't always use the cs table
as you don't always know what devices are connected to
it. The problem that I have with your solution is how
can I deal with a hotplugable adapter? I don't know
what is bus number will be as I don't know what order
the adapters will be registered in. How else can I
solve this problem?

>
> > > - There's stuff I don't think should be in the
> > > device, like a semaphore
> > > (in addition to the driver model one!) and
> an
> > > "spi_driver" pointer
> > > (mirroring driver model dev->driver).
> >
> > As the comment says, it is used to sleep on for
> > blocking transfers, it's not a structure lock.
>
> The traditional way to do this is more like what I
> did in the refresh of my patch:
>

I'll have a look.

>
>
http://marc.theaimsgroup.com/?l=linux-kernel&m=112631114922427&w=2
>
>
http://marc.theaimsgroup.com/?l=linux-kernel&m=112631175219099&w=2
>
> The first of those shows how to do it without that
> sort
> of pre-allocation; see the synchronous calls.
>
>
> > > - And there's code to register and unregister
> a
> > > "spi_device". I've been
> > > thinking of that as the responsibility of
> the
> > > "SPI core", and that things
> > > should work like most other bus frameworks
> on
> > > Linux (other than I2C!!).
> > > That is, few folk (basically just subsystem
> > > maintainers) ever need to muck
> > > with those particular "innards". (That's
> the
> > > case in the code I sent.)
> > > For better or worse, such code has proven
> quite
> > > error prone.
> >
> > But the idea is that it's the same style as the
> > platform bus in which you register and unregister
> a
> > platform device.
>
> For SPI, there needs to be a layer between the
> platform device and the
> SPI devices. That's the sort of code that I was
> pointing out tends to
> be error prone.
>

I'm not talking about a layer between platform device
and SPI devices. I'm talking about using the same
style.

>
> > How else is the core going to know about the
> device?
>
> See the patches above; I've explained that before.
>
>
> > My idea is that most people will
> > pass in a cs_table which has the SPI devices on
> that
> > adapter in it, but as you have pointed out this
> will
> > not always be what people want (for more
> hotplug'ish
> > devices) in which case they register those SPI
> devices
> > in the same manor as platform devices (I need to
> do a
> > bit work more work on this).
>
> I've already done this work. You could just reuse
> my code. :)
>

I plan to :), although I might modify it a bit :O, I
would prefer to use bus_id then some randon bus number
number (it also makes it easier from looking at the
code to see where devices are in the system and
vicevisa).

>
> > > The I/O layer stub is less minimal than what I
> sent,
> > > but it's very similar
> > > in structure, names, and roles. A few things
> live
> > > in different places.
> > > I see convergence there!
> >
> > Good :). Although that patch didn't have the
> transfer
> > method in it :(.
>
> I'm not sure how to parse that. I remember seeing a
> transfer
> method of some kind; otherwise I'd not have been
> able to
> make such a comment. (And the "different places"
> seemed
> like a real issue ... per-request flags in the
> device,
> per-device flags in the request, etc.)
>

There is a transfer prototype (for adapters) and
message structures in the header file :).

>
> > > As for I/O fast paths, I was thinking more about
> > > "write word then read word"
> > > stuff than bulk transfers, since so many SPI
> devices
> > > seem to use that sort
> > > of protocol convention (including MicroWire
> > > hardware). And the message
> > > structure that's there now is prety
> bulk-oriented
> > > ... I modeled it on one
> > > from a DataFlash driver, not a sensor driver.
> > >
> >
> > The problem is that the "write word then read
> word"
> > model is not universal and the msg structure is.
>
> You can express "write word then read word" in terms
> of those messages;
> necessarily so. The controller method accepting a
> message structure
> would be be a "must implement" (otherwise the rest
> couldn't just be
> optional "fast path" support).
>
> OR ... for the very common case that the hardware
> supports such things
> directly, you could also add some fast paths. So
> folk wanting quick
> audio control, or sensor access, could get that.
> (Unless the controller
> were busy with some other request, in which case the
> request would need
> to get queued up as a "message". Not a fastpath.)
>

I'm not quite sure what your trying to achieve here.
These suggestions sound like hacks for specific
classes of devices which will not work (or would
require more code in the core layer) as you have to
remember that all these devices (EEPROM's, ADC's,
network adapters, flash devices etc) all have to live
on the same adapter and so should all use the same
transfer method/structures.

>
> > > > but what about
> > > > PCI cards which will have one interrupt line
> to
> > > > the interrupt controller and a interrupt
> status
> > > > register.
> > > > In this case the adapter will have to take the
> > > > interrupt and workout if it was for it or for
> the
> > > > SPI device.
> > >
> > > Are you implying the standard interrupt
> framework
> > > would NOT work here?
> >
> > Yes.
>
> You should maybe explain why it fails, in some other
> thread.
> I assure you the IRQ framework will not be changed
> just to
> make one exotic SPI implementation happier.
>

I'm not talking about changing the IRQ framework just
pointing out that it fails in this case.

>
> > > It already handles shared IRQs, whether for PCI
> or
> > > GPIO or whatever.
> > > (Or are you getting at something else?)
> >
> > But if a SPI device generates an interrupt on a
> PCI
> > card which has only one interrupt line, how does
> the
> > SPI device enable/disable/clear that interrupt
> given
> > that the register is in the PCI device and the SPI
> > device has no knowledge of that?
>
> This sounds like exactly the same problem lots of
> device drivers
> have: they may have one PCI interrupt, but
> internally the device
> has its own IRQ status, enable, and disable masks.
> Then they do
> the dispatch to different parts of the driver
> internally, based
> on the status which triggered the IRQ.
>
> SPI over PCI is reasonably exotic. Seems to me as
> if the best way
> to solve that kind of problem would involve
> specialized SPI-over-PCI
> code, matching that one board. Don't force every
> SPI implementations
> to cope with the quirks of that one PCI card,
> especially when most
> SPI hardware tres to be dead simple, to save costs.
>

No, no, no and no! For a start many manufactures
producing SPI devices don't have a OMAP board to test
there device but they all have a PC :). If you can do
it right then do it right, don't have 'specialized' or
hacked version of things. We are working on a 2.6
kernel not a 2.4 kernel ;). The way I suggested
requires 0 work to be done by most adapters and a
small amount of work the be done by 'special'
adapters.

>
> > > > 2) Extra GPO lines used for reset, etc.
> > > > These could be treated like extra CS lines
> which
> > > > can be changed in a spi_msg.
> > >
> > > Same as with IRQs: just pass them in with the
> chip's
> > > platform_data.
> > >
> > > What's problematic is the fact that all GPIO
> code is
> > > platform specific.
> > > What works on PXA250 won't work on x86 or OMAP,
> and
> > > it may not even work
> > > on PXA270. We'll still have limits to how
> portable
> > > drivers can be.
> > >
> >
> > Not if you treat GPO pins as chip selects and let
> the
> > adapter abstract them for you :).
>
> That assumes the GPIO pin is being used as a chip
> select,
> not as an input or output. :)

The point is that you can pass GPIO pins and routines
to handle them to the adapter (after first setting
them to output) as platform data this means that you
don't have to do any nasty platform specific hacks in
the SPI device driver, which can only be a good thing
:).

Mark

>
> - Dave
>
> -
> 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

2005-09-15 22:15:00

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> _Only_ the core uses the work queue. This is what
> happens:
>
> 1) SPI device driver creates message.
> 2) SPI device passes this message to the core.
> 3) The core puts this message on the work queue.
> 4) When the controller is ready to receive the next
> message the message is passed to the adapter driver.
>
> The work queue is used to isolate and serializes the
> transfer calls from the SPI driver to the adapter.

I see what's going on. You're assuming there even IS a core!

Where I assumed otherwise ... drivers that want a workqueue should
allocate their own. The cost of that task shouldn't be inflicted on
drivers that are happy just having their IRQ handlers start the next
transfer right from the driver's request queue. (I tend to avoid
layers -- like a "core" -- unless they're unavoidable. That helps
code stay smaller, faster, and more comprehensible code.)


> > > You have to
> > > have such a mechanism as you may have several SPI
> > > devices on an adapter doing asynchronous IO at the
> > > same time (i.e. giving an adapter that is in the
> > > middle of doing work more work).
> >
> > No, you do not "have" to have such a mechanism. You
> > can look at the USB
> > stack for a counterexample. Both host and
> > peripheral/gadget side adapter
> > drivers manage request queues just fine without
> > requiring such a task.
> >
>
> First, quoting from urb.c
> "Host Controller Drivers (HCDs) place all the URBs for
> a particular endpoint in a queue."
> So USB does use queues.

Exactly as I wrote ... the issue isn't having request queues.
The point of the example is that USB doesn't require workqueues.
In fact, they're normally not used by HCDs.


> > Well, I've repeatedly given examples of cases where
> > that is the
> > wrong model ... where the devices are associated
> > with an expansion
> > card, so any reliance on platform data (that's set
> > up by and coupled
> > to the baseboard) causes lots of problems.
> >
>
> I know! I agree that you can't always use the cs table
> as you don't always know what devices are connected to
> it. The problem that I have with your solution is how
> can I deal with a hotplugable adapter? I don't know
> what is bus number will be as I don't know what order
> the adapters will be registered in. How else can I
> solve this problem?

A hot pluggable adapter is going into some bus, and its driver will
be bound through that bus. Its probe routine will then create and
register the "struct spi_master". Output of that step includes the bus
number assigned to the adapter ... which is pretty much The Linux Way,
I can't think of a bus that works any other way.


> > OR ... for the very common case that the hardware
> > supports such things
> > directly, you could also add some fast paths. So
> > folk wanting quick
> > audio control, or sensor access, could get that.
> >
>
> I'm not quite sure what your trying to achieve here.

Trying to derail some confusion of yours, unsuccessfully. :(

The original observation was just that fast pathing _could_ be done
that way. I'd hope to avoid needing it, but experience suggests it's
good to explore that design space before settling on one approach.


> > SPI over PCI is reasonably exotic. Seems to me as
> > if the best way
> > to solve that kind of problem would involve
> > specialized SPI-over-PCI
> > code, matching that one board.
>
> No, no, no and no! For a start many manufactures
> producing SPI devices don't have a OMAP board to test
> there device but they all have a PC :)

Actually they can use any of hundreds of different Linux-equipped
embedded boards. Their _target_ environment is rarely a PC, and that
means most of the development cycle won't run there either. Ergo the
framework shouldn't be stretching _too_ much to handle issues that
only appear with one particular vendor's PCI-based eval hardware.

I can certainly understand how, say, Philips might want to support
evaluation boards in PC environments. It can be easier to debug on
PCs; not everyone has JTAG tools; and so on.


> > > Not if you treat GPO pins as chip selects and let
> > > the adapter abstract them for you :).
> >
> > That assumes the GPIO pin is being used as a chip
> > select, not as an input or output. :)
>
> The point is that you can pass GPIO pins and routines
> to handle them to the adapter (after first setting
> them to output) as platform data this means that you
> don't have to do any nasty platform specific hacks in
> the SPI device driver, which can only be a good thing :).

Sure, I've certainly done that. And that'd be the best way
to build a bitbanging SPI adapter too ... every platform
will bang the bits differently (GPIOs, parport, etc).

- Dave


2005-09-15 23:27:31

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> wrote:

> > _Only_ the core uses the work queue. This is what
> > happens:
> >
> > 1) SPI device driver creates message.
> > 2) SPI device passes this message to the core.
> > 3) The core puts this message on the work queue.
> > 4) When the controller is ready to receive the
> next
> > message the message is passed to the adapter
> driver.
> >
> > The work queue is used to isolate and serializes
> the
> > transfer calls from the SPI driver to the adapter.
>
> I see what's going on. You're assuming there even
> IS a core!
>
> Where I assumed otherwise ... drivers that want a
> workqueue should
> allocate their own. The cost of that task shouldn't
> be inflicted on
> drivers that are happy just having their IRQ
> handlers start the next
> transfer right from the driver's request queue. (I
> tend to avoid
> layers -- like a "core" -- unless they're
> unavoidable. That helps
> code stay smaller, faster, and more comprehensible
> code.)
>

I think my explanation is getting there! I'll
hopefully post the transfer routines this weekend
which will shine some light on this, but I keep having
to work on other things :( ).

The workqueue belongs to an adapter as you _will_ have
several SPI device drivers all trying trying send
messages through that _one_ adapter. Unless you only
allow blocking I/O which I _don't_ want to do then the
core has to handle the fact that _different_ SPI
device drivers will be trying to transfer messages
while the adapter is already in the middle of a
transfer.

ASCII art time :)

SPI device drivers| Core | Adapter driver
---------- msg | |
| EEPROM |------> |- |
---------- | | |
------------ msg | | ------- | msg ------
| ETHERNET |----> |-->|Queue|->|---->|xfer|
------------ | | ------- | ------
--------- msg | | ^ | |
| FLASH |-------> |- ------|<------- Next/done
--------- | |

>
> > > > You have to
> > > > have such a mechanism as you may have several
> SPI
> > > > devices on an adapter doing asynchronous IO at
> the
> > > > same time (i.e. giving an adapter that is in
> the
> > > > middle of doing work more work).
> > >
> > > No, you do not "have" to have such a mechanism.
> You
> > > can look at the USB
> > > stack for a counterexample. Both host and
> > > peripheral/gadget side adapter
> > > drivers manage request queues just fine without
> > > requiring such a task.
> > >
> >
> > First, quoting from urb.c
> > "Host Controller Drivers (HCDs) place all the URBs
> for
> > a particular endpoint in a queue."
> > So USB does use queues.
>
> Exactly as I wrote ... the issue isn't having
> request queues.
> The point of the example is that USB doesn't require
> workqueues.
> In fact, they're normally not used by HCDs.
>

If you don't have a queue of some sort how do you
handle the fact that one or more devices will want to
send asynchronous messages through one adapter while
that adapter is still busy transferring a previous
message?

>
> > > Well, I've repeatedly given examples of cases
> where
> > > that is the
> > > wrong model ... where the devices are associated
> > > with an expansion
> > > card, so any reliance on platform data (that's
> set
> > > up by and coupled
> > > to the baseboard) causes lots of problems.
> > >
> >
> > I know! I agree that you can't always use the cs
> table
> > as you don't always know what devices are
> connected to
> > it. The problem that I have with your solution is
> how
> > can I deal with a hotplugable adapter? I don't
> know
> > what is bus number will be as I don't know what
> order
> > the adapters will be registered in. How else can I
> > solve this problem?
>
> A hot pluggable adapter is going into some bus, and
> its driver will
> be bound through that bus. Its probe routine will
> then create and
> register the "struct spi_master". Output of that
> step includes the bus
> number assigned to the adapter ... which is pretty
> much The Linux Way,
> I can't think of a bus that works any other way.
>

OK. But you also need to see what else is in the cs
table, namely the default level of the cs's. The issue
which we have to solve is that all the cs's have to be
put into their non-active state before any transfer
can be done. If devices are added into a running SPI
adapter (I mean registration of devices not physical
plugging) then how do you know what their idle state
is at the beginning (when the first SPI device does a
transfer? To me the only solution seems to be to pass
the idle state of all the devices that will be on that
device (be they hardwire, plugged or what ever) when
then adapter gets registered which is why I put it in
as part of platform data.

>
> > > OR ... for the very common case that the
> hardware
> > > supports such things
> > > directly, you could also add some fast paths.
> So
> > > folk wanting quick
> > > audio control, or sensor access, could get that.
>
> > >
> >
> > I'm not quite sure what your trying to achieve
> here.
>
> Trying to derail some confusion of yours,
> unsuccessfully. :(
>
> The original observation was just that fast pathing
> _could_ be done
> that way. I'd hope to avoid needing it, but
> experience suggests it's
> good to explore that design space before settling on
> one approach.
>

I agree. The problem is, as far as I can see, every
message will have to be placed in a queue and then
passed to an adapter if several devices are going to
be living on the same adapter.

>
> > > SPI over PCI is reasonably exotic. Seems to me
> as
> > > if the best way
> > > to solve that kind of problem would involve
> > > specialized SPI-over-PCI
> > > code, matching that one board.
> >
> > No, no, no and no! For a start many manufactures
> > producing SPI devices don't have a OMAP board to
> test
> > there device but they all have a PC :)
>
> Actually they can use any of hundreds of different
> Linux-equipped
> embedded boards. Their _target_ environment is
> rarely a PC, and that
> means most of the development cycle won't run there
> either. Ergo the
> framework shouldn't be stretching _too_ much to
> handle issues that
> only appear with one particular vendor's PCI-based
> eval hardware.

OK.

>
> I can certainly understand how, say, Philips might
> want to support
> evaluation boards in PC environments. It can be
> easier to debug on
> PCs; not everyone has JTAG tools; and so on.
>

That's my thinking. Plus I was planning on writing a
parallel port bit-banging adapter as a sweetnear for
the PC Linux folk and later a SPI MMC driver so your
PC could have a MMC slot :) (even old 386's & 486's
which don't have USB and thus no card readers!).

>
> > > > Not if you treat GPO pins as chip selects and
> let
> > > > the adapter abstract them for you :).
> > >
> > > That assumes the GPIO pin is being used as a
> chip
> > > select, not as an input or output. :)
> >
> > The point is that you can pass GPIO pins and
> routines
> > to handle them to the adapter (after first setting
> > them to output) as platform data this means that
> you
> > don't have to do any nasty platform specific hacks
> in
> > the SPI device driver, which can only be a good
> thing :).
>
> Sure, I've certainly done that. And that'd be the
> best way
> to build a bitbanging SPI adapter too ... every
> platform
> will bang the bits differently (GPIOs, parport,
> etc).

More common ground :)!

Mark

>
> - Dave
>
>
> -
> 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/
>






___________________________________________________________
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com

2005-09-16 03:26:21

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> ASCII art time :)

ASCII also works for English and C, but this is good too. ;)



> SPI device drivers| Core | Adapter driver

Or in my terminology, with no core/midlayer expectation:

SPI Master | SPI Master
Protocol Driver | Controller Driver

> ---------- msg | |
> | EEPROM |------> |- |
> ---------- | | |
> ------------ msg | | ------- | msg ------
> | ETHERNET |----> |-->|Queue|->|---->|xfer|
> ------------ | | ------- | ------
> --------- msg | | ^ | |
> | FLASH |-------> |- ------|<------- Next/done
> --------- | |

And the workqueue usage you're describing (to manage that queue)
is what I'd call just one of many implementation strategies; one
that wouldn't be universally appropriate.

You don't NEED a separate "core" to hold the queue; the driver
can just as easily manage a list_head itself.

- A PIO driver running with IRQs blocked wouldn't need a queue.
(And that might be fine for low volume usage, since it might
take all of an hour to write and debug.)

- A DMA driver might build the queue out of DMA descriptors, so
that the hardware would process many transactions back-to-back
without needing intermediate attention from the CPU. (Great
for high volume usage, but likely takes more than an hour.)

You don't NEED to allocate a task ("workqueue") to handle the
movement of requests from the queue to the hardware.

- Drivers are often written so that the "next" transaction is
started from an IRQ handler ... it's a lot less I/O latency
than if you need the workqueue's task to schedule before that
transaction can start, and better throughput too.

That's three more implementation strategies ... ones that don't need
or want to have a "workqueue" task, and can easily manage a list_head.
But you want them all to not just have a workqueue, but not be able
to work unless they interact with it!! Why?



> If you don't have a queue of some sort how do you
> handle the fact that one or more devices will want to
> send asynchronous messages through one adapter while
> that adapter is still busy transferring a previous
> message?

You seem to have been replying to someone else's posts! The
examples I gave _included_ queues. Queues that advanced using
IRQs, and didn't need to schedule a workqueue's task before
starting another transaction.


> OK. But you also need to see what else is in the cs
> table, namely the default level of the cs's. The issue
> which we have to solve is that all the cs's have to be
> put into their non-active state before any transfer
> can be done.

If you observed the code I sent by, you'll observe comments about that
issue in the declaration of both "spi_device" and "spi_board_info".

There are other protocol tweaks to be dealt with over time too. As one
person commented off-line, there are multiple protocols to fit into this
API (SPI, MicroWire, SSI, SSP, and more) that differ in only minor ways.
The controller driver may need to know about some of those tweaks in
order to talk properly with a given chip.


> If devices are added into a running SPI
> adapter (I mean registration of devices not physical
> plugging) then how do you know what their idle state
> is at the beginning (when the first SPI device does a
> transfer?

Until someone has to hook up hardware that acts atypically, it's the
normal SPI convention: chipselect is "active low". When someone
needs that, I've already identified where to record "active high".


> To me the only solution seems to be to pass
> the idle state of all the devices that will be on that
> device (be they hardwire, plugged or what ever) when
> then adapter gets registered which is why I put it in
> as part of platform data.

Much like I did with "spi_board_info", but on a per-chip basis. It's
possible to hook an "active low" chip to one chipselect while another
uses "active high" signaling. It's just an inverter. :)



> > I can certainly understand how, say, Philips might
> > want to support
> > evaluation boards in PC environments. It can be
> > easier to debug on
> > PCs; not everyone has JTAG tools; and so on.
> >
>
> That's my thinking. Plus I was planning on writing a
> parallel port bit-banging adapter as a sweetnear for
> the PC Linux folk and later a SPI MMC driver so your
> PC could have a MMC slot :) (even old 386's & 486's
> which don't have USB and thus no card readers!).

Yes, I expect an SPI bitbanger will be useful to some folk.
As for MMC ... it'll be interesting to watch that play out;
won't the mmc_block code need to change?


I attach the latest snapshot of my code. You'll notice two changes:
(a) suspend/resume calls; this code seems pretty complete except for
the protocol tweaking options; (b) a new method that'll be handy
when doing things like hotplugging a card with an SPI controller
and a few soldered-on SPI devices. (Like that USB prototype.)

- Dave


===========================

This is the start of a small SPI framework that started fresh, so that
doesn't perpetuate the "i2c driver model mess".

- It's still less than 2KB of ".text" (ARM, modules enabled). If there
must be more code than the drivers, that's the right size budget. :)

- The guts use board-specific SPI device tables to build the driver
model tree. (Hardware probing is rarely an option.)

- The Kconfig should be informative about the scope and content of SPI.

- Building more drivers into this framework likely means updating the
I/O "async message" model to include protocol tweaking (like I2C).

- No userspace API. There are several implementations to compare.
Implement them like any other driver; no magic hooks!

This should suffice for writing (or adapting) real driver code, such
as for SPI master controllers (including hotpluggable ones like a
parport bitbanger) or protocol masters.


--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/include/linux/spi.h 2005-09-15 18:08:00.293267631 -0700
@@ -0,0 +1,268 @@
+/*
+ * Copyright (C) 2005 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef __LINUX_SPI_H
+#define __LINUX_SPI_H
+
+/*
+ * PROTOTYPE !!!
+ *
+ * The focus is on driver model support ... enough for SPI mastering
+ * board setups to work. The I/O model still needs attention, since
+ * SPI protocols seem to need to tweaking.
+ */
+
+/*---------------------------------------------------------------------------*/
+
+/*
+ * INTERFACES between SPI master drivers and infrastructure
+ *
+ * There are two types of master (or slave) driver: "controller" drivers
+ * usually work on platform devices and touch chip registers; "protocol"
+ * drivers work on abstract SPI devices by asking a controller driver to
+ * transfer the relevant data, and they shouldn't much care if they use
+ * one controller or another.
+ *
+ * A "struct device_driver" for an SPI device uses "spi_bus_type" and needs
+ * no special API wrappers (much like platform_bus). These drivers are
+ * bound to devices based on their names (much like platform_bus), and
+ * are available in dev->driver.
+ */
+extern struct bus_type spi_bus_type;
+
+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)
+
+ // MAYBE ADD hooks for protocol options that affect how
+ // the controller talks to its chips:
+ // - chipselect polarity (default active low)
+ // - spi wordsize (default 8 bits unsigned)
+ // - bit order (default is wordwise msb-first)
+ // - memory packing (default: 12 bit samples into 16 bits, msb)
+ // - priority
+ // - delays
+ // - ...
+
+};
+
+static inline struct spi_device *to_spi_device(struct device *dev)
+{
+ return container_of(dev, struct spi_device, dev);
+}
+
+
+struct spi_message;
+
+struct spi_master {
+ struct class_device cdev;
+
+ /* other than zero (== assign one dynamically), bus_num is fully
+ * board-specific. usually that simplifies to being SOC-specific.
+ * for example: one SOC has three SPI controllers, numbered 1..3,
+ * and one board's schematics might show it using SPI-2. software
+ * would normally use bus_num=2 for that controller.
+ */
+ u16 bus_num;
+
+ /* chipselects will be integral to many controllers, while others
+ * might use board-specific GPIOs.
+ */
+ u16 num_chipselect;
+
+ /* setup mode and clock, etc */
+ int (*setup)(struct spi_device *spi);
+
+ /* bidirectional bulk transfers
+ * + transfer() may not sleep
+ * + to a given spi_device, message queueing is pure fifo
+ * + if there are multiple spi_device children, the i/o queue
+ * arbitration algorithm is unspecified (round robin, fifo,
+ * priority, reservations, preemption, etc)
+ * + the master's main job is to process its message queue,
+ * selecting a chip then transferring data
+ * + for now there's no remove-from-queue operation, or
+ * any other request management
+ */
+ int (*transfer)(struct spi_device *spi,
+ struct spi_message *mesg);
+};
+
+/* the controller driver manages memory for the spi_master classdev,
+ * normally contained inside its associated private state.
+ */
+extern int spi_register_master(struct device *host, struct spi_master *spi);
+extern void spi_unregister_master(struct spi_master *spi);
+
+
+/*---------------------------------------------------------------------------*/
+
+/*
+ * I/O INTERFACE between SPI controller and protocol drivers
+ *
+ * The interface is a queue of spi_messages, each transferring data
+ * between the controller and memory buffers. SPI protocol drivers just
+ * provide spi_messages to hardware controller drivers. SPI controller
+ * drivers process that queue, issuing completion callbacks as appropriate.
+ *
+ * The spi_messages themselves consist of a series of read+write transfer
+ * segments. Those segments always read the same number of bits as they
+ * write; but one or the other is easily ignored by passing a null buffer
+ * pointer. (This is unlike most types of I/O API, because SPI hardware
+ * is full duplex.)
+ *
+ * NOTE: Allocation of spi_transfer and spi_message memory is entirely
+ * up to the protocol driver, which guarantees the integrity of both (as
+ * well as the data buffers) for as long as the message is queued.
+ */
+
+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;
+};
+
+/* spi messages will often be stack-allocated */
+struct spi_message {
+ struct spi_transfer *transfers;
+ unsigned n_transfer;
+
+ struct spi_device *dev;
+
+ /* Optionally leave this chipselect active afterward */
+ unsigned csrel_disable:1;
+
+ /* 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;
+};
+
+/**
+ * spi_setup -- setup SPI mode and clock rate
+ * @spi: the device whose settings are being modified
+ *
+ * SPI protocol drivers may need to update the transfer mode if the
+ * device doesn't work with the mode 0 default. They may likewise need
+ * to update clock rates. This function changes those settings,
+ * and must be called from a context that can sleep.
+ */
+static inline int
+spi_setup(struct spi_device *spi)
+{
+ return spi->master->setup(spi);
+}
+
+
+/* synchronous SPI transfers; these may sleep uninterruptibly */
+extern int spi_sync(struct spi_device *spi, struct spi_message *message);
+extern int spi_w8r8(struct spi_device *spi, u8 cmd);
+extern int spi_w8r16(struct spi_device *spi, u8 cmd);
+
+
+/**
+ * spi_async -- asynchronous SPI transfer
+ * @spi: device with which data will be exchanged
+ * @message: describes the data transfers, including completion callback
+ *
+ * This call may be used in_irq in other contexts which can't sleep,
+ * as well as from task contexts which can sleep.
+ *
+ * The completion callback is invoked in a context which can't sleep.
+ * Before that invocation, the value of message->status is undefined.
+ * When the callback is issue, message->status holds either zero (to
+ * indicate complete success) or a negative error code.
+ */
+static inline int
+spi_async(struct spi_device *spi, struct spi_message *message)
+{
+ message->dev = spi;
+ return spi->master->transfer(spi, message);
+}
+
+/*---------------------------------------------------------------------------*/
+
+/*
+ * INTERFACE between board init code and SPI infrastructure.
+ *
+ * No SPI driver ever sees these SPI device table segments, but
+ * it's how the SPI core (or adapters that get hotplugged) grows
+ * the driver model tree.
+ */
+
+/* board-specific information about each SPI device */
+struct spi_board_info {
+ /* the device name and module name are coupled, like platform_bus.
+ *
+ * platform_data has things like IRQ assignments (just pass the int
+ * that goes to request_irq), related GPIOs, and so on.
+ */
+ char modalias[KOBJ_NAME_LEN];
+ void *platform_data;
+
+ /* slower signaling on noisy or low voltage boards */
+ u32 max_speed_hz;
+
+ /* bus_num is board specific and matches the bus_num of some
+ * spi_master that will probably be registered later.
+ *
+ * chip_select reflects how this chip is wired to that master;
+ * it's less than num_chipselect.
+ */
+ u16 bus_num;
+ u16 chip_select;
+
+ /* ... may need additional spi_device chip config data here.
+ * avoid stuff protocol drivers can set; but include stuff
+ * needed to behave without being bound to a driver:
+ * - chipselect polarity
+ * - quirks like clock rate mattering when not selected
+ */
+};
+
+extern int
+spi_register_board_info(struct spi_board_info const *info, unsigned n);
+
+extern struct spi_device *
+spi_new_device(struct spi_master *, struct spi_board_info *);
+
+static inline void
+spi_unregister_device(struct spi_device *spi)
+{
+ if (spi)
+ device_unregister(&spi->dev);
+}
+
+
+#endif /* __LINUX_SPI_H */
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/spi/spi.c 2005-09-15 17:22:46.104517401 -0700
@@ -0,0 +1,482 @@
+/* spi.c -- prototype of SPI init/core code
+ *
+ * Copyright (C) 2005 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/autoconf.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/spi.h>
+
+
+/* SPI bustype and spi_master class are registered during early boot,
+ * usually after board init code provided the SPI device tables, and
+ * are available when driver init code needs them.
+ *
+ * Drivers for SPI devices are like those for platform bus devices:
+ * (a) no bus-specific API wrappers (== needless bloat here)
+ * (b) matched to devices using device names
+ * (c) should support "native" suspend and resume methods
+ */
+static void spidev_release(struct device *dev)
+{
+ const struct spi_device *spi = to_spi_device(dev);
+
+ class_device_put(&spi->master->cdev);
+ kfree(dev);
+}
+
+// no, we probably don't need to list speed and mode in sysfs...
+
+static ssize_t
+modalias_show(struct device *dev, struct device_attribute *a, char *buf)
+{
+ const char *modalias = strchr(dev->bus_id, '-') + 1;
+
+ return snprintf(buf, BUS_ID_SIZE + 1, "%s\n", modalias);
+}
+
+#ifdef DEBUG
+
+static ssize_t
+maxspeed_show(struct device *dev, struct device_attribute *a, char *buf)
+{
+ const struct spi_device *spi = to_spi_device(dev);
+
+ return sprintf(buf, "%u\n", spi->max_speed_hz);
+}
+
+static ssize_t
+mode_show(struct device *dev, struct device_attribute *a, char *buf)
+{
+ struct spi_device *spi = to_spi_device(dev);
+
+ return sprintf(buf, "%u\n", spi->mode);
+}
+
+#endif /* DEBUG */
+
+static struct device_attribute spi_dev_attrs[] = {
+ __ATTR_RO(modalias),
+#ifdef DEBUG
+ __ATTR_RO(maxspeed),
+ __ATTR_RO(mode),
+#endif /* DEBUG */
+ __ATTR_NULL,
+};
+
+static int spi_match_device(struct device *dev, struct device_driver *drv)
+{
+ const char *modalias = strchr(dev->bus_id, '-') + 1;
+
+ return strncmp(modalias, drv->name, BUS_ID_SIZE) == 0;
+}
+
+static int spi_hotplug(struct device *dev, char **envp, int num_envp,
+ char *buffer, int buffer_size)
+{
+ const char *modalias = strchr(dev->bus_id, '-') + 1;
+
+ envp[0] = buffer;
+ snprintf(buffer, buffer_size, "MODALIAS=%s", modalias);
+ envp[1] = NULL;
+ return 0;
+}
+
+/* suspend/resume in "struct device_driver" don't really need that
+ * strange third parameter, so we just make it a constant and expect
+ * drivers to ignore it.
+ */
+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;
+}
+
+static int spi_resume(struct device *dev)
+{
+ if (dev->driver && dev->driver->resume)
+ return dev->driver->resume(dev, RESUME_POWER_ON);
+ else
+ return 0;
+}
+
+struct bus_type spi_bus_type = {
+ .name = "spi",
+ .dev_attrs = spi_dev_attrs,
+ .match = spi_match_device,
+ .hotplug = spi_hotplug,
+ .suspend = spi_suspend,
+ .resume = spi_resume,
+};
+EXPORT_SYMBOL_GPL(spi_bus_type);
+
+static struct class spi_master_class = {
+ .name = "spi_master",
+ .owner = THIS_MODULE,
+};
+
+
+/*-------------------------------------------------------------------------*/
+
+/* SPI devices should normally not be created by SPI device drivers; that
+ * would make them board-specific. Similarly with SPI master drivers.
+ * Device registration normally goes into like arch/.../mach.../board-YYY.c
+ * with other information about mainboard devices.
+ */
+
+struct boardinfo {
+ struct list_head list;
+ unsigned n_board_info;
+ struct spi_board_info board_info[0];
+};
+
+static LIST_HEAD(board_list);
+static DECLARE_MUTEX(board_lock);
+
+#define kzalloc(n, flags) kcalloc(1,(n),(flags))
+
+static int __init_or_module
+check_child(struct device *dev, void *data)
+{
+ const struct spi_device *spi = to_spi_device(dev);
+ const struct spi_board_info *chip = data;
+
+ return (spi->chip_select == chip->chip_select);
+}
+
+
+/* On typical mainboards, this is purely internal; and it's not needed
+ * after board init creates the hard-wired devices. Some development
+ * platforms may not be able to use spi_register_board_info though, and
+ * this is exported so that for example a USB or parport based adapter
+ * driver could add devices.
+ */
+struct spi_device *__init_or_module
+spi_new_device(struct spi_master *master, struct spi_board_info *chip)
+{
+ struct spi_device *proxy;
+ struct device *dev = master->cdev.dev;
+ int status;
+
+ /* NOTE: caller did any chip->bus_num checks necessary */
+
+ /* only one child per chipselect, ever */
+ if (device_for_each_child(dev, chip, check_child))
+ return NULL;
+
+ if (!class_device_get(&master->cdev))
+ return NULL;
+
+ proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
+ if (!proxy) {
+ dev_err(dev, "can't alloc dev for cs%d\n",
+ chip->chip_select);
+ goto fail;
+ }
+ proxy->master = master;
+ proxy->chip_select = chip->chip_select;
+ proxy->max_speed_hz = chip->max_speed_hz;
+
+ snprintf(proxy->dev.bus_id, sizeof proxy->dev.bus_id,
+ "%s.%u-%s", master->cdev.class_id,
+ chip->chip_select, chip->modalias);
+ proxy->dev.parent = dev;
+ proxy->dev.bus = &spi_bus_type;
+ proxy->dev.platform_data = chip->platform_data;
+ proxy->dev.release = spidev_release;
+
+ /* drivers may modify this default i/o setup */
+ status = master->setup(proxy);
+ if (status < 0) {
+ dev_dbg(dev, "can't %s %s, status %d\n",
+ "setup", proxy->dev.bus_id, status);
+ goto fail;
+ }
+
+ status = device_register(&proxy->dev);
+ if (status < 0) {
+ dev_dbg(dev, "can't %s %s, status %d\n",
+ "add", proxy->dev.bus_id, status);
+fail:
+ class_device_put(&master->cdev);
+ kfree(proxy);
+ return NULL;
+ }
+ dev_dbg(dev, "registered child %s\n", proxy->dev.bus_id);
+ return proxy;
+}
+EXPORT_SYMBOL_GPL(spi_new_device);
+
+/*
+ * Board-specific early init code calls this (probably during arch_initcall)
+ * with segments of the SPI device table. Any device nodes are created later,
+ * after the relevant parent SPI controller (bus_num) is defined. We keep
+ * this table of devices forever, so that reloading a controller driver will
+ * not make Linux forget about these hard-wired devices.
+ *
+ * Other code can also call this, e.g. a particular add-on board might provide
+ * SPI devices through its expansion connector, so code initializing that board
+ * would naturally declare its SPI devices.
+ *
+ * The board info passed can safely be __initdata ... but be careful of
+ * any embedded pointers (platform_data, etc), they're copied as-is.
+ */
+int __init_or_module // would be __init except for SPI_EXAMPLE
+spi_register_board_info(struct spi_board_info const *info, unsigned n)
+{
+ struct boardinfo *bi;
+
+ bi = kmalloc (sizeof (*bi) + n * sizeof (*info), GFP_KERNEL);
+ if (!bi)
+ return -ENOMEM;
+ bi->n_board_info = n;
+ memcpy(bi->board_info, info, n * sizeof (*info));
+
+ down(&board_lock);
+ list_add_tail(&bi->list, &board_list);
+ up(&board_lock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spi_register_board_info);
+
+static void __init_or_module
+scan_boardinfo(struct spi_master *master)
+{
+ struct boardinfo *bi;
+ struct device *dev = master->cdev.dev;
+
+ down(&board_lock);
+ list_for_each_entry(bi, &board_list, list) {
+ struct spi_board_info *chip = bi->board_info;
+ unsigned n;
+
+ for (n = bi->n_board_info; n > 0; n--, chip++) {
+ if (chip->bus_num != master->bus_num)
+ continue;
+ if (chip->chip_select >= master->num_chipselect) {
+ dev_dbg(dev, "cs%d > max %d\n",
+ chip->chip_select,
+ master->num_chipselect);
+ continue;
+ }
+ (void) spi_new_device(master, chip);
+ }
+ }
+ up(&board_lock);
+}
+
+/*-------------------------------------------------------------------------*/
+
+/**
+ * spi_register_master - register SPI master controller
+ * @dev: the controller
+ * @master: the master, with all uninitialized fields zeroed
+ *
+ * This call is used only by SPI master controller drivers, which are the
+ * only ones directly touching chip registers.
+ *
+ * SPI master controllers connect to their drivers using some non-SPI bus,
+ * such as the platform bus. The final stage of probe() in that code
+ * includes calling spi_register_master(), with memory managed by that
+ * controller, to hook up to this SPI bus glue.
+ *
+ * SPI controllers use board specific (often SOC specific) bus numbers,
+ * and board-specific addressing for SPI devices combines those numbers
+ * with chip select numbers. Since SPI does not directly support dynamic
+ * device identification, boards need configuration tables tellin which
+ * chip is at which address.
+ *
+ * This must be called from context that can sleep. It returns zero
+ * on success, else a negative error code.
+ */
+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;
+ }
+
+ /* convention: dynamically assigned bus IDs count down from the max */
+ if (master->bus_num == 0) {
+ master->bus_num = atomic_dec_return(&dyn_bus_id);
+ dev_dbg(dev, "spi%d, dynamic bus number\n", master->bus_num);
+ }
+
+ /* ELSE: verify that the ID isn't in use already */
+
+ master->cdev.class = &spi_master_class;
+ master->cdev.dev = get_device(dev);
+ class_set_devdata(&master->cdev, master);
+ snprintf(master->cdev.class_id, sizeof master->cdev.class_id,
+ "spi%u", master->bus_num);
+
+ /* register the device, then userspace will see it */
+ status = class_device_register(&master->cdev);
+ if (status < 0) {
+ put_device(dev);
+ goto done;
+ }
+ dev_dbg(dev, "registered master %s\n", master->cdev.class_id);
+
+ scan_boardinfo(master);
+ status = 0;
+done:
+ return status;
+}
+EXPORT_SYMBOL_GPL(spi_register_master);
+
+
+/**
+ * spi_unregister_master - unregister SPI master controller
+ * @master: the master being unregistered
+ *
+ * This call is used only by SPI master controller drivers, which are the
+ * only ones directly touching chip registers.
+ *
+ * This must be called from context that can sleep.
+ */
+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);
+
+
+/*-------------------------------------------------------------------------*/
+
+static void spi_sync_complete(void *done)
+{
+ complete(done);
+}
+
+/**
+ * spi_sync - blocking/synchronous SPI data transfers
+ * @spi: device with which data will be exchanged
+ * @message: describes the data transfers
+ *
+ * This call may only be used from a context that may sleep.
+ * The sleep is non-interruptible, and has no timeout.
+ *
+ * The return value is a negative error code if the message could not be
+ * submitted, else zero. When the value is zero, then message->status is
+ * also defined: it's the completion code for the transfer, either zero
+ * or a negative error code from the controller driver.
+ */
+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);
+
+/**
+ * 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.
+ */
+int spi_w8r8(struct spi_device *spi, u8 cmd)
+{
+ int status;
+ struct spi_message message;
+ struct spi_transfer x[2];
+ u8 result;
+
+ x[0].tx_buf = &cmd;
+ x[0].rx_buf = NULL;
+ x[0].len = 1;
+
+ x[1].tx_buf = NULL;
+ x[1].rx_buf = &result;
+ x[1].len = 1;
+
+ /* do the i/o */
+ message.transfers = &x[0];
+ message.n_transfer = ARRAY_SIZE(x);
+ status = spi_sync(spi, &message);
+
+ /* return negative errno or unsigned value */
+ return (status < 0) ? status : result;
+}
+EXPORT_SYMBOL(spi_w8r8);
+
+/**
+ * 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.
+ */
+int spi_w8r16(struct spi_device *spi, u8 cmd)
+{
+ int status;
+ struct spi_message message;
+ struct spi_transfer x[2];
+ u16 result;
+
+ x[0].tx_buf = &cmd;
+ x[0].rx_buf = NULL;
+ x[0].len = 1;
+
+ x[1].tx_buf = NULL;
+ x[1].rx_buf = &result;
+ x[1].len = 2;
+
+ /* do the i/o */
+ message.transfers = &x[0];
+ message.n_transfer = ARRAY_SIZE(x);
+ status = spi_sync(spi, &message);
+
+ /* return negative errno or unsigned value */
+ return (status < 0) ? status : result;
+}
+EXPORT_SYMBOL(spi_w8r16);
+
+/*-------------------------------------------------------------------------*/
+
+static int __init spi_init(void)
+{
+ bus_register(&spi_bus_type);
+ class_register(&spi_master_class);
+ return 0;
+}
+postcore_initcall(spi_init);
+
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/spi/Kconfig 2005-09-15 18:08:50.311703596 -0700
@@ -0,0 +1,319 @@
+#
+# This is what Linux SPI might look support after adding drivers for
+# some embedded SPI-capable hardware that's used with Linux.
+#
+# NOTE: over half of the SPI protocol masters mentioned actually exist,
+# and sometimes even on 2.6. Few of the SPI master controller drivers
+# exist. None of those drivers use the 2.6 driver model, and they all
+# have different I/O models too.
+#
+
+#
+# SPI device configuration
+#
+menu "SPI support"
+
+config SPI
+ bool "SPI support"
+ depends on SPI_ARCH_HAS_MASTER || SPI_ARCH_HAS_SLAVE
+ help
+ The "Serial Peripheral Interface" is a low level synchronous
+ protocol used to talk with sensors, eeprom and flash memory,
+ codecs and various other controller chips, analog to digital
+ (and d-to-a) converters, and more. MMC and SD cards can be
+ accessed using SPI protocol; and for DataFlash cards used in
+ MMC sockets, SPI must be used.
+
+ Chips that support SPI can have data transfer rates up to several
+ tens of Mbit/sec, and the controllers often support DMA. Chips
+ are addressed with a controller and a chipselect. Most SPI
+ devices don't support dynamic device discovery; some are even
+ write-only or read-only.
+
+ SPI is one of a family of similar protocols using a four wire
+ interface (select, clock, data in, data out) including Microwire
+ (half duplex), SSP, SSI, and PSP. This driver framework should
+ work with most such devices and controllers.
+
+config SPI_DEBUG
+ boolean "Debug support for SPI drivers"
+ depends on SPI && DEBUG_KERNEL
+ help
+ Say "yes" to enable debug messaging (like dev_dbg and pr_debug),
+ sysfs, and debugfs support in SPI controller and protocol drivers.
+
+# someday this stuff should be set using arch/CPU/PLATFORM/Kconfig
+config SPI_ARCH_HAS_MASTER
+ boolean
+ default y if ARCH_OMAP
+ default y if ARCH_PXA
+ default y if ARCH_AT91
+ default y if X86 # DEVEL HACK
+
+config SPI_ARCH_HAS_SLAVE
+ boolean
+ default y if ARCH_OMAP
+ default y if ARCH_PXA
+ default y if ARCH_AT91
+ default y if X86 # DEVEL HACK
+
+#
+# MASTER side ... talking to discrete SPI slave chips including microcontrollers
+#
+comment "No SPI master hardware is available."
+ depends on SPI && !SPI_ARCH_HAS_MASTER
+
+menuconfig SPI_MASTER
+ boolean "SPI Master support"
+ depends on SPI && SPI_ARCH_HAS_MASTER
+ help
+ If your system has an master-capable SPI controller (which
+ provides the clock and chipselect), you can enable that
+ controller and the protocol drivers for the SPI slave chips
+ that are connected.
+
+if SPI_MASTER
+
+comment "SPI Master Controller Drivers"
+
+config SPI_EXAMPLE
+ tristate "SPI Platform Example"
+ help
+ This just builds some sample code uses the core APIs to build
+ some SPI devices and plug in dummy controller drivers.
+
+# Atmel AT91rm9200 (and some other AT91 family chips)
+config SPI_AT91
+ tristate "AT91 as SPI master"
+ depends on ARCH_AT91
+ help
+ This implements SPI master mode using an SPI controller.
+
+# FIXME bitbangers need arch-specific ways to access the
+# right GPIO pins, probably using platform data and maybe
+# using platform-specific minidrivers.
+config SPI_BITBANG
+ tristate "Bitbanging SPI master"
+ help
+ You can implement SPI using GPIO pins, as this driver
+ eventually should do.
+
+# Motorola Coldfire (m68k)
+config SPI_COLDFIRE
+ tristate "Coldfire QSPI as SPI master"
+ depends on COLDFIRE
+ help
+ This implements SPI master mode using the QSPI controller.
+
+# Motorola MPC (PPC)
+config SPI_MPC
+ tristate "MPC SPI master"
+ depends on PPC && MPC
+ help
+ This implements SPI master mode using the MPC SPI controller.
+
+# TI OMAP (ARM)
+config SPI_OMAP
+ tristate "OMAP SPI controller as master"
+ depends on ARCH_OMAP
+ help
+ This implements SPI master mode using the dedicated SPI
+ controller.
+
+config SPI_OMAP_MCBSP
+ tristate "OMAP MCBSP as SPI master"
+ depends on ARCH_OMAP
+ help
+ This implements master mode SPI using a McBSP controller.
+
+config SPI_OMAP_UWIRE
+ tristate "OMAP MicroWire as SPI master"
+ depends on ARCH_OMAP
+ select OMAP_UWIRE
+ help
+ This implements master mode SPI using the MicroWire controller.
+
+config SPI_OMAP_MMC
+ tristate "OMAP MMC as SPI master"
+ depends on ARCH_OMAP
+ help
+ This implements master mode SPI using an MMC controller.
+
+
+# Intel PXA (ARM)
+config SPI_PXA_SSP
+ tristate "PXA SSP controller as SPI master"
+ depends on ARCH_PXA
+ help
+ This implements SPI master mode using an SSP controller.
+
+config SPI_PXA_NSSP
+ tristate "PXA NSSP (or ASSP) controller as SPI master"
+ depends on ARCH_PXA
+ help
+ This implements SPI master mode using the NSSP controller, which
+ is available with PXA 255 and PXA 26x processors.
+
+config SPI_PXA_MMC
+ tristate "PXA MMC as SPI master"
+ depends on ARCH_PXA
+ help
+ This implements master mode SPI using an MMC controller.
+
+
+#
+# Add new SPI master controllers in alphabetical order above this line
+#
+
+
+#
+# There are lots of SPI device types, with sensors and memory
+# being probably the most widely used ones.
+#
+comment "SPI Protocol Masters"
+
+config SPI_ADS7846
+ tristate "ADS 7846 touchscreen and sensors"
+ help
+ This chip has touchscreen, voltage, and temperature sensors.
+ The driver packages the touchscreen as an input device,
+ and publishes the sensor values in sysfs.
+
+config SPI_DATAFLASH
+ tristate "DataFlash -- MTD support"
+ depends on MTD
+ help
+ This driver provides an MTD interface to Atmel's DataFlash.
+ It probes the device to see what size chip is present.
+ Some MCUs can boot directly from DataFlash.
+
+config SPI_EEPROM
+ tristate "EEPROM -- sysfs support"
+ depends on SYSFS
+ help
+ This provides a read/write "eeprom" file in sysfs for the
+ specified device. Platform data identifies the EEPROM type.
+
+config SPI_ETHERNET
+ tristate "Ethernet Master"
+ depends on NET
+ help
+ Uses SPI to exchange Ethernet packets with some SPI
+ protocol slave. No Ethernet adapter hardware is used.
+
+config SPI_FLASH
+ tristate "FLASH -- MTD support"
+ depends on MTD
+ help
+ This provides an MTD interface to SPI flash chips like the
+ AT25 or M25 series. Platform data identifies the FLASH type.
+
+config SPI_M41T94
+ tristate "M41T94 RTC/WDT support"
+ help
+ This supports the ST M41T94 chips, providing the conventional
+ miscdev files for the real-time clock and watchdog timer.
+
+config SPI_MCP320x
+ tristate "MPC 320x ADC support"
+ help
+ These are multi-channel 12-bit A/D converters with sample-and-hold.
+
+config SPI_TC77
+ tristate "TC77 temperature sensor"
+ depends on SENSORS
+ help
+ This is a 12-bit (plus sign) temperature sensor.
+
+config SPI_TSC2101
+ tristate "TSC 2101 touchscreen, sensors, and audio"
+ help
+ This chip has touchscreen, voltage, and temperature sensors
+ along with audio i/O for microphone, headphone, and speaker.
+ The driver packages the touchscreen as an input device,
+ publishes the sensor values in sysfs, and packages the audio
+ through the ALSA driver stack.
+
+# USB: n9604_udc, cy7c67200_hcd, ...
+
+#
+# Add new SPI protocol masters in alphabetical order above this line
+#
+
+endif # "SPI Master Implementations"
+
+
+#
+# SLAVE side ... widely implemented on microcontrollers,
+# but Linux-capable hardware often supports this too
+#
+comment "No SPI slave hardware is available."
+ depends on SPI && !SPI_ARCH_HAS_SLAVE
+
+menuconfig SPI_SLAVE
+ boolean "SPI Slave support"
+ depends on SPI && SPI_ARCH_HAS_SLAVE
+ help
+ If your system has a slave-capable SPI controller (driven
+ by a master when it provides chipselect and clock), you can
+ enable drivers for that controller and for the SPI protocol
+ slave functions you're implementing.
+
+if SPI_SLAVE
+
+comment "SPI Slave Controller Drivers"
+
+config SPI_AT91_SLAVE
+ tristate "AT91 as SPI slave"
+ depends on ARCH_AT91
+ help
+ This implements SPI slave mode using an SPI controller.
+
+config SPI_OMAP_SLAVE
+ tristate "OMAP SPI controller as slave"
+ depends on ARCH_OMAP
+ help
+ This implements SPI slave mode using the dedicated SPI
+ controller.
+
+config SPI_OMAP_MCBSP_SLAVE
+ tristate "OMAP MCBSP as SPI slave"
+ depends on ARCH_OMAP
+ help
+ This implements slave mode SPI using a McBSP controller.
+
+config SPI_PXA_SSP_SLAVE
+ tristate "PXA SSP controller as SPI slave"
+ depends on ARCH_PXA
+ help
+ This implements SPI slave mode using an SSP controller.
+
+config SPI_PXA_NSSP_SLAVE
+ tristate "PXA NSSP (or ASSP) controller as SPI slave"
+ depends on ARCH_PXA
+ help
+ This implements SPI slave mode using the NSSP controller, which
+ is available with PXA 255 and PXA 26x processors.
+
+#
+# Add new SPI slave controllers in alphabetical order above this line
+#
+
+
+comment "SPI Protocol Slaves"
+
+config SPI_ETHERNET_SLAVE
+ tristate "Ethernet Slave"
+ depends on NET
+ help
+ Uses SPI to exchange Ethernet packets with some SPI
+ protocol master.
+
+#
+# Add new SPI protocol slaves in alphabetical order above this line
+#
+
+endif # "SPI Slave Implementations"
+
+endmenu # "SPI support"
+
--- osk.orig/arch/arm/Kconfig 2005-09-09 16:53:17.000000000 -0700
+++ osk/arch/arm/Kconfig 2005-09-09 16:53:47.000000000 -0700
@@ -746,6 +746,8 @@ source "drivers/char/Kconfig"

source "drivers/i2c/Kconfig"

+source "drivers/spi/Kconfig"
+
source "drivers/hwmon/Kconfig"

#source "drivers/l3/Kconfig"
--- osk.orig/drivers/Kconfig 2005-09-09 16:53:17.000000000 -0700
+++ osk/drivers/Kconfig 2005-09-09 16:53:47.000000000 -0700
@@ -42,6 +42,8 @@ source "drivers/char/Kconfig"

source "drivers/i2c/Kconfig"

+source "drivers/spi/Kconfig"
+
source "drivers/w1/Kconfig"

source "drivers/hwmon/Kconfig"
--- osk.orig/drivers/Makefile 2005-09-09 16:53:17.000000000 -0700
+++ osk/drivers/Makefile 2005-09-09 16:53:47.000000000 -0700
@@ -29,6 +29,7 @@ obj-$(CONFIG_PARPORT) += parport/
obj-y += base/ block/ misc/ net/
obj-$(CONFIG_I2C) += i2c/
obj-y += media/ ssi/
+obj-$(CONFIG_SPI) += spi/
obj-$(CONFIG_NUBUS) += nubus/
obj-$(CONFIG_ATM) += atm/
obj-$(CONFIG_PPC_PMAC) += macintosh/
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ osk/drivers/spi/Makefile 2005-09-09 16:53:47.000000000 -0700
@@ -0,0 +1,34 @@
+#
+# Makefile for kernel SPI drivers.
+#
+
+ifeq ($(CONFIG_SPI_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
+
+# small core, mostly translating board-specific
+# config declarations into driver model code
+obj-$(CONFIG_SPI_MASTER) += spi.o
+
+# STUBBED IMPLEMENTATION
+obj-$(CONFIG_SPI_EXAMPLE) += spi_plat_example.o
+
+# SPI master controller drivers (bus)
+#obj-$(CONFIG_SPI_BITBANG) += spi_bitbang.o
+#obj-$(CONFIG_SPI_OMAP) += omap_spi.o
+#obj-$(CONFIG_SPI_OMAP_MMC) += omap_spi_mmc.o
+#obj-$(CONFIG_SPI_PXA_SSP) += pxa_ssp.o
+#obj-$(CONFIG_SPI_PXA_MMC) += pxa_spi_mmc.o
+
+# SPI protocol drivers (device/link on bus)
+# obj-$(CONFIG_SPI_DATAFLASH) += dataflash.o
+# obj-$(CONFIG_SPI_EEPROM) += at25c.o
+# obj-$(CONFIG_SPI_ETHERNET) += spi_ether.o
+obj-$(CONFIG_SPI_ADS7846) += ads7846.o
+
+# SPI slave controller drivers (upstream link)
+#obj-$(CONFIG_SPI_OMAP_SLAVE) += omap_spi_slave.o
+#obj-$(CONFIG_SPI_PXA_SSP_SLAVE) += pxa_ssp_slave.o
+
+# SPI slave drivers (protocol for that link)
+# obj-$(CONFIG_SPI_ETHERNET_SLAVE) += spi_ether_slave.o

2005-09-16 17:55:45

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> wrote:

> > ASCII art time :)
>
> ASCII also works for English and C, but this is good
> too. ;)
>
>
>
> > SPI device drivers| Core | Adapter driver
>
> Or in my terminology, with no core/midlayer
> expectation:
>
> SPI Master | SPI Master
> Protocol Driver | Controller Driver
>
> > ---------- msg | |
> > | EEPROM |------> |- |
> > ---------- | | |
> > ------------ msg | | ------- | msg ------
> > | ETHERNET |----> |-->|Queue|->|---->|xfer|
> > ------------ | | ------- | ------
> > --------- msg | | ^ | |
> > | FLASH |-------> |- ------|<------- Next/done
> > --------- | |
>
> And the workqueue usage you're describing (to manage
> that queue)
> is what I'd call just one of many implementation
> strategies; one
> that wouldn't be universally appropriate.
>
> You don't NEED a separate "core" to hold the queue;
> the driver
> can just as easily manage a list_head itself.
>
> - A PIO driver running with IRQs blocked wouldn't
> need a queue.
> (And that might be fine for low volume usage,
> since it might
> take all of an hour to write and debug.)
>
> - A DMA driver might build the queue out of DMA
> descriptors, so
> that the hardware would process many
> transactions back-to-back
> without needing intermediate attention from the
> CPU. (Great
> for high volume usage, but likely takes more
> than an hour.)
>
> You don't NEED to allocate a task ("workqueue") to
> handle the
> movement of requests from the queue to the hardware.
>
> - Drivers are often written so that the "next"
> transaction is
> started from an IRQ handler ... it's a lot less
> I/O latency
> than if you need the workqueue's task to
> schedule before that
> transaction can start, and better throughput
> too.
>
> That's three more implementation strategies ... ones
> that don't need
> or want to have a "workqueue" task, and can easily
> manage a list_head.
> But you want them all to not just have a workqueue,
> but not be able
> to work unless they interact with it!! Why?
>

Thank you! I have seen the light. Yes, I can see now
that the workqueue should really be in my adapter
driver not the core. The only thing about having a
work queue in the core is that it means adapter
drivers don't have to do the sleep for blocking
transfers, but I guess that's just laziness creeping
in ;).
Thinking about it, for blocking transfers the core
could call the adapters transfer routine and then
start a wait on completeion. When the message has been
sent the adapter would finish the completeion and the
call to the core would then return (I think this is
how the mmc core layer does it). How do you feel about
that sugguestion?

How would you feel about having a list head for
messages in the adapter structure? I think every
adapter driver would at least need this.

>
>
> > If you don't have a queue of some sort how do you
> > handle the fact that one or more devices will want
> to
> > send asynchronous messages through one adapter
> while
> > that adapter is still busy transferring a previous
> > message?
>
> You seem to have been replying to someone else's
> posts! The
> examples I gave _included_ queues. Queues that
> advanced using
> IRQs, and didn't need to schedule a workqueue's task
> before
> starting another transaction.
>

Sorry I must have had a blind spot :(.

>
> > OK. But you also need to see what else is in the
> cs
> > table, namely the default level of the cs's. The
> issue
> > which we have to solve is that all the cs's have
> to be
> > put into their non-active state before any
> transfer
> > can be done.
>
> If you observed the code I sent by, you'll observe
> comments about that
> issue in the declaration of both "spi_device" and
> "spi_board_info".
>
> There are other protocol tweaks to be dealt with
> over time too. As one
> person commented off-line, there are multiple
> protocols to fit into this
> API (SPI, MicroWire, SSI, SSP, and more) that differ
> in only minor ways.
> The controller driver may need to know about some of
> those tweaks in
> order to talk properly with a given chip.
>

Yes, at some time I was thinking about how you might
create a serial bus subsystem that would even
incorporate I2C :O.

>
> > If devices are added into a running SPI
> > adapter (I mean registration of devices not
> physical
> > plugging) then how do you know what their idle
> state
> > is at the beginning (when the first SPI device
> does a
> > transfer?
>
> Until someone has to hook up hardware that acts
> atypically, it's the
> normal SPI convention: chipselect is "active low".
> When someone
> needs that, I've already identified where to record
> "active high".
>
>
> > To me the only solution seems to be to pass
> > the idle state of all the devices that will be on
> that
> > device (be they hardwire, plugged or what ever)
> when
> > then adapter gets registered which is why I put it
> in
> > as part of platform data.
>
> Much like I did with "spi_board_info", but on a
> per-chip basis. It's
> possible to hook an "active low" chip to one
> chipselect while another
> uses "active high" signaling. It's just an
> inverter. :)
>
>
>
> > > I can certainly understand how, say, Philips
> might
> > > want to support
> > > evaluation boards in PC environments. It can
> be
> > > easier to debug on
> > > PCs; not everyone has JTAG tools; and so on.
> > >
> >
> > That's my thinking. Plus I was planning on writing
> a
> > parallel port bit-banging adapter as a sweetnear
> for
> > the PC Linux folk and later a SPI MMC driver so
> your
> > PC could have a MMC slot :) (even old 386's &
> 486's
> > which don't have USB and thus no card readers!).
>
> Yes, I expect an SPI bitbanger will be useful to
> some folk.
> As for MMC ... it'll be interesting to watch that
> play out;
> won't the mmc_block code need to change?
>

I don't know, I would hope not. If the mmc core is
completely generic then I think I should only have to
write a driver like mmci and not have to change the
mmc_block or mmc core layers.

>
> I attach the latest snapshot of my code. You'll
> notice two changes:
> (a) suspend/resume calls; this code seems pretty
> complete except for
> the protocol tweaking options; (b) a new method
> that'll be handy
> when doing things like hotplugging a card with an
> SPI controller
> and a few soldered-on SPI devices. (Like that USB
> prototype.)

OK. I'll have a look at this and send another reply.

Mark

>
> - Dave
>
>
> ===========================
>
> This is the start of a small SPI framework that
> started fresh, so that
> doesn't perpetuate the "i2c driver model mess".
>
> - It's still less than 2KB of ".text" (ARM,
> modules enabled). If there
> must be more code than the drivers, that's the
> right size budget. :)
>
> - The guts use board-specific SPI device tables to
> build the driver
> model tree. (Hardware probing is rarely an
> option.)
>
> - The Kconfig should be informative about the
> scope and content of SPI.
>
> - Building more drivers into this framework likely
> means updating the
> I/O "async message" model to include protocol
> tweaking (like I2C).
>
> - No userspace API. There are several
> implementations to compare.
> Implement them like any other driver; no magic
> hooks!
>
> This should suffice for writing (or adapting) real
> driver code, such
> as for SPI master controllers (including
> hotpluggable ones like a
> parport bitbanger) or protocol masters.
>
>
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ osk/include/linux/spi.h 2005-09-15
> 18:08:00.293267631 -0700
> @@ -0,0 +1,268 @@
> +/*
> + * Copyright (C) 2005 David Brownell
> + *
> + * This program is free software; you can
> redistribute it and/or modify
> + * it under the terms of the GNU General Public
> License as published by
> + * the Free Software Foundation; either version 2
> of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it
> will be useful,
> + * but WITHOUT ANY WARRANTY; without even the
> implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR
> PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU
> General Public License
> + * along with this program; if not, write to the
> Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA
> 02139, USA.
> + */
> +
> +#ifndef __LINUX_SPI_H
> +#define __LINUX_SPI_H
> +
> +/*
> + * PROTOTYPE !!!
> + *
> + * The focus is on driver model support ... enough
> for SPI mastering
> + * board setups to work. The I/O model still needs
> attention, since
> + * SPI protocols seem to need to tweaking.
> + */
> +
>
+/*---------------------------------------------------------------------------*/
> +
> +/*
> + * INTERFACES between SPI master drivers and
> infrastructure
> + *
> + * There are two types of master (or slave) driver:
> "controller" drivers
> + * usually work on platform devices and touch chip
> registers; "protocol"
> + * drivers work on abstract SPI devices by asking a
> controller driver to
> + * transfer the relevant data, and they shouldn't
> much care if they use
> + * one controller or another.
> + *
> + * A "struct device_driver" for an SPI device uses
> "spi_bus_type" and needs
> + * no special API wrappers (much like
> platform_bus). These drivers are
> + * bound to devices based on their names (much like
> platform_bus), and
> + * are available in dev->driver.
> + */
> +extern struct bus_type spi_bus_type;
> +
> +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)
> +
> + // MAYBE ADD hooks for protocol options that
> affect how
> + // the controller talks to its chips:
> + // - chipselect polarity (default active low)
> + // - spi wordsize (default 8 bits unsigned)
> + // - bit order (default is wordwise msb-first)
> + // - memory packing (default: 12 bit samples into
> 16 bits, msb)
> + // - priority
> + // - delays
> + // - ...
> +
> +};
> +
> +static inline struct spi_device
> *to_spi_device(struct device *dev)
> +{
> + return container_of(dev, struct spi_device, dev);
> +}
> +
> +
> +struct spi_message;
> +
> +struct spi_master {
> + struct class_device cdev;
> +
> + /* other than zero (== assign one dynamically),
> bus_num is fully
> + * board-specific. usually that simplifies to
> being SOC-specific.
> + * for example: one SOC has three SPI
> controllers, numbered 1..3,
> + * and one board's schematics might show it using
> SPI-2. software
> + * would normally use bus_num=2 for that
> controller.
> + */
> + u16 bus_num;
> +
> + /* chipselects will be integral to many
> controllers, while others
> + * might use board-specific GPIOs.
> + */
> + u16 num_chipselect;
> +
> + /* setup mode and clock, etc */
> + int (*setup)(struct spi_device *spi);
> +
> + /* bidirectional bulk transfers
> + * + transfer() may not sleep
> + * + to a given spi_device, message queueing is
> pure fifo
> + * + if there are multiple spi_device children,
> the i/o queue
> + * arbitration algorithm is unspecified (round
> robin, fifo,
> + * priority, reservations, preemption, etc)
> + * + the master's main job is to process its
> message queue,
> + * selecting a chip then transferring data
> + * + for now there's no remove-from-queue
> operation, or
> + * any other request management
> + */
> + int (*transfer)(struct spi_device *spi,
> + struct spi_message *mesg);
> +};
> +
> +/* the controller driver manages memory for the
> spi_master classdev,
> + * normally contained inside its associated private
> state.
> + */
> +extern int spi_register_master(struct device *host,
> struct spi_master *spi);
> +extern void spi_unregister_master(struct spi_master
> *spi);
> +
> +
>
+/*---------------------------------------------------------------------------*/
> +
> +/*
> + * I/O INTERFACE between SPI controller and
> protocol drivers
> + *
> + * The interface is a queue of spi_messages, each
> transferring data
> + * between the controller and memory buffers. SPI
> protocol drivers just
> + * provide spi_messages to hardware controller
> drivers. SPI controller
> + * drivers process that queue, issuing completion
> callbacks as appropriate.
> + *
> + * The spi_messages themselves consist of a series
> of read+write transfer
> + * segments. Those segments always read the same
> number of bits as they
> + * write; but one or the other is easily ignored by
> passing a null buffer
> + * pointer. (This is unlike most types of I/O API,
> because SPI hardware
> + * is full duplex.)
> + *
> + * NOTE: Allocation of spi_transfer and
> spi_message memory is entirely
> + * up to the protocol driver, which guarantees the
> integrity of both (as
> + * well as the data buffers) for as long as the
> message is queued.
> + */
> +
> +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;
> +};
> +
> +/* spi messages will often be stack-allocated */
> +struct spi_message {
> + struct spi_transfer *transfers;
> + unsigned n_transfer;
> +
> + struct spi_device *dev;
> +
> + /* Optionally leave this chipselect active
> afterward */
> + unsigned csrel_disable:1;
> +
> + /* 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;
> +};
> +
> +/**
> + * spi_setup -- setup SPI mode and clock rate
> + * @spi: the device whose settings are being
> modified
> + *
> + * SPI protocol drivers may need to update the
> transfer mode if the
> + * device doesn't work with the mode 0 default.
> They may likewise need
> + * to update clock rates. This function changes
> those settings,
> + * and must be called from a context that can
> sleep.
> + */
> +static inline int
> +spi_setup(struct spi_device *spi)
> +{
> + return spi->master->setup(spi);
> +}
> +
> +
> +/* synchronous SPI transfers; these may sleep
> uninterruptibly */
> +extern int spi_sync(struct spi_device *spi, struct
> spi_message *message);
> +extern int spi_w8r8(struct spi_device *spi, u8
> cmd);
> +extern int spi_w8r16(struct spi_device *spi, u8
> cmd);
> +
> +
> +/**
> + * spi_async -- asynchronous SPI transfer
> + * @spi: device with which data will be exchanged
> + * @message: describes the data transfers,
> including completion callback
> + *
> + * This call may be used in_irq in other contexts
> which can't sleep,
> + * as well as from task contexts which can sleep.
> + *
> + * The completion callback is invoked in a context
> which can't sleep.
> + * Before that invocation, the value of
> message->status is undefined.
> + * When the callback is issue, message->status
> holds either zero (to
> + * indicate complete success) or a negative error
> code.
> + */
> +static inline int
> +spi_async(struct spi_device *spi, struct
> spi_message *message)
> +{
> + message->dev = spi;
> + return spi->master->transfer(spi, message);
> +}
> +
>
+/*---------------------------------------------------------------------------*/
> +
> +/*
> + * INTERFACE between board init code and SPI
> infrastructure.
> + *
> + * No SPI driver ever sees these SPI device table
> segments, but
> + * it's how the SPI core (or adapters that get
> hotplugged) grows
> + * the driver model tree.
> + */
> +
> +/* board-specific information about each SPI device
> */
> +struct spi_board_info {
> + /* the device name and module name are coupled,
> like platform_bus.
> + *
> + * platform_data has things like IRQ assignments
> (just pass the int
> + * that goes to request_irq), related GPIOs, and
> so on.
> + */
> + char modalias[KOBJ_NAME_LEN];
> + void *platform_data;
> +
> + /* slower signaling on noisy or low voltage boards
> */
> + u32 max_speed_hz;
> +
> + /* bus_num is board specific and matches the
> bus_num of some
> + * spi_master that will probably be registered
> later.
> + *
> + * chip_select reflects how this chip is wired to
> that master;
> + * it's less than num_chipselect.
> + */
> + u16 bus_num;
> + u16 chip_select;
> +
> + /* ... may need additional spi_device chip config
> data here.
> + * avoid stuff protocol drivers can set; but
> include stuff
> + * needed to behave without being bound to a
> driver:
> + * - chipselect polarity
> + * - quirks like clock rate mattering when not
> selected
> + */
> +};
> +
> +extern int
> +spi_register_board_info(struct spi_board_info const
> *info, unsigned n);
> +
> +extern struct spi_device *
> +spi_new_device(struct spi_master *, struct
> spi_board_info *);
> +
> +static inline void
> +spi_unregister_device(struct spi_device *spi)
> +{
> + if (spi)
> + device_unregister(&spi->dev);
> +}
> +
> +
> +#endif /* __LINUX_SPI_H */
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ osk/drivers/spi/spi.c 2005-09-15
> 17:22:46.104517401 -0700
> @@ -0,0 +1,482 @@
> +/* spi.c -- prototype of SPI init/core code
> + *
> + * Copyright (C) 2005 David Brownell
> + *
> + * This program is free software; you can
> redistribute it and/or modify
> + * it under the terms of the GNU General Public
> License as published by
> + * the Free Software Foundation; either version 2
> of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it
> will be useful,
> + * but WITHOUT ANY WARRANTY; without even the
> implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR
> PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU
> General Public License
> + * along with this program; if not, write to the
> Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA
> 02139, USA.
> + */
> +
> +#include <linux/autoconf.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/spi.h>
> +
> +
> +/* SPI bustype and spi_master class are registered
> during early boot,
> + * usually after board init code provided the SPI
> device tables, and
> + * are available when driver init code needs them.
> + *
> + * Drivers for SPI devices are like those for
> platform bus devices:
> + * (a) no bus-specific API wrappers (== needless
> bloat here)
> + * (b) matched to devices using device names
> + * (c) should support "native" suspend and resume
> methods
> + */
> +static void spidev_release(struct device *dev)
> +{
> + const struct spi_device *spi = to_spi_device(dev);
> +
> + class_device_put(&spi->master->cdev);
> + kfree(dev);
> +}
> +
> +// no, we probably don't need to list speed and
> mode in sysfs...
> +
> +static ssize_t
> +modalias_show(struct device *dev, struct
> device_attribute *a, char *buf)
> +{
> + const char *modalias = strchr(dev->bus_id, '-') +
> 1;
> +
> + return snprintf(buf, BUS_ID_SIZE + 1, "%s\n",
> modalias);
> +}
> +
> +#ifdef DEBUG
> +
> +static ssize_t
> +maxspeed_show(struct device *dev, struct
> device_attribute *a, char *buf)
> +{
> + const struct spi_device *spi = to_spi_device(dev);
> +
> + return sprintf(buf, "%u\n", spi->max_speed_hz);
> +}
> +
> +static ssize_t
> +mode_show(struct device *dev, struct
> device_attribute *a, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> +
> + return sprintf(buf, "%u\n", spi->mode);
> +}
> +
> +#endif /* DEBUG */
> +
> +static struct device_attribute spi_dev_attrs[] = {
> + __ATTR_RO(modalias),
> +#ifdef DEBUG
> + __ATTR_RO(maxspeed),
> + __ATTR_RO(mode),
> +#endif /* DEBUG */
> + __ATTR_NULL,
> +};
> +
> +static int spi_match_device(struct device *dev,
> struct device_driver *drv)
> +{
> + const char *modalias = strchr(dev->bus_id, '-') +
> 1;
> +
> + return strncmp(modalias, drv->name, BUS_ID_SIZE)
> == 0;
> +}
> +
> +static int spi_hotplug(struct device *dev, char
> **envp, int num_envp,
> + char *buffer, int buffer_size)
> +{
> + const char *modalias = strchr(dev->bus_id, '-') +
> 1;
> +
> + envp[0] = buffer;
> + snprintf(buffer, buffer_size, "MODALIAS=%s",
> modalias);
> + envp[1] = NULL;
> + return 0;
> +}
> +
> +/* suspend/resume in "struct device_driver" don't
> really need that
> + * strange third parameter, so we just make it a
> constant and expect
> + * drivers to ignore it.
> + */
> +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;
> +}
> +
> +static int spi_resume(struct device *dev)
> +{
> + if (dev->driver && dev->driver->resume)
> + return dev->driver->resume(dev, RESUME_POWER_ON);
> + else
> + return 0;
> +}
> +
> +struct bus_type spi_bus_type = {
> + .name = "spi",
> + .dev_attrs = spi_dev_attrs,
> + .match = spi_match_device,
> + .hotplug = spi_hotplug,
> + .suspend = spi_suspend,
> + .resume = spi_resume,
> +};
> +EXPORT_SYMBOL_GPL(spi_bus_type);
> +
> +static struct class spi_master_class = {
> + .name = "spi_master",
> + .owner = THIS_MODULE,
> +};
> +
> +
>
+/*-------------------------------------------------------------------------*/
> +
> +/* SPI devices should normally not be created by
> SPI device drivers; that
> + * would make them board-specific. Similarly with
> SPI master drivers.
> + * Device registration normally goes into like
> arch/.../mach.../board-YYY.c
> + * with other information about mainboard devices.
> + */
> +
> +struct boardinfo {
> + struct list_head list;
> + unsigned n_board_info;
> + struct spi_board_info board_info[0];
> +};
> +
> +static LIST_HEAD(board_list);
> +static DECLARE_MUTEX(board_lock);
> +
> +#define kzalloc(n, flags) kcalloc(1,(n),(flags))
> +
> +static int __init_or_module
> +check_child(struct device *dev, void *data)
> +{
> + const struct spi_device *spi =
> to_spi_device(dev);
> + const struct spi_board_info *chip = data;
> +
> + return (spi->chip_select == chip->chip_select);
> +}
> +
> +
> +/* On typical mainboards, this is purely internal;
> and it's not needed
> + * after board init creates the hard-wired devices.
> Some development
> + * platforms may not be able to use
> spi_register_board_info though, and
> + * this is exported so that for example a USB or
> parport based adapter
> + * driver could add devices.
> + */
> +struct spi_device *__init_or_module
> +spi_new_device(struct spi_master *master, struct
> spi_board_info *chip)
> +{
> + struct spi_device *proxy;
> + struct device *dev = master->cdev.dev;
> + int status;
> +
> + /* NOTE: caller did any chip->bus_num checks
> necessary */
> +
> + /* only one child per chipselect, ever */
> + if (device_for_each_child(dev, chip, check_child))
> + return NULL;
> +
> + if (!class_device_get(&master->cdev))
> + return NULL;
> +
> + proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
> + if (!proxy) {
> + dev_err(dev, "can't alloc dev for cs%d\n",
> + chip->chip_select);
> + goto fail;
> + }
> + proxy->master = master;
> + proxy->chip_select = chip->chip_select;
> + proxy->max_speed_hz = chip->max_speed_hz;
> +
> + snprintf(proxy->dev.bus_id, sizeof
> proxy->dev.bus_id,
> + "%s.%u-%s", master->cdev.class_id,
> + chip->chip_select, chip->modalias);
> + proxy->dev.parent = dev;
> + proxy->dev.bus = &spi_bus_type;
> + proxy->dev.platform_data = chip->platform_data;
> + proxy->dev.release = spidev_release;
> +
> + /* drivers may modify this default i/o setup */
> + status = master->setup(proxy);
> + if (status < 0) {
> + dev_dbg(dev, "can't %s %s, status %d\n",
> + "setup", proxy->dev.bus_id, status);
> + goto fail;
> + }
> +
> + status = device_register(&proxy->dev);
> + if (status < 0) {
> + dev_dbg(dev, "can't %s %s, status %d\n",
> + "add", proxy->dev.bus_id, status);
> +fail:
> + class_device_put(&master->cdev);
> + kfree(proxy);
> + return NULL;
> + }
> + dev_dbg(dev, "registered child %s\n",
> proxy->dev.bus_id);
> + return proxy;
> +}
> +EXPORT_SYMBOL_GPL(spi_new_device);
> +
> +/*
> + * Board-specific early init code calls this
> (probably during arch_initcall)
> + * with segments of the SPI device table. Any
> device nodes are created later,
> + * after the relevant parent SPI controller
> (bus_num) is defined. We keep
> + * this table of devices forever, so that reloading
> a controller driver will
> + * not make Linux forget about these hard-wired
> devices.
> + *
> + * Other code can also call this, e.g. a particular
> add-on board might provide
> + * SPI devices through its expansion connector, so
> code initializing that board
> + * would naturally declare its SPI devices.
> + *
> + * The board info passed can safely be __initdata
> ... but be careful of
> + * any embedded pointers (platform_data, etc),
> they're copied as-is.
> + */
> +int __init_or_module // would be __init except for
> SPI_EXAMPLE
> +spi_register_board_info(struct spi_board_info const
> *info, unsigned n)
> +{
> + struct boardinfo *bi;
> +
> + bi = kmalloc (sizeof (*bi) + n * sizeof (*info),
> GFP_KERNEL);
> + if (!bi)
> + return -ENOMEM;
> + bi->n_board_info = n;
> + memcpy(bi->board_info, info, n * sizeof (*info));
> +
> + down(&board_lock);
> + list_add_tail(&bi->list, &board_list);
> + up(&board_lock);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_register_board_info);
> +
> +static void __init_or_module
> +scan_boardinfo(struct spi_master *master)
> +{
> + struct boardinfo *bi;
> + struct device *dev = master->cdev.dev;
> +
> + down(&board_lock);
> + list_for_each_entry(bi, &board_list, list) {
> + struct spi_board_info *chip = bi->board_info;
> + unsigned n;
> +
> + for (n = bi->n_board_info; n > 0; n--, chip++) {
> + if (chip->bus_num != master->bus_num)
> + continue;
> + if (chip->chip_select >= master->num_chipselect)
> {
> + dev_dbg(dev, "cs%d > max %d\n",
> + chip->chip_select,
> + master->num_chipselect);
> + continue;
> + }
> + (void) spi_new_device(master, chip);
> + }
> + }
> + up(&board_lock);
> +}
> +
>
+/*-------------------------------------------------------------------------*/
> +
> +/**
> + * spi_register_master - register SPI master
> controller
> + * @dev: the controller
> + * @master: the master, with all uninitialized
> fields zeroed
> + *
> + * This call is used only by SPI master controller
> drivers, which are the
> + * only ones directly touching chip registers.
> + *
> + * SPI master controllers connect to their drivers
> using some non-SPI bus,
> + * such as the platform bus. The final stage of
> probe() in that code
> + * includes calling spi_register_master(), with
> memory managed by that
> + * controller, to hook up to this SPI bus glue.
> + *
> + * SPI controllers use board specific (often SOC
> specific) bus numbers,
> + * and board-specific addressing for SPI devices
> combines those numbers
> + * with chip select numbers. Since SPI does not
> directly support dynamic
> + * device identification, boards need configuration
> tables tellin which
> + * chip is at which address.
> + *
> + * This must be called from context that can sleep.
> It returns zero
> + * on success, else a negative error code.
> + */
> +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;
> + }
> +
> + /* convention: dynamically assigned bus IDs count
> down from the max */
> + if (master->bus_num == 0) {
> + master->bus_num = atomic_dec_return(&dyn_bus_id);
> + dev_dbg(dev, "spi%d, dynamic bus number\n",
> master->bus_num);
> + }
> +
> + /* ELSE: verify that the ID isn't in use already
> */
> +
> + master->cdev.class = &spi_master_class;
> + master->cdev.dev = get_device(dev);
> + class_set_devdata(&master->cdev, master);
> + snprintf(master->cdev.class_id, sizeof
> master->cdev.class_id,
> + "spi%u", master->bus_num);
> +
> + /* register the device, then userspace will see it
> */
> + status = class_device_register(&master->cdev);
> + if (status < 0) {
> + put_device(dev);
> + goto done;
> + }
> + dev_dbg(dev, "registered master %s\n",
> master->cdev.class_id);
> +
> + scan_boardinfo(master);
> + status = 0;
> +done:
> + return status;
> +}
> +EXPORT_SYMBOL_GPL(spi_register_master);
> +
> +
> +/**
> + * spi_unregister_master - unregister SPI master
> controller
> + * @master: the master being unregistered
> + *
> + * This call is used only by SPI master controller
> drivers, which are the
> + * only ones directly touching chip registers.
> + *
> + * This must be called from context that can sleep.
> + */
> +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);
> +
> +
>
+/*-------------------------------------------------------------------------*/
> +
> +static void spi_sync_complete(void *done)
> +{
> + complete(done);
> +}
> +
> +/**
> + * spi_sync - blocking/synchronous SPI data
> transfers
> + * @spi: device with which data will be exchanged
> + * @message: describes the data transfers
> + *
> + * This call may only be used from a context that
> may sleep.
> + * The sleep is non-interruptible, and has no
> timeout.
> + *
> + * The return value is a negative error code if the
> message could not be
> + * submitted, else zero. When the value is zero,
> then message->status is
> + * also defined: it's the completion code for the
> transfer, either zero
> + * or a negative error code from the controller
> driver.
> + */
> +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);
> +
> +/**
> + * 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.
> + */
> +int spi_w8r8(struct spi_device *spi, u8 cmd)
> +{
> + int status;
> + struct spi_message message;
> + struct spi_transfer x[2];
> + u8 result;
> +
> + x[0].tx_buf = &cmd;
> + x[0].rx_buf = NULL;
> + x[0].len = 1;
> +
> + x[1].tx_buf = NULL;
> + x[1].rx_buf = &result;
> + x[1].len = 1;
> +
> + /* do the i/o */
> + message.transfers = &x[0];
> + message.n_transfer = ARRAY_SIZE(x);
> + status = spi_sync(spi, &message);
> +
> + /* return negative errno or unsigned value */
> + return (status < 0) ? status : result;
> +}
> +EXPORT_SYMBOL(spi_w8r8);
> +
> +/**
> + * 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.
> + */
> +int spi_w8r16(struct spi_device *spi, u8 cmd)
> +{
> + int status;
> + struct spi_message message;
> + struct spi_transfer x[2];
> + u16 result;
> +
> + x[0].tx_buf = &cmd;
> + x[0].rx_buf = NULL;
> + x[0].len = 1;
> +
> + x[1].tx_buf = NULL;
> + x[1].rx_buf = &result;
> + x[1].len = 2;
> +
> + /* do the i/o */
> + message.transfers = &x[0];
> + message.n_transfer = ARRAY_SIZE(x);
> + status = spi_sync(spi, &message);
> +
> + /* return negative errno or unsigned value */
> + return (status < 0) ? status : result;
> +}
> +EXPORT_SYMBOL(spi_w8r16);
> +
>
+/*-------------------------------------------------------------------------*/
> +
> +static int __init spi_init(void)
> +{
> + bus_register(&spi_bus_type);
> + class_register(&spi_master_class);
> + return 0;
> +}
> +postcore_initcall(spi_init);
> +
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ osk/drivers/spi/Kconfig 2005-09-15
> 18:08:50.311703596 -0700
> @@ -0,0 +1,319 @@
> +#
> +# This is what Linux SPI might look support after
> adding drivers for
> +# some embedded SPI-capable hardware that's used
> with Linux.
> +#
> +# NOTE: over half of the SPI protocol masters
> mentioned actually exist,
> +# and sometimes even on 2.6. Few of the SPI master
> controller drivers
> +# exist. None of those drivers use the 2.6 driver
> model, and they all
> +# have different I/O models too.
> +#
> +
> +#
> +# SPI device configuration
> +#
> +menu "SPI support"
> +
> +config SPI
> + bool "SPI support"
> + depends on SPI_ARCH_HAS_MASTER ||
> SPI_ARCH_HAS_SLAVE
> + help
> + The "Serial Peripheral Interface" is a low level
> synchronous
> + protocol used to talk with sensors, eeprom and
> flash memory,
> + codecs and various other controller chips,
> analog to digital
> + (and d-to-a) converters, and more. MMC and SD
> cards can be
> + accessed using SPI protocol; and for DataFlash
> cards used in
> + MMC sockets, SPI must be used.
> +
> + Chips that support SPI can have data transfer
> rates up to several
> + tens of Mbit/sec, and the controllers often
> support DMA. Chips
> + are addressed with a controller and a
> chipselect. Most SPI
> + devices don't support dynamic device discovery;
> some are even
> + write-only or read-only.
> +
> + SPI is one of a family of similar protocols
> using a four wire
> + interface (select, clock, data in, data out)
> including Microwire
> + (half duplex), SSP, SSI, and PSP. This driver
> framework should
> + work with most such devices and controllers.
> +
> +config SPI_DEBUG
> + boolean "Debug support for SPI drivers"
> + depends on SPI && DEBUG_KERNEL
> + help
> + Say "yes" to enable debug messaging (like
> dev_dbg and pr_debug),
> + sysfs, and debugfs support in SPI controller and
> protocol drivers.
> +
> +# someday this stuff should be set using
> arch/CPU/PLATFORM/Kconfig
> +config SPI_ARCH_HAS_MASTER
> + boolean
> + default y if ARCH_OMAP
> + default y if ARCH_PXA
> + default y if ARCH_AT91
> + default y if X86 # DEVEL HACK
> +
> +config SPI_ARCH_HAS_SLAVE
> + boolean
> + default y if ARCH_OMAP
> + default y if ARCH_PXA
> + default y if ARCH_AT91
> + default y if X86 # DEVEL HACK
> +
> +#
> +# MASTER side ... talking to discrete SPI slave
> chips including microcontrollers
> +#
> +comment "No SPI master hardware is available."
> + depends on SPI && !SPI_ARCH_HAS_MASTER
> +
> +menuconfig SPI_MASTER
> + boolean "SPI Master support"
> + depends on SPI && SPI_ARCH_HAS_MASTER
> + help
> + If your system has an master-capable SPI
> controller (which
> + provides the clock and chipselect), you can
> enable that
> + controller and the protocol drivers for the SPI
> slave chips
> + that are connected.
> +
> +if SPI_MASTER
> +
> +comment "SPI Master Controller Drivers"
> +
> +config SPI_EXAMPLE
> + tristate "SPI Platform Example"
> + help
> + This just builds some sample code uses the core
> APIs to build
> + some SPI devices and plug in dummy controller
> drivers.
> +
> +# Atmel AT91rm9200 (and some other AT91 family
> chips)
> +config SPI_AT91
> + tristate "AT91 as SPI master"
> + depends on ARCH_AT91
> + help
> + This implements SPI master mode using an SPI
> controller.
> +
> +# FIXME bitbangers need arch-specific ways to
> access the
> +# right GPIO pins, probably using platform data and
> maybe
> +# using platform-specific minidrivers.
> +config SPI_BITBANG
> + tristate "Bitbanging SPI master"
> + help
> + You can implement SPI using GPIO pins, as this
> driver
> + eventually should do.
> +
> +# Motorola Coldfire (m68k)
> +config SPI_COLDFIRE
> + tristate "Coldfire QSPI as SPI master"
> + depends on COLDFIRE
> + help
> + This implements SPI master mode using the QSPI
> controller.
> +
> +# Motorola MPC (PPC)
> +config SPI_MPC
> + tristate "MPC SPI master"
> + depends on PPC && MPC
> + help
> + This implements SPI master mode using the MPC
> SPI controller.
> +
> +# TI OMAP (ARM)
> +config SPI_OMAP
> + tristate "OMAP SPI controller as master"
> + depends on ARCH_OMAP
> + help
> + This implements SPI master mode using the
> dedicated SPI
> + controller.
> +
> +config SPI_OMAP_MCBSP
> + tristate "OMAP MCBSP as SPI master"
> + depends on ARCH_OMAP
> + help
> + This implements master mode SPI using a McBSP
> controller.
> +
> +config SPI_OMAP_UWIRE
> + tristate "OMAP MicroWire as SPI master"
> + depends on ARCH_OMAP
> + select OMAP_UWIRE
> + help
> + This implements master mode SPI using the
> MicroWire controller.
> +
> +config SPI_OMAP_MMC
> + tristate "OMAP MMC as SPI master"
> + depends on ARCH_OMAP
> + help
> + This implements master mode SPI using an MMC
> controller.
> +
> +
> +# Intel PXA (ARM)
> +config SPI_PXA_SSP
> + tristate "PXA SSP controller as SPI master"
> + depends on ARCH_PXA
> + help
> + This implements SPI master mode using an SSP
> controller.
> +
> +config SPI_PXA_NSSP
> + tristate "PXA NSSP (or ASSP) controller as SPI
> master"
> + depends on ARCH_PXA
> + help
> + This implements SPI master mode using the NSSP
> controller, which
> + is available with PXA 255 and PXA 26x
> processors.
> +
> +config SPI_PXA_MMC
> + tristate "PXA MMC as SPI master"
> + depends on ARCH_PXA
> + help
> + This implements master mode SPI using an MMC
> controller.
> +
> +
> +#
> +# Add new SPI master controllers in alphabetical
> order above this line
> +#
> +
> +
> +#
> +# There are lots of SPI device types, with sensors
> and memory
> +# being probably the most widely used ones.
> +#
> +comment "SPI Protocol Masters"
> +
> +config SPI_ADS7846
> + tristate "ADS 7846 touchscreen and sensors"
> + help
> + This chip has touchscreen, voltage, and
> temperature sensors.
> + The driver packages the touchscreen as an input
> device,
> + and publishes the sensor values in sysfs.
> +
> +config SPI_DATAFLASH
> + tristate "DataFlash -- MTD support"
> + depends on MTD
> + help
> + This driver provides an MTD interface to Atmel's
> DataFlash.
> + It probes the device to see what size chip is
> present.
> + Some MCUs can boot directly from DataFlash.
> +
> +config SPI_EEPROM
> + tristate "EEPROM -- sysfs support"
> + depends on SYSFS
> + help
> + This provides a read/write "eeprom" file in
> sysfs for the
> + specified device. Platform data identifies the
> EEPROM type.
> +
> +config SPI_ETHERNET
> + tristate "Ethernet Master"
> + depends on NET
> + help
> + Uses SPI to exchange Ethernet packets with some
> SPI
> + protocol slave. No Ethernet adapter hardware is
> used.
> +
> +config SPI_FLASH
> + tristate "FLASH -- MTD support"
> + depends on MTD
> + help
> + This provides an MTD interface to SPI flash
> chips like the
> + AT25 or M25 series. Platform data identifies
> the FLASH type.
> +
> +config SPI_M41T94
> + tristate "M41T94 RTC/WDT support"
> + help
> + This supports the ST M41T94 chips, providing the
> conventional
> + miscdev files for the real-time clock and
> watchdog timer.
> +
> +config SPI_MCP320x
> + tristate "MPC 320x ADC support"
> + help
> + These are multi-channel 12-bit A/D converters
> with sample-and-hold.
> +
> +config SPI_TC77
> + tristate "TC77 temperature sensor"
> + depends on SENSORS
> + help
> + This is a 12-bit (plus sign) temperature sensor.
> +
> +config SPI_TSC2101
> + tristate "TSC 2101 touchscreen, sensors, and
> audio"
> + help
> + This chip has touchscreen, voltage, and
> temperature sensors
> + along with audio i/O for microphone, headphone,
> and speaker.
> + The driver packages the touchscreen as an input
> device,
> + publishes the sensor values in sysfs, and
> packages the audio
> + through the ALSA driver stack.
> +
> +# USB: n9604_udc, cy7c67200_hcd, ...
> +
> +#
> +# Add new SPI protocol masters in alphabetical
> order above this line
> +#
> +
> +endif # "SPI Master Implementations"
> +
> +
> +#
> +# SLAVE side ... widely implemented on
> microcontrollers,
> +# but Linux-capable hardware often supports this
> too
> +#
> +comment "No SPI slave hardware is available."
> + depends on SPI && !SPI_ARCH_HAS_SLAVE
> +
> +menuconfig SPI_SLAVE
> + boolean "SPI Slave support"
> + depends on SPI && SPI_ARCH_HAS_SLAVE
> + help
> + If your system has a slave-capable SPI
> controller (driven
> + by a master when it provides chipselect and
> clock), you can
> + enable drivers for that controller and for the
> SPI protocol
> + slave functions you're implementing.
> +
> +if SPI_SLAVE
> +
> +comment "SPI Slave Controller Drivers"
> +
> +config SPI_AT91_SLAVE
> + tristate "AT91 as SPI slave"
> + depends on ARCH_AT91
> + help
> + This implements SPI slave mode using an SPI
> controller.
> +
> +config SPI_OMAP_SLAVE
> + tristate "OMAP SPI controller as slave"
> + depends on ARCH_OMAP
> + help
> + This implements SPI slave mode using the
> dedicated SPI
> + controller.
> +
> +config SPI_OMAP_MCBSP_SLAVE
> + tristate "OMAP MCBSP as SPI slave"
> + depends on ARCH_OMAP
> + help
> + This implements slave mode SPI using a McBSP
> controller.
> +
> +config SPI_PXA_SSP_SLAVE
> + tristate "PXA SSP controller as SPI slave"
> + depends on ARCH_PXA
> + help
> + This implements SPI slave mode using an SSP
> controller.
> +
> +config SPI_PXA_NSSP_SLAVE
> + tristate "PXA NSSP (or ASSP) controller as SPI
> slave"
> + depends on ARCH_PXA
> + help
> + This implements SPI slave mode using the NSSP
> controller, which
> + is available with PXA 255 and PXA 26x
> processors.
> +
> +#
> +# Add new SPI slave controllers in alphabetical
> order above this line
> +#
> +
> +
> +comment "SPI Protocol Slaves"
> +
> +config SPI_ETHERNET_SLAVE
> + tristate "Ethernet Slave"
> + depends on NET
> + help
> + Uses SPI to exchange Ethernet packets with some
> SPI
> + protocol master.
> +
> +#
> +# Add new SPI protocol slaves in alphabetical order
> above this line
> +#
> +
> +endif # "SPI Slave Implementations"
> +
> +endmenu # "SPI support"
> +
> --- osk.orig/arch/arm/Kconfig 2005-09-09
> 16:53:17.000000000 -0700
> +++ osk/arch/arm/Kconfig 2005-09-09
> 16:53:47.000000000 -0700
> @@ -746,6 +746,8 @@ source "drivers/char/Kconfig"
>
> source "drivers/i2c/Kconfig"
>
> +source "drivers/spi/Kconfig"
> +
> source "drivers/hwmon/Kconfig"
>
> #source "drivers/l3/Kconfig"
> --- osk.orig/drivers/Kconfig 2005-09-09
> 16:53:17.000000000 -0700
> +++ osk/drivers/Kconfig 2005-09-09
> 16:53:47.000000000 -0700
> @@ -42,6 +42,8 @@ source "drivers/char/Kconfig"
>
> source "drivers/i2c/Kconfig"
>
> +source "drivers/spi/Kconfig"
> +
> source "drivers/w1/Kconfig"
>
> source "drivers/hwmon/Kconfig"
> --- osk.orig/drivers/Makefile 2005-09-09
> 16:53:17.000000000 -0700
> +++ osk/drivers/Makefile 2005-09-09
> 16:53:47.000000000 -0700
> @@ -29,6 +29,7 @@ obj-$(CONFIG_PARPORT) += parport/
> obj-y += base/ block/ misc/ net/
> obj-$(CONFIG_I2C) += i2c/
> obj-y += media/ ssi/
> +obj-$(CONFIG_SPI) += spi/
> obj-$(CONFIG_NUBUS) += nubus/
> obj-$(CONFIG_ATM) += atm/
> obj-$(CONFIG_PPC_PMAC) += macintosh/
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ osk/drivers/spi/Makefile 2005-09-09
> 16:53:47.000000000 -0700
> @@ -0,0 +1,34 @@
> +#
> +# Makefile for kernel SPI drivers.
> +#
> +
> +ifeq ($(CONFIG_SPI_DEBUG),y)
> +EXTRA_CFLAGS += -DDEBUG
> +endif
> +
> +# small core, mostly translating board-specific
> +# config declarations into driver model code
> +obj-$(CONFIG_SPI_MASTER) += spi.o
> +
> +# STUBBED IMPLEMENTATION
> +obj-$(CONFIG_SPI_EXAMPLE) += spi_plat_example.o
> +
> +# SPI master controller drivers (bus)
> +#obj-$(CONFIG_SPI_BITBANG) += spi_bitbang.o
> +#obj-$(CONFIG_SPI_OMAP) += omap_spi.o
> +#obj-$(CONFIG_SPI_OMAP_MMC) += omap_spi_mmc.o
> +#obj-$(CONFIG_SPI_PXA_SSP) += pxa_ssp.o
> +#obj-$(CONFIG_SPI_PXA_MMC) += pxa_spi_mmc.o
> +
> +# SPI protocol drivers (device/link on bus)
> +# obj-$(CONFIG_SPI_DATAFLASH) += dataflash.o
> +# obj-$(CONFIG_SPI_EEPROM) += at25c.o
> +# obj-$(CONFIG_SPI_ETHERNET) += spi_ether.o
> +obj-$(CONFIG_SPI_ADS7846) += ads7846.o
> +
> +# SPI slave controller drivers (upstream link)
> +#obj-$(CONFIG_SPI_OMAP_SLAVE) += omap_spi_slave.o
> +#obj-$(CONFIG_SPI_PXA_SSP_SLAVE) +=
> pxa_ssp_slave.o
> +
> +# SPI slave drivers (protocol for that link)
> +# obj-$(CONFIG_SPI_ETHERNET_SLAVE) +=
> spi_ether_slave.o
> -
> 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

2005-09-16 18:43:21

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> Thinking about it, for blocking transfers the core
> could call the adapters transfer routine and then
> start a wait on completeion. When the message has been
> sent the adapter would finish the completeion and the
> call to the core would then return (I think this is
> how the mmc core layer does it). How do you feel about
> that sugguestion?

It resembles some code in my patch, which you included in your reply.
I've deleted the other parts; see at the end of this message.

(By the way, you should trim down your email replies and stop re-wrapping
things to 56-character borders... it breaks attribution prefixes as well
as patches, and makes your posts hard to read.)


> How would you feel about having a list head for
> messages in the adapter structure? I think every
> adapter driver would at least need this.

It's just as simple to use:

struct my_spi_controller {
struct spi_master master;
struct list_head queue;
... register pointers
... and other controller-private state
};

I prefer the information hiding approach. In this case, no code
outside the controller driver ever has any business looking at that
queue; they shouldn't even be able to see it. That way there will
be no temptation to change it and break anything.


> > As for MMC ... it'll be interesting to watch that
> > play out; won't the mmc_block code need to change?
>
> I don't know, I would hope not. If the mmc core is
> completely generic then I think I should only have to
> write a driver like mmci and not have to change the
> mmc_block or mmc core layers.

I seem to recall MMC/SD card specs showing different commands are
used in SPI mode than MMC/SD mode. I'd be (pleasantly) surprised if
current mmc_block code already understood them. (As I recall, the
specs from SanDisk were pretty informative.)

- Dave


> > +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);

2005-09-18 14:45:25

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> wrote:
-= snip =-

> I attach the latest snapshot of my code. You'll notice two changes:
> (a) suspend/resume calls; this code seems pretty complete except for
> the protocol tweaking options; (b) a new method that'll be handy
> when doing things like hotplugging a card with an SPI controller
> and a few soldered-on SPI devices. (Like that USB prototype.)
>
> - Dave
>

-= snip =-

> +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.

-= snip =-

> +struct spi_master {
> + struct class_device cdev;
> +
> + /* other than zero (== assign one dynamically), bus_num is fully
> + * board-specific. usually that simplifies to being SOC-specific.
> + * for example: one SOC has three SPI controllers, numbered 1..3,
> + * and one board's schematics might show it using SPI-2. software
> + * would normally use bus_num=2 for that controller.
> + */
> + u16 bus_num;
> +
> + /* chipselects will be integral to many controllers, while others
> + * might use board-specific GPIOs.
> + */
> + u16 num_chipselect;
> +
> + /* setup mode and clock, etc */
> + int (*setup)(struct spi_device *spi);
> +
> + /* bidirectional bulk transfers
> + * + transfer() may not sleep
> + * + to a given spi_device, message queueing is pure fifo
> + * + if there are multiple spi_device children, the i/o queue
> + * arbitration algorithm is unspecified (round robin, fifo,
> + * priority, reservations, preemption, etc)
> + * + the master's main job is to process its message queue,
> + * selecting a chip then transferring data
> + * + for now there's no remove-from-queue operation, or
> + * any other request management
> + */
> + int (*transfer)(struct spi_device *spi,
> + struct spi_message *mesg);
> +};

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?

-= snip =-

> +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 :).

> +
> +/* spi messages will often be stack-allocated */
> +struct spi_message {
> + struct spi_transfer *transfers;
> + unsigned n_transfer;
> +
> + struct spi_device *dev;

If you call this element *dev might it not confuse people into thinking that this is for a device
structure? That's why I used sdev.

> +
> + /* 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?.

> +
> + /* 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?

> +};

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.

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).

> +
> +/**
> + * spi_setup -- setup SPI mode and clock rate
> + * @spi: the device whose settings are being modified
> + *
> + * SPI protocol drivers may need to update the transfer mode if the
> + * device doesn't work with the mode 0 default. They may likewise need
> + * to update clock rates. This function changes those settings,
> + * and must be called from a context that can sleep.
> + */
> +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.

> +
> +/* synchronous SPI transfers; these may sleep uninterruptibly */
> +extern int spi_sync(struct spi_device *spi, struct spi_message *message);
> +extern int spi_w8r8(struct spi_device *spi, u8 cmd);
> +extern int spi_w8r16(struct spi_device *spi, u8 cmd);
> +
> +
> +/**
> + * spi_async -- asynchronous SPI transfer
> + * @spi: device with which data will be exchanged
> + * @message: describes the data transfers, including completion callback
> + *
> + * This call may be used in_irq in other contexts which can't sleep,
> + * as well as from task contexts which can sleep.
> + *
> + * The completion callback is invoked in a context which can't sleep.
> + * Before that invocation, the value of message->status is undefined.
> + * When the callback is issue, message->status holds either zero (to
> + * indicate complete success) or a negative error code.
> + */
> +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).

-= snip =-

> +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).

-= snip =-

> +/* suspend/resume in "struct device_driver" don't really need that
> + * strange third parameter, so we just make it a constant and expect
> + * drivers to ignore it.
> + */
> +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;
> +}
> +
> +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?
Does the driver core suspend them for you? If so could you show me where because I missed it.

-= snip=-

> +struct spi_device *__init_or_module
> +spi_new_device(struct spi_master *master, struct spi_board_info *chip)
> +{
> + struct spi_device *proxy;
> + struct device *dev = master->cdev.dev;
> + int status;
> +
> + /* NOTE: caller did any chip->bus_num checks necessary */
> +
> + /* only one child per chipselect, ever */
> + if (device_for_each_child(dev, chip, check_child))
> + return NULL;
> +
> + if (!class_device_get(&master->cdev))
> + return NULL;
> +
> + proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
> + if (!proxy) {
> + dev_err(dev, "can't alloc dev for cs%d\n",
> + chip->chip_select);
> + goto fail;
> + }
> + proxy->master = master;
> + proxy->chip_select = chip->chip_select;
> + proxy->max_speed_hz = chip->max_speed_hz;
> +
> + snprintf(proxy->dev.bus_id, sizeof proxy->dev.bus_id,
> + "%s.%u-%s", master->cdev.class_id,
> + chip->chip_select, chip->modalias);
> + proxy->dev.parent = dev;
> + proxy->dev.bus = &spi_bus_type;
> + proxy->dev.platform_data = chip->platform_data;
> + proxy->dev.release = spidev_release;
> +
> + /* 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.
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.

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.

> + if (status < 0) {
> + dev_dbg(dev, "can't %s %s, status %d\n",
> + "setup", proxy->dev.bus_id, status);
> + goto fail;
> + }
> +
> + status = device_register(&proxy->dev);
> + if (status < 0) {
> + dev_dbg(dev, "can't %s %s, status %d\n",
> + "add", proxy->dev.bus_id, status);
> +fail:
> + class_device_put(&master->cdev);
> + kfree(proxy);
> + return NULL;
> + }
> + dev_dbg(dev, "registered child %s\n", proxy->dev.bus_id);
> + return proxy;
> +}
> +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.

-= snip =-

> +int __init_or_module // would be __init except for SPI_EXAMPLE
> +spi_register_board_info(struct spi_board_info const *info, unsigned n)
> +{
> + struct boardinfo *bi;
> +
> + bi = kmalloc (sizeof (*bi) + n * sizeof (*info), GFP_KERNEL);
> + if (!bi)
> + return -ENOMEM;
> + bi->n_board_info = n;
> + memcpy(bi->board_info, info, n * sizeof (*info));
> +
> + down(&board_lock);
> + list_add_tail(&bi->list, &board_list);
> + up(&board_lock);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_register_board_info);

This function should call scan_boardinfo as there may be devices in this list that sit on adapters
that have been registered already.

Please can we have a 'undo' version (the general rule being you should be able to undo what you
have done ;), i.e. 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.

-= snip =-

> +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.

-= snip =-

> +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.

-= snip =-

> +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.

> +
> +/**
> + * 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.
> + */
> +int spi_w8r8(struct spi_device *spi, u8 cmd)
> +{
> + int status;
> + struct spi_message message;
> + struct spi_transfer x[2];
> + u8 result;
> +
> + x[0].tx_buf = &cmd;
> + x[0].rx_buf = NULL;
> + x[0].len = 1;
> +
> + x[1].tx_buf = NULL;
> + x[1].rx_buf = &result;
> + x[1].len = 1;
> +
> + /* do the i/o */
> + message.transfers = &x[0];
> + message.n_transfer = ARRAY_SIZE(x);
> + status = spi_sync(spi, &message);
> +
> + /* return negative errno or unsigned value */
> + return (status < 0) ? status : result;
> +}
> +EXPORT_SYMBOL(spi_w8r8);
> +
> +/**
> + * 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.
> + */
> +int spi_w8r16(struct spi_device *spi, u8 cmd)
> +{
> + int status;
> + struct spi_message message;
> + struct spi_transfer x[2];
> + u16 result;
> +
> + x[0].tx_buf = &cmd;
> + x[0].rx_buf = NULL;
> + x[0].len = 1;
> +
> + x[1].tx_buf = NULL;
> + x[1].rx_buf = &result;
> + x[1].len = 2;
> +
> + /* do the i/o */
> + message.transfers = &x[0];
> + message.n_transfer = ARRAY_SIZE(x);
> + status = spi_sync(spi, &message);
> +
> + /* return negative errno or unsigned value */
> + return (status < 0) ? status : result;
> +}
> +EXPORT_SYMBOL(spi_w8r16);
> +

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?

Hmm, using local variables for messages, so DMA adapter drivers have to check if this is
non-kmalloc'ed space (how?) 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.

Mark






___________________________________________________________
Yahoo! Messenger - NEW crystal clear PC to PC calling worldwide with voicemail http://uk.messenger.yahoo.com

2005-09-26 16:49:40

by Vitaly Wool

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

Hi David,

David Brownell wrote:

>Most platform drivers I've seen just handle the power on/off
>requests. I think there's some historical reason that the
>"reason" stuff exists ... but I suspect not many folk would
>get unhappy if that were removed, and those calls got simplified.
>
>
No, I can't agree with you. This state machine provides useful framework
for drivers that work with DMA and/or drivers that set their own wakeup
bits etc. etc. It allows the driver essence to enter the state where it
doesn't accept any new requests but is able to successfully finish the
current ones.

Best regards,
Vitaly

2005-09-30 01:02:16

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

Sorry for the delay getting back to these comments; I wanted to
give them proper attention, which kept not arriving.


> 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.


> > +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".


> > +
> > + /* 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.

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!)


> > + /* 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.


> > +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.


> > +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.


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.


> > +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.

* 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?


> > +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) ...

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).


> > +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.


> > +/**
> > + * 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.

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().


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

- Dave


> Mark
>

2005-10-02 12:36:22

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- 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

2005-10-03 04:47:43

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> > > 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.


> 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.


> 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.


> > > > +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.



> 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.

- Dave

2005-10-03 10:57:49

by Mark Underwood

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem


--- David Brownell <[email protected]> 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 [email protected]
> 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

2005-10-03 11:07:51

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

On Mon, 2005-10-03 at 11:57 +0100, Mark Underwood wrote:

> > > > > 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

please NEVER EVER do dma from or to a stack variable. It's not allowed
on all architectures and it is really really bad practice in general
anyway.

2005-10-03 15:15:01

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> From: Arjan van de Ven <[email protected]>
> Date: Mon, 03 Oct 2005 13:07:36 +0200
>
> please NEVER EVER do dma from or to a stack variable. It's not allowed
> on all architectures and it is really really bad practice in general
> anyway.

Arjan, could you mention some Linuxes which don't allow it?

Every time the topic of DMA to/from stack comes up, the advice is
always to avoid it ... but so far as I recall, nobody's yet provided
us with an example where it actually doesn't work.

Failing such examples, it's normal to discount such dire warnings and
just plan to apply the relevant minor tweaks if/when they're needed.

- Dave


2005-10-03 15:23:32

by Russell King

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

On Mon, Oct 03, 2005 at 08:14:57AM -0700, David Brownell wrote:
> > From: Arjan van de Ven <[email protected]>
> > Date: Mon, 03 Oct 2005 13:07:36 +0200
> >
> > please NEVER EVER do dma from or to a stack variable. It's not allowed
> > on all architectures and it is really really bad practice in general
> > anyway.
>
> Arjan, could you mention some Linuxes which don't allow it?
>
> Every time the topic of DMA to/from stack comes up, the advice is
> always to avoid it ... but so far as I recall, nobody's yet provided
> us with an example where it actually doesn't work.
>
> Failing such examples, it's normal to discount such dire warnings and
> just plan to apply the relevant minor tweaks if/when they're needed.

I believe the issue is that you can't properly control the alignment
to ensure that you don't inadvertantly dirty the cache lines
corresponding with the memory you're performing DMA to/from.

Eg, on ARM, if the memory you're DMAing to/from is just above the
next stack location which will be used when you call a sub-function,
you'll have just undone the effects of the DMA API. Provided GCC
decided to arrange the stack that way.

Or maybe there's a local variable right next to the DMA region.
Same effect.

However, who knows how different gcc versions will lay out the
stack? That's completely up to the compilers whims.

So, if you want portable code and don't want random failures,
avoid DMA to/from the stack.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2005-10-03 15:40:23

by David Brownell

[permalink] [raw]
Subject: Re: [RFC][PATCH] SPI subsystem

> I believe the issue is that you can't properly control the alignment
> to ensure that you don't inadvertantly dirty the cache lines
> corresponding with the memory you're performing DMA to/from.

That's a much better (== content-ful) explanation; thanks Russell.

Cache line sharing can indeed be a PITA ... and while it's an issue
that's not unique to DMA from the stack, it's something that's less
manageable there. Plus, DMA isn't always cache-coherent. Meaning
that for example DMA to the stack might bypass the cache and then
later be overwritten by flushing cached stack updates. (etc.)

- Dave