| Message ID | 1516105859-3525-5-git-send-email-patrice.chotard@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[...] > /* > @@ -1616,6 +1625,32 @@ static int mmci_probe(struct amba_device *dev, > host = mmc_priv(mmc); > host->mmc = mmc; > > + /* > + * Some variant (STM32) doesn't have opendrain bit, nevertheless > + * pins can be set accordingly using pinctrl > + */ > + if (!variant->opendrain) { > + host->pinctrl = devm_pinctrl_get(&dev->dev); > + if (IS_ERR(host->pinctrl)) { > + dev_err(&dev->dev, "failed to get pinctrl"); > + goto host_free; > + } > + > + host->pins_default = pinctrl_lookup_state(host->pinctrl, > + PINCTRL_STATE_DEFAULT); > + if (IS_ERR(host->pins_default)) { > + dev_warn(mmc_dev(mmc), "Can't select default pins\n"); > + host->pins_default = NULL; This is wrong, I think you should bail out and return the error code instead. Moreover, calling pinctrl_select_state() from ->set_ios by using a NULL state, will likely trigger a NULL pointer deference bug in the pinctrl layer. > + } > + > + host->pins_opendrain = pinctrl_lookup_state(host->pinctrl, > + MMCI_PINCTRL_STATE_OPENDRAIN); > + if (IS_ERR(host->pins_opendrain)) { > + dev_warn(mmc_dev(mmc), "Can't select opendrain pins\n"); > + host->pins_opendrain = NULL; Ditto. > + } > + } > + [...] Kind regards Uffe
Hi Ulf On 01/17/2018 10:34 AM, Ulf Hansson wrote: > [...] > >> /* >> @@ -1616,6 +1625,32 @@ static int mmci_probe(struct amba_device *dev, >> host = mmc_priv(mmc); >> host->mmc = mmc; >> >> + /* >> + * Some variant (STM32) doesn't have opendrain bit, nevertheless >> + * pins can be set accordingly using pinctrl >> + */ >> + if (!variant->opendrain) { >> + host->pinctrl = devm_pinctrl_get(&dev->dev); >> + if (IS_ERR(host->pinctrl)) { >> + dev_err(&dev->dev, "failed to get pinctrl"); >> + goto host_free; >> + } >> + >> + host->pins_default = pinctrl_lookup_state(host->pinctrl, >> + PINCTRL_STATE_DEFAULT); >> + if (IS_ERR(host->pins_default)) { >> + dev_warn(mmc_dev(mmc), "Can't select default pins\n"); >> + host->pins_default = NULL; > > This is wrong, I think you should bail out and return the error code instead. Ok > > Moreover, calling pinctrl_select_state() from ->set_ios by using a > NULL state, will likely trigger a NULL pointer deference bug in the > pinctrl layer. Regarding pinctrl_select_state() call with a NULL state, this case is managed inside pinctrl_state(), but ok, it will be more elegant to exit directly in case of no DT pins definition found. > >> + } >> + >> + host->pins_opendrain = pinctrl_lookup_state(host->pinctrl, >> + MMCI_PINCTRL_STATE_OPENDRAIN); >> + if (IS_ERR(host->pins_opendrain)) { >> + dev_warn(mmc_dev(mmc), "Can't select opendrain pins\n"); >> + host->pins_opendrain = NULL; > > Ditto. ok Thanks Patrice > >> + } >> + } >> + > > [...] > > Kind regards > Uffe >
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index eb5fcfe..2a7aea7 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1465,13 +1465,22 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) ~MCI_ST_DATA2DIREN); } - if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN && - host->variant->opendrain) { + if (host->variant->opendrain) { + if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) + /* + * The ST Micro variant use the ROD bit for + * something else and only has OD (Open Drain). + */ + pwr |= host->variant->opendrain; + } else { /* - * The ST Micro variant use the ROD bit for - * something else and only has OD (Open Drain). + * If the variant cannot configure the pads by its own, then we + * expect the pinctrl to be able to do that for us */ - pwr |= host->variant->opendrain; + if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) + pinctrl_select_state(host->pinctrl, host->pins_opendrain); + else + pinctrl_select_state(host->pinctrl, host->pins_default); } /* @@ -1616,6 +1625,32 @@ static int mmci_probe(struct amba_device *dev, host = mmc_priv(mmc); host->mmc = mmc; + /* + * Some variant (STM32) doesn't have opendrain bit, nevertheless + * pins can be set accordingly using pinctrl + */ + if (!variant->opendrain) { + host->pinctrl = devm_pinctrl_get(&dev->dev); + if (IS_ERR(host->pinctrl)) { + dev_err(&dev->dev, "failed to get pinctrl"); + goto host_free; + } + + host->pins_default = pinctrl_lookup_state(host->pinctrl, + PINCTRL_STATE_DEFAULT); + if (IS_ERR(host->pins_default)) { + dev_warn(mmc_dev(mmc), "Can't select default pins\n"); + host->pins_default = NULL; + } + + host->pins_opendrain = pinctrl_lookup_state(host->pinctrl, + MMCI_PINCTRL_STATE_OPENDRAIN); + if (IS_ERR(host->pins_opendrain)) { + dev_warn(mmc_dev(mmc), "Can't select opendrain pins\n"); + host->pins_opendrain = NULL; + } + } + host->hw_designer = amba_manf(dev); host->hw_revision = amba_rev(dev); dev_dbg(mmc_dev(mmc), "designer ID = 0x%02x\n", host->hw_designer); diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h index 83160a9..f91cdf7 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h @@ -192,6 +192,8 @@ #define NR_SG 128 +#define MMCI_PINCTRL_STATE_OPENDRAIN "opendrain" + struct clk; struct variant_data; struct dma_chan; @@ -227,6 +229,9 @@ struct mmci_host { bool vqmmc_enabled; struct mmci_platform_data *plat; struct variant_data *variant; + struct pinctrl *pinctrl; + struct pinctrl_state *pins_default; + struct pinctrl_state *pins_opendrain; u8 hw_designer; u8 hw_revision:4;