Re: [PATCH RFC 2/2] mmc: sdhci-pxav3: add state_uhs pinctrl setting

[Adrian Hunter <adrian.hunter@intel.com>]

On 19/07/2025 00:12, Duje Mihanović wrote:
> Different bus clocks require different pinctrl states to remain stable.
> Add support for selecting between a default and UHS state according to
> the bus clock.
> 
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> ---
>  drivers/mmc/host/sdhci-pxav3.c          | 30 +++++++++++++++++++++++++++++-
>  include/linux/platform_data/pxa_sdhci.h |  7 +++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 3fb56face3d81259b693c8569682d05c95be2880..0e1bd75a1252e5559deb0b3caabcdc75919aee3d 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -20,9 +20,11 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/mbus.h>
> +#include <linux/units.h>
>  
>  #include "sdhci.h"
>  #include "sdhci-pltfm.h"
> @@ -313,8 +315,24 @@ static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
>  		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>  }
>  
> +static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
> +	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
> +
> +	if (clock < 100 * HZ_PER_MHZ) {
> +		if (!IS_ERR(pdata->pins_default))
> +			pinctrl_select_state(pdata->pinctrl, pdata->pins_default);
> +	} else {
> +		if (!IS_ERR(pdata->pins_uhs))
> +			pinctrl_select_state(pdata->pinctrl, pdata->pins_uhs);
> +	}
> +
> +	sdhci_set_clock(host, clock);
> +}
> +
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> -	.set_clock = sdhci_set_clock,
> +	.set_clock = pxav3_set_clock,
>  	.set_power = pxav3_set_power,
>  	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
>  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> @@ -441,6 +459,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  			host->mmc->pm_caps |= pdata->pm_caps;
>  	}
>  
> +	pdata->pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(pdata->pinctrl))
> +		dev_warn(dev, "could not get pinctrl handle\n");
> +	pdata->pins_default = pinctrl_lookup_state(pdata->pinctrl, "default");

Mustn't use pdata->pinctrl if it is not valid

> +	if (IS_ERR(pdata->pins_default))
> +		dev_warn(dev, "could not get default state\n");
> +	pdata->pins_uhs = pinctrl_lookup_state(pdata->pinctrl, "state_uhs");

Mustn't use pdata->pinctrl if it is not valid, and probably don't
want to change pin state unless both pdata->pins_default and
pdata->pins_uhs are valid

> +	if (IS_ERR(pdata->pins_uhs))
> +		dev_warn(dev, "could not get uhs state\n");

I'd suggest dev_dbg() rather than dev_warn() for all the
new dev_warn()s

> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
> diff --git a/include/linux/platform_data/pxa_sdhci.h b/include/linux/platform_data/pxa_sdhci.h
> index 899457cee425d33f82606f0b8c280003bc73d48d..540aa36db11243719707bdf22db23a8e2035674d 100644
> --- a/include/linux/platform_data/pxa_sdhci.h
> +++ b/include/linux/platform_data/pxa_sdhci.h
> @@ -35,6 +35,9 @@
>   * @quirks: quirks of platfrom
>   * @quirks2: quirks2 of platfrom
>   * @pm_caps: pm_caps of platfrom
> + * @pinctrl: pinctrl handle
> + * @pins_default: default pinctrl state
> + * @pins_uhs: pinctrl state for fast (>100 MHz) bus clocks
>   */
>  struct sdhci_pxa_platdata {
>  	unsigned int	flags;
> @@ -47,5 +50,9 @@ struct sdhci_pxa_platdata {
>  	unsigned int	quirks;
>  	unsigned int	quirks2;
>  	unsigned int	pm_caps;
> +
> +	struct pinctrl	     *pinctrl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_uhs;
>  };
>  #endif /* _PXA_SDHCI_H_ */
>