Re: [PATCH v1 7/9] mmc: sdhci-cadence: refactor driver structure for V6 controller support

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

On 11/05/2026 23:21, Tanmay Kathpalia wrote:
> Refactor the sdhci-cadence driver in preparation for adding SD6HC
> (V6 controller) support. Separate PHY parameter handling into a
> dedicated sdhci_cdns4_phy structure and move PHY initialization
> logic into a dedicated sdhci_cdns4_phy_probe() function. This
> allows different controller versions to manage their PHY
> configurations independently while keeping shared logic in the
> main driver.
> 
> Each compatible entry now carries its own driver data, so drop the
> silent fallback to sdhci_cdns4_drv_data and return an error if
> platform data is missing.
> 
> Signed-off-by: Tanmay Kathpalia <tanmay.kathpalia@altera.com>

A few minor style comments below

> ---
>  drivers/mmc/host/sdhci-cadence.c | 57 ++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 47690a52a221..fe3f7c5109fc 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -83,6 +83,11 @@ struct sdhci_cdns4_phy_param {
>  	u8 data;
>  };
>  
> +struct sdhci_cdns4_phy {
> +	unsigned int nr_phy_params;
> +	struct sdhci_cdns4_phy_param phy_params[];
> +};
> +
>  struct sdhci_cdns_priv {
>  	void __iomem *hrs_addr;
>  	void __iomem *ctl_addr;	/* write control */
> @@ -90,8 +95,7 @@ struct sdhci_cdns_priv {
>  	bool enhanced_strobe;
>  	void (*priv_writel)(struct sdhci_cdns_priv *priv, u32 val, void __iomem *reg);
>  	struct reset_control *rst_hw;
> -	unsigned int nr_phy_params;
> -	struct sdhci_cdns4_phy_param phy_params[];
> +	struct sdhci_cdns4_phy *phy;
>  };
>  
>  struct sdhci_cdns4_phy_cfg {
> @@ -169,9 +173,9 @@ static unsigned int sdhci_cdns4_phy_param_count(struct device_node *np)
>  }
>  
>  static void sdhci_cdns4_phy_param_parse(struct device_node *np,
> -					struct sdhci_cdns_priv *priv)
> +					struct sdhci_cdns4_phy *phy)
>  {
> -	struct sdhci_cdns4_phy_param *p = priv->phy_params;
> +	struct sdhci_cdns4_phy_param *p = phy->phy_params;
>  	u32 val;
>  	int ret, i;
>  
> @@ -190,10 +194,11 @@ static void sdhci_cdns4_phy_param_parse(struct device_node *np,
>  static int sdhci_cdns4_phy_init(struct sdhci_cdns_priv *priv)
>  {
>  	int ret, i;
> +	struct sdhci_cdns4_phy *phy = priv->phy;

Nicer to place locals in reverse order of line length e.g.

+	struct sdhci_cdns4_phy *phy = priv->phy;
	int ret, i;

>  
> -	for (i = 0; i < priv->nr_phy_params; i++) {
> -		ret = sdhci_cdns4_write_phy_reg(priv, priv->phy_params[i].addr,
> -						priv->phy_params[i].data);
> +	for (i = 0; i < phy->nr_phy_params; i++) {
> +		ret = sdhci_cdns4_write_phy_reg(priv, phy->phy_params[i].addr,
> +						phy->phy_params[i].data);
>  		if (ret)
>  			return ret;
>  	}
> @@ -542,6 +547,26 @@ static void sdhci_cdns_mmc_hw_reset(struct mmc_host *mmc)
>  	usleep_range(300, 1000);
>  }
>  
> +static int sdhci_cdns4_phy_probe(struct platform_device *pdev,
> +				 struct sdhci_cdns_priv *priv)

Please wrap after 100 columns not 80

> +{
> +	unsigned int nr_phy_params;
> +	struct sdhci_cdns4_phy *phy;
> +	struct device *dev = &pdev->dev;

Nicer to place locals in reverse order of line length e.g.

+	struct device *dev = &pdev->dev;
+	struct sdhci_cdns4_phy *phy;
+	unsigned int nr_phy_params;

> +
> +	nr_phy_params = sdhci_cdns4_phy_param_count(dev->of_node);
> +	phy = devm_kzalloc(dev, struct_size(phy, phy_params, nr_phy_params),
> +			   GFP_KERNEL);

Please wrap after 100 columns not 80

> +	if (!phy)
> +		return -ENOMEM;
> +
> +	phy->nr_phy_params = nr_phy_params;
> +	sdhci_cdns4_phy_param_parse(dev->of_node, phy);
> +	priv->phy = phy;
> +
> +	return sdhci_cdns4_phy_init(priv);
> +}
> +
>  static int sdhci_cdns_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -549,7 +574,6 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host;
>  	struct sdhci_cdns_priv *priv;
>  	struct clk *clk;
> -	unsigned int nr_phy_params;
>  	int ret;
>  	struct device *dev = &pdev->dev;
>  	static const u16 version = SDHCI_SPEC_400 << SDHCI_SPEC_VER_SHIFT;
> @@ -560,11 +584,10 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>  
>  	data = of_device_get_match_data(dev);
>  	if (!data)
> -		data = &sdhci_cdns4_drv_data;
> +		return dev_err_probe(dev, -EINVAL,
> +				     "missing platform driver data\n");

Please wrap after 100 columns not 80

>  
> -	nr_phy_params = sdhci_cdns4_phy_param_count(dev->of_node);
> -	host = sdhci_pltfm_init(pdev, &data->pltfm_data,
> -				struct_size(priv, phy_params, nr_phy_params));
> +	host = sdhci_pltfm_init(pdev, &data->pltfm_data, sizeof(*priv));
>  	if (IS_ERR(host))
>  		return PTR_ERR(host);
>  
> @@ -572,7 +595,6 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>  	pltfm_host->clk = clk;
>  
>  	priv = sdhci_pltfm_priv(pltfm_host);
> -	priv->nr_phy_params = nr_phy_params;
>  	priv->hrs_addr = host->ioaddr;
>  	priv->enhanced_strobe = false;
>  	priv->priv_writel = cdns_writel;
> @@ -593,9 +615,7 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	sdhci_cdns4_phy_param_parse(dev->of_node, priv);
> -
> -	ret = sdhci_cdns4_phy_init(priv);
> +	ret = sdhci_cdns4_phy_probe(pdev, priv);
>  	if (ret)
>  		return ret;
>  
> @@ -653,7 +673,10 @@ static const struct of_device_id sdhci_cdns_match[] = {
>  		.compatible = "mobileye,eyeq-sd4hc",
>  		.data = &sdhci_eyeq_drv_data,
>  	},
> -	{ .compatible = "cdns,sd4hc" },
> +	{
> +		.compatible = "cdns,sd4hc",
> +		.data = &sdhci_cdns4_drv_data,
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_cdns_match);