Hi Ulf
On 01/15/2018 01:43 PM, Ulf Hansson wrote:
> On 12 January 2018 at 13:15, <patrice.chotard@xxxxxx> wrote:
>> From: Patrice Chotard <patrice.chotard@xxxxxx>
>>
>> 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 <andrea.merello@xxxxxxxxx>
>> Signed-off-by: Patrice Chotard <patrice.chotard@xxxxxx>
>> ---
>> 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.
ok
>
> [...]
>
>> @@ -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?
Of course
>
>> + 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?
ok
>
>> + 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.
yes agree
Thanks
Patrice
>
>> +#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
> ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
