RE: [PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver
Hi Uffe/Michal, > -Original Message- > From: Michal Simek > Sent: Wednesday, November 18, 2020 11:54 PM > To: Ulf Hansson ; Manish Narani > > Cc: Michal Simek ; Adrian Hunter > ; Linux ARM ker...@lists.infradead.org>; linux-...@vger.kernel.org; Linux Kernel > Mailing List ; git > Subject: Re: [PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver > > > > On 18. 11. 20 16:43, Ulf Hansson wrote: > > On Wed, 18 Nov 2020 at 07:22, Manish Narani > wrote: > >> > >> Driver should be able to handle optional pinctrl setting. > >> > >> Signed-off-by: Michal Simek > >> Signed-off-by: Manish Narani > >> --- > >> drivers/mmc/host/sdhci-of-arasan.c | 24 > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c > b/drivers/mmc/host/sdhci-of-arasan.c > >> index 829ccef87426..f788cc9d5914 100644 > >> --- a/drivers/mmc/host/sdhci-of-arasan.c > >> +++ b/drivers/mmc/host/sdhci-of-arasan.c > >> @@ -23,6 +23,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include "cqhci.h" > >> #include "sdhci-pltfm.h" > >> @@ -135,6 +136,8 @@ struct sdhci_arasan_clk_data { > >> * @clk_ops: Struct for the Arasan Controller Clock Operations. > >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. > >> + * @pinctrl: Per-device pin control state holder. > >> + * @pins_default: Pinctrl state for a device. > >> * @quirks:Arasan deviations from spec. > >> */ > >> struct sdhci_arasan_data { > >> @@ -149,6 +152,8 @@ struct sdhci_arasan_data { > >> > >> struct regmap *soc_ctl_base; > >> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > >> + struct pinctrl *pinctrl; > >> + struct pinctrl_state *pins_default; > >> unsigned intquirks; > >> > >> /* Controller does not have CD wired and will not function normally > without */ > >> @@ -1619,6 +1624,25 @@ static int sdhci_arasan_probe(struct > platform_device *pdev) > >> goto unreg_clk; > >> } > >> > >> + sdhci_arasan->pinctrl = devm_pinctrl_get(>dev); > >> + if (!IS_ERR(sdhci_arasan->pinctrl)) { > >> + sdhci_arasan->pins_default = > >> + pinctrl_lookup_state(sdhci_arasan->pinctrl, > >> +PINCTRL_STATE_DEFAULT); > >> + if (IS_ERR(sdhci_arasan->pins_default)) { > >> + dev_err(>dev, "Missing default pinctrl > >> config\n"); > >> + ret = PTR_ERR(sdhci_arasan->pins_default); > >> + goto unreg_clk; > >> + } > >> + > >> + ret = pinctrl_select_state(sdhci_arasan->pinctrl, > >> + sdhci_arasan->pins_default); > >> + if (ret) { > >> + dev_err(>dev, "could not select default > >> state\n"); > >> + goto unreg_clk; > >> + } > >> + } > > > > Isn't all this already taken care of via pinctrl_bind_pins() called by > > driver core during probe? > > > > Thanks for the hint. > Manish: Can you please check it? Thanks Uffe. Yes, this is already taken care of via pinctrl_bind_pins() called during probe. This patch is not required to be merged. Thanks, Manish
Re: [PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver
On 18. 11. 20 16:43, Ulf Hansson wrote: > On Wed, 18 Nov 2020 at 07:22, Manish Narani wrote: >> >> Driver should be able to handle optional pinctrl setting. >> >> Signed-off-by: Michal Simek >> Signed-off-by: Manish Narani >> --- >> drivers/mmc/host/sdhci-of-arasan.c | 24 >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c >> b/drivers/mmc/host/sdhci-of-arasan.c >> index 829ccef87426..f788cc9d5914 100644 >> --- a/drivers/mmc/host/sdhci-of-arasan.c >> +++ b/drivers/mmc/host/sdhci-of-arasan.c >> @@ -23,6 +23,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "cqhci.h" >> #include "sdhci-pltfm.h" >> @@ -135,6 +136,8 @@ struct sdhci_arasan_clk_data { >> * @clk_ops: Struct for the Arasan Controller Clock Operations. >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. >> + * @pinctrl: Per-device pin control state holder. >> + * @pins_default: Pinctrl state for a device. >> * @quirks:Arasan deviations from spec. >> */ >> struct sdhci_arasan_data { >> @@ -149,6 +152,8 @@ struct sdhci_arasan_data { >> >> struct regmap *soc_ctl_base; >> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; >> + struct pinctrl *pinctrl; >> + struct pinctrl_state *pins_default; >> unsigned intquirks; >> >> /* Controller does not have CD wired and will not function normally without >> */ >> @@ -1619,6 +1624,25 @@ static int sdhci_arasan_probe(struct platform_device >> *pdev) >> goto unreg_clk; >> } >> >> + sdhci_arasan->pinctrl = devm_pinctrl_get(>dev); >> + if (!IS_ERR(sdhci_arasan->pinctrl)) { >> + sdhci_arasan->pins_default = >> + pinctrl_lookup_state(sdhci_arasan->pinctrl, >> +PINCTRL_STATE_DEFAULT); >> + if (IS_ERR(sdhci_arasan->pins_default)) { >> + dev_err(>dev, "Missing default pinctrl >> config\n"); >> + ret = PTR_ERR(sdhci_arasan->pins_default); >> + goto unreg_clk; >> + } >> + >> + ret = pinctrl_select_state(sdhci_arasan->pinctrl, >> + sdhci_arasan->pins_default); >> + if (ret) { >> + dev_err(>dev, "could not select default >> state\n"); >> + goto unreg_clk; >> + } >> + } > > Isn't all this already taken care of via pinctrl_bind_pins() called by > driver core during probe? > Thanks for the hint. Manish: Can you please check it? Thanks, Michal
Re: [PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver
On Wed, 18 Nov 2020 at 07:22, Manish Narani wrote: > > Driver should be able to handle optional pinctrl setting. > > Signed-off-by: Michal Simek > Signed-off-by: Manish Narani > --- > drivers/mmc/host/sdhci-of-arasan.c | 24 > 1 file changed, 24 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c > b/drivers/mmc/host/sdhci-of-arasan.c > index 829ccef87426..f788cc9d5914 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "cqhci.h" > #include "sdhci-pltfm.h" > @@ -135,6 +136,8 @@ struct sdhci_arasan_clk_data { > * @clk_ops: Struct for the Arasan Controller Clock Operations. > * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > * @soc_ctl_map: Map to get offsets into soc_ctl registers. > + * @pinctrl: Per-device pin control state holder. > + * @pins_default: Pinctrl state for a device. > * @quirks:Arasan deviations from spec. > */ > struct sdhci_arasan_data { > @@ -149,6 +152,8 @@ struct sdhci_arasan_data { > > struct regmap *soc_ctl_base; > const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > + struct pinctrl *pinctrl; > + struct pinctrl_state *pins_default; > unsigned intquirks; > > /* Controller does not have CD wired and will not function normally without > */ > @@ -1619,6 +1624,25 @@ static int sdhci_arasan_probe(struct platform_device > *pdev) > goto unreg_clk; > } > > + sdhci_arasan->pinctrl = devm_pinctrl_get(>dev); > + if (!IS_ERR(sdhci_arasan->pinctrl)) { > + sdhci_arasan->pins_default = > + pinctrl_lookup_state(sdhci_arasan->pinctrl, > +PINCTRL_STATE_DEFAULT); > + if (IS_ERR(sdhci_arasan->pins_default)) { > + dev_err(>dev, "Missing default pinctrl > config\n"); > + ret = PTR_ERR(sdhci_arasan->pins_default); > + goto unreg_clk; > + } > + > + ret = pinctrl_select_state(sdhci_arasan->pinctrl, > + sdhci_arasan->pins_default); > + if (ret) { > + dev_err(>dev, "could not select default > state\n"); > + goto unreg_clk; > + } > + } Isn't all this already taken care of via pinctrl_bind_pins() called by driver core during probe? [...] Kind regards Uffe
Re: [PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver
On 18. 11. 20 7:21, Manish Narani wrote: > Driver should be able to handle optional pinctrl setting. > > Signed-off-by: Michal Simek > Signed-off-by: Manish Narani > --- > drivers/mmc/host/sdhci-of-arasan.c | 24 > 1 file changed, 24 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c > b/drivers/mmc/host/sdhci-of-arasan.c > index 829ccef87426..f788cc9d5914 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "cqhci.h" > #include "sdhci-pltfm.h" > @@ -135,6 +136,8 @@ struct sdhci_arasan_clk_data { > * @clk_ops: Struct for the Arasan Controller Clock Operations. > * @soc_ctl_base:Pointer to regmap for syscon for soc_ctl registers. > * @soc_ctl_map: Map to get offsets into soc_ctl registers. > + * @pinctrl: Per-device pin control state holder. > + * @pins_default:Pinctrl state for a device. > * @quirks: Arasan deviations from spec. > */ > struct sdhci_arasan_data { > @@ -149,6 +152,8 @@ struct sdhci_arasan_data { > > struct regmap *soc_ctl_base; > const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > + struct pinctrl *pinctrl; > + struct pinctrl_state *pins_default; > unsigned intquirks; > > /* Controller does not have CD wired and will not function normally without > */ > @@ -1619,6 +1624,25 @@ static int sdhci_arasan_probe(struct platform_device > *pdev) > goto unreg_clk; > } > > + sdhci_arasan->pinctrl = devm_pinctrl_get(>dev); > + if (!IS_ERR(sdhci_arasan->pinctrl)) { > + sdhci_arasan->pins_default = > + pinctrl_lookup_state(sdhci_arasan->pinctrl, > + PINCTRL_STATE_DEFAULT); > + if (IS_ERR(sdhci_arasan->pins_default)) { > + dev_err(>dev, "Missing default pinctrl config\n"); > + ret = PTR_ERR(sdhci_arasan->pins_default); > + goto unreg_clk; > + } > + > + ret = pinctrl_select_state(sdhci_arasan->pinctrl, > +sdhci_arasan->pins_default); > + if (ret) { > + dev_err(>dev, "could not select default state\n"); > + goto unreg_clk; > + } > + } > + > sdhci_arasan->phy = ERR_PTR(-ENODEV); > if (of_device_is_compatible(pdev->dev.of_node, > "arasan,sdhci-5.1")) { > Ulf: Is there any need to describe in binding doc? I mean all txt based binding have it described. But not quite sure if there is a need to describe it in yaml if only default option is supported. And when this is optional it should be fine also for others SOC. For patch itself. Acked-by: Michal Simek Thanks, Michal
[PATCH] mmc: sdhci-of-arasan: Add pinctrl support to the driver
Driver should be able to handle optional pinctrl setting. Signed-off-by: Michal Simek Signed-off-by: Manish Narani --- drivers/mmc/host/sdhci-of-arasan.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 829ccef87426..f788cc9d5914 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -23,6 +23,7 @@ #include #include #include +#include #include "cqhci.h" #include "sdhci-pltfm.h" @@ -135,6 +136,8 @@ struct sdhci_arasan_clk_data { * @clk_ops: Struct for the Arasan Controller Clock Operations. * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. * @soc_ctl_map: Map to get offsets into soc_ctl registers. + * @pinctrl: Per-device pin control state holder. + * @pins_default: Pinctrl state for a device. * @quirks:Arasan deviations from spec. */ struct sdhci_arasan_data { @@ -149,6 +152,8 @@ struct sdhci_arasan_data { struct regmap *soc_ctl_base; const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; + struct pinctrl *pinctrl; + struct pinctrl_state *pins_default; unsigned intquirks; /* Controller does not have CD wired and will not function normally without */ @@ -1619,6 +1624,25 @@ static int sdhci_arasan_probe(struct platform_device *pdev) goto unreg_clk; } + sdhci_arasan->pinctrl = devm_pinctrl_get(>dev); + if (!IS_ERR(sdhci_arasan->pinctrl)) { + sdhci_arasan->pins_default = + pinctrl_lookup_state(sdhci_arasan->pinctrl, +PINCTRL_STATE_DEFAULT); + if (IS_ERR(sdhci_arasan->pins_default)) { + dev_err(>dev, "Missing default pinctrl config\n"); + ret = PTR_ERR(sdhci_arasan->pins_default); + goto unreg_clk; + } + + ret = pinctrl_select_state(sdhci_arasan->pinctrl, + sdhci_arasan->pins_default); + if (ret) { + dev_err(>dev, "could not select default state\n"); + goto unreg_clk; + } + } + sdhci_arasan->phy = ERR_PTR(-ENODEV); if (of_device_is_compatible(pdev->dev.of_node, "arasan,sdhci-5.1")) { -- 2.17.1