-----Original Message-----
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
Sent: Wednesday, May 21, 2025 7:45 PM
To: Luke Wang <redacted>; Bough Chen
[off-list ref]; adrian.hunter@intel.com; ulf.hansson@linaro.org;
linux-mmc@vger.kernel.org
Cc: imx@lists.linux.dev; dl-S32 <S32@nxp.com>; shawnguo@kernel.org;
s.hauer@pengutronix.de; linux-kernel@vger.kernel.org;
kernel@pengutronix.de; festevam@gmail.com; linux-arm-
kernel@lists.infradead.org
Subject: [EXT] Re: [PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from
pins_200mhz when unconfigured

[You don't often get email from a.fatoum@pengutronix.de. Learn why this is
important at https://aka.ms/LearnAboutSenderIdentification ]

Caution: This is an external email. Please take care when clicking links or
opening attachments. When in doubt, report the message using the 'Report
this email' button


Hi Luke,

Thanks for your patch.

On 21.05.25 13:20, ziniu.wang_1@nxp.com wrote:

quoted

From: Luke Wang <redacted>

On some new i.MX platforms, hardware guidelines recommend using

identical

quoted

pin configurations for SDR50/DDR50 (100MHz) and SDR104/HS400

(200MHz)

quoted

modes. But defining two identical pinctrl for 100MHz and 200MHz in dts
creates redundancy.

I am not convinced this is an improvement. If 200mhz is missing, it's
understood
that it's not supported. Now if 100mhz is missing, it means something
different
depending on whether state_200mhz exists or not. This is more mental
overhead
than having:

  pinctrl-names = "default", "state_100mhz", "state_200mhz";
  pinctrl-0 = <&pinctrl_usdhc1>;
  pinctrl-1 = <&pinctrl_usdhc1_uhs>;
  pinctrl-2 = <&pinctrl_usdhc1_uhs>;

Hi Ahmad,

This way looks better. Will use pinctrl_usdhc1_uhs for both state_100mhz and state_200mhz if there are identical.

Thanks
Luke

where it's directly evident that you share pinctrl states.

quoted

In this case, omit explicit 100MHz configuration,
driver will inherit 100MHz pinctrl from 200MHz.

Preserves existing behavior if 100MHz is configured but 200MHz not (e.g,
imx8mp-navq.dts usdhc1 supports SDR50/DDR50 but SDR104/HS400 not).

This conflicts with the binding, which doesn't allow for state_200mhz
to exist without state_100mhz, so that would need adaptation.

As noted before though, I don't think we really need to change anything
here though.

Thanks,
Ahmad

quoted

Signed-off-by: Luke Wang <redacted>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-

esdhc-imx.c

quoted

index f206b562a6e3..dfd8be5000c8 100644

--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c

@@ -1810,6 +1810,9 @@ sdhci_esdhc_imx_probe_dt(struct

platform_device *pdev,

quoted

                                              ESDHC_PINCTRL_STATE_100MHZ);
              imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
                                              ESDHC_PINCTRL_STATE_200MHZ);
+
+             if (IS_ERR_OR_NULL(imx_data->pins_100mhz))
+                     imx_data->pins_100mhz = imx_data->pins_200mhz;
      }

      /* call to generic mmc_of_parse to support additional capabilities */

--
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       |
http://www.p/
engutronix.de%2F&data=05%7C02%7Cziniu.wang_1%40nxp.com%7Cd8145bf
22a114de4256f08dd985ced07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
0%7C0%7C638834247059505189%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0
eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpb
CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=tXhmm%2Bd3rGbtfl2VorcnL
OvfZ8NcVFc0EJ%2B4fTxYJns%3D&reserved=0  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |