Re: [PATCH v1 6/9] mmc: sdhci-cadence: rename V4 functions for V6 controller groundwork

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

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@altera.com>
>> 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;
>>>