[RFC] OpenFirmware bindings for the MMC-over-SPI driver

[RFC] OpenFirmware bindings for the MMC-over-SPI driver

[Anton Vorontsov]

Hi all,

This is second attempt to write the OpenFirmware bindings for the
MMC-over-SPI (and SPI bindings in general). First attempt[1] was discussed
in the linuxppc-dev list only, since I tried to do the bindings without
touching the drivers them self. That attempt got a cool welcome, so here
is an alternative.

Grant, I'm somewhat sure you'll like it now... :-) As a plus, these
bindings doesn't require changes to the spi_of core...

[1] http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056727.html

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[RFC PATCH 1/2] mmc_spi: export probe and remove functions

[Anton Vorontsov]

...so we'll able to write bindings for the OpenFirmware without
messing with #ifdefs in the driver itself.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/mmc/host/mmc_spi.c |    6 ++++--
 drivers/mmc/host/mmc_spi.h |    7 +++++++
 2 files changed, 11 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/host/mmc_spi.h

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 85d9853..f2c4fc2 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1184,7 +1184,7 @@ static int maybe_count_child(struct device *dev, void *c)
 	return 0;
 }
 
-static int mmc_spi_probe(struct spi_device *spi)
+int mmc_spi_probe(struct spi_device *spi)
 {
 	void			*ones;
 	struct mmc_host		*mmc;
@@ -1366,9 +1366,10 @@ nomem:
 	kfree(ones);
 	return status;
 }
+EXPORT_SYMBOL(mmc_spi_probe);
 
 
-static int __devexit mmc_spi_remove(struct spi_device *spi)
+int __devexit mmc_spi_remove(struct spi_device *spi)
 {
 	struct mmc_host		*mmc = dev_get_drvdata(&spi->dev);
 	struct mmc_spi_host	*host;
@@ -1398,6 +1399,7 @@ static int __devexit mmc_spi_remove(struct spi_device *spi)
 	}
 	return 0;
 }
+EXPORT_SYMBOL(mmc_spi_remove);
 
 
 static struct spi_driver mmc_spi_driver = {
diff --git a/drivers/mmc/host/mmc_spi.h b/drivers/mmc/host/mmc_spi.h
new file mode 100644
index 0000000..d4fcd91
--- /dev/null
+++ b/drivers/mmc/host/mmc_spi.h
@@ -0,0 +1,7 @@
+#ifndef __DRIVERS_MMC_HOST_MMC_SPI_H
+#define __DRIVERS_MMC_HOST_MMC_SPI_H
+
+extern int mmc_spi_probe(struct spi_device *spi);
+extern int __devexit mmc_spi_remove(struct spi_device *spi);
+
+#endif
-- 
1.5.5.1

Re: [RFC PATCH 1/2] mmc_spi: export probe and remove functions

[Anton Vorontsov]

On Mon, May 26, 2008 at 02:18:36PM +0200, Pierre Ossman wrote:
> On Fri, 23 May 2008 22:28:34 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > ...so we'll able to write bindings for the OpenFirmware without
> > messing with #ifdefs in the driver itself.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> This looks extremely wrong. Encapsulating probe functions isn't exactly
> in line with the device model and bound to confuse people.
> 
> Your patches doesn't give a complete picture of the OF side of things,
> but can't you solve this by having an init callback somewhere?

Easily, I think this is good (better) idea. Will do.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [RFC PATCH 1/2] mmc_spi: export probe and remove functions

[Anton Vorontsov]

On Mon, May 26, 2008 at 04:25:33PM +0400, Anton Vorontsov wrote:
> On Mon, May 26, 2008 at 02:18:36PM +0200, Pierre Ossman wrote:
> > On Fri, 23 May 2008 22:28:34 +0400
> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > ...so we'll able to write bindings for the OpenFirmware without
> > > messing with #ifdefs in the driver itself.
> > > 
> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > 
> > This looks extremely wrong. Encapsulating probe functions isn't exactly
> > in line with the device model and bound to confuse people.

Btw, this isn't actually drivers encapsulating. This is about making
mmc_spi export some "library" function which could be used by other
bindings.

Think of usb_add_hcd() used by various drivers' bindings for e.g.
drivers/usb/host/ehci-*.c. Though usb_add_hcd() is more generic
than just "EHCI" bindings, but only because there is nothing to
share between them. (for MMC over SPI bindings all we want to do is fill
the platform data).

Another example is drivers/ata/pata_platform.c which exports "library"
function used by the drivers/ata/pata_of_platform.c. This is simply to
avoid code duplication across the bindings.

> > Your patches doesn't give a complete picture of the OF side of things,
> > but can't you solve this by having an init callback somewhere?
> 
> Easily, I think this is good (better) idea. Will do.

Maybe something like this? I don't like it so much, but given that
you don't like to export functions from mmc_spi, we'll have to place
some calls into the driver itself. :-/ And there is no easy way to do
generic callbacks, since that way we'll have implement "mmc_spi
callbacks subsystem". :-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index dead617..f468544 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -130,3 +130,10 @@ config MMC_SPI
 
 	  If unsure, or if your system has no SPI master driver, say N.
 
+config OF_MMC_SPI
+	tristate "MMC/SD over SPI OpenFirmware bindings"
+	depends on MMC_SPI && SPI_MASTER_OF && OF_GPIO
+	default y
+	help
+	  Say Y here to enable OpenFirmware bindings for the MMC/SD over SPI
+	  driver.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 3877c87..d77f880 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -17,4 +17,4 @@ obj-$(CONFIG_MMC_OMAP)		+= omap.o
 obj-$(CONFIG_MMC_AT91)		+= at91_mci.o
 obj-$(CONFIG_MMC_TIFM_SD)	+= tifm_sd.o
 obj-$(CONFIG_MMC_SPI)		+= mmc_spi.o
-
+obj-$(CONFIG_OF_MMC_SPI)	+= of_mmc_spi.o
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 85d9853..395dd77 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -40,6 +40,7 @@
 
 #include <asm/unaligned.h>
 
+#include "mmc_spi.h"
 
 /* NOTES:
  *
@@ -1191,6 +1192,10 @@ static int mmc_spi_probe(struct spi_device *spi)
 	struct mmc_spi_host	*host;
 	int			status;
 
+	status = of_mmc_spi_probe(spi);
+	if (status)
+		return status;
+
 	/* MMC and SD specs only seem to care that sampling is on the
 	 * rising edge ... meaning SPI modes 0 or 3.  So either SPI mode
 	 * should be legit.  We'll use mode 0 since it seems to be a
@@ -1364,6 +1369,7 @@ fail_nobuf1:
 
 nomem:
 	kfree(ones);
+	of_mmc_spi_remove(spi);
 	return status;
 }
 
@@ -1395,6 +1401,7 @@ static int __devexit mmc_spi_remove(struct spi_device *spi)
 		spi->max_speed_hz = mmc->f_max;
 		mmc_free_host(mmc);
 		dev_set_drvdata(&spi->dev, NULL);
+		of_mmc_spi_remove(spi);
 	}
 	return 0;
 }
diff --git a/drivers/mmc/host/mmc_spi.h b/drivers/mmc/host/mmc_spi.h
new file mode 100644
index 0000000..a780761
--- /dev/null
+++ b/drivers/mmc/host/mmc_spi.h
@@ -0,0 +1,18 @@
+#ifndef __DRIVERS_MMC_HOST_MMC_SPI_H
+#define __DRIVERS_MMC_HOST_MMC_SPI_H
+
+#ifdef CONFIG_OF_MMC_SPI
+extern int of_mmc_spi_probe(struct spi_device *spi);
+extern int __devexit of_mmc_spi_remove(struct spi_device *spi);
+#else
+static inline int of_mmc_spi_probe(struct spi_device *spi)
+{
+	return 0;
+}
+static inline int __devexit of_mmc_spi_remove(struct spi_device *spi)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MMC_SPI */
+
+#endif /* __DRIVERS_MMC_HOST_MMC_SPI_H */
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
new file mode 100644
index 0000000..7512f8b
--- /dev/null
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -0,0 +1,135 @@
+/*
+ * OpenFirmware bindings for the MMC-over-SPI driver
+ *
+ * Copyright (c) MontaVista Software, Inc. 2008.
+ *
+ * Author: Anton Vorontsov <avorontsov@ru.mvista.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, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_of.h>
+#include <linux/spi/mmc_spi.h>
+#include <linux/mmc/host.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include "mmc_spi.h"
+
+struct of_mmc_spi {
+	int wp_gpio;
+	int cd_gpio;
+	struct mmc_spi_platform_data mmc_pdata;
+};
+
+static int mmc_get_ro(struct device *dev)
+{
+	struct of_mmc_spi *oms = dev->archdata.of_node->data;
+
+	return gpio_get_value(oms->wp_gpio);
+}
+
+static int mmc_get_cd(struct device *dev)
+{
+	struct of_mmc_spi *oms = dev->archdata.of_node->data;
+
+	return gpio_get_value(oms->cd_gpio);
+}
+
+int of_mmc_spi_probe(struct spi_device *spi)
+{
+	int ret = -EINVAL;
+	struct device_node *np = spi->dev.archdata.of_node;
+	struct device *dev = &spi->dev;
+	struct of_mmc_spi *oms;
+	const u32 *ocr_mask;
+	int size;
+
+	/* not an OF SPI device, this isn't error though */
+	if (!np)
+		return 0;
+
+	oms = kzalloc(sizeof(*oms), GFP_KERNEL);
+	if (!oms)
+		return -ENOMEM;
+
+	/* Somebody occupied node's data? */
+	WARN_ON(np->data);
+
+	/*
+	 * mmc_spi_probe will use drvdata, so we can't use it. Use node's
+	 * data instead.
+	 */
+	np->data = oms;
+
+	/* We don't support interrupts yet, let's poll. */
+	oms->mmc_pdata.caps |= MMC_CAP_NEEDS_POLL;
+
+	ocr_mask = of_get_property(np, "linux,mmc-ocr-mask", &size);
+	if (ocr_mask && size >= sizeof(ocr_mask))
+		oms->mmc_pdata.ocr_mask = *ocr_mask;
+
+	oms->wp_gpio = of_get_gpio(np, 0);
+	if (gpio_is_valid(oms->wp_gpio)) {
+		ret = gpio_request(oms->wp_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_wp_gpio;
+		oms->mmc_pdata.get_ro = &mmc_get_ro;
+	}
+
+	oms->cd_gpio = of_get_gpio(np, 1);
+	if (gpio_is_valid(oms->cd_gpio)) {
+		ret = gpio_request(oms->cd_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_cd_gpio;
+		oms->mmc_pdata.get_cd = &mmc_get_cd;
+	}
+
+	spi->dev.platform_data = &oms->mmc_pdata;
+
+	return 0;
+
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+err_cd_gpio:
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+err_wp_gpio:
+	np->data = NULL;
+	kfree(oms);
+	return ret;
+}
+EXPORT_SYMBOL(of_mmc_spi_probe);
+
+int __devexit of_mmc_spi_remove(struct spi_device *spi)
+{
+	struct device_node *np = spi->dev.archdata.of_node;
+	struct of_mmc_spi *oms;
+
+	/* not an OF SPI device, this isn't error though */
+	if (!np)
+		return 0;
+
+	oms = np->data;
+
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+
+	spi->dev.archdata.of_node->data = NULL;
+	kfree(oms);
+	return 0;
+}
+EXPORT_SYMBOL(of_mmc_spi_remove);
+
+MODULE_DESCRIPTION("OpenFirmware bindings for the MMC-over-SPI driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.5.1

Re: [RFC PATCH 1/2] mmc_spi: export probe and remove functions

[Anton Vorontsov]

On Sun, Jun 01, 2008 at 12:18:41PM +0200, Pierre Ossman wrote:
> On Mon, 26 May 2008 17:10:09 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > 
> > Btw, this isn't actually drivers encapsulating. This is about making
> > mmc_spi export some "library" function which could be used by other
> > bindings.
> > 
> > Think of usb_add_hcd() used by various drivers' bindings for e.g.
> > drivers/usb/host/ehci-*.c. Though usb_add_hcd() is more generic
> > than just "EHCI" bindings, but only because there is nothing to
> > share between them. (for MMC over SPI bindings all we want to do is fill
> > the platform data).
> > 
> 
> There's a big difference.

This depends on the perception. :-)

> usb_add_hcd() is designed specifically to be called by other, real probe
> functions.

Yes, by convention (or better, by design).

> mmc_spi_probe() _is_ a probe function. 

Yes, so far.

> Also exporting it as a library function is very confusing.

No, if designed/documented properly.

Just imagine this (100% similarity to USB code):

mmc_spi_create_hcd(&mmc_spi_driver, dev, dev->bus_id);
mmc_spi_add_hcd(dev, irq, irqflags);

> > Maybe something like this? I don't like it so much, but given that
> > you don't like to export functions from mmc_spi, we'll have to place
> > some calls into the driver itself. :-/ And there is no easy way to do
> > generic callbacks, since that way we'll have implement "mmc_spi
> > callbacks subsystem". :-)
> 
> That's not a callback, but an explicit call to another module.
> 
> 
> All of this work looks a bit like trying to wedge a square piece into a
> round hole. It looks to me that the kernel needs a bit of restructuring
> to handle it. You can't really export every probe function of every
> platform device so that you can add OF hooks to it.
> 
> From what I can tell, the OF stuff behaves very much like the PNP
> system on PCs. The information relayed is a bit more versatile though.
> Perhaps what is needed is a more advanced "platform" bus that is
> modeled after the PNP bus, but with the extra ability of handling the
> stuff currently crammed into the platform structures. mmc_spi would
> then be extended to be driver for the "platform" bus and we could have
> generic calls like platform_get_pin(dev, "ro");.

platform_get_pin()? Um, maybe platform_get_gpio(), as _irq()? Yes,
this is doable (and someday this should be done). But this way we can
pass only GPIOs and then teach mmc_spi to work with them directly
(in addition to callbacks).

But this is not enough, there is still no way to pass real platform data,
such as: caps and ocr_mask. Any idea how to deliver these?


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

This patch depends on the Grant Likely's SPI patches, so this is
for RFC only.

Also, later we'll able to remove OF_GPIO dependency.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |   24 ++++
 drivers/mmc/host/Kconfig                     |    7 ++
 drivers/mmc/host/Makefile                    |    2 +-
 drivers/mmc/host/of_mmc_spi.c                |  151 ++++++++++++++++++++++++++
 4 files changed, 183 insertions(+), 1 deletions(-)
 create mode 100644 drivers/mmc/host/of_mmc_spi.c

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 423cb2b..7d0ef80 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -3151,7 +3151,31 @@ platforms are moved over to use the flattened-device-tree model.
 			};
 		};
 
+    ...) MMC-over-SPI
 
+      Required properties:
+      - #address-cells : should be 0.
+      - #size-cells : should be 0.
+      - compatible : should be "linux,mmc-spi".
+      - linux,modalias  - should be "of_mmc_spi".
+      - reg : should specify SPI address (chip-select number).
+      - max-speed : (optional) maximum frequency for this device (Hz).
+      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
+        (slot voltage).
+      - gpios : (optional) may specify GPIOs in this order: Write-Protect GPIO,
+        Card-Detect GPIO.
+
+      Example:
+
+	mmc-slot@0 {
+		compatible = "linux,mmc-spi";
+		linux,modalias = "of_mmc_spi";
+		reg = <0>;
+		max-speed = <50000000>;
+		linux,mmc-ocr-mask = <0x00200000>;
+		gpios = <&sdcsr_pio 6 0   /*  WP */
+			 &sdcsr_pio 7 1>; /* nCD */
+	};
 
 VII - Marvell Discovery mv64[345]6x System Controller chips
 ===========================================================
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index dead617..f468544 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -130,3 +130,10 @@ config MMC_SPI
 
 	  If unsure, or if your system has no SPI master driver, say N.
 
+config OF_MMC_SPI
+	tristate "MMC/SD over SPI OpenFirmware bindings"
+	depends on MMC_SPI && SPI_MASTER_OF && OF_GPIO
+	default y
+	help
+	  Say Y here to enable OpenFirmware bindings for the MMC/SD over SPI
+	  driver.
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 3877c87..d77f880 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -17,4 +17,4 @@ obj-$(CONFIG_MMC_OMAP)		+= omap.o
 obj-$(CONFIG_MMC_AT91)		+= at91_mci.o
 obj-$(CONFIG_MMC_TIFM_SD)	+= tifm_sd.o
 obj-$(CONFIG_MMC_SPI)		+= mmc_spi.o
-
+obj-$(CONFIG_OF_MMC_SPI)	+= of_mmc_spi.o
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
new file mode 100644
index 0000000..b8211ce
--- /dev/null
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -0,0 +1,151 @@
+/*
+ * OpenFirmware bindings for the MMC-over-SPI driver
+ *
+ * Copyright (c) MontaVista Software, Inc. 2008.
+ *
+ * Author: Anton Vorontsov <avorontsov@ru.mvista.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, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of_platform.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_of.h>
+#include <linux/spi/mmc_spi.h>
+#include <linux/mmc/host.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include "mmc_spi.h"
+
+struct of_mmc_spi {
+	int wp_gpio;
+	int cd_gpio;
+	struct mmc_spi_platform_data mmc_pdata;
+};
+
+static int mmc_get_ro(struct device *dev)
+{
+	struct of_mmc_spi *oms = dev->archdata.of_node->data;
+
+	return gpio_get_value(oms->wp_gpio);
+}
+
+static int mmc_get_cd(struct device *dev)
+{
+	struct of_mmc_spi *oms = dev->archdata.of_node->data;
+
+	return gpio_get_value(oms->cd_gpio);
+}
+
+static int of_mmc_spi_probe(struct spi_device *spi)
+{
+	int ret = -EINVAL;
+	struct device_node *np = spi->dev.archdata.of_node;
+	struct device *dev = &spi->dev;
+	struct of_mmc_spi *oms = kzalloc(sizeof(*oms), GFP_KERNEL);
+	const u32 *ocr_mask;
+	int size;
+
+	if (!oms)
+		return -ENOMEM;
+
+	/* Somebody occupied node's data? */
+	WARN_ON(spi->dev.archdata.of_node->data);
+
+	/*
+	 * mmc_spi_probe will use drvdata, so we can't use it. Use node's
+	 * data instead.
+	 */
+	spi->dev.archdata.of_node->data = oms;
+
+	/* We don't support interrupts yet, let's poll. */
+	oms->mmc_pdata.caps |= MMC_CAP_NEEDS_POLL;
+
+	ocr_mask = of_get_property(np, "linux,mmc-ocr-mask", &size);
+	if (ocr_mask && size >= sizeof(ocr_mask))
+		oms->mmc_pdata.ocr_mask = *ocr_mask;
+
+	oms->wp_gpio = of_get_gpio(np, 0);
+	if (gpio_is_valid(oms->wp_gpio)) {
+		ret = gpio_request(oms->wp_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_wp_gpio;
+		oms->mmc_pdata.get_ro = &mmc_get_ro;
+	}
+
+	oms->cd_gpio = of_get_gpio(np, 1);
+	if (gpio_is_valid(oms->cd_gpio)) {
+		ret = gpio_request(oms->cd_gpio, dev->bus_id);
+		if (ret < 0)
+			goto err_cd_gpio;
+		oms->mmc_pdata.get_cd = &mmc_get_cd;
+	}
+
+	spi->dev.platform_data = &oms->mmc_pdata;
+
+	ret = mmc_spi_probe(spi);
+	if (ret)
+		goto err_mmc_spi;
+
+	return 0;
+err_mmc_spi:
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+err_cd_gpio:
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+err_wp_gpio:
+	spi->dev.archdata.of_node->data = NULL;
+	kfree(oms);
+	return ret;
+}
+
+int __devexit of_mmc_spi_remove(struct spi_device *spi)
+{
+	struct of_mmc_spi *oms = spi->dev.archdata.of_node->data;
+	int ret;
+
+	ret = mmc_spi_remove(spi);
+	if (ret)
+		return ret;
+
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+
+	spi->dev.archdata.of_node->data = NULL;
+	kfree(oms);
+	return 0;
+}
+
+static struct spi_driver of_mmc_spi_driver = {
+	.driver = {
+		.name = "of_mmc_spi",
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+	.probe = of_mmc_spi_probe,
+	.remove = __devexit_p(of_mmc_spi_remove),
+};
+
+static int __init of_mmc_spi_init(void)
+{
+	return spi_register_driver(&of_mmc_spi_driver);
+}
+module_init(of_mmc_spi_init);
+
+static void __exit of_mmc_spi_exit(void)
+{
+	spi_unregister_driver(&of_mmc_spi_driver);
+}
+module_exit(of_mmc_spi_exit);
+
+MODULE_DESCRIPTION("OpenFirmware bindings for the MMC-over-SPI driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.5.1

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

Hi Anton,

On Fri, 23 May 2008 22:28:42 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +++ b/drivers/mmc/host/of_mmc_spi.c

> +static int of_mmc_spi_probe(struct spi_device *spi)

> +	/*
> +	 * mmc_spi_probe will use drvdata, so we can't use it. Use node's
> +	 * data instead.
> +	 */
> +	spi->dev.archdata.of_node->data = oms;

If you delay this assignment, you may not have to clean it up in the
error path.

> +	ocr_mask = of_get_property(np, "linux,mmc-ocr-mask", &size);

You should really explicitly include linux/of.h to use of_get_property etc

> +int __devexit of_mmc_spi_remove(struct spi_device *spi)
> +{
> +	struct of_mmc_spi *oms = spi->dev.archdata.of_node->data;
> +	int ret;
> +
> +	ret = mmc_spi_remove(spi);
> +	if (ret)
> +		return ret;

Maybe you should do this last so that you don't leak more than necessary
if mmc_spi_remove fails.  (I don't know what state mmc_spi_remove leaves
stuff in if it fails ...)

> +static struct spi_driver of_mmc_spi_driver = {
> +	.driver = {
> +		.name = "of_mmc_spi",
> +		.bus = &spi_bus_type,

This is initialised by spi_register_driver().

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Sat, May 24, 2008 at 12:35:27PM +1000, Stephen Rothwell wrote:
> Hi Anton,
> 
> On Fri, 23 May 2008 22:28:42 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> >
> > +++ b/drivers/mmc/host/of_mmc_spi.c
> 
> > +static int of_mmc_spi_probe(struct spi_device *spi)
> 
> > +	/*
> > +	 * mmc_spi_probe will use drvdata, so we can't use it. Use node's
> > +	 * data instead.
> > +	 */
> > +	spi->dev.archdata.of_node->data = oms;
> 
> If you delay this assignment, you may not have to clean it up in the
> error path.

No, if I delay it I'll have to clean it up anyway, just a bit later
(after mmc_spi_probe() fail).

> > +	ocr_mask = of_get_property(np, "linux,mmc-ocr-mask", &size);
> 
> You should really explicitly include linux/of.h to use of_get_property etc

Thanks, will do.

> > +int __devexit of_mmc_spi_remove(struct spi_device *spi)
> > +{
> > +	struct of_mmc_spi *oms = spi->dev.archdata.of_node->data;
> > +	int ret;
> > +
> > +	ret = mmc_spi_remove(spi);
> > +	if (ret)
> > +		return ret;
> 
> Maybe you should do this last so that you don't leak more than necessary
> if mmc_spi_remove fails. (I don't know what state mmc_spi_remove leaves
> stuff in if it fails ...)

If I do this last, then of_mmc_spi_remove() would be not safe to call
multiple times on non-fatal errors (e.g. when mmc_spi_remove() returns
-EBUSY (despite that currently mmc_spi_remove can't fail)).

Also, if I do this last, then there will be a potential use-after-free
in the mmc_spi driver.

> > +static struct spi_driver of_mmc_spi_driver = {
> > +	.driver = {
> > +		.name = "of_mmc_spi",
> > +		.bus = &spi_bus_type,
> 
> This is initialised by spi_register_driver().

This was straight copy from the mmc_spi.c. Will fix.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

Yup, I like this approach better.  I had been thinking about putting
this all in the same file (drivers/mmc/host/mmc_spi.c) instead of
exporting the probe/remove symbols and by using clear comment blocks
to divide the sections, but I've got no issues with this approach.

This is good work.  Some comments below.  (I won't repeat Stephen's comments)

On Fri, May 23, 2008 at 12:28 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch depends on the Grant Likely's SPI patches, so this is
> for RFC only.
>
> Also, later we'll able to remove OF_GPIO dependency.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Documentation/powerpc/booting-without-of.txt |   24 ++++
>  drivers/mmc/host/Kconfig                     |    7 ++
>  drivers/mmc/host/Makefile                    |    2 +-
>  drivers/mmc/host/of_mmc_spi.c                |  151 ++++++++++++++++++++++++++
>  4 files changed, 183 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/mmc/host/of_mmc_spi.c
>
> diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> index 423cb2b..7d0ef80 100644
> --- a/Documentation/powerpc/booting-without-of.txt
> +++ b/Documentation/powerpc/booting-without-of.txt
> @@ -3151,7 +3151,31 @@ platforms are moved over to use the flattened-device-tree model.
>                        };
>                };
>
> +    ...) MMC-over-SPI
>
> +      Required properties:
> +      - #address-cells : should be 0.
> +      - #size-cells : should be 0.

Are these properties required at all?  Will this node have any children.

> +      - compatible : should be "linux,mmc-spi".
> +      - linux,modalias  - should be "of_mmc_spi".

I'm not even sure if the whole linux,modalias is even a good idea.  I
had kind of thrown it in there as a convenient way to override
compatible when needed, but I haven't really thought it out very well
and I think it is rather a hack.

The real problem is we don't yet have good method (or place) to apply
a translation table from compatible values to modaliases.  Ideally,
the translations should be part of the drivers themselves, but that
causes a chicken and egg problem of needing to load the driver to get
access to the table to know if it is the correct driver... Of course,
I'm really not very familiar with the whole module autoloading
mechanism.  Regardless; binding should be based on compatible, not on
a hacky and bogus linux,modalias property.

> +      - reg : should specify SPI address (chip-select number).
> +      - max-speed : (optional) maximum frequency for this device (Hz).
> +      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
> +        (slot voltage).

Should this property be better defined?

> +      - gpios : (optional) may specify GPIOs in this order: Write-Protect GPIO,
> +        Card-Detect GPIO.
> +
> +      Example:
> +
> +       mmc-slot@0 {
> +               compatible = "linux,mmc-spi";
> +               linux,modalias = "of_mmc_spi";
> +               reg = <0>;
> +               max-speed = <50000000>;
> +               linux,mmc-ocr-mask = <0x00200000>;
> +               gpios = <&sdcsr_pio 6 0   /*  WP */
> +                        &sdcsr_pio 7 1>; /* nCD */
> +       };
>
>  VII - Marvell Discovery mv64[345]6x System Controller chips
>  ===========================================================
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index dead617..f468544 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -130,3 +130,10 @@ config MMC_SPI
>
>          If unsure, or if your system has no SPI master driver, say N.
>
> +config OF_MMC_SPI
> +       tristate "MMC/SD over SPI OpenFirmware bindings"
> +       depends on MMC_SPI && SPI_MASTER_OF && OF_GPIO
> +       default y
> +       help
> +         Say Y here to enable OpenFirmware bindings for the MMC/SD over SPI
> +         driver.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 3877c87..d77f880 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -17,4 +17,4 @@ obj-$(CONFIG_MMC_OMAP)                += omap.o
>  obj-$(CONFIG_MMC_AT91)         += at91_mci.o
>  obj-$(CONFIG_MMC_TIFM_SD)      += tifm_sd.o
>  obj-$(CONFIG_MMC_SPI)          += mmc_spi.o
> -
> +obj-$(CONFIG_OF_MMC_SPI)       += of_mmc_spi.o
> diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
> new file mode 100644
> index 0000000..b8211ce
> --- /dev/null
> +++ b/drivers/mmc/host/of_mmc_spi.c
> @@ -0,0 +1,151 @@
> +/*
> + * OpenFirmware bindings for the MMC-over-SPI driver
> + *
> + * Copyright (c) MontaVista Software, Inc. 2008.
> + *
> + * Author: Anton Vorontsov <avorontsov@ru.mvista.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, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of_platform.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi_of.h>
> +#include <linux/spi/mmc_spi.h>
> +#include <linux/mmc/host.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include "mmc_spi.h"
> +
> +struct of_mmc_spi {
> +       int wp_gpio;
> +       int cd_gpio;
> +       struct mmc_spi_platform_data mmc_pdata;
> +};
> +
> +static int mmc_get_ro(struct device *dev)
> +{
> +       struct of_mmc_spi *oms = dev->archdata.of_node->data;
> +
> +       return gpio_get_value(oms->wp_gpio);
> +}
> +
> +static int mmc_get_cd(struct device *dev)
> +{
> +       struct of_mmc_spi *oms = dev->archdata.of_node->data;
> +
> +       return gpio_get_value(oms->cd_gpio);
> +}
> +
> +static int of_mmc_spi_probe(struct spi_device *spi)
> +{
> +       int ret = -EINVAL;
> +       struct device_node *np = spi->dev.archdata.of_node;
> +       struct device *dev = &spi->dev;
> +       struct of_mmc_spi *oms = kzalloc(sizeof(*oms), GFP_KERNEL);
> +       const u32 *ocr_mask;
> +       int size;
> +
> +       if (!oms)
> +               return -ENOMEM;
> +
> +       /* Somebody occupied node's data? */
> +       WARN_ON(spi->dev.archdata.of_node->data);

Perhaps bail at this point to avoid corruption?  Would it be better to
use container_of() instead for getting a pointer back to the private
structure from the pdata pointer?

> +
> +       /*
> +        * mmc_spi_probe will use drvdata, so we can't use it. Use node's
> +        * data instead.
> +        */
> +       spi->dev.archdata.of_node->data = oms;
> +
> +       /* We don't support interrupts yet, let's poll. */
> +       oms->mmc_pdata.caps |= MMC_CAP_NEEDS_POLL;
> +
> +       ocr_mask = of_get_property(np, "linux,mmc-ocr-mask", &size);
> +       if (ocr_mask && size >= sizeof(ocr_mask))
> +               oms->mmc_pdata.ocr_mask = *ocr_mask;
> +
> +       oms->wp_gpio = of_get_gpio(np, 0);
> +       if (gpio_is_valid(oms->wp_gpio)) {
> +               ret = gpio_request(oms->wp_gpio, dev->bus_id);
> +               if (ret < 0)
> +                       goto err_wp_gpio;
> +               oms->mmc_pdata.get_ro = &mmc_get_ro;
> +       }
> +
> +       oms->cd_gpio = of_get_gpio(np, 1);
> +       if (gpio_is_valid(oms->cd_gpio)) {
> +               ret = gpio_request(oms->cd_gpio, dev->bus_id);
> +               if (ret < 0)
> +                       goto err_cd_gpio;
> +               oms->mmc_pdata.get_cd = &mmc_get_cd;
> +       }
> +
> +       spi->dev.platform_data = &oms->mmc_pdata;
> +
> +       ret = mmc_spi_probe(spi);
> +       if (ret)
> +               goto err_mmc_spi;
> +
> +       return 0;
> +err_mmc_spi:
> +       if (gpio_is_valid(oms->cd_gpio))
> +               gpio_free(oms->cd_gpio);
> +err_cd_gpio:
> +       if (gpio_is_valid(oms->wp_gpio))
> +               gpio_free(oms->wp_gpio);
> +err_wp_gpio:
> +       spi->dev.archdata.of_node->data = NULL;
> +       kfree(oms);
> +       return ret;
> +}
> +
> +int __devexit of_mmc_spi_remove(struct spi_device *spi)
> +{
> +       struct of_mmc_spi *oms = spi->dev.archdata.of_node->data;
> +       int ret;
> +
> +       ret = mmc_spi_remove(spi);
> +       if (ret)
> +               return ret;
> +
> +       if (gpio_is_valid(oms->cd_gpio))
> +               gpio_free(oms->cd_gpio);
> +       if (gpio_is_valid(oms->wp_gpio))
> +               gpio_free(oms->wp_gpio);
> +
> +       spi->dev.archdata.of_node->data = NULL;
> +       kfree(oms);
> +       return 0;
> +}
> +
> +static struct spi_driver of_mmc_spi_driver = {
> +       .driver = {
> +               .name = "of_mmc_spi",
> +               .bus = &spi_bus_type,
> +               .owner = THIS_MODULE,
> +       },
> +       .probe = of_mmc_spi_probe,
> +       .remove = __devexit_p(of_mmc_spi_remove),
> +};
> +
> +static int __init of_mmc_spi_init(void)
> +{
> +       return spi_register_driver(&of_mmc_spi_driver);
> +}
> +module_init(of_mmc_spi_init);
> +
> +static void __exit of_mmc_spi_exit(void)
> +{
> +       spi_unregister_driver(&of_mmc_spi_driver);
> +}
> +module_exit(of_mmc_spi_exit);
> +
> +MODULE_DESCRIPTION("OpenFirmware bindings for the MMC-over-SPI driver");
> +MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
> +MODULE_LICENSE("GPL");
> --
> 1.5.5.1
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Jochen Friedrich]

Hi Grant,

>> +      - compatible : should be "linux,mmc-spi".
>> +      - linux,modalias  - should be "of_mmc_spi".
> 
> I'm not even sure if the whole linux,modalias is even a good idea.  I
> had kind of thrown it in there as a convenient way to override
> compatible when needed, but I haven't really thought it out very well
> and I think it is rather a hack.
> 
> The real problem is we don't yet have good method (or place) to apply
> a translation table from compatible values to modaliases.  Ideally,
> the translations should be part of the drivers themselves, but that
> causes a chicken and egg problem of needing to load the driver to get
> access to the table to know if it is the correct driver... Of course,
> I'm really not very familiar with the whole module autoloading
> mechanism.  Regardless; binding should be based on compatible, not on
> a hacky and bogus linux,modalias property.

i2c exactly has the same problem. Here the compatible entry is used
in drivers/of/of_i2c.c and mangled into a name to be used as modalias.
It's still sort of hackish, but it seems to be a compromise acceptable
by both OF and i2c folks.

Thanks,
Jochen

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Segher Boessenkool]

>> The real problem is we don't yet have good method (or place) to apply
>> a translation table from compatible values to modaliases.  Ideally,
>> the translations should be part of the drivers themselves, but that
>> causes a chicken and egg problem of needing to load the driver to get
>> access to the table to know if it is the correct driver... Of course,
>> I'm really not very familiar with the whole module autoloading
>> mechanism.  Regardless; binding should be based on compatible, not on
>> a hacky and bogus linux,modalias property.
>
> i2c exactly has the same problem. Here the compatible entry is used
> in drivers/of/of_i2c.c and mangled into a name to be used as modalias.
> It's still sort of hackish, but it seems to be a compromise acceptable
> by both OF and i2c folks.

It's perfectly acceptable.  The sole purpose of "compatible" is for
a client (i.e., the kernel) to decide what to do with this device;
most often, to decide what driver to use.

It would be nice to have a completely generic thing that matches device
tree nodes to drivers, and it sounds perfectly reasonable to me to go
via modalias for that (i.e., just translate from device tree namespace
to the kernel driver namespace).

As a bonus, it would make driver matching more correct in some places:
currently, whatever driver matches first wins (i.e., which "compatible"
value is tried first), but the ordering of different values in 
"compatible"
is supposed to be significant (whatever is mentioned there first should
win).


Segher

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Segher Boessenkool]

>> +      Required properties:
>> +      - #address-cells : should be 0.
>> +      - #size-cells : should be 0.
>
> Are these properties required at all?  Will this node have any 
> children.

You mean, "does this node define a bus".  If it doesn't, there
shouldn't be #a and #s; if it does, the binding should describe
what the addressing is.  #a = #s = 0 describes a bus without any
addressing; pretty unusual :-)

>> +      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
>> +        (slot voltage).
>
> Should this property be better defined?

Yes please.  There's a good chance that it doesn't turn out to be
Linux-specific at all (after some modification, perhaps).


Segher

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Fri, May 23, 2008 at 11:19:28PM -0600, Grant Likely wrote:
> Yup, I like this approach better.  I had been thinking about putting
> this all in the same file (drivers/mmc/host/mmc_spi.c) instead of
> exporting the probe/remove symbols and by using clear comment blocks
> to divide the sections, but I've got no issues with this approach.
> 
> This is good work.  Some comments below.  (I won't repeat Stephen's comments)

Thanks!

> On Fri, May 23, 2008 at 12:28 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > This patch depends on the Grant Likely's SPI patches, so this is
> > for RFC only.
> >
> > Also, later we'll able to remove OF_GPIO dependency.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  Documentation/powerpc/booting-without-of.txt |   24 ++++
> >  drivers/mmc/host/Kconfig                     |    7 ++
> >  drivers/mmc/host/Makefile                    |    2 +-
> >  drivers/mmc/host/of_mmc_spi.c                |  151 ++++++++++++++++++++++++++
> >  4 files changed, 183 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/mmc/host/of_mmc_spi.c
> >
> > diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> > index 423cb2b..7d0ef80 100644
> > --- a/Documentation/powerpc/booting-without-of.txt
> > +++ b/Documentation/powerpc/booting-without-of.txt
> > @@ -3151,7 +3151,31 @@ platforms are moved over to use the flattened-device-tree model.
> >                        };
> >                };
> >
> > +    ...) MMC-over-SPI
> >
> > +      Required properties:
> > +      - #address-cells : should be 0.
> > +      - #size-cells : should be 0.
> 
> Are these properties required at all?  Will this node have any children.

Yeah, my weakness for #a/#s, I always insert it where unnecessary. :-)
Will fix.

> > +      - compatible : should be "linux,mmc-spi".
> > +      - linux,modalias  - should be "of_mmc_spi".
> 
> I'm not even sure if the whole linux,modalias is even a good idea.  I
> had kind of thrown it in there as a convenient way to override
> compatible when needed, but I haven't really thought it out very well
> and I think it is rather a hack.
> 
> The real problem is we don't yet have good method (or place) to apply
> a translation table from compatible values to modaliases.  Ideally,
> the translations should be part of the drivers themselves, but that
> causes a chicken and egg problem of needing to load the driver to get
> access to the table to know if it is the correct driver... Of course,
> I'm really not very familiar with the whole module autoloading
> mechanism.  Regardless; binding should be based on compatible, not on
> a hacky and bogus linux,modalias property.

I fully agree. Though, I just tried to use your spi_of with a belief
that you'll implement compatible->modalias translation in spi_of. :-)

> > +      - reg : should specify SPI address (chip-select number).
> > +      - max-speed : (optional) maximum frequency for this device (Hz).
> > +      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
> > +        (slot voltage).
> 
> Should this property be better defined?

Yes, will do. Was a bit lazy to do this for the RFC.

[...]
> > +static int mmc_get_cd(struct device *dev)
> > +{
> > +       struct of_mmc_spi *oms = dev->archdata.of_node->data;
> > +
> > +       return gpio_get_value(oms->cd_gpio);
> > +}
> > +
> > +static int of_mmc_spi_probe(struct spi_device *spi)
> > +{
> > +       int ret = -EINVAL;
> > +       struct device_node *np = spi->dev.archdata.of_node;
> > +       struct device *dev = &spi->dev;
> > +       struct of_mmc_spi *oms = kzalloc(sizeof(*oms), GFP_KERNEL);
> > +       const u32 *ocr_mask;
> > +       int size;
> > +
> > +       if (!oms)
> > +               return -ENOMEM;
> > +
> > +       /* Somebody occupied node's data? */
> > +       WARN_ON(spi->dev.archdata.of_node->data);
> 
> Perhaps bail at this point to avoid corruption?  Would it be better to
> use container_of() instead for getting a pointer back to the private
> structure from the pdata pointer?

Using platform_data with container_of isn't always safe (e.g. for
platform bus we can't use it, this is done so that board's platform_data
definitions could be made __initdata).

David, would you [not] suggest us to use platform_data with
container_of, that is, are there any plans to make SPI core copy
the platform_data instead of passing a pointer directly?

> > +
> > +       /*
> > +        * mmc_spi_probe will use drvdata, so we can't use it. Use node's
> > +        * data instead.
> > +        */
> > +       spi->dev.archdata.of_node->data = oms;
> > +
[...]

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[David Brownell]

On Monday 26 May 2008, Anton Vorontsov wrote:
> David, would you [not] suggest us to use platform_data with
> container_of, that is, are there any plans to make SPI core copy
> the platform_data instead of passing a pointer directly?

No such plans.  How could it copy, not knowing the size?

Re: [RFC PATCH 2/2] mmc: add OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Mon, May 26, 2008 at 03:19:01PM -0700, David Brownell wrote:
> On Monday 26 May 2008, Anton Vorontsov wrote:
> > David, would you [not] suggest us to use platform_data with
> > container_of, that is, are there any plans to make SPI core copy
> > the platform_data instead of passing a pointer directly?
> 
> No such plans.

Ok, great.

> How could it copy, not knowing the size?

Platform bus usually knows the size via platform_device_add_data()...
anyway, we're in safe if there are no plans for this in SPI.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [RFC] OpenFirmware bindings for the MMC-over-SPI driver

[David Brownell]

On Friday 23 May 2008, Anton Vorontsov wrote:
> 
> This is second attempt to write the OpenFirmware bindings for the
> MMC-over-SPI (and SPI bindings in general).

Summary:  an OF-specific wrapper around the mmc_spi platform code.

I think a wrapper to encapsulate all the OF-specific knowledge makes
much sense here.

The only thing that looks odd to me about this is that the wrapper
is a spi_device rather than an of_device.  To me it makes more sense
to just have an of_device setting up the right spi_device.  (Though
maybe I missed some discussion about why that can't work.)

Example:  drivers/usb/host/sl811_cs.c does that for PCMCIA card
that just wraps an sl811 chip that's more often used standalone.
The PCMCIA descriptors being analagous to OF device table entries.

- Dave

Re: [RFC] OpenFirmware bindings for the MMC-over-SPI driver

[Grant Likely]

On Sat, May 24, 2008 at 1:56 PM, David Brownell <david-b@pacbell.net> wrote:
> On Friday 23 May 2008, Anton Vorontsov wrote:
>>
>> This is second attempt to write the OpenFirmware bindings for the
>> MMC-over-SPI (and SPI bindings in general).
>
> Summary:  an OF-specific wrapper around the mmc_spi platform code.
>
> I think a wrapper to encapsulate all the OF-specific knowledge makes
> much sense here.
>
> The only thing that looks odd to me about this is that the wrapper
> is a spi_device rather than an of_device.  To me it makes more sense
> to just have an of_device setting up the right spi_device.  (Though
> maybe I missed some discussion about why that can't work.)

It's not so much that I can't work; more like it's not necessary.
of_platform bus is by no means the prescribed way to work with the
device tree.  In fact, there is talk about moving away from
of_platform bus entirely and using
platform_device/spi_device/i2c_device/etc directly instead since the
of_platform bus is mostly a clone of the platform bus with different
device binding semantics.

As Anton's patch shows, it is straight forward to add a binding that
can extract the platform data out of the device tree without the
overhead of adding an additional of_platform bus device & driver pair.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.