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 <[email protected]>
---
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 <[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/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 <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.5.5.1
On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
<[email protected]> 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.
On Thu, Jun 05, 2008 at 10:45:17AM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 10:16 AM, Anton Vorontsov
> <[email protected]> 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: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
<[email protected]> 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
>> <[email protected]> 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.
On Thu, Jun 05, 2008 at 11:36:09AM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 11:27 AM, Anton Vorontsov
> <[email protected]> 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
> >> <[email protected]> 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: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jun 5, 2008 at 12:00 PM, Anton Vorontsov
<[email protected]> 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
>> <[email protected]> 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.
On Thu, Jun 05, 2008 at 12:18:56PM -0600, Grant Likely wrote:
> On Thu, Jun 5, 2008 at 12:00 PM, Anton Vorontsov
> <[email protected]> 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
> >> <[email protected]> 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: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jun 5, 2008 at 12:31 PM, Anton Vorontsov
<[email protected]> 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
>> <[email protected]> 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
>> >> <[email protected]> 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.
On Thu, 5 Jun 2008 20:16:24 +0400
Anton Vorontsov <[email protected]> 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?
>
> - 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.)
>
That might be a good idea in the long run. MMC OCR masks have some
gotchas, so it's best if we have as few pieces of code as possible
dealing with it.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org
WARNING: This correspondence is being monitored by the
Swedish government. Use end-to-end encryption where
possible.
On Sat, Jun 14, 2008 at 05:57:23PM +0200, Pierre Ossman wrote:
> On Thu, 5 Jun 2008 20:16:24 +0400
> Anton Vorontsov <[email protected]> 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: [email protected]
irc://irc.freenode.net/bd2
On Sat, Jun 14, 2008 at 9:57 AM, Pierre Ossman <[email protected]> wrote:
> On Thu, 5 Jun 2008 20:16:24 +0400
> Anton Vorontsov <[email protected]> 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.
On 6/5/08, Anton Vorontsov <[email protected]> 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 <[email protected]>
> ---
> 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 <[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/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 <[email protected]>");
> +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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Jon Smirl
[email protected]