[RFC] SPI core

[RFC] SPI core

[dmitry pervushin]

Hello guys,

In order to support the specific board, we have ported the generic SPI core to the 2.6 kernel. This core provides basic API to create/manage SPI devices like the I2C core does. We need to continue providing support of SPI devices and would like to maintain the SPI subtree. It would be nice if SPI core patch were applied to the vanilla kernel.
I2C people do not like to mainain this code as well as I2C, so...

Signed-off-by: dmitry pervushin <dpervushin@gmail.com>

KernelVersion: 2.6.12-rc4

PATCH FOLLOWS
diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/Kconfig linux/drivers/spi/Kconfig
--- linux-2.6.12-rc4/drivers/spi/Kconfig	1970-01-01 03:00:00.000000000 +0300
+++ linux/drivers/spi/Kconfig	2005-05-31 19:59:01.000000000 +0400
@@ -0,0 +1,26 @@
+#
+# Character device configuration
+#
+
+menu "SPI support"
+
+config SPI
+	tristate "SPI support"
+
+config SPI_ALGOBIT
+	tristate "SPI bit-banging interfaces"
+	depends on SPI
+
+config SPI_CHARDEV
+	tristate "SPI device interface"
+	depends on SPI
+	help
+	  Say Y here to use spi-* device files, usually found in the /dev
+	  directory on your system.  They make it possible to have user-space
+	  programs use the SPI bus.  Information on how to do this is
+	  contained in the file <file:Documentation/spi/dev-interface>.
+
+	  This support is also available as a module.  If so, the module 
+	  will be called spi-dev.
+
+endmenu
diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/Makefile linux/drivers/spi/Makefile
--- linux-2.6.12-rc4/drivers/spi/Makefile	1970-01-01 03:00:00.000000000 +0300
+++ linux/drivers/spi/Makefile	2005-05-31 19:59:01.000000000 +0400
@@ -0,0 +1,12 @@
+#
+# Makefile for the kernel spi bus driver.
+#
+
+# Init order: core, chardev, bit adapters, pcf adapters
+
+obj-$(CONFIG_SPI)		+= spi-core.o
+obj-$(CONFIG_SPI_CHARDEV)	+= spi-dev.o
+
+# Bit adapters
+
+# PCF adapters
diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-core.c linux/drivers/spi/spi-core.c
--- linux-2.6.12-rc4/drivers/spi/spi-core.c	1970-01-01 03:00:00.000000000 +0300
+++ linux/drivers/spi/spi-core.c	2005-05-31 19:59:12.000000000 +0400
@@ -0,0 +1,385 @@
+/*
+ *  linux/drivers/spi/spi-core.c
+ *
+ *  Copyright (C) 2001 Russell King
+ *  Copyright (C) 2002 Compaq Computer Corporation
+ *
+ *  Adapted from l3-core.c by Jamey Hicks <jamey.hicks@compaq.com>
+ *
+ * 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.
+ *
+ *  See linux/Documentation/spi for further documentation.
+ */
+#include <linux/module.h>
+#include <linux/config.h>
+#include <linux/kernel.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/spi/spi.h>
+
+static DECLARE_MUTEX(adapter_lock);
+static LIST_HEAD(adapter_list);
+
+static DECLARE_MUTEX(driver_lock);
+static LIST_HEAD(driver_list);
+
+/**
+ * spi_add_adapter - register a new SPI bus adapter
+ * @adap: spi_adapter structure for the registering adapter
+ *
+ * Make the adapter available for use by clients using name adap->name.
+ * The adap->adapters list is initialised by this function.
+ *
+ * Returns 0;
+ */
+int spi_add_adapter(struct spi_adapter *adap)
+{
+	struct list_head *l;
+
+	INIT_LIST_HEAD(&adap->clients);
+	down(&adapter_lock);
+	init_MUTEX(&adap->lock);
+	list_add(&adap->adapters, &adapter_list);
+	up(&adapter_lock);
+
+	list_for_each(l, &driver_list) {
+		struct spi_driver *drv =
+		    list_entry(l, struct spi_driver, drivers);
+
+		if (drv->attach_adapter)
+			drv->attach_adapter(adap);
+	}
+
+	return 0;
+}
+
+/**
+ * spi_del_adapter - unregister a SPI bus adapter
+ * @adap: spi_adapter structure to unregister
+ *
+ * Remove an adapter from the list of available SPI Bus adapters.
+ *
+ * Returns 0;
+ */
+int spi_del_adapter(struct spi_adapter *adap)
+{
+	struct list_head *l;
+
+	down(&adapter_lock);
+	list_del(&adap->adapters);
+	up(&adapter_lock);
+
+	list_for_each(l, &driver_list) {
+		struct spi_driver *drv =
+		    list_entry(l, struct spi_driver, drivers);
+
+		if (drv->detach_adapter)
+			drv->detach_adapter(adap);
+	}
+
+	return 0;
+}
+
+/**
+ * spi_get_adapter - get a reference to an adapter
+ * @id: driver id
+ *
+ * Obtain a spi_adapter structure for the specified adapter.  If the adapter
+ * is not currently load, then load it.  The adapter will be locked in core
+ * until all references are released via spi_put_adapter.
+ */
+struct spi_adapter *spi_get_adapter(int id)
+{
+	struct list_head *item;
+	struct spi_adapter *adapter;
+
+	down(&adapter_lock);
+	list_for_each(item, &adapter_list) {
+		adapter = list_entry(item, struct spi_adapter, adapters);
+		if (id == adapter->nr && try_module_get(adapter->owner)) {
+			up(&adapter_lock);
+			return adapter;
+		}
+	}
+	up(&adapter_lock);
+	return NULL;
+
+}
+
+/**
+ * spi_put_adapter - release a reference to an adapter
+ * @adap: driver to release reference
+ *
+ * Indicate to the SPI core that you no longer require the adapter reference.
+ * The adapter module may be unloaded when there are no references to its
+ * data structure.
+ *
+ * You must not use the reference after calling this function.
+ */
+void spi_put_adapter(struct spi_adapter *adap)
+{
+	if (adap && adap->owner)
+		module_put(adap->owner);
+}
+
+/**
+ * spi_add_driver - register a new SPI device driver
+ * @driver - driver structure to make available
+ *
+ * Make the driver available for use by clients using name driver->name.
+ * The driver->drivers list is initialised by this function.
+ *
+ * Returns 0;
+ */
+int spi_add_driver(struct spi_driver *driver)
+{
+	down(&driver_lock);
+	list_add(&driver->drivers, &driver_list);
+	up(&driver_lock);
+	return 0;
+}
+
+/**
+ * spi_del_driver - unregister a SPI device driver
+ * @driver: driver to remove
+ *
+ * Remove an driver from the list of available SPI Bus device drivers.
+ *
+ * Returns 0;
+ */
+int spi_del_driver(struct spi_driver *driver)
+{
+	down(&driver_lock);
+	list_del(&driver->drivers);
+	up(&driver_lock);
+	return 0;
+}
+
+static struct spi_driver *__spi_get_driver(const char *name)
+{
+	struct list_head *l;
+
+	list_for_each(l, &driver_list) {
+		struct spi_driver *drv =
+		    list_entry(l, struct spi_driver, drivers);
+
+		if (strcmp(drv->name, name) == 0)
+			return drv;
+	}
+
+	return NULL;
+}
+
+/**
+ * spi_get_driver - get a reference to a driver
+ * @name: driver name
+ *
+ * Obtain a spi_driver structure for the specified driver.  If the driver is
+ * not currently load, then load it.  The driver will be locked in core
+ * until all references are released via spi_put_driver.
+ */
+struct spi_driver *spi_get_driver(const char *name)
+{
+	struct spi_driver *drv = NULL;
+	int try;
+
+	for (try = 0; try < 2; try++) {
+		down(&adapter_lock);
+		drv = __spi_get_driver(name);
+		if (drv && !try_module_get(drv->owner))
+			drv = NULL;
+		up(&adapter_lock);
+
+		if (drv)
+			break;
+
+		if (try == 0)
+			request_module(name);
+	}
+
+	return drv;
+}
+
+/**
+ * spi_put_driver - release a reference to a driver
+ * @drv: driver to release reference
+ *
+ * Indicate to the SPI core that you no longer require the driver reference.
+ * The driver module may be unloaded when there are no references to its
+ * data structure.
+ *
+ * You must not use the reference after calling this function.
+ */
+void spi_put_driver(struct spi_driver *drv)
+{
+	if (drv && drv->owner)
+		module_put(drv->owner);
+}
+
+/**
+ * spi_attach_client - attach a client to an adapter and driver
+ * @client: client structure to attach
+ * @adap: adapter (module) name
+ * @drv: driver (module) name
+ *
+ * Attempt to attach a client (a user of a device driver) to a particular
+ * driver and adapter.  If the specified driver or adapter aren't registered,
+ * request_module is used to load the relevant modules.
+ *
+ * Returns 0 on success, or negative error code.
+ */
+int spi_attach_client(struct spi_client *client, int id, const char *drv)
+{
+	struct spi_adapter *adapter = spi_get_adapter(id);
+	struct spi_driver *driver = spi_get_driver(drv);
+	int ret = -ENOENT;
+
+	if (!adapter)
+		printk(KERN_ERR "%s: unable to get adapter: %d\n", __FUNCTION__,
+		       id);
+	if (!driver)
+		printk(KERN_ERR "%s: unable to get driver: %s\n", __FUNCTION__,
+		       drv);
+
+	if (adapter && driver) {
+		ret = 0;
+
+		client->adapter = adapter;
+		client->driver = driver;
+
+		list_add(&client->__adap, &adapter->clients);
+
+		if (driver->attach_client)
+			ret = driver->attach_client(client);
+	}
+
+	if (ret) {
+		spi_put_driver(driver);
+		spi_put_adapter(adapter);
+	}
+	return ret;
+}
+
+/**
+ * spi_detach_client - detach a client from an adapter and driver
+ * @client: client structure to detach
+ *
+ * Detach the client from the adapter and driver.
+ */
+int spi_detach_client(struct spi_client *client)
+{
+	struct spi_adapter *adapter = client->adapter;
+	struct spi_driver *driver = client->driver;
+
+	driver->detach_client(client);
+
+	client->adapter = NULL;
+	client->driver = NULL;
+
+	spi_put_driver(driver);
+	spi_put_adapter(adapter);
+
+	list_del(&client->__adap);
+
+	return 0;
+}
+
+/**
+ * spi_transfer - transfer information on an SPI bus
+ * @adap: adapter structure to perform transfer on
+ * @msgs: array of spi_msg structures describing transfer
+ * @num: number of spi_msg structures
+ *
+ * Transfer the specified messages to/from a device on the SPI bus.
+ *
+ * Returns number of messages successfully transferred, otherwise negative
+ * error code.
+ */
+int spi_transfer(struct spi_adapter *adap, struct spi_msg msgs[], int num)
+{
+	int ret = -ENOSYS;
+
+	if (adap->algo->xfer) {
+		down(&adap->lock);
+		ret = adap->algo->xfer(adap, msgs, num);
+		up(&adap->lock);
+	}
+	return ret;
+}
+
+/**
+ * spi_write - send data to a device on an SPI bus
+ * @client: registered client structure
+ * @addr: SPI bus address
+ * @buf: buffer for bytes to send
+ * @len: number of bytes to send
+ *
+ * Send len bytes pointed to by buf to device address addr on the SPI bus
+ * described by client.
+ *
+ * Returns the number of bytes transferred, or negative error code.
+ */
+int spi_write(struct spi_client *client, int addr, const char *buf, int len)
+{
+	struct spi_adapter *adap = client->adapter;
+	struct spi_msg msg;
+	int ret;
+
+	msg.addr = addr;
+	msg.flags = 0;
+	msg.buf = (char *)buf;
+	msg.len = len;
+
+	ret = spi_transfer(adap, &msg, 1);
+	return ret == 1 ? len : ret;
+}
+
+/**
+ * spi_read - receive data from a device on an SPI bus
+ * @client: registered client structure
+ * @addr: SPI bus address
+ * @buf: buffer for bytes to receive
+ * @len: number of bytes to receive
+ *
+ * Receive len bytes from device address addr on the SPI bus described by
+ * client to a buffer pointed to by buf.
+ *
+ * Returns the number of bytes transferred, or negative error code.
+ */
+int spi_read(struct spi_client *client, int addr, char *buf, int len)
+{
+	struct spi_adapter *adap = client->adapter;
+	struct spi_msg msg;
+	int ret;
+
+	msg.addr = addr;
+	msg.flags = SPI_M_RD;
+	msg.buf = buf;
+	msg.len = len;
+
+	ret = spi_transfer(adap, &msg, 1);
+	return ret == 1 ? len : ret;
+}
+
+EXPORT_SYMBOL(spi_add_adapter);
+EXPORT_SYMBOL(spi_del_adapter);
+EXPORT_SYMBOL(spi_get_adapter);
+EXPORT_SYMBOL(spi_put_adapter);
+
+EXPORT_SYMBOL(spi_add_driver);
+EXPORT_SYMBOL(spi_del_driver);
+EXPORT_SYMBOL(spi_get_driver);
+EXPORT_SYMBOL(spi_put_driver);
+
+EXPORT_SYMBOL(spi_attach_client);
+EXPORT_SYMBOL(spi_detach_client);
+
+EXPORT_SYMBOL(spi_transfer);
+EXPORT_SYMBOL(spi_write);
+EXPORT_SYMBOL(spi_read);
diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-dev.c linux/drivers/spi/spi-dev.c
--- linux-2.6.12-rc4/drivers/spi/spi-dev.c	1970-01-01 03:00:00.000000000 +0300
+++ linux/drivers/spi/spi-dev.c	2005-05-31 19:59:18.000000000 +0400
@@ -0,0 +1,514 @@
+/*
+    spi-dev.c - spi-bus driver, char device interface  
+
+    Copyright (C) 1995-97 Simon G. Vogl
+    Copyright (C) 1998-99 Frodo Looijaard <frodol@dds.nl>
+    Copyright (C) 2002 Compaq Computer Corporation
+
+    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.
+*/
+
+/* Adapted from i2c-dev module by Jamey Hicks <jamey.hicks@compaq.com> */
+
+/* Note that this is a complete rewrite of Simon Vogl's i2c-dev module.
+   But I have used so much of his original code and ideas that it seems
+   only fair to recognize him as co-author -- Frodo */
+
+/* The devfs code is contributed by Philipp Matthias Hahn 
+   <pmhahn@titan.lahn.de> */
+
+/* Modifications to allow work with current spi-core by 
+   Andrey Ivolgin <aivolgin@ru.mvista.com>, Sep 2004
+ */
+
+/* devfs code corrected to support automatic device addition/deletion
+   by Vitaly Wool <vwool@ru.mvista.com> (C) 2004 MontaVista Software, Inc. 
+ */
+
+/* $Id: cee_lsp-philips-melody.patch,v 1.1.4.8 2005/02/25 10:20:15 wool Exp $ */
+
+#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>
+#ifdef CONFIG_DEVFS_FS
+#include <linux/devfs_fs_kernel.h>
+#endif
+
+/*  Define SPIDEV_DEBUG for debugging info  */
+#undef SPIDEV_DEBUG
+
+#ifdef SPIDEV_DEBUG
+#define DBG(args...)	printk(KERN_INFO"spi-dev.o: " args)
+#else
+#define DBG(args...)
+#endif
+
+#include <linux/init.h>
+#include <asm/uaccess.h>
+#include <linux/spi/spi.h>
+
+#define SPI_TRANSFER_MAX	65535
+#define SPI_ADAP_MAX		32
+
+extern struct spi_adapter *spi_get_adapter(int id);	/* spi-core.c ? */
+extern void spi_put_adapter(struct spi_adapter *);
+
+/* struct file_operations changed too often in the 2.1 series for nice code */
+
+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);
+static int spidev_ioctl(struct inode *inode, struct file *file,
+			unsigned int cmd, unsigned long arg);
+static int spidev_attach_adapter(struct spi_adapter *);
+static int spidev_detach_adapter(struct spi_adapter *);
+static int __init spi_dev_init(void);
+static void spidev_cleanup(void);
+
+static struct file_operations spidev_fops = {
+      owner:THIS_MODULE,
+      llseek:no_llseek,
+      read:spidev_read,
+      write:spidev_write,
+      open:spidev_open,
+      release:spidev_release,
+      ioctl:spidev_ioctl,
+};
+
+static struct spi_driver spidev_driver = {
+      name:"spi",
+      attach_adapter:spidev_attach_adapter,
+      detach_adapter:spidev_detach_adapter,
+      owner:THIS_MODULE,
+};
+
+static struct spi_client spidev_client_template = {
+      driver:&spidev_driver
+};
+
+struct spi_dev {
+	int minor;
+	struct spi_adapter *adap;
+	struct class_device class_dev;
+	struct completion released;	/* FIXME, we need a class_device_unregister() */
+};
+
+#define to_spi_dev(d) container_of(d, struct spi_dev, class_dev)
+
+#define SPI_MINORS	256
+static struct spi_dev *spi_dev_array[SPI_MINORS];
+static DEFINE_SPINLOCK(spi_dev_array_lock);
+
+struct spi_dev *spi_dev_get_by_minor(unsigned index)
+{
+	struct spi_dev *spi_dev;
+
+	spin_lock(&spi_dev_array_lock);
+	spi_dev = spi_dev_array[index];
+	spin_unlock(&spi_dev_array_lock);
+	return spi_dev;
+}
+
+struct spi_dev *spi_dev_get_by_adapter(struct spi_adapter *adap)
+{
+	struct spi_dev *spi_dev = NULL;
+
+	spin_lock(&spi_dev_array_lock);
+	if ((spi_dev_array[adap->nr]) &&
+	    (spi_dev_array[adap->nr]->adap == adap))
+		spi_dev = spi_dev_array[adap->nr];
+	spin_unlock(&spi_dev_array_lock);
+	return spi_dev;
+}
+
+static struct spi_dev *get_free_spi_dev(struct spi_adapter *adap)
+{
+	struct spi_dev *spi_dev;
+
+	spi_dev = kmalloc(sizeof(*spi_dev), GFP_KERNEL);
+	if (!spi_dev)
+		return ERR_PTR(-ENOMEM);
+	memset(spi_dev, 0x00, sizeof(*spi_dev));
+
+	spin_lock(&spi_dev_array_lock);
+	if (spi_dev_array[adap->nr]) {
+		spin_unlock(&spi_dev_array_lock);
+		dev_err(&adap->dev,
+			"spi-dev already has a device assigned to this adapter\n");
+		goto error;
+	}
+	spi_dev->minor = adap->nr;
+	spi_dev_array[adap->nr] = spi_dev;
+	spin_unlock(&spi_dev_array_lock);
+	return spi_dev;
+      error:
+	kfree(spi_dev);
+	return ERR_PTR(-ENODEV);
+}
+
+static void return_spi_dev(struct spi_dev *spi_dev)
+{
+	spin_lock(&spi_dev_array_lock);
+	spi_dev_array[spi_dev->minor] = NULL;
+	spin_unlock(&spi_dev_array_lock);
+}
+
+static ssize_t show_dev(struct class_device *class_dev, char *buf)
+{
+	struct spi_dev *spi_dev = to_spi_dev(class_dev);
+	return print_dev_t(buf, MKDEV(SPI_MAJOR, spi_dev->minor));
+}
+
+static CLASS_DEVICE_ATTR(dev, S_IRUGO, show_dev, NULL);
+
+static ssize_t show_adapter_name(struct class_device *class_dev, char *buf)
+{
+	struct spi_dev *spi_dev = to_spi_dev(class_dev);
+	return sprintf(buf, "%s\n", spi_dev->adap->name);
+}
+
+static CLASS_DEVICE_ATTR(name, S_IRUGO, show_adapter_name, NULL);
+
+static int spidev_ioctl(struct inode *inode, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	struct spi_client *client = (struct spi_client *)file->private_data;
+	struct spi_rdwr_ioctl_data rdwr_arg;
+	struct spi_msg *rdwr_pa;
+	int res, i;
+	unsigned long (*cpy_to_user) (void *to_user, const void *from,
+				      unsigned long len);
+	unsigned long (*cpy_from_user) (void *to, const void *from_user,
+					unsigned long len);
+	void *(*alloc) (size_t, int);
+	void (*free) (const void *);
+
+	DBG("spi-%d ioctl, cmd: 0x%x, arg: %lx.\n",
+	    MINOR(inode->i_rdev), cmd, arg);
+
+	cpy_to_user =
+	    client->adapter->copy_to_user ? client->adapter->
+	    copy_to_user : copy_to_user;
+	cpy_from_user =
+	    client->adapter->copy_from_user ? client->adapter->
+	    copy_from_user : copy_from_user;
+	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
+	free = client->adapter->free ? client->adapter->free : kfree;
+
+	switch (cmd) {
+	case SPI_RDWR:
+		if (copy_from_user(&rdwr_arg,
+				   (struct spi_rdwr_ioctl_data *)arg,
+				   sizeof(rdwr_arg)))
+			return -EFAULT;
+
+		rdwr_pa = (struct spi_msg *)
+		    kmalloc(rdwr_arg.nmsgs * sizeof(struct spi_msg),
+			    GFP_KERNEL);
+
+		if (rdwr_pa == NULL)
+			return -ENOMEM;
+
+		res = 0;
+		for (i = 0; i < rdwr_arg.nmsgs; i++) {
+			if (copy_from_user(&(rdwr_pa[i]),
+					   &(rdwr_arg.msgs[i]),
+					   sizeof(rdwr_pa[i]))) {
+				res = -EFAULT;
+				break;
+			}
+			rdwr_pa[i].buf = alloc(rdwr_pa[i].len, GFP_KERNEL);
+			if (rdwr_pa[i].buf == NULL) {
+				res = -ENOMEM;
+				break;
+			}
+			if (cpy_from_user(rdwr_pa[i].buf,
+					  rdwr_arg.msgs[i].buf,
+					  rdwr_pa[i].len)) {
+				free(rdwr_pa[i].buf);
+				res = -EFAULT;
+				break;
+			}
+		}
+		if (!res) {
+			res = spi_transfer(client->adapter,
+					   rdwr_pa, rdwr_arg.nmsgs);
+		}
+		while (i-- > 0) {
+			if (res >= 0 && (rdwr_pa[i].flags & SPI_M_RD))
+				if (cpy_to_user(rdwr_arg.msgs[i].buf,
+						rdwr_pa[i].buf,
+						rdwr_pa[i].len)) {
+					res = -EFAULT;
+				}
+			free(rdwr_pa[i].buf);
+		}
+		kfree(rdwr_pa);
+		return res;
+
+	case SPI_CLK_RATE:
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static void release_spi_dev(struct class_device *dev)
+{
+	struct spi_dev *spi_dev = to_spi_dev(dev);
+	complete(&spi_dev->released);
+}
+
+static struct class spi_dev_class = {
+	.name = "spi-dev",
+	.release = &release_spi_dev,
+};
+
+static int spidev_attach_adapter(struct spi_adapter *adap)
+{
+	struct spi_dev *spi_dev;
+	int retval;
+
+	spi_dev = get_free_spi_dev(adap);
+	if (IS_ERR(spi_dev))
+		return PTR_ERR(spi_dev);
+
+#if defined( CONFIG_DEVFS_FS )
+	devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor),
+		      S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor);
+#endif
+	dev_dbg(&adap->dev, "Registered as minor %d\n", spi_dev->minor);
+
+	/* register this spi device with the driver core */
+	spi_dev->adap = adap;
+	if (adap->dev.parent == &platform_bus)
+		spi_dev->class_dev.dev = &adap->dev;
+	else
+		spi_dev->class_dev.dev = adap->dev.parent;
+	spi_dev->class_dev.class = &spi_dev_class;
+	snprintf(spi_dev->class_dev.class_id, BUS_ID_SIZE, "spi-%d",
+		 spi_dev->minor);
+	retval = class_device_register(&spi_dev->class_dev);
+	if (retval)
+		goto error;
+	class_device_create_file(&spi_dev->class_dev, &class_device_attr_dev);
+	class_device_create_file(&spi_dev->class_dev, &class_device_attr_name);
+	return 0;
+      error:
+	return_spi_dev(spi_dev);
+	kfree(spi_dev);
+	return retval;
+
+}
+
+static int spidev_detach_adapter(struct spi_adapter *adap)
+{
+	struct spi_dev *spi_dev;
+
+	spi_dev = spi_dev_get_by_adapter(adap);
+	if (!spi_dev)
+		return -ENODEV;
+
+	init_completion(&spi_dev->released);
+#if defined( CONFIG_DEVFS_FS )
+	devfs_remove("spi/%d", spi_dev->minor);
+#endif
+	return_spi_dev(spi_dev);
+	class_device_unregister(&spi_dev->class_dev);
+	wait_for_completion(&spi_dev->released);
+	kfree(spi_dev);
+
+	dev_dbg(&adap->dev, "Adapter unregistered\n");
+	return 0;
+
+}
+
+static ssize_t spidev_read(struct file *file, char *buf, size_t count,
+			   loff_t * offset)
+{
+	char *tmp;
+	int ret;
+#ifdef SPIDEV_DEBUG
+	struct inode *inode = file->f_dentry->d_inode;
+#endif
+
+	struct spi_client *client = (struct spi_client *)file->private_data;
+	unsigned long (*cpy_to_user) (void *to_user, const void *from,
+				      unsigned long len);
+	void *(*alloc) (size_t, int);
+	void (*free) (const void *);
+	if (count > SPI_TRANSFER_MAX)
+		count = SPI_TRANSFER_MAX;
+
+	cpy_to_user =
+	    client->adapter->copy_to_user ? client->adapter->
+	    copy_to_user : copy_to_user;
+	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
+	free = client->adapter->free ? client->adapter->free : kfree;
+
+	/* copy user space data to kernel space. */
+	tmp = alloc(count, GFP_KERNEL);
+	if (tmp == NULL)
+		return -ENOMEM;
+
+	DBG("spi-%d reading %d bytes.\n", MINOR(inode->i_rdev), count);
+
+	ret = spi_read(client, 0, tmp, count);
+	if (ret >= 0)
+		ret = cpy_to_user(buf, tmp, count) ? -EFAULT : ret;
+	free(tmp);
+	return ret;
+}
+
+static ssize_t spidev_write(struct file *file, const char *buf, size_t count,
+			    loff_t * offset)
+{
+	int ret;
+	char *tmp;
+	struct spi_client *client = (struct spi_client *)file->private_data;
+#ifdef SPIDEV_DEBUG
+	struct inode *inode = file->f_dentry->d_inode;
+#endif
+	unsigned long (*cpy_from_user) (void *to, const void *from_user,
+					unsigned long len);
+	void *(*alloc) (size_t, int);
+	void (*free) (const void *);
+
+	if (count > SPI_TRANSFER_MAX)
+		count = SPI_TRANSFER_MAX;
+
+	cpy_from_user =
+	    client->adapter->copy_from_user ? client->adapter->
+	    copy_from_user : copy_from_user;
+	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
+	free = client->adapter->free ? client->adapter->free : kfree;
+
+	/* copy user space data to kernel space. */
+	tmp = alloc(count, GFP_KERNEL);
+	if (tmp == NULL)
+		return -ENOMEM;
+
+	if (cpy_from_user(tmp, buf, count)) {
+		free(tmp);
+		return -EFAULT;
+	}
+
+	DBG("spi-%d writing %d bytes.\n", MINOR(inode->i_rdev), count);
+	ret = spi_write(client, 0, tmp, count);
+	free(tmp);
+	return ret;
+}
+
+int spidev_open(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	struct spi_client *client;
+	struct spi_adapter *adap;
+	struct spi_dev *spi_dev;
+
+	spi_dev = spi_dev_get_by_minor(minor);
+	if (!spi_dev)
+		return -ENODEV;
+
+	adap = spi_get_adapter(spi_dev->adap->nr);
+	if (!adap)
+		return -ENODEV;
+
+	client = kmalloc(sizeof(*client), GFP_KERNEL);
+	if (!client) {
+		spi_put_adapter(adap);
+		return -ENOMEM;
+	}
+	memcpy(client, &spidev_client_template, sizeof(*client));
+
+	/* registered with adapter, passed as client to user */
+	client->adapter = adap;
+	file->private_data = client;
+
+	return 0;
+
+}
+
+static int spidev_release(struct inode *inode, struct file *file)
+{
+	struct spi_client *client = file->private_data;
+
+	spi_put_adapter(client->adapter);
+	kfree(client);
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static int __init spi_dev_init(void)
+{
+	int res;
+
+	printk(KERN_INFO "spi /dev entries driver\n");
+
+	res = register_chrdev(SPI_MAJOR, "spi", &spidev_fops);
+	if (res)
+		goto out;
+
+	res = class_register(&spi_dev_class);
+	if (res)
+		goto out_unreg_chrdev;
+
+	res = spi_add_driver(&spidev_driver);
+	if (res)
+		goto out_unreg_class;
+
+#ifdef CONFIG_DEVFS_FS
+	devfs_mk_dir("spi");
+#endif
+	return 0;
+
+      out_unreg_class:
+	class_unregister(&spi_dev_class);
+      out_unreg_chrdev:
+	unregister_chrdev(SPI_MAJOR, "spi");
+      out:
+	printk(KERN_ERR "%s: Driver Initialisation failed\n", __FILE__);
+	return res;
+}
+
+static void spidev_cleanup(void)
+{
+	spi_del_driver(&spidev_driver);
+	class_unregister(&spi_dev_class);
+#ifdef CONFIG_DEVFS_FS
+	devfs_remove("spi");
+#endif
+	unregister_chrdev(SPI_MAJOR, "spi");
+}
+
+MODULE_AUTHOR
+    ("Jamey Hicks <jamey.hicks@compaq.com> Frodo Looijaard <frodol@dds.nl> and Simon G. Vogl <simon@tk.uni-linz.ac.at>");
+MODULE_DESCRIPTION("SPI /dev entries driver");
+MODULE_LICENSE("GPL");
+
+module_init(spi_dev_init);
+module_exit(spidev_cleanup);
diff -uNr -X dontdiff linux-2.6.12-rc4/include/linux/spi/spi.h linux/include/linux/spi/spi.h
--- linux-2.6.12-rc4/include/linux/spi/spi.h	1970-01-01 03:00:00.000000000 +0300
+++ linux/include/linux/spi/spi.h	2005-05-31 19:59:26.000000000 +0400
@@ -0,0 +1,265 @@
+/*
+ *  linux/include/linux/spi/spi.h
+ *
+ *  Copyright (C) 2001 Russell King, All Rights Reserved.
+ *  Copyright (C) 2002 Compaq Computer Corporation, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 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
+
+struct spi_msg {
+	unsigned char addr;	/* slave address        */
+	unsigned char flags;
+#define SPI_M_RD		0x01
+#define SPI_M_NOADDR	0x02
+	unsigned short len;	/* msg length           */
+	unsigned char *buf;	/* pointer to msg data  */
+};
+
+/*  TODO: should be in a separate header file?  */
+
+/*  Commands for the ioctl call */
+#define SPI_RDWR	0x0801	/*  Read/write transfer in a single transaction */
+#define SPI_CLK_RATE	0x0802	/*  Sets SPI clock divisor (int type)           */
+#define SPI_SDMA	0x0803	/*  Turns on/off SDMA usage (int type).         */
+				 /*  Pass non-zero value to turn SDMA on         */
+struct spi_rdwr_ioctl_data {
+	struct spi_msg *msgs;	/* pointers to spi_msgs */
+	int nmsgs;		/* number of spi_msgs */
+};
+/*------------^^^^^^^^^^^^^^^--------------*/
+
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <linux/list.h>
+
+#define SPI_ADAP_MAX	32
+#define SPI_MAJOR	98
+
+struct spi_client;
+struct spi_adapter;
+
+struct spi_ops {
+	int (*open) (struct spi_client *);
+	int (*command) (struct spi_client *, int cmd, void *arg);
+	void (*close) (struct spi_client *);
+};
+
+/*
+ * A driver is capable of handling one or more physical devices present on
+ * SPI adapters. This information is used to inform the driver of adapter
+ * events.
+ */
+struct spi_driver {
+	/*
+	 * This name is used to uniquely identify the driver.
+	 * It should be the same as the module name.
+	 */
+	char name[32];
+
+	/* Notifies the driver that new adapter added to spi-core
+	 */
+	int (*attach_adapter) (struct spi_adapter *);
+
+	/* Notifies the driver that adapter has been deleted from spi-core
+	 */
+	int (*detach_adapter) (struct spi_adapter *);
+
+	/*
+	 * Notifies the driver that a new client wishes to use its
+	 * services.  Note that the module use count will be increased
+	 * prior to this function being called.  In addition, the
+	 * clients driver and adapter fields will have been setup.
+	 */
+	int (*attach_client) (struct spi_client *);
+
+	/*
+	 * Notifies the driver that the client has finished with its
+	 * services, and any memory that it allocated for this client
+	 * should be cleaned up.  In addition the chip should be
+	 * shut down.
+	 */
+	void (*detach_client) (struct spi_client *);
+
+	/*
+	 * Possible operations on the driver.
+	 */
+	struct spi_ops *ops;
+
+	/*
+	 * Module structure, if any.    
+	 */
+	struct module *owner;
+
+	/*
+	 * drivers list
+	 */
+	struct list_head drivers;
+};
+
+struct spi_adapter;
+
+struct spi_algorithm {
+	/* textual description */
+	char name[32];
+
+	/* perform bus transactions */
+	int (*xfer) (struct spi_adapter *, struct spi_msg msgs[], int num);
+};
+
+struct semaphore;
+
+/*
+ * spi_adapter is the structure used to identify a physical SPI bus along
+ * with the access algorithms necessary to access it.
+ */
+struct spi_adapter {
+	/*
+	 * This name is used to uniquely identify the adapter.
+	 * It should be the same as the module name.
+	 */
+	char name[32];
+
+	/*
+	 * the algorithm to access the bus
+	 */
+	struct spi_algorithm *algo;
+
+	/*
+	 * Algorithm specific data
+	 */
+	void *algo_data;
+
+	/*
+	 * This may be NULL, or should point to the module struct
+	 */
+	struct module *owner;
+
+	/*
+	 * private data for the adapter
+	 */
+	void *data;
+
+	/*
+	 * Our lock.
+	 */
+	struct semaphore lock;
+
+	/*
+	 * List of attached clients.
+	 */
+	struct list_head clients;
+
+	/*
+	 * List of all adapters.
+	 */
+	struct list_head adapters;
+
+	/*
+	 * The alternative way to allocate/free memory.
+	 */
+	void *(*alloc) (size_t, int);
+	void (*free) (const void *);
+
+	/*
+	 * Copy data from/to user in case the alternative
+	 * alloc/free functions are used.
+	 */
+	unsigned long (*copy_from_user) (void *to, const void *from_user,
+					 unsigned long len);
+	unsigned long (*copy_to_user) (void *to_user, const void *from,
+				       unsigned long len);
+
+	int nr;
+
+	struct device dev;	/* the adapter device */
+	struct class_device class_dev;	/* the class device */
+
+	struct completion dev_released;
+	struct completion class_dev_released;
+
+};
+
+/*
+ * spi_client identifies a single device (i.e. chip) that is connected to an 
+ * SPI bus. The behaviour is defined by the routines of the driver. This
+ * function is mainly used for lookup & other admin. functions.
+ */
+struct spi_client {
+	struct spi_adapter *adapter;	/* the adapter we sit on        */
+	struct spi_driver *driver;	/* and our access routines      */
+	void *driver_data;	/* private driver data          */
+	struct list_head __adap;
+};
+
+extern int spi_add_adapter(struct spi_adapter *);
+extern int spi_del_adapter(struct spi_adapter *);
+
+extern int spi_add_driver(struct spi_driver *);
+extern int spi_del_driver(struct spi_driver *);
+
+extern int spi_attach_client(struct spi_client *, int, const char *);
+extern int spi_detach_client(struct spi_client *);
+
+extern int spi_transfer(struct spi_adapter *, struct spi_msg msgs[], int);
+extern int spi_write(struct spi_client *, int, const char *, int);
+extern int spi_read(struct spi_client *, int, char *, int);
+
+static inline void *spi_get_adapdata(struct spi_adapter *dev)
+{
+	return dev_get_drvdata(&dev->dev);
+}
+
+static inline void spi_set_adapdata(struct spi_adapter *dev, void *data)
+{
+	dev_set_drvdata(&dev->dev, data);
+}
+
+/**
+ * spi_command - send a command to a SPI device driver
+ * @client: registered client structure
+ * @cmd: device driver command
+ * @arg: device driver arguments
+ *
+ * Ask the SPI device driver to perform some function.  Further information
+ * should be sought from the device driver in question.
+ *
+ * Returns negative error code on failure.
+ */
+static inline int spi_command(struct spi_client *clnt, int cmd, void *arg)
+{
+	struct spi_ops *ops = clnt->driver->ops;
+	int ret = -EINVAL;
+
+	if (ops && ops->command)
+		ret = ops->command(clnt, cmd, arg);
+
+	return ret;
+}
+
+static inline int spi_open(struct spi_client *clnt)
+{
+	struct spi_ops *ops = clnt->driver->ops;
+	int ret = 0;
+
+	if (ops && ops->open)
+		ret = ops->open(clnt);
+	return ret;
+}
+
+static inline void spi_close(struct spi_client *clnt)
+{
+	struct spi_ops *ops = clnt->driver->ops;
+	if (ops && ops->close)
+		ops->close(clnt);
+}
+#endif
+
+#endif				/* SPI_H */

Re: [RFC] SPI core

[randy_dunlap]

On Tue, 31 May 2005 20:09:16 +0400 dmitry pervushin wrote:

| Hello guys,
| 
| In order to support the specific board, we have ported the generic SPI core to the 2.6 kernel. This core provides basic API to create/manage SPI devices like the I2C core does. We need to continue providing support of SPI devices and would like to maintain the SPI subtree. It would be nice if SPI core patch were applied to the vanilla kernel.
| I2C people do not like to mainain this code as well as I2C, so...
| 
| Signed-off-by: dmitry pervushin <dpervushin@gmail.com>
| 
| KernelVersion: 2.6.12-rc4

Hi,

Are there more parts to the patch (other than "core")?
E.g., the patch refers to some Documentation/spi/, but those
parts are missing.

I was going to read that to see what SPI means.... so what
does it mean?


Also, a diffstat summary would be nice to see:

 drivers/spi/Kconfig     |   26 ++
 drivers/spi/Makefile    |   12 +
 drivers/spi/spi-core.c  |  385 +++++++++++++++++++++++++++++++++++
 drivers/spi/spi-dev.c   |  514 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |  266 ++++++++++++++++++++++++
 5 files changed, 1202 insertions(+), 1 deletion(-)



| diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/Makefile linux/drivers/spi/Makefile
| --- linux-2.6.12-rc4/drivers/spi/Makefile	1970-01-01 03:00:00.000000000 +0300
| +++ linux/drivers/spi/Makefile	2005-05-31 19:59:01.000000000 +0400
| @@ -0,0 +1,12 @@
| +#
| +# Makefile for the kernel spi bus driver.

Don't need such obvious comments.


| diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-core.c linux/drivers/spi/spi-core.c
| --- linux-2.6.12-rc4/drivers/spi/spi-core.c	1970-01-01 03:00:00.000000000 +0300
| +++ linux/drivers/spi/spi-core.c	2005-05-31 19:59:12.000000000 +0400
| @@ -0,0 +1,385 @@
| +#include <linux/module.h>
| +#include <linux/config.h>
| +#include <linux/kernel.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/spi/spi.h>

Please put those in alphabetical order as much as possible.


| +/**
| + * spi_get_adapter - get a reference to an adapter
| + * @id: driver id
| + *
| + * Obtain a spi_adapter structure for the specified adapter.  If the adapter
| + * is not currently load, then load it.  The adapter will be locked in core
                       loaded

| + * until all references are released via spi_put_adapter.
| + */
| +struct spi_adapter *spi_get_adapter(int id)
| +{
| +	struct list_head *item;
| +	struct spi_adapter *adapter;
| +
| +	down(&adapter_lock);
| +	list_for_each(item, &adapter_list) {
| +		adapter = list_entry(item, struct spi_adapter, adapters);
| +		if (id == adapter->nr && try_module_get(adapter->owner)) {
| +			up(&adapter_lock);
| +			return adapter;
| +		}
| +	}
| +	up(&adapter_lock);
| +	return NULL;
| +
| +}
| +
| +/**
| + * spi_get_driver - get a reference to a driver
| + * @name: driver name
| + *
| + * Obtain a spi_driver structure for the specified driver.  If the driver is
| + * not currently load, then load it.  The driver will be locked in core
                    loaded

| + * until all references are released via spi_put_driver.
| + */


| +/**
| + * spi_detach_client - detach a client from an adapter and driver
| + * @client: client structure to detach
| + *
| + * Detach the client from the adapter and driver.

    * Returns 0


| diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-dev.c linux/drivers/spi/spi-dev.c
| --- linux-2.6.12-rc4/drivers/spi/spi-dev.c	1970-01-01 03:00:00.000000000 +0300
| +++ linux/drivers/spi/spi-dev.c	2005-05-31 19:59:18.000000000 +0400
| @@ -0,0 +1,514 @@
| +
| +#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>

Alpha order as much as possible, please.

| +#ifdef CONFIG_DEVFS_FS
| +#include <linux/devfs_fs_kernel.h>
| +#endif

Shouldn't need to surround #includes with #ifdef/#endif.

| +#include <linux/init.h>
| +#include <asm/uaccess.h>
| +#include <linux/spi/spi.h>
| +
| +#define SPI_TRANSFER_MAX	65535
| +#define SPI_ADAP_MAX		32

What is SPI_ADAP_MAX used for?  It's not used in the core patch at all.
And it's #defined in 2 places, should just be put into a header file
and #included if it's needed at all.  Oh, it's in spi.h, so it's
not needed here.


| +extern struct spi_adapter *spi_get_adapter(int id);	/* spi-core.c ? */
| +extern void spi_put_adapter(struct spi_adapter *);

These should be declared in some header file, please.

| +/* struct file_operations changed too often in the 2.1 series for nice code */

Does that comment need to be here?

| +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);
| +static int spidev_ioctl(struct inode *inode, struct file *file,
| +			unsigned int cmd, unsigned long arg);
| +static int spidev_attach_adapter(struct spi_adapter *);
| +static int spidev_detach_adapter(struct spi_adapter *);
| +static int __init spi_dev_init(void);
| +static void spidev_cleanup(void);
| +
| +static struct file_operations spidev_fops = {
| +      owner:THIS_MODULE,
| +      llseek:no_llseek,
| +      read:spidev_read,
| +      write:spidev_write,
| +      open:spidev_open,
| +      release:spidev_release,
| +      ioctl:spidev_ioctl,
| +};

Please use C99-style initializers and align the init values so that
it is more readable.


| +static struct spi_driver spidev_driver = {
| +      name:"spi",
| +      attach_adapter:spidev_attach_adapter,
| +      detach_adapter:spidev_detach_adapter,
| +      owner:THIS_MODULE,
| +};

ditto

| +static struct spi_client spidev_client_template = {
| +      driver:&spidev_driver
| +};

ditto

| +struct spi_dev {
| +	int minor;
| +	struct spi_adapter *adap;
| +	struct class_device class_dev;
| +	struct completion released;	/* FIXME, we need a class_device_unregister() */
| +};

| +struct spi_dev *spi_dev_get_by_minor(unsigned index)
| +{
| +	struct spi_dev *spi_dev;
| +
| +	spin_lock(&spi_dev_array_lock);
| +	spi_dev = spi_dev_array[index];
| +	spin_unlock(&spi_dev_array_lock);
| +	return spi_dev;
| +}

Should this increment a reference count?

| +struct spi_dev *spi_dev_get_by_adapter(struct spi_adapter *adap)
| +{
| +	struct spi_dev *spi_dev = NULL;
| +
| +	spin_lock(&spi_dev_array_lock);
| +	if ((spi_dev_array[adap->nr]) &&
| +	    (spi_dev_array[adap->nr]->adap == adap))
| +		spi_dev = spi_dev_array[adap->nr];
| +	spin_unlock(&spi_dev_array_lock);
| +	return spi_dev;
| +}

same question.

| +static void return_spi_dev(struct spi_dev *spi_dev)
| +{
| +	spin_lock(&spi_dev_array_lock);
| +	spi_dev_array[spi_dev->minor] = NULL;
| +	spin_unlock(&spi_dev_array_lock);
| +}

same question (with decrement)

| +static int spidev_ioctl(struct inode *inode, struct file *file,
| +			unsigned int cmd, unsigned long arg)
| +{
| +	struct spi_client *client = (struct spi_client *)file->private_data;
| +	struct spi_rdwr_ioctl_data rdwr_arg;
| +	struct spi_msg *rdwr_pa;
| +	int res, i;
| +	unsigned long (*cpy_to_user) (void *to_user, const void *from,
| +				      unsigned long len);
| +	unsigned long (*cpy_from_user) (void *to, const void *from_user,
| +					unsigned long len);
| +	void *(*alloc) (size_t, int);
| +	void (*free) (const void *);
| +
| +	cpy_to_user =
| +	    client->adapter->copy_to_user ? client->adapter->
| +	    copy_to_user : copy_to_user;
| +	cpy_from_user =
| +	    client->adapter->copy_from_user ? client->adapter->
| +	    copy_from_user : copy_from_user;
| +	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
| +	free = client->adapter->free ? client->adapter->free : kfree;
| +
| +	switch (cmd) {
| +	case SPI_RDWR:
| +		if (copy_from_user(&rdwr_arg,
| +				   (struct spi_rdwr_ioctl_data *)arg,
| +				   sizeof(rdwr_arg)))
| +			return -EFAULT;
| +
| +		rdwr_pa = (struct spi_msg *)
| +		    kmalloc(rdwr_arg.nmsgs * sizeof(struct spi_msg),
| +			    GFP_KERNEL);
| +
| +		if (rdwr_pa == NULL)
| +			return -ENOMEM;
| +
| +		res = 0;
| +		for (i = 0; i < rdwr_arg.nmsgs; i++) {
| +			if (copy_from_user(&(rdwr_pa[i]),
| +					   &(rdwr_arg.msgs[i]),
| +					   sizeof(rdwr_pa[i]))) {
| +				res = -EFAULT;
| +				break;
| +			}
| +			rdwr_pa[i].buf = alloc(rdwr_pa[i].len, GFP_KERNEL);
| +			if (rdwr_pa[i].buf == NULL) {
| +				res = -ENOMEM;
| +				break;
| +			}

when do you user copy_from_user() vs. cpy_from_user() ?

| +			if (cpy_from_user(rdwr_pa[i].buf,
| +					  rdwr_arg.msgs[i].buf,
| +					  rdwr_pa[i].len)) {
| +				free(rdwr_pa[i].buf);
| +				res = -EFAULT;
| +				break;
| +			}
| +		}
| +		if (!res) {
| +			res = spi_transfer(client->adapter,
| +					   rdwr_pa, rdwr_arg.nmsgs);
| +		}
| +		while (i-- > 0) {
| +			if (res >= 0 && (rdwr_pa[i].flags & SPI_M_RD))
| +				if (cpy_to_user(rdwr_arg.msgs[i].buf,
| +						rdwr_pa[i].buf,
| +						rdwr_pa[i].len)) {
| +					res = -EFAULT;
| +				}
| +			free(rdwr_pa[i].buf);
| +		}
| +		kfree(rdwr_pa);
| +		return res;
| +
| +	case SPI_CLK_RATE:
| +	default:
| +		break;
| +	}
| +
| +	return 0;
| +}
| +

| +static ssize_t spidev_read(struct file *file, char *buf, size_t count,
| +			   loff_t * offset)
| +{
| +	char *tmp;
| +	int ret;
| +#ifdef SPIDEV_DEBUG
| +	struct inode *inode = file->f_dentry->d_inode;
| +#endif
| +
| +	struct spi_client *client = (struct spi_client *)file->private_data;
| +	unsigned long (*cpy_to_user) (void *to_user, const void *from,
| +				      unsigned long len);
| +	void *(*alloc) (size_t, int);
| +	void (*free) (const void *);

insert blank line here (at end of data), please.

| +	if (count > SPI_TRANSFER_MAX)
| +		count = SPI_TRANSFER_MAX;
| +
| +	cpy_to_user =
| +	    client->adapter->copy_to_user ? client->adapter->
| +	    copy_to_user : copy_to_user;
| +	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
| +	free = client->adapter->free ? client->adapter->free : kfree;
| +
| +	/* copy user space data to kernel space. */
| +	tmp = alloc(count, GFP_KERNEL);
| +	if (tmp == NULL)
| +		return -ENOMEM;
| +
| +	ret = spi_read(client, 0, tmp, count);
| +	if (ret >= 0)
| +		ret = cpy_to_user(buf, tmp, count) ? -EFAULT : ret;
| +	free(tmp);
| +	return ret;
| +}


| +int spidev_open(struct inode *inode, struct file *file)
Can't this be static?
| +{


---
~Randy

Re: [RFC] SPI core

[Alexey Dobriyan]

On Tuesday 31 May 2005 20:09, dmitry pervushin wrote:
> In order to support the specific board, we have ported the generic SPI core
> to the 2.6 kernel. This core provides basic API to create/manage SPI devices
> like the I2C core does. We need to continue providing support of SPI devices
> and would like to maintain the SPI subtree.

> +#ifdef CONFIG_DEVFS_FS
> +#include <linux/devfs_fs_kernel.h>
> +#endif

devfs will be removed from mainline in a month.

Re: [RFC] SPI core

[NZG]

SPI is serial-peripheral interface. A very common 3 wire bus in embedded 
systems, especially m68k arch's. That fact that it's not there already is 
actually a little weird IMHO.

NZG.

On Tuesday 31 May 2005 15:44, Alexey Dobriyan wrote:
> On Tuesday 31 May 2005 20:09, dmitry pervushin wrote:
> > In order to support the specific board, we have ported the generic SPI
> > core to the 2.6 kernel. This core provides basic API to create/manage SPI
> > devices like the I2C core does. We need to continue providing support of
> > SPI devices and would like to maintain the SPI subtree.
> >
> > +#ifdef CONFIG_DEVFS_FS
> > +#include <linux/devfs_fs_kernel.h>
> > +#endif
>
> devfs will be removed from mainline in a month.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] SPI core

[Greg KH]

On Wed, Jun 01, 2005 at 12:44:34AM +0400, Alexey Dobriyan wrote:
> On Tuesday 31 May 2005 20:09, dmitry pervushin wrote:
> > In order to support the specific board, we have ported the generic SPI core
> > to the 2.6 kernel. This core provides basic API to create/manage SPI devices
> > like the I2C core does. We need to continue providing support of SPI devices
> > and would like to maintain the SPI subtree.
> 
> > +#ifdef CONFIG_DEVFS_FS
> > +#include <linux/devfs_fs_kernel.h>
> > +#endif
> 
> devfs will be removed from mainline in a month.

True, but the #ifdef should never be there in the first place...

thanks,

greg k-h

Re: [RFC] SPI core

[Greg KH]

On Tue, May 31, 2005 at 08:09:16PM +0400, dmitry pervushin wrote:
> Hello guys,
> 
> In order to support the specific board, we have ported the generic SPI
> core to the 2.6 kernel. This core provides basic API to create/manage
> SPI devices like the I2C core does. We need to continue providing
> support of SPI devices and would like to maintain the SPI subtree. It
> would be nice if SPI core patch were applied to the vanilla kernel.
> I2C people do not like to mainain this code as well as I2C, so...

What do you mean by this?  Which i2c people?

Is this code intergrated into the driver model?
What does the /sys/ tree look like?
Why are you using a char device node?

> +/**
> + * spi_add_adapter - register a new SPI bus adapter
> + * @adap: spi_adapter structure for the registering adapter
> + *
> + * Make the adapter available for use by clients using name adap->name.
> + * The adap->adapters list is initialised by this function.
> + *
> + * Returns 0;

You have this a lot.  If the function can not fail, just make it a void
type :)

> +int spi_add_adapter(struct spi_adapter *adap)
> +{
> +	struct list_head *l;
> +
> +	INIT_LIST_HEAD(&adap->clients);
> +	down(&adapter_lock);
> +	init_MUTEX(&adap->lock);
> +	list_add(&adap->adapters, &adapter_list);
> +	up(&adapter_lock);
> +
> +	list_for_each(l, &driver_list) {

list_for_each_entry() please.

> +		struct spi_driver *drv =
> +		    list_entry(l, struct spi_driver, drivers);
> +
> +		if (drv->attach_adapter)
> +			drv->attach_adapter(adap);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * spi_del_adapter - unregister a SPI bus adapter
> + * @adap: spi_adapter structure to unregister
> + *
> + * Remove an adapter from the list of available SPI Bus adapters.
> + *
> + * Returns 0;
> + */
> +int spi_del_adapter(struct spi_adapter *adap)
> +{
> +	struct list_head *l;
> +
> +	down(&adapter_lock);
> +	list_del(&adap->adapters);
> +	up(&adapter_lock);
> +
> +	list_for_each(l, &driver_list) {
> +		struct spi_driver *drv =
> +		    list_entry(l, struct spi_driver, drivers);
> +
> +		if (drv->detach_adapter)
> +			drv->detach_adapter(adap);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * spi_get_adapter - get a reference to an adapter
> + * @id: driver id
> + *
> + * Obtain a spi_adapter structure for the specified adapter.  If the adapter
> + * is not currently load, then load it.  The adapter will be locked in core
> + * until all references are released via spi_put_adapter.
> + */
> +struct spi_adapter *spi_get_adapter(int id)
> +{
> +	struct list_head *item;
> +	struct spi_adapter *adapter;
> +
> +	down(&adapter_lock);
> +	list_for_each(item, &adapter_list) {
> +		adapter = list_entry(item, struct spi_adapter, adapters);
> +		if (id == adapter->nr && try_module_get(adapter->owner)) {
> +			up(&adapter_lock);
> +			return adapter;
> +		}
> +	}
> +	up(&adapter_lock);
> +	return NULL;
> +
> +}

Hm, that comment is not correct.  Please fix it as nothing is "loaded".

> +/**
> + * spi_put_adapter - release a reference to an adapter
> + * @adap: driver to release reference
> + *
> + * Indicate to the SPI core that you no longer require the adapter reference.
> + * The adapter module may be unloaded when there are no references to its
> + * data structure.
> + *
> + * You must not use the reference after calling this function.
> + */
> +void spi_put_adapter(struct spi_adapter *adap)
> +{
> +	if (adap && adap->owner)
> +		module_put(adap->owner);
> +}

Then why not use the traditional kref style of reference counting?  That
ensures that if you try to use the reference, bad things will happen?
Right now all you are doing is relying on module references, and you
aren't cleaning up the memory.

> +EXPORT_SYMBOL(spi_add_adapter);
> +EXPORT_SYMBOL(spi_del_adapter);
> +EXPORT_SYMBOL(spi_get_adapter);
> +EXPORT_SYMBOL(spi_put_adapter);
> +
> +EXPORT_SYMBOL(spi_add_driver);
> +EXPORT_SYMBOL(spi_del_driver);
> +EXPORT_SYMBOL(spi_get_driver);
> +EXPORT_SYMBOL(spi_put_driver);
> +
> +EXPORT_SYMBOL(spi_attach_client);
> +EXPORT_SYMBOL(spi_detach_client);
> +
> +EXPORT_SYMBOL(spi_transfer);
> +EXPORT_SYMBOL(spi_write);
> +EXPORT_SYMBOL(spi_read);

EXPORT_SYMBOL_GPL() perhaps?

> +/*  Define SPIDEV_DEBUG for debugging info  */
> +#undef SPIDEV_DEBUG

Use a Kconfig entry instead.

> +#ifdef SPIDEV_DEBUG
> +#define DBG(args...)	printk(KERN_INFO"spi-dev.o: " args)
> +#else
> +#define DBG(args...)
> +#endif

Please use dev_dbg() and friends instead of your own debugging macros.
The error log people will thank you (along with your users...)

> +#include <linux/init.h>
> +#include <asm/uaccess.h>
> +#include <linux/spi/spi.h>

Why a separate subdir for spi.h?

> +static int spidev_ioctl(struct inode *inode, struct file *file,
> +			unsigned int cmd, unsigned long arg)

Ick, please do NOT add new ioctls to the kernel.  Especially one as
complex and hard to audit as this one :(

The i2c dev interface is a mess, please don't duplicate it, there is no
need to do so.

Also, please run the code through sparse, I think it will spit out a lot
of errors here.

> +static int spidev_attach_adapter(struct spi_adapter *adap)
> +{
> +	struct spi_dev *spi_dev;
> +	int retval;
> +
> +	spi_dev = get_free_spi_dev(adap);
> +	if (IS_ERR(spi_dev))
> +		return PTR_ERR(spi_dev);
> +
> +#if defined( CONFIG_DEVFS_FS )
> +	devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor),
> +		      S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor);
> +#endif

No #if needed.  You do this a lot.


> +/*
> + * A driver is capable of handling one or more physical devices present on
> + * SPI adapters. This information is used to inform the driver of adapter
> + * events.
> + */
> +struct spi_driver {

Please use the driver model code.  That's what it is there for.

> +/*
> + * spi_adapter is the structure used to identify a physical SPI bus along
> + * with the access algorithms necessary to access it.
> + */
> +struct spi_adapter {

<snip>  Ick, don't copy the mess I did in the i2c core for i2c adapter
structures please.  It was a hack then, and I regret it still.  Please
fix it up properly.

This code is _very_ close to just a copy of the i2c core code.  Why
duplicate it and not work with the i2c people instead?

thanks,

greg k-h

Re: [RFC] SPI core

[David Brownell]

Yeah, I've been wondering why there's not an SPI core too.

Couldn't this code look a lot more like standard driver model
code?  Sure, probing and init should work a bit differently
(probably driven mostly by static "this board has this chip
at this chipselect" declarations, like platform_device) but
I never got the impression the I2C model was expected to grow
into new subsystems ...

How does this compare to the AT91 SPI and spidev interfaces?
(I'm picking on those only because I know they exist and seemed
to work nicely when I tested them a while back.  And they're
a rather direct analogue of this "core" patch.)


Quoth NZG <ngustavson at emacinc.com>
> SPI is serial-peripheral interface. A very common 3 wire bus in embedded 
> systems, especially m68k arch's. That fact that it's not there already is 
> actually a little weird IMHO.

Also on ARM ... PXA, OMAP, AT91, and others support it.

MMC cards can be accessed using SPI, as can serial flash chips
including notably DataFlash (over which JFFS2 works, though not
yet in the kernel.org MMC code).

It'd make sense to me to have for example the DataFlash driver
(in the AT91 patches) work using such a standard API.  Likewise
to have a few basic chip drivers working in the framework before
it's merged.

What other drivers do you see getting added "soon" to this
framework?  That is, SPI drivers that's currently in use in 2.6
and so should arguably switch to shared infrastructure to
help get more review and code reuse.

- Dave

Re: [RFC] SPI core

[Mark M. Hoffman]

Hi Greg, Dmitry:

> On Tue, May 31, 2005 at 08:09:16PM +0400, dmitry pervushin wrote:
> > In order to support the specific board, we have ported the generic SPI
> > core to the 2.6 kernel. This core provides basic API to create/manage
> > SPI devices like the I2C core does. We need to continue providing
> > support of SPI devices and would like to maintain the SPI subtree. It
> > would be nice if SPI core patch were applied to the vanilla kernel.
> > I2C people do not like to mainain this code as well as I2C, so...

* Greg KH <greg@kroah.com> [2005-05-31 16:32:15 -0700]:
> What do you mean by this?  Which i2c people?

(...)

* Greg KH <greg@kroah.com> [2005-05-31 16:32:15 -0700]:
> This code is _very_ close to just a copy of the i2c core code.  Why
> duplicate it and not work with the i2c people instead?

It was discussed briefly on the lm-sensors mailing list [1].  I didn't 
reply at the time, but I do agree that SPI and I2C/SMBus are different
enough to warrant independent subsystems.

[1] http://lists.lm-sensors.org/pipermail/lm-sensors/2005-May/012385.html

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [RFC] SPI core

[Greg KH]

On Thu, Jun 02, 2005 at 12:06:55AM -0400, Mark M. Hoffman wrote:
> * Greg KH <greg@kroah.com> [2005-05-31 16:32:15 -0700]:
> > This code is _very_ close to just a copy of the i2c core code.  Why
> > duplicate it and not work with the i2c people instead?
> 
> It was discussed briefly on the lm-sensors mailing list [1].  I didn't 
> reply at the time, but I do agree that SPI and I2C/SMBus are different
> enough to warrant independent subsystems.

Independant is fine.  But direct copies, including making the same
mistakes (i2c dev interface, i2c driver model mess) isn't :)

thanks,

greg k-h

Re: [RFC] SPI core

[dmitry pervushin]

On Tue, 2005-05-31 at 16:32 -0700, Greg KH wrote:

Thanks for you comments; my answers follow. This patch will be reworked
in short time.

> Is this code intergrated into the driver model?
It will be.
> What does the /sys/ tree look like?
> Why are you using a char device node?
Because it's not a block device :) I assume that's common practice to
access low-level devices, isn't it ?
> 
> > +/**
> > + * spi_add_adapter - register a new SPI bus adapter
> > + * @adap: spi_adapter structure for the registering adapter
> > + *
> > + * Make the adapter available for use by clients using name adap->name.
> > + * The adap->adapters list is initialised by this function.
> > + *
> > + * Returns 0;
> 
> You have this a lot.  If the function can not fail, just make it a void
> type :)
I'd like to keep this of type int because of possible future
enhancements
> > +	list_for_each(l, &driver_list) {
> 
> list_for_each_entry() please.
Looks reasonable, thank you 
> 
> > +/**
> > + * spi_get_adapter - get a reference to an adapter
> > + * @id: driver id
> > + *
> > + * Obtain a spi_adapter structure for the specified adapter.  If the adapter
> > + * is not currently load, then load it.  The adapter will be locked in core
> > + * until all references are released via spi_put_adapter.
> > + */
> 
> Hm, that comment is not correct.  Please fix it as nothing is "loaded".
OK
> > +void spi_put_adapter(struct spi_adapter *adap)
> > +{
> > +	if (adap && adap->owner)
> > +		module_put(adap->owner);
> > +}
> 
> Then why not use the traditional kref style of reference counting?  That
> ensures that if you try to use the reference, bad things will happen?
> Right now all you are doing is relying on module references, and you
> aren't cleaning up the memory.
I'll consider this during rework
> > +EXPORT_SYMBOL(spi_transfer);
> > +EXPORT_SYMBOL(spi_write);
> > +EXPORT_SYMBOL(spi_read);
> 
> EXPORT_SYMBOL_GPL() perhaps?
Yup.
> Please use dev_dbg() and friends instead of your own debugging macros.
> The error log people will thank you (along with your users...)
I'd rather use pr_debug
> > +#include <linux/spi/spi.h>
> 
> Why a separate subdir for spi.h?
Due to historical reasons :) And functional drivers can pput their
public headers to separate directory.
> 
> > +static int spidev_ioctl(struct inode *inode, struct file *file,
> > +			unsigned int cmd, unsigned long arg)
This function will be removed, because its functionality is duplicated by spidev_read/spidev_write.


> > +static int spidev_attach_adapter(struct spi_adapter *adap)
> > +{
> > +	struct spi_dev *spi_dev;
> > +	int retval;
> > +
> > +	spi_dev = get_free_spi_dev(adap);
> > +	if (IS_ERR(spi_dev))
> > +		return PTR_ERR(spi_dev);
> > +
> > +#if defined( CONFIG_DEVFS_FS )
> > +	devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor),
> > +		      S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor);
> > +#endif
> 
> No #if needed.  You do this a lot.

As devfs is going to become obsolete, maybe just drop all the
devfs-related code?

> > +/*
> > + * spi_adapter is the structure used to identify a physical SPI bus along
> > + * with the access algorithms necessary to access it.
> > + */
> > +struct spi_adapter {
> 
> <snip>  Ick, don't copy the mess I did in the i2c core for i2c adapter
> structures please.  It was a hack then, and I regret it still.  Please
> fix it up properly.
> 
> This code is _very_ close to just a copy of the i2c core code.  Why
> duplicate it and not work with the i2c people instead?

Well, those two are both simple serial interfaces, so their similarity
is just reflected in driver structures.

Re: [RFC] SPI core

[Rui Sousa]

Hi Greg,

On Wed, 2005-06-01 at 21:51 -0700, Greg KH wrote:
> On Thu, Jun 02, 2005 at 12:06:55AM -0400, Mark M. Hoffman wrote:
> > * Greg KH <greg@kroah.com> [2005-05-31 16:32:15 -0700]:
> > > This code is _very_ close to just a copy of the i2c core code.  Why
> > > duplicate it and not work with the i2c people instead?
> > 
> > It was discussed briefly on the lm-sensors mailing list [1].  I didn't 
> > reply at the time, but I do agree that SPI and I2C/SMBus are different
> > enough to warrant independent subsystems.
> 
> Independant is fine.  But direct copies, including making the same
> mistakes (i2c dev interface, i2c driver model mess) isn't :)

I have also worked on a(nother) SPI layer implementation. Like Dmitry, I
ended up following closely the i2c implementation, so, I'm curious to
know more details on what you call "i2c driver model mess".

> thanks,
> 
> greg k-h

Thanks,

Rui

Re: [RFC] SPI core

[Greg KH]

On Thu, Jun 02, 2005 at 03:02:35PM +0200, Rui Sousa wrote:
> Hi Greg,
> 
> On Wed, 2005-06-01 at 21:51 -0700, Greg KH wrote:
> > On Thu, Jun 02, 2005 at 12:06:55AM -0400, Mark M. Hoffman wrote:
> > > * Greg KH <greg@kroah.com> [2005-05-31 16:32:15 -0700]:
> > > > This code is _very_ close to just a copy of the i2c core code.  Why
> > > > duplicate it and not work with the i2c people instead?
> > > 
> > > It was discussed briefly on the lm-sensors mailing list [1].  I didn't 
> > > reply at the time, but I do agree that SPI and I2C/SMBus are different
> > > enough to warrant independent subsystems.
> > 
> > Independant is fine.  But direct copies, including making the same
> > mistakes (i2c dev interface, i2c driver model mess) isn't :)
> 
> I have also worked on a(nother) SPI layer implementation. Like Dmitry, I
> ended up following closely the i2c implementation, so, I'm curious to
> know more details on what you call "i2c driver model mess".

The fact that the i2c drivers are not really true "drivers" in the
driver model.  We bind them by hand to the device and then register the
device with the core.  That isn't a nice thing to do...

Also the sysfs representation of the sensor stuff is tied to the i2c
sysfs code very tightly, which isn't good for other types of sensors.
It should be in the class portion of sysfs, and the recent hwmon patches
are moving it in that direction.

thanks,

greg k-h

Re: [RFC] SPI core

[Hinko Kocevar]

Greg KH wrote:

> 
> The fact that the i2c drivers are not really true "drivers" in the
> driver model.  We bind them by hand to the device and then register the
> device with the core.  That isn't a nice thing to do...

And introduces alot of code do simple stuff that SPI is supposed to do. I also 
ported i2c-core,i2c-dev, i2c-bit-algo and parport bus to work with SPI device. 
Resulting SPI code base was huge and I was confused in the begining why, and 
later wondered if there is need for such a design.

I dumped evrything and created three functions : spi_access, spi_transfer and 
spi_release. First and last functions only assert/deassert CS line, 
respectevely. Spi_transfer is the core function and is more-or-less different 
on every CPU (if not using bit-banging). As every CPU has different approach in 
handling SPI interface is almost neccesary to write CPU dependent SPI part for 
each CPU out there (at least transfer function). Also you can use CPUs 
synchronous serial interface (if one supports it) or just use bit-bang algo to 
get bits in and out.

I have two devices on SPI and I drive them both by bit-banging bits in and out. 
While I was using I2C-like-SPI model I wanted to make it base for all other SPI 
devices my board would/could hold. Sad thing is that every manufacturer and/or 
device (let it be serial flash, audio codec, A/D converter, ...) has its own 
concept of accessing and transfering data to and from the SPI device. I 
experienced this a short while ago while trying to make tsc2301 audio codec and 
datakey spi serial flash to use common SPI code. I ended up duplicating the 
three aforementioned functions in the each driver and still SPI code is ~10-15 
times smaller than initial I2C-to-SPI port I did.

I would also like to see SPI core in linux driver model, but nothing like I2C 
stuff. SPI is far to simple (and yet so diverse) that much more simple concept 
could be used.

just my .2 €

regards,
hinko k

-- 
..because under Linux "if something is possible in principle,
then it is already implemented or somebody is working on it".

					--LKI

Re: [RFC] SPI core

[Pekka Enberg]

Hi,

Here are some coding style comments. Sorry for any duplicates, I read
other comments only after I reviewed the patch.

                                   Pekka

On 5/31/05, dmitry pervushin <dpervushin@ru.mvista.com> wrote:
> diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-core.c linux/drivers/spi/spi-core.c
> --- linux-2.6.12-rc4/drivers/spi/spi-core.c	1970-01-01 03:00:00.000000000 +0300
> +++ linux/drivers/spi/spi-core.c	2005-05-31 19:59:12.000000000 +0400
> +int spi_add_adapter(struct spi_adapter *adap)
> +{
> +	struct list_head *l;
> +
> +	INIT_LIST_HEAD(&adap->clients);
> +	down(&adapter_lock);
> +	init_MUTEX(&adap->lock);

adapter_lock protects adapter_list, not adap. Therefore, please move
adap->lock initialization outside the critical region.

> +struct spi_adapter *spi_get_adapter(int id)
> +{
> +	struct list_head *item;
> +	struct spi_adapter *adapter;
> +
> +	down(&adapter_lock);
> +	list_for_each(item, &adapter_list) {
> +		adapter = list_entry(item, struct spi_adapter, adapters);
> +		if (id == adapter->nr && try_module_get(adapter->owner)) {
> +			up(&adapter_lock);
> +			return adapter;

One exit path please. You can use goto here.

> +
> +EXPORT_SYMBOL(spi_add_adapter);
> +EXPORT_SYMBOL(spi_del_adapter);
> +EXPORT_SYMBOL(spi_get_adapter);
> +EXPORT_SYMBOL(spi_put_adapter);
> +
> +EXPORT_SYMBOL(spi_add_driver);
> +EXPORT_SYMBOL(spi_del_driver);
> +EXPORT_SYMBOL(spi_get_driver);
> +EXPORT_SYMBOL(spi_put_driver);
> +
> +EXPORT_SYMBOL(spi_attach_client);
> +EXPORT_SYMBOL(spi_detach_client);
> +
> +EXPORT_SYMBOL(spi_transfer);
> +EXPORT_SYMBOL(spi_write);
> +EXPORT_SYMBOL(spi_read);

Preferred location for EXPORT_SYMBOLs is immediately after the function
definition.

> diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-dev.c linux/drivers/spi/spi-dev.c
> --- linux-2.6.12-rc4/drivers/spi/spi-dev.c	1970-01-01 03:00:00.000000000 +0300
> +++ linux/drivers/spi/spi-dev.c	2005-05-31 19:59:18.000000000 +0400
> @@ -0,0 +1,514 @@
> +#include <linux/init.h>
> +#include <asm/uaccess.h>
> +#include <linux/spi/spi.h>
> +
> +#define SPI_TRANSFER_MAX	65535
> +#define SPI_ADAP_MAX		32

Unused define.

> +static struct file_operations spidev_fops = {
> +      owner:THIS_MODULE,
> +      llseek:no_llseek,
> +      read:spidev_read,
> +      write:spidev_write,
> +      open:spidev_open,
> +      release:spidev_release,
> +      ioctl:spidev_ioctl,

Use C99 struct initializers.

> +};
> +
> +static struct spi_driver spidev_driver = {
> +      name:"spi",
> +      attach_adapter:spidev_attach_adapter,
> +      detach_adapter:spidev_detach_adapter,
> +      owner:THIS_MODULE,

Ditto.

> +};
> +
> +static struct spi_client spidev_client_template = {
> +      driver:&spidev_driver

Same here.

> +static struct spi_dev *get_free_spi_dev(struct spi_adapter *adap)
> +{
> +	struct spi_dev *spi_dev;
> +
> +	spi_dev = kmalloc(sizeof(*spi_dev), GFP_KERNEL);
> +	if (!spi_dev)
> +		return ERR_PTR(-ENOMEM);
> +	memset(spi_dev, 0x00, sizeof(*spi_dev));

See below.

> +
> +	spin_lock(&spi_dev_array_lock);
> +	if (spi_dev_array[adap->nr]) {
> +		spin_unlock(&spi_dev_array_lock);

One spin_unlock, please. Use gotos here.

> +		dev_err(&adap->dev,
> +			"spi-dev already has a device assigned to this adapter\n");
> +		goto error;
> +	}
> +	spi_dev->minor = adap->nr;

Instead of memset() followed by assignment, you can use C99 struct
initializers like this:

	*spi_dev = (struct spi_dev) { .minor = adap->nr; };

> +	spi_dev_array[adap->nr] = spi_dev;
> +	spin_unlock(&spi_dev_array_lock);
> +	return spi_dev;
> +      error:

The placement of this label is strange.

> +static int spidev_ioctl(struct inode *inode, struct file *file,
> +			unsigned int cmd, unsigned long arg)
> +{
> +	struct spi_client *client = (struct spi_client *)file->private_data;

Please remove the redundant cast.

> +	struct spi_rdwr_ioctl_data rdwr_arg;
> +	struct spi_msg *rdwr_pa;
> +	int res, i;
> +	unsigned long (*cpy_to_user) (void *to_user, const void *from,
> +				      unsigned long len);
> +	unsigned long (*cpy_from_user) (void *to, const void *from_user,
> +					unsigned long len);
> +	void *(*alloc) (size_t, int);
> +	void (*free) (const void *);

No type declarations within function body, please.

> +
> +	DBG("spi-%d ioctl, cmd: 0x%x, arg: %lx.\n",
> +	    MINOR(inode->i_rdev), cmd, arg);
> +
> +	cpy_to_user =
> +	    client->adapter->copy_to_user ? client->adapter->
> +	    copy_to_user : copy_to_user;
> +	cpy_from_user =
> +	    client->adapter->copy_from_user ? client->adapter->
> +	    copy_from_user : copy_from_user;
> +	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
> +	free = client->adapter->free ? client->adapter->free : kfree;
> +
> +	switch (cmd) {
> +	case SPI_RDWR:
> +		if (copy_from_user(&rdwr_arg,
> +				   (struct spi_rdwr_ioctl_data *)arg,
> +				   sizeof(rdwr_arg)))
> +			return -EFAULT;
> +
> +		rdwr_pa = (struct spi_msg *)
> +		    kmalloc(rdwr_arg.nmsgs * sizeof(struct spi_msg),
> +			    GFP_KERNEL);

Redundant cast.

> +static int spidev_attach_adapter(struct spi_adapter *adap)
> +{
> +	struct spi_dev *spi_dev;
> +	int retval;
> +
> +	spi_dev = get_free_spi_dev(adap);
> +	if (IS_ERR(spi_dev))
> +		return PTR_ERR(spi_dev);
> +
> +#if defined( CONFIG_DEVFS_FS )
> +	devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor),
> +		      S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor);
> +#endif
> +	dev_dbg(&adap->dev, "Registered as minor %d\n", spi_dev->minor);
> +
> +	/* register this spi device with the driver core */
> +	spi_dev->adap = adap;
> +	if (adap->dev.parent == &platform_bus)
> +		spi_dev->class_dev.dev = &adap->dev;
> +	else
> +		spi_dev->class_dev.dev = adap->dev.parent;
> +	spi_dev->class_dev.class = &spi_dev_class;
> +	snprintf(spi_dev->class_dev.class_id, BUS_ID_SIZE, "spi-%d",
> +		 spi_dev->minor);
> +	retval = class_device_register(&spi_dev->class_dev);
> +	if (retval)
> +		goto error;
> +	class_device_create_file(&spi_dev->class_dev, &class_device_attr_dev);
> +	class_device_create_file(&spi_dev->class_dev, &class_device_attr_name);
> +	return 0;
> +      error:

The placement of this label is strange.

> +static ssize_t spidev_read(struct file *file, char *buf, size_t count,
> +			   loff_t * offset)
> +{
> +	char *tmp;
> +	int ret;
> +#ifdef SPIDEV_DEBUG
> +	struct inode *inode = file->f_dentry->d_inode;
> +#endif
> +
> +	struct spi_client *client = (struct spi_client *)file->private_data;
> +	unsigned long (*cpy_to_user) (void *to_user, const void *from,
> +				      unsigned long len);
> +	void *(*alloc) (size_t, int);
> +	void (*free) (const void *);

Please remove the duplicate function pointer type declarations.

> +	if (count > SPI_TRANSFER_MAX)
> +		count = SPI_TRANSFER_MAX;
> +
> +	cpy_to_user =
> +	    client->adapter->copy_to_user ? client->adapter->
> +	    copy_to_user : copy_to_user;
> +	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
> +	free = client->adapter->free ? client->adapter->free : kfree;

More duplicate code.

> +
> +	/* copy user space data to kernel space. */
> +	tmp = alloc(count, GFP_KERNEL);
> +	if (tmp == NULL)
> +		return -ENOMEM;
> +
> +	DBG("spi-%d reading %d bytes.\n", MINOR(inode->i_rdev), count);
> +
> +	ret = spi_read(client, 0, tmp, count);
> +	if (ret >= 0)
> +		ret = cpy_to_user(buf, tmp, count) ? -EFAULT : ret;
> +	free(tmp);
> +	return ret;
> +}
> +
> +static ssize_t spidev_write(struct file *file, const char *buf, size_t count,
> +			    loff_t * offset)
> +{
> +	int ret;
> +	char *tmp;
> +	struct spi_client *client = (struct spi_client *)file->private_data;
> +#ifdef SPIDEV_DEBUG
> +	struct inode *inode = file->f_dentry->d_inode;
> +#endif
> +	unsigned long (*cpy_from_user) (void *to, const void *from_user,
> +					unsigned long len);
> +	void *(*alloc) (size_t, int);
> +	void (*free) (const void *);

Aiie! Duplicates.

> +
> +	if (count > SPI_TRANSFER_MAX)
> +		count = SPI_TRANSFER_MAX;
> +
> +	cpy_from_user =
> +	    client->adapter->copy_from_user ? client->adapter->
> +	    copy_from_user : copy_from_user;
> +	alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc;
> +	free = client->adapter->free ? client->adapter->free : kfree;

More of the same.

> +int spidev_open(struct inode *inode, struct file *file)
> +{
> +	unsigned int minor = iminor(inode);
> +	struct spi_client *client;
> +	struct spi_adapter *adap;
> +	struct spi_dev *spi_dev;
> +
> +	spi_dev = spi_dev_get_by_minor(minor);
> +	if (!spi_dev)
> +		return -ENODEV;
> +
> +	adap = spi_get_adapter(spi_dev->adap->nr);
> +	if (!adap)
> +		return -ENODEV;
> +
> +	client = kmalloc(sizeof(*client), GFP_KERNEL);
> +	if (!client) {
> +		spi_put_adapter(adap);
> +		return -ENOMEM;
> +	}
> +	memcpy(client, &spidev_client_template, sizeof(*client));
> +
> +	/* registered with adapter, passed as client to user */
> +	client->adapter = adap;

Please use C99 struct initializer here.

> +static int __init spi_dev_init(void)
> +{
> +	int res;
> +
> +	printk(KERN_INFO "spi /dev entries driver\n");
> +
> +	res = register_chrdev(SPI_MAJOR, "spi", &spidev_fops);
> +	if (res)
> +		goto out;
> +
> +	res = class_register(&spi_dev_class);
> +	if (res)
> +		goto out_unreg_chrdev;
> +
> +	res = spi_add_driver(&spidev_driver);
> +	if (res)
> +		goto out_unreg_class;
> +
> +#ifdef CONFIG_DEVFS_FS
> +	devfs_mk_dir("spi");
> +#endif
> +	return 0;
> +
> +      out_unreg_class:

The label indentation is strange.

> +	class_unregister(&spi_dev_class);
> +      out_unreg_chrdev:

Ditto.

> +	unregister_chrdev(SPI_MAJOR, "spi");
> +      out:

Here as well.

> diff -uNr -X dontdiff linux-2.6.12-rc4/include/linux/spi/spi.h linux/include/linux/spi/spi.h
> --- linux-2.6.12-rc4/include/linux/spi/spi.h	1970-01-01 03:00:00.000000000 +0300
> +++ linux/include/linux/spi/spi.h	2005-05-31 19:59:26.000000000 +0400
> @@ -0,0 +1,265 @@
> +/*
> + *  linux/include/linux/spi/spi.h
> + *
> + *  Copyright (C) 2001 Russell King, All Rights Reserved.
> + *  Copyright (C) 2002 Compaq Computer Corporation, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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
> +
> +struct spi_msg {
> +	unsigned char addr;	/* slave address        */
> +	unsigned char flags;
> +#define SPI_M_RD		0x01
> +#define SPI_M_NOADDR	0x02

Please use enums instead.

> +	unsigned short len;	/* msg length           */
> +	unsigned char *buf;	/* pointer to msg data  */
> +};
> +
> +/*  TODO: should be in a separate header file?  */
> +
> +/*  Commands for the ioctl call */
> +#define SPI_RDWR	0x0801	/*  Read/write transfer in a single transaction */
> +#define SPI_CLK_RATE	0x0802	/*  Sets SPI clock divisor (int type)           */
> +#define SPI_SDMA	0x0803	/*  Turns on/off SDMA usage (int type).         */

Same here.

> +				 /*  Pass non-zero value to turn SDMA on         */
> +struct spi_rdwr_ioctl_data {
> +	struct spi_msg *msgs;	/* pointers to spi_msgs */
> +	int nmsgs;		/* number of spi_msgs */
> +};
> +/*------------^^^^^^^^^^^^^^^--------------*/

Please drop the above ASCII art.

> +
> +#ifdef __KERNEL__
> +
> +#include <linux/types.h>
> +#include <linux/list.h>
> +
> +#define SPI_ADAP_MAX	32

Unused constant.

Re: [RFC] SPI core

[Lee Revell]

On Thu, 2005-06-09 at 12:39 +0200, Hinko Kocevar wrote:
> I would also like to see SPI core in linux driver model, but nothing
> like I2C 
> stuff. SPI is far to simple (and yet so diverse) that much more simple
> concept 
> could be used. 

Did you look at sound/i2c/i2c.c?  ALSA has its own i2c implementation,
only 333 lines, due to the kernel i2c core being overly complex for
ALSA's needs.

"Although there is a standard i2c layer on Linux, ALSA has its own i2c
codes for some cards, because the soundcard needs only a simple
operation and the standard i2c API is too complicated for such a
purpose." 

(from http://www.alsa-project.org/~iwai/writing-an-alsa-driver/x77.htm)

Personally, I would start with this, then add any needed functionality.

Lee

Re: [RFC] SPI core

[Alexey Dobriyan]

On Thursday 09 June 2005 16:33, Pekka Enberg wrote:
> On 5/31/05, dmitry pervushin <dpervushin@ru.mvista.com> wrote:
> > +EXPORT_SYMBOL(spi_add_adapter);
> > +EXPORT_SYMBOL(spi_del_adapter);
> > +EXPORT_SYMBOL(spi_get_adapter);
> > +EXPORT_SYMBOL(spi_put_adapter);
> > +
> > +EXPORT_SYMBOL(spi_add_driver);
> > +EXPORT_SYMBOL(spi_del_driver);
> > +EXPORT_SYMBOL(spi_get_driver);
> > +EXPORT_SYMBOL(spi_put_driver);
> > +
> > +EXPORT_SYMBOL(spi_attach_client);
> > +EXPORT_SYMBOL(spi_detach_client);
> > +
> > +EXPORT_SYMBOL(spi_transfer);
> > +EXPORT_SYMBOL(spi_write);
> > +EXPORT_SYMBOL(spi_read);
> 
> Preferred location for EXPORT_SYMBOLs is immediately after the function
> definition.

New files can choose any style.

Re: [RFC] SPI core

[Pekka Enberg]

Hi,

On Thursday 09 June 2005 16:33, Pekka Enberg wrote:
> > Preferred location for EXPORT_SYMBOLs is immediately after the function
> > definition.

On 6/9/05, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> New files can choose any style.

Sure they can but hopefully they choose a style that make sense.
Putting EXPORT_SYMBOLs immediately after function definition makes
most sense to me. You can immediately see if a function is exported or
not plus you can get listing of all symbols with grep. So there's
really no good reason why you should put them at the end of a file
(and one good reason to put them after definition).

You are right, though, that authors and maintainers decide what style
they use. I only provide suggestions and hope for the best ;).

                                       Pekka