[PATCH 03/14] mmc: mmci: Add support for setting pad type via pinctrl
From: Ulf Hansson <hidden>
Date: 2018-01-15 12:43:55
Also in:
linux-clk, linux-devicetree, linux-gpio, linux-mmc, lkml
On 12 January 2018 at 13:15, [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Patrice Chotard <redacted> The STM32 variant hasn't the control bit to switch pads in opendrain mode. In this case we can achieve the same result by asking to the pinmux driver to configure pins for us. This patch make the mmci driver able to do this whenever needed. Signed-off-by: Andrea Merello <redacted> Signed-off-by: Patrice Chotard <redacted> --- drivers/mmc/host/mmci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------- drivers/mmc/host/mmci.h | 5 +++++ 2 files changed, 50 insertions(+), 9 deletions(-)diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index 7e56f85..38e8c20 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c@@ -85,6 +85,8 @@ * @mmcimask1: true if variant have a MMCIMASK1 register. * @start_err: true is the variant has STARTBITERR bit inside MMCISTATUS * register. + * @opendrain: true if variant have dedicated bit for opendrain pins + * configuration. */ struct variant_data { unsigned int clkreg;@@ -116,6 +118,7 @@ struct variant_data { bool reversed_irq_handling; bool mmcimask1; bool start_err; + bool opendrain;
Similar comment as for patch2. To be consistent with how we implement support for similar variant variations, I would prefer to have this being a u32. Something along the lines of how the "busy_detect_flag" is being used. [...]
quoted hunk ↗ jump to hunk
@@ -1394,9 +1405,11 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct mmci_host *host = mmc_priv(mmc); struct variant_data *variant = host->variant; + struct pinctrl_state *pins; u32 pwr = 0; unsigned long flags; int ret; + bool is_opendrain; if (host->plat->ios_handler && host->plat->ios_handler(mmc_dev(mmc), ios))@@ -1455,16 +1468,31 @@ static void mmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) ~MCI_ST_DATA2DIREN); } - if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) { - if (host->hw_designer != AMBA_VENDOR_ST) - pwr |= MCI_ROD; - else { - /* - * The ST Micro variant use the ROD bit for something - * else and only has OD (Open Drain). - */ - pwr |= MCI_OD;
Seems like you should actually split this change into two parts. One that adds the variant flag for the open drain bit, when then can clean up this code. Then a patch on top that starts using pinctrl in case there is no open drain bit set. Does that sounds reasonable?
+ if (host->variant->opendrain) {
+ if (ios->bus_mode == MMC_BUSMODE_OPENDRAIN) {
+ if (host->hw_designer != AMBA_VENDOR_ST) {
+ pwr |= MCI_ROD;
+ } else {
+ /*
+ * The ST Micro variant use the ROD bit for
+ * something else and only has OD (Open Drain).
+ */
+ pwr |= MCI_OD;
+ }
}
+ } else {
+ /*
+ * If the variant cannot configure the pads by its own, then we
+ * expect the pinctrl to be able to do that for us
+ */
+ is_opendrain = (ios->bus_mode == MMC_BUSMODE_OPENDRAIN);
+ pins = pinctrl_lookup_state(host->pinctrl, is_opendrain ?How about doing the lookup in ->probe() instead? Then just select the state here, if supported?
quoted hunk ↗ jump to hunk
+ MMCI_PINCTRL_STATE_OPENDRAIN : + MMCI_PINCTRL_STATE_PUSHPULL); + if (IS_ERR(pins)) + dev_warn(mmc_dev(mmc), "Cannot select pin drive type via pinctrl\n"); + else + pinctrl_select_state(host->pinctrl, pins); } /*@@ -1609,6 +1637,14 @@ static int mmci_probe(struct amba_device *dev, host = mmc_priv(mmc); host->mmc = mmc; + 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->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..de3d0b3 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h@@ -192,6 +192,10 @@ #define NR_SG 128 +/* pinctrl configs */ +#define MMCI_PINCTRL_STATE_PUSHPULL "default"
Seems like we should be able to cope fine without having to add a separate define for the PUSHPULL, but rather just select the default state instead.
quoted hunk ↗ jump to hunk
+#define MMCI_PINCTRL_STATE_OPENDRAIN "opendrain" + struct clk; struct variant_data; struct dma_chan;@@ -227,6 +231,7 @@ struct mmci_host { bool vqmmc_enabled; struct mmci_platform_data *plat; struct variant_data *variant; + struct pinctrl *pinctrl; u8 hw_designer; u8 hw_revision:4; --1.9.1
Kind regards Uffe