Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933054AbeAOMnz (ORCPT + 1 other); Mon, 15 Jan 2018 07:43:55 -0500 Received: from mail-it0-f68.google.com ([209.85.214.68]:34180 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966405AbeAOMnn (ORCPT ); Mon, 15 Jan 2018 07:43:43 -0500 X-Google-Smtp-Source: ACJfBou/BajaPKAkw4RO80jNdQjsJWACewE8DoCVL4E/FP/CVOv2A71uGFAsKHvlcNG3IlQYxhXlvT721LYJ41nZi00= MIME-Version: 1.0 In-Reply-To: <1515759368-16946-4-git-send-email-patrice.chotard@st.com> References: <1515759368-16946-1-git-send-email-patrice.chotard@st.com> <1515759368-16946-4-git-send-email-patrice.chotard@st.com> From: Ulf Hansson Date: Mon, 15 Jan 2018 13:43:41 +0100 Message-ID: Subject: Re: [PATCH 03/14] mmc: mmci: Add support for setting pad type via pinctrl To: Patrice CHOTARD Cc: Russell King , Michael Turquette , Stephen Boyd , Linus Walleij , Rob Herring , Mark Rutland , Alexandre Torgue , "linux-mmc@vger.kernel.org" , Linux Kernel Mailing List , linux-clk , linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, Andrea Merello Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 12 January 2018 at 13:15, wrote: > From: Patrice Chotard > > 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 > Signed-off-by: Patrice Chotard > --- > 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. [...] > @@ -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? > + 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. > +#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