Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support
Hi,
On Wed, Jul 06, 2011 at 03:05:18PM -0600, Grant Likely wrote:
On Thu, Jul 07, 2011 at 12:47:50AM +0800, Shawn Guo wrote:
The patch adds device tree probe support for sdhci-esdhc-imx driver.
Signed-off-by: Shawn Guo shawn@linaro.org
Cc: Wolfram Sang w.s...@pengutronix.de
Cc: Chris Ball c...@laptop.org
Cc: Grant Likely grant.lik...@secretlab.ca
Acked-by: Grant Likely grant.lik...@secretlab.ca
[...]
+Optional properties:
+- fsl,card-wired : Indicate the card is wired to host permanently
+- fsl,cd-internal : Indicate to use controller internal card detection
+- fsl,wp-internal : Indicate to use controller internal write protection
+- cd-gpios : Specify GPIOs for card detection
+- wp-gpios : Specify GPIOs for write protection
[...]
+ cd-gpios = gpio0 6 0; /* GPIO1_6 */
+ wp-gpios = gpio0 5 0; /* GPIO1_5 */
Is there any particular reason why we started using named GPIOs
in the device tree? To me, that's quite drastic change... should
we start using named regs and interrupts as well? Personally, I
don't think that the idea is great, as now you don't know where
to expect memory resources, interrupt resources and, eventually
GPIO resources.
Just a few disadvantages off the top of my head:
- You can't statically validate your device tree for correctness.
Today dtc checks #{address,size}-cells sanity for 'regs' and
'ranges';
- You can't automatically convert named resources into 'struct
resource *', as we do for platform devices now;
- Any pros for using named resources in the device tree? I don't
see any.
So, I suggest to at least discuss this stuff a little bit more
before polluting device trees with dubious ideas.
p.s.
As for this particular patch, I really don't see why we should
use named GPIOs. For consistency, I'd suggest to reuse bindings
from Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt.
Plus, fsl,cd-internal and fsl,wp-internal is not needed: either
you specify GPIOs or not. That already signals whether driver
should use internal detection (i.e. 'present' register) or
external (i.e. GPIO).
And also, why {cd,wp}-gpioS (plural)?
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support
On Wed, 13 Jul 2011 19:52:12 +0400
Anton Vorontsov cbouatmai...@gmail.com wrote:
On Wed, Jul 06, 2011 at 03:05:18PM -0600, Grant Likely wrote:
On Thu, Jul 07, 2011 at 12:47:50AM +0800, Shawn Guo wrote:
+- cd-gpios : Specify GPIOs for card detection
+- wp-gpios : Specify GPIOs for write protection
[...]
+ cd-gpios = gpio0 6 0; /* GPIO1_6 */
+ wp-gpios = gpio0 5 0; /* GPIO1_5 */
Is there any particular reason why we started using named GPIOs
in the device tree? To me, that's quite drastic change... should
we start using named regs and interrupts as well? Personally, I
don't think that the idea is great, as now you don't know where
to expect memory resources, interrupt resources and, eventually
GPIO resources.
Just check the binding, and you'll know. :-)
It makes it easier to deal with cases where some resources are optional,
especially when dealing with a binding for a family of devices that grows
over time, and helps avoid resources being mismatched since order no longer
matters.
Just a few disadvantages off the top of my head:
- You can't statically validate your device tree for correctness.
Today dtc checks #{address,size}-cells sanity for 'regs' and
'ranges';
The vast majority of stuff in the device tree already cannot be checked in
this manner, without adding binding knowledge to dtc.
Perhaps it could check things that end in -gpios, -regs, etc., if we
avoid adding new properties that match those patterns that aren't what they
appear to be, and let dtc know about any existing cases.
- You can't automatically convert named resources into 'struct
resource *', as we do for platform devices now;
So add named resource support to platform devices. :-)
-Scott
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support
On Wed, Jul 13, 2011 at 11:09:40AM -0500, Scott Wood wrote:
[...]
- You can't automatically convert named resources into 'struct
resource *', as we do for platform devices now;
So add named resource support to platform devices. :-)
There are surely no technical difficulties, it's more about 'type
safety', and as a consequence -- resources being discoverable.
If we want to use named resources, I think we should explicitly reserve
the *-{regs,interrupts,gpios} suffixes for such purposes. And once such
a reservation is done, we're safe again -- resources become discoverable.
Thanks,
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support
On Thu, Jul 14, 2011 at 12:52 AM, Anton Vorontsov
cbouatmai...@gmail.com wrote:
Hi,
On Wed, Jul 06, 2011 at 03:05:18PM -0600, Grant Likely wrote:
On Thu, Jul 07, 2011 at 12:47:50AM +0800, Shawn Guo wrote:
The patch adds device tree probe support for sdhci-esdhc-imx driver.
Signed-off-by: Shawn Guo shawn@linaro.org
Cc: Wolfram Sang w.s...@pengutronix.de
Cc: Chris Ball c...@laptop.org
Cc: Grant Likely grant.lik...@secretlab.ca
Acked-by: Grant Likely grant.lik...@secretlab.ca
[...]
+Optional properties:
+- fsl,card-wired : Indicate the card is wired to host permanently
+- fsl,cd-internal : Indicate to use controller internal card detection
+- fsl,wp-internal : Indicate to use controller internal write protection
+- cd-gpios : Specify GPIOs for card detection
+- wp-gpios : Specify GPIOs for write protection
[...]
+ cd-gpios = gpio0 6 0; /* GPIO1_6 */
+ wp-gpios = gpio0 5 0; /* GPIO1_5 */
Is there any particular reason why we started using named GPIOs
in the device tree? To me, that's quite drastic change... should
we start using named regs and interrupts as well? Personally, I
don't think that the idea is great, as now you don't know where
to expect memory resources, interrupt resources and, eventually
GPIO resources.
No, there are no plans to move to named irqs or regs. irq and reg
resources tend to be a lot more regular than gpios. It's not a
drastic change though because existing bindings remain unaffected, and
new bindings can still use unnamed gpios if that is more appropriate.
Just a few disadvantages off the top of my head:
- You can't statically validate your device tree for correctness.
Today dtc checks #{address,size}-cells sanity for 'regs' and
'ranges';
We can. The gpio binding was extended to be in the form
[name-]gpios. Any property with the string gpios at the end can
be statically validated.
- You can't automatically convert named resources into 'struct
resource *', as we do for platform devices now;
Only for irqs and regs. gpios have never been automatically loaded
into resources.
- Any pros for using named resources in the device tree? I don't
see any.
Human readability. To know exactly what a gpio is intended to be used
for. Particularly for the case where a device might not use all the
gpios that it could use. Yes, the gpios property can have 'holes' in
it, but the observation was made by several people that it is easy to
get wrong. I for one thing the concern was well justified.
So, I suggest to at least discuss this stuff a little bit more
before polluting device trees with dubious ideas.
It was discussed on list quite a while ago.
p.s.
As for this particular patch, I really don't see why we should
use named GPIOs. For consistency, I'd suggest to reuse bindings
from Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt.
Plus, fsl,cd-internal and fsl,wp-internal is not needed: either
you specify GPIOs or not. That already signals whether driver
should use internal detection (i.e. 'present' register) or
external (i.e. GPIO).
wp-internal and cd-internal differentiates between using the internal
functionality and not having wp/cd at all.
And also, why {cd,wp}-gpioS (plural)?
For consistency. Doing it that way means that the plural gpios is
always the suffix for both the gpios and name-gpios use cases.
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support
On Thu, Jul 07, 2011 at 12:47:50AM +0800, Shawn Guo wrote:
The patch adds device tree probe support for sdhci-esdhc-imx driver.
Signed-off-by: Shawn Guo shawn@linaro.org
Cc: Wolfram Sang w.s...@pengutronix.de
Cc: Chris Ball c...@laptop.org
Cc: Grant Likely grant.lik...@secretlab.ca
Acked-by: Grant Likely grant.lik...@secretlab.ca
---
.../devicetree/bindings/mmc/fsl-imx-esdhc.txt | 34 +
drivers/mmc/host/sdhci-esdhc-imx.c | 77
+---
2 files changed, 102 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
new file mode 100644
index 000..ab22fe6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
@@ -0,0 +1,34 @@
+* Freescale Enhanced Secure Digital Host Controller (eSDHC) for i.MX
+
+The Enhanced Secure Digital Host Controller on Freescale i.MX family
+provides an interface for MMC, SD, and SDIO types of memory cards.
+
+Required properties:
+- compatible : Should be fsl,chip-esdhc
+- reg : Should contain eSDHC registers location and length
+- interrupts : Should contain eSDHC interrupt
+
+Optional properties:
+- fsl,card-wired : Indicate the card is wired to host permanently
+- fsl,cd-internal : Indicate to use controller internal card detection
+- fsl,wp-internal : Indicate to use controller internal write protection
+- cd-gpios : Specify GPIOs for card detection
+- wp-gpios : Specify GPIOs for write protection
+
+Examples:
+
+esdhc@70004000 {
+ compatible = fsl,imx51-esdhc;
+ reg = 0x70004000 0x4000;
+ interrupts = 1;
+ fsl,cd-internal;
+ fsl,wp-internal;
+};
+
+esdhc@70008000 {
+ compatible = fsl,imx51-esdhc;
+ reg = 0x70008000 0x4000;
+ interrupts = 2;
+ cd-gpios = gpio0 6 0; /* GPIO1_6 */
+ wp-gpios = gpio0 5 0; /* GPIO1_5 */
+};
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
b/drivers/mmc/host/sdhci-esdhc-imx.c
index f3efb3b..cbdd91f 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -20,6 +20,9 @@
#include linux/mmc/host.h
#include linux/mmc/mmc.h
#include linux/mmc/sdio.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/of_gpio.h
#include mach/esdhc.h
#include sdhci-pltfm.h
#include sdhci-esdhc.h
@@ -73,6 +76,14 @@ static struct platform_device_id imx_esdhc_devtype[] = {
}
};
+static const struct of_device_id imx_esdhc_dt_ids[] = {
+ { .compatible = fsl,imx25-esdhc, .data =
imx_esdhc_devtype[IMX25_ESDHC], },
+ { .compatible = fsl,imx35-esdhc, .data =
imx_esdhc_devtype[IMX35_ESDHC], },
+ { .compatible = fsl,imx51-esdhc, .data =
imx_esdhc_devtype[IMX51_ESDHC], },
+ { .compatible = fsl,imx53-esdhc, .data =
imx_esdhc_devtype[IMX53_ESDHC], },
+ { /* sentinel */ }
+};
+
static inline int is_imx25_esdhc(struct pltfm_imx_data *data)
{
return data-devtype == IMX25_ESDHC;
@@ -307,8 +318,48 @@ static irqreturn_t cd_irq(int irq, void *data)
return IRQ_HANDLED;
};
+#ifdef CONFIG_OF
+static int __devinit
+sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
+ struct esdhc_platform_data *boarddata)
+{
+ struct device_node *np = pdev-dev.of_node;
+
+ if (!np)
+ return -ENODEV;
+
+ if (of_get_property(np, fsl,card-wired, NULL))
+ boarddata-cd_type = ESDHC_CD_PERMANENT;
+
+ if (of_get_property(np, fsl,cd-controller, NULL))
+ boarddata-cd_type = ESDHC_CD_CONTROLLER;
+
+ if (of_get_property(np, fsl,wp-controller, NULL))
+ boarddata-wp_type = ESDHC_WP_CONTROLLER;
+
+ boarddata-cd_gpio = of_get_named_gpio(np, cd-gpios, 0);
+ if (gpio_is_valid(boarddata-cd_gpio))
+ boarddata-cd_type = ESDHC_CD_GPIO;
+
+ boarddata-wp_gpio = of_get_named_gpio(np, wp-gpios, 0);
+ if (gpio_is_valid(boarddata-wp_gpio))
+ boarddata-wp_type = ESDHC_WP_GPIO;
+
+ return 0;
+}
+#else
+static inline int
+sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
+ struct esdhc_platform_data *boarddata)
+{
+ return -ENODEV;
+}
+#endif
+
static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
{
+ const struct of_device_id *of_id =
+ of_match_device(imx_esdhc_dt_ids, pdev-dev);
struct sdhci_pltfm_host *pltfm_host;
struct sdhci_host *host;
struct esdhc_platform_data *boarddata;
@@ -323,9 +374,13 @@ static int __devinit sdhci_esdhc_imx_probe(struct
platform_device *pdev)
pltfm_host = sdhci_priv(host);
imx_data = kzalloc(sizeof(struct pltfm_imx_data), GFP_KERNEL);
- if (!imx_data)
- return -ENOMEM;
+ if (!imx_data) {
+ err = -ENOMEM;
+
