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

["Kathpalia, Tanmay" <tanmay.kathpalia@altera.com>]

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@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);