[PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

Here is v3. I'm out of ideas if you won't like it. :-)

v3:
- Now these bindings are using bus notifiers chain, thus we adhere to the
  spi bus.

  By the way, this scheme (IMO) looks good for I2C devices which needs
  platform_data extracted from the device tree too (Cc'ing Jochen).

- Plus changed the OF bindings themselves, implemented voltage-range
  property. (Pierre, please take a look at vddrange_to_ocrmask(). I
  wonder if you would like this in the MMC core instead, with a kernel
  doc, of course.)

v2:
- Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.

v1:
- Bindings were adhered to the OF SPI core. Withdrawn by OF people.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/powerpc/booting-without-of.txt |   21 +++
 drivers/of/Kconfig                           |    8 +
 drivers/of/Makefile                          |    1 +
 drivers/of/of_mmc_spi.c                      |  210 ++++++++++++++++++++++++++
 4 files changed, 240 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/of_mmc_spi.c

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 6e1711c..6c55f3f 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -3143,7 +3143,28 @@ platforms are moved over to use the flattened-device-tree model.
 			};
 		};
 
+    ...) MMC-over-SPI
 
+      Required properties:
+      - compatible : should be "mmc-spi".
+      - reg : should specify SPI address (chip-select number).
+      - max-speed : (optional) maximum frequency for this device (Hz).
+      - voltage-range : two cells are required, first cell specifies minimum
+        slot voltage (mV), second cell specifies maximum slot voltage (mV).
+      - gpios : (optional) may specify GPIOs in this order: Write-Protect GPIO,
+        Card-Detect GPIO.
+
+      Example:
+
+	mmc-slot@0 {
+		compatible = "mmc-spi";
+		reg = <0>;
+		max-speed = <50000000>;
+		/* Unregulated slot. */
+		voltage-range = <3300 3300>;
+		gpios = <&sdcsr_pio 1 0   /*  WP */
+			 &sdcsr_pio 0 1>; /* nCD */
+	};
 
 VII - Marvell Discovery mv64[345]6x System Controller chips
 ===========================================================
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 3a7a11a..aadbfb3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -13,3 +13,11 @@ config OF_I2C
 	depends on PPC_OF && I2C
 	help
 	  OpenFirmware I2C accessors
+
+config OF_MMC_SPI
+	bool "OpenFirmware bindings for MMC/SD over SPI"
+	depends on PPC_OF && SPI
+	default y if MMC_SPI
+	help
+	  Say Y here to enable OpenFirmware bindings for the MMC/SD over SPI
+	  driver.
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 548772e..a7c44fc 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,3 +2,4 @@ obj-y = base.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
+obj-$(CONFIG_OF_MMC_SPI)	+= of_mmc_spi.o
diff --git a/drivers/of/of_mmc_spi.c b/drivers/of/of_mmc_spi.c
new file mode 100644
index 0000000..aa4e017
--- /dev/null
+++ b/drivers/of/of_mmc_spi.c
@@ -0,0 +1,210 @@
+/*
+ * 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/device.h>
+#include <linux/notifier.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mmc_spi.h>
+#include <linux/mmc/host.h>
+
+/*
+ * XXX: Place it somewhere in the generic MMC code?
+ */
+static int vdd_to_bitnum(int vdd)
+{
+	int bit;
+	const int max_bit = ilog2(MMC_VDD_35_36);
+
+	if (vdd < 1650 || vdd > 3600)
+		return -EINVAL;
+
+	if (vdd >= 1650 && vdd <= 1950)
+		return ilog2(MMC_VDD_165_195);
+
+	/* base 2000 mV, step 100 mV, bit's base 8 */
+	bit = (vdd - 2000) / 100 + 8;
+	if (bit > max_bit)
+		return max_bit;
+	return bit;
+}
+
+static int vddrange_to_ocrmask(int vdd_min, int vdd_max, unsigned int *mask)
+{
+	if (vdd_max < vdd_min)
+		return -EINVAL;
+
+	vdd_max = vdd_to_bitnum(vdd_max);
+	if (vdd_max < 0)
+		return -EINVAL;
+
+	vdd_min = vdd_to_bitnum(vdd_min);
+	if (vdd_min < 0)
+		return -EINVAL;
+
+	/* fill the mask, from max bit to min bit */
+	while (vdd_max >= vdd_min)
+		*mask |= 1 << vdd_max--;
+	return 0;
+}
+
+struct of_mmc_spi {
+	int wp_gpio;
+	int cd_gpio;
+	struct mmc_spi_platform_data mmc_pdata;
+};
+
+static struct of_mmc_spi *to_of_mmc_spi(struct device *dev)
+{
+	return container_of(dev->platform_data, struct of_mmc_spi, mmc_pdata);
+}
+
+static int mmc_get_ro(struct device *dev)
+{
+	struct of_mmc_spi *oms = to_of_mmc_spi(dev);
+
+	return gpio_get_value(oms->wp_gpio);
+}
+
+static int mmc_get_cd(struct device *dev)
+{
+	struct of_mmc_spi *oms = to_of_mmc_spi(dev);
+
+	return gpio_get_value(oms->cd_gpio);
+}
+
+static int of_mmc_spi_add(struct device *dev)
+{
+	int ret = -EINVAL;
+	struct device_node *np = dev->archdata.of_node;
+	struct of_mmc_spi *oms;
+	const u32 *voltage_range;
+	int size;
+
+	if (!np || !of_device_is_compatible(np, "mmc-spi"))
+		return NOTIFY_DONE;
+
+	oms = kzalloc(sizeof(*oms), GFP_KERNEL);
+	if (!oms)
+		return notifier_to_errno(-ENOMEM);
+
+	/* We don't support interrupts yet, let's poll. */
+	oms->mmc_pdata.caps |= MMC_CAP_NEEDS_POLL;
+
+	voltage_range = of_get_property(np, "voltage-range", &size);
+	if (!voltage_range || size < sizeof(*voltage_range) * 2) {
+		dev_err(dev, "OF: voltage-range unspecified\n");
+		goto err_ocr;
+	}
+
+	ret = vddrange_to_ocrmask(voltage_range[0], voltage_range[1],
+				  &oms->mmc_pdata.ocr_mask);
+	if (ret) {
+		dev_err(dev, "OF: specified voltage-range is invalid\n");
+		goto err_ocr;
+	}
+
+	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;
+	}
+
+	dev->platform_data = &oms->mmc_pdata;
+
+	return NOTIFY_STOP;
+
+err_cd_gpio:
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+err_wp_gpio:
+err_ocr:
+	kfree(oms);
+	return notifier_to_errno(ret);
+}
+
+static int of_mmc_spi_del(struct device *dev)
+{
+	struct device_node *np = dev->archdata.of_node;
+	struct of_mmc_spi *oms;
+
+	if (!np || !of_device_is_compatible(np, "mmc-spi") ||
+			!dev->platform_data)
+		return NOTIFY_DONE;
+
+	oms = to_of_mmc_spi(dev);
+
+	if (gpio_is_valid(oms->cd_gpio))
+		gpio_free(oms->cd_gpio);
+	if (gpio_is_valid(oms->wp_gpio))
+		gpio_free(oms->wp_gpio);
+
+	kfree(oms);
+	return NOTIFY_STOP;
+}
+
+static int of_mmc_spi_notify(struct notifier_block *nb, unsigned long action,
+			     void *_dev)
+{
+	struct device *dev = _dev;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		return of_mmc_spi_add(dev);
+	case BUS_NOTIFY_DEL_DEVICE:
+		return of_mmc_spi_del(dev);
+	};
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block of_mmc_spi_notifier = {
+	.notifier_call = of_mmc_spi_notify,
+};
+
+/*
+ * Should be called early enough, but after SPI core initializes. So, we
+ * use subsys_initcall (as in the SPI core), and link order guaranties
+ * that we'll be called at the right time.
+ */
+static int __init of_mmc_spi_init(void)
+{
+	int ret;
+
+	ret = bus_register_notifier(&spi_bus_type, &of_mmc_spi_notifier);
+	if (ret) {
+		pr_err("%s: unable to register notifier on the spi bus\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
+}
+subsys_initcall(of_mmc_spi_init);
+
+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: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> Here is v3. I'm out of ideas if you won't like it. :-)
>
> v3:
> - Now these bindings are using bus notifiers chain, thus we adhere to the
>  spi bus.
>
>  By the way, this scheme (IMO) looks good for I2C devices which needs
>  platform_data extracted from the device tree too (Cc'ing Jochen).
>
> - Plus changed the OF bindings themselves, implemented voltage-range
>  property. (Pierre, please take a look at vddrange_to_ocrmask(). I
>  wonder if you would like this in the MMC core instead, with a kernel
>  doc, of course.)
>
> v2:
> - Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.

Personally I think your v2 was better, and if I'm interpreting
Pierre's comments correctly I think his main point is that instead of
using the 'stock' probe/remove hooks for the spi mmc driver, the
driver should be mildly reworked to provide a common block of code
that can be used by both the OF and non-OF versions of the
probe/remove routines.  I also think that is the way to go.

Cheers,
g.


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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Thu, Jun 05, 2008 at 10:45:17AM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > Here is v3. I'm out of ideas if you won't like it. :-)
> >
> > v3:
> > - Now these bindings are using bus notifiers chain, thus we adhere to the
> >  spi bus.
> >
> >  By the way, this scheme (IMO) looks good for I2C devices which needs
> >  platform_data extracted from the device tree too (Cc'ing Jochen).
> >
> > - Plus changed the OF bindings themselves, implemented voltage-range
> >  property. (Pierre, please take a look at vddrange_to_ocrmask(). I
> >  wonder if you would like this in the MMC core instead, with a kernel
> >  doc, of course.)
> >
> > v2:
> > - Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.
> 
> Personally I think your v2 was better, and if I'm interpreting
> Pierre's comments correctly I think his main point is that instead of
> using the 'stock' probe/remove hooks for the spi mmc driver, the
> driver should be mildly reworked to provide a common block of code
> that can be used by both the OF and non-OF versions of the
> probe/remove routines.  I also think that is the way to go.

Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
host... The absence of enthusiasm I equaled to "no".

Heh.

p.s.
Btw, you forgot another downside of v2 approach: struct spi_driver
duplication... Not sure if everyone will be happy about it.

Though, v2 is only version where we can make modular OF_MMC_SPI.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Jun 05, 2008 at 10:45:17AM -0600, Grant Likely wrote:
>> On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > Here is v3. I'm out of ideas if you won't like it. :-)
>> >
>> > v3:
>> > - Now these bindings are using bus notifiers chain, thus we adhere to the
>> >  spi bus.
>> >
>> >  By the way, this scheme (IMO) looks good for I2C devices which needs
>> >  platform_data extracted from the device tree too (Cc'ing Jochen).
>> >
>> > - Plus changed the OF bindings themselves, implemented voltage-range
>> >  property. (Pierre, please take a look at vddrange_to_ocrmask(). I
>> >  wonder if you would like this in the MMC core instead, with a kernel
>> >  doc, of course.)
>> >
>> > v2:
>> > - Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.
>>
>> Personally I think your v2 was better, and if I'm interpreting
>> Pierre's comments correctly I think his main point is that instead of
>> using the 'stock' probe/remove hooks for the spi mmc driver, the
>> driver should be mildly reworked to provide a common block of code
>> that can be used by both the OF and non-OF versions of the
>> probe/remove routines.  I also think that is the way to go.
>
> Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
> host... The absence of enthusiasm I equaled to "no".
>
> Heh.

I'm allergic to USB HCD code; I was probably having convulsions under my desk.

> p.s.
> Btw, you forgot another downside of v2 approach: struct spi_driver
> duplication... Not sure if everyone will be happy about it.
>
> Though, v2 is only version where we can make modular OF_MMC_SPI.

I think we've got our wires crossed.  I'm not referring to the option
of an of_mmc_spi driver registering an mmc_spi device (which can then
be probed by the mmc_spi_driver).  I'm referring to refactoring the
probe/remove code so that common stuff is callable by both the mmc_spi
and of_mmc_spi drivers without the oddity of the of_mmc_spi probe hook
calling the mmc_spi probe hook.

Cheers,
g.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Thu, Jun 05, 2008 at 11:36:09AM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Thu, Jun 05, 2008 at 10:45:17AM -0600, Grant Likely wrote:
> >> On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
> >> <avorontsov@ru.mvista.com> wrote:
> >> > Here is v3. I'm out of ideas if you won't like it. :-)
> >> >
> >> > v3:
> >> > - Now these bindings are using bus notifiers chain, thus we adhere to the
> >> >  spi bus.
> >> >
> >> >  By the way, this scheme (IMO) looks good for I2C devices which needs
> >> >  platform_data extracted from the device tree too (Cc'ing Jochen).
> >> >
> >> > - Plus changed the OF bindings themselves, implemented voltage-range
> >> >  property. (Pierre, please take a look at vddrange_to_ocrmask(). I
> >> >  wonder if you would like this in the MMC core instead, with a kernel
> >> >  doc, of course.)
> >> >
> >> > v2:
> >> > - Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.
> >>
> >> Personally I think your v2 was better, and if I'm interpreting
> >> Pierre's comments correctly I think his main point is that instead of
> >> using the 'stock' probe/remove hooks for the spi mmc driver, the
> >> driver should be mildly reworked to provide a common block of code
> >> that can be used by both the OF and non-OF versions of the
> >> probe/remove routines.  I also think that is the way to go.
> >
> > Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
> > host... The absence of enthusiasm I equaled to "no".
> >
> > Heh.
> 
> I'm allergic to USB HCD code; I was probably having convulsions under my desk.

:-)

Ok, I also mentioned drivers/ata/pata_of_platform.c (OF version is using
common code from drivers/ata/pata_platform.c).

Please look there, and tell me if this is what you have in mind. (ignore
_probe in the __pata_platform_probe name. Imagine
pata_platform_add_controller or something).

> > p.s.
> > Btw, you forgot another downside of v2 approach: struct spi_driver
> > duplication... Not sure if everyone will be happy about it.
> >
> > Though, v2 is only version where we can make modular OF_MMC_SPI.
> 
> I think we've got our wires crossed.  I'm not referring to the option
> of an of_mmc_spi driver registering an mmc_spi device (which can then
> be probed by the mmc_spi_driver).

I'm not refrering to this option either.

> I'm referring to refactoring the
> probe/remove code so that common stuff is callable by both the mmc_spi
> and of_mmc_spi drivers without the oddity of the of_mmc_spi probe hook
> calling the mmc_spi probe hook.

I understand this.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

On Thu, Jun 5, 2008 at 12:00 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Jun 05, 2008 at 11:36:09AM -0600, Grant Likely wrote:
>> On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
>> > host... The absence of enthusiasm I equaled to "no".
>> >
>> > Heh.
>>
>> I'm allergic to USB HCD code; I was probably having convulsions under my desk.
>
> :-)
>
> Ok, I also mentioned drivers/ata/pata_of_platform.c (OF version is using
> common code from drivers/ata/pata_platform.c).
>
> Please look there, and tell me if this is what you have in mind. (ignore
> _probe in the __pata_platform_probe name. Imagine
> pata_platform_add_controller or something).

Yes, I like that.  I've done something very similar for drivers with
both of and non-of bindings.  For another example, this time all
contained within a single .c file, see drivers/video/xilinxfb.c

>> > p.s.
>> > Btw, you forgot another downside of v2 approach: struct spi_driver
>> > duplication... Not sure if everyone will be happy about it.
>> >
>> > Though, v2 is only version where we can make modular OF_MMC_SPI.
>>
>> I think we've got our wires crossed.  I'm not referring to the option
>> of an of_mmc_spi driver registering an mmc_spi device (which can then
>> be probed by the mmc_spi_driver).
>
> I'm not refrering to this option either.

Okay, I'm confused then.  Where is the duplication of struct spi_driver?

Cheers,
g.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Thu, Jun 05, 2008 at 12:18:56PM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 12:00 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Thu, Jun 05, 2008 at 11:36:09AM -0600, Grant Likely wrote:
> >> On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
> >> <avorontsov@ru.mvista.com> wrote:
> >> > Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
> >> > host... The absence of enthusiasm I equaled to "no".
> >> >
> >> > Heh.
> >>
> >> I'm allergic to USB HCD code; I was probably having convulsions under my desk.
> >
> > :-)
> >
> > Ok, I also mentioned drivers/ata/pata_of_platform.c (OF version is using
> > common code from drivers/ata/pata_platform.c).
> >
> > Please look there, and tell me if this is what you have in mind. (ignore
> > _probe in the __pata_platform_probe name. Imagine
> > pata_platform_add_controller or something).
> 
> Yes, I like that.  I've done something very similar for drivers with
> both of and non-of bindings.  For another example, this time all
> contained within a single .c file, see drivers/video/xilinxfb.c

Ok, great. As I said previously, this is quite easy to do.

> >> > p.s.
> >> > Btw, you forgot another downside of v2 approach: struct spi_driver
> >> > duplication... Not sure if everyone will be happy about it.
> >> >
> >> > Though, v2 is only version where we can make modular OF_MMC_SPI.
> >>
> >> I think we've got our wires crossed.  I'm not referring to the option
> >> of an of_mmc_spi driver registering an mmc_spi device (which can then
> >> be probed by the mmc_spi_driver).
> >
> > I'm not refrering to this option either.
> 
> Okay, I'm confused then.  Where is the duplication of struct spi_driver?

Here it is http://lkml.org/lkml/2008/5/23/299
+ static struct spi_driver of_mmc_spi_driver = {

And here http://lkml.org/lkml/2008/5/24/153 David Brownell says:
> 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.)

^^^ That reminds me v1.

Here http://lkml.org/lkml/2008/5/25/5 you answered though, but there
was no bottom line.

I hope the bottom line is that we're now all happy to create another
spi_driver to handle "OF MMC-o-SPI" devices..?

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

On Thu, Jun 5, 2008 at 12:31 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Jun 05, 2008 at 12:18:56PM -0600, Grant Likely wrote:
>> On Thu, Jun 5, 2008 at 12:00 PM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > On Thu, Jun 05, 2008 at 11:36:09AM -0600, Grant Likely wrote:
>> >> On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
>> >> <avorontsov@ru.mvista.com> wrote:
>> >> > Well, I mentioned the usb_add_hcd()-alike approach for the mmc_spi
>> >> > host... The absence of enthusiasm I equaled to "no".
>> >> >
>> >> > Heh.
>> >>
>> >> I'm allergic to USB HCD code; I was probably having convulsions under my desk.
>> >
>> > :-)
>> >
>> > Ok, I also mentioned drivers/ata/pata_of_platform.c (OF version is using
>> > common code from drivers/ata/pata_platform.c).
>> >
>> > Please look there, and tell me if this is what you have in mind. (ignore
>> > _probe in the __pata_platform_probe name. Imagine
>> > pata_platform_add_controller or something).
>>
>> Yes, I like that.  I've done something very similar for drivers with
>> both of and non-of bindings.  For another example, this time all
>> contained within a single .c file, see drivers/video/xilinxfb.c
>
> Ok, great. As I said previously, this is quite easy to do.
>
>> >> > p.s.
>> >> > Btw, you forgot another downside of v2 approach: struct spi_driver
>> >> > duplication... Not sure if everyone will be happy about it.
>> >> >
>> >> > Though, v2 is only version where we can make modular OF_MMC_SPI.
>> >>
>> >> I think we've got our wires crossed.  I'm not referring to the option
>> >> of an of_mmc_spi driver registering an mmc_spi device (which can then
>> >> be probed by the mmc_spi_driver).
>> >
>> > I'm not refrering to this option either.
>>
>> Okay, I'm confused then.  Where is the duplication of struct spi_driver?
>
> Here it is http://lkml.org/lkml/2008/5/23/299
> + static struct spi_driver of_mmc_spi_driver = {

Right; I was going down the wrong thought path.  I have no problem with this.

BTW, while on that topic, I think it is reasonable to roll the members
of of_mmc_spi into either the mmc_spi_platform_data or the
mmc_spi_host structure.  It is just 2 integers and that would
eliminate storing driver data pointers in seemingly random places.

> And here http://lkml.org/lkml/2008/5/24/153 David Brownell says:
>> 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.)

Yeah; I'm not fond of that approach.  It incurs runtime cost of
multiple 'struct device' for a single device which is unnecessary.

> I hope the bottom line is that we're now all happy to create another
> spi_driver to handle "OF MMC-o-SPI" devices..?

Yes, I'm cool with it.

Cheers,
g.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Anton Vorontsov]

On Sat, Jun 14, 2008 at 05:57:23PM +0200, Pierre Ossman wrote:
> On Thu, 5 Jun 2008 20:16:24 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > Here is v3. I'm out of ideas if you won't like it. :-)
> > 
> > v3:
> > - Now these bindings are using bus notifiers chain, thus we adhere to the
> >   spi bus.
> > 
> 
> Now this was nice and clean. I take it Grant doesn't like this version
> though. What's the downside of it?

To me the main downside of it is inability to make this modular. :-/

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Grant Likely]

On Sat, Jun 14, 2008 at 9:57 AM, Pierre Ossman <drzeus-mmc@drzeus.cx> wrote:
> On Thu, 5 Jun 2008 20:16:24 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
>> Here is v3. I'm out of ideas if you won't like it. :-)
>>
>> v3:
>> - Now these bindings are using bus notifiers chain, thus we adhere to the
>>   spi bus.
>>
>
> Now this was nice and clean. I take it Grant doesn't like this version
> though. What's the downside of it?

No, I don't.  I like the v2 approach better as long as it is been
changed to address your comments.  This approach I think is needlessly
complex and non-obvious (relying on the notification chain instead of
being part of the probe call).  It will break in non-obvious ways if
the notifier module is loaded after the mmc_spi driver.

Cheers,
g.

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

Re: [PATCH] [RFC v3] OF: OpenFirmware bindings for the mmc_spi driver

[Jon Smirl]

On 6/5/08, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> Here is v3. I'm out of ideas if you won't like it. :-)
>
>  v3:
>  - Now these bindings are using bus notifiers chain, thus we adhere to the
>   spi bus.
>
>   By the way, this scheme (IMO) looks good for I2C devices which needs
>   platform_data extracted from the device tree too (Cc'ing Jochen).
>
>  - Plus changed the OF bindings themselves, implemented voltage-range
>   property. (Pierre, please take a look at vddrange_to_ocrmask(). I
>   wonder if you would like this in the MMC core instead, with a kernel
>   doc, of course.)
>
>  v2:
>  - Bindings were adhered to the MMC_SPI driver. Withdrawn by Pierre Ossman.
>
>  v1:
>  - Bindings were adhered to the OF SPI core. Withdrawn by OF people.
>
>  Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>  ---
>   Documentation/powerpc/booting-without-of.txt |   21 +++
>   drivers/of/Kconfig                           |    8 +
>   drivers/of/Makefile                          |    1 +
>   drivers/of/of_mmc_spi.c                      |  210 ++++++++++++++++++++++++++
>   4 files changed, 240 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/of/of_mmc_spi.c
>
>  diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
>  index 6e1711c..6c55f3f 100644
>  --- a/Documentation/powerpc/booting-without-of.txt
>  +++ b/Documentation/powerpc/booting-without-of.txt
>  @@ -3143,7 +3143,28 @@ platforms are moved over to use the flattened-device-tree model.
>                         };
>                 };
>
>  +    ...) MMC-over-SPI
>
>  +      Required properties:
>  +      - compatible : should be "mmc-spi".
>  +      - reg : should specify SPI address (chip-select number).
>  +      - max-speed : (optional) maximum frequency for this device (Hz).
>  +      - voltage-range : two cells are required, first cell specifies minimum
>  +        slot voltage (mV), second cell specifies maximum slot voltage (mV).
>  +      - gpios : (optional) may specify GPIOs in this order: Write-Protect GPIO,
>  +        Card-Detect GPIO.
>  +
>  +      Example:
>  +
>  +       mmc-slot@0 {
>  +               compatible = "mmc-spi";
>  +               reg = <0>;
>  +               max-speed = <50000000>;
>  +               /* Unregulated slot. */
>  +               voltage-range = <3300 3300>;
>  +               gpios = <&sdcsr_pio 1 0   /*  WP */
>  +                        &sdcsr_pio 0 1>; /* nCD */
>  +       };
>
>   VII - Marvell Discovery mv64[345]6x System Controller chips
>   ===========================================================
>  diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>  index 3a7a11a..aadbfb3 100644
>  --- a/drivers/of/Kconfig
>  +++ b/drivers/of/Kconfig
>  @@ -13,3 +13,11 @@ config OF_I2C
>         depends on PPC_OF && I2C
>         help
>           OpenFirmware I2C accessors
>  +
>  +config OF_MMC_SPI
>  +       bool "OpenFirmware bindings for MMC/SD over SPI"
>  +       depends on PPC_OF && SPI
>  +       default y if MMC_SPI
>  +       help
>  +         Say Y here to enable OpenFirmware bindings for the MMC/SD over SPI
>  +         driver.
>  diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>  index 548772e..a7c44fc 100644
>  --- a/drivers/of/Makefile
>  +++ b/drivers/of/Makefile
>  @@ -2,3 +2,4 @@ obj-y = base.o
>   obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>   obj-$(CONFIG_OF_GPIO)   += gpio.o
>   obj-$(CONFIG_OF_I2C)   += of_i2c.o
>  +obj-$(CONFIG_OF_MMC_SPI)       += of_mmc_spi.o
>  diff --git a/drivers/of/of_mmc_spi.c b/drivers/of/of_mmc_spi.c
>  new file mode 100644
>  index 0000000..aa4e017
>  --- /dev/null
>  +++ b/drivers/of/of_mmc_spi.c
>  @@ -0,0 +1,210 @@
>  +/*
>  + * 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/device.h>
>  +#include <linux/notifier.h>
>  +#include <linux/gpio.h>
>  +#include <linux/of.h>
>  +#include <linux/of_gpio.h>
>  +#include <linux/of_platform.h>
>  +#include <linux/spi/spi.h>
>  +#include <linux/spi/mmc_spi.h>
>  +#include <linux/mmc/host.h>
>  +
>  +/*
>  + * XXX: Place it somewhere in the generic MMC code?
>  + */
>  +static int vdd_to_bitnum(int vdd)
>  +{
>  +       int bit;
>  +       const int max_bit = ilog2(MMC_VDD_35_36);
>  +
>  +       if (vdd < 1650 || vdd > 3600)
>  +               return -EINVAL;
>  +
>  +       if (vdd >= 1650 && vdd <= 1950)
>  +               return ilog2(MMC_VDD_165_195);
>  +
>  +       /* base 2000 mV, step 100 mV, bit's base 8 */
>  +       bit = (vdd - 2000) / 100 + 8;
>  +       if (bit > max_bit)
>  +               return max_bit;
>  +       return bit;
>  +}
>  +
>  +static int vddrange_to_ocrmask(int vdd_min, int vdd_max, unsigned int *mask)
>  +{
>  +       if (vdd_max < vdd_min)
>  +               return -EINVAL;
>  +
>  +       vdd_max = vdd_to_bitnum(vdd_max);
>  +       if (vdd_max < 0)
>  +               return -EINVAL;
>  +
>  +       vdd_min = vdd_to_bitnum(vdd_min);
>  +       if (vdd_min < 0)
>  +               return -EINVAL;
>  +
>  +       /* fill the mask, from max bit to min bit */
>  +       while (vdd_max >= vdd_min)
>  +               *mask |= 1 << vdd_max--;
>  +       return 0;
>  +}
>  +
>  +struct of_mmc_spi {
>  +       int wp_gpio;
>  +       int cd_gpio;
>  +       struct mmc_spi_platform_data mmc_pdata;
>  +};
>  +
>  +static struct of_mmc_spi *to_of_mmc_spi(struct device *dev)
>  +{
>  +       return container_of(dev->platform_data, struct of_mmc_spi, mmc_pdata);
>  +}
>  +
>  +static int mmc_get_ro(struct device *dev)
>  +{
>  +       struct of_mmc_spi *oms = to_of_mmc_spi(dev);
>  +
>  +       return gpio_get_value(oms->wp_gpio);
>  +}
>  +
>  +static int mmc_get_cd(struct device *dev)
>  +{
>  +       struct of_mmc_spi *oms = to_of_mmc_spi(dev);
>  +
>  +       return gpio_get_value(oms->cd_gpio);
>  +}
>  +
>  +static int of_mmc_spi_add(struct device *dev)
>  +{
>  +       int ret = -EINVAL;
>  +       struct device_node *np = dev->archdata.of_node;
>  +       struct of_mmc_spi *oms;
>  +       const u32 *voltage_range;
>  +       int size;
>  +
>  +       if (!np || !of_device_is_compatible(np, "mmc-spi"))
>  +               return NOTIFY_DONE;
>  +
>  +       oms = kzalloc(sizeof(*oms), GFP_KERNEL);
>  +       if (!oms)
>  +               return notifier_to_errno(-ENOMEM);
>  +
>  +       /* We don't support interrupts yet, let's poll. */
>  +       oms->mmc_pdata.caps |= MMC_CAP_NEEDS_POLL;
>  +
>  +       voltage_range = of_get_property(np, "voltage-range", &size);
>  +       if (!voltage_range || size < sizeof(*voltage_range) * 2) {
>  +               dev_err(dev, "OF: voltage-range unspecified\n");
>  +               goto err_ocr;
>  +       }
>  +
>  +       ret = vddrange_to_ocrmask(voltage_range[0], voltage_range[1],
>  +                                 &oms->mmc_pdata.ocr_mask);
>  +       if (ret) {
>  +               dev_err(dev, "OF: specified voltage-range is invalid\n");
>  +               goto err_ocr;
>  +       }
>  +
>  +       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;
>  +       }
>  +
>  +       dev->platform_data = &oms->mmc_pdata;
>  +
>  +       return NOTIFY_STOP;
>  +
>  +err_cd_gpio:
>  +       if (gpio_is_valid(oms->wp_gpio))
>  +               gpio_free(oms->wp_gpio);
>  +err_wp_gpio:
>  +err_ocr:
>  +       kfree(oms);
>  +       return notifier_to_errno(ret);
>  +}
>  +
>  +static int of_mmc_spi_del(struct device *dev)
>  +{
>  +       struct device_node *np = dev->archdata.of_node;
>  +       struct of_mmc_spi *oms;
>  +
>  +       if (!np || !of_device_is_compatible(np, "mmc-spi") ||
>  +                       !dev->platform_data)
>  +               return NOTIFY_DONE;
>  +
>  +       oms = to_of_mmc_spi(dev);
>  +
>  +       if (gpio_is_valid(oms->cd_gpio))
>  +               gpio_free(oms->cd_gpio);
>  +       if (gpio_is_valid(oms->wp_gpio))
>  +               gpio_free(oms->wp_gpio);
>  +
>  +       kfree(oms);
>  +       return NOTIFY_STOP;
>  +}
>  +
>  +static int of_mmc_spi_notify(struct notifier_block *nb, unsigned long action,
>  +                            void *_dev)
>  +{
>  +       struct device *dev = _dev;
>  +
>  +       switch (action) {
>  +       case BUS_NOTIFY_ADD_DEVICE:
>  +               return of_mmc_spi_add(dev);
>  +       case BUS_NOTIFY_DEL_DEVICE:
>  +               return of_mmc_spi_del(dev);
>  +       };
>  +       return NOTIFY_DONE;
>  +}
>  +
>  +static struct notifier_block of_mmc_spi_notifier = {
>  +       .notifier_call = of_mmc_spi_notify,
>  +};
>  +
>  +/*
>  + * Should be called early enough, but after SPI core initializes. So, we
>  + * use subsys_initcall (as in the SPI core), and link order guaranties
>  + * that we'll be called at the right time.
>  + */

Why does this need to be loaded at subsys_initcall time? We have the
equivalent code on i2c loading as a normal module.

When the spi bus driver code is processing the child SPI device nodes,
it should request_module() this driver as a module. I made a note to
fix that on Grant's SPI bus driver.

		spi@f00 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mpc5200b-spi","fsl,mpc5200-spi";
Triggers loading of Grant's SPI module via normal OF module mechanism
			reg = <0xf00 0x20>;
			interrupts = <0x2 0xd 0x0 0x2 0xe 0x0>;
			interrupt-parent = <&mpc5200_pic>;
			
Grant's code loops through the child nodes
			mmc-slot@0 {
				compatible = "mmc-spi";
Finds this and does request_module("mmc-spi");
Then it registers the device
				reg = <0>;
				max-speed = <50000000>;
				/* Unregulated slot. */
				voltage-range = <3300 3300>;
				/*gpios = <&sdcsr_pio 1 0   /*  WP */
				/*		 &sdcsr_pio 0 1>; /* nCD */
			};			
		};

As part of this module's initialization it may need to request_module
generic mmc core module, but I haven't traced things out that far yet.
None of these parts should need to be compiled in, they should all be
capable of being loaded as a modules.

>  +static int __init of_mmc_spi_init(void)
>  +{
>  +       int ret;
>  +
>  +       ret = bus_register_notifier(&spi_bus_type, &of_mmc_spi_notifier);
>  +       if (ret) {
>  +               pr_err("%s: unable to register notifier on the spi bus\n",
>  +                       __func__);
>  +               return ret;
>  +       }
>  +
>  +       return 0;
>  +}
>  +subsys_initcall(of_mmc_spi_init);
>  +
>  +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
>
> --
>  To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Jon Smirl
jonsmirl@gmail.com