OpenFirmware bindings for the MMC-over-SPI driver

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

Signed-off-by Anton Vorontsov <[email protected]>

Quoted 3 lines · On Fri, 23 May 2008 22:28:34 +0400 Anton Vorontsov <[email protected]>…

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

Quoted 1 line

Signed-off-by Anton Vorontsov <[email protected]>

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?

Quoted 10 lines · Pierre Ossman

Quoted 4 lines · On Fri, 23 May 2008 22:28:34 +0400 Anton Vorontsov <[email protected]>…

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

Signed-off-by Anton Vorontsov <[email protected]>

This looks extremely wrong. Encapsulating probe functions isn't exactly in line with the device model and bound to confuse people.

Quoted 2 lines

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.

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

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

From what I can tell, the OF stuff behaves very much like the PNP

Quoted 5 lines · On Mon, 26 May 2008 17:10:09 +0400 Anton Vorontsov <[email protected]>…

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

Quoted 6 lines

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.

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

mmc_spi_probe() _is_ a probe function.

Also exporting it as a library function is very confusing.

Quoted 5 lines

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

Quoted 8 lines · Pierre Ossman

Quoted 5 lines · On Mon, 26 May 2008 17:10:09 +0400 Anton Vorontsov <[email protected]>…

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

Quoted 8 lines

Quoted 6 lines

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

Quoted 2 lines

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

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

Quoted 1 line

mmc_spi_probe() _is_ a probe function.

Yes, so far.

Quoted 1 line

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

Quoted 8 lines

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?

Hi Anton,

Quoted 4 lines · On Fri, 23 May 2008 22:28:42 +0400 Anton Vorontsov <[email protected]>…

+++ b/drivers/mmc/host/of_mmc_spi.c

+static int of_mmc_spi_probe(struct spi_device *spi)

drivers/mmc/host/of_mmc_spi.c line 64

Quoted 5 lines

drivers/mmc/host/of_mmc_spi.c new file +5 -0

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

drivers/mmc/host/of_mmc_spi.c line 69

Quoted 1 line

drivers/mmc/host/of_mmc_spi.c new file +1 -0

+	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

Quoted 4 lines

drivers/mmc/host/of_mmc_spi.c new file +4 -0

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

drivers/mmc/host/of_mmc_spi.c line 115

Quoted 4 lines

drivers/mmc/host/of_mmc_spi.c new file +4 -0

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

drivers/mmc/host/of_mmc_spi.c line 130

Quoted 4 lines

drivers/mmc/host/of_mmc_spi.c new file +4 -0

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

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

Documentation/powerpc/booting-without-of.txt +0 -0

index 423cb2b..7d0ef80 100644

Documentation/powerpc/booting-without-of.txt +14 -0

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

drivers/mmc/host/Kconfig +7 -0

index dead617..f468544 100644
@@ -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.

drivers/mmc/host/Makefile +1 -1

index 3877c87..d77f880 100644
@@ -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

drivers/mmc/host/of_mmc_spi.c new file +48 -0

new file mode 100644
index 0000000..b8211ce
@@ -0,0 +1,151 @@
+/*
+ * OpenFirmware bindings for the MMC-over-SPI driver
+ *
+ * Copyright (c) MontaVista Software, Inc. 2008.
+ *
+ * Author: Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#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;

drivers/mmc/host/of_mmc_spi.c new file +93 -1

+
+       /*
+        * 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 <[email protected]>");
+MODULE_LICENSE("GPL");
--

1.5.5.1

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3160

Quoted 2 lines

Documentation/powerpc/booting-without-of.txt +2 -0

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

Quoted 8 lines

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.

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3158

Quoted 3 lines

Documentation/powerpc/booting-without-of.txt +3 -0

+      Required properties:
+      - #address-cells : should be 0.
+      - #size-cells : should be 0.

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

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3164

Quoted 2 lines

Documentation/powerpc/booting-without-of.txt +2 -0

+      - linux,mmc-ocr-mask : (optional) Linux-specific MMC OCR mask
+        (slot voltage).

Should this property be better defined?

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)

Quoted 22 lines · 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 <[email protected]>

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

Documentation/powerpc/booting-without-of.txt +0 -0

index 423cb2b..7d0ef80 100644
@@ -3151,7 +3151,31 @@ platforms are moved over to use the flattened-device-tree model.
                       };
               };
 

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3158

Quoted 5 lines

Documentation/powerpc/booting-without-of.txt +4 -0

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

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3160

Quoted 2 lines

Documentation/powerpc/booting-without-of.txt +2 -0

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

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 3164

Quoted 4 lines

Documentation/powerpc/booting-without-of.txt +4 -0

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

Quoted 17 lines

Documentation/powerpc/booting-without-of.txt +17 -0

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

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model.

Quoted 4 lines

Documentation/powerpc/booting-without-of.txt +4 -0

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

Quoted 7 lines

Documentation/powerpc/booting-without-of.txt +7 -0

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

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?

Quoted 3 lines · Anton Vorontsov

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?

This is second attempt to write the OpenFirmware bindings for the MMC-over-SPI (and SPI bindings in general).

Quoted 3 lines · Anton Vorontsov

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