linux-kernel - Re: [PATCH v3 2/2] mmc: sdhci-pxav3: add state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7bddc691-df7a-430a-b7e7-01ad85c27cbe@intel.com>
Date: Mon, 18 Aug 2025 10:25:56 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Duje Mihanović <duje@...emihanovic.xyz>, Ulf Hansson
	<ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
CC: Karel Balej <balejk@...fyz.cz>, David Wronek <david@...nlining.org>,
	<linux-mmc@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <phone-devel@...r.kernel.org>,
	<~postmarketos/upstreaming@...ts.sr.ht>
Subject: Re: [PATCH v3 2/2] mmc: sdhci-pxav3: add state_uhs pinctrl setting

On 06/08/2025 12:43, 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@...emihanovic.xyz>

One minor cosmetic issue below.  Address that and add:

Acked-by: Adrian Hunter <adrian.hunter@...el.com>

> ---
> Changes in v3:
> - Move pinctrl stuff out of platdata
> - Add helper for pinstate lookup
> - Thanks to Adrian for the suggestions
> 
> Changes in v2:
> - Don't attempt to lookup pinstates if getting pinctrl fails
> - Only select pinstates if both of them are valid
> - dev_warn() -> dev_dbg()
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 3fb56face3d81259b693c8569682d05c95be2880..ceb06f1c18146eac296c6d20483a9f006567a978 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"
> @@ -51,6 +53,9 @@ struct sdhci_pxa {
>  	struct clk *clk_io;
>  	u8	power_mode;
>  	void __iomem *sdio3_conf_reg;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_uhs;
>  };
>  
>  /*
> @@ -313,8 +318,20 @@ 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 sdhci_pltfm_host *phost = sdhci_priv(host);
> +	struct sdhci_pxa *pxa = sdhci_pltfm_priv(phost);
> +	struct pinctrl_state *pins = clock < 100 * HZ_PER_MHZ ? pxa->pins_default : pxa->pins_uhs;
> +
> +	if (pins)
> +		pinctrl_select_state(pxa->pinctrl, pins);
> +
> +	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,
> @@ -366,6 +383,19 @@ static inline struct sdhci_pxa_platdata *pxav3_get_mmc_pdata(struct device *dev)
>  }
>  #endif
>  
> +static struct pinctrl_state *pxav3_lookup_pinstate(struct device *dev, struct pinctrl *pinctrl,
> +						   const char *name)
> +{
> +	struct pinctrl_state *pins = pinctrl_lookup_state(pinctrl, name);
> +
> +	if (IS_ERR(pins)) {
> +		dev_dbg(dev, "could not get pinstate '%s': %ld\n", name, PTR_ERR(pins));
> +		return NULL;
> +	}
> +
> +	return pins;
> +}
> +
>  static int sdhci_pxav3_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_pltfm_host *pltfm_host;
> @@ -441,6 +471,14 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  			host->mmc->pm_caps |= pdata->pm_caps;
>  	}
>  
> +	pxa->pinctrl = devm_pinctrl_get(dev);
> +	if (!IS_ERR(pxa->pinctrl)) {
> +		pxa->pins_default = pxav3_lookup_pinstate(dev, pxa->pinctrl, "default");
> +		if (pxa->pins_default)
> +			pxa->pins_uhs = pxav3_lookup_pinstate(dev, pxa->pinctrl, "state_uhs");
> +	} else
> +		dev_dbg(dev, "could not get pinctrl handle: %ld\n", PTR_ERR(pxa->pinctrl));

Kernel style is to have braces here.  ./scripts/checkpatch.pl --strict
can tell about that kind of thing:

CHECK: braces {} should be used on all arms of this statement
#92: FILE: drivers/mmc/host/sdhci-pxav3.c:474:
+       if (!IS_ERR(pxa->pinctrl)) {
[...]
+       } else
[...]

CHECK: Unbalanced braces around else statement
#96: FILE: drivers/mmc/host/sdhci-pxav3.c:478:
+       } else


> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
>