Re: [PATCH v1 6/9] mmc: sdhci-cadence: rename V4 functions for V6 controller groundwork
From: Adrian Hunter
Date: Thu May 21 2026 - 13:55:24 EST
On 21/05/2026 18:53, Kathpalia, Tanmay wrote:
> Hi Adrian,
>
> On 5/19/2026 8:59 PM, Adrian Hunter wrote:
>> On 11/05/2026 23:21, Tanmay Kathpalia wrote:
>>> PHY-related functions and data structures in the driver are not
>>> explicitly scoped to the SD4HC (V4) controller, making it unclear
>>> which code is shared and which is version-specific.
>>>
>>> Rename them with a "cdns4" prefix to distinguish SD4HC-specific
>>> implementation from the shared driver core, and to avoid naming
>>> conflicts when SD6HC (V6) support is introduced.
>>>
>>> Signed-off-by: Tanmay Kathpalia <tanmay.kathpalia@xxxxxxxxxx>
>> One minor comment below
>
> Thanks for reviewing.
>
>>
>>> ---
>>> drivers/mmc/host/sdhci-cadence.c | 74 ++++++++++++++++----------------
>>> 1 file changed, 37 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
>>> index 435603c8c00b..47690a52a221 100644
>>> --- a/drivers/mmc/host/sdhci-cadence.c
>>> +++ b/drivers/mmc/host/sdhci-cadence.c
>>> @@ -78,7 +78,7 @@
>>> */
>>> #define SDHCI_CDNS_MAX_TUNING_LOOP 40
>>> -struct sdhci_cdns_phy_param {
>>> +struct sdhci_cdns4_phy_param {
>>> u8 addr;
>>> u8 data;
>>> };
>>> @@ -91,10 +91,10 @@ struct sdhci_cdns_priv {
>>> 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_cdns_phy_param phy_params[];
>>> + struct sdhci_cdns4_phy_param phy_params[];
>>> };
>>> -struct sdhci_cdns_phy_cfg {
>>> +struct sdhci_cdns4_phy_cfg {
>>> const char *property;
>>> u8 addr;
>>> };
>>> @@ -104,7 +104,7 @@ struct sdhci_cdns_drv_data {
>>> const struct sdhci_pltfm_data pltfm_data;
>>> };
>>> -static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
>>> +static const struct sdhci_cdns4_phy_cfg sdhci_cdns4_phy_cfgs[] = {
>>> { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
>>> { "cdns,phy-input-delay-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
>>> { "cdns,phy-input-delay-sd-uhs-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
>>> @@ -124,8 +124,8 @@ static inline void cdns_writel(struct sdhci_cdns_priv *priv, u32 val,
>>> writel(val, reg);
>>> }
>>> -static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
>>> - u8 addr, u8 data)
>>> +static int sdhci_cdns4_write_phy_reg(struct sdhci_cdns_priv *priv,
>>> + u8 addr, u8 data)
>>> {
>>> void __iomem *reg = priv->hrs_addr + SDHCI_CDNS_HRS04;
>>> u32 tmp;
>>> @@ -156,44 +156,44 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
>>> return ret;
>>> }
>>> -static unsigned int sdhci_cdns_phy_param_count(struct device_node *np)
>>> +static unsigned int sdhci_cdns4_phy_param_count(struct device_node *np)
>>> {
>>> unsigned int count = 0;
>>> int i;
>>> - for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs); i++)
>>> - if (of_property_present(np, sdhci_cdns_phy_cfgs[i].property))
>>> + for (i = 0; i < ARRAY_SIZE(sdhci_cdns4_phy_cfgs); i++)
>>> + if (of_property_present(np, sdhci_cdns4_phy_cfgs[i].property))
>>> count++;
>>> return count;
>>> }
>>> -static void sdhci_cdns_phy_param_parse(struct device_node *np,
>>> - struct sdhci_cdns_priv *priv)
>>> +static void sdhci_cdns4_phy_param_parse(struct device_node *np,
>>> + struct sdhci_cdns_priv *priv)
>>> {
>>> - struct sdhci_cdns_phy_param *p = priv->phy_params;
>>> + struct sdhci_cdns4_phy_param *p = priv->phy_params;
>>> u32 val;
>>> int ret, i;
>>> - for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs); i++) {
>>> - ret = of_property_read_u32(np, sdhci_cdns_phy_cfgs[i].property,
>>> + for (i = 0; i < ARRAY_SIZE(sdhci_cdns4_phy_cfgs); i++) {
>>> + ret = of_property_read_u32(np, sdhci_cdns4_phy_cfgs[i].property,
>>> &val);
>>> if (ret)
>>> continue;
>>> - p->addr = sdhci_cdns_phy_cfgs[i].addr;
>>> + p->addr = sdhci_cdns4_phy_cfgs[i].addr;
>>> p->data = val;
>>> p++;
>>> }
>>> }
>>> -static int sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
>>> +static int sdhci_cdns4_phy_init(struct sdhci_cdns_priv *priv)
>>> {
>>> int ret, i;
>>> for (i = 0; i < priv->nr_phy_params; i++) {
>>> - ret = sdhci_cdns_write_phy_reg(priv, priv->phy_params[i].addr,
>>> - priv->phy_params[i].data);
>>> + ret = sdhci_cdns4_write_phy_reg(priv, priv->phy_params[i].addr,
>>> + priv->phy_params[i].data);
>>> if (ret)
>>> return ret;
>>> }
>>> @@ -201,7 +201,7 @@ static int sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
>>> return 0;
>>> }
>>> -static void *sdhci_cdns_priv(struct sdhci_host *host)
>>> +static void *sdhci_cdns_get_priv(struct sdhci_host *host)
>> Why rename this? It seems to just create churn.
>
> The reason was to resolve a name collision: in the original code
> both the data structure and the accessor function were named
> sdhci_cdns_priv, which can reduce readability. Renaming the
> accessor to sdhci_cdns_get_priv() makes the intent clearer and
> avoids this ambiguity.
> If you would prefer to keep the original name and avoid the churn,
> I will revert this rename in v2.
Yes please
>
>>
>>> {
>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> @@ -238,7 +238,7 @@ static u32 sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv)
>>> static int sdhci_cdns_set_tune_val(struct sdhci_host *host, unsigned int val)
>>> {
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> void __iomem *reg = priv->hrs_addr + SDHCI_CDNS_HRS06;
>>> u32 tmp;
>>> int i, ret;
>>> @@ -354,7 +354,7 @@ static int sdhci_cdns_execute_tuning(struct sdhci_host *host, u32 opcode)
>>> static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
>>> unsigned int timing)
>>> {
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> u32 mode;
>>> switch (timing) {
>>> @@ -407,12 +407,12 @@ static void elba_priv_writel(struct sdhci_cdns_priv *priv, u32 val,
>>> static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
>>> {
>>> - elba_priv_writel(sdhci_cdns_priv(host), val, host->ioaddr + reg);
>>> + elba_priv_writel(sdhci_cdns_get_priv(host), val, host->ioaddr + reg);
>>> }
>>> static void elba_write_w(struct sdhci_host *host, u16 val, int reg)
>>> {
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> u32 shift = reg & GENMASK(1, 0);
>>> unsigned long flags;
>>> u32 byte_enables;
>>> @@ -426,7 +426,7 @@ static void elba_write_w(struct sdhci_host *host, u16 val, int reg)
>>> static void elba_write_b(struct sdhci_host *host, u8 val, int reg)
>>> {
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> u32 shift = reg & GENMASK(1, 0);
>>> unsigned long flags;
>>> u32 byte_enables;
>>> @@ -452,7 +452,7 @@ static const struct sdhci_ops sdhci_elba_ops = {
>>> static int elba_drv_init(struct platform_device *pdev)
>>> {
>>> struct sdhci_host *host = platform_get_drvdata(pdev);
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> void __iomem *ioaddr;
>>> host->mmc->caps |= MMC_CAP_1_8V_DDR | MMC_CAP_8_BIT_DATA;
>>> @@ -470,7 +470,7 @@ static int elba_drv_init(struct platform_device *pdev)
>>> return 0;
>>> }
>>> -static const struct sdhci_ops sdhci_cdns_ops = {
>>> +static const struct sdhci_ops sdhci_cdns4_ops = {
>>> .set_clock = sdhci_set_clock,
>>> .get_timeout_clock = sdhci_cdns_get_timeout_clock,
>>> .set_bus_width = sdhci_set_bus_width,
>>> @@ -481,7 +481,7 @@ static const struct sdhci_ops sdhci_cdns_ops = {
>>> static const struct sdhci_cdns_drv_data sdhci_cdns_uniphier_drv_data = {
>>> .pltfm_data = {
>>> - .ops = &sdhci_cdns_ops,
>>> + .ops = &sdhci_cdns4_ops,
>>> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> },
>>> };
>>> @@ -495,14 +495,14 @@ static const struct sdhci_cdns_drv_data sdhci_elba_drv_data = {
>>> static const struct sdhci_cdns_drv_data sdhci_eyeq_drv_data = {
>>> .pltfm_data = {
>>> - .ops = &sdhci_cdns_ops,
>>> + .ops = &sdhci_cdns4_ops,
>>> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> },
>>> };
>>> -static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = {
>>> +static const struct sdhci_cdns_drv_data sdhci_cdns4_drv_data = {
>>> .pltfm_data = {
>>> - .ops = &sdhci_cdns_ops,
>>> + .ops = &sdhci_cdns4_ops,
>>> },
>>> };
>>> @@ -510,7 +510,7 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
>>> struct mmc_ios *ios)
>>> {
>>> struct sdhci_host *host = mmc_priv(mmc);
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> u32 mode;
>>> priv->enhanced_strobe = ios->enhanced_strobe;
>>> @@ -529,7 +529,7 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
>>> static void sdhci_cdns_mmc_hw_reset(struct mmc_host *mmc)
>>> {
>>> struct sdhci_host *host = mmc_priv(mmc);
>>> - struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
>>> + struct sdhci_cdns_priv *priv = sdhci_cdns_get_priv(host);
>>> dev_dbg(mmc_dev(host->mmc), "emmc hardware reset\n");
>>> @@ -560,9 +560,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>>> data = of_device_get_match_data(dev);
>>> if (!data)
>>> - data = &sdhci_cdns_drv_data;
>>> + data = &sdhci_cdns4_drv_data;
>>> - nr_phy_params = sdhci_cdns_phy_param_count(dev->of_node);
>>> + 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));
>>> if (IS_ERR(host))
>>> @@ -593,9 +593,9 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>>> if (ret)
>>> return ret;
>>> - sdhci_cdns_phy_param_parse(dev->of_node, priv);
>>> + sdhci_cdns4_phy_param_parse(dev->of_node, priv);
>>> - ret = sdhci_cdns_phy_init(priv);
>>> + ret = sdhci_cdns4_phy_init(priv);
>>> if (ret)
>>> return ret;
>>> @@ -622,7 +622,7 @@ static int sdhci_cdns_resume(struct device *dev)
>>> if (ret)
>>> return ret;
>>> - ret = sdhci_cdns_phy_init(priv);
>>> + ret = sdhci_cdns4_phy_init(priv);
>>> if (ret)
>>> goto disable_clk;
>>>