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

[Kathpalia, Tanmay]

Hi Adrian,

Thanks for your feedback. I will fix all of the below in v2.

On 5/19/2026 8:59 PM, Adrian Hunter wrote:
> 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@xxxxxxxxxx>
> 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);