| Message ID | a42729885c593f61bb62c45f229174d9f7370c40.1348809217.git.viresh.kumar2@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Viresh, On Fri, Sep 28 2012, Viresh Kumar wrote: > This adds simple DT bindings for SDHCI SPEAr controller. It uses cd-gpios from > common mmc bindings. > > This also fixes spear300-evb.dts with correct name for card detect binding. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V1->resend: > - Use cd-gpios instead of int-gpio as binding for card detect gpio. > - Fix spear300-evb.dts binding name for cd-gpios > > .../devicetree/bindings/mmc/sdhci-spear.txt | 28 +++++++++ > arch/arm/boot/dts/spear300-evb.dts | 2 +- > drivers/mmc/host/sdhci-spear.c | 71 +++++++++++++++++++++- > 3 files changed, 98 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-spear.txt > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-spear.txt b/Documentation/devicetree/bindings/mmc/sdhci-spear.txt > new file mode 100644 > index 0000000..cd34a05 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/sdhci-spear.txt > @@ -0,0 +1,28 @@ > +* SPEAr SDHCI Controller > + > +Required properties: > +- compatible : "st,spear300-sdhci" > +- reg : Address range of the sdhci > +- interrupt-parent: Should be the phandle for the interrupt controller > + that services interrupts for this device > +- interrupt: Should contain the sdhci interrupt number You don't have to mention reg/interrupts, because they're already covered in mmc.txt. > +Optional Properties: > +- cd-gpios: card detect gpio, with zero flags. > +- power-gpio: specifies the power gpio pin with flags: active low:1, active > + high:0 > +- power_always_enb: power should be on before inserting the card and so can't be > + switched off. Only valid when power gpio is supported. power-gpio should be "power-gpios" (even though there's only one), and power_always_enb should use hyphens instead of underscores, but I have a more fundamental request: You should use a fixed regulator instead of this power-gpio hack. It's easy to hook up a fixed regulator to a gpio: vmmc1: fixedregulator@0 { compatible = "regulator-fixed"; regulator-name = "fixed-supply"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; gpio = <&gpio1 16 0>; startup-delay-us = <70000>; vin-supply = <&parent_reg>; }; and then you can encode the regulator inside your SD host: sdhci@fc000000 { compatible = "st,spear300-sdhci"; reg = <0xfc000000 0x1000>; cd-gpios = <&gpio0 6 0>; vmmc-supply = <&vmmc1>; }; and the MMC core will take care of making sure that it's powered up only when needed. What do you think? Thanks, - Chris.
On Fri, Sep 28, 2012 at 2:33 PM, Chris Ball <cjb@laptop.org> wrote: > On Fri, Sep 28 2012, Viresh Kumar wrote: >> +- reg : Address range of the sdhci >> +- interrupt-parent: Should be the phandle for the interrupt controller >> + that services interrupts for this device >> +- interrupt: Should contain the sdhci interrupt number > > You don't have to mention reg/interrupts, because they're already > covered in mmc.txt. Ok. >> +Optional Properties: >> +- cd-gpios: card detect gpio, with zero flags. >> +- power-gpio: specifies the power gpio pin with flags: active low:1, active >> + high:0 >> +- power_always_enb: power should be on before inserting the card and so can't be >> + switched off. Only valid when power gpio is supported. > > power-gpio should be "power-gpios" (even though there's only one), and > power_always_enb should use hyphens instead of underscores, Sure. > but I have > a more fundamental request: > > You should use a fixed regulator instead of this power-gpio hack. > It's easy to hook up a fixed regulator to a gpio: > > vmmc1: fixedregulator@0 { > compatible = "regulator-fixed"; > regulator-name = "fixed-supply"; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > gpio = <&gpio1 16 0>; > startup-delay-us = <70000>; > vin-supply = <&parent_reg>; > }; > > and then you can encode the regulator inside your SD host: > > sdhci@fc000000 { > compatible = "st,spear300-sdhci"; > reg = <0xfc000000 0x1000>; > cd-gpios = <&gpio0 6 0>; > vmmc-supply = <&vmmc1>; > }; > > and the MMC core will take care of making sure that it's powered up > only when needed. What do you think? We haven't used regulator framework till now for SPEAr and i am not much knowledgeable in that. Because i am not adding this power hack now and it had been there since ever, i would request you to take this patchset as is.. @Shiraz: Can you please explore this a bit and provide a separate patch for it in future? -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Sep 28 2012, viresh kumar wrote: >> You should use a fixed regulator instead of this power-gpio hack. >> It's easy to hook up a fixed regulator to a gpio: >> >> vmmc1: fixedregulator@0 { >> compatible = "regulator-fixed"; >> regulator-name = "fixed-supply"; >> regulator-min-microvolt = <1800000>; >> regulator-max-microvolt = <1800000>; >> gpio = <&gpio1 16 0>; >> startup-delay-us = <70000>; >> vin-supply = <&parent_reg>; >> }; >> >> and then you can encode the regulator inside your SD host: >> >> sdhci@fc000000 { >> compatible = "st,spear300-sdhci"; >> reg = <0xfc000000 0x1000>; >> cd-gpios = <&gpio0 6 0>; >> vmmc-supply = <&vmmc1>; >> }; >> >> and the MMC core will take care of making sure that it's powered up >> only when needed. What do you think? > > We haven't used regulator framework till now for SPEAr and i am not much > knowledgeable in that. > > Because i am not adding this power hack now and it had been there > since ever, i would request you to take this patchset as is.. > > @Shiraz: Can you please explore this a bit and provide a separate > patch for it in future? I think now is a good time to get the DT bindings right instead of propagating previous hacks into the DT, and this should be a pretty simple change -- it's not even a code change, since the code for automatically handling a vmmc-supply exists already. Once we start accepting power-gpios properties we can't easily get rid of them later. If you don't have time to work on this, I guess I could accept the patch without the power-gpios handling included (since it'll still work with a DT that provides a vmmc-supply), but I don't want to take the patch as-is and legitimize the power-gpios binding. Does that make sense? Thanks, - Chris.
On Fri, Sep 28, 2012 at 3:43 PM, Chris Ball <cjb@laptop.org> wrote: > I think now is a good time to get the DT bindings right instead of > propagating previous hacks into the DT, and this should be a pretty > simple change -- it's not even a code change, since the code for > automatically handling a vmmc-supply exists already. Once we start > accepting power-gpios properties we can't easily get rid of them later. > > If you don't have time to work on this, I guess I could accept the patch > without the power-gpios handling included (since it'll still work with > a DT that provides a vmmc-supply), but I don't want to take the patch > as-is and legitimize the power-gpios binding. Does that make sense? Can't disagree with you, for not accepting it :) Ok. For now i would just remove the power-gpios stuff and somebody from ST will fix it with regulator stuff later. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-spear.txt b/Documentation/devicetree/bindings/mmc/sdhci-spear.txt new file mode 100644 index 0000000..cd34a05 --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/sdhci-spear.txt @@ -0,0 +1,28 @@ +* SPEAr SDHCI Controller + +Required properties: +- compatible : "st,spear300-sdhci" +- reg : Address range of the sdhci +- interrupt-parent: Should be the phandle for the interrupt controller + that services interrupts for this device +- interrupt: Should contain the sdhci interrupt number + +Optional Properties: +- cd-gpios: card detect gpio, with zero flags. +- power-gpio: specifies the power gpio pin with flags: active low:1, active + high:0 +- power_always_enb: power should be on before inserting the card and so can't be + switched off. Only valid when power gpio is supported. + +If your board don't support these gpios then don't pass the entry. + +Example: + + sdhci@fc000000 { + compatible = "st,spear300-sdhci"; + reg = <0xfc000000 0x1000>; + + power-gpio = <&gpio0 5 0> + power_always_enb; + cd-gpios = <&gpio0 6 0> + }; diff --git a/arch/arm/boot/dts/spear300-evb.dts b/arch/arm/boot/dts/spear300-evb.dts index d71b8d5..83fa5f9 100644 --- a/arch/arm/boot/dts/spear300-evb.dts +++ b/arch/arm/boot/dts/spear300-evb.dts @@ -80,7 +80,7 @@ }; sdhci@70000000 { - int-gpio = <&gpio1 0 0>; + cd-gpios = <&gpio1 0 0>; power-gpio = <&gpio1 2 1>; status = "okay"; }; diff --git a/drivers/mmc/host/sdhci-spear.c b/drivers/mmc/host/sdhci-spear.c index 423da81..1e82d0f 100644 --- a/drivers/mmc/host/sdhci-spear.c +++ b/drivers/mmc/host/sdhci-spear.c @@ -20,6 +20,8 @@ #include <linux/module.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/slab.h> @@ -68,8 +70,56 @@ static irqreturn_t sdhci_gpio_irq(int irq, void *dev_id) return IRQ_HANDLED; } +#ifdef CONFIG_OF +static struct sdhci_plat_data * __devinit +sdhci_probe_config_dt(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct sdhci_plat_data *pdata = NULL; + enum of_gpio_flags flags; + int pgpio, igpio; + + pgpio = of_get_named_gpio_flags(np, "power-gpio", 0, &flags); + if (!gpio_is_valid(pgpio)) + pgpio = -1; + + igpio = of_get_named_gpio(np, "cd-gpios", 0); + if (!gpio_is_valid(igpio)) + igpio = -1; + + /* If pdata is required */ + if ((pgpio != -1) || (igpio != -1)) { + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + dev_err(&pdev->dev, "DT: kzalloc failed\n"); + return ERR_PTR(-ENOMEM); + } + } + + /* if power gpio is supported */ + if (pgpio != -1) { + pdata->power_active_high = (flags != OF_GPIO_ACTIVE_LOW); + + if (of_property_read_bool(np, "power_always_enb")) + pdata->power_always_enb = true; + } + + pdata->card_power_gpio = pgpio; + pdata->card_int_gpio = igpio; + + return pdata; +} +#else +static struct sdhci_plat_data * __devinit +sdhci_probe_config_dt(struct platform_device *pdev) +{ + return ERR_PTR(-ENOSYS); +} +#endif + static int __devinit sdhci_probe(struct platform_device *pdev) { + struct device_node *np = pdev->dev.of_node; struct sdhci_host *host; struct resource *iomem; struct spear_sdhci *sdhci; @@ -110,8 +160,16 @@ static int __devinit sdhci_probe(struct platform_device *pdev) goto put_clk; } - /* overwrite platform_data */ - sdhci->data = dev_get_platdata(&pdev->dev); + if (np) { + sdhci->data = sdhci_probe_config_dt(pdev); + if (IS_ERR(sdhci->data)) { + dev_err(&pdev->dev, "DT: Failed to get pdata\n"); + return -ENODEV; + } + } else { + sdhci->data = dev_get_platdata(&pdev->dev); + } + pdev->dev.platform_data = sdhci; if (pdev->dev.parent) @@ -276,11 +334,20 @@ static int sdhci_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(sdhci_pm_ops, sdhci_suspend, sdhci_resume); +#ifdef CONFIG_OF +static const struct of_device_id sdhci_spear_id_table[] = { + { .compatible = "st,spear300-sdhci" }, + {} +}; +MODULE_DEVICE_TABLE(of, sdhci_spear_id_table); +#endif + static struct platform_driver sdhci_driver = { .driver = { .name = "sdhci", .owner = THIS_MODULE, .pm = &sdhci_pm_ops, + .of_match_table = of_match_ptr(sdhci_spear_id_table), }, .probe = sdhci_probe, .remove = __devexit_p(sdhci_remove),
This adds simple DT bindings for SDHCI SPEAr controller. It uses cd-gpios from common mmc bindings. This also fixes spear300-evb.dts with correct name for card detect binding. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V1->resend: - Use cd-gpios instead of int-gpio as binding for card detect gpio. - Fix spear300-evb.dts binding name for cd-gpios .../devicetree/bindings/mmc/sdhci-spear.txt | 28 +++++++++ arch/arm/boot/dts/spear300-evb.dts | 2 +- drivers/mmc/host/sdhci-spear.c | 71 +++++++++++++++++++++- 3 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-spear.txt