[PATCH v2] Add GPIO-based MMC/SD driver

[PATCH v2] Add GPIO-based MMC/SD driver

[Michael Buesch]

This driver hooks up the mmc_spi and spi_gpio modules so that
MMC/SD cards can be used on a GPIO based bus by bitbanging
the SPI protocol in software.

This driver provides a sysfs interface to dynamically create
and destroy GPIO-based MMC/SD card interfaces. It also provides
a platform device interface API.
See Documentation/gpiommc.txt for details.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

This driver is used in OpenWrt since quite some time, so please
consider for inclusion in mainline.

Index: linux-next/drivers/mmc/host/gpiommc.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-next/drivers/mmc/host/gpiommc.c	2008-07-18 21:54:05.000000000 +0200
@@ -0,0 +1,328 @@
+/*
+ * Driver an MMC/SD card on a bitbanging GPIO SPI bus.
+ * This module hooks up the mmc_spi and spi_gpio modules and also
+ * provides a sysfs interface.
+ *
+ * Copyright 2008 Michael Buesch <mb@bu3sch.de>
+ *
+ * Licensed under the GNU/GPL. See COPYING for details.
+ */
+
+#include <linux/mmc/gpiommc.h>
+#include <linux/platform_device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi_gpio.h>
+
+
+#define PFX				"gpio-mmc: "
+#define GPIOMMC_MAX_NAMELEN_STR		__stringify(GPIOMMC_MAX_NAMELEN)
+
+struct gpiommc_device {
+	struct platform_device *pdev;
+	struct platform_device *spi_pdev;
+	struct spi_board_info boardinfo;
+};
+
+
+MODULE_DESCRIPTION("GPIO based MMC driver");
+MODULE_AUTHOR("Michael Buesch");
+MODULE_LICENSE("GPL");
+
+
+static int gpiommc_boardinfo_setup(struct spi_board_info *bi,
+				   struct spi_master *master,
+				   void *data)
+{
+	struct gpiommc_device *d = data;
+	struct gpiommc_platform_data *pdata = d->pdev->dev.platform_data;
+
+	/* Bind the SPI master to the MMC-SPI host driver. */
+	strlcpy(bi->modalias, "mmc_spi", sizeof(bi->modalias));
+
+	bi->max_speed_hz = pdata->max_bus_speed;
+	bi->bus_num = master->bus_num;
+	bi->mode = pdata->mode;
+
+	return 0;
+}
+
+static int gpiommc_probe(struct platform_device *pdev)
+{
+	struct gpiommc_platform_data *mmc_pdata = pdev->dev.platform_data;
+	struct spi_gpio_platform_data spi_pdata;
+	struct gpiommc_device *d;
+	int err;
+
+	err = -ENXIO;
+	if (!mmc_pdata)
+		goto error;
+
+	/* Allocate the GPIO-MMC device */
+	err = -ENOMEM;
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		goto error;
+	d->pdev = pdev;
+
+	/* Create the SPI-GPIO device */
+	d->spi_pdev = platform_device_alloc(SPI_GPIO_PLATDEV_NAME,
+					    spi_gpio_next_id());
+	if (!d->spi_pdev)
+		goto err_free_d;
+
+	memset(&spi_pdata, 0, sizeof(spi_pdata));
+	spi_pdata.pin_clk = mmc_pdata->pins.gpio_clk;
+	spi_pdata.pin_miso = mmc_pdata->pins.gpio_do;
+	spi_pdata.pin_mosi = mmc_pdata->pins.gpio_di;
+	spi_pdata.pin_cs = mmc_pdata->pins.gpio_cs;
+	spi_pdata.cs_activelow = mmc_pdata->pins.cs_activelow;
+	spi_pdata.no_spi_delay = mmc_pdata->no_spi_delay;
+	spi_pdata.boardinfo_setup = gpiommc_boardinfo_setup;
+	spi_pdata.boardinfo_setup_data = d;
+
+	err = platform_device_add_data(d->spi_pdev, &spi_pdata,
+				       sizeof(spi_pdata));
+	if (err)
+		goto err_free_pdev;
+	err = platform_device_add(d->spi_pdev);
+	if (err)
+		goto err_free_pdata;
+	platform_set_drvdata(pdev, d);
+
+	printk(KERN_INFO PFX "MMC-Card \"%s\" "
+	       "attached to GPIO pins di=%u, do=%u, clk=%u, cs=%u\n",
+	       mmc_pdata->name, mmc_pdata->pins.gpio_di,
+	       mmc_pdata->pins.gpio_do,
+	       mmc_pdata->pins.gpio_clk,
+	       mmc_pdata->pins.gpio_cs);
+
+	return 0;
+
+err_free_pdata:
+	kfree(d->spi_pdev->dev.platform_data);
+	d->spi_pdev->dev.platform_data = NULL;
+err_free_pdev:
+	platform_device_put(d->spi_pdev);
+err_free_d:
+	kfree(d);
+error:
+	return err;
+}
+
+static int gpiommc_remove(struct platform_device *pdev)
+{
+	struct gpiommc_device *d = platform_get_drvdata(pdev);
+	struct gpiommc_platform_data *pdata = d->pdev->dev.platform_data;
+
+	platform_device_unregister(d->spi_pdev);
+	printk(KERN_INFO PFX "GPIO based MMC-Card \"%s\" removed\n", pdata->name);
+	platform_device_put(d->spi_pdev);
+
+	return 0;
+}
+
+/* Wrapper for the platform data with context data for the sysfs interface. */
+struct gpiommc_sysfs_platform_data {
+	struct gpiommc_platform_data p; /* Keep as first element */
+
+	/* The platform device that we allocated. */
+	struct platform_device *pdev;
+	/* gpiommc_sysfs_list */
+	struct list_head list;
+};
+
+static LIST_HEAD(gpiommc_sysfs_list);
+static DEFINE_MUTEX(gpiommc_sysfs_mutex);
+
+static struct gpiommc_sysfs_platform_data *gpiommc_sysfs_find_dev(const char *name)
+{
+	struct gpiommc_sysfs_platform_data *pdata;
+
+	list_for_each_entry(pdata, &gpiommc_sysfs_list, list) {
+		if (strcmp(pdata->p.name, name) == 0)
+			return pdata;
+	}
+
+	return NULL;
+}
+
+static ssize_t gpiommc_add_store(struct device_driver *drv,
+				 const char *buf, size_t count)
+{
+	int res, err;
+	struct gpiommc_sysfs_platform_data pdata_local, *pdata;
+	struct platform_device *pdev;
+	unsigned int no_spi_delay = 0, mode = 0, csactivelow = 0;
+
+	mutex_lock(&gpiommc_sysfs_mutex);
+
+	pdata = &pdata_local;
+	memset(pdata, 0, sizeof(*pdata));
+
+	err = -EINVAL;
+	res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u %u %u %u %u %u %u %u",
+		     pdata->p.name,
+		     &pdata->p.pins.gpio_di,
+		     &pdata->p.pins.gpio_do,
+		     &pdata->p.pins.gpio_clk,
+		     &pdata->p.pins.gpio_cs,
+		     &mode,
+		     &pdata->p.max_bus_speed,
+		     &no_spi_delay,
+		     &csactivelow);
+	pdata->p.mode = mode;
+	pdata->p.no_spi_delay = !!no_spi_delay;
+	pdata->p.pins.cs_activelow = !!csactivelow;
+	if (res < 9)
+		pdata->p.pins.cs_activelow = 1; /* Default: CS = activelow */
+	if (res < 8)
+		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
+	if (res < 7)
+		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */
+	if (res < 6)
+		pdata->p.mode = 0; /* Default: SPI mode 0 */
+	if (res < 5 || res > 9)
+		goto out; /* First 5 args are mandatory. */
+
+	/* Convert mode so that the SPI subsystem does understand it. */
+	switch (pdata->p.mode) {
+	case 0:
+		pdata->p.mode = SPI_MODE_0;
+		break;
+	case 1:
+		pdata->p.mode = SPI_MODE_1;
+		break;
+	case 2:
+		pdata->p.mode = SPI_MODE_2;
+		break;
+	case 3:
+		pdata->p.mode = SPI_MODE_3;
+		break;
+	default:
+		goto out; /* Invalid mode */
+	}
+
+	err = -EEXIST;
+	if (gpiommc_sysfs_find_dev(pdata->p.name))
+		goto out;
+
+	err = -ENOMEM;
+	pdev = platform_device_alloc(GPIOMMC_PLATDEV_NAME, gpiommc_next_id());
+	if (!pdev)
+		goto out;
+
+	err = platform_device_add_data(pdev, pdata, sizeof(*pdata));
+	if (err)
+		goto err_free_pdev;
+	pdata = pdev->dev.platform_data;
+
+	err = platform_device_add(pdev);
+	if (err)
+		goto err_free_pdev;
+
+	pdata->pdev = pdev;
+	INIT_LIST_HEAD(&pdata->list);
+	list_add(&pdata->list, &gpiommc_sysfs_list);
+
+	err = 0;
+out:
+	mutex_unlock(&gpiommc_sysfs_mutex);
+
+	return err ? err : count;
+
+err_free_pdev:
+	platform_device_put(pdev);
+	goto out;
+}
+
+static ssize_t gpiommc_remove_store(struct device_driver *drv,
+				    const char *buf, size_t count)
+{
+	struct gpiommc_sysfs_platform_data *pdata;
+	int err;
+
+	mutex_lock(&gpiommc_sysfs_mutex);
+
+	err = -ENODEV;
+	pdata = gpiommc_sysfs_find_dev(buf);
+	if (!pdata)
+		goto out;
+
+	list_del(&pdata->list);
+	platform_device_unregister(pdata->pdev);
+
+out:
+	mutex_unlock(&gpiommc_sysfs_mutex);
+
+	return err ? err : count;
+}
+
+static DRIVER_ATTR(add, 0200,
+		   NULL, gpiommc_add_store);
+static DRIVER_ATTR(remove, 0200,
+		   NULL, gpiommc_remove_store);
+
+static struct platform_driver gpiommc_plat_driver = {
+	.probe	= gpiommc_probe,
+	.remove	= gpiommc_remove,
+	.driver	= {
+		.name	= GPIOMMC_PLATDEV_NAME,
+		.owner	= THIS_MODULE,
+	},
+};
+
+int gpiommc_next_id(void)
+{
+	static atomic_t counter = ATOMIC_INIT(-1);
+
+	return atomic_inc_return(&counter);
+}
+EXPORT_SYMBOL(gpiommc_next_id);
+
+static int __init gpiommc_modinit(void)
+{
+	int err;
+
+	err = platform_driver_register(&gpiommc_plat_driver);
+	if (err)
+		return err;
+	err = driver_create_file(&gpiommc_plat_driver.driver,
+				 &driver_attr_add);
+	if (err)
+		goto err_drv_unreg;
+	err = driver_create_file(&gpiommc_plat_driver.driver,
+				 &driver_attr_remove);
+	if (err)
+		goto err_remove_add;
+
+	return 0;
+
+err_remove_add:
+	driver_remove_file(&gpiommc_plat_driver.driver,
+			   &driver_attr_add);
+err_drv_unreg:
+	platform_driver_unregister(&gpiommc_plat_driver);
+	return err;
+}
+module_init(gpiommc_modinit);
+
+static void __exit gpiommc_modexit(void)
+{
+	struct gpiommc_sysfs_platform_data *pdata, *pdata_tmp;
+
+	driver_remove_file(&gpiommc_plat_driver.driver,
+			   &driver_attr_remove);
+	driver_remove_file(&gpiommc_plat_driver.driver,
+			   &driver_attr_add);
+
+	mutex_lock(&gpiommc_sysfs_mutex);
+	list_for_each_entry_safe(pdata, pdata_tmp, &gpiommc_sysfs_list, list) {
+		list_del(&pdata->list);
+		platform_device_unregister(pdata->pdev);
+	}
+	mutex_unlock(&gpiommc_sysfs_mutex);
+
+	platform_driver_unregister(&gpiommc_plat_driver);
+}
+module_exit(gpiommc_modexit);
Index: linux-next/drivers/mmc/host/Kconfig
===================================================================
--- linux-next.orig/drivers/mmc/host/Kconfig	2008-07-18 21:52:46.000000000 +0200
+++ linux-next/drivers/mmc/host/Kconfig	2008-07-18 21:57:08.000000000 +0200
@@ -164,3 +164,23 @@ config MMC_S3C
 
 	  If unsure, say N.
 
+config GPIOMMC
+	tristate "MMC/SD over GPIO-based SPI"
+	depends on MMC && MMC_SPI && SPI_GPIO
+	help
+	  This driver hooks up the mmc_spi and spi_gpio modules so that
+	  MMC/SD cards can be used on a GPIO based bus by bitbanging
+	  the SPI protocol in software.
+
+	  This driver provides a sysfs interface to dynamically create
+	  and destroy GPIO-based MMC/SD card interfaces. It also provides
+	  a platform device interface API.
+	  See Documentation/gpiommc.txt for details.
+
+	  The module will be called gpiommc.
+
+	  If unsure, say N.
+
+config MMC_S3C
+	tristate "Samsung S3C SD/MMC Card Interface support"
+	depends on ARCH_S3C2410 && MMC
Index: linux-next/drivers/mmc/host/Makefile
===================================================================
--- linux-next.orig/drivers/mmc/host/Makefile	2008-07-18 21:52:46.000000000 +0200
+++ linux-next/drivers/mmc/host/Makefile	2008-07-18 21:54:19.000000000 +0200
@@ -20,3 +20,4 @@ obj-$(CONFIG_MMC_ATMELMCI)	+= atmel-mci.
 obj-$(CONFIG_MMC_TIFM_SD)	+= tifm_sd.o
 obj-$(CONFIG_MMC_SPI)		+= mmc_spi.o
 obj-$(CONFIG_MMC_S3C)   	+= s3cmci.o
+obj-$(CONFIG_GPIOMMC)		+= gpiommc.o
Index: linux-next/include/linux/mmc/gpiommc.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-next/include/linux/mmc/gpiommc.h	2008-07-18 21:54:05.000000000 +0200
@@ -0,0 +1,62 @@
+/*
+ * Device driver for MMC/SD cards driven over a GPIO bus.
+ *
+ * Copyright (c) 2008 Michael Buesch
+ *
+ * Licensed under the GNU/GPL version 2.
+ */
+#ifndef LINUX_GPIOMMC_H_
+#define LINUX_GPIOMMC_H_
+
+#include <linux/types.h>
+
+
+#define GPIOMMC_MAX_NAMELEN		15
+
+/** struct gpiommc_pins - Hardware pin assignments
+ * @gpio_di: The GPIO number of the DATA IN pin
+ * @gpio_do: The GPIO number of the DATA OUT pin
+ * @gpio_clk: The GPIO number of the CLOCK pin
+ * @gpio_cs: The GPIO number of the CHIPSELECT pin
+ * @cs_activelow: If true, the chip is considered selected if @gpio_cs is low.
+ */
+struct gpiommc_pins {
+	unsigned int gpio_di;
+	unsigned int gpio_do;
+	unsigned int gpio_clk;
+	unsigned int gpio_cs;
+	bool cs_activelow;
+};
+
+/** struct gpiommc_platform_data - Platform data for a MMC-over-SPI-GPIO device.
+ * @name: The unique name string of the device.
+ * @pins: The hardware pin assignments.
+ * @mode: The hardware mode. This is either SPI_MODE_0,
+ *        SPI_MODE_1, SPI_MODE_2 or SPI_MODE_3. See the SPI documentation.
+ * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
+ *                This is not standards compliant, but may be required for some
+ *                embedded machines to gain reasonable speed.
+ * @max_bus_speed: The maximum speed of the SPI bus, in Hertz.
+ */
+struct gpiommc_platform_data {
+	char name[GPIOMMC_MAX_NAMELEN + 1];
+	struct gpiommc_pins pins;
+	u8 mode;
+	bool no_spi_delay;
+	unsigned int max_bus_speed;
+};
+
+/** GPIOMMC_PLATDEV_NAME - The platform device name string.
+ * The name string that has to be used for platform_device_alloc
+ * when allocating a gpiommc device.
+ */
+#define GPIOMMC_PLATDEV_NAME	"gpiommc"
+
+/** gpiommc_next_id - Get another platform device ID number.
+ * This returns the next platform device ID number that has to be used
+ * for platform_device_alloc. The ID is opaque and should not be used for
+ * anything else.
+ */
+int gpiommc_next_id(void);
+
+#endif /* LINUX_GPIOMMC_H_ */
Index: linux-next/Documentation/gpiommc.txt
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-next/Documentation/gpiommc.txt	2008-07-18 21:54:05.000000000 +0200
@@ -0,0 +1,96 @@
+GPIOMMC - Driver for an MMC/SD card on a bitbanging GPIO SPI bus
+================================================================
+
+The gpiommc module hooks up the mmc_spi and spi_gpio modules for running an
+MMC or SD card on GPIO pins.
+
+Two interfaces for registering a new MMC/SD card device are provided.
+A static platform-device based mechanism and a dynamic sysfs based interface.
+
+
+Registering devices via platform-device
+=======================================
+
+The platform-device interface is used for registering MMC/SD devices that are
+part of the hardware platform. This is most useful only for embedded machines
+with MMC/SD devices statically connected to the platform GPIO bus.
+
+The data structures are declared in <linux/mmc/gpiommc.h>
+
+To register a new device, define an instance of struct gpiommc_platform_data.
+This structure holds any information about how the device is hooked up to the
+GPIO pins and what hardware modes the device supports. See the docbook-style
+documentation in the header file for more information on the struct fields.
+
+Then allocate a new instance of a platform device by doing:
+
+	pdev = platform_device_alloc(GPIOMMC_PLATDEV_NAME, gpiommc_next_id());
+
+This will allocate the platform device data structures and hook it up to the
+gpiommc driver.
+Then add the gpiommc_platform_data to the platform device.
+
+	err = platform_device_add_data(pdev, pdata, sizeof(struct gpiommc_platform_data));
+
+You may free the local instance of struct gpiommc_platform_data now.
+Now simply register the platform device.
+
+	err = platform_device_add(pdev);
+
+Done. The gpiommc probe routine should be called and you should see a dmesg
+message for the added device.
+
+
+Registering devices via sysfs
+=============================
+
+MMC/SD cards connected via GPIO often are a pretty dynamic thing. For example
+selfmade hacks for soldering an MMC/SD card to standard GPIO pins on embedded
+hardware are a common situation.
+So we provide a dynamic interface to conveniently handle adding and removing
+devices from userspace, without the need to recompile the kernel.
+
+There are two sysfs files responsible for that:
+export ADD=/sys/bus/platform/drivers/gpiommc/add
+export REMOVE=/sys/bus/platform/drivers/gpiommc/remove
+
+To add a new device, simply echo the configuration string to the "add" file.
+The config string is composed out of the following elements:
+
+DEVNAME DIpin DOpin CLKpin CSpin SPIMODE MAXBUSSPEED NO_SPI_DELAY CSACTIVELOW
+
+DEVNAME is a unique name string for the device.
+DIpin is the SPI DI GPIO pin.
+DOpin is the SPI DO GPIO pin.
+CLKpin is the SPI CLOCK GPIO pin.
+CSpin is the SPI CHIPSELECT GPIO pin.
+SPIMODE is the hardware mode the device will run at. Can be 0-3.
+MAXBUSSPEED is the maximum bus speed in Hertz.
+NO_SPI_DELAY can be 1 or 0. If it is 1, then the lowlevel SPI delay
+will not be performed. This is not standards compliant, but may be required
+to gain reasonable speeds on embedded hardware.
+CSACTIVELOW can be 1 or 0. If it is 1, the chip is considered to be selected, if CS
+is at a logical 0.
+
+Note that the elements SPIMODE, MAXBUSSPEED and NO_SPI_DELAY are optional
+and can be omitted.
+SPIMODE will default to 0.
+MAXBUSSSPEED will default to 5Mhz.
+NO_SPI_DELAY will default to 0.
+CSACTIVELOW will default to 1.
+
+Example:
+
+	echo -n "my_device 5 4 3 7 0 1000000 1" > $ADD
+
+This will add a new device called "my_device" with the GPIO pins assigned as
+DI=5, DO=4, CLK=3, CS=7
+The hardware mode will be SPI_MODE_0.
+The maximum bus speed will be 1000000 Hz (1Mhz)
+And the explicit SPI delay at the lowlevel bitbang loop will be switched off.
+
+To remove a device, simply echo the device name string to the "remove" file.
+
+Example:
+
+	echo -n "my_device" > $REMOVE
Index: linux-next/MAINTAINERS
===================================================================
--- linux-next.orig/MAINTAINERS	2008-07-18 21:54:00.000000000 +0200
+++ linux-next/MAINTAINERS	2008-07-18 21:54:05.000000000 +0200
@@ -1875,6 +1875,11 @@ L:	gigaset307x-common@lists.sourceforge.
 W:	http://gigaset307x.sourceforge.net/
 S:	Maintained
 
+GPIOMMC DRIVER
+P:	Michael Buesch
+M:	mb@bu3sch.de
+S:	Maintained
+
 HARDWARE MONITORING
 P:	Mark M. Hoffman
 M:	mhoffman@lightlink.com

-- 
Greetings Michael.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Randy Dunlap]

On Fri, 18 Jul 2008 22:01:33 +0200 Michael Buesch wrote:

> This driver hooks up the mmc_spi and spi_gpio modules so that
> MMC/SD cards can be used on a GPIO based bus by bitbanging
> the SPI protocol in software.
> 
> This driver provides a sysfs interface to dynamically create
> and destroy GPIO-based MMC/SD card interfaces. It also provides
> a platform device interface API.
> See Documentation/gpiommc.txt for details.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> ---
> 
> This driver is used in OpenWrt since quite some time, so please
> consider for inclusion in mainline.

Full diffstat here would be Good.

> Index: linux-next/drivers/mmc/host/gpiommc.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-next/drivers/mmc/host/gpiommc.c	2008-07-18 21:54:05.000000000 +0200
> @@ -0,0 +1,328 @@
> +/*
> + * Driver an MMC/SD card on a bitbanging GPIO SPI bus.
> + * This module hooks up the mmc_spi and spi_gpio modules and also
> + * provides a sysfs interface.
> + *
> + * Copyright 2008 Michael Buesch <mb@bu3sch.de>
> + *
> + * Licensed under the GNU/GPL. See COPYING for details.
> + */
> +
> +#include <linux/mmc/gpiommc.h>
> +#include <linux/platform_device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi_gpio.h>
> +
> +
> +#define PFX				"gpio-mmc: "
> +#define GPIOMMC_MAX_NAMELEN_STR		__stringify(GPIOMMC_MAX_NAMELEN)
> +
> +struct gpiommc_device {
> +	struct platform_device *pdev;
> +	struct platform_device *spi_pdev;
> +	struct spi_board_info boardinfo;
> +};
> +
> +
> +MODULE_DESCRIPTION("GPIO based MMC driver");
> +MODULE_AUTHOR("Michael Buesch");
> +MODULE_LICENSE("GPL");
> +
> +
> +static int gpiommc_boardinfo_setup(struct spi_board_info *bi,
> +				   struct spi_master *master,
> +				   void *data)
> +{
> +	struct gpiommc_device *d = data;
> +	struct gpiommc_platform_data *pdata = d->pdev->dev.platform_data;
> +
> +	/* Bind the SPI master to the MMC-SPI host driver. */
> +	strlcpy(bi->modalias, "mmc_spi", sizeof(bi->modalias));
> +
> +	bi->max_speed_hz = pdata->max_bus_speed;
> +	bi->bus_num = master->bus_num;
> +	bi->mode = pdata->mode;
> +
> +	return 0;
> +}
> +
> +static int gpiommc_probe(struct platform_device *pdev)
> +{
> +	struct gpiommc_platform_data *mmc_pdata = pdev->dev.platform_data;
> +	struct spi_gpio_platform_data spi_pdata;
> +	struct gpiommc_device *d;
> +	int err;
> +
> +	err = -ENXIO;
> +	if (!mmc_pdata)
> +		goto error;
> +
> +	/* Allocate the GPIO-MMC device */
> +	err = -ENOMEM;
> +	d = kzalloc(sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		goto error;
> +	d->pdev = pdev;
> +
> +	/* Create the SPI-GPIO device */
> +	d->spi_pdev = platform_device_alloc(SPI_GPIO_PLATDEV_NAME,
> +					    spi_gpio_next_id());
> +	if (!d->spi_pdev)
> +		goto err_free_d;
> +
> +	memset(&spi_pdata, 0, sizeof(spi_pdata));
> +	spi_pdata.pin_clk = mmc_pdata->pins.gpio_clk;
> +	spi_pdata.pin_miso = mmc_pdata->pins.gpio_do;
> +	spi_pdata.pin_mosi = mmc_pdata->pins.gpio_di;
> +	spi_pdata.pin_cs = mmc_pdata->pins.gpio_cs;
> +	spi_pdata.cs_activelow = mmc_pdata->pins.cs_activelow;
> +	spi_pdata.no_spi_delay = mmc_pdata->no_spi_delay;
> +	spi_pdata.boardinfo_setup = gpiommc_boardinfo_setup;
> +	spi_pdata.boardinfo_setup_data = d;
> +
> +	err = platform_device_add_data(d->spi_pdev, &spi_pdata,
> +				       sizeof(spi_pdata));
> +	if (err)
> +		goto err_free_pdev;
> +	err = platform_device_add(d->spi_pdev);
> +	if (err)
> +		goto err_free_pdata;
> +	platform_set_drvdata(pdev, d);
> +
> +	printk(KERN_INFO PFX "MMC-Card \"%s\" "
> +	       "attached to GPIO pins di=%u, do=%u, clk=%u, cs=%u\n",
> +	       mmc_pdata->name, mmc_pdata->pins.gpio_di,
> +	       mmc_pdata->pins.gpio_do,
> +	       mmc_pdata->pins.gpio_clk,
> +	       mmc_pdata->pins.gpio_cs);
> +
> +	return 0;
> +
> +err_free_pdata:
> +	kfree(d->spi_pdev->dev.platform_data);
> +	d->spi_pdev->dev.platform_data = NULL;
> +err_free_pdev:
> +	platform_device_put(d->spi_pdev);
> +err_free_d:
> +	kfree(d);
> +error:
> +	return err;
> +}
> +
> +static int gpiommc_remove(struct platform_device *pdev)
> +{
> +	struct gpiommc_device *d = platform_get_drvdata(pdev);
> +	struct gpiommc_platform_data *pdata = d->pdev->dev.platform_data;
> +
> +	platform_device_unregister(d->spi_pdev);
> +	printk(KERN_INFO PFX "GPIO based MMC-Card \"%s\" removed\n", pdata->name);
> +	platform_device_put(d->spi_pdev);
> +
> +	return 0;
> +}
> +
> +/* Wrapper for the platform data with context data for the sysfs interface. */
> +struct gpiommc_sysfs_platform_data {
> +	struct gpiommc_platform_data p; /* Keep as first element */
> +
> +	/* The platform device that we allocated. */
> +	struct platform_device *pdev;
> +	/* gpiommc_sysfs_list */
> +	struct list_head list;
> +};
> +
> +static LIST_HEAD(gpiommc_sysfs_list);
> +static DEFINE_MUTEX(gpiommc_sysfs_mutex);
> +
> +static struct gpiommc_sysfs_platform_data *gpiommc_sysfs_find_dev(const char *name)
> +{
> +	struct gpiommc_sysfs_platform_data *pdata;
> +
> +	list_for_each_entry(pdata, &gpiommc_sysfs_list, list) {
> +		if (strcmp(pdata->p.name, name) == 0)
> +			return pdata;
> +	}
> +
> +	return NULL;
> +}
> +
> +static ssize_t gpiommc_add_store(struct device_driver *drv,
> +				 const char *buf, size_t count)
> +{
> +	int res, err;
> +	struct gpiommc_sysfs_platform_data pdata_local, *pdata;
> +	struct platform_device *pdev;
> +	unsigned int no_spi_delay = 0, mode = 0, csactivelow = 0;
> +
> +	mutex_lock(&gpiommc_sysfs_mutex);
> +
> +	pdata = &pdata_local;
> +	memset(pdata, 0, sizeof(*pdata));
> +
> +	err = -EINVAL;
> +	res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u %u %u %u %u %u %u %u",
> +		     pdata->p.name,
> +		     &pdata->p.pins.gpio_di,
> +		     &pdata->p.pins.gpio_do,
> +		     &pdata->p.pins.gpio_clk,
> +		     &pdata->p.pins.gpio_cs,
> +		     &mode,
> +		     &pdata->p.max_bus_speed,
> +		     &no_spi_delay,
> +		     &csactivelow);
> +	pdata->p.mode = mode;
> +	pdata->p.no_spi_delay = !!no_spi_delay;
> +	pdata->p.pins.cs_activelow = !!csactivelow;
> +	if (res < 9)
> +		pdata->p.pins.cs_activelow = 1; /* Default: CS = activelow */
> +	if (res < 8)
> +		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
> +	if (res < 7)
> +		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */
> +	if (res < 6)
> +		pdata->p.mode = 0; /* Default: SPI mode 0 */
> +	if (res < 5 || res > 9)
> +		goto out; /* First 5 args are mandatory. */
> +
> +	/* Convert mode so that the SPI subsystem does understand it. */
> +	switch (pdata->p.mode) {
> +	case 0:
> +		pdata->p.mode = SPI_MODE_0;
> +		break;
> +	case 1:
> +		pdata->p.mode = SPI_MODE_1;
> +		break;
> +	case 2:
> +		pdata->p.mode = SPI_MODE_2;
> +		break;
> +	case 3:
> +		pdata->p.mode = SPI_MODE_3;
> +		break;
> +	default:
> +		goto out; /* Invalid mode */
> +	}
> +
> +	err = -EEXIST;
> +	if (gpiommc_sysfs_find_dev(pdata->p.name))
> +		goto out;
> +
> +	err = -ENOMEM;
> +	pdev = platform_device_alloc(GPIOMMC_PLATDEV_NAME, gpiommc_next_id());
> +	if (!pdev)
> +		goto out;
> +
> +	err = platform_device_add_data(pdev, pdata, sizeof(*pdata));
> +	if (err)
> +		goto err_free_pdev;
> +	pdata = pdev->dev.platform_data;
> +
> +	err = platform_device_add(pdev);
> +	if (err)
> +		goto err_free_pdev;
> +
> +	pdata->pdev = pdev;
> +	INIT_LIST_HEAD(&pdata->list);
> +	list_add(&pdata->list, &gpiommc_sysfs_list);
> +
> +	err = 0;
> +out:
> +	mutex_unlock(&gpiommc_sysfs_mutex);
> +
> +	return err ? err : count;
> +
> +err_free_pdev:
> +	platform_device_put(pdev);
> +	goto out;
> +}
> +
> +static ssize_t gpiommc_remove_store(struct device_driver *drv,
> +				    const char *buf, size_t count)
> +{
> +	struct gpiommc_sysfs_platform_data *pdata;
> +	int err;
> +
> +	mutex_lock(&gpiommc_sysfs_mutex);
> +
> +	err = -ENODEV;
> +	pdata = gpiommc_sysfs_find_dev(buf);
> +	if (!pdata)
> +		goto out;
> +
> +	list_del(&pdata->list);
> +	platform_device_unregister(pdata->pdev);
> +
> +out:
> +	mutex_unlock(&gpiommc_sysfs_mutex);
> +
> +	return err ? err : count;
> +}
> +
> +static DRIVER_ATTR(add, 0200,

Use defined symbols instead of inline constants (for 0200).

> +		   NULL, gpiommc_add_store);
> +static DRIVER_ATTR(remove, 0200,
> +		   NULL, gpiommc_remove_store);
> +
> +static struct platform_driver gpiommc_plat_driver = {
> +	.probe	= gpiommc_probe,
> +	.remove	= gpiommc_remove,
> +	.driver	= {
> +		.name	= GPIOMMC_PLATDEV_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +int gpiommc_next_id(void)
> +{
> +	static atomic_t counter = ATOMIC_INIT(-1);
> +
> +	return atomic_inc_return(&counter);
> +}
> +EXPORT_SYMBOL(gpiommc_next_id);
> +
> +static int __init gpiommc_modinit(void)
> +{
> +	int err;
> +
> +	err = platform_driver_register(&gpiommc_plat_driver);
> +	if (err)
> +		return err;
> +	err = driver_create_file(&gpiommc_plat_driver.driver,
> +				 &driver_attr_add);
> +	if (err)
> +		goto err_drv_unreg;
> +	err = driver_create_file(&gpiommc_plat_driver.driver,
> +				 &driver_attr_remove);
> +	if (err)
> +		goto err_remove_add;
> +
> +	return 0;
> +
> +err_remove_add:
> +	driver_remove_file(&gpiommc_plat_driver.driver,
> +			   &driver_attr_add);
> +err_drv_unreg:
> +	platform_driver_unregister(&gpiommc_plat_driver);
> +	return err;
> +}
> +module_init(gpiommc_modinit);
> +
> +static void __exit gpiommc_modexit(void)
> +{
> +	struct gpiommc_sysfs_platform_data *pdata, *pdata_tmp;
> +
> +	driver_remove_file(&gpiommc_plat_driver.driver,
> +			   &driver_attr_remove);
> +	driver_remove_file(&gpiommc_plat_driver.driver,
> +			   &driver_attr_add);
> +
> +	mutex_lock(&gpiommc_sysfs_mutex);
> +	list_for_each_entry_safe(pdata, pdata_tmp, &gpiommc_sysfs_list, list) {
> +		list_del(&pdata->list);
> +		platform_device_unregister(pdata->pdev);
> +	}
> +	mutex_unlock(&gpiommc_sysfs_mutex);
> +
> +	platform_driver_unregister(&gpiommc_plat_driver);
> +}
> +module_exit(gpiommc_modexit);
> Index: linux-next/drivers/mmc/host/Kconfig
> ===================================================================
> --- linux-next.orig/drivers/mmc/host/Kconfig	2008-07-18 21:52:46.000000000 +0200
> +++ linux-next/drivers/mmc/host/Kconfig	2008-07-18 21:57:08.000000000 +0200
> @@ -164,3 +164,23 @@ config MMC_S3C
>  
>  	  If unsure, say N.
>  
> +config GPIOMMC
> +	tristate "MMC/SD over GPIO-based SPI"
> +	depends on MMC && MMC_SPI && SPI_GPIO
> +	help
> +	  This driver hooks up the mmc_spi and spi_gpio modules so that
> +	  MMC/SD cards can be used on a GPIO based bus by bitbanging
> +	  the SPI protocol in software.
> +
> +	  This driver provides a sysfs interface to dynamically create
> +	  and destroy GPIO-based MMC/SD card interfaces. It also provides
> +	  a platform device interface API.
> +	  See Documentation/gpiommc.txt for details.
> +
> +	  The module will be called gpiommc.
> +
> +	  If unsure, say N.
> +
> +config MMC_S3C
> +	tristate "Samsung S3C SD/MMC Card Interface support"
> +	depends on ARCH_S3C2410 && MMC

> Index: linux-next/include/linux/mmc/gpiommc.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-next/include/linux/mmc/gpiommc.h	2008-07-18 21:54:05.000000000 +0200
> @@ -0,0 +1,62 @@
> +/*
> + * Device driver for MMC/SD cards driven over a GPIO bus.
> + *
> + * Copyright (c) 2008 Michael Buesch
> + *
> + * Licensed under the GNU/GPL version 2.
> + */
> +#ifndef LINUX_GPIOMMC_H_
> +#define LINUX_GPIOMMC_H_
> +
> +#include <linux/types.h>
> +
> +
> +#define GPIOMMC_MAX_NAMELEN		15
> +
> +/** struct gpiommc_pins - Hardware pin assignments

All of these kernel-doc blocks should be like:

/**
 * struct gpiommc_pines - Hardware pin assignments


> + * @gpio_di: The GPIO number of the DATA IN pin
> + * @gpio_do: The GPIO number of the DATA OUT pin
> + * @gpio_clk: The GPIO number of the CLOCK pin
> + * @gpio_cs: The GPIO number of the CHIPSELECT pin
> + * @cs_activelow: If true, the chip is considered selected if @gpio_cs is low.
> + */
> +struct gpiommc_pins {
> +	unsigned int gpio_di;
> +	unsigned int gpio_do;
> +	unsigned int gpio_clk;
> +	unsigned int gpio_cs;
> +	bool cs_activelow;
> +};
> +
> +/** struct gpiommc_platform_data - Platform data for a MMC-over-SPI-GPIO device.
> + * @name: The unique name string of the device.
> + * @pins: The hardware pin assignments.
> + * @mode: The hardware mode. This is either SPI_MODE_0,
> + *        SPI_MODE_1, SPI_MODE_2 or SPI_MODE_3. See the SPI documentation.
> + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
> + *                This is not standards compliant, but may be required for some
> + *                embedded machines to gain reasonable speed.
> + * @max_bus_speed: The maximum speed of the SPI bus, in Hertz.
> + */
> +struct gpiommc_platform_data {
> +	char name[GPIOMMC_MAX_NAMELEN + 1];
> +	struct gpiommc_pins pins;
> +	u8 mode;
> +	bool no_spi_delay;
> +	unsigned int max_bus_speed;
> +};
> +
> +/** GPIOMMC_PLATDEV_NAME - The platform device name string.
> + * The name string that has to be used for platform_device_alloc
> + * when allocating a gpiommc device.
> + */
> +#define GPIOMMC_PLATDEV_NAME	"gpiommc"
> +
> +/** gpiommc_next_id - Get another platform device ID number.
> + * This returns the next platform device ID number that has to be used
> + * for platform_device_alloc. The ID is opaque and should not be used for
> + * anything else.
> + */
> +int gpiommc_next_id(void);
> +
> +#endif /* LINUX_GPIOMMC_H_ */
> Index: linux-next/Documentation/gpiommc.txt
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-next/Documentation/gpiommc.txt	2008-07-18 21:54:05.000000000 +0200
> @@ -0,0 +1,96 @@
> +GPIOMMC - Driver for an MMC/SD card on a bitbanging GPIO SPI bus
> +================================================================
> +
> +The gpiommc module hooks up the mmc_spi and spi_gpio modules for running an
> +MMC or SD card on GPIO pins.
> +
> +Two interfaces for registering a new MMC/SD card device are provided.

                                                           are provided:
   a static

> +A static platform-device based mechanism and a dynamic sysfs based interface.
> +
> +
> +Registering devices via platform-device
> +=======================================
> +
> +The platform-device interface is used for registering MMC/SD devices that are
> +part of the hardware platform. This is most useful only for embedded machines
> +with MMC/SD devices statically connected to the platform GPIO bus.
> +
> +The data structures are declared in <linux/mmc/gpiommc.h>

end sentence with '.'

> +
> +To register a new device, define an instance of struct gpiommc_platform_data.
> +This structure holds any information about how the device is hooked up to the
> +GPIO pins and what hardware modes the device supports. See the docbook-style
> +documentation in the header file for more information on the struct fields.
> +
> +Then allocate a new instance of a platform device by doing:
> +
> +	pdev = platform_device_alloc(GPIOMMC_PLATDEV_NAME, gpiommc_next_id());
> +
> +This will allocate the platform device data structures and hook it up to the
> +gpiommc driver.
> +Then add the gpiommc_platform_data to the platform device.
> +
> +	err = platform_device_add_data(pdev, pdata, sizeof(struct gpiommc_platform_data));
> +
> +You may free the local instance of struct gpiommc_platform_data now.
> +Now simply register the platform device.
> +
> +	err = platform_device_add(pdev);
> +
> +Done. The gpiommc probe routine should be called and you should see a dmesg

s/dmesg/kernel log/  (?)

> +message for the added device.
> +
> +
> +Registering devices via sysfs
> +=============================
> +
> +MMC/SD cards connected via GPIO often are a pretty dynamic thing. For example
> +selfmade hacks for soldering an MMC/SD card to standard GPIO pins on embedded
> +hardware are a common situation.

I think that the two sentences above/below would be better if joined with a comma...

> +So we provide a dynamic interface to conveniently handle adding and removing
> +devices from userspace, without the need to recompile the kernel.
> +
> +There are two sysfs files responsible for that:
> +export ADD=/sys/bus/platform/drivers/gpiommc/add
> +export REMOVE=/sys/bus/platform/drivers/gpiommc/remove
> +
> +To add a new device, simply echo the configuration string to the "add" file.
> +The config string is composed out of the following elements:
> +
> +DEVNAME DIpin DOpin CLKpin CSpin SPIMODE MAXBUSSPEED NO_SPI_DELAY CSACTIVELOW
> +
> +DEVNAME is a unique name string for the device.
> +DIpin is the SPI DI GPIO pin.
> +DOpin is the SPI DO GPIO pin.
> +CLKpin is the SPI CLOCK GPIO pin.
> +CSpin is the SPI CHIPSELECT GPIO pin.
> +SPIMODE is the hardware mode the device will run at. Can be 0-3.
> +MAXBUSSPEED is the maximum bus speed in Hertz.
> +NO_SPI_DELAY can be 1 or 0. If it is 1, then the lowlevel SPI delay
> +will not be performed. This is not standards compliant, but may be required
> +to gain reasonable speeds on embedded hardware.
> +CSACTIVELOW can be 1 or 0. If it is 1, the chip is considered to be selected, if CS
> +is at a logical 0.
> +

Would this be better done via configfs?  sysfs files are supposed to be
single-value files.

> +Note that the elements SPIMODE, MAXBUSSPEED and NO_SPI_DELAY are optional
> +and can be omitted.
> +SPIMODE will default to 0.
> +MAXBUSSSPEED will default to 5Mhz.
> +NO_SPI_DELAY will default to 0.
> +CSACTIVELOW will default to 1.
> +
> +Example:
> +
> +	echo -n "my_device 5 4 3 7 0 1000000 1" > $ADD
> +
> +This will add a new device called "my_device" with the GPIO pins assigned as
> +DI=5, DO=4, CLK=3, CS=7

end sentence with '.'

> +The hardware mode will be SPI_MODE_0.
> +The maximum bus speed will be 1000000 Hz (1Mhz)

ditto

> +And the explicit SPI delay at the lowlevel bitbang loop will be switched off.
> +
> +To remove a device, simply echo the device name string to the "remove" file.
> +
> +Example:
> +
> +	echo -n "my_device" > $REMOVE


---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Michael Buesch]

Thanks for the comments.

On Saturday 19 July 2008 00:10:37 Randy Dunlap wrote:
> > +To add a new device, simply echo the configuration string to the "add" file.
> > +The config string is composed out of the following elements:
> > +
> > +DEVNAME DIpin DOpin CLKpin CSpin SPIMODE MAXBUSSPEED NO_SPI_DELAY CSACTIVELOW
> > +
> > +DEVNAME is a unique name string for the device.
> > +DIpin is the SPI DI GPIO pin.
> > +DOpin is the SPI DO GPIO pin.
> > +CLKpin is the SPI CLOCK GPIO pin.
> > +CSpin is the SPI CHIPSELECT GPIO pin.
> > +SPIMODE is the hardware mode the device will run at. Can be 0-3.
> > +MAXBUSSPEED is the maximum bus speed in Hertz.
> > +NO_SPI_DELAY can be 1 or 0. If it is 1, then the lowlevel SPI delay
> > +will not be performed. This is not standards compliant, but may be required
> > +to gain reasonable speeds on embedded hardware.
> > +CSACTIVELOW can be 1 or 0. If it is 1, the chip is considered to be selected, if CS
> > +is at a logical 0.
> > +
> 
> Would this be better done via configfs?  sysfs files are supposed to be
> single-value files.


Well, I really want to avoid over-engineering this thing.
I thought about using configfs, but that would require to keep lots of state
information across the operations.
So one would have to allocate a device with mkdir. Then configure the parameters.
And then somehow tell the kernel to register it. State has to be maintained over
this time and I'm not sure how that "register" operation would look like.
Writing a "1" to a "register" file? So why not write all config parameters
to an "add" file and be done with all the stuff. ;)

It all depends on how you define "one thing per file", IMO.
This "add" file does one thing. It creates a device. We must, of course, pass
some configuration parameters, too.

-- 
Greetings Michael.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Greg KH]

On Sat, Jul 19, 2008 at 12:38:07AM +0200, Michael Buesch wrote:
> Thanks for the comments.
> 
> On Saturday 19 July 2008 00:10:37 Randy Dunlap wrote:
> > > +To add a new device, simply echo the configuration string to the "add" file.
> > > +The config string is composed out of the following elements:
> > > +
> > > +DEVNAME DIpin DOpin CLKpin CSpin SPIMODE MAXBUSSPEED NO_SPI_DELAY CSACTIVELOW
> > > +
> > > +DEVNAME is a unique name string for the device.
> > > +DIpin is the SPI DI GPIO pin.
> > > +DOpin is the SPI DO GPIO pin.
> > > +CLKpin is the SPI CLOCK GPIO pin.
> > > +CSpin is the SPI CHIPSELECT GPIO pin.
> > > +SPIMODE is the hardware mode the device will run at. Can be 0-3.
> > > +MAXBUSSPEED is the maximum bus speed in Hertz.
> > > +NO_SPI_DELAY can be 1 or 0. If it is 1, then the lowlevel SPI delay
> > > +will not be performed. This is not standards compliant, but may be required
> > > +to gain reasonable speeds on embedded hardware.
> > > +CSACTIVELOW can be 1 or 0. If it is 1, the chip is considered to be selected, if CS
> > > +is at a logical 0.
> > > +
> > 
> > Would this be better done via configfs?  sysfs files are supposed to be
> > single-value files.
> 
> 
> Well, I really want to avoid over-engineering this thing.
> I thought about using configfs, but that would require to keep lots of state
> information across the operations.
> So one would have to allocate a device with mkdir. Then configure the parameters.
> And then somehow tell the kernel to register it. State has to be maintained over
> this time and I'm not sure how that "register" operation would look like.
> Writing a "1" to a "register" file? So why not write all config parameters
> to an "add" file and be done with all the stuff. ;)
> 
> It all depends on how you define "one thing per file", IMO.
> This "add" file does one thing. It creates a device. We must, of course, pass
> some configuration parameters, too.

We have precidence with this for the device id being "added" to a driver
at run time in PCI and USB.  Not that I really like this, I do think
configfs would be better to do this with here instead of sysfs.

That being said, as this is a new sysfs file, please break this
documentation up into a file in the Documentation/ABI directory instead
of here.

thanks,

greg k-h

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Michael Buesch]

On Saturday 19 July 2008 00:59:52 Greg KH wrote:
> On Sat, Jul 19, 2008 at 12:38:07AM +0200, Michael Buesch wrote:
> > Thanks for the comments.
> > 
> > On Saturday 19 July 2008 00:10:37 Randy Dunlap wrote:
> > > > +To add a new device, simply echo the configuration string to the "add" file.
> > > > +The config string is composed out of the following elements:
> > > > +
> > > > +DEVNAME DIpin DOpin CLKpin CSpin SPIMODE MAXBUSSPEED NO_SPI_DELAY CSACTIVELOW
> > > > +
> > > > +DEVNAME is a unique name string for the device.
> > > > +DIpin is the SPI DI GPIO pin.
> > > > +DOpin is the SPI DO GPIO pin.
> > > > +CLKpin is the SPI CLOCK GPIO pin.
> > > > +CSpin is the SPI CHIPSELECT GPIO pin.
> > > > +SPIMODE is the hardware mode the device will run at. Can be 0-3.
> > > > +MAXBUSSPEED is the maximum bus speed in Hertz.
> > > > +NO_SPI_DELAY can be 1 or 0. If it is 1, then the lowlevel SPI delay
> > > > +will not be performed. This is not standards compliant, but may be required
> > > > +to gain reasonable speeds on embedded hardware.
> > > > +CSACTIVELOW can be 1 or 0. If it is 1, the chip is considered to be selected, if CS
> > > > +is at a logical 0.
> > > > +
> > > 
> > > Would this be better done via configfs?  sysfs files are supposed to be
> > > single-value files.
> > 
> > 
> > Well, I really want to avoid over-engineering this thing.
> > I thought about using configfs, but that would require to keep lots of state
> > information across the operations.
> > So one would have to allocate a device with mkdir. Then configure the parameters.
> > And then somehow tell the kernel to register it. State has to be maintained over
> > this time and I'm not sure how that "register" operation would look like.
> > Writing a "1" to a "register" file? So why not write all config parameters
> > to an "add" file and be done with all the stuff. ;)
> > 
> > It all depends on how you define "one thing per file", IMO.
> > This "add" file does one thing. It creates a device. We must, of course, pass
> > some configuration parameters, too.
> 
> We have precidence with this for the device id being "added" to a driver
> at run time in PCI and USB.  Not that I really like this, I do think
> configfs would be better to do this with here instead of sysfs.
> 
> That being said, as this is a new sysfs file, please break this
> documentation up into a file in the Documentation/ABI directory instead
> of here.

Ok, let us keep this stuff some time in the experimental -mm tree
and I will think about configfs again. I don't really like that sysfs "add"
file, too. But I can't say if I like configfs more.

I probably need to play with configfs a little bit to find out what's best.
One advantage is that you can easily add more config stuff later without
the risk of breaking stuff. That's quite an advantage, too...
But that "how do we register the device after allocating it?" does worry
me a little bit. Any ideas on how to implement allocate/config/register
in configfs?

-- 
Greetings Michael.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[David Brownell]

[ I see "v3" switches to configfs ... same comments apply. ]

On Friday 18 July 2008, Randy Dunlap wrote:
> On Fri, 18 Jul 2008 22:01:33 +0200 Michael Buesch wrote:
> 
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-next/drivers/mmc/host/gpiommc.c	2008-07-18 21:54:05.000000000 +0200
> > @@ -0,0 +1,328 @@
> > +/*
> > + * Driver an MMC/SD card on a bitbanging GPIO SPI bus.

Driver "for" an MMC/SD card ...


> > + * This module hooks up the mmc_spi and spi_gpio modules and also
> > + * provides a sysfs interface.

I'm not quite following things here.  The mmc_spi driver has
no particular issues running over any correct implementation
of SPI, whether it uses GPIO or anything else.

In fact, I did doing a bunch of early mmc_spi work using a
SPI-over-GPIO bitbanger.  (Also some tuning for the GPIO
interfaces, making sure those calls inlined transparently
to get more usable speeds.)  So I know that already works
just fine ...


So what this driver adds is mostly a userspace interface, yes?
The reason to select THIS feature is to get the userspace
configuration ... not to get the MMC/SD-over-GPIO, since
that can already be done without this code.


If so, please refocus the descriptions a bit more on that;
it's not actually a "GPIO based MMC/SD driver"; it's really
a "Configfs creation interface for GPIO based MMC/SD".


> > +	res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u %u %u %u %u %u %u %u",

Kind of ugly, yes?  ;)


> > +		     pdata->p.name,
> > +		     &pdata->p.pins.gpio_di,
> > +		     &pdata->p.pins.gpio_do,

Can we avoid that DI/DO convention?  The master DI is the slave DO,
and vice versa.  Your documentation doesn't say which is which; and
for such ambiguous names, that's critical.  Since these GPIOs are
on the master side, I'd expect these labels are for the master
side not the slave side.

Please switch to the more conventional "MOSI" (Master Out, Slave In)
and "MISO" (Master In, Slave Out).  That's unambiguous.  If you want
to be a bit more clear you could say that MOSI goes to the MMC/SD
card pin 2 (card data in), and MISO goes to MMC/SD card pin 7 (card
data out).


> > +		     &pdata->p.pins.gpio_clk,
> > +		     &pdata->p.pins.gpio_cs,
> > +		     &mode,
> > +		     &pdata->p.max_bus_speed,
> > +		     &no_spi_delay,
> > +		     &csactivelow);
> > +	pdata->p.mode = mode;
> > +	pdata->p.no_spi_delay = !!no_spi_delay;
> > +	pdata->p.pins.cs_activelow = !!csactivelow;
> > +	if (res < 9)
> > +		pdata->p.pins.cs_activelow = 1; /* Default: CS = activelow */

Do you actually need this?  If so, why?  The MMC (and SD) specs
say that pin 1 (chipselect) is active low, and that's normal
for all SPI interfaces.  This may need a comment that it's never
needed unless there's an inverter in that signal path.


> > +	if (res < 8)
> > +		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
> > +	if (res < 7)
> > +		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */

Maybe it's just the hardware I've used, but I've had a hard time
getting bitbanged SPI to run that fast ... more like 2.5 MHz unless
you're using big word sizes (and MMC is stuck at 8-bit words), and
that speed assumes inlined gpio code not subroutine-call-per-bit.

So I'd default to leaving the delay off, and wouldn't bother trying
to place a ceiling on the speeds which is lower than the 25 MHz that
SD cards allow.


> > +	if (res < 6)
> > +		pdata->p.mode = 0; /* Default: SPI mode 0 */

Don't need to do this.  The mmc_spi driver forces mode 0 in
any case...



> > --- linux-next.orig/drivers/mmc/host/Kconfig	2008-07-18 21:52:46.000000000 +0200
> > +++ linux-next/drivers/mmc/host/Kconfig	2008-07-18 21:57:08.000000000 +0200
> > @@ -164,3 +164,23 @@ config MMC_S3C
> >  
> >  	  If unsure, say N.
> >  
> > +config GPIOMMC
> > +	tristate "MMC/SD over GPIO-based SPI"

"Sysfs interface for ..."

Because we can already do MMC/SD over GPIO-SPI,
without using this code.


> > +	depends on MMC && MMC_SPI && SPI_GPIO
> > +	help
> > +	  This driver hooks up the mmc_spi and spi_gpio modules so that
> > +	  MMC/SD cards can be used on a GPIO based bus by bitbanging
> > +	  the SPI protocol in software.
> > +
> > +	  This driver provides a sysfs interface to dynamically create
> > +	  and destroy GPIO-based MMC/SD card interfaces. It also provides
> > +	  a platform device interface API.

Probably best to just switch those two paragraphs around,
and highlight that this SPI bus can't be used for anything
except that MMC/SD slot even if we someday extend the SPI
interfaces with exclusive access primitives.

The platform device interface API is not actually needed
to implement MMC/SD-over-bitbanged-SPI; it's only really
needed to implement this driver.  So I'd not mention the
presence of such a module-internal interface.


Now, if you were to implement the MMC protocol directly
over GPIOs, rather than indirectly through SPI-over-GPIO,
that would be worth packaging for general use.  (Such a
thing would be worth doing.  The latest JEDEC versions
of the "embedded MMC" specs -- parallel 8 bit data paths,
used for SMT chips not just cards -- omit SPI support.
Using platform-specific parallel GPIO support to support
4 or 8 bit data widths would give a big speedup too.)


> > +	  See Documentation/gpiommc.txt for details.
> > +
> > +	  The module will be called gpiommc.
> > +
> > +	  If unsure, say N.
> > +
> > +config MMC_S3C
> > +	tristate "Samsung S3C SD/MMC Card Interface support"
> > +	depends on ARCH_S3C2410 && MMC

MMC_S3C doesn't belong in this patch...



> > + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
> > + *                This is not standards compliant, but may be required for some
> > + *                embedded machines to gain reasonable speed.

Rather than emphasize "not standards compliant", I'd instead emphasize that
bitbanged SPI is so slow that you probably don't want to slow it down any
more by additional delays between per-bit operations!


> > Index: linux-next/Documentation/gpiommc.txt
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-next/Documentation/gpiommc.txt	2008-07-18 21:54:05.000000000 +0200

Should there be a  Documentation/mmc/... for such stuff, instead?
The toplevel Documentation directory is getting unmanageable.


> > @@ -0,0 +1,96 @@
> > +GPIOMMC - Driver for an MMC/SD card on a bitbanging GPIO SPI bus
> > +================================================================
> > +
> > +The gpiommc module hooks up the mmc_spi and spi_gpio modules for running an
> > +MMC or SD card on GPIO pins.
> > +
> > +Two interfaces for registering a new MMC/SD card device are provided.
> 
>                                                            are provided:
>    a static
> 
> > +A static platform-device based mechanism and a dynamic sysfs based interface.

Again, please don't promote a *SECOND* platform-device based mechanism
for this.  The current one works just fine:  standard definition of
a SPI bus (in this case using GPIO bitbanging), combined with standard
definition of an mmc-over-spi card slot.

(I notice you don't support card detect and writeprotect detect GPIOs
in your configfs interface.  Such omissions are easy to address through
the standard mmc-over-spi card slot mechanism ...)


> > +
> > +
> > +Registering devices via platform-device
> > +=======================================
> > +
> > +The platform-device interface is used for registering MMC/SD devices that are
> > +part of the hardware platform. This is most useful only for embedded machines
> > +with MMC/SD devices statically connected to the platform GPIO bus.

There is no "platform GPIO bus" ... 

This is the interface I'd rather just not see exposed ...

> > +The data structures are declared in <linux/mmc/gpiommc.h>

... and just see internal to this driver, without a new header
duplicating existing mechanisms.



> > +Registering devices via sysfs
> > +=============================

This is the interface I don't have any particular issues with.


> > +
> > +MMC/SD cards connected via GPIO often are a pretty dynamic thing. For example
> > +selfmade hacks for soldering an MMC/SD card to standard GPIO pins on embedded
> > +hardware are a common situation.
> 
> I think that the two sentences above/below would be better if joined with a comma...

I certainly wouldn't say "often", even though I've done it!

How about:  "... via GPIO may be easier to configure through
sysfs than through the normal platform device declaration
mechanisms.  For example, selfmade ... situation, and using
sysfs to verify the pin assignments may save a few iterations
of a kernel modify/build/install/test cycle."

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Randy Dunlap]

On Mon, 21 Jul 2008 13:41:35 -0700 David Brownell wrote:

> [ I see "v3" switches to configfs ... same comments apply. ]


> > > Index: linux-next/Documentation/gpiommc.txt
> > > ===================================================================
> > > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > > +++ linux-next/Documentation/gpiommc.txt	2008-07-18 21:54:05.000000000 +0200
> 
> Should there be a  Documentation/mmc/... for such stuff, instead?

What other files could be moved into that subdir?

> The toplevel Documentation directory is getting unmanageable.

Yes, it is.



---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Michael Buesch]

On Monday 21 July 2008 22:41:35 David Brownell wrote:
> So what this driver adds is mostly a userspace interface, yes?

Mostly. It also provides a simple platform device.

> If so, please refocus the descriptions a bit more on that;
> it's not actually a "GPIO based MMC/SD driver"; it's really
> a "Configfs creation interface for GPIO based MMC/SD".
> 
> 
> > > +	res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u %u %u %u %u %u %u %u",
> 
> Kind of ugly, yes?  ;)

This is already gone.

> Please switch to the more conventional "MOSI" (Master Out, Slave In)
> and "MISO" (Master In, Slave Out).  That's unambiguous.  If you want
> to be a bit more clear you could say that MOSI goes to the MMC/SD
> card pin 2 (card data in), and MISO goes to MMC/SD card pin 7 (card
> data out).

Maybe.

> > > +	if (res < 8)
> > > +		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
> > > +	if (res < 7)
> > > +		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */
> 

> So I'd default to leaving the delay off

How to?

> > > +	if (res < 6)
> > > +		pdata->p.mode = 0; /* Default: SPI mode 0 */
> 
> Don't need to do this.  The mmc_spi driver forces mode 0 in
> any case...

Where's that documented?

> > > +config GPIOMMC
> > > +	tristate "MMC/SD over GPIO-based SPI"
> 
> "Sysfs interface for ..."
> 
> Because we can already do MMC/SD over GPIO-SPI,
> without using this code.

This also adds a platform device interface for convenience
(as it also uses that internally).

> > > +config MMC_S3C
> > > +	tristate "Samsung S3C SD/MMC Card Interface support"
> > > +	depends on ARCH_S3C2410 && MMC
> 
> MMC_S3C doesn't belong in this patch...

Yeah that was a mismerge. You are commenting on an obsolete patch.

> > > + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
> > > + *                This is not standards compliant, but may be required for some
> > > + *                embedded machines to gain reasonable speed.
> 
> Rather than emphasize "not standards compliant", I'd instead emphasize that
> bitbanged SPI is so slow that you probably don't want to slow it down any
> more by additional delays between per-bit operations!

I'd say you cannot tell how fast the GPIOs are. They can be pretty fast
on a huge machine.

> Again, please don't promote a *SECOND* platform-device based mechanism

It is not a second mechanism, it is a mechanism on top of the first two
mechanisms.

> > > +Registering devices via platform-device
> > > +=======================================
> > > +
> > > +The platform-device interface is used for registering MMC/SD devices that are
> > > +part of the hardware platform. This is most useful only for embedded machines
> > > +with MMC/SD devices statically connected to the platform GPIO bus.
> 
> There is no "platform GPIO bus" ... 

hm??

> This is the interface I'd rather just not see exposed ...

People asked me to expose it. It was hidden in the first patch that
I submitted. I'm not going to change this just to see the next one
yelling "I want to see this interface in public" again.

> > > +Registering devices via sysfs
> > > +=============================
> 
> This is the interface I don't have any particular issues with.

Yeah, but it's gone. See updated patches.

-- 
Greetings Michael.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[David Brownell]

Hmm, I just now saw your reply ...

> On Monday 21 July 2008 22:41:35 David Brownell wrote:
> > So what this driver adds is mostly a userspace interface, yes?
> 
> Mostly. It also provides a simple platform device.

Yeah but I don't like interfaces like that ... which provide
less functional clones of existing interfaces.  If the goal is
convenience, then just wrap the existing ones together to give
the "one stop shopping" experience.

 
> > Please switch to the more conventional "MOSI" (Master Out, Slave In)
> > and "MISO" (Master In, Slave Out).  That's unambiguous.  If you want
> > to be a bit more clear you could say that MOSI goes to the MMC/SD
> > card pin 2 (card data in), and MISO goes to MMC/SD card pin 7 (card
> > data out).
> 
> Maybe.

Not clear what you mean.  You are using "DI" and "DO", which is quite
ambiguous (data in to card?  or data in to host?).  And you don't say
which direction is meant.  So that's ambigious ... just like "maybe".


> 
> > > > +	if (res < 8)
> > > > +		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
> > > > +	if (res < 7)
> > > > +		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */
> > 
> 
> > So I'd default to leaving the delay off
> 
> How to?

Simplest to just rip out the support for using one!  And in any case:
as a rule, any delay should be a function of the target bus speed.

If you wanted to get fancy you could measure how long it takes to issue a
non-inlined GPIO call, and solve for the delay:

	cycle time = 1/max_speed_hz
	cycle_time = 4 * call_cost + 2 * delay_len

One way to measure would be wrapping ktime_get_ts() calls around a loop that
toggles the SPI clock a few hundred times.


> > > > +	if (res < 6)
> > > > +		pdata->p.mode = 0; /* Default: SPI mode 0 */
> > 
> > Don't need to do this.  The mmc_spi driver forces mode 0 in
> > any case...
> 
> Where's that documented?

UTSL ... mmc_spi_probe(), first thing.  :)


> > > > +config GPIOMMC
> > > > +	tristate "MMC/SD over GPIO-based SPI"
> > 
> > "Sysfs interface for ..."
> > 
> > Because we can already do MMC/SD over GPIO-SPI,
> > without using this code.
> 
> This also adds a platform device interface for convenience
> (as it also uses that internally).

Don't export that.  It's a (less expressive) subset of the standard
way to do that.


> > > > + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
> > > > + *                This is not standards compliant, but may be required for some
> > > > + *                embedded machines to gain reasonable speed.
> > 
> > Rather than emphasize "not standards compliant", I'd instead emphasize that
> > bitbanged SPI is so slow that you probably don't want to slow it down any
> > more by additional delays between per-bit operations!
> 
> I'd say you cannot tell how fast the GPIOs are. They can be pretty fast
> on a huge machine.

Huge machine tends to mean lots of busses to traverse before getting to
where the actual I/O gets performed.  Like a PCI posted write, followed
by a read to make sure the write completed and the I/O was performed.

Regardless, the point isn't standards, it's platform performance ... which
is, in all cases I've used bitbanged SPI, at most 1 MHz when non-inlined
GPIO calls are used.  And many times faster with inlined calls that go
directly to the GPIO controller...


> > Again, please don't promote a *SECOND* platform-device based mechanism
> 
> It is not a second mechanism, it is a mechanism on top of the first two
> mechanisms.

There's already a way to define an MMC-over-SPI device.  This is another
one.  In what way would that not be a "second" mechanism??


> > > > +Registering devices via platform-device
> > > > +=======================================
> > > > +
> > > > +The platform-device interface is used for registering MMC/SD devices that are
> > > > +part of the hardware platform. This is most useful only for embedded machines
> > > > +with MMC/SD devices statically connected to the platform GPIO bus.
> > 
> > There is no "platform GPIO bus" ... 
> 
> hm??

If you look in /sys/bus you won't see GPIO.  It's not a bus.


> > This is the interface I'd rather just not see exposed ...
> 
> People asked me to expose it. It was hidden in the first patch that
> I submitted. I'm not going to change this just to see the next one
> yelling "I want to see this interface in public" again.

Could you send me some URLs for that feedback?  Again, I'm not keen
on adding a second (and less functional) way to do this.


> 
> > > > +Registering devices via sysfs
> > > > +=============================
> > 
> > This is the interface I don't have any particular issues with.
> 
> Yeah, but it's gone. See updated patches.

I meant "the userspace interface".  I did see the update patch
that changed it to use about configfs.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[David Brownell]

And a reminder why crossposting is frowned upon -- I removed openwrt from
the CC list here.  It's evidently not-so-openwrt.  ;)


> You are not allowed to post to this mailing list, and your message has
> been automatically rejected.  If you think that your messages are
> being rejected in error, contact the mailing list owner at
> openwrt-devel-owner@lists.openwrt.org.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Michael Buesch]

On Monday 28 July 2008 01:30:07 David Brownell wrote:
> > How to?
> 
> Simplest to just rip out the support for using one!  And in any case:
> as a rule, any delay should be a function of the target bus speed.

Well, that wasn't exactly an answer to my question. ;)
So setting boardinfo->max_speed_hz to zero will do?

> > Where's that documented?
> 
> UTSL ... mmc_spi_probe(), first thing.  :)

Well, the code of a function isn't really a documentation for its API. ;)
But well, thanks for the hint.

> Don't export that.  It's a (less expressive) subset of the standard
> way to do that.

Well, if we have that code in one place, why would we want to reinvent the
wheel over and over again? We can just export this API and let users use
it instead of requireing them to reinvent it.
I'm really really not too sure where your problem is with exporting
that API. If you don't want to use it, simply don't do so and reinvent
the wheel in your code. I'm prefectly fine with that. But IMO that's not
a good reason to hide the code from everybody else who wants to use it.

> > > 
> > > There is no "platform GPIO bus" ... 
> > 
> > hm??
> 
> If you look in /sys/bus you won't see GPIO.  It's not a bus.

Oh well... This is getting boring...
Then call it "platform GPIO pins" or "platform GPIO controller" or whatever.
That's really just splitting the hair into 16 parts.

> > People asked me to expose it. It was hidden in the first patch that
> > I submitted. I'm not going to change this just to see the next one
> > yelling "I want to see this interface in public" again.
> 
> Could you send me some URLs for that feedback?

It was private. One of the OpenWRT guys requested that to have a
convenient way to define a hardwired MMC card in the platform code
by just defining a simple data structure.

-- 
Greetings Michael.

Re: [PATCH v2] Add GPIO-based MMC/SD driver

[Piotr Skamruk]

2008/7/28 Michael Buesch <mb@bu3sch.de>:
> On Monday 28 July 2008 01:30:07 David Brownell wrote:
>> > How to?
>>
>> Simplest to just rip out the support for using one!  And in any case:
>> as a rule, any delay should be a function of the target bus speed.
>
> Well, that wasn't exactly an answer to my question. ;)
> So setting boardinfo->max_speed_hz to zero will do?
David wants fastest code (so it could run optimally on embedded
devices), and Yours version is more overview.
IMHO both approach are right, but in diffrent situations.
If i need fastest implementation, which i could use on some special
device, probably i would wrote machine dependent platform device, with
inlined gpio calls, because this would be faster.
But, when i want more generic interface, which could be used in more
diffrent situations - i would preferre Michaels approach.

>> > Where's that documented?
>>
>> UTSL ... mmc_spi_probe(), first thing.  :)
:)

> [...]
>> > > There is no "platform GPIO bus" ...
>> >
>> > hm??
>>
>> If you look in /sys/bus you won't see GPIO.  It's not a bus.
>
> Oh well... This is getting boring...
> Then call it "platform GPIO pins" or "platform GPIO controller" or whatever.
> That's really just splitting the hair into 16 parts.
Michael - ok, change this to "platform GPIO lines" :)

>> > People asked me to expose it. It was hidden in the first patch that
>> > I submitted. I'm not going to change this just to see the next one
>> > yelling "I want to see this interface in public" again.
>>
>> Could you send me some URLs for that feedback?
>
> It was private. One of the OpenWRT guys requested that to have a
> convenient way to define a hardwired MMC card in the platform code
> by just defining a simple data structure.
Maybe I was this one, ore one of them. I wanted some method to
experiment with MMC/SD connected to GPIO. Not some determined lines,
but in some way configurable. There was several ugly patches on net
(mostly around OpenWRT, and mostly tested by users of OpenWRT), but
all of them was ugly hacks, dependent on some particular architecture
(ARM as i remember).
So, in that time, simply I started writing something more general,
less architecture dependent, something, what could be more easy to
configure (in kernel/module loading time).
I started this, and Michael did the rest...