| Message ID | 1331469965-28846-6-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | New |
| Headers | show |
Thomas Abraham wrote: > The platform specific callback to setup the sdhci pin mux and pin config > is removed and the pinctrl subsystem interface is used to setup the > mux and config. > > Signed-off-by: Thomas Abraham<thomas.abraham@linaro.org> > --- > drivers/mmc/host/sdhci-s3c.c | 15 +++++++++++++-- > 1 files changed, 13 insertions(+), 2 deletions(-) > [...] > > +#include<plat/map-s5p.h> > +#include<plat/map-base.h> You can add <mach/map.h> instead of above. > + > static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > { > struct s3c_sdhci_platdata *pdata; > @@ -538,6 +543,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > struct sdhci_s3c *sc; > struct resource *res; > int ret, irq, ptr, clks; > + char *pstate; > > if (!pdev->dev.platform_data&& !pdev->dev.of_node) { > dev_err(dev, "no device data specified\n"); > @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > } > > /* Ensure we have minimal gpio selected CMD/CLK/Detect */ > - if (pdata->cfg_gpio) > - pdata->cfg_gpio(pdev, pdata->max_width); I'm not sure we can remove above now for all of Samsung stuff? > + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; Is this right? Current max_width can be 4 or 8 in each board file now. > + sc->pinctrl = pinctrl_get_select(&pdev->dev, pstate); > + if (IS_ERR(sc->pinctrl)) { > + dev_err(dev, "failed to setup pin configuration\n"); > + ret = -ENXIO; > + goto err_req_regs; > + } > > host->hw_name = "samsung-hsmmc"; > host->ops =&sdhci_s3c_ops; Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
On 3/11/12, Thomas Abraham <thomas.abraham@linaro.org> wrote: > The platform specific callback to setup the sdhci pin mux and pin config > is removed and the pinctrl subsystem interface is used to setup the > mux and config. > > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/mmc/host/sdhci-s3c.c | 15 +++++++++++++-- > 1 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c > index ea0767e..76c1c36 100644 > --- a/drivers/mmc/host/sdhci-s3c.c > +++ b/drivers/mmc/host/sdhci-s3c.c > @@ -22,6 +22,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_gpio.h> > +#include <linux/pinctrl/consumer.h> > > #include <linux/mmc/host.h> > > @@ -50,6 +51,7 @@ struct sdhci_s3c { > struct platform_device *pdev; > struct resource *ioarea; > struct s3c_sdhci_platdata *pdata; > + struct pinctrl *pinctrl; > unsigned int cur_clk; > int ext_cd_irq; > int ext_cd_gpio; > @@ -529,6 +531,9 @@ static inline struct sdhci_s3c_drv_data > *sdhci_s3c_get_driver_data( > platform_get_device_id(pdev)->driver_data; > } > > +#include <plat/map-s5p.h> > +#include <plat/map-base.h> Why this header files are required? > + > static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > { > struct s3c_sdhci_platdata *pdata; > @@ -538,6 +543,7 @@ static int __devinit sdhci_s3c_probe(struct > platform_device *pdev) > struct sdhci_s3c *sc; > struct resource *res; > int ret, irq, ptr, clks; > + char *pstate; > > if (!pdev->dev.platform_data && !pdev->dev.of_node) { > dev_err(dev, "no device data specified\n"); > @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct > platform_device *pdev) > } > > /* Ensure we have minimal gpio selected CMD/CLK/Detect */ > - if (pdata->cfg_gpio) > - pdata->cfg_gpio(pdev, pdata->max_width); > + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; You should check pdata->max_width == 8 instead of max_width itself. BTW if platform set the amx_width as 1. How do you handle this? Thank you, Kyungmin Park > + sc->pinctrl = pinctrl_get_select(&pdev->dev, pstate); > + if (IS_ERR(sc->pinctrl)) { > + dev_err(dev, "failed to setup pin configuration\n"); > + ret = -ENXIO; > + goto err_req_regs; > + } > > host->hw_name = "samsung-hsmmc"; > host->ops = &sdhci_s3c_ops; > -- > 1.6.6.rc2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
2012/3/12 Kukjin Kim <kgene.kim@samsung.com>: > Thomas Abraham wrote: >> The platform specific callback to setup the sdhci pin mux and pin config >> is removed and the pinctrl subsystem interface is used to setup the >> mux and config. >> >> Signed-off-by: Thomas Abraham<thomas.abraham@linaro.org> >> --- >> drivers/mmc/host/sdhci-s3c.c | 15 +++++++++++++-- >> 1 files changed, 13 insertions(+), 2 deletions(-) >> > [...] > >> >> +#include<plat/map-s5p.h> >> +#include<plat/map-base.h> > > You can add <mach/map.h> instead of above. > >> + >> static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> { >> struct s3c_sdhci_platdata *pdata; >> @@ -538,6 +543,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> struct sdhci_s3c *sc; >> struct resource *res; >> int ret, irq, ptr, clks; >> + char *pstate; >> >> if (!pdev->dev.platform_data&& !pdev->dev.of_node) { >> dev_err(dev, "no device data specified\n"); >> @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> } >> >> /* Ensure we have minimal gpio selected CMD/CLK/Detect */ >> - if (pdata->cfg_gpio) >> - pdata->cfg_gpio(pdev, pdata->max_width); > > I'm not sure we can remove above now for all of Samsung stuff? We the pin map table is fully populated for each SoC (and board as necessary), then the platform callback functions to setup the pinmux and pinconfig can be removed for the drivers. But before doing that, the next step would be to add gpio interrupts and wakeup interrupts support into the samsung pinctrl driver. When we have a fully functional pinctrl driver, we could add the required bits in SoC and board files and switch over to the pinctrl driver. > >> + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; > > Is this right? Current max_width can be 4 or 8 in each board file now. Sorry, that is not correct. I will fix it. Thanks. Regards, Thomas.
On 12 March 2012 08:08, Kyungmin Park <kmpark@infradead.org> wrote: > On 3/11/12, Thomas Abraham <thomas.abraham@linaro.org> wrote: >> The platform specific callback to setup the sdhci pin mux and pin config >> is removed and the pinctrl subsystem interface is used to setup the >> mux and config. >> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> --- >> drivers/mmc/host/sdhci-s3c.c | 15 +++++++++++++-- >> 1 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c >> index ea0767e..76c1c36 100644 >> --- a/drivers/mmc/host/sdhci-s3c.c >> +++ b/drivers/mmc/host/sdhci-s3c.c >> @@ -22,6 +22,7 @@ >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_gpio.h> >> +#include <linux/pinctrl/consumer.h> >> >> #include <linux/mmc/host.h> >> >> @@ -50,6 +51,7 @@ struct sdhci_s3c { >> struct platform_device *pdev; >> struct resource *ioarea; >> struct s3c_sdhci_platdata *pdata; >> + struct pinctrl *pinctrl; >> unsigned int cur_clk; >> int ext_cd_irq; >> int ext_cd_gpio; >> @@ -529,6 +531,9 @@ static inline struct sdhci_s3c_drv_data >> *sdhci_s3c_get_driver_data( >> platform_get_device_id(pdev)->driver_data; >> } >> >> +#include <plat/map-s5p.h> >> +#include <plat/map-base.h> > > Why this header files are required? Sorry, that was a mistake. This was left over here after adding it here for some debugging. I will take care next time. >> + >> static int __devinit sdhci_s3c_probe(struct platform_device *pdev) >> { >> struct s3c_sdhci_platdata *pdata; >> @@ -538,6 +543,7 @@ static int __devinit sdhci_s3c_probe(struct >> platform_device *pdev) >> struct sdhci_s3c *sc; >> struct resource *res; >> int ret, irq, ptr, clks; >> + char *pstate; >> >> if (!pdev->dev.platform_data && !pdev->dev.of_node) { >> dev_err(dev, "no device data specified\n"); >> @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct >> platform_device *pdev) >> } >> >> /* Ensure we have minimal gpio selected CMD/CLK/Detect */ >> - if (pdata->cfg_gpio) >> - pdata->cfg_gpio(pdev, pdata->max_width); >> + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; > You should check pdata->max_width == 8 instead of max_width itself. Thanks. I will fix that. > BTW if platform set the amx_width as 1. How do you handle this? If the bus width is 1, we need to add pin map entry for 1 bit bus width and correspondingly change the code here. I used the sdhci-s3c driver for testing the samsung pinctrl driver and only tested with 4-bit and 8-bit bus width. The changes in this patch are not final yet and was mainly included in this series to show how platform callbacks can be removed. Thanks, Thomas.
On Sun, Mar 11, 2012 at 06:16:05PM +0530, Thomas Abraham wrote: > The platform specific callback to setup the sdhci pin mux and pin config > is removed and the pinctrl subsystem interface is used to setup the > mux and config. You've only added pinctrl support for Exynos but this driver is also used by SoCs going back to s3c24xx era. Either all those SoCs need to be converted to use pinctrl or the driver needs to understand both methods (eg, using pinctrl if no callback is supplied).
On 12 March 2012 19:51, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Sun, Mar 11, 2012 at 06:16:05PM +0530, Thomas Abraham wrote: >> The platform specific callback to setup the sdhci pin mux and pin config >> is removed and the pinctrl subsystem interface is used to setup the >> mux and config. > > You've only added pinctrl support for Exynos but this driver is also > used by SoCs going back to s3c24xx era. Either all those SoCs need to > be converted to use pinctrl or the driver needs to understand both > methods (eg, using pinctrl if no callback is supplied). Yes, I agree with your comment. I did refer to manuals of s3c24xx to Exynos to ensure that the samsung pinctrl driver is generic and reusable on all samsung soc's. I hope I have not missed out something important that would require additional tweaks in the samsung pinctrl driver. The missing bits now are gpio interrupt and wakeup interrupt support in the samsung pinctrl driver. Once that is complete, I believe it should be easy to add support for other SoC and convert the drivers to use pinctrl (other option being to let pinctrl driver hog all the mappings at boot and remove the cfg_gpio platform callbacks from the driver). Thank you Mark for your comments. Regards, Thomas.
On Mon, Mar 12, 2012 at 08:01:34PM +0530, Thomas Abraham wrote: > Yes, I agree with your comment. I did refer to manuals of s3c24xx to > Exynos to ensure that the samsung pinctrl driver is generic and > reusable on all samsung soc's. I hope I have not missed out something > important that would require additional tweaks in the samsung pinctrl > driver. The missing bits now are gpio interrupt and wakeup interrupt > support in the samsung pinctrl driver. Once that is complete, I > believe it should be easy to add support for other SoC and convert the > drivers to use pinctrl (other option being to let pinctrl driver hog > all the mappings at boot and remove the cfg_gpio platform callbacks > from the driver). Yes, I don't see any fundamental problems here - it's more that either we'll need to get all the SoCs converted over (which is a lot of work) or we'll need to have drivers be able to cope with running either way.
On 03/11/2012 06:46 AM, Thomas Abraham wrote: > The platform specific callback to setup the sdhci pin mux and pin config > is removed and the pinctrl subsystem interface is used to setup the > mux and config. > @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) > } > > /* Ensure we have minimal gpio selected CMD/CLK/Detect */ > - if (pdata->cfg_gpio) > - pdata->cfg_gpio(pdev, pdata->max_width); > + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; If the driver is going to select a single state ("sdhci-pcfg8" or "sdhci-pcfg4" above) at probe() time and never change it (which seems quite reasonable for an SDHCI controller), then the driver should always use state PINCTRL_STATE_DEFAULT, and it should be up to the board to set up the mapping table such that PINCTRL_STATE_DEFAULT sets up the pins for either 4-bit or 8-bit as appropriate for the board.
diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c index ea0767e..76c1c36 100644 --- a/drivers/mmc/host/sdhci-s3c.c +++ b/drivers/mmc/host/sdhci-s3c.c @@ -22,6 +22,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <linux/mmc/host.h> @@ -50,6 +51,7 @@ struct sdhci_s3c { struct platform_device *pdev; struct resource *ioarea; struct s3c_sdhci_platdata *pdata; + struct pinctrl *pinctrl; unsigned int cur_clk; int ext_cd_irq; int ext_cd_gpio; @@ -529,6 +531,9 @@ static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data( platform_get_device_id(pdev)->driver_data; } +#include <plat/map-s5p.h> +#include <plat/map-base.h> + static int __devinit sdhci_s3c_probe(struct platform_device *pdev) { struct s3c_sdhci_platdata *pdata; @@ -538,6 +543,7 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) struct sdhci_s3c *sc; struct resource *res; int ret, irq, ptr, clks; + char *pstate; if (!pdev->dev.platform_data && !pdev->dev.of_node) { dev_err(dev, "no device data specified\n"); @@ -643,8 +649,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev) } /* Ensure we have minimal gpio selected CMD/CLK/Detect */ - if (pdata->cfg_gpio) - pdata->cfg_gpio(pdev, pdata->max_width); + pstate = pdata->max_width ? "sdhci-pcfg8" : "sdhci-pcfg4"; + sc->pinctrl = pinctrl_get_select(&pdev->dev, pstate); + if (IS_ERR(sc->pinctrl)) { + dev_err(dev, "failed to setup pin configuration\n"); + ret = -ENXIO; + goto err_req_regs; + } host->hw_name = "samsung-hsmmc"; host->ops = &sdhci_s3c_ops;
The platform specific callback to setup the sdhci pin mux and pin config is removed and the pinctrl subsystem interface is used to setup the mux and config. Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- drivers/mmc/host/sdhci-s3c.c | 15 +++++++++++++-- 1 files changed, 13 insertions(+), 2 deletions(-)