2005-12-01 16:10:59

by Vitaly Wool

[permalink] [raw]
Subject: [PATCH 2.6-git] SPI core refresh

Greetings,

This is an updated version of SPI framework developed by Dmitry Pervushin and Vitaly Wool.

The main changes are:

- Matching rmk's 2.6.14-git5+ changes for device_driver suspend and resume calls
- Matching rmk's request to get rid of device_driver's calls to suspend/resume/probe/remove
- Matching Greg K-H's requests expressed in his review
- The character device interface was reworked
- No more redundant memcpy's (that should have been in the previous core, I don't know why it's missing there, nm now anyway :))

I still think that we need to continue converging with David Brownell's core, despite some misalignments happening in the email exchange :)
As a part of this convergence process, we took some fifteen minutes to port MTD dataflash driver posted by David to our core and it has really become smaller and more understandlable. This will be the subject of the next message, though.

Again, some advantages of our core compared to David's I'd like to mention

- it can be compiled as a module
- it is priority inversion-free
- it can gain more performance with multiple controllers
- it's more adapted for use in real-time environments
- it's not so lightweight, but it leaves less effort for the bus driver developer.

(As a response to the last bullet David claims that we limit the flexibility. It's not correct.
The core method for message retrieval is just a default one and can easily be overridden by the bus driver. It's a common practice, for instance, see MTD/NAND interface.)

It's also been proven to work on SPI EEPROMs and SPI WiFi module (the latter was a really good survival test! :)).

Signed-off-by: Vitaly Wool <[email protected]>
Signed-off-by: dmitry pervushin <[email protected]>

Documentation/spi.txt | 382 ++++++++++++++++++++++++++
arch/arm/Kconfig | 2
drivers/Kconfig | 2
drivers/Makefile | 1
drivers/spi/Kconfig | 30 ++
drivers/spi/Makefile | 15 +
drivers/spi/spi-core.c | 700 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/spi/spi-dev.c | 192 +++++++++++++
include/linux/spi.h | 422 +++++++++++++++++++++++++++++
9 files changed, 1746 insertions(+)

Index: lsm-2.6/arch/arm/Kconfig
===================================================================
--- lsm-2.6.orig/arch/arm/Kconfig
+++ lsm-2.6/arch/arm/Kconfig
@@ -738,6 +738,8 @@ source "drivers/usb/Kconfig"

source "drivers/mmc/Kconfig"

+source "drivers/spi/Kconfig"
+
endmenu

source "fs/Kconfig"
Index: lsm-2.6/Documentation/spi.txt
===================================================================
--- /dev/null
+++ lsm-2.6/Documentation/spi.txt
@@ -0,0 +1,382 @@
+Documentation/spi.txt
+========================================================
+Table of contents
+1. Introduction -- what is SPI ?
+2. Purposes of this code
+3. SPI devices stack
+3.1 SPI outline
+3.2 How the SPI devices gets discovered and probed ?
+3.3 DMA and SPI messages
+4. SPI functions and structures reference
+5. How to contact authors
+========================================================
+
+1. What is SPI ?
+----------------
+SPI stands for "Serial Peripheral Interface", a full-duplex synchronous
+serial interface for connecting low-/medium-bandwidth external devices
+using four wires. SPI devices communicate using a master/slave relation-
+ship over two data lines and two control lines:
+- Master Out Slave In (MOSI): supplies the output data from the master
+ to the inputs of the slaves;
+- Master In Slave Out (MISO): supplies the output data from a slave to
+ the input of the master. It is important to note that there can be no
+ more than one slave that is transmitting data during any particular
+ transfer;
+- Serial Clock (SCLK): a control line driven by the master, regulating
+ the flow of data bits;
+- Slave Select (SS): a control line that allows slaves to be turned on
+ and off with hardware control.
+More information is also available at http://en.wikipedia.org/wiki/Serial_Peripheral_Interface
+
+2. Purposes of this code
+------------------------
+The supplied patch is starting point for implementing drivers for various
+SPI busses as well as devices connected to these busses. Currently, the
+SPI core supports only for MASTER mode for system running Linux.
+
+3. SPI devices stack
+--------------------
+
+3.1 The SPI outline
+
+The SPI infrastructure deals with several levels of abstraction. They are
+"SPI bus", "SPI bus driver", "SPI slave device" and "SPI device driver". The
+"SPI bus" is hardware device, which usually called "SPI adapter", and has
+"SPI slave devices" connected. From the Linux' point of view, the "SPI bus" is
+structure of type platform_device, and "SPI slave device" is structure of type
+spi_device. The "SPI bus driver" is the driver which controls the whole
+SPI bus (and, particularly, creates and destroys "SPI slave devices" on the bus),
+and "SPI device driver" is driver that controls the only device on the SPI
+bus, controlled by "SPI bus driver". "SPI device driver" can indirectly
+call "SPI bus driver" to send/receive messages using API provided by SPI
+core, and provide its own interface to the kernel and/or userland.
+So, the device stack looks as follows:
+
+ +--------------+ +---------+
+ | some_bus | | spi_bus |
+ +--------------+ +---------+
+ |..| |
+ |..|--------+ +---------------+
+ +------------+| is parent to | SPI devices |
+ | SPI busses |+-------------> | |
+ +------------+ +---------------+
+ | |
+ +----------------+ +----------------------+
+ | SPI bus driver | | SPI device driver |
+ +----------------+ +----------------------+
+
+3.2 How do the SPI devices gets discovered and probed ?
+
+In general, the SPI bus driver cannot effective discover devices
+on its bus. Fortunately, the devices on SPI bus usually implemented
+onboard, so the following method has been chosen: the SPI bus driver
+calls the function named spi_bus_populate and passed the `topology
+string' to it. The function will parse the string and call the callback
+for each device, just before registering it. This allows bus driver
+to determine parameters like CS# for each device, retrieve them from
+string and store somewhere like spi_device->platform_data. An example:
+ err = spi_bus_populate( the_spi_bus,
+ "Dev1 0 1 2\0" "Dev2 2 1 0\0",
+ extract_name )
+In this example, function like extract_name would put the '\0' on the
+1st space of device's name, so names will become just "Dev1", "Dev2",
+and the rest of string will become parameters of device.
+
+The another way is to create array of structures spi_device_desc and pass
+this array to function spi_bus_populate2, like this:
+ struct spi_device_desc spi_slaves[] = {
+ [0] = {
+ .name = "device1",
+ .param = device1_params,
+ },
+ [1] = {
+ .name = "device2",
+ .param = NULL,
+ }
+ [2] = {
+ NULL, NULL
+ };
+ spi_bus_populate2( the_spi_bus, spi_slaves, callback );
+
+3.3. DMA and SPI messages
+-------------------------
+
+To handle DMA transfers on SPI bus, any device driver might provide special
+callbacks to allocate/free/get access to buffer. These callbacks are defined
+in subsection iii of section 4.
+To send data using DMA, the buffers should be allocated using
+dma_alloc_coherent function. Usually buffers are allocated statically or
+using kmalloc function.
+To allow drivers to allocate buffers in non-standard
+When one allocates the structure for spi message, it needs to provide target
+device. If its driver wants to allocate buffer in driver-specific way, it may
+provide its own allocation/free methods: alloc and free. If driver does not
+provide these methods, kmalloc and kfree will be used.
+After allocation, the buffer must be accessed to copy the buffer to be send
+or retrieve buffer that has been just received from device. If buffer was
+allocated using driver's alloc method,ccessed using
+get_buffer. Driver should provide accessible buffer that corresponds buffer
+allocated by driver's alloc method. If there is no get_buffer method,
+the result of alloc will be used.
+After reading/writing from/to buffer, it will be released by call to driver's
+release_buffer method.
+
+
+4. SPI functions are structures reference
+-----------------------------------------
+This section describes structures and functions that listed
+in include/linux/spi.h
+
+i. struct spi_msg
+~~~~~~~~~~~~~~~~~
+
+struct spi_msg {
+ unsigned char flags;
+ unsigned short len;
+ unsigned long clock;
+ struct spi_device* device;
+ void *context;
+ struct list_head link;
+ void (*status)( struct spi_msg* msg, int code );
+ void *devbuf_rd, *devbuf_wr;
+ u8 *databuf_rd, *databuf_wr;
+};
+This structure represents the message that SPI device driver sends to the
+SPI bus driver to handle.
+Fields:
+ flags combination of message flags
+ SPI_M_RD "read" operation (from device to host)
+ SPI_M_WR "write" operation (from host to device)
+ SPI_M_CS assert the CS signal before sending the message
+ SPI_M_CSREL clear the CS signal after sending the message
+ SPI_M_CSPOL set clock polarity to high
+ SPI_M_CPHA set clock phase to high
+ len length, in bytes, of allocated buffer
+ clock reserved, set to zero
+ device the target device of the message
+ context user-defined field; to associate any user data with the message
+ link used by bus driver to queue messages
+ status user-provided callback function to inform about message flow
+ devbuf_rd, devbuf_wr
+ so-called "device buffers". These buffers allocated by the
+ device driver, if device driver provides approproate callback.
+ Otherwise, the kmalloc API will be used.
+ databuf_rd, databuf_wr
+ pointers to access content of device buffers. They are acquired
+ using get_buffer callback, if device driver provides one.
+ Otherwise, they are just pointers to corresponding
+ device buffers
+
+struct spi_msg* spimsg_alloc( struct spi_device* device,
+ unsigned flags,
+ unsigned short len,
+ void (*status)( struct spi_msg*, int code ) )
+This functions is called to allocate the spi_msg structure and set the
+corresponding fields in structure. If device->platform_data provides callbacks
+to handle buffers, alloc/get_buffer are to be used. Returns NULL on errors.
+
+struct void spimsg_free( struct spi_msg* msg )
+Deallocate spi_msg as well as internal buffers. If msg->device->platform_data
+provides callbacks to handle buffers, release_buffer and free are to be used.
+
+u8* spimsg_buffer_rd( struct spi_msg* msg )
+u8* spimsg_buffer_wr( struct spi_msg* msg )
+u8* spimsg_buffer( struct spi_msg* )
+Return the corresponding data buffer, which can be directly modified by driver.
+spimsg_buffer checks flags and return either databuf_rd or databuf_wr basing on
+value of `flags' in spi_msg structure.
+
+ii. struct spi_device
+~~~~~~~~~~~~~~~~~~~~~
+
+struct spi_device
+{
+ char name[ BUS_ID_SIZE ];
+ struct device dev;
+};
+This structure represents the physical device on SPI bus. The SPI bus driver
+will create and register this structure for you.
+ name the name of the device. It should match to the SPI device
+ driver name
+ dev field used to be registered with core
+
+struct spi_device* spi_device_add( struct device* parent,
+ char* name )
+This function registers the device on the spi bus, and set its parent
+to `parent', which represents the SPI bus. The device name will be set to name,
+that should be non-empty, non-NULL string. Returns pointer to allocated
+spi_device structure, NULL on error.
+
+void spi_device_del( struct spi_device* dev )
+Unregister the SPI device. Return value is ignored
+
+iii. struct spi_driver
+~~~~~~~~~~~~~~~~~~~~~~
+
+struct spi_driver {
+ void* (*alloc)( size_t, int );
+ void (*free)( const void* );
+ unsigned char* (*get_buffer)( struct spi_device*, void* );
+ void (*release_buffer)( struct spi_device*, unsigned char*);
+ void (*control)( struct spi_device*, int mode, u32 ctl );
+ struct device_driver driver;
+};
+This structure represents the SPI device driver object. Before registering,
+all fields of driver sub-structure should be properly filled, e.g., the
+`bus_type' should be set to spi_bus. Otherwise, the driver will be incorrectly
+registered and its callbacks might never been called. An example of will-
+formed spi_driver structure:
+ extern struct bus_type spi_bus;
+ static struct spi_driver pnx4008_eeprom_driver = {
+ .driver = {
+ .bus = &spi_bus,
+ .name = "pnx4008-eeprom",
+ .probe = pnx4008_spiee_probe,
+ .remove = pnx4008_spiee_remove,
+ .suspend = pnx4008_spiee_suspend,
+ .resume = pnx4008_spiee_resume,
+ },
+};
+The method control gets called during the processing of SPI message.
+For detailed description of malloc/free/get_buffer/release_buffer, please
+look to section 3.3, "DMA and SPI messages"
+
+
+int spi_driver_add( struct spi_driver* driver )
+Register the SPI device driver with core; returns 0 on no errors, error code
+otherwise.
+
+void spi_driver_del( struct spi_driver* driver )
+Unregisters the SPI device driver; return value ignored.
+
+iv. struct spi_bus_driver
+~~~~~~~~~~~~~~~~~~~~~~~~~
+To handle transactions over the SPI bus, the spi_bus_driver structure must
+be prepared and registered with core. Any transactions, either synchronous
+or asynchronous, go through spi_bus_driver->xfer function.
+
+struct spi_bus_driver
+{
+ int (*xfer)( struct spi_msg* msgs );
+ void (*select) ( struct spi_device* arg );
+ void (*deselect)( struct spi_device* arg );
+
+ struct spi_msg *(*retrieve)( struct spi_bus_driver *this, struct spi_bus_data *bd);
+ void (*reset)( struct spi_bus_driver *this, u32 context);
+
+ struct device_driver driver;
+};
+
+Fields:
+ xfer pointer to function to execute actual transaction on SPI bus
+ msg message to handle
+ select pointer to function that gets called when bus needs to
+ select another device to be target of transfers
+ deselect
+ pointer to function that gets called before another device
+ is selected to be the target of transfers
+ reset
+ pointer to function that performs reset of SPI bus
+ retrieve
+ this function is used to retrieve next message from queue. If NULL,
+ spi_bus_fifo_retrieve is used
+
+
+spi_bus_driver_register( struct spi_bus_driver* )
+
+Register the SPI bus driver with the system. The driver sub-structure should
+be properly filled before using this function, otherwise you may get unpredi-
+ctable results when trying to exchange data. An example of correctly prepared
+spi_bus_driver structure:
+ static struct spi_bus_driver spipnx_driver = {
+ .driver = {
+ .bus = &platform_bus_type,
+ .name = "spipnx",
+ .probe = spipnx_probe,
+ .remove = spipnx_remove,
+ .suspend = spipnx_suspend,
+ .resume = spipnx_resume,
+ },
+ .xfer = spipnx_xfer,
+};
+The driver and corresponding platform device are matched by name, so, in
+order the example abive to work, the platform_device named "spipnx" should
+be registered somewhere.
+
+void spi_bus_driver_unregister( struct spi_bus_driver* )
+
+Unregister the SPI bus driver registered by call to spi_buys_driver_register
+function; returns void.
+
+int spi_bus_populate( struct device* parent,
+ char* devices,
+ void (*callback)( struct device* parent, struct spi_device* new_one ) )
+This function usually called by SPI bus drivers in order to populate the SPI
+bus (see also section 3.2, "How the SPI devices gets discovered and probed ?").
+After creating the spi_device, the spi_bus_populate calls the `callback'
+function to allow to modify spi_device's fields before registering it with core.
+ parent pointer to SPI bus
+ devices string representing the current topology of SPI bus. It should
+ be formed like
+ "dev-1_and_its_info\0dev-2_and_its_info\0another_device\0\0"
+ the spi_bus_populate delimits this string by '\0' characters,
+ creates spi_device and after calling the callback registers the
+ spi_device
+ callback
+ pointer to function which could modify spi_device fields just
+ before registering them with core
+
+int spi_bus_populate2 (struct device *parent, struct spi_device_desc *devices,
+ void (*callback) (struct device* parent, struct spi_device* new_dev,
+ void *params ))
+This is another way to populate the bus; but instead of string with device names and
+parameters, the array of structures spi_device_desc is passed. Each item in array describes
+the SPI slave device to create.
+
+
+v. spi_transfer and spi_queue
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The driver that uses SPI core can initiate transfers either by calling
+spi_transfer function (that will wait till the transfer is funished) or
+queueing the message using spi_queue function (you need to provide function
+that will be called during message is processed). In any way, you need to
+prepare the spimsg structure and know the target device to your message to
+be sent.
+
+int spi_transfer( struct spi_msg msgs,
+ void (*callback)( struct spi_msg* msg, int ) )
+If callback is zero, start synchronous transfer. Otherwise, queue
+the message.
+ msg message to be handled
+ callback the callback function to be called during
+ message processing. If NULL, the function
+ will wait until end of processing.
+
+int spi_queue( struct spi_device* device,
+ struct spi_msg* msg )
+Queue the only message to the device. Returns status of queueing. To obtain
+status of message processing, you have to provide `status' callback in message
+and examine its parameters
+ msg message to be queued
+
+vi. the spi_bus variable
+~~~~~~~~~~~~~~~~~~~~~~~~
+This variable is created during initialization of spi core, and has to be
+specified as `bus' on any SPI device driver (look to section iii, "struct
+spi_driver" ). If you do not specify spi_bus, your driver will be never
+matched to spi_device and never be probed with hardware. Note that
+spi_bus.match points to function that matches drivers and devices by name,
+so SPI devices and their drivers should have the same name.
+
+5. How to contact authors
+-------------------------
+Do you have any comments ? Enhancements ? Device driver ? Feel free
+to contact me:
+ [email protected]
+ [email protected]
+Visit our project page:
+ http://spi-devel.sourceforge.net
+Subscribe to mailing list:
+ [email protected]
+
Index: lsm-2.6/drivers/Kconfig
===================================================================
--- lsm-2.6.orig/drivers/Kconfig
+++ lsm-2.6/drivers/Kconfig
@@ -44,6 +44,8 @@ source "drivers/char/Kconfig"

source "drivers/i2c/Kconfig"

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

source "drivers/hwmon/Kconfig"
Index: lsm-2.6/drivers/spi/Kconfig
===================================================================
--- /dev/null
+++ lsm-2.6/drivers/spi/Kconfig
@@ -0,0 +1,30 @@
+#
+# SPI device configuration
+#
+menu "SPI support"
+
+config SPI
+ tristate "SPI (Serial Peripheral Interface) bus support"
+ default false
+ help
+ Say Y if you need to enable SPI support on your kernel.
+ Say M if you want to create the spi-core loadable module.
+
+config SPI_DEBUG
+ bool "SPI debug output"
+ depends on SPI
+ default false
+ help
+ Say Y there if you'd like to see debug output from SPI drivers
+ If unsure, say N
+
+config SPI_CHARDEV
+ default Y
+ bool "SPI device interface"
+ depends on SPI
+ help
+ Say Y here to use /dev/spiNN device files. They make it possible to have user-space
+ programs use the SPI bus.
+
+endmenu
+
Index: lsm-2.6/drivers/spi/Makefile
===================================================================
--- /dev/null
+++ lsm-2.6/drivers/spi/Makefile
@@ -0,0 +1,15 @@
+#
+# Makefile for the kernel spi bus driver.
+#
+
+spi-y += spi-core.o
+spi-$(CONFIG_SPI_CHARDEV) += spi-dev.o
+# bus drivers
+# ...functional drivers
+# ...and the common spi-dev driver
+obj-$(CONFIG_SPI) += spi.o
+
+ifeq ($(CONFIG_SPI_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
+
Index: lsm-2.6/drivers/spi/spi-core.c
===================================================================
--- /dev/null
+++ lsm-2.6/drivers/spi/spi-core.c
@@ -0,0 +1,700 @@
+/*
+ * drivers/spi/spi-core.c
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/config.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/proc_fs.h>
+#include <linux/kmod.h>
+#include <linux/init.h>
+#include <linux/wait.h>
+#include <linux/kthread.h>
+#include <linux/spi.h>
+#include <asm/atomic.h>
+
+static int spi_thread(void *context);
+static int spi_device_del(struct device *dev, void *data);
+
+/**
+ * spi_bus_match_name - verify that driver matches device on spi bus
+ *
+ * @dev: device that hasn't yet being assigned to any driver
+ * @drv: driver for spi device
+ *
+ * Drivers and devices on SPI bus are matched by name, just like the
+ * platform devices, with exception of SPI_DEV_CHAR. Driver with this name
+ * will be matched against any device
+**/
+static int spi_bus_match_name(struct device *dev, struct device_driver *drv)
+{
+ return !strcmp(TO_SPI_DEV(dev)->name, drv->name);
+}
+
+/**
+ * spi_bus_suspend - suspend all devices on the spi bus
+ *
+ * @dev: spi device to be suspended
+ * @message: PM message
+ *
+ * This function set device on SPI bus to suspended state, just like platform_bus does
+**/
+static int spi_bus_suspend(struct device * dev, pm_message_t message)
+{
+ int ret = 0;
+
+ if (dev && dev->driver && dev->driver->suspend ) {
+ ret = TO_SPI_DRIVER(dev->driver)->suspend( TO_SPI_DEV(dev), message);
+ if (ret == 0 )
+ dev->power.power_state = message;
+ }
+ return ret;
+}
+
+/**
+ * spi_bus_resume - resume functioning of all devices on spi bus
+ *
+ * @dev: device to resume
+ *
+ * This function resumes device on SPI bus, just like platform_bus does
+**/
+static int spi_bus_resume(struct device * dev)
+{
+ int ret = 0;
+
+ if (dev && dev->driver && dev->driver->suspend ) {
+ ret = TO_SPI_DRIVER(dev->driver)->resume(TO_SPI_DEV(dev));
+ if (ret == 0)
+ dev->power.power_state = PMSG_ON;
+ }
+ return ret;
+}
+
+/**
+ * spi_bus - the &bus_type structure for SPI devices and drivers
+ *
+ * @name: the name of subsystem, "spi" here
+ * @match: function that matches devices to their drivers
+ * @suspend: PM callback to suspend device
+ * @resume: PM callback to resume device
+**/
+struct bus_type spi_bus = {
+ .name = "spi",
+ .match = spi_bus_match_name,
+ .suspend = spi_bus_suspend,
+ .resume = spi_bus_resume,
+};
+
+/**
+ * spi_bus_driver_init - init internal bus driver structures
+ *
+ * @bus: registered spi_bus_driver structure
+ * @dev: device that represents spi controller
+ *
+ * Once registered by spi_bus_register, the bus driver needs initialization, that
+ * includes starting thread, initializing internal structures.. The best place where
+ * the spi_bus_driver_init is in the `probe' function, when we already sure that passed
+ * device object is SPI master controller
+**/
+int spi_bus_driver_init(struct spi_bus_driver *bus, struct device *dev)
+{
+ struct spi_bus_data *pd =
+ kmalloc(sizeof(struct spi_bus_data), GFP_KERNEL);
+ int err = 0;
+
+ if (!pd) {
+ err = -ENOMEM;
+ goto init_failed_1;
+ }
+ atomic_set(&pd->exiting, 0);
+ pd->bus = bus;
+ init_MUTEX(&pd->lock);
+ INIT_LIST_HEAD(&pd->msgs);
+ init_waitqueue_head(&pd->queue);
+ pd->id = dev->bus_id;
+ pd->thread = kthread_run(spi_thread, pd, "%s-work", pd->id);
+ if (IS_ERR(pd->thread)) {
+ err = PTR_ERR(pd->thread);
+ goto init_failed_2;
+ }
+ dev->platform_data = pd;
+ return 0;
+
+init_failed_2:
+ kfree(pd);
+init_failed_1:
+ return err;
+}
+
+/**
+ * __spi_bus_free -- unregister all children of the spi bus
+ *
+ * @dev: the spi bus `device' object
+ * @context: not used, will be NULL
+ *
+ * This is internal function that is called when unregistering bus driver. Responsibility
+ * of this function is freeing the resources that were requested by spi_bus_driver_init
+ **/
+static int __spi_bus_free(struct device *dev, void *context)
+{
+ struct spi_bus_data *pd = dev->platform_data;
+
+ if (pd) {
+ atomic_inc(&pd->exiting);
+ kthread_stop(pd->thread);
+ kfree(pd);
+ }
+
+ dev_dbg(dev, "unregistering children\n");
+
+ device_for_each_child(dev, NULL, spi_device_del);
+ return 0;
+}
+
+/**
+ * spi_bus_driver_unregister - unregister SPI bus controller from the system
+ *
+ * @bus_driver: driver registered by call to spi_bus_driver_register
+ *
+ * unregisters the SPI bus from the system. Before unregistering, it deletes
+ * each SPI device on the bus using call to __spi_device_free
+**/
+void spi_bus_driver_unregister(struct spi_bus_driver *bus_driver)
+{
+ if (bus_driver) {
+ driver_for_each_device(&bus_driver->driver, NULL, NULL, __spi_bus_free);
+ driver_unregister(&bus_driver->driver);
+ }
+}
+
+/**
+ * spi_device_release - release the spi device structure
+ *
+ * @dev: spi_device to be released
+ *
+ * Pointer to this function will be put to dev->release place
+ * This fus called as a part of device removing
+**/
+void spi_device_release(struct device *dev)
+{
+ struct spi_device* sdev = TO_SPI_DEV(dev);
+
+ kfree(sdev);
+}
+
+/**
+ * spi_device_add - add the new (discovered) SPI device to the bus. Mostly used by bus drivers
+ *
+ * @parent: the bus device object
+ * @name: name of device (non-null!)
+ * @bus_data: bus data to be assigned to device
+ *
+ * SPI devices usually cannot be discovered by SPI bus driver, so it needs to take the configuration
+ * somewhere from hardcoded structures, and prepare bus_data for its devices
+**/
+struct spi_device* spi_device_add(struct device *parent, char *name, void *bus_data)
+{
+ struct spi_device* dev;
+ static int minor = 0;
+
+ if (!name)
+ goto dev_add_out;
+
+ dev = kmalloc(sizeof(struct spi_device), GFP_KERNEL);
+ if(!dev)
+ goto dev_add_out;
+
+ memset(&dev->dev, 0, sizeof(dev->dev));
+ dev->dev.parent = parent;
+ dev->dev.bus = &spi_bus;
+ strncpy(dev->name, name, sizeof(dev->name));
+ strncpy(dev->dev.bus_id, name, sizeof(dev->dev.bus_id));
+ dev->dev.release = spi_device_release;
+ dev->dev.platform_data = bus_data;
+
+ if (device_register(&dev->dev)<0) {
+ dev_dbg(parent, "device '%s' cannot be added\n", name);
+ goto dev_add_out_2;
+ }
+ dev->cdev = spi_class_device_create(minor, &dev->dev);
+ dev->minor = minor++;
+ return dev;
+
+dev_add_out_2:
+ kfree(dev);
+dev_add_out:
+ return NULL;
+}
+
+static int spi_device_del(struct device *dev, void *data)
+{
+ struct spi_device *spidev = TO_SPI_DEV(dev);
+ if (spidev->cdev) {
+ spi_class_device_destroy(spidev->cdev);
+ spidev->cdev = NULL;
+ }
+ device_unregister(&spidev->dev);
+ return 0;
+}
+/**
+ * spi_queue - queue the message to be processed asynchronously
+ *
+ * @msg: message to be sent
+ *
+ * This function queues the message to spi bus driver's queue. The bus driver
+ * retrieves the message from queue according to its own rules (see retrieve method)
+ * and sends the message to target device. If message has no callback method, originator
+ * of message would get no chance to know where the message is processed. The better
+ * solution is using spi_transfer function, which will return error code if no callback
+ * is provided, or transfer the message synchronously.
+**/
+int spi_queue(struct spi_msg *msg)
+{
+ struct device *dev = &msg->device->dev;
+ struct spi_bus_data *pd = dev->parent->platform_data;
+
+ down(&pd->lock);
+ list_add_tail(&msg->link, &pd->msgs);
+ dev_dbg(dev->parent, "message has been queued\n");
+ up(&pd->lock);
+ wake_up_interruptible(&pd->queue);
+ return 0;
+}
+
+/**
+ * __spi_transfer_callback - callback to process synchronous messages
+ *
+ * @msg: message that is about to complete
+ * @code: message status
+ *
+ * callback for synchronously processed message. If spi_transfer determines
+ * that there is no callback provided neither by msg->status nor callback
+ * parameter, the __spi_transfer_callback will be used, and spi_transfer
+ * does not return until transfer is finished
+ *
+**/
+static void __spi_transfer_callback(struct spi_msg *msg, int code)
+{
+ if (code & (SPIMSG_OK | SPIMSG_FAILED))
+ complete((struct completion *)msg->context);
+}
+
+/*
+ * spi_transfer - transfer the message either in sync or async way
+ *
+ * @msg: message to process
+ * @callback: user-supplied callback
+ *
+ * If both msg->status and callback are set, the error code of -EINVAL
+ * will be returned
+ */
+int spi_transfer(struct spi_msg *msg, void (*callback) (struct spi_msg *, int))
+{
+ struct completion msg_done;
+ int err = -EINVAL;
+
+ if (callback && !msg->status) {
+ msg->status = callback;
+ callback = NULL;
+ }
+
+ if (!callback) {
+ if (!msg->status) {
+ init_completion(&msg_done);
+ msg->context = &msg_done;
+ msg->status = __spi_transfer_callback;
+ spi_queue(msg);
+ wait_for_completion(&msg_done);
+ err = 0;
+ } else {
+ err = spi_queue(msg);
+ }
+ }
+
+ return err;
+}
+
+/**
+ * spi_thread_awake - function that called to determine if thread needs to process any messages
+ *
+ * @bd: pointer to struct spi_bus_data
+ *
+ * Thread wakes up if there is signal to exit (bd->exiting is set) or there are any messages
+ * in bus' queue.
+ */
+static int spi_thread_awake(struct spi_bus_data *bd)
+{
+ int ret;
+
+ if (atomic_read(&bd->exiting)) {
+ return 1;
+ }
+ down(&bd->lock);
+ ret = !list_empty(&bd->msgs);
+ up(&bd->lock);
+ return ret;
+}
+
+static void spi_async_callback(void *_msg)
+{
+ struct spi_msg *msg = _msg;
+
+ msg->status (msg, SPIMSG_OK);
+}
+
+/**
+ * spi_bus_fifo_retrieve - simple function to retrieve the first message from the queue
+ *
+ * @this: spi_bus_driver that needs to retrieve next message from queue
+ * @data: pointer to spi_bus_data structure associated with spi_bus_driver
+ *
+ * This is pretty simple `retrieve' function. It retrieves the first message from the queue,
+ * and does not care about target of the message. For simple cases, this function is the best
+ * and the fastest solution to provide as retrieve method of bus driver
+ **/
+static struct spi_msg *spi_bus_fifo_retrieve (struct spi_bus_driver *this, struct spi_bus_data *data)
+{
+ return list_entry(data->msgs.next, struct spi_msg, link);
+}
+
+/**
+ * spi_bus_simple_retrieve -- retrieve message from the queue with taking into account previous target
+ *
+ * @this: spi_bus_driver that needs to retrieve next message from queue
+ * @data: pointer to spi_bus_data structure associated with spi_bus_driver
+ *
+ * this function is more complex than spi_bus_fifo_retrieve; it takes into account the already selected
+ * device on SPI bus, and tries to retrieve the message targeted to the same device.
+ *
+ **/
+static struct spi_msg *spi_bus_simple_retrieve(struct spi_bus_driver *this, struct spi_bus_data *data)
+{
+ int found = 0;
+ struct spi_msg *msg;
+
+ list_for_each_entry(msg, &data->msgs, link) {
+ if (!data->selected_device || msg->device == data->selected_device) {
+ found = 1;
+ break;
+ }
+ }
+ if (!found)
+ /*
+ * all messages for current selected_device
+ * are processed.
+ * let's switch to another device
+ */
+ msg = list_entry(data->msgs.next, struct spi_msg, link);
+
+ return msg;
+}
+
+/**
+ * spi_bus_next_msg - the wrapper for retrieve method for bus driver
+ *
+ * @this: spi_bus_driver that needs to retrieve next message from queue
+ * @data: pointer to spi_bus_data structure associated with spi_bus_driver
+ *
+ * If bus driver provides the `retrieve' method, it is called to retrieve the next message
+ * from queue. Otherwise, the spi_bus_fifo_retrieve is called
+ *
+ **/
+static struct spi_msg *spi_bus_next_msg(struct spi_bus_driver *this, struct spi_bus_data *data)
+{
+ if (!this)
+ return NULL;
+ if (this->retrieve)
+ return this->retrieve (this, data);
+ return spi_bus_fifo_retrieve(this, data);
+}
+
+/**
+ * spi_thread - the thread that calls bus functions to perform actual transfers
+ *
+ * @pd: pointer to struct spi_bus_data with bus-specific data
+ *
+ * This function is started as separate thread to perform actual
+ * transfers on SPI bus
+ **/
+static int spi_thread(void *context)
+{
+ struct spi_bus_data *bd = context;
+ struct spi_msg *msg;
+ int xfer_status;
+ struct workqueue_struct *wq;
+
+ wq = create_workqueue (bd->id);
+ if (!wq)
+ pr_debug("%s: cannot create workqueue, async callbacks will be unavailable\n", bd->id);
+
+ while (!kthread_should_stop()) {
+
+ wait_event_interruptible(bd->queue, spi_thread_awake(bd));
+
+ if (atomic_read(&bd->exiting))
+ goto thr_exit;
+
+ down(&bd->lock);
+ while (!list_empty(&bd->msgs)) {
+ /*
+ * this part is locked by bus_data->lock,
+ * to protect spi_msg extraction
+ */
+ msg = spi_bus_next_msg(bd->bus, bd);
+
+ /* verify if device needs re-selecting */
+ if (bd->selected_device != msg->device) {
+ if (bd->selected_device && bd->bus->deselect)
+ bd->bus->deselect (bd->selected_device);
+ bd->selected_device = msg->device;
+ if (bd->bus->select)
+ bd->bus->select (bd->selected_device);
+ }
+ list_del(&msg->link);
+ up(&bd->lock);
+
+ /*
+ * and this part is locked by device's lock;
+ * spi_queue will be able to queue new
+ * messages
+ *
+ * note that bd->selected_device is locked, not msg->device
+ * they are the same, but msg can be freed in msg->status function
+ */
+ spi_device_lock(bd->selected_device);
+ if (bd->bus->set_clock && msg->clock)
+ bd->bus->set_clock(msg->device->dev.parent,
+ msg->clock);
+ xfer_status = bd->bus->xfer(msg);
+ if (msg->status) {
+ if (msg->flags & SPI_M_ASYNC_CB) {
+ INIT_WORK(&msg->wq_item, spi_async_callback, msg);
+ queue_work (wq, &msg->wq_item);
+ } else {
+ msg->status(msg,
+ xfer_status == 0 ? SPIMSG_OK :
+ SPIMSG_FAILED);
+ }
+ }
+
+ spi_device_unlock(bd->selected_device);
+
+ /* lock the bus_data again... */
+ down(&bd->lock);
+ }
+ if (bd->bus->deselect)
+ bd->bus->deselect(bd->selected_device);
+ bd->selected_device = NULL;
+ /* device has been just deselected, unlocking the bus */
+ up(&bd->lock);
+ }
+
+thr_exit:
+ if (wq)
+ destroy_workqueue (wq);
+ return 0;
+}
+
+/**
+ * spi_write - send data to a device on an SPI bus
+ *
+ * @dev: the target device
+ * @buf: buffer to be sent
+ * @len: buffer's length
+ *
+ * Returns the number of bytes transferred, or negative error code.
+**/
+int spi_write(struct spi_device *dev, u32 flags, char *buf, size_t len)
+{
+ struct spi_msg *msg = spimsg_alloc(dev, SPI_M_WR | flags, buf, len, NULL);
+ int ret;
+
+ spimsg_sync(msg, buf, len);
+ ret = spi_transfer(msg, NULL);
+ return ret == 1 ? len : ret;
+}
+
+/**
+ * spi_read - receive data from a device on an SPI bus
+ *
+ * @dev: the target device
+ * @buf: buffer to be sent
+ * @len: buffer's length
+ *
+ * Returns the number of bytes transferred, or negative error code.
+**/
+int spi_read(struct spi_device *dev, u32 flags, char *buf, size_t len)
+{
+ int ret;
+ struct spi_msg *msg = spimsg_alloc(dev, SPI_M_RD | flags, buf, len, NULL);
+
+ ret = spi_transfer(msg, NULL);
+ spimsg_sync(msg, buf, len);
+ return ret == 1 ? len : ret;
+}
+
+/**
+ * spi_bus_populate/spi_bus_populate2 - populate the bus
+ *
+ * @parent: the SPI bus device object
+ * @devices: string that represents bus population
+ * @devices_s: array of structures that represents bus population
+ * @callback: optional pointer to function that called on each device's add
+ *
+ * These two functions intended to populate the SPI bus corresponding to
+ * device passed as 1st parameter. The difference is in the way to describe
+ * new SPI slave devices: the spi_bus_populate takes the ASCII string delimited
+ * by '\0', where each section matches one SPI device name _and_ its parameters,
+ * and the spi_bus_populate2 takes the array of structures spi_device_desc.
+ *
+ * If some device cannot be added because of spi_device_add fail, the function will
+ * not try to parse the rest of list
+ */
+int spi_bus_populate(struct device *parent,
+ char *devices,
+ void (*callback) (struct device * bus,
+ struct spi_device * new_dev))
+{
+ struct spi_device *new_device;
+ int count = 0;
+
+ while (devices[0]) {
+ dev_dbg(parent, " discovered new SPI device, name '%s'\n",
+ devices);
+ if ((new_device = spi_device_add(parent, devices, NULL)) == NULL)
+ break;
+ if (callback)
+ callback(parent, new_device);
+ devices += (strlen(devices) + 1);
+ count++;
+ }
+ return count;
+}
+
+int spi_bus_populate2(struct device *parent,
+ struct spi_device_desc* devices_s,
+ void (*callback) (struct device* bus,
+ struct spi_device *new_dev,
+ void* params))
+{
+ struct spi_device *new_device;
+ int count = 0;
+
+ while (devices_s->name) {
+ dev_dbg(parent, " discovered new SPI device, name '%s'\n",
+ devices_s->name);
+ if ((new_device = spi_device_add(parent, devices_s->name, devices_s->params)) == NULL)
+ break;
+ if (callback)
+ callback(parent, new_device, devices_s->params);
+ devices_s++;
+ count++;
+ }
+ return count;
+}
+
+/**
+ * spi_bus_reset - reset the spi bus
+ *
+ * @bus: device object that represents the SPI bus
+ * @context: u32 value to be passed to reset method of bus
+ *
+ * This is simple wrapper for bus' `reset' method
+ *
+**/
+void spi_bus_reset (struct device *bus, u32 context)
+{
+ if (bus && bus->driver && TO_SPI_BUS_DRIVER(bus->driver)->reset)
+ TO_SPI_BUS_DRIVER(bus->driver)->reset(bus, context);
+}
+
+/*
+ * the functions below are wrappers for corresponding device_driver's methods
+ */
+int spi_driver_probe (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return sdrv && sdrv->probe ? sdrv->probe(sdev) : -EFAULT;
+}
+
+int spi_driver_remove (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return (sdrv && sdrv->remove) ? sdrv->remove(sdev) : -EFAULT;
+}
+
+void spi_driver_shutdown (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ if (sdrv && sdrv->shutdown)
+ sdrv->shutdown(sdev);
+}
+
+int spi_driver_suspend (struct device *dev, pm_message_t pm)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return (sdrv && sdrv->suspend) ? sdrv->suspend(sdev, pm) : -EFAULT;
+}
+
+int spi_driver_resume (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return (sdrv && sdrv->resume) ? sdrv->resume(sdev) : -EFAULT;
+}
+
+static int __init spi_core_init(void)
+{
+ int ret = spidev_init();
+
+ if (ret == 0)
+ ret = bus_register(&spi_bus);
+
+ return ret;
+}
+
+static void __exit spi_core_exit(void)
+{
+ bus_unregister(&spi_bus);
+ spidev_cleanup();
+}
+
+subsys_initcall(spi_core_init);
+module_exit(spi_core_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("dmitry pervushin <[email protected]>");
+
+EXPORT_SYMBOL_GPL(spi_bus_reset);
+EXPORT_SYMBOL_GPL(spi_queue);
+EXPORT_SYMBOL_GPL(spi_device_add);
+EXPORT_SYMBOL_GPL(spi_bus_driver_unregister);
+EXPORT_SYMBOL_GPL(spi_bus_populate);
+EXPORT_SYMBOL_GPL(spi_bus_populate2);
+EXPORT_SYMBOL_GPL(spi_transfer);
+EXPORT_SYMBOL_GPL(spi_write);
+EXPORT_SYMBOL_GPL(spi_read);
+EXPORT_SYMBOL_GPL(spi_bus);
+EXPORT_SYMBOL_GPL(spi_bus_driver_init);
+EXPORT_SYMBOL_GPL(spi_bus_fifo_retrieve);
+EXPORT_SYMBOL_GPL(spi_bus_simple_retrieve);
+
Index: lsm-2.6/drivers/spi/spi-dev.c
===================================================================
--- /dev/null
+++ lsm-2.6/drivers/spi/spi-dev.c
@@ -0,0 +1,192 @@
+/*
+ spi-dev.c - spi driver, char device interface
+
+ Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+
+ 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/init.h>
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/version.h>
+#include <linux/smp_lock.h>
+
+#include <linux/init.h>
+#include <asm/uaccess.h>
+#include <linux/spi.h>
+
+#define SPI_TRANSFER_MAX 65535L
+
+static struct class *spidev_class;
+
+static ssize_t spidev_read(struct file *file, char *buf, size_t count,
+ loff_t * offset);
+static ssize_t spidev_write(struct file *file, const char *buf, size_t count,
+ loff_t * offset);
+
+static int spidev_open(struct inode *inode, struct file *file);
+static int spidev_release(struct inode *inode, struct file *file);
+
+/**
+ * spi_class_device_create - wrapper for class_device_create to be used in spi core
+ *
+ * @minor: sequental minor number of SPI slave device
+ * @device: pointer to struct device embedded to spi_device
+ *
+**/
+struct class_device *spi_class_device_create(int minor, struct device *device)
+{
+ return class_device_create(spidev_class, NULL, MKDEV(SPI_MAJOR, minor), device, "spi%d", minor);
+}
+
+/**
+ * spi_class_device_destroy - wrapper for class_device_destroy to be used in spi core
+ *
+ * @minor: minor number of device
+ *
+**/
+void spi_class_device_destroy(struct class_device *cdev)
+{
+ class_device_destroy(spidev_class, cdev->devt);
+}
+
+static struct file_operations spidev_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .read = spidev_read,
+ .write = spidev_write,
+ .open = spidev_open,
+ .release = spidev_release,
+};
+
+static ssize_t spidev_read(struct file *file, char __user *buf, size_t count,
+ loff_t * offset)
+{
+ int rc = 0;
+ char *kbuf = kmalloc(count, GFP_DMA);
+ struct spi_device *dev = (struct spi_device *)file->private_data;
+
+ if (!kbuf)
+ rc = -ENOMEM;
+ else {
+ rc = spi_read(dev, SPI_M_DNA, kbuf, count);
+ if (rc < 0 || copy_to_user(buf, kbuf, count))
+ rc = -EFAULT;
+ kfree(kbuf);
+ }
+ return rc;
+}
+
+static ssize_t spidev_write(struct file *file, const char __user *buf, size_t count,
+ loff_t * offset)
+{
+ int rc = 0;
+ char *kbuf = kmalloc(count, GFP_DMA);
+ struct spi_device *dev = (struct spi_device *)file->private_data;
+
+ if (!kbuf)
+ rc = -ENOMEM;
+ else {
+ if (!copy_from_user(kbuf, buf, count))
+ rc = spi_write(dev, SPI_M_DNA, kbuf, count);
+ else
+ rc = -EFAULT;
+ kfree(kbuf);
+ }
+ return rc;
+}
+
+struct spidev_openclose {
+ unsigned int minor;
+ struct file *file;
+};
+
+static int spidev_do_open(struct device *the_dev, void *context)
+{
+ struct spidev_openclose *o = (struct spidev_openclose *)context;
+ struct spi_device *dev = TO_SPI_DEV(the_dev);
+
+ pr_debug("device->minor = %d vs %d\n", dev->minor, o->minor);
+ if (dev->minor == o->minor) {
+ get_device(&dev->dev);
+ o->file->private_data = dev;
+ return 1;
+ }
+
+ return 0;
+}
+
+int spidev_open(struct inode *inode, struct file *file)
+{
+ struct spidev_openclose o;
+ int status;
+
+ o.minor = iminor(inode);
+ o.file = file;
+ status = bus_for_each_dev(&spi_bus, NULL, &o, spidev_do_open);
+ if (status == 0) {
+ status = -ENODEV;
+ }
+ return status < 0 ? status : 0;
+}
+
+static int spidev_release(struct inode *inode, struct file *file)
+{
+ struct spi_device *dev = file->private_data;
+
+ if (dev)
+ put_device(&dev->dev);
+ file->private_data = NULL;
+
+ return 0;
+}
+
+int __init spidev_init(void)
+{
+ int res;
+
+ if ((res = register_chrdev(SPI_MAJOR, "spi", &spidev_fops)) != 0) {
+ goto out;
+ }
+
+ spidev_class = class_create(THIS_MODULE, "spi");
+ if (IS_ERR(spidev_class)) {
+ printk(KERN_ERR "%s: error creating class\n", __FUNCTION__);
+ res = -EINVAL;
+ goto out_unreg;
+ }
+
+ return 0;
+
+out_unreg:
+ unregister_chrdev(SPI_MAJOR, "spi");
+out:
+ return res;
+}
+
+void __exit spidev_cleanup(void)
+{
+ class_destroy(spidev_class);
+ unregister_chrdev(SPI_MAJOR, "spi");
+}
+
+MODULE_AUTHOR("dmitry pervushin <[email protected]>");
+MODULE_DESCRIPTION("SPI class device driver");
+MODULE_LICENSE("GPL");
Index: lsm-2.6/include/linux/spi.h
===================================================================
--- /dev/null
+++ lsm-2.6/include/linux/spi.h
@@ -0,0 +1,422 @@
+/*
+ * linux/include/linux/spi/spi.h
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ * Derived from l3.h by Jamey Hicks
+ */
+#ifndef SPI_H
+#define SPI_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+
+struct spi_device;
+struct spi_driver;
+struct spi_msg;
+struct spi_bus_driver;
+
+extern struct bus_type spi_bus;
+
+struct spi_bus_data {
+ struct semaphore lock;
+ struct list_head msgs;
+ atomic_t exiting;
+ struct task_struct *thread;
+ wait_queue_head_t queue;
+ struct spi_device *selected_device;
+ struct spi_bus_driver *bus;
+ char *id;
+};
+
+#define TO_SPI_BUS_DRIVER(drv) container_of(drv, struct spi_bus_driver, driver)
+struct spi_bus_driver {
+ int (*xfer) (struct spi_msg * msg);
+ void (*select) (struct spi_device * dev);
+ void (*deselect) (struct spi_device * dev);
+ void (*set_clock) (struct device * bus_device, u32 clock_hz);
+ void (*reset) (struct device *bus_device, u32 context);
+
+ struct spi_msg *
+ (*retrieve) (struct spi_bus_driver *bus, struct spi_bus_data *data);
+
+ void *(*alloc) (size_t, int);
+ void (*free) (const void *);
+ u8 *(*get_buffer) (struct spi_device *, void *);
+ void (*release_buffer) (struct spi_device *, unsigned char *);
+
+ struct device_driver driver;
+};
+
+#define TO_SPI_DEV(device) container_of(device, struct spi_device, dev)
+struct spi_device {
+ char name[BUS_ID_SIZE];
+ int minor;
+ struct class_device *cdev;
+ struct device dev;
+};
+
+#define TO_SPI_DRIVER(drv) container_of(drv, struct spi_driver, driver)
+struct spi_driver {
+
+ char name [BUS_ID_SIZE];
+
+ int (*probe) (struct spi_device * dev);
+ int (*remove) (struct spi_device * dev);
+ void (*shutdown) (struct spi_device * dev);
+ int (*suspend) (struct spi_device * dev, pm_message_t pm);
+ int (*resume) (struct spi_device * dev);
+
+ void (*control) (struct spi_device *, int mode, u32 ctl);
+
+ struct device_driver driver;
+};
+
+#define SPI_DEV_DRV(device) TO_SPI_BUS_DRIVER((device)->dev.parent->driver)
+
+#define spi_device_lock(spi_dev) down(&(spi_dev)->dev.sem)
+#define spi_device_unlock(spi_dev) up(&(spi_dev)->dev.sem)
+
+/*
+ * struct spi_msg
+ *
+ * This structure represent the SPI message internally. You should never use fields of
+ * this structure directly.
+ * Please use corresponding functions to create/destroy/access fields, like
+ * spimsg_alloc
+ * spimsg_free
+ * spimsg_get_buffer
+ * spimsg_put_buffer
+ * spimsg_sync
+ * spimsg_set_rdbuf
+ * spimsg_set_wrbuf
+ * @len: message length
+ * @clock: clock setting for this message sending
+ * @device: target device
+ * @context: user-supplied context for message
+ * @parent: used to hierarchically link messages
+ * @link: item of the message queue
+ * @status: user-supplied pointer to callback function
+ */
+struct spi_msg {
+ u32 flags;
+#define SPI_M_RD 0x00000001
+#define SPI_M_WR 0x00000002 /**< Write mode flag */
+#define SPI_M_CSREL 0x00000004 /**< CS release level at end of the frame */
+#define SPI_M_CS 0x00000008 /**< CS active level at begining of frame */
+#define SPI_M_CPOL 0x00000010 /**< Clock polarity */
+#define SPI_M_CPHA 0x00000020 /**< Clock Phase */
+#define SPI_M_EXTBUF 0x80000000 /** externally allocated buffers */
+#define SPI_M_ASYNC_CB 0x40000000 /** use workqueue to deliver callbacks */
+#define SPI_M_DNA 0x20000000 /** do not allocate buffers */
+
+ u16 len; /* msg length */
+ u32 clock;
+ struct spi_device *device;
+ void *context;
+ void *parent; /* in case of complex messages */
+ struct list_head link;
+
+ void (*status) (struct spi_msg * msg, int code);
+
+ void *databuf_rd, *databuf_wr;
+
+ struct work_struct wq_item;
+};
+
+#if defined (CONFIG_SPI_CHARDEV)
+extern struct class_device *spi_class_device_create(int minor, struct device *device);
+extern void spi_class_device_destroy(struct class_device *cdev);
+#else
+static inline struct class_device *spi_class_device_create(int minor, struct device *device)
+{
+ return NULL;
+}
+static inline void spi_class_device_destroy(struct class_device *cdev)
+{
+}
+#endif
+
+/**
+ * spimsg_alloc - allocate the spi message
+ *
+ * @device: target device
+ * @flags: SPI message flags
+ * @buf: user-supplied buffer
+ * @len: buffer's length
+ * @status: user-supplied callback function
+**/
+static inline struct spi_msg *spimsg_alloc(struct spi_device *device,
+ u32 flags,
+ void *buf,
+ unsigned short len,
+ void (*status) (struct spi_msg *,
+ int code))
+{
+ struct spi_msg *msg;
+ struct spi_bus_driver *drv = SPI_DEV_DRV(device);
+ int msgsize = sizeof (struct spi_msg);
+
+ if (drv->alloc || (flags & (SPI_M_RD|SPI_M_WR)) == (SPI_M_RD | SPI_M_WR)) {
+ pr_debug ("%s: external buffers\n", __FUNCTION__);
+ flags |= SPI_M_EXTBUF;
+ } else {
+ pr_debug ("%s: no ext buffers, msgsize increased from %d by %d to %d\n", __FUNCTION__,
+ msgsize, len, msgsize + len);
+ msgsize += len;
+ }
+
+ msg = kmalloc(msgsize, GFP_KERNEL);
+ if (!msg)
+ return NULL;
+ memset(msg, 0, sizeof(struct spi_msg));
+ msg->len = len;
+ msg->status = status;
+ msg->device = device;
+ msg->flags = flags;
+ INIT_LIST_HEAD(&msg->link);
+
+ if (flags & SPI_M_DNA) {
+ msg->databuf_rd = msg->databuf_wr = buf;
+ return msg;
+ }
+
+ /* otherwise, we need to set up pointers */
+ if (!(flags & SPI_M_EXTBUF)) {
+ msg->databuf_rd = msg->databuf_wr =
+ (u8*)msg + sizeof (struct spi_msg);
+ } else {
+ if (flags & SPI_M_RD) {
+ msg->databuf_rd = drv->alloc ?
+ drv->alloc(len, GFP_KERNEL) : kmalloc(len, GFP_KERNEL);
+ }
+ if (flags & SPI_M_WR) {
+ msg->databuf_wr = drv->alloc ?
+ drv->alloc(len, GFP_KERNEL) : kmalloc(len, GFP_KERNEL);
+ }
+ }
+ pr_debug("%s: msg = %p, RD=(%p) WR=(%p). Actual flags = %s+%s\n",
+ __FUNCTION__,
+ msg,
+ msg->databuf_rd,
+ msg->databuf_wr,
+ msg->flags & SPI_M_RD ? "RD" : "~rd",
+ msg->flags & SPI_M_WR ? "WR" : "~wr");
+ return msg;
+}
+
+/**
+ * spimsg_free - free the message allocated by spimsg_alloc
+ *
+ * @msg: message to free
+ **/
+static inline void spimsg_free(struct spi_msg *msg)
+{
+ void (*do_free) (const void *) = kfree;
+ struct spi_bus_driver *drv = SPI_DEV_DRV(msg->device);
+
+ if (msg) {
+
+ if (!(msg->flags & SPI_M_DNA) || (msg->flags & SPI_M_EXTBUF)) {
+ if (drv->free)
+ do_free = drv->free;
+ if (msg->databuf_rd)
+ do_free(msg->databuf_rd);
+ if (msg->databuf_wr)
+ do_free(msg->databuf_wr);
+ }
+ kfree(msg);
+ }
+}
+
+static inline void *spimsg_sync(struct spi_msg *msg, void *buf, size_t len)
+{
+ void *ret = buf;
+
+ switch (msg->flags & (SPI_M_RD|SPI_M_WR)) {
+ case SPI_M_RD:
+ if (!(msg->flags & SPI_M_DNA))
+ ret = memcpy(buf, msg->databuf_rd, len);
+ break;
+ case SPI_M_WR:
+ if (!(msg->flags & SPI_M_DNA))
+ ret = memcpy(msg->databuf_wr, buf, len);
+ break;
+ default:
+ printk(KERN_ERR "%s: what buffer do you really want ?\n",
+ __FUNCTION__);
+ buf = NULL;
+ }
+ return buf;
+}
+
+static inline u8 *spimsg_get_rdbuf(struct spi_msg *msg)
+{
+ u8 *retbuf = NULL;
+ if (msg) {
+ if (msg->flags & SPI_M_DNA || !(msg->flags & SPI_M_EXTBUF))
+ retbuf = msg->databuf_rd;
+ else
+ retbuf = SPI_DEV_DRV(msg->device)->get_buffer(msg->device, msg->databuf_rd);
+ }
+ return retbuf;
+}
+
+static inline u8 *spimsg_get_wrbuf(struct spi_msg *msg)
+{
+ u8 *retbuf = NULL;
+ if (msg) {
+ if (msg->flags & SPI_M_DNA || !(msg->flags & SPI_M_EXTBUF))
+ retbuf = msg->databuf_wr;
+ else
+ retbuf = SPI_DEV_DRV(msg->device)->get_buffer(msg->device, msg->databuf_wr);
+ }
+ return retbuf;
+}
+
+static inline void spimsg_put_rdbuf(struct spi_msg *msg)
+{
+ if (msg)
+ if (!(msg->flags & SPI_M_DNA || !(msg->flags & SPI_M_EXTBUF)))
+ SPI_DEV_DRV(msg->device)->release_buffer(msg->device, msg->databuf_rd);
+}
+
+static inline void spimsg_put_wrbuf(struct spi_msg *msg)
+{
+ if (msg)
+ if (!(msg->flags & SPI_M_DNA || !(msg->flags & SPI_M_EXTBUF)))
+ SPI_DEV_DRV(msg->device)->release_buffer(msg->device, msg->databuf_wr);
+}
+
+static inline u8 *spimsg_get_buffer(struct spi_msg *msg)
+{
+ if (!msg)
+ return NULL;
+ if ((msg->flags & (SPI_M_RD | SPI_M_WR)) == (SPI_M_RD | SPI_M_WR)) {
+ printk(KERN_ERR "%s: what buffer do you really want ?\n",
+ __FUNCTION__);
+ return NULL;
+ }
+ if (msg->flags & SPI_M_RD)
+ return spimsg_get_rdbuf(msg);
+ if (msg->flags & SPI_M_WR)
+ return spimsg_get_wrbuf(msg);
+}
+
+static inline void spimsg_put_buffer(struct spi_msg *msg)
+{
+ if (!msg)
+ return;
+ if ((msg->flags & (SPI_M_RD | SPI_M_WR)) == (SPI_M_RD | SPI_M_WR)) {
+ printk(KERN_ERR "%s: what buffer do you really want ?\n",
+ __FUNCTION__);
+ return;
+ }
+ if (msg->flags & SPI_M_RD)
+ spimsg_put_rdbuf(msg);
+ if (msg->flags & SPI_M_WR)
+ spimsg_put_wrbuf(msg);
+}
+
+static inline void spimsg_set_rd(struct spi_msg* msg, void* buf)
+{
+ msg->databuf_rd = buf;
+}
+
+static inline void spimsg_set_wr (struct spi_msg *msg, void *buf)
+{
+ msg->databuf_wr = buf;
+}
+
+
+#define SPIMSG_OK 0x01
+#define SPIMSG_FAILED 0x80
+#define SPIMSG_STARTED 0x02
+#define SPIMSG_DONE 0x04
+
+#define SPI_MAJOR 153
+
+struct spi_driver;
+struct spi_device;
+
+#if defined (CONFIG_SPI_CHARDEV)
+extern int __init spidev_init(void);
+extern void __exit spidev_cleanup(void);
+#else
+static inline int spidev_init(void)
+{
+ return 0;
+}
+static inline void spidev_cleanup(void)
+{
+}
+#endif
+
+static inline int spi_bus_driver_register (struct spi_bus_driver *bus_driver)
+{
+ return driver_register (&bus_driver->driver);
+}
+
+void spi_bus_driver_unregister(struct spi_bus_driver *);
+int spi_bus_driver_init(struct spi_bus_driver *driver, struct device *dev);
+struct spi_device* spi_device_add(struct device *parent, char *name, void *private);
+int spi_driver_probe (struct device *dev);
+int spi_driver_remove (struct device *dev);
+void spi_driver_shutdown (struct device *dev);
+int spi_driver_suspend (struct device *dev, pm_message_t state);
+int spi_driver_resume (struct device *dev);
+
+static inline int spi_driver_add(struct spi_driver *drv)
+{
+ drv->driver.bus = &spi_bus;
+ drv->driver.probe = spi_driver_probe;
+ drv->driver.remove = spi_driver_remove;
+ drv->driver.shutdown = spi_driver_shutdown;
+ drv->driver.suspend = spi_driver_suspend;
+ drv->driver.resume = spi_driver_resume;
+ drv->driver.name = drv->name;
+ return driver_register(&drv->driver);
+}
+static inline void spi_driver_del(struct spi_driver *drv)
+{
+ driver_unregister(&drv->driver);
+}
+
+extern void spi_bus_reset(struct device* bus, u32 context);
+extern int spi_write(struct spi_device *dev, u32 flags, char *buf, size_t len);
+extern int spi_read(struct spi_device *dev, u32 flags, char *buf, size_t len);
+
+extern int spi_queue(struct spi_msg *message);
+extern int spi_transfer(struct spi_msg *message,
+ void (*status) (struct spi_msg *, int));
+extern int spi_bus_populate(struct device *parent, char *device,
+ void (*assign) (struct device *parent,
+ struct spi_device *));
+struct spi_device_desc {
+ char* name;
+ void* params;
+};
+
+static inline int spi_w8r8 (struct spi_device *dev, u8 wr)
+{
+ u8 byte;
+ int rc;
+
+ rc = spi_write (dev, SPI_M_CS, &wr, 1);
+ if (rc >= 0)
+ rc = spi_read (dev, SPI_M_CSREL, &byte, 1);
+
+ return rc < 0 ? rc : byte;
+}
+
+extern int spi_bus_populate2(struct device *parent,
+ struct spi_device_desc *devices,
+ void (*assign) (struct device *parent,
+ struct spi_device *,
+ void *));
+
+#endif /* SPI_H */
Index: lsm-2.6/drivers/Makefile
===================================================================
--- lsm-2.6.orig/drivers/Makefile
+++ lsm-2.6/drivers/Makefile
@@ -67,3 +67,4 @@ obj-$(CONFIG_INFINIBAND) += infiniband/
obj-y += firmware/
obj-$(CONFIG_CRYPTO) += crypto/
obj-$(CONFIG_SUPERH) += sh/
+obj-$(CONFIG_SPI) += spi/


2005-12-01 16:18:18

by Vitaly Wool

[permalink] [raw]
Subject: [PATCH 2.6-git] MTD/SPI dataflash driver

Greetings,

this is a follow-up message on the one posted a few minutes ago containing the updated version of Pervushin/Wool's SPI core.
It took about fifteen minutes of mindwork to port the MTD dataflash driver posted by David Brownell 11/23 to our SPI core from David's, reducing both the size of the source code and the footprint of the object code.
I'd also like to mention that now it looks significatnly easier to understand due to no more use of complicated SPI message structures. The number of variables used was also decreased.
No other activity than porting the driver was performed.

Signed-off-by: Vitaly Wool <[email protected]>
Signed-off-by: dmitry pervushin <[email protected]>

drivers/mtd/devices/Kconfig | 8
drivers/mtd/devices/Makefile | 1
drivers/mtd/devices/mtd_dataflash.c | 568 ++++++++++++++++++++++++++++++++++++
include/linux/spi/flash.h | 27 +
4 files changed, 604 insertions(+)

Index: lsm-2.6/drivers/mtd/devices/Kconfig
===================================================================
--- lsm-2.6.orig/drivers/mtd/devices/Kconfig
+++ lsm-2.6/drivers/mtd/devices/Kconfig
@@ -255,5 +255,13 @@ config MTD_DOCPROBE_55AA
LinuxBIOS or if you need to recover a DiskOnChip Millennium on which
you have managed to wipe the first block.

+config MTD_DATAFLASH
+ tristate "Support for AT45xxx DataFlash"
+ depends on MTD && SPI && EXPERIMENTAL
+ help
+ This enables access to AT45xxx DataFlash chips, using SPI.
+ Sometimes DataFlash chips are packaged inside MMC-format
+ cards; at this writing, the MMC stack won't handle those.
+
endmenu

Index: lsm-2.6/include/linux/spi/flash.h
===================================================================
--- /dev/null
+++ lsm-2.6/include/linux/spi/flash.h
@@ -0,0 +1,27 @@
+#ifndef LINUX_SPI_FLASH_H
+#define LINUX_SPI_FLASH_H
+
+struct mtd_partition;
+
+/**
+ * struct flash_platform_data: board-specific flash data
+ * @name: optional flash device name (eg, as used with mtdparts=)
+ * @parts: optional array of mtd_partitions for static partitioning
+ * @nr_parts: number of mtd_partitions for static partitoning
+ *
+ * Board init code (in arch/.../mach-xxx/board-yyy.c files) can
+ * provide information about SPI flash parts (such as DataFlash) to
+ * help set up the device and its appropriate default partitioning.
+ *
+ * Note that for DataFlash, sizes for pages, blocks, and sectors are
+ * rarely powers of two; and partitions should be sector-aligned.
+ */
+struct flash_platform_data {
+ char *name;
+ struct mtd_partition *parts;
+ unsigned int nr_parts;
+
+ /* we'll likely add more ... use JEDEC IDs, etc */
+};
+
+#endif
Index: lsm-2.6/drivers/mtd/devices/Makefile
===================================================================
--- lsm-2.6.orig/drivers/mtd/devices/Makefile
+++ lsm-2.6/drivers/mtd/devices/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_MTD_MTDRAM) += mtdram.o
obj-$(CONFIG_MTD_LART) += lart.o
obj-$(CONFIG_MTD_BLKMTD) += blkmtd.o
obj-$(CONFIG_MTD_BLOCK2MTD) += block2mtd.o
+obj-$(CONFIG_MTD_DATAFLASH) += mtd_dataflash.o
Index: lsm-2.6/drivers/mtd/devices/mtd_dataflash.c
===================================================================
--- /dev/null
+++ lsm-2.6/drivers/mtd/devices/mtd_dataflash.c
@@ -0,0 +1,568 @@
+/*
+ * Atmel AT45xxx DataFlash MTD driver for lightweight SPI framework
+ *
+ * Largely derived from at91_dataflash.c:
+ * Copyright (C) 2003-2005 SAN People (Pty) Ltd
+ *
+ * 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.
+*/
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/spi.h>
+#include <linux/spi/flash.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+
+
+/*
+ * DataFlash is a kind of SPI flash. Most AT45 chips have two buffers in
+ * each chip, which may be used for double buffered I/O; but this driver
+ * doesn't (yet) use these for any kind of i/o overlap or prefetching.
+ *
+ * Sometimes DataFlash is packaged in MMC-format cards. This driver does
+ * not (yet) expect to be hotplugged through MMC or SPI. That's fair,
+ * since the MMC stack can't even use SPI (yet), much less distinguish
+ * between MMC and DataFlash protocols during enumeration.
+ */
+
+#define CONFIG_DATAFLASH_WRITE_VERIFY
+
+
+/* reads can bypass the buffers */
+#define OP_READ_CONTINUOUS 0xE8
+#define OP_READ_PAGE 0xD2
+
+/* group B requests can run even while status reports "busy" */
+#define OP_READ_STATUS 0xD7 /* group B */
+
+/* move data between host and buffer */
+#define OP_READ_BUFFER1 0xD4 /* group B */
+#define OP_READ_BUFFER2 0xD6 /* group B */
+#define OP_WRITE_BUFFER1 0x84 /* group B */
+#define OP_WRITE_BUFFER2 0x87 /* group B */
+
+/* erasing flash */
+#define OP_ERASE_PAGE 0x81
+#define OP_ERASE_BLOCK 0x50
+
+/* move data between buffer and flash */
+#define OP_TRANSFER_BUF1 0x53
+#define OP_TRANSFER_BUF2 0x55
+#define OP_MREAD_BUFFER1 0xD4
+#define OP_MREAD_BUFFER2 0xD6
+#define OP_MWERASE_BUFFER1 0x83
+#define OP_MWERASE_BUFFER2 0x86
+#define OP_MWRITE_BUFFER1 0x88 /* sector must be pre-erased */
+#define OP_MWRITE_BUFFER2 0x89 /* sector must be pre-erased */
+
+/* write to buffer, then write-erase to flash */
+#define OP_PROGRAM_VIA_BUF1 0x82
+#define OP_PROGRAM_VIA_BUF2 0x85
+
+/* compare buffer to flash */
+#define OP_COMPARE_BUF1 0x60
+#define OP_COMPARE_BUF2 0x61
+
+/* read flash to buffer, then write-erase to flash */
+#define OP_REWRITE_VIA_BUF1 0x58
+#define OP_REWRITE_VIA_BUF2 0x59
+
+/* newer chips report JEDEC manufacturer and device IDs; chip
+ * serial number and OTP bits; and per-sector writeprotect.
+ */
+#define OP_READ_ID 0x9F
+#define OP_READ_SECURITY 0x77
+#define OP_WRITE_SECURITY 0x9A /* OTP bits */
+
+
+struct dataflash {
+ u8 command[4];
+ char name[24];
+
+ unsigned short page_offset; /* offset in flash address */
+ unsigned int page_size; /* of bytes per page */
+
+ struct semaphore lock;
+ struct spi_device *spi;
+
+ struct mtd_info mtd;
+};
+
+/* ......................................................................... */
+
+/*
+ * Return the status of the DataFlash device.
+ */
+static inline int dataflash_status(struct spi_device *spi)
+{
+ /* NOTE: at45db321c over 25 MHz wants to write
+ * a dummy byte after the opcode...
+ */
+ return spi_w8r8(spi, OP_READ_STATUS);
+}
+
+/*
+ * Poll the DataFlash device until it is READY.
+ * This usually takes 5-20 msec or so; more for sector erase.
+ */
+static int dataflash_waitready(struct spi_device *spi)
+{
+ int status;
+
+ for (;;) {
+ status = dataflash_status(spi);
+ if (status < 0) {
+ DEBUG(MTD_DEBUG_LEVEL1, "%s: status %d?\n",
+ spi->dev.bus_id, status);
+ status = 0;
+ }
+
+ if (status & (1 << 7)) /* RDY/nBSY */
+ return status;
+
+ msleep(3);
+ }
+}
+
+/* ......................................................................... */
+
+/*
+ * Erase pages of flash.
+ */
+static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+ struct dataflash *priv = (struct dataflash *)mtd->priv;
+ struct spi_device *spi = priv->spi;
+ u8 *command;
+
+ DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=%x len %x\n",
+ spi->dev.bus_id,
+ instr->addr, instr->len);
+
+ /* Sanity checks */
+ if ((instr->addr + instr->len) > mtd->size
+ || (instr->len % priv->page_size) != 0
+ || (instr->addr % priv->page_size) != 0)
+ return -EINVAL;
+
+ command = priv->command;
+
+ down(&priv->lock);
+ while (instr->len > 0) {
+ unsigned int pageaddr;
+ int status;
+
+ /* Calculate flash page address */
+ // REVISIT: pageaddr == instr->addr, yes???
+ pageaddr = (instr->addr / priv->page_size) << priv->page_offset;
+
+ /* REVISIT block erase is faster, if this page is at a block
+ * boundary and the next eight pages must all be erased ...
+ */
+ command[0] = OP_ERASE_PAGE;
+ command[1] = (u8)(pageaddr >> 16);
+ command[2] = (u8)(pageaddr >> 8);
+ command[3] = 0;
+
+ DEBUG(MTD_DEBUG_LEVEL3, "ERASE: (%x) %x %x %x [%i]\n",
+ command[0], command[1], command[2], command[3],
+ pageaddr);
+
+ status = spi_write (spi, SPI_M_CS | SPI_M_CSREL, command, 4);
+ (void) dataflash_waitready(spi);
+
+ if (status < 0) {
+ printk(KERN_ERR "%s: erase %x, err %d\n",
+ spi->dev.bus_id, pageaddr, status);
+ continue;
+ }
+
+ instr->addr += priv->page_size; /* next page */
+ instr->len -= priv->page_size;
+ }
+ up(&priv->lock);
+
+ /* Inform MTD subsystem that erase is complete */
+ instr->state = MTD_ERASE_DONE;
+ mtd_erase_callback(instr);
+
+ return 0;
+}
+
+/*
+ * Read from the DataFlash device.
+ * from : Start offset in flash device
+ * len : Amount to read
+ * retlen : About of data actually read
+ * buf : Buffer containing the data
+ */
+static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
+ size_t *retlen, u_char *buf)
+{
+ struct dataflash *priv = (struct dataflash *)mtd->priv;
+ unsigned int addr;
+ u8 *command;
+ int status;
+
+ DEBUG(MTD_DEBUG_LEVEL2, "%s: read %x..%x\n",
+ priv->spi->dev.bus_id, (unsigned)from, (unsigned)(from + len));
+
+ *retlen = 0;
+
+ /* Sanity checks */
+ if (!len)
+ return 0;
+ if (from + len > mtd->size)
+ return -EINVAL;
+
+ /* Calculate flash page/byte address */
+ addr = (((unsigned)from / priv->page_size) << priv->page_offset)
+ + ((unsigned)from % priv->page_size);
+
+ command = priv->command;
+
+ DEBUG(MTD_DEBUG_LEVEL3, "READ: (%x) %x %x %x\n",
+ command[0], command[1], command[2], command[3]);
+
+ down(&priv->lock);
+
+ /* Continuous read, max clock = f(car) which may be less than
+ * the peak rate available. Some chips support commands with
+ * fewer "don't care" bytes. Both buffers stay unchanged.
+ */
+ command[0] = OP_READ_CONTINUOUS;
+ command[1] = (u8)(addr >> 16);
+ command[2] = (u8)(addr >> 8);
+ command[3] = (u8)(addr >> 0);
+ /* plus 4 "don't care" bytes */
+
+ spi_read (priv->spi, SPI_M_CS, command, 8);
+ status = spi_read (priv->spi, SPI_M_CSREL, buf, len);
+
+ up(&priv->lock);
+
+ if (status >= 0) {
+ *retlen = status;
+ status = 0;
+ } else
+ DEBUG(MTD_DEBUG_LEVEL1, "%s: read %x..%x --> %d\n",
+ priv->spi->dev.bus_id,
+ (unsigned)from, (unsigned)(from + len),
+ status);
+ return status;
+}
+
+/*
+ * Write to the DataFlash device.
+ * to : Start offset in flash device
+ * len : Amount to write
+ * retlen : Amount of data actually written
+ * buf : Buffer containing the data
+ */
+static int dataflash_write(struct mtd_info *mtd, loff_t to, size_t len,
+ size_t * retlen, const u_char * buf)
+{
+ struct dataflash *priv = (struct dataflash *)mtd->priv;
+ struct spi_device *spi = priv->spi;
+ unsigned int pageaddr, addr, offset, writelen;
+ size_t remaining = len;
+ u_char *writebuf = (u_char *) buf;
+ int status = -EINVAL;
+ u8 *command = priv->command;
+
+ DEBUG(MTD_DEBUG_LEVEL2, "%s: write %x..%x\n",
+ spi->dev.bus_id, (unsigned)to, (unsigned)(to + len));
+
+ *retlen = 0;
+
+ /* Sanity checks */
+ if (!len)
+ return 0;
+ if ((to + len) > mtd->size)
+ return -EINVAL;
+
+ pageaddr = ((unsigned)to / priv->page_size);
+ offset = ((unsigned)to % priv->page_size);
+ if (offset + len > priv->page_size)
+ writelen = priv->page_size - offset;
+ else
+ writelen = len;
+
+ down(&priv->lock);
+ while (remaining > 0) {
+ DEBUG(MTD_DEBUG_LEVEL3, "write @ %i:%i len=%i\n",
+ pageaddr, offset, writelen);
+
+ /* REVISIT:
+ * (a) each page in a sector must be rewritten at least
+ * once every 10K sibling erase/program operations.
+ * (b) for pages that are already erased, we could
+ * use WRITE+MWRITE not PROGRAM for ~30% speedup.
+ * (c) WRITE to buffer could be done while waiting for
+ * a previous MWRITE/MWERASE to complete ...
+ * (d) error handling here seems to be mostly missing.
+ *
+ * Two persistent bits per page, plus a per-sector counter,
+ * could support (a) and (b) ... we might consider using
+ * the second half of sector zero, which is just one block,
+ * to track that state. (On AT91, that sector should also
+ * support boot-from-DataFlash.)
+ */
+
+ addr = pageaddr << priv->page_offset;
+
+ /* (1) Maybe transfer partial page to Buffer1 */
+ if (writelen != priv->page_size) {
+ command[0] = OP_TRANSFER_BUF1;
+ command[1] = (addr & 0x00FF0000) >> 16;
+ command[2] = (addr & 0x0000FF00) >> 8;
+ command[3] = 0;
+
+ DEBUG(MTD_DEBUG_LEVEL3, "TRANSFER: (%x) %x %x %x\n",
+ command[0], command[1], command[2], command[3]);
+
+ spi_write (spi, SPI_M_CS | SPI_M_CSREL, command, 4);
+ if (status < 0)
+ DEBUG(MTD_DEBUG_LEVEL1, "%s: xfer %u -> %d \n",
+ spi->dev.bus_id, addr, status);
+
+ (void) dataflash_waitready(priv->spi);
+ }
+
+ /* (2) Program full page via Buffer1 */
+ addr += offset;
+ command[0] = OP_PROGRAM_VIA_BUF1;
+ command[1] = (addr & 0x00FF0000) >> 16;
+ command[2] = (addr & 0x0000FF00) >> 8;
+ command[3] = (addr & 0x000000FF);
+
+ DEBUG(MTD_DEBUG_LEVEL3, "PROGRAM: (%x) %x %x %x\n",
+ command[0], command[1], command[2], command[3]);
+
+ spi_write (spi, SPI_M_CS, command, 4);
+ spi_write (spi, SPI_M_CSREL, writebuf, writelen);
+
+ if (status < 0)
+ DEBUG(MTD_DEBUG_LEVEL1, "%s: pgm %u/%u -> %d \n",
+ spi->dev.bus_id, addr, writelen, status);
+
+ (void) dataflash_waitready(priv->spi);
+
+#ifdef CONFIG_DATAFLASH_WRITE_VERIFY
+
+ /* (3) Compare to Buffer1 */
+ addr = pageaddr << priv->page_offset;
+ command[0] = OP_COMPARE_BUF1;
+ command[1] = (addr & 0x00FF0000) >> 16;
+ command[2] = (addr & 0x0000FF00) >> 8;
+ command[3] = 0;
+
+ DEBUG(MTD_DEBUG_LEVEL3, "COMPARE: (%x) %x %x %x\n",
+ command[0], command[1], command[2], command[3]);
+
+ status = spi_write (spi, SPI_M_CS | SPI_M_CSREL, command, 4);
+ if (status < 0)
+ DEBUG(MTD_DEBUG_LEVEL1, "%s: compare %u -> %d \n",
+ spi->dev.bus_id, addr, status);
+
+ status = dataflash_waitready(priv->spi);
+
+ /* Check result of the compare operation */
+ if ((status & (1 << 6)) == 1) {
+ printk(KERN_ERR "%s: compare page %u, err %d\n",
+ spi->dev.bus_id, pageaddr, status);
+ remaining = 0;
+ status = -EIO;
+ break;
+ } else
+ status = 0;
+
+#endif /* CONFIG_DATAFLASH_WRITE_VERIFY */
+
+ remaining = remaining - writelen;
+ pageaddr++;
+ offset = 0;
+ writebuf += writelen;
+ *retlen += writelen;
+
+ if (remaining > priv->page_size)
+ writelen = priv->page_size;
+ else
+ writelen = remaining;
+ }
+ up(&priv->lock);
+
+ return status;
+}
+
+/* ......................................................................... */
+
+/*
+ * Register DataFlash device with MTD subsystem.
+ */
+static int __init
+add_dataflash(struct spi_device *spi, char *name,
+ int nr_pages, int pagesize, int pageoffset)
+{
+ struct dataflash *priv;
+ struct mtd_info *device;
+ struct flash_platform_data *pdata = spi->dev.platform_data;
+
+ priv = (struct dataflash *) kzalloc(sizeof *priv, GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ init_MUTEX(&priv->lock);
+ priv->spi = spi;
+ priv->page_size = pagesize;
+ priv->page_offset = pageoffset;
+
+ /* name must be usable with cmdlinepart */
+ sprintf(priv->name, "mtd-%s",
+ spi->dev.parent->bus_id); /* mtd-spipnx0 */
+
+ device = &priv->mtd;
+ device->name = (pdata && pdata->name) ? pdata->name : priv->name;
+ device->size = nr_pages * pagesize;
+ device->erasesize = pagesize;
+ device->owner = THIS_MODULE;
+ device->type = MTD_DATAFLASH;
+ device->flags = MTD_CAP_NORFLASH;
+ device->erase = dataflash_erase;
+ device->read = dataflash_read;
+ device->write = dataflash_write;
+ device->priv = priv;
+
+ dev_info(&spi->dev, "%s (%d KBytes)\n", name, device->size/1024);
+ dev_set_drvdata(&spi->dev, priv);
+
+#ifdef CONFIG_MTD_PARTITIONS
+ {
+ struct mtd_partition *parts;
+ int nr_parts = 0;
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+ static const char *part_probes[] = { "cmdlinepart", NULL, };
+
+ nr_parts = parse_mtd_partitions(device, part_probes, &parts, 0);
+#endif
+
+ if (nr_parts <= 0 && pdata && pdata->parts) {
+ parts = pdata->parts;
+ nr_parts = pdata->nr_parts;
+ }
+
+ if (nr_parts > 0)
+ return add_mtd_partitions(device, parts, nr_parts);
+ }
+#endif
+ return add_mtd_device(device);
+}
+
+/*
+ * Detect and initialize DataFlash device:
+ *
+ * Device Density ID code #Pages PageSize Offset
+ * AT45DB011B 1Mbit (128K) xx0011xx (0x0c) 512 264 9
+ * AT45DB021B 2Mbit (256K) xx0101xx (0x14) 1025 264 9
+ * AT45DB041B 4Mbit (512K) xx0111xx (0x1c) 2048 264 9
+ * AT45DB081B 8Mbit (1M) xx1001xx (0x24) 4096 264 9
+ * AT45DB0161B 16Mbit (2M) xx1011xx (0x2c) 4096 528 10
+ * AT45DB0321B 32Mbit (4M) xx1101xx (0x34) 8192 528 10
+ * AT45DB0642 64Mbit (8M) xx111xxx (0x3c) 8192 1056 11
+ * AT45DB1282 128Mbit (16M) xx0100xx (0x10) 16384 1056 11
+ */
+static int __init dataflash_probe(struct spi_device *spi)
+{
+ int status;
+
+ /* if there's a device there, assume it's dataflash.
+ * board setup should have set spi->max_speed_max to
+ * match f(car) for continuous reads, mode 0 or 3.
+ */
+ status = dataflash_status(spi);
+ if (status > 0 && status != 0xff) {
+ switch (status & 0x3c) {
+ case 0x0c: /* 0 0 1 1 x x */
+ status = add_dataflash(spi, "AT45DB011B",
+ 512, 264, 9);
+ break;
+ case 0x14: /* 0 1 0 1 x x */
+ status = add_dataflash(spi, "AT45DB021B",
+ 1025, 264, 9);
+ break;
+ case 0x1c: /* 0 1 1 1 x x */
+ status = add_dataflash(spi, "AT45DB041B",
+ 2048, 264, 9);
+ break;
+ case 0x24: /* 1 0 0 1 x x */
+ status = add_dataflash(spi, "AT45DB081B",
+ 4096, 264, 9);
+ break;
+ case 0x2c: /* 1 0 1 1 x x */
+ status = add_dataflash(spi, "AT45DB161x",
+ 4096, 528, 10);
+ break;
+ case 0x34: /* 1 1 0 1 x x */
+ status = add_dataflash(spi, "AT45DB321x",
+ 8192, 528, 10);
+ break;
+ case 0x38: /* 1 1 1 x x x */
+ case 0x3c:
+ status = add_dataflash(spi, "AT45DB642x",
+ 8192, 1056, 11);
+ break;
+ /* obsolete AT45DB1282 not (yet?) supported */
+ default:
+ printk(KERN_WARNING "%s: unsupported device (%x)\n",
+ spi->dev.bus_id, status & 0x3c);
+ status = -EINVAL;
+ }
+ }
+
+ return status;
+}
+
+static int __exit dataflash_remove(struct spi_device *spi)
+{
+ // struct dataflash *flash = dev_get_drvdata(dev);
+
+ dev_warn(&spi->dev, "implement remove!\n");
+ return -EINVAL;
+}
+
+static struct spi_driver dataflash_driver = {
+ .name = "mtd_dataflash",
+
+ .probe = dataflash_probe,
+ .remove = __exit_p(dataflash_remove),
+
+ /* FIXME: investigate suspend and resume... */
+};
+
+static int __init dataflash_init(void)
+{
+ return spi_driver_add(&dataflash_driver);
+}
+module_init(dataflash_init);
+
+#if 0
+static void __exit dataflash_exit(void)
+{
+ driver_unregister(&dataflash_driver);
+}
+module_exit(dataflash_exit);
+#endif
+
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andrew Victor, David Brownell");
+MODULE_DESCRIPTION("MTD DataFlash driver");

2005-12-01 16:21:14

by Russell King

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Thu, Dec 01, 2005 at 07:11:09PM +0300, Vitaly Wool wrote:
> The main changes are:
>
> - Matching rmk's 2.6.14-git5+ changes for device_driver suspend and
> resume calls
> - Matching rmk's request to get rid of device_driver's calls to
> suspend/resume/probe/remove

Thanks. Please see comments below though.

> +iii. struct spi_driver
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +struct spi_driver {
> + void* (*alloc)( size_t, int );
> + void (*free)( const void* );
> + unsigned char* (*get_buffer)( struct spi_device*, void* );
> + void (*release_buffer)( struct spi_device*, unsigned char*);
> + void (*control)( struct spi_device*, int mode, u32 ctl );
> + struct device_driver driver;
> +};

This doesn't appear to have been updated.

> +formed spi_driver structure:
> + extern struct bus_type spi_bus;
> + static struct spi_driver pnx4008_eeprom_driver = {
> + .driver = {
> + .bus = &spi_bus,
> + .name = "pnx4008-eeprom",
> + .probe = pnx4008_spiee_probe,
> + .remove = pnx4008_spiee_remove,
> + .suspend = pnx4008_spiee_suspend,
> + .resume = pnx4008_spiee_resume,
> + },
> +};

Ditto.

> +iv. struct spi_bus_driver
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +To handle transactions over the SPI bus, the spi_bus_driver structure must
> +be prepared and registered with core. Any transactions, either synchronous
> +or asynchronous, go through spi_bus_driver->xfer function.
> +
> +struct spi_bus_driver
> +{
> + int (*xfer)( struct spi_msg* msgs );
> + void (*select) ( struct spi_device* arg );
> + void (*deselect)( struct spi_device* arg );
> +
> + struct spi_msg *(*retrieve)( struct spi_bus_driver *this, struct spi_bus_data *bd);
> + void (*reset)( struct spi_bus_driver *this, u32 context);
> +
> + struct device_driver driver;
> +};

Does this need updating as well?

> +spi_bus_driver structure:
> + static struct spi_bus_driver spipnx_driver = {
> + .driver = {
> + .bus = &platform_bus_type,
> + .name = "spipnx",
> + .probe = spipnx_probe,
> + .remove = spipnx_remove,
> + .suspend = spipnx_suspend,
> + .resume = spipnx_resume,
> + },
> + .xfer = spipnx_xfer,
> +};

>From this it looks like it does.

> +/**
> + * spi_bus_suspend - suspend all devices on the spi bus
> + *
> + * @dev: spi device to be suspended
> + * @message: PM message
> + *
> + * This function set device on SPI bus to suspended state, just like platform_bus does
> +**/
> +static int spi_bus_suspend(struct device * dev, pm_message_t message)
> +{
> + int ret = 0;
> +
> + if (dev && dev->driver && dev->driver->suspend ) {
> + ret = TO_SPI_DRIVER(dev->driver)->suspend( TO_SPI_DEV(dev), message);
> + if (ret == 0 )
> + dev->power.power_state = message;
> + }
> + return ret;
> +}
> +
> +/**
> + * spi_bus_resume - resume functioning of all devices on spi bus
> + *
> + * @dev: device to resume
> + *
> + * This function resumes device on SPI bus, just like platform_bus does
> +**/
> +static int spi_bus_resume(struct device * dev)
> +{
> + int ret = 0;
> +
> + if (dev && dev->driver && dev->driver->suspend ) {
> + ret = TO_SPI_DRIVER(dev->driver)->resume(TO_SPI_DEV(dev));
> + if (ret == 0)
> + dev->power.power_state = PMSG_ON;
> + }
> + return ret;
> +}

Ok, your bus_type suspend/resume methods call your spi_driver's suspend
and resume methods - good.

> +/*
> + * the functions below are wrappers for corresponding device_driver's methods
> + */
> +int spi_driver_probe (struct device *dev)
> +{
> + struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
> + struct spi_device *sdev = TO_SPI_DEV(dev);
> +
> + return sdrv && sdrv->probe ? sdrv->probe(sdev) : -EFAULT;
> +}
> +
> +int spi_driver_remove (struct device *dev)
> +{
> + struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
> + struct spi_device *sdev = TO_SPI_DEV(dev);
> +
> + return (sdrv && sdrv->remove) ? sdrv->remove(sdev) : -EFAULT;
> +}

These are fine, although sdrv will always be non-NULL here.

> +
> +void spi_driver_shutdown (struct device *dev)
> +{
> + struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
> + struct spi_device *sdev = TO_SPI_DEV(dev);
> +
> + if (sdrv && sdrv->shutdown)
> + sdrv->shutdown(sdev);
> +}

dev->driver may be NULL here. If it is NULL, sdrv will not be.
Hence you want to do:

if (dev->driver && sdrv->shutdown)

instead.

> +
> +int spi_driver_suspend (struct device *dev, pm_message_t pm)
> +{
> + struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
> + struct spi_device *sdev = TO_SPI_DEV(dev);
> +
> + return (sdrv && sdrv->suspend) ? sdrv->suspend(sdev, pm) : -EFAULT;
> +}
> +
> +int spi_driver_resume (struct device *dev)
> +{
> + struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
> + struct spi_device *sdev = TO_SPI_DEV(dev);
> +
> + return (sdrv && sdrv->resume) ? sdrv->resume(sdev) : -EFAULT;
> +}

If the bus_type does not call the device_driver suspend/resume methods,
these are not necessary.

Another oddity I notice is that if there isn't a driver or method, you're
returning -EFAULT - seems odd since if there isn't a driver do you really
want to prevent suspend/resume?

> +static inline int spi_driver_add(struct spi_driver *drv)
> +{
> + drv->driver.bus = &spi_bus;
> + drv->driver.probe = spi_driver_probe;
> + drv->driver.remove = spi_driver_remove;
> + drv->driver.shutdown = spi_driver_shutdown;

> + drv->driver.suspend = spi_driver_suspend;
> + drv->driver.resume = spi_driver_resume;

As a result of the comment above, you don't need these two initialisers.

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

2005-12-01 16:30:34

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

Russell King wrote:

>>+iii. struct spi_driver
>>+~~~~~~~~~~~~~~~~~~~~~~
>>+
>>+struct spi_driver {
>>+ void* (*alloc)( size_t, int );
>>+ void (*free)( const void* );
>>+ unsigned char* (*get_buffer)( struct spi_device*, void* );
>>+ void (*release_buffer)( struct spi_device*, unsigned char*);
>>+ void (*control)( struct spi_device*, int mode, u32 ctl );
>>+ struct device_driver driver;
>>+};
>>
>>
>
>This doesn't appear to have been updated.
>
>
>
>>+formed spi_driver structure:
>>+ extern struct bus_type spi_bus;
>>+ static struct spi_driver pnx4008_eeprom_driver = {
>>+ .driver = {
>>+ .bus = &spi_bus,
>>+ .name = "pnx4008-eeprom",
>>+ .probe = pnx4008_spiee_probe,
>>+ .remove = pnx4008_spiee_remove,
>>+ .suspend = pnx4008_spiee_suspend,
>>+ .resume = pnx4008_spiee_resume,
>>+ },
>>+};
>>
>>
>
>Ditto.
>
>
>
>>+iv. struct spi_bus_driver
>>+~~~~~~~~~~~~~~~~~~~~~~~~~
>>+To handle transactions over the SPI bus, the spi_bus_driver structure must
>>+be prepared and registered with core. Any transactions, either synchronous
>>+or asynchronous, go through spi_bus_driver->xfer function.
>>+
>>+struct spi_bus_driver
>>+{
>>+ int (*xfer)( struct spi_msg* msgs );
>>+ void (*select) ( struct spi_device* arg );
>>+ void (*deselect)( struct spi_device* arg );
>>+
>>+ struct spi_msg *(*retrieve)( struct spi_bus_driver *this, struct spi_bus_data *bd);
>>+ void (*reset)( struct spi_bus_driver *this, u32 context);
>>+
>>+ struct device_driver driver;
>>+};
>>
>>
>
>Does this need updating as well?
>
>
>
>>+spi_bus_driver structure:
>>+ static struct spi_bus_driver spipnx_driver = {
>>+ .driver = {
>>+ .bus = &platform_bus_type,
>>+ .name = "spipnx",
>>+ .probe = spipnx_probe,
>>+ .remove = spipnx_remove,
>>+ .suspend = spipnx_suspend,
>>+ .resume = spipnx_resume,
>>+ },
>>+ .xfer = spipnx_xfer,
>>+};
>>
>>
>
>From this it looks like it does.
>
>
>
Yep, thanks for pointing that out. The Documentation part needs to be
updated. Will do soon.

>
>
>>+
>>+int spi_driver_suspend (struct device *dev, pm_message_t pm)
>>+{
>>+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
>>+ struct spi_device *sdev = TO_SPI_DEV(dev);
>>+
>>+ return (sdrv && sdrv->suspend) ? sdrv->suspend(sdev, pm) : -EFAULT;
>>+}
>>+
>>+int spi_driver_resume (struct device *dev)
>>+{
>>+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
>>+ struct spi_device *sdev = TO_SPI_DEV(dev);
>>+
>>+ return (sdrv && sdrv->resume) ? sdrv->resume(sdev) : -EFAULT;
>>+}
>>
>>
>
>If the bus_type does not call the device_driver suspend/resume methods,
>these are not necessary.
>
>Another oddity I notice is that if there isn't a driver or method, you're
>returning -EFAULT - seems odd since if there isn't a driver do you really
>want to prevent suspend/resume?
>
>
Yep, I must admit here that it's really better to do something like
if (!dev->driver)
return 0;
else if (sdrv->suspend)
return sdrv->suspend(...)

Does that sound better to you?

>
>
>>+static inline int spi_driver_add(struct spi_driver *drv)
>>+{
>>+ drv->driver.bus = &spi_bus;
>>+ drv->driver.probe = spi_driver_probe;
>>+ drv->driver.remove = spi_driver_remove;
>>+ drv->driver.shutdown = spi_driver_shutdown;
>>
>>
>
>
>
>>+ drv->driver.suspend = spi_driver_suspend;
>>+ drv->driver.resume = spi_driver_resume;
>>
>>
>
>As a result of the comment above, you don't need these two initialisers.
>
>
>
Yup. Thanks.

Vitaly

2005-12-01 18:04:29

by Stephen Street

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Thu, 2005-12-01 at 19:11 +0300, Vitaly Wool wrote:
> Again, some advantages of our core compared to David's I'd like to mention
>
> - it can be compiled as a module
> - it is priority inversion-free
> - it can gain more performance with multiple controllers
> - it's more adapted for use in real-time environments
> - it's not so lightweight, but it leaves less effort for the bus driver developer.
>
> (As a response to the last bullet David claims that we limit the flexibility. It's not correct.
> The core method for message retrieval is just a default one and can easily be overridden by the bus driver. It's a common practice, for instance, see MTD/NAND interface.)
>
> It's also been proven to work on SPI EEPROMs and SPI WiFi module (the latter was a really good survival test! :)).

I have a question about your proposed core. But first a little
background:

My board has a 3 Cirrus Logic SPI devices (CS8415A, CS8405A and a
CS4341) connected to a PXA255 NSSP port. I have implemented the PXA2xx
NSSP SPI driver with DMA support using Davids framework and implemented
an working CS8415A driver.

Page 18 of the CS8415A data sheet discusses the SPI IO operation. Three
paragraphs and 1 timing diagram.

http://www.cirrus.com/en/pubs/proDatasheet/CS8415A_F4.pdf

The critical things to get from the datasheet are:

1) The chip has an internal register file pointer MAP which must be
positioned before write and read register operations.

2) The MAP has a auto-increment feature.

3) Register writes can be performed in one chip select cycles while
register reads MAY require a MAP write cycle first and thus require two
chip select cycles.

Now assume the CS8415A register operations will be generated from two
different sources: "process context" and "interrupt context". This
assumption forces a "guaranteed message order" requirement onto the IO
Model because of the possibility that an "interrupt context" will move
the MAP in between an "process context" write MAP message and read
register message. If this is not clear, let me know because it is
important.

I'm using David's SPI IO model to enforce "guaranteed message order" by
building multiple write/read transfers in a single SPI message which is
guaranteed execute in the correct order.

How do I accomplish the same with your core?

-Stephen





2005-12-01 18:24:43

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Thu, Dec 01, 2005 at 07:11:09PM +0300, Vitaly Wool wrote:
> - Matching Greg K-H's requests expressed in his review

No, you got the kernel doc wrong, and you didn't move your inline
functions.

> - The character device interface was reworked

reworked how?

> - it's more adapted for use in real-time environments

I still do not see why you are stating this. Why do you say this?

I think you should work with David more...

thanks,

greg k-h

2005-12-01 18:33:44

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] MTD/SPI dataflash driver

On Thursday 01 December 2005 8:18 am, Vitaly Wool wrote:

> It took about fifteen minutes of mindwork to port the MTD dataflash driver
> posted by David Brownell 11/23 to our SPI core from David's,

Interesting and informative; +1!

Did you have a chance to test this? There are some post-2.6.15-rc3-mm1
updates to this code, mostly to catch startup fauults better but also to
hotplug reasonably. The glitches I saw may as easily be JFFS2 integration
issues for the DataFlash code as bitbang adapter problems. (I think you
started more or less from what's in rc3-mm1.)


> reducing both
> the size of the source code and the footprint of the object code.
> I'd also like to mention that now it looks significatnly easier to understand
> due to no more use of complicated SPI message structures. The number of variables
> used was also decreased

I think that's all the same issue. Other than "spi_driver" replacing
"device_driver" (I'd like to see a patch doing that to rc3-mm1), the
main changes seem to be:

- Move from original atomic requests like this

{ write command, read response }

over to two separate requests

{ write command, and leave CS active }
{ read response, and leave CS off }

- Lower the per-request performance ceiling on this driver

* original code could be implemented in a single DMA chain on
at least two systems I happen to have handy ... one IRQ.

* this version requires two separate chains, with an intervening
task schedule. (More than twice the cost.)

* in your framework I still can't be _sure_ it never does memcpy
for those buffers (the last version I looked at did so). the
original code just used normal DMA models, so it "obviously"
doesn't risk memcpy.

- Add fault handling problems ... probably an oversight, you call
routines and don't check for fault reports. Though I'm not clear
how the faults between the two requests would be handled (in your
framework) with any robustness... doing this right could easily
cost all the codespace you've conserved.

- I might agree with this being "easier to understand" code, though
it's debatable. (The device sees one transaction, why should the
driver writer have to split it up into two?) But that doesn't
matter much: filesystems are better with "faster to run" code.

- Chipselect works differently in your code. You're giving one
driver control over all the devices sharing a controller, by
blocking requests going to other devices until your driver yields
the chipselect. This seems like a bad idea even if you ignore
how it produces priority inversions in your framework. :)

So the way I see it, this is a good example of why my original I/O model
is better. It provides _flexibility_ in the API so that some drivers
can be really smart, if they want to. You haven't liked the consequence
of that though: controller drivers being able to choose how they
represent and manage I/O queues, rather than doing that in your "core".

- Dave



2005-12-02 06:01:56

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] MTD/SPI dataflash driver

David Brownell wrote:

>On Thursday 01 December 2005 8:18 am, Vitaly Wool wrote:
>
>
>
>>It took about fifteen minutes of mindwork to port the MTD dataflash driver
>>posted by David Brownell 11/23 to our SPI core from David's,
>>
>>
>
>Interesting and informative; +1!
>
>Did you have a chance to test this? There are some post-2.6.15-rc3-mm1
>updates to this code, mostly to catch startup fauults better but also to
>hotplug reasonably. The glitches I saw may as easily be JFFS2 integration
>issues for the DataFlash code as bitbang adapter problems. (I think you
>started more or less from what's in rc3-mm1.)
>
>
>
Very basically. I've make a note that it was a mindwork.I also didn't
make the attempt to make it a clean MTD driver.
However, it would be nice if someone compared the performance of both
variants.

>
>
>>reducing both
>>the size of the source code and the footprint of the object code.
>>I'd also like to mention that now it looks significatnly easier to understand
>>due to no more use of complicated SPI message structures. The number of variables
>>used was also decreased
>>
>>
>
>I think that's all the same issue. Other than "spi_driver" replacing
>"device_driver" (I'd like to see a patch doing that to rc3-mm1), the
>main changes seem to be:
>
> - Move from original atomic requests like this
>
> { write command, read response }
>
> over to two separate requests
>
> { write command, and leave CS active }
> { read response, and leave CS off }
>
> - Lower the per-request performance ceiling on this driver
>
> * original code could be implemented in a single DMA chain on
> at least two systems I happen to have handy ... one IRQ.
>
>
Whoops. Lemme express my thoughts here.
First, the DMA controller that can handle chained requests which are
working with different devices (i.e. write then read does that - first
mem2per, then per2mem) are *very* rare thing.
Then, what _really_ can happen in a single DMA chain? 99,9999% cases of
such will be "read x bytes to get aware of the length of the packet,
then read the data packet". Here you won't have any noticable
performance gain as the first transfer is to small. It's soooo small
that the overhead of having a linked list for DMA will probably be
bigger than that of having two transfers. A smart controller driver may
even want to have a PIO read on first x bytes (as x will be equal to 2
or 4, I guess :)

> * this version requires two separate chains, with an intervening
> task schedule. (More than twice the cost.)
>
>
See above.

> * in your framework I still can't be _sure_ it never does memcpy
> for those buffers (the last version I looked at did so). the
> original code just used normal DMA models, so it "obviously"
> doesn't risk memcpy.
>
>
You can be sure if you set SPI_M_DNA flag. I'll update the Doc accordingly.

> - I might agree with this being "easier to understand" code, though
> it's debatable. (The device sees one transaction, why should the
> driver writer have to split it up into two?) But that doesn't
> matter much: filesystems are better with "faster to run" code.
>
>
Your clains that the originl code is faster are arguable.

> - Chipselect works differently in your code. You're giving one
> driver control over all the devices sharing a controller, by
> blocking requests going to other devices until your driver yields
> the chipselect.
>
>
The controllers we were working with are _not_ able to handle
simultaneous requests to different devices on the same bus.
Can you please tell me what specific controller you're referring to?

>So the way I see it, this is a good example of why my original I/O model
>is better. It provides _flexibility_ in the API so that some drivers
>can be really smart, if they want to. You haven't liked the consequence
>of that though: controller drivers being able to choose how they
>represent and manage I/O queues, rather than doing that in your "core".
>
>
>
Not that I agreed to your vision, for the reasons listed above. ;-)

Vitaly

2005-12-02 06:06:54

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

Greg KH wrote:

>>- The character device interface was reworked
>>
>>
>
>reworked how?
>
>
It was originally designed for 2.6.10 and now it's 2.6-git-synchronized.

>
>
>>- it's more adapted for use in real-time environments
>>
>>
>
>I still do not see why you are stating this. Why do you say this?
>
>
Due to possible priority inversion problems in David's core.

>I think you should work with David more...
>
>
P'haps you're right. I suggest re-enumerating all the differences
between the cores and working them out.
However, if David's not going to accept any facts or speculations that
contradict his being sure his core is the best a man could ever do,
we're screwed. :(

Vitaly

2005-12-02 18:50:44

by Mark Underwood

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh


--- Vitaly Wool <[email protected]> wrote:

> Greg KH wrote:
>
> >>- The character device interface was reworked
> >>
> >>
> >
> >reworked how?
> >
> >
> It was originally designed for 2.6.10 and now it's 2.6-git-synchronized.
>
> >
> >
> >>- it's more adapted for use in real-time environments
> >>
> >>
> >
> >I still do not see why you are stating this. Why do you say this?
> >
> >
> Due to possible priority inversion problems in David's core.

Which you still haven't proven, in fact you now seem to be changing your mind and saying that
there might be a problem if an adapter driver was implemented badly although I still don't see how
this could happen (the priority inversion I mean not the badly implemented driver ;).

>
> >I think you should work with David more...
> >
> >
> P'haps you're right. I suggest re-enumerating all the differences
> between the cores and working them out.
> However, if David's not going to accept any facts or speculations that
> contradict his being sure his core is the best a man could ever do,
> we're screwed. :(

When I worked with David he was very helpful. He pointed out my misconceptions and accepted my
comments that is core was missing functionality which he then added.

In fact at one time there were 3 SPI subsystems being prosposed, David's, Vitaly's and mine. David
and I work together to take the best from both which is now the solution he is proposing.

Mark

>
> 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-12-02 21:38:32

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Fri, Dec 02, 2005 at 09:06:50AM +0300, Vitaly Wool wrote:
> Greg KH wrote:
> >>- it's more adapted for use in real-time environments
> >>
> >I still do not see why you are stating this. Why do you say this?
> >
> Due to possible priority inversion problems in David's core.

I am still not convinced of your statements here, sorry. Specifics
please.

thanks,

greg k-h

2005-12-02 22:07:24

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] MTD/SPI dataflash driver


> However, it would be nice if someone compared the performance of both
> variants.

> > ? ? * original code could be implemented in a single DMA chain on
> >? ? ? at least two systems I happen to have handy ... one IRQ.
> >
> >? ? * this version requires two separate chains, with an intervening
> >? ? ? task schedule. ?(More than twice the cost.)

They differ by at least the cost of an extra IRQ and task schedule, so
the performance delta would be load-dependant. In a real-time setup
where that task was given time slices at fixed intervals, that might
mean the two-request version takes twice as long. ;)



> Whoops. Lemme express my thoughts here.
> First, the DMA controller that can handle chained requests which are
> working with different devices (i.e. write then read does that - first
> mem2per, then per2mem) are *very* rare thing.

The way they usually work is allocate two DMA channels to the device,
one for read and one for write. The controllers just makes sure there's
one byte read for every one written, and the DMA channels just stream
along with them. (Possibly one channel talks to a bitbucket.)

AT91, OMAP, and PXA hardware all claim hardware support sufficient to
do that. (Errata aside.) That's not even rare hardware, much less
"very rare"; they're stocked at DigiKey, ready to throw into boards.


> > - Chipselect works differently in your code. You're giving one
> > driver control over all the devices sharing a controller, by
> > blocking requests going to other devices until your driver yields
> > the chipselect.
> >
> >
> The controllers we were working with are _not_ able to handle
> simultaneous requests to different devices on the same bus.

Of course not; SPI doesn't work that way. But my point was that your
changes show a controller driver that's clearly required to "lock" the
bus, through that chipselect, between requests ... which is not only
antithetical to the "requests are asynchronous" model that had earlier
been agreed on, but also creates problems of its own:

> > ? This seems like a bad idea even if you ignore
> > how it produces priority inversions in your framework. ?:)

So consider two different tasks accessing devices on the same SPI bus.

One is lower priority, currently waiting for an SPI request to complete.
A request that has your magic "leave me owning chipselect/bus after
this request flag" ... because the first thing it's going to do when
it returns is start another transfer. (And in the case of the MTD driver,
that transfer could already have been queued, removing the issue as
well as the need for that flag.)

Now the high priority task issues a request to the other device on
that same SPI bus. This means that *two* other tasks ought to be
temporarily operating with that higher priority: whatever task
you've allocated to manage the I/O queue on that bus (plus maybe
an IRQ task with RT_PREEMPT); and the task that's waiting for that
transfer to complete. Inversions ...

- Dave

2005-12-03 11:45:20

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

> > >>- it's more adapted for use in real-time environments
> > >>
> > >I still do not see why you are stating this. Why do you say this?
> > >
> > Due to possible priority inversion problems in David's core.
>
> I am still not convinced of your statements here, sorry. Specifics
> please.

So, again, David's write_then_read function which is the basic one for all the synchronous
transfers uses single static pointer to kmalloc'ed buffer and protects its usage by
semaphores. If there are multiple controllers onboard, this serialization is suboptimal
and may cause priority inversion if, for instance, two threads with different priorities
will use this function at the same time.

Vitaly

2005-12-03 11:49:42

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

Mark,

> > >I still do not see why you are stating this. Why do you say this?
> > >
> > >
> > Due to possible priority inversion problems in David's core.
>
> Which you still haven't proven, in fact you now seem to be changing your mind and saying
> that
> there might be a problem if an adapter driver was implemented badly although I still
> don't see how
> this could happen (the priority inversion I mean not the badly implemented driver ;).

Truly admiring your deep understanding of the real-time technology, I should remind you
that within the real-time conditions almost each event may happen and may not happen, for
instance, two calls from different context to the same funtion may happen at the same or
almost the same time, and may not happen that way. Therefore I used the word "possible".
Hope I clarified that a bit for you.

Please also see my previous emails for the explanation of how priority inversion can
happen. This is not gonna be a rare case, BTW.

Vitaly

2005-12-03 12:00:14

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] MTD/SPI dataflash driver

David,

> So consider two different tasks accessing devices on the same SPI bus.
>
> One is lower priority, currently waiting for an SPI request to complete.
> A request that has your magic "leave me owning chipselect/bus after
> this request flag" ... because the first thing it's going to do when
> it returns is start another transfer. (And in the case of the MTD driver,
> that transfer could already have been queued, removing the issue as
> well as the need for that flag.)
>
> Now the high priority task issues a request to the other device on
> that same SPI bus. This means that *two* other tasks ought to be
> temporarily operating with that higher priority: whatever task
> you've allocated to manage the I/O queue on that bus (plus maybe
> an IRQ task with RT_PREEMPT); and the task that's waiting for that
> transfer to complete. Inversions ...

Can you please tell me if that's not the same for your core? I'm afraid this problem is
common between the cores :(

2005-12-03 17:10:39

by Mark Underwood

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh


--- [email protected] wrote:

> Mark,
>
> > > >I still do not see why you are stating this. Why do you say this?
> > > >
> > > >
> > > Due to possible priority inversion problems in David's core.
> >
> > Which you still haven't proven, in fact you now seem to be changing your mind and saying
> > that
> > there might be a problem if an adapter driver was implemented badly although I still
> > don't see how
> > this could happen (the priority inversion I mean not the badly implemented driver ;).
>
> Truly admiring your deep understanding of the real-time technology, I should remind you
> that within the real-time conditions almost each event may happen and may not happen, for
> instance, two calls from different context to the same funtion may happen at the same or
> almost the same time, and may not happen that way. Therefore I used the word "possible".
> Hope I clarified that a bit for you.
>
> Please also see my previous emails for the explanation of how priority inversion can
> happen. This is not gonna be a rare case, BTW.

Vitaly,

First, please can you not change the CC list in the midle of a thread.

Second, I studied real-time OS's at university and even started to write my own RTOS so I do know
the basic's of real-time technology. My problem wasn't understanding what you meant, just which
part of the code you where referring to :(.

OK, looking through the code after a cup of coffe I can see the problem you are pointing out,
thank you :), for some reason I thought that that code was protected by a spin_lock :/.

How to fix this?

David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure? This
helper routine could then use this thus solving the one buffer to many callers problem (well
moving into the adapter driver, but as that serialise's transfers anyway I think this would remove
the priority inversion problem, Vitaly?)

The other solution is to do a kmalloc for each caller (would could try to be smart and only do
that if the buffer is being used).

Let me know, if noone else is interested in fixing this then I'll do it and send a patch.

Mark

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




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

2005-12-03 19:20:23

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

Mark Underwood wrote:

>--- [email protected] wrote:
>
>
>
>>Mark,
>>
>>
>>
>>>>>I still do not see why you are stating this. Why do you say this?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>Due to possible priority inversion problems in David's core.
>>>>
>>>>
>>>Which you still haven't proven, in fact you now seem to be changing your mind and saying
>>>that
>>>there might be a problem if an adapter driver was implemented badly although I still
>>>don't see how
>>>this could happen (the priority inversion I mean not the badly implemented driver ;).
>>>
>>>
>>Truly admiring your deep understanding of the real-time technology, I should remind you
>>that within the real-time conditions almost each event may happen and may not happen, for
>>instance, two calls from different context to the same funtion may happen at the same or
>>almost the same time, and may not happen that way. Therefore I used the word "possible".
>>Hope I clarified that a bit for you.
>>
>>Please also see my previous emails for the explanation of how priority inversion can
>>happen. This is not gonna be a rare case, BTW.
>>
>>
>
>Vitaly,
>
>First, please can you not change the CC list in the midle of a thread.
>
Yeah, sorry for that. You see, I was emailing not from my computer.

>
>OK, looking through the code after a cup of coffe I can see the problem you are pointing out,
>thank you :), for some reason I thought that that code was protected by a spin_lock :/.
>
>How to fix this?
>
>David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure? This
>helper routine could then use this thus solving the one buffer to many callers problem (well
>moving into the adapter driver, but as that serialise's transfers anyway I think this would remove
>the priority inversion problem, Vitaly?)
>
>The other solution is to do a kmalloc for each caller (would could try to be smart and only do
>that if the buffer is being used).
>
>
And each one of the techniques suggested will make David's core closer
to ours :)

Vitaly

2005-12-03 23:50:05

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Saturday 03 December 2005 9:10 am, Mark Underwood wrote:
>
> David, how would you feel about adding a NOT_DMAABLE flag in the spi_message structure?

Not good; it'd mean that every controller driver would have to support
both PIO and DMA modes. This minor issue (despite the noise!) isn't
worth making such an intrusive demand on all drivers.


> The other solution is to do a kmalloc for each caller (would could try to be smart and only do
> that if the buffer is being used).

That's far preferable. You could just submit the patch against rc3-mm1
and that'll do the job.

- Dave

2005-12-05 18:00:56

by Vitaly Wool

[permalink] [raw]
Subject: [PATCH 2.6-git] SPI core refresh

Greetings,

This is an updated version of SPI framework developed by Dmitry Pervushin and Vitaly Wool.

The main changes are:

- simplified memory allocation framework in case of non-DMAable buffers
- Greg's review matched
- thread-based handling of the async messages is now an option
- spi_msg is now an abstract structure for SPI core users
- spi_msg can now be chained
- documentation updated

Again, some advantages of our core compared to David's I'd like to mention

- it can be compiled as a module (as opposed to basic David's core w/o Mark Underwood's patch)
- it is less priority inversion-exposed
- it can gain more performance with multiple controllers
- it's more adapted for use in real-time environments

Well, what else?
1. Now thw footprint of the pure (i. e. w/o device interface and thread-based handling) .text section is _less_ than 2k for ARM.
2. We still think that thread-based async messages processing will be the most commonly used option so we didn't remove it from core but made it a compication option whether to include it or not. In any case it can be overridden by a specific bus driver, so even when compiled in, thread-based handling won't necessarily _start_ the threads, so the overhead is minimal even in that case.
3. We still don't feel comportable with complicated structure of SPI message in David's core being exposed to all over the world. On the other hand, chaining SPI messages can really be helpful, so we added this option to our core (yeeeep, convergence is gong on :)) but
4. We made struct spi_msg not being exposed to anywhere except the core itself. Thus we're free to change its implementation in future if necessary.
5. Some changes weren't tested thoroughly yet, so you may encounter problems. Feel free to provide feedback and patches :)

Documentation/spi.txt | 115 +++++++++
arch/arm/Kconfig | 2
drivers/Kconfig | 2
drivers/Makefile | 1
drivers/spi/Kconfig | 39 +++
drivers/spi/Makefile | 16 +
drivers/spi/spi-core.c | 551 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/spi/spi-core.h | 41 +++
drivers/spi/spi-dev.c | 191 ++++++++++++++++
drivers/spi/spi-thread.c | 192 ++++++++++++++++
drivers/spi/spi-thread.h | 33 ++
include/linux/spi.h | 169 ++++++++++++++
12 files changed, 1352 insertions(+)

diff -uNr linux-2.6.orig/arch/arm/Kconfig linux-2.6/arch/arm/Kconfig
--- linux-2.6.orig/arch/arm/Kconfig 2005-11-30 19:38:45.000000000 +0300
+++ linux-2.6/arch/arm/Kconfig 2005-12-05 18:38:05.000000000 +0300
@@ -748,6 +748,8 @@

source "drivers/mmc/Kconfig"

+source "drivers/spi/Kconfig"
+
endmenu

source "fs/Kconfig"
diff -uNr linux-2.6.orig/Documentation/spi.txt linux-2.6/Documentation/spi.txt
--- linux-2.6.orig/Documentation/spi.txt 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/Documentation/spi.txt 2005-12-05 18:38:05.000000000 +0300
@@ -0,0 +1,115 @@
+Documentation/spi.txt
+========================================================
+Table of contents
+1. Introduction -- what is SPI ?
+2. Purposes of this code
+3. SPI devices stack
+3.1 SPI outline
+3.2 How the SPI devices gets discovered and probed ?
+3.3 DMA and SPI messages
+4. SPI functions and structures reference
+5. How to contact authors
+========================================================
+
+1. What is SPI ?
+----------------
+SPI stands for "Serial Peripheral Interface", a full-duplex synchronous
+serial interface for connecting low-/medium-bandwidth external devices
+using four wires. SPI devices communicate using a master/slave relation-
+ship over two data lines and two control lines:
+- Master Out Slave In (MOSI): supplies the output data from the master
+ to the inputs of the slaves;
+- Master In Slave Out (MISO): supplies the output data from a slave to
+ the input of the master. It is important to note that there can be no
+ more than one slave that is transmitting data during any particular
+ transfer;
+- Serial Clock (SCLK): a control line driven by the master, regulating
+ the flow of data bits;
+- Slave Select (SS): a control line that allows slaves to be turned on
+ and off with hardware control.
+More information is also available at http://en.wikipedia.org/wiki/Serial_Peripheral_Interface
+
+2. Purposes of this code
+------------------------
+The supplied patch is starting point for implementing drivers for various
+SPI busses as well as devices connected to these busses. Currently, the
+SPI core supports only for MASTER mode for system running Linux.
+
+3. SPI devices stack
+--------------------
+
+3.1 The SPI outline
+
+The SPI infrastructure deals with several levels of abstraction. They are
+"SPI bus", "SPI bus driver", "SPI slave device" and "SPI device driver". The
+"SPI bus" is hardware device, which usually called "SPI adapter", and has
+"SPI slave devices" connected. From the Linux' point of view, the "SPI bus" is
+structure of type platform_device, and "SPI slave device" is structure of type
+spi_device. The "SPI bus driver" is the driver which controls the whole
+SPI bus (and, particularly, creates and destroys "SPI slave devices" on the bus),
+and "SPI device driver" is driver that controls the only device on the SPI
+bus, controlled by "SPI bus driver". "SPI device driver" can indirectly
+call "SPI bus driver" to send/receive messages using API provided by SPI
+core, and provide its own interface to the kernel and/or userland.
+So, the device stack looks as follows:
+
+ +--------------+ +---------+
+ | some_bus | | spi_bus |
+ +--------------+ +---------+
+ |..| |
+ |..|--------+ +---------------+
+ +------------+| is parent to | SPI devices |
+ | SPI busses |+-------------> | |
+ +------------+ +---------------+
+ | |
+ +----------------+ +----------------------+
+ | SPI bus driver | | SPI device driver |
+ +----------------+ +----------------------+
+
+3.2 How do the SPI devices get discovered and probed ?
+
+In general, the SPI bus driver cannot effectively discover devices
+on its bus. Fortunately, the devices on SPI bus usually implemented
+onboard, so you need to create array of structures spi_device_desc and
+pass this array to function spi_bus_populate, like this:
+ struct spi_device_desc spi_slaves[] = {
+ [0] = {
+ .name = "device1",
+ .param = device1_params,
+ },
+ [1] = {
+ .name = "device2",
+ .param = NULL,
+ }
+ [2] = {
+ NULL, NULL
+ };
+ err = spi_bus_populate( the_spi_bus, spi_slaves, callback );
+
+3.3. DMA and SPI messages
+-------------------------
+
+The core provides additional robustness when the buffer suppiled is not
+DMA-safe. If it is, the core will allocate DMA-safe buffer and copy user-
+supplied buffer to it (before operation in WRITE case, and after in READ case).
+This two situations are distinguished by specific flag SPI_M_DMAUNSAFE.
+Bus driver should use spimsg_get_buffer and spimsg_put_buffer to access buffer.
+The buffer received from spimsg_get_buffer will be always DMA-safe and suitable for
+DMA mapping.
+
+4. SPI functions are structures reference
+-----------------------------------------
+Please refer to kerneldocs for the information. To create it, use command
+ $ scripts/kernel-doc -html drivers/spi/* > spi.html
+
+5. How to contact authors
+-------------------------
+Do you have any comments ? Enhancements ? Device driver ? Feel free
+to contact me:
+ [email protected]
+ [email protected]
+Visit our project page:
+ http://spi-devel.sourceforge.net
+Subscribe to mailing list:
+ [email protected]
+
diff -uNr linux-2.6.orig/drivers/Kconfig linux-2.6/drivers/Kconfig
--- linux-2.6.orig/drivers/Kconfig 2005-11-30 19:38:45.000000000 +0300
+++ linux-2.6/drivers/Kconfig 2005-12-05 18:38:05.000000000 +0300
@@ -44,6 +44,8 @@

source "drivers/i2c/Kconfig"

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

source "drivers/hwmon/Kconfig"
diff -uNr linux-2.6.orig/drivers/Makefile linux-2.6/drivers/Makefile
--- linux-2.6.orig/drivers/Makefile 2005-11-30 19:38:45.000000000 +0300
+++ linux-2.6/drivers/Makefile 2005-12-05 18:38:05.000000000 +0300
@@ -69,3 +69,4 @@
obj-y += firmware/
obj-$(CONFIG_CRYPTO) += crypto/
obj-$(CONFIG_SUPERH) += sh/
+obj-$(CONFIG_SPI) += spi/
diff -uNr linux-2.6.orig/drivers/spi/Kconfig linux-2.6/drivers/spi/Kconfig
--- linux-2.6.orig/drivers/spi/Kconfig 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/Kconfig 2005-12-05 18:42:38.000000000 +0300
@@ -0,0 +1,39 @@
+#
+# SPI device configuration
+#
+menu "SPI support"
+
+config SPI
+ tristate "SPI (Serial Peripheral Interface) bus support"
+ default false
+ help
+ Say Y if you need to enable SPI support on your kernel.
+ Say M if you want to create the spi-core loadable module.
+
+config SPI_THREAD
+ bool "Threaded handling of SPI asynchronous messages"
+ default true
+ help
+ Say Y here to compile thread-based asynchronous message
+ handling for SPI. This will be a default SPI async message
+ handling method, which can be overridden by bus driver.
+ If unsure, say Y.
+
+config SPI_DEBUG
+ bool "SPI debug output"
+ depends on SPI
+ default false
+ help
+ Say Y there if you'd like to see debug output from SPI drivers
+ If unsure, say N
+
+config SPI_CHARDEV
+ default Y
+ bool "SPI device interface"
+ depends on SPI
+ help
+ Say Y here to use /dev/spiNN device files. They make it possible to have user-space
+ programs use the SPI bus.
+
+endmenu
+
diff -uNr linux-2.6.orig/drivers/spi/Makefile linux-2.6/drivers/spi/Makefile
--- linux-2.6.orig/drivers/spi/Makefile 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/Makefile 2005-12-05 18:38:05.000000000 +0300
@@ -0,0 +1,16 @@
+#
+# Makefile for the kernel spi bus driver.
+#
+
+spi-y += spi-core.o
+spi-$(CONFIG_SPI_CHARDEV) += spi-dev.o
+spi-$(CONFIG_SPI_THREAD) += spi-thread.o
+# bus drivers
+# ...functional drivers
+# ...and the common spi-dev driver
+obj-$(CONFIG_SPI) += spi.o
+
+ifeq ($(CONFIG_SPI_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
+
diff -uNr linux-2.6.orig/drivers/spi/spi-core.c linux-2.6/drivers/spi/spi-core.c
--- linux-2.6.orig/drivers/spi/spi-core.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/spi-core.c 2005-12-05 18:38:05.000000000 +0300
@@ -0,0 +1,551 @@
+/*
+ * drivers/spi/spi-core.c
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/config.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/proc_fs.h>
+#include <linux/kmod.h>
+#include <linux/init.h>
+#include <linux/wait.h>
+#include <linux/kthread.h>
+#include <linux/spi.h>
+#include <asm/atomic.h>
+
+#include "spi-thread.h"
+#include "spi-core.h"
+
+static int spi_device_del(struct device *dev, void *data);
+
+void spimsg_set_clock (struct spi_msg* message, u32 clock)
+{
+ message->clock = clock;
+}
+
+u32 spimsg_get_clock (struct spi_msg* message)
+{
+ return message->clock;
+}
+
+u32 spimsg_get_flags (struct spi_msg* message)
+{
+ return message->flags;
+}
+
+u32 spimsg_get_buffer (struct spi_msg *message, void **buffer)
+{
+ if (!buffer)
+ return 0;
+ *buffer = message->buf_ptr;
+
+ if (message->flags & SPI_M_DMAUNSAFE) {
+
+ *buffer = kmalloc (message->len+sizeof(void*), GFP_DMA);
+ if (!*buffer)
+ return 0;
+ *(void**)((u8*)*buffer + message->len) = message->buf_ptr;
+ if (message->flags & SPI_M_WR)
+ memcpy( *buffer, message->buf_ptr, message->len );
+ }
+ return message->len;
+}
+
+void spimsg_put_buffer (struct spi_msg *message, void *buffer)
+{
+ if (message->flags & SPI_M_DMAUNSAFE) {
+ if (message->flags & SPI_M_RD)
+ memcpy (message->buf_ptr, buffer, message->len);
+ kfree(buffer);
+ }
+}
+
+/**
+ * spimsg_alloc - allocate the spi message
+ *
+ * @device: target device
+ * @flags: SPI message flags
+ * @buf: user-supplied buffer
+ * @len: buffer's length
+ * @status: user-supplied callback function
+**/
+struct spi_msg *spimsg_alloc(struct spi_device *device,
+ struct spi_msg *link,
+ u32 flags,
+ void *buf,
+ unsigned short len,
+ void (*status) (struct spi_msg *,
+ int code))
+{
+ struct spi_msg *msg;
+
+ if ((flags & (SPI_M_RD|SPI_M_WR)) == (SPI_M_RD|SPI_M_WR))
+ return NULL;
+ msg = kzalloc( sizeof (struct spi_msg), GFP_KERNEL);
+ if (!msg)
+ return NULL;
+ msg->len = len;
+ msg->status = status;
+ msg->device = device;
+ msg->flags = flags;
+ INIT_LIST_HEAD(&msg->link);
+
+ msg->buf_ptr = buf;
+
+ if (link)
+ link->next = msg;
+
+ return msg;
+}
+
+/**
+ * spimsg_free - free the message allocated by spimsg_alloc
+ *
+ * @msg: message to free
+ **/
+void spimsg_free(struct spi_msg *msg)
+{
+ kfree(msg);
+}
+
+
+
+/**
+ * spi_bus_match_name - verify that driver matches device on spi bus
+ * @dev: device that hasn't yet being assigned to any driver
+ * @drv: driver for spi device
+ * Description: match the device to driver.Drivers and devices on SPI bus
+ * are matched by name, just like the platform devices
+ */
+static int spi_bus_match_name(struct device *dev, struct device_driver *drv)
+{
+ return !strcmp(TO_SPI_DEV(dev)->name, drv->name);
+}
+
+/**
+ * spi_bus_suspend - suspend all devices on the spi bus
+ *
+ * @dev: spi device to be suspended
+ * @message: PM message
+ *
+ * This function set device on SPI bus to suspended state, just like platform_bus does
+ */
+static int spi_bus_suspend(struct device * dev, pm_message_t message)
+{
+ int ret = 0;
+
+ if (dev && dev->driver && TO_SPI_DRIVER(dev->driver)->suspend ) {
+ ret = TO_SPI_DRIVER(dev->driver)->suspend( TO_SPI_DEV(dev), message);
+ if (ret == 0 )
+ dev->power.power_state = message;
+ }
+ return ret;
+}
+
+/**
+ * spi_bus_resume - resume functioning of all devices on spi bus
+ *
+ * @dev: device to resume
+ *
+ * This function resumes device on SPI bus, just like platform_bus does
+**/
+static int spi_bus_resume(struct device * dev)
+{
+ int ret = 0;
+
+ if (dev && dev->driver && TO_SPI_DRIVER(dev->driver)->suspend ) {
+ ret = TO_SPI_DRIVER(dev->driver)->resume(TO_SPI_DEV(dev));
+ if (ret == 0)
+ dev->power.power_state = PMSG_ON;
+ }
+ return ret;
+}
+
+struct bus_type spi_bus = {
+ .name = "spi",
+ .match = spi_bus_match_name,
+ .suspend = spi_bus_suspend,
+ .resume = spi_bus_resume,
+};
+
+/**
+ * spi_bus_driver_init - init internal bus driver structures
+ *
+ * @bus: registered spi_bus_driver structure
+ * @dev: device that represents spi controller
+ *
+ * Once registered by spi_bus_register, the bus driver needs initialization, that
+ * includes starting thread, initializing internal structures.. The best place where
+ * the spi_bus_driver_init is in the `probe' function, when we already sure that passed
+ * device object is SPI master controller
+**/
+int spi_bus_driver_init(struct spi_bus_driver *bus, struct device *dev)
+{
+ struct spi_bus_data *pd =
+ kmalloc(sizeof(struct spi_bus_data), GFP_KERNEL);
+ int err = 0;
+
+ if (!pd) {
+ err = -ENOMEM;
+ goto init_failed;
+ }
+
+ pd->bus = bus;
+ init_MUTEX(&pd->lock);
+ INIT_LIST_HEAD(&pd->msgs);
+ pd->id = dev->bus_id;
+
+ if (!bus->start_async && !bus->stop_async) {
+ bus->start_async = spi_start_async;
+ bus->stop_async = spi_stop_async;
+ }
+
+ dev->platform_data = pd;
+
+ pd->async_data = bus->start_async ? bus->start_async(dev) : NULL;
+
+ return 0;
+
+init_failed:
+ return err;
+}
+
+/**
+ * __spi_bus_free -- unregister all children of the spi bus
+ *
+ * @dev: the spi bus `device' object
+ * @context: not used, will be NULL
+ *
+ * This is internal function that is called when unregistering bus driver. Responsibility
+ * of this function is freeing the resources that were requested by spi_bus_driver_init
+ **/
+static int __spi_bus_free(struct device *dev, void *context)
+{
+ struct spi_bus_data *pd = dev->platform_data;
+ struct spi_bus_driver *bus= TO_SPI_BUS_DRIVER(dev->driver);
+
+ if (bus->stop_async)
+ bus->stop_async(dev, pd->async_data);
+
+ dev_dbg(dev, "unregistering children\n");
+ device_for_each_child(dev, NULL, spi_device_del);
+ return 0;
+}
+
+/**
+ * spi_bus_driver_unregister - unregister SPI bus controller from the system
+ *
+ * @bus_driver: driver registered by call to spi_bus_driver_register
+ *
+ * unregisters the SPI bus from the system. Before unregistering, it deletes
+ * each SPI device on the bus using call to __spi_device_free
+**/
+void spi_bus_driver_unregister(struct spi_bus_driver *bus_driver)
+{
+ if (bus_driver) {
+ driver_for_each_device(&bus_driver->driver, NULL, NULL, __spi_bus_free);
+ driver_unregister(&bus_driver->driver);
+ }
+}
+
+/**
+ * spi_device_release - release the spi device structure
+ *
+ * @dev: spi_device to be released
+ *
+ * Pointer to this function will be put to dev->release place
+ * This fus called as a part of device removing
+**/
+void spi_device_release(struct device *dev)
+{
+ struct spi_device* sdev = TO_SPI_DEV(dev);
+
+ kfree(sdev);
+}
+
+/**
+ * spi_device_add - add the new (discovered) SPI device to the bus. Mostly used by bus drivers
+ *
+ * @parent: the bus device object
+ * @name: name of device (non-null!)
+ * @bus_data: bus data to be assigned to device
+ *
+ * SPI devices usually cannot be discovered by SPI bus driver, so it needs to take the configuration
+ * somewhere from hardcoded structures, and prepare bus_data for its devices
+**/
+struct spi_device* spi_device_add(struct device *parent, char *name, void *bus_data)
+{
+ struct spi_device* dev;
+ static int minor = 0;
+
+ if (!name)
+ goto dev_add_out;
+
+ dev = kmalloc(sizeof(struct spi_device), GFP_KERNEL);
+ if(!dev)
+ goto dev_add_out;
+
+ memset(&dev->dev, 0, sizeof(dev->dev));
+ dev->dev.parent = parent;
+ dev->dev.bus = &spi_bus;
+ strncpy(dev->name, name, sizeof(dev->name));
+ strncpy(dev->dev.bus_id, name, sizeof(dev->dev.bus_id));
+ dev->dev.release = spi_device_release;
+ dev->dev.platform_data = bus_data;
+
+ if (device_register(&dev->dev)<0) {
+ dev_dbg(parent, "device '%s' cannot be added\n", name);
+ goto dev_add_out_2;
+ }
+ dev->cdev = spi_class_device_create(minor, &dev->dev);
+ dev->minor = minor++;
+ return dev;
+
+dev_add_out_2:
+ kfree(dev);
+dev_add_out:
+ return NULL;
+}
+
+static int spi_device_del(struct device *dev, void *data)
+{
+ struct spi_device *spidev = TO_SPI_DEV(dev);
+ if (spidev->cdev) {
+ spi_class_device_destroy(spidev->cdev);
+ spidev->cdev = NULL;
+ }
+ device_unregister(&spidev->dev);
+ return 0;
+}
+/**
+ * __spi_transfer_callback - callback to process synchronous messages
+ *
+ * @msg: message that is about to complete
+ * @code: message status
+ *
+ * callback for synchronously processed message. If spi_transfer determines
+ * that there is no callback provided neither by msg->status nor callback
+ * parameter, the __spi_transfer_callback will be used, and spi_transfer
+ * does not return until transfer is finished
+ *
+**/
+static void __spi_transfer_callback(struct spi_msg *msg, int code)
+{
+ if (code & (SPIMSG_OK | SPIMSG_FAILED))
+ complete((struct completion *)msg->context);
+}
+
+/*
+ * spi_transfer - transfer the message either in sync or async way
+ *
+ * @msg: message to process
+ * @callback: user-supplied callback
+ *
+ * If both msg->status and callback are set, the error code of -EINVAL
+ * will be returned
+ */
+int spi_transfer(struct spi_msg *msg, void (*callback) (struct spi_msg *, int))
+{
+ struct completion msg_done;
+ int err = -EINVAL;
+ struct device *bus = msg->device->dev.parent;
+
+ if (TO_SPI_BUS_DRIVER(bus->driver)->queue)
+ {
+ if (callback && !msg->status) {
+ msg->status = callback;
+ callback = NULL;
+ }
+
+ if (!callback) {
+ if (!msg->status) {
+ init_completion(&msg_done);
+ msg->context = &msg_done;
+ msg->status = __spi_transfer_callback;
+ err = TO_SPI_BUS_DRIVER(bus->driver)->queue(msg);
+ wait_for_completion(&msg_done);
+ err = 0;
+ } else {
+ err = TO_SPI_BUS_DRIVER(bus->driver)->queue(msg);
+ }
+ }
+ }
+
+ return err;
+}
+
+/**
+ * spi_write - send data to a device on an SPI bus
+ *
+ * @dev: the target device
+ * @flags: additional flags for message
+ * @buf: buffer to be sent
+ * @len: buffer's length
+ *
+ * Returns the number of bytes transferred, or negative error code.
+**/
+int spi_write(struct spi_device *dev, u32 flags, char *buf, size_t len)
+{
+ struct spi_msg *msg = spimsg_alloc(dev, NULL, SPI_M_WR | SPI_M_DMAUNSAFE | flags, buf, len, NULL);
+ int ret;
+
+ ret = spi_transfer(msg, NULL);
+ return ret == 1 ? len : ret;
+}
+
+/**
+ * spi_read - receive data from a device on an SPI bus
+ *
+ * @dev: the target device
+ * @flags: additional flags for message
+ * @buf: buffer to be sent
+ * @len: buffer's length
+ *
+ * Returns the number of bytes transferred, or negative error code.
+**/
+int spi_read(struct spi_device *dev, u32 flags, char *buf, size_t len)
+{
+ int ret;
+ struct spi_msg *msg = spimsg_alloc(dev, NULL, SPI_M_RD | SPI_M_DMAUNSAFE | flags, buf, len, NULL);
+
+ ret = spi_transfer(msg, NULL);
+ return ret == 1 ? len : ret;
+}
+
+/**
+ * spi_bus_populate - populate the bus
+ *
+ * @parent: the SPI bus device object
+ * @devices_s: array of structures that represents bus population
+ * @callback: optional pointer to function that called on each device's add
+ *
+ * This function is intended to populate the SPI bus corresponding to
+ * device passed as 1st parameter.
+ * If some device cannot be added because of spi_device_add fail, the function will
+ * not try to parse the rest of list
+ */
+int spi_bus_populate(struct device *parent,
+ struct spi_device_desc* devices_s,
+ void (*callback) (struct device* bus,
+ struct spi_device *new_dev,
+ void* params))
+{
+ struct spi_device *new_device;
+ int count = 0;
+
+ while (devices_s->name) {
+ dev_dbg(parent, " discovered new SPI device, name '%s'\n",
+ devices_s->name);
+ if ((new_device = spi_device_add(parent, devices_s->name, devices_s->params)) == NULL)
+ break;
+ if (callback)
+ callback(parent, new_device, devices_s->params);
+ devices_s++;
+ count++;
+ }
+ return count;
+}
+
+/**
+ * spi_bus_reset - reset the spi bus
+ *
+ * @bus: device object that represents the SPI bus
+ * @context: u32 value to be passed to reset method of bus
+ *
+ * This is simple wrapper for bus' `reset' method
+ *
+**/
+void spi_bus_reset (struct device *bus, u32 context)
+{
+ if (bus && bus->driver && TO_SPI_BUS_DRIVER(bus->driver)->reset)
+ TO_SPI_BUS_DRIVER(bus->driver)->reset(bus, context);
+}
+
+/*
+ * the functions below are wrappers for corresponding device_driver's methods
+ */
+static int spi_driver_probe (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return sdrv->probe ? sdrv->probe(sdev) : -EFAULT;
+}
+
+static int spi_driver_remove (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ return sdrv->remove ? sdrv->remove(sdev) : -EFAULT;
+}
+
+static void spi_driver_shutdown (struct device *dev)
+{
+ struct spi_driver *sdrv = TO_SPI_DRIVER(dev->driver);
+ struct spi_device *sdev = TO_SPI_DEV(dev);
+
+ if (dev->driver && sdrv->shutdown)
+ sdrv->shutdown(sdev);
+}
+
+static int __init spi_core_init(void)
+{
+ int ret = spidev_init();
+
+ if (ret == 0)
+ ret = bus_register(&spi_bus);
+
+ return ret;
+}
+
+int spi_driver_add(struct spi_driver *drv)
+{
+ drv->driver.bus = &spi_bus;
+ drv->driver.probe = spi_driver_probe;
+ drv->driver.remove = spi_driver_remove;
+ drv->driver.shutdown = spi_driver_shutdown;
+ return driver_register(&drv->driver);
+}
+
+static void __exit spi_core_exit(void)
+{
+ bus_unregister(&spi_bus);
+ spidev_cleanup();
+}
+
+subsys_initcall(spi_core_init);
+module_exit(spi_core_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("dmitry pervushin <[email protected]>");
+
+EXPORT_SYMBOL_GPL(spi_bus_reset);
+EXPORT_SYMBOL_GPL(spi_device_add);
+EXPORT_SYMBOL_GPL(spi_driver_add);
+EXPORT_SYMBOL_GPL(spi_bus_driver_unregister);
+EXPORT_SYMBOL_GPL(spi_bus_populate);
+EXPORT_SYMBOL_GPL(spi_transfer);
+EXPORT_SYMBOL_GPL(spi_write);
+EXPORT_SYMBOL_GPL(spi_read);
+EXPORT_SYMBOL_GPL(spi_bus);
+EXPORT_SYMBOL_GPL(spi_bus_driver_init);
+
+EXPORT_SYMBOL_GPL(spimsg_alloc);
+EXPORT_SYMBOL_GPL(spimsg_free);
+EXPORT_SYMBOL_GPL(spimsg_put_buffer);
+EXPORT_SYMBOL_GPL(spimsg_get_flags);
+EXPORT_SYMBOL_GPL(spimsg_get_buffer);
+EXPORT_SYMBOL_GPL(spimsg_get_clock);
+EXPORT_SYMBOL_GPL(spimsg_set_clock);
+
diff -uNr linux-2.6.orig/drivers/spi/spi-core.h linux-2.6/drivers/spi/spi-core.h
--- linux-2.6.orig/drivers/spi/spi-core.h 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/spi-core.h 2005-12-05 18:54:25.000000000 +0300
@@ -0,0 +1,41 @@
+/*
+ * linux/drivers/spi/spi-core.h
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ */
+#ifndef __SPI_CORE_H
+#define __SPI_CORE_H
+
+struct spi_msg {
+ u32 flags;
+#define SPI_M_RD 0x00000001
+#define SPI_M_WR 0x00000002 /**< Write mode flag */
+#define SPI_M_CSREL 0x00000004 /**< CS release level at end of the frame */
+#define SPI_M_CS 0x00000008 /**< CS active level at begining of frame */
+#define SPI_M_CPOL 0x00000010 /**< Clock polarity */
+#define SPI_M_CPHA 0x00000020 /**< Clock Phase */
+#define SPI_M_EXTBUF 0x80000000 /** externally allocated buffers */
+#define SPI_M_ASYNC_CB 0x40000000 /** use workqueue to deliver callbacks */
+#define SPI_M_DNA 0x20000000 /** do not allocate buffers */
+#define SPI_M_DMAUNSAFE 0x10000000 /** buffer is dma-unsafe */
+
+ u16 len; /* msg length */
+ u32 clock;
+ struct spi_device *device;
+ void *context;
+
+ struct spi_msg *next;
+
+ struct list_head link;
+
+ void (*status) (struct spi_msg * msg, int code);
+
+ void *buf_ptr;
+};
+
+#endif /* __SPI_CORE_H */
diff -uNr linux-2.6.orig/drivers/spi/spi-dev.c linux-2.6/drivers/spi/spi-dev.c
--- linux-2.6.orig/drivers/spi/spi-dev.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/spi-dev.c 2005-12-05 20:26:17.000000000 +0300
@@ -0,0 +1,191 @@
+/*
+ spi-dev.c - spi driver, char device interface
+
+ Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+
+ 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/init.h>
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/version.h>
+#include <linux/smp_lock.h>
+
+#include <linux/init.h>
+#include <asm/uaccess.h>
+#include <linux/spi.h>
+
+#define SPI_TRANSFER_MAX 65535L
+
+static struct class *spidev_class;
+
+static ssize_t spidev_read(struct file *file, char *buf, size_t count,
+ loff_t * offset);
+static ssize_t spidev_write(struct file *file, const char *buf, size_t count,
+ loff_t * offset);
+
+static int spidev_open(struct inode *inode, struct file *file);
+static int spidev_release(struct inode *inode, struct file *file);
+
+/**
+ * spi_class_device_create - wrapper for class_device_create to be used in spi core
+ *
+ * @minor: sequental minor number of SPI slave device
+ * @device: pointer to struct device embedded to spi_device
+ *
+**/
+struct class_device *spi_class_device_create(int minor, struct device *device)
+{
+ return class_device_create(spidev_class, NULL, MKDEV(SPI_MAJOR, minor), device, "spi%d", minor);
+}
+
+/**
+ * spi_class_device_destroy - wrapper for class_device_destroy to be used in spi core
+ *
+ * @cdev: class device created by spi_class_device_create
+ */
+void spi_class_device_destroy(struct class_device *cdev)
+{
+ class_device_destroy(spidev_class, cdev->devt);
+}
+
+static struct file_operations spidev_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .read = spidev_read,
+ .write = spidev_write,
+ .open = spidev_open,
+ .release = spidev_release,
+};
+
+static ssize_t spidev_read(struct file *file, char __user *buf, size_t count,
+ loff_t * offset)
+{
+ int rc = 0;
+ char *kbuf = kmalloc(count, GFP_DMA);
+ struct spi_device *dev = (struct spi_device *)file->private_data;
+
+ if (!kbuf)
+ rc = -ENOMEM;
+ else {
+ rc = spi_read(dev, SPI_M_DNA, kbuf, count);
+ if (rc < 0 || copy_to_user(buf, kbuf, count))
+ rc = -EFAULT;
+ kfree(kbuf);
+ }
+ return rc;
+}
+
+static ssize_t spidev_write(struct file *file, const char __user *buf, size_t count,
+ loff_t * offset)
+{
+ int rc = 0;
+ char *kbuf = kmalloc(count, GFP_DMA);
+ struct spi_device *dev = (struct spi_device *)file->private_data;
+
+ if (!kbuf)
+ rc = -ENOMEM;
+ else {
+ if (!copy_from_user(kbuf, buf, count))
+ rc = spi_write(dev, SPI_M_DNA, kbuf, count);
+ else
+ rc = -EFAULT;
+ kfree(kbuf);
+ }
+ return rc;
+}
+
+struct spidev_openclose {
+ unsigned int minor;
+ struct file *file;
+};
+
+static int spidev_do_open(struct device *the_dev, void *context)
+{
+ struct spidev_openclose *o = (struct spidev_openclose *)context;
+ struct spi_device *dev = TO_SPI_DEV(the_dev);
+
+ pr_debug("device->minor = %d vs %d\n", dev->minor, o->minor);
+ if (dev->minor == o->minor) {
+ get_device(&dev->dev);
+ o->file->private_data = dev;
+ return 1;
+ }
+
+ return 0;
+}
+
+int spidev_open(struct inode *inode, struct file *file)
+{
+ struct spidev_openclose o;
+ int status;
+
+ o.minor = iminor(inode);
+ o.file = file;
+ status = bus_for_each_dev(&spi_bus, NULL, &o, spidev_do_open);
+ if (status == 0) {
+ status = -ENODEV;
+ }
+ return status < 0 ? status : 0;
+}
+
+static int spidev_release(struct inode *inode, struct file *file)
+{
+ struct spi_device *dev = file->private_data;
+
+ if (dev)
+ put_device(&dev->dev);
+ file->private_data = NULL;
+
+ return 0;
+}
+
+int __init spidev_init(void)
+{
+ int res;
+
+ if ((res = register_chrdev(SPI_MAJOR, "spi", &spidev_fops)) != 0) {
+ goto out;
+ }
+
+ spidev_class = class_create(THIS_MODULE, "spi");
+ if (IS_ERR(spidev_class)) {
+ printk(KERN_ERR "%s: error creating class\n", __FUNCTION__);
+ res = -EINVAL;
+ goto out_unreg;
+ }
+
+ return 0;
+
+out_unreg:
+ unregister_chrdev(SPI_MAJOR, "spi");
+out:
+ return res;
+}
+
+void __exit spidev_cleanup(void)
+{
+ class_destroy(spidev_class);
+ unregister_chrdev(SPI_MAJOR, "spi");
+}
+
+MODULE_AUTHOR("dmitry pervushin <[email protected]>");
+MODULE_DESCRIPTION("SPI class device driver");
+MODULE_LICENSE("GPL");
diff -uNr linux-2.6.orig/drivers/spi/spi-thread.c linux-2.6/drivers/spi/spi-thread.c
--- linux-2.6.orig/drivers/spi/spi-thread.c 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/spi-thread.c 2005-12-05 20:07:56.000000000 +0300
@@ -0,0 +1,192 @@
+/*
+ * drivers/spi/spi-thread.c
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/config.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/proc_fs.h>
+#include <linux/kmod.h>
+#include <linux/init.h>
+#include <linux/wait.h>
+#include <linux/kthread.h>
+#include <linux/spi.h>
+#include <asm/atomic.h>
+#include "spi-core.h"
+
+static int spi_thread(void *context);
+
+struct threaded_async_data {
+ atomic_t exiting;
+ struct device *dev;
+ struct task_struct *thread;
+};
+
+void *__spi_start_async (struct device *dev)
+{
+ struct threaded_async_data *td = kmalloc (sizeof (struct threaded_async_data), GFP_KERNEL);
+
+ if (!td)
+ return NULL;
+
+ td->dev = dev;
+ atomic_set(&td->exiting, 0);
+ td->thread = kthread_run(spi_thread, td, "%s-work", dev->bus_id);
+ return NULL;
+}
+
+void __spi_stop_async (struct device *dev, void *ctx)
+{
+ struct threaded_async_data *td = ctx;
+
+ if (ctx) {
+ atomic_inc (&td->exiting);
+ kthread_stop(td->thread);
+ kfree(td);
+ }
+}
+
+/**
+ * spi_thread_awake - function that called to determine if thread needs to process any messages
+ *
+ * @td: pointer to struct threaded_async_data
+ *
+ * Thread wakes up if there is signal to exit (bd->exiting is set) or there are any messages
+ * in bus' queue.
+ */
+static int spi_thread_awake(struct threaded_async_data *td)
+{
+ int ret = -EINVAL;
+ struct spi_bus_data *bd = td->dev->platform_data;
+
+ if (atomic_read(&td->exiting)) {
+ return 1;
+ }
+
+ if (bd) {
+ down(&bd->lock);
+ ret = !list_empty(&bd->msgs);
+ up(&bd->lock);
+ }
+ return ret;
+}
+
+/**
+ * spi_bus_next_msg - the wrapper for retrieve method for bus driver
+ *
+ * @this: spi_bus_driver that needs to retrieve next message from queue
+ * @data: pointer to spi_bus_data structure associated with spi_bus_driver
+ *
+ * If bus driver provides the `retrieve' method, it is called to retrieve the next message
+ * from queue. Otherwise, the spi_bus_fifo_retrieve is called
+ *
+ **/
+static struct spi_msg *spi_bus_next_msg(struct spi_bus_driver *this, struct spi_bus_data *data)
+{
+ return list_entry(data->msgs.next, struct spi_msg, link);
+}
+
+/**
+ * spi_thread - the thread that calls bus functions to perform actual transfers
+ *
+ * @context: pointer to struct spi_bus_data with bus-specific data
+ *
+ * Description:
+ * This function is started as separate thread to perform actual
+ * transfers on SPI bus
+ **/
+static int spi_thread(void *context)
+{
+ struct threaded_async_data *td = context;
+ struct spi_bus_data *bd = td->dev->platform_data;
+ struct spi_msg *cmsg;
+ int xfer_status;
+
+ while (!kthread_should_stop()) {
+
+ wait_event_interruptible(bd->queue, spi_thread_awake(td));
+
+ if (atomic_read(&td->exiting))
+ goto thr_exit;
+
+ down(&bd->lock);
+ cmsg = NULL;
+ while (!list_empty(&bd->msgs) || cmsg) {
+ /*
+ * this part is locked by bus_data->lock,
+ * to protect spi_msg extraction
+ */
+ if (!cmsg)
+ cmsg = spi_bus_next_msg(bd->bus, bd);
+ else
+ cmsg = cmsg->next;
+
+ if (cmsg == NULL)
+ break;
+
+ list_del(&cmsg->link);
+ up(&bd->lock);
+
+ /*
+ * and this part is locked by device's lock;
+ * spi_queue will be able to queue new
+ * messages
+ *
+ * note that bd->selected_device is locked, not msg->device
+ * they are the same, but msg can be freed in msg->status function
+ */
+ spi_device_lock(bd->selected_device);
+ if (bd->bus->set_clock && cmsg->clock)
+ bd->bus->set_clock(cmsg->device->dev.parent,
+ cmsg->clock);
+ xfer_status = bd->bus->xfer(cmsg);
+ if (cmsg->status)
+ cmsg->status(cmsg, xfer_status);
+
+ spi_device_unlock(bd->selected_device);
+
+ /* lock the bus_data again... */
+ down(&bd->lock);
+ }
+ up(&bd->lock);
+ }
+
+thr_exit:
+ return 0;
+}
+
+/**
+ * spi_queue - queue the message to be processed asynchronously
+ *
+ * @msg: message to be sent
+ *
+ * This function queues the message to spi bus driver's queue. The bus driver
+ * retrieves the message from queue according to its own rules (see retrieve method)
+ * and sends the message to target device. If message has no callback method, originator
+ * of message would get no chance to know where the message is processed. The better
+ * solution is using spi_transfer function, which will return error code if no callback
+ * is provided, or transfer the message synchronously.
+**/
+int spi_queue(struct spi_msg *msg)
+{
+ struct device *dev = &msg->device->dev;
+ struct spi_bus_data *pd = dev->parent->platform_data;
+
+ down(&pd->lock);
+ list_add_tail(&msg->link, &pd->msgs);
+ dev_dbg(dev->parent, "message has been queued\n");
+ up(&pd->lock);
+ wake_up_interruptible(&pd->queue);
+ return 0;
+}
+
+
diff -uNr linux-2.6.orig/drivers/spi/spi-thread.h linux-2.6/drivers/spi/spi-thread.h
--- linux-2.6.orig/drivers/spi/spi-thread.h 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/drivers/spi/spi-thread.h 2005-12-05 19:33:39.000000000 +0300
@@ -0,0 +1,33 @@
+/*
+ * linux/drivers/spi/spi-thread.h
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ */
+#ifndef __SPI_THREAD_H
+#define __SPI_THREAD_H
+
+
+static inline void *spi_start_async (struct device *dev)
+{
+#ifdef CONFIG_SPI_THREAD
+extern void *__spi_start_async (struct device *dev);
+ return __spi_start_async(dev);
+#else
+ return 0;
+#endif
+}
+
+static inline void spi_stop_async (struct device *dev, void *t)
+{
+#ifdef CONFIG_SPI_THREAD
+extern void __spi_stop_async (struct device *dev, void *t);
+ __spi_stop_async (dev, t);
+#endif
+}
+
+#endif /* __SPI_THREAD_H */
diff -uNr linux-2.6.orig/include/linux/spi.h linux-2.6/include/linux/spi.h
--- linux-2.6.orig/include/linux/spi.h 1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6/include/linux/spi.h 2005-12-05 18:38:05.000000000 +0300
@@ -0,0 +1,169 @@
+/*
+ * linux/include/linux/spi/spi.h
+ *
+ * Copyright (C) 2005 MontaVista Software, Inc <[email protected]>
+ *
+ * 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.
+ *
+ * Derived from l3.h by Jamey Hicks
+ */
+#ifndef SPI_H
+#define SPI_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+
+struct spi_device;
+struct spi_driver;
+struct spi_msg;
+struct spi_bus_driver;
+
+extern struct bus_type spi_bus;
+
+struct spi_bus_data {
+ struct semaphore lock;
+ struct list_head msgs;
+ void *async_data;
+ wait_queue_head_t queue;
+ struct spi_device *selected_device;
+ struct spi_bus_driver *bus;
+ char *id;
+};
+
+#define TO_SPI_BUS_DRIVER(drv) container_of(drv, struct spi_bus_driver, driver)
+struct spi_bus_driver {
+
+ int (*xfer) (struct spi_msg * msg);
+ void (*set_clock) (struct device * bus_device, u32 clock_hz);
+ void (*reset) (struct device *bus_device, u32 context);
+
+ int (*queue) (struct spi_msg *msg);
+ void *(*start_async)( struct device *bus);
+ void (*stop_async)( struct device *bus, void *async);
+
+ struct device_driver driver;
+};
+
+#define TO_SPI_DEV(device) container_of(device, struct spi_device, dev)
+struct spi_device {
+ char name[BUS_ID_SIZE];
+ int minor;
+ struct class_device *cdev;
+ struct device dev;
+};
+
+#define TO_SPI_DRIVER(drv) container_of(drv, struct spi_driver, driver)
+struct spi_driver {
+
+ int (*probe) (struct spi_device * dev);
+ int (*remove) (struct spi_device * dev);
+ void (*shutdown) (struct spi_device * dev);
+ int (*suspend) (struct spi_device * dev, pm_message_t pm);
+ int (*resume) (struct spi_device * dev);
+
+ struct device_driver driver;
+};
+
+#define SPI_DEV_DRV(device) TO_SPI_BUS_DRIVER((device)->dev.parent->driver)
+
+#define spi_device_lock(spi_dev) down(&(spi_dev)->dev.sem)
+#define spi_device_unlock(spi_dev) up(&(spi_dev)->dev.sem)
+
+#define SPI_M_RD 0x00000001
+#define SPI_M_WR 0x00000002 /**< Write mode flag */
+#define SPI_M_CSREL 0x00000004 /**< CS release level at end of the frame */
+#define SPI_M_CS 0x00000008 /**< CS active level at begining of frame */
+#define SPI_M_CPOL 0x00000010 /**< Clock polarity */
+#define SPI_M_CPHA 0x00000020 /**< Clock Phase */
+#define SPI_M_EXTBUF 0x80000000 /** externally allocated buffers */
+#define SPI_M_ASYNC_CB 0x40000000 /** use workqueue to deliver callbacks */
+#define SPI_M_DNA 0x20000000 /** do not allocate buffers */
+#define SPI_M_DMAUNSAFE 0x10000000 /** buffer is dma-unsafe */
+
+void spimsg_set_clock (struct spi_msg* message, u32 clock);
+u32 spimsg_get_clock (struct spi_msg* message);
+u32 spimsg_get_flags (struct spi_msg* message);
+u32 spimsg_get_len (struct spi_msg *message);
+u32 spimsg_get_buffer (struct spi_msg *message, void **buffer);
+void spimsg_put_buffer (struct spi_msg *message, void *buffer);
+struct spi_msg *spimsg_alloc(struct spi_device *device,
+ struct spi_msg *link,
+ u32 flags,
+ void *buf,
+ unsigned short len,
+ void (*status) (struct spi_msg *,
+ int code));
+void spimsg_free (struct spi_msg *message);
+
+#if defined (CONFIG_SPI_CHARDEV)
+extern struct class_device *spi_class_device_create(int minor, struct device *device);
+extern void spi_class_device_destroy(struct class_device *cdev);
+#else
+static inline struct class_device *spi_class_device_create(int minor, struct device *device)
+{
+ return NULL;
+}
+static inline void spi_class_device_destroy(struct class_device *cdev)
+{
+}
+#endif
+
+enum {
+ SPIMSG_OK = 0,
+ SPIMSG_FAILED = -1,
+};
+
+#define SPI_MAJOR 153
+
+struct spi_driver;
+struct spi_device;
+
+#if defined (CONFIG_SPI_CHARDEV)
+extern int __init spidev_init(void);
+extern void __exit spidev_cleanup(void);
+#else
+static inline int spidev_init(void)
+{
+ return 0;
+}
+static inline void spidev_cleanup(void)
+{
+}
+#endif
+
+static inline int spi_bus_driver_register (struct spi_bus_driver *bus_driver)
+{
+ return driver_register (&bus_driver->driver);
+}
+
+void spi_bus_driver_unregister(struct spi_bus_driver *);
+int spi_bus_driver_init(struct spi_bus_driver *driver, struct device *dev);
+struct spi_device* spi_device_add(struct device *parent, char *name, void *private);
+
+extern int spi_driver_add(struct spi_driver *drv);
+
+static inline void spi_driver_del(struct spi_driver *drv)
+{
+ driver_unregister(&drv->driver);
+}
+
+extern void spi_bus_reset(struct device* bus, u32 context);
+extern int spi_write(struct spi_device *dev, u32 flags, char *buf, size_t len);
+extern int spi_read(struct spi_device *dev, u32 flags, char *buf, size_t len);
+
+extern int spi_queue(struct spi_msg *message);
+extern int spi_transfer(struct spi_msg *message,
+ void (*status) (struct spi_msg *, int));
+struct spi_device_desc {
+ char* name;
+ void* params;
+};
+extern int spi_bus_populate(struct device *parent,
+ struct spi_device_desc *devices,
+ void (*assign) (struct device *parent,
+ struct spi_device *,
+ void *));
+
+#endif /* SPI_H */

2005-12-08 02:00:01

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

On Monday 05 December 2005 10:01 am, Vitaly Wool wrote:
>
> Again, some advantages of our core compared to David's I'd like to mention
>
> - it can be compiled as a module (as opposed to basic David's core w/o Mark Underwood's patch)
> - it is less priority inversion-exposed

These are actually minor issues, with almost trivial fixes. (Pending.)

And I'd argue about the priority inversion thing ... the inversions in
your stuff come from the API, not the implementation. Which makes the
problems inherent, rather than fixable: "more" exposed, not "less".


> - it can gain more performance with multiple controllers

If this isn't a repeat of that priority-inversion case (fix available),
then I don't see what you're implying. I have a hard time seeing any
potential for an issue there, since the I/O request path just shortcuts
to the controller driver. It can't exactly get in the way!


> - it's more adapted for use in real-time environments

I think you still haven't explained what you mean by this, other than
maybe reminding that PREEMPT_RT interacts with some implementations.


> Well, what else?
> 1. Now thw footprint of the pure (i. e. w/o device interface and thread-based
> handling) .text section is _less_ than 2k for ARM.

Hey, I started from that size *including* device interface etc. If it's
exceeded now, it's because of the extra overhead from wrapping device_driver
with some spi_driver code. ;)


> 2. We still think that thread-based async messages processing will be the most
> commonly used option so we didn't remove it from core but made it a compication
> option whether to include it or not. In any case it can be overridden by a
> specific bus driver, so even when compiled in, thread-based handling won't
> necessarily _start_ the threads, so the overhead is minimal even in that case.

Whereas I've just said such threading policies don't belong in a "core" at
all. You may have noticed the bitbanging adapter I posted ... oddly, that
implementation allocates a thread. Hmm ...

That is: you're not talking about capabilities that aren't already in
the SPI patches already circulating in 2.6.15-rc5-mm1 (from Sunday).
They're just layered differently ... the core is _minimal_ and those
implementation policies can be chosen without adding to the core.
(Or impacting drivers that want different implementation policies.)


> 3. We still don't feel comportable with complicated structure of SPI message in
> David's core being exposed to all over the world. On the other hand, chaining
> SPI messages can really be helpful,

The point has been made that such chaining is actually "essential", since
device interaction protocols have constraints like "chipselect must be
asserted during all these transfers" or contrariwise "between these transfers,
chipselect must be dropped for N microseconds". If the async messages didn't
cover such linked message, then two activities could interfere with each other
quite badly ... they'd break hardware protocol requirements.


> so we added this option to our core (yeeeep,
> convergence is gong on :)) but

My preferred level of convergence would be changes to make your code look
more like mine, especially where you're providing a mechanism that's been
in mine all along ... hmm, like spi_driver is the same now. I made one
such change; maybe it's your turn now. ;)

- Dave

2005-12-08 06:34:13

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>On Monday 05 December 2005 10:01 am, Vitaly Wool wrote:
>
>
>>Again, some advantages of our core compared to David's I'd like to mention
>>
>>- it can be compiled as a module (as opposed to basic David's core w/o Mark Underwood's patch)
>>- it is less priority inversion-exposed
>>
>>
>
>These are actually minor issues, with almost trivial fixes. (Pending.)
>
>And I'd argue about the priority inversion thing ... the inversions in
>your stuff come from the API, not the implementation. Which makes the
>problems inherent, rather than fixable: "more" exposed, not "less".
>
>
I don't think it's true any more (with this new patch).

>>2. We still think that thread-based async messages processing will be the most
>>commonly used option so we didn't remove it from core but made it a compication
>>option whether to include it or not. In any case it can be overridden by a
>>specific bus driver, so even when compiled in, thread-based handling won't
>>necessarily _start_ the threads, so the overhead is minimal even in that case.
>>
>>
>
>Whereas I've just said such threading policies don't belong in a "core" at
>all. You may have noticed the bitbanging adapter I posted ... oddly, that
>implementation allocates a thread. Hmm ...
>
>
Please remember that using threads is just a default option which may be
even turned off at the kernel configuration stage.
Using threads for async transfers is reasonable _default_ assumption. It
can save time for controller driver dev'ers as well as reduce overall
kernel footprint (as opposed to different controller drivers each
implementing thread-based async messages handling).

>That is: you're not talking about capabilities that aren't already in
>the SPI patches already circulating in 2.6.15-rc5-mm1 (from Sunday).
>They're just layered differently ... the core is _minimal_ and those
>implementation policies can be chosen without adding to the core.
>(Or impacting drivers that want different implementation policies.)
>
>
Hmm, how are we impacting drivers that want different implementation
policies? Please look at the latest core :)

>
>
>
>>3. We still don't feel comportable with complicated structure of SPI message in
>>David's core being exposed to all over the world. On the other hand, chaining
>>SPI messages can really be helpful,
>>
>>
>
>The point has been made that such chaining is actually "essential", since
>device interaction protocols have constraints like "chipselect must be
>asserted during all these transfers" or contrariwise "between these transfers,
>chipselect must be dropped for N microseconds". If the async messages didn't
>cover such linked message, then two activities could interfere with each other
>quite badly ... they'd break hardware protocol requirements.
>
>
>
I don't argue about the need of chaining.
But don't you want to agree that things like this

+ struct spi_transfer x[1] = { { .tx_dma = 0, }, };
...more initialization follows, spread around the code...


are not well-readable and may shadow what's going on and even lead to
errors during support/extension of the functionality?
Exposing SPI message structure doesn't look good to me also because
you're making it a part of the interface (and thus unlikely to change).
We're hiding spi_msg internals what allows us to be more flexible in
implementation (for instance, implement our own allocation technique for
spi_msg (more lightweight as the size is always the same).

Yeah thus we don't have an ability to allocate SPI messages on stack as
you do, that's what votes for your approach. Yours is thus a bit faster,
though I suspect that this method is a possible *danger* for really
high-speed devices with data bursts on the SPI bus like WiFi adapters:
stack's gonna suffer from large amounts of data allocated.

>>so we added this option to our core (yeeeep,
>>convergence is gong on :)) but
>>
>>
>
>My preferred level of convergence would be changes to make your code look
>more like mine, especially where you're providing a mechanism that's been
>in mine all along ... hmm, like spi_driver is the same now. I made one
>such change; maybe it's your turn now. ;)
>
>
Okay :)

Vitaly

2005-12-09 22:55:13

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh


> >Whereas I've just said such threading policies don't belong in a "core" at
> >all. You may have noticed the bitbanging adapter I posted ... oddly, that
> >implementation allocates a thread. Hmm ...
> >
> >
> Please remember that using threads is just a default option which may be
> even turned off at the kernel configuration stage.

People keep saying "make it a library" in such cases, and that's the
kind of layering I've come to think is more appropriate. So that
bitbang code needs some reshaping. As a library, not driver or core.


> I don't argue about the need of chaining.
> But don't you want to agree that things like this
>
> + struct spi_transfer x[1] = { { .tx_dma = 0, }, };
> ...more initialization follows, spread around the code...

Not quite accurate. That initializes everything to zero, much like
memcpy. What happens later is just kicking in the relevant options.


> are not well-readable and may shadow what's going on and even lead to
> errors during support/extension of the functionality?

I've had my concerns, but that "zero init is safe" rule makes it easy
to add things in backwards-compatible ways. (I'll document it.) So
that code is equivalent to GCC calling

static inline void
spi_transfer_init(struct spi_transfer *t, unsigned count)
{ memset(t, 0, count * sizeof t); }

It might be useful having and using some SPI_TRANSFER_INITIALIZER for
cases like that one.


> Exposing SPI message structure doesn't look good to me also because
> you're making it a part of the interface (and thus unlikely to change).

The interface will be the interface, sure. Whatever it is.
And will accordingly be unlikely to change. We know that
from every other API in Linux and elsewhere; nothing new.

Was there some specific issue you forgot to raise here?

This one includes a chained/atomic message, which we agreed
are important. It's also simple to set up, IMO another
Good Thing. Dropping transfer sequencing, or making things
harder to set up, doesn't sound so good ...


> We're hiding spi_msg internals what allows us to be more flexible in
> implementation (for instance, implement our own allocation technique for
> spi_msg (more lightweight as the size is always the same).

What you're doing is requiring a standard dynamic allocation model for
your "spi_msg" objects ... one per transfer even, much heavier weight than
the "one per transfer group" model of current "spi_message". (And much
heavier than stack based allocation too.)

At which point krefcounting should surely kick in, and all kinds of stuff.
I'd rather not require such things, but there's no reason such a model
couldn't be a layer on top of what I've shown. Either a thin one (just
add krefs) or a fat one (which one expects would add real value).


> Yeah thus we don't have an ability to allocate SPI messages on stack as
> you do, that's what votes for your approach. Yours is thus a bit faster,
> though I suspect that this method is a possible *danger* for really
> high-speed devices with data bursts on the SPI bus like WiFi adapters:
> stack's gonna suffer from large amounts of data allocated.

No, you're still thinking about a purely synchronous programming model;
which we had agreed ages ago was not required.

Have a look at that ADS 7846 driver. It always submits batched requests,
which happen to be heap allocated (not stack allocated) since some of them
must be issued from hardware IRQ context. The technique generalizes easily.
And yes, kzalloc() is your friend.

- Dave

2005-12-10 11:16:01

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>Please remember that using threads is just a default option which may be
>>even turned off at the kernel configuration stage.
>>
>>
>
>People keep saying "make it a library" in such cases, and that's the
>kind of layering I've come to think is more appropriate. So that
>bitbang code needs some reshaping. As a library, not driver or core.
>
>
Well, effectively this is the way we do it now... Of course one can
establish drivers/spi/lib subdir... :)

>>Exposing SPI message structure doesn't look good to me also because
>>you're making it a part of the interface (and thus unlikely to change).
>>
>>
>
>The interface will be the interface, sure. Whatever it is.
>And will accordingly be unlikely to change. We know that
>from every other API in Linux and elsewhere; nothing new.
>
>Was there some specific issue you forgot to raise here?
>
>
Well, I've had an experience working with full duplex SPI controller.
I'm afraid that if you're gonna support it some time in the future,
you'll have to rework the message interface, won't you?

>>We're hiding spi_msg internals what allows us to be more flexible in
>>implementation (for instance, implement our own allocation technique for
>>spi_msg (more lightweight as the size is always the same).
>>
>>
>
>What you're doing is requiring a standard dynamic allocation model for
>your "spi_msg" objects ... one per transfer even, much heavier weight than
>the "one per transfer group" model of current "spi_message". (And much
>heavier than stack based allocation too.)
>
>
We can allocate a pool of messages at the very init stage and use our
own technique to grab the next one from the pool in spimsg_alloc.
So we're not requiring _standard_ dynamic allocation model.

Vitaly

2005-12-11 12:38:49

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>Yeah thus we don't have an ability to allocate SPI messages on stack as
>>you do, that's what votes for your approach. Yours is thus a bit faster,
>>though I suspect that this method is a possible *danger* for really
>>high-speed devices with data bursts on the SPI bus like WiFi adapters:
>>stack's gonna suffer from large amounts of data allocated.
>>
>>
>
>No, you're still thinking about a purely synchronous programming model;
>which we had agreed ages ago was not required.
>
>
Ah yes. But wait... I've got an important question here.
For instance, let's take your MTD driver. You're allocating a message
structure on stack and passing it then down to spi->master->transfer
function.
The benefit you're talking about is that you don't have to use
heavyweight memory allocation. But... the transfer is basically async so
spi->master->transfer will need to copy your message structure to its
own-allocated structure so some memory copying will occur as this might
be an async transfer (and therefore the stack-allocated message may be
freed at some point when it's yet used!)
So your model implies concealed double message allocation/copying,
doesn't it?
And if I'm wrong, can you please explain me this?

Vitaly

2005-12-11 17:05:34

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

Vitaly Wool wrote:

> David Brownell wrote:
>
>>> Yeah thus we don't have an ability to allocate SPI messages on stack
>>> as you do, that's what votes for your approach. Yours is thus a bit
>>> faster, though I suspect that this method is a possible *danger* for
>>> really high-speed devices with data bursts on the SPI bus like WiFi
>>> adapters: stack's gonna suffer from large amounts of data allocated.
>>>
>>
>>
>> No, you're still thinking about a purely synchronous programming model;
>> which we had agreed ages ago was not required.
>>
>>
> Ah yes. But wait... I've got an important question here.
> For instance, let's take your MTD driver. You're allocating a message
> structure on stack and passing it then down to spi->master->transfer
> function.
> The benefit you're talking about is that you don't have to use
> heavyweight memory allocation. But... the transfer is basically async
> so spi->master->transfer will need to copy your message structure to
> its own-allocated structure so some memory copying will occur as this
> might be an async transfer (and therefore the stack-allocated message
> may be freed at some point when it's yet used!)
> So your model implies concealed double message allocation/copying,
> doesn't it?
> And if I'm wrong, can you please explain me this?

Oh, now looks like I understood what is meant. If a function uses
stack-allocated messages, it should ensure that it will not exit until
the message is processed (shouldn't it be documented somewhere?). But
this solves the problem only partially since this technique fits only
the synchronous transfers.
Functions targeting async transfers will anyway have to kmalloc the
memory for message structure which makes your approach not really more
lightweight then ours. It's mainly async transfers that need high
throughput; and the drivers based on your core will have to kmalloc
memory for messages in that case.

Vitaly

2005-12-11 20:17:37

by David Brownell

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh


> > The benefit you're talking about is that you don't have to use
> > heavyweight memory allocation. But... the transfer is basically async
> > so spi->master->transfer will need to copy your message structure to
> > its own-allocated structure so some memory copying will occur as this

Incorrect, as you note below.


> > might be an async transfer (and therefore the stack-allocated message
> > may be freed at some point when it's yet used!)
> > So your model implies concealed double message allocation/copying,
> > doesn't it?
> > And if I'm wrong, can you please explain me this?
>
> Oh, now looks like I understood what is meant. If a function uses
> stack-allocated messages, it should ensure that it will not exit until
> the message is processed (shouldn't it be documented somewhere?).

It is documented, but I'll make sure it comes up in a few more of the
places this confusion might arise. It's a fairly basic rule for
C programming: don't use pointers after they become invalid by
means of freeing back to the heap, or invalidating a stack frame.


> But
> this solves the problem only partially since this technique fits only
> the synchronous transfers.

Synchronous transfers can easily use stack allocation for
the descriptions, yes.

Not that they need to ... the ads7846 driver allocates its
spi_message and spi_transfer objects on the heap both for
synchronous operations (temperature and voltage sensing) and
for asynch ones (touchscreen tracking from timer and irq).


> Functions targeting async transfers will anyway have to kmalloc the
> memory for message structure which makes your approach not really more
> lightweight then ours.

If you measure the number of error/fault cases when you ask how
lightweight an API is, it's clearly lighter weight to allow for
example one kzalloc -(with spi_message and its N spi_transfer
descriptors, plus possibly other driver state) rather than to
require many of them. Just one fault path to write -- and debug.

My usual rule of thumb is that 1/3 of code (by lines) must handle
fault cases. So APIs requiring more fault handling require more
driver code ... not lightweight! That was a help, when fitting
into a tight size budget. (As appropriate to what's more or less
a shift register API, needing to run quickly in uCLinux and such.)

Plus, letting the driver do the kzalloc means there's no new API.
No-new-API is another way to promote lighter weight systems. ;)


That said ... I know some people _do_ like krefcounted APIs that
do that kind of stuff. Strongly. Greg's been silent here other
than pointing out that your request alloc was too fat to inline.
Mine is trivially inlined, but not refcounted. Likely there's a
happy middle ground, maybe

mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
mesg = spi_message_get(mesg);
spi_message_put();

Or whatever. Just add a kref to spi_message, and patch against
the current mm set (to the core and both drivers, but not
necessarily the spi_bitbang stuff).

- Dave

2005-12-11 22:14:46

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>But
>>this solves the problem only partially since this technique fits only
>>the synchronous transfers.
>>
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to
allocate messages on stack but this approach has some limitations we've
just agrred upon.
On the other hand, our approach is flexible in means of message
allocation. I. e. a small memory allocation library can be implemented
transparently to device drivers that handles message allocation. It's
very easy (and lightweight :)) since the message structure is always of
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff. Strongly. Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted. Likely there's a
>happy middle ground, maybe
>
> mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
> mesg = spi_message_get(mesg);
> spi_message_put();
>
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_mjsg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post
the update tomorrow).
I would just like to say that defining such an API looks better thing to
me than dealing with SPI message structure explicitly.

Vitaly

2005-12-11 22:16:34

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>But
>>this solves the problem only partially since this technique fits only
>>the synchronous transfers.
>>
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to
allocate messages on stack but this approach has some limitations we've
just agrred upon.
On the other hand, our approach is flexible in means of message
allocation. I. e. a small memory allocation library can be implemented
transparently to device drivers that handles message allocation. It's
very easy (and lightweight :)) since the message structure is always of
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff. Strongly. Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted. Likely there's a
>happy middle ground, maybe
>
> mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
> mesg = spi_message_get(mesg);
> spi_message_put();
>
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_msg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post
the update tomorrow).
I would just like to say that defining such an API looks better thing to
me than dealing with SPI message structure explicitly.

Vitaly

2005-12-11 22:18:56

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>But
>>this solves the problem only partially since this technique fits only
>>the synchronous transfers.
>>
>>
>
>Synchronous transfers can easily use stack allocation for
>the descriptions, yes.
>
>Not that they need to ... the ads7846 driver allocates its
>spi_message and spi_transfer objects on the heap both for
>synchronous operations (temperature and voltage sensing) and
>for asynch ones (touchscreen tracking from timer and irq).
>
>
So are you talking about only kzalloc vs kmalloc?
I was trying to compare the approaches in a somehow deeper way.
That said, I meant that not exposing any structures you don't have to
expose was usually a right way to do things.
We don't expose SPI message and you do.
The only advantage of such exposure I could think of was possibilily to
allocate messages on stack but this approach has some limitations we've
just agrred upon.
On the other hand, our approach is flexible in means of message
allocation. I. e. a small memory allocation library can be implemented
transparently to device drivers that handles message allocation. It's
very easy (and lightweight :)) since the message structure is always of
a same length... Agree?

>That said ... I know some people _do_ like krefcounted APIs that
>do that kind of stuff. Strongly. Greg's been silent here other
>than pointing out that your request alloc was too fat to inline.
>Mine is trivially inlined, but not refcounted. Likely there's a
>happy middle ground, maybe
>
> mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
> mesg = spi_message_get(mesg);
> spi_message_put();
>
>
Well, it's pretty much what we do in the latest one...
Though we use one-by-one chaining and not specifying the number of
messages in chain.
I'm not sure which approach is better, really. Maybe an API like
struct spi_msg *spi_message_alloc(struct spi_device *, unsigned ntrans);
spi_add_transfer(struct spi_msg *, ...);
...
is better than what we've got now (see patch sent 12/05; I plan to post
the update tomorrow).
I would just like to say that defining such an API looks better thing to
me than dealing with SPI message structure explicitly.

Vitaly

2005-12-11 23:54:55

by David Brownell

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh


> >Synchronous transfers can easily use stack allocation for
> >the descriptions, yes.
> >
> >Not that they need to ... the ads7846 driver allocates its
> >spi_message and spi_transfer objects on the heap both for
> >synchronous operations (temperature and voltage sensing) and
> >for asynch ones (touchscreen tracking from timer and irq).
>
> So are you talking about only kzalloc vs kmalloc?

Certainly not. But I was re-emphasizing that zero-init rule,
in a different part of my reply.


> I was trying to compare the approaches in a somehow deeper way.
> That said, I meant that not exposing any structures you don't have to
> expose was usually a right way to do things.
> We don't expose SPI message and you do.

What information there _isn't_ in the "have to expose" category?

Yours certainly has some ... clocks as per-message not per-device,
and your message utilities still doing kmalloc() and memcpy() in
places where it's not clearly necessary. Plus the error-prone
notion that completion callbacks could be optional, and a few
other things I've pointed out previously.

The choice of an array of spi_transfer rather than a custom
singly linked list (not even list_head)? I just picked the
one with the smallest mandatory overhead (including adding the
least number of fault cases to test and recover from). Lots of
APIs made the same choice; it works just fine here. Heck, it's
one of the few things here that resembles the I2C API!


> On the other hand, our approach is flexible in means of message
> allocation. I. e. a small memory allocation library can be implemented
> transparently to device drivers that handles message allocation. It's
> very easy (and lightweight :)) since the message structure is always of
> a same length... Agree?

No, as I explained before. But I'd not mind having optional layers
like that, so long as they were in fact optional. Yours is not; plus
it's heavy-weight (embedding mallocation of dma bounce buffers and
copying into/out of them, even when it's not necessary) and it's not
refcounted.


> >That said ... I know some people _do_ like krefcounted APIs that
> >do that kind of stuff. Strongly. Greg's been silent here other
> >than pointing out that your request alloc was too fat to inline.
> >Mine is trivially inlined, but not refcounted. Likely there's a
> >happy middle ground, maybe
> >
> > mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
> > mesg = spi_message_get(mesg);
> > spi_message_put();
> >
> >
> Well, it's pretty much what we do in the latest one...

No, you have no get/put krefcounting, and (to repeat an earlier
comment) you also require "ntrans" separate allocations. So
"what 'we' do in the latest one" is substantially different.


> Though we use one-by-one chaining and not specifying the number of
> messages in chain.
> I'm not sure which approach is better, really.

In terms of potential faults that requires (debugging) driver code to
recover from, having a single allocation is a clear win. Have you
ever noticed now many patches go into Linux to handle cases where
driver fault cleanup got confused, and oopsed in one of the less
common (but still observed in the Real World) scenarios?


> Maybe an API like
> struct spi_mjsg *spi_message_alloc(struct spi_device *, unsigned ntrans);
> spi_add_transfer(struct spi_msg *, ...);
> ...

I thought about such an "add" call, but there'd need to be three of them
(to add a TX buffer, an RX buffer, or both TX and RX buffers) and it really
doesn't seem even conceptually hard to expect folk to write

msg->transfer[0].tx_buf = ...;
msg->transfer[0].length = ...;

msg->transfer[1].rx_buf = ...;
msg->transfer[1].length = ...;

msg->transfer[2].tx_buf = ...;
msg->transfer[2].rx_buf = ...;
msg->transfer[2].length = ...;

And in fact, having that stuff explicit seems preferable to me; no point
in "convenience" wrappers for something already that simple.

The only potentially useful thing about an spi_transfer_add_{rx,tx,txrx}()
set of inlines would be that it might make the protocol tweaking options
stand out more. Say, like the "drop chipselect for 20 usec after this
and before the next one", or other customization. Me, I'd rather highlight
such options with intelligent use of whitespace and comments.


> is better than what we've got now (see patch sent 12/05; I plan to post
> the update tomorrow).
> I would just like to say that defining such an API looks better thing to
> me than dealing with SPI message structure explicitly.

You've heard where and why I disagree. But I'd probably not turn down a
patch that adds optional krefcounting support for spi_message, returning
a message with the transfer[] array ready to be filled out.

- Dave

2005-12-12 07:10:17

by Vitaly Wool

[permalink] [raw]
Subject: Re: [spi-devel-general] Re: [PATCH 2.6-git] SPI core refresh

David Brownell wrote:

>>I was trying to compare the approaches in a somehow deeper way.
>>That said, I meant that not exposing any structures you don't have to
>>expose was usually a right way to do things.
>>We don't expose SPI message and you do.
>>
>>
>
>What information there _isn't_ in the "have to expose" category?
>
>
I'm sure that any message internals aren't; it leaves freedom to
implement messages allocation in the way we like.

>Yours certainly has some ... clocks as per-message not per-device,
>and your message utilities still doing kmalloc() and memcpy() in
>places where it's not clearly necessary. Plus the error-prone
>notion that completion callbacks could be optional, and a few
>other things I've pointed out previously.
>
>
What's wrong with a) clocks per device b) no completion callback?
b) just makes it a sync call, it's a convenient thing for a developer.
As for places with kmalloc's and memcpy's, can you just point out one?
I'm afraid it's all bare words here.
On the _contrary_, the approach we use allows to use lightweight
non-standard allocation methods for messages which you just can't do.

>The choice of an array of spi_transfer rather than a custom
>singly linked list (not even list_head)? I just picked the
>one with the smallest mandatory overhead (including adding the
>least number of fault cases to test and recover from). Lots of
>APIs made the same choice; it works just fine here. Heck, it's
>one of the few things here that resembles the I2C API!
>
>
Yes, but the transfer array is not of a fixed size, therefore it's not
that easy to use custom allocation techniques.
I'm afraid though that you don't quite understand what I'm meaning here,
but you'll see it in the core I'll post today.

>
>
>
>>On the other hand, our approach is flexible in means of message
>>allocation. I. e. a small memory allocation library can be implemented
>>transparently to device drivers that handles message allocation. It's
>>very easy (and lightweight :)) since the message structure is always of
>>a same length... Agree?
>>
>>
>
>No, as I explained before. But I'd not mind having optional layers
>like that, so long as they were in fact optional. Yours is not; plus
>it's heavy-weight (embedding mallocation of dma bounce buffers and
>copying into/out of them, even when it's not necessary) and it's not
>refcounted.
>
>
What? What optional layers are you talking about???
And please look *attentively* into the code, memory allocation/copying
happens only if a specific flag is set so it's no way it can be unnecessary.

>
>
>
>>>That said ... I know some people _do_ like krefcounted APIs that
>>>do that kind of stuff. Strongly. Greg's been silent here other
>>>than pointing out that your request alloc was too fat to inline.
>>>Mine is trivially inlined, but not refcounted. Likely there's a
>>>happy middle ground, maybe
>>>
>>> mesg = spi_message_alloc(struct spi_device *, unsigned ntrans);
>>> mesg = spi_message_get(mesg);
>>> spi_message_put();
>>>
>>>
>>>
>>>
>>Well, it's pretty much what we do in the latest one...
>>
>>
>
>No, you have no get/put krefcounting, and (to repeat an earlier
>comment) you also require "ntrans" separate allocations. So
>"what 'we' do in the latest one" is substantially different.
>
>
I was speaking about the API. Sorry for being not that clear here,

>
>
>
>>Though we use one-by-one chaining and not specifying the number of
>>messages in chain.
>>I'm not sure which approach is better, really.
>>
>>
>
>In terms of potential faults that requires (debugging) driver code to
>recover from, having a single allocation is a clear win. Have you
>ever noticed now many patches go into Linux to handle cases where
>driver fault cleanup got confused, and oopsed in one of the less
>common (but still observed in the Real World) scenarios?
>
>
Not necessarily; if we've got a preallocated memory pool, than it's not
that important.
On the contrary, having messages of a same size makes it easier for
write a custom malloc routine.

Vitaly