| Message ID | 20250410090102.20781-6-quic_nitirawa@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor phy powerup sequence | expand |
On 4/14/2025 1:13 PM, Dmitry Baryshkov wrote: > On Mon, Apr 14, 2025 at 12:58:48PM +0530, Nitin Rawat wrote: >> >> >> On 4/11/2025 4:26 PM, Dmitry Baryshkov wrote: >>> On Fri, 11 Apr 2025 at 13:42, Nitin Rawat <quic_nitirawa@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 4/11/2025 1:39 AM, Dmitry Baryshkov wrote: >>>>> On Thu, Apr 10, 2025 at 02:30:58PM +0530, Nitin Rawat wrote: >>>>>> Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into >>>>>> qmp_ufs_power_on(). This change removes unnecessary function calls and >>>>>> ensures that the initialization logic is directly within the power-on >>>>>> routine, maintaining the same functionality. >>>>> >>>>> Which problem is this patch trying to solve? >>>> >>>> Hi Dmitry, >>>> >>>> As part of the patch, I simplified the code by moving qmp_ufs_com_init >>>> inline to qmp_ufs_power_on, since qmp_ufs_power_on was merely calling >>>> qmp_ufs_com_init. This change eliminates unnecessary function call. >>> >>> You again are describing what you did. Please start by stating the >>> problem or the issue. >>> >>>> >> Hi Dmitry, >> >> Sure, will update the commit with "problem" first in the next patchset when >> I post. > > Before posting the next iteration, maybe you can respond inline? It well > might be that there is no problem to solve.a Hi Dmitry, Apologies for late reply , I just realized I missed responding to your comment on this patch. There is no functional "problem" here. =================================================================== The qmp_ufs_power_on() function acts as a wrapper, solely invoking qmp_ufs_com_init(). Additionally, the code within qmp_ufs_com_init() does not correspond well with its name. Therefore, to enhance the readability and eliminate unnecessary function call inline qmp_ufs_com_init() into qmp_ufs_power_on(). There is no change to the functionality. ================================================================== I agree with you that there isn't a significant issue here. If you insist, I'm okay with skipping this patch. Let me know your thoughts. Regards, Nitin > >> >> Thanks, >> Nitin >> >>>> Regards, >>>> Nitin >>>> >>>> >>>> >>>>> >>>>>> >>>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >>>>>> --- >>>>>> drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++--------------- >>>>>> 1 file changed, 18 insertions(+), 26 deletions(-) >>>>>> >>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c >>>>>> index 12dad28cc1bd..2cc819089d71 100644 >>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c >>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c >>>>>> @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg >>>>>> qmp_ufs_init_all(qmp, &cfg->tbls_hs_b); >>>>>> } >>>>>> >>>>>> -static int qmp_ufs_com_init(struct qmp_ufs *qmp) >>>>>> -{ >>>>>> - const struct qmp_phy_cfg *cfg = qmp->cfg; >>>>>> - void __iomem *pcs = qmp->pcs; >>>>>> - int ret; >>>>>> - >>>>>> - ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >>>>>> - if (ret) { >>>>>> - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); >>>>>> - return ret; >>>>>> - } >>>>>> - >>>>>> - ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks); >>>>>> - if (ret) >>>>>> - goto err_disable_regulators; >>>>>> - >>>>>> - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN); >>>>>> - >>>>>> - return 0; >>>>>> - >>>>>> -err_disable_regulators: >>>>>> - regulator_bulk_disable(cfg->num_vregs, qmp->vregs); >>>>>> - >>>>>> - return ret; >>>>>> -} >>>>>> >>>>>> static int qmp_ufs_com_exit(struct qmp_ufs *qmp) >>>>>> { >>>>>> @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp) >>>>>> static int qmp_ufs_power_on(struct phy *phy) >>>>>> { >>>>>> struct qmp_ufs *qmp = phy_get_drvdata(phy); >>>>>> + const struct qmp_phy_cfg *cfg = qmp->cfg; >>>>>> + void __iomem *pcs = qmp->pcs; >>>>>> int ret; >>>>>> + >>>>>> dev_vdbg(qmp->dev, "Initializing QMP phy\n"); >>>>>> >>>>>> - ret = qmp_ufs_com_init(qmp); >>>>>> + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); >>>>>> + if (ret) { >>>>>> + dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks); >>>>>> + if (ret) >>>>>> + goto err_disable_regulators; >>>>>> + >>>>>> + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN); >>>>>> + return 0; >>>>>> + >>>>>> +err_disable_regulators: >>>>>> + regulator_bulk_disable(cfg->num_vregs, qmp->vregs); >>>>>> return ret; >>>>>> } >>>>>> >>>>>> -- >>>>>> 2.48.1 >>>>>> >>>>> >>>> >>> >>> >> >> >> -- >> linux-phy mailing list >> linux-phy@lists.infradead.org >> https://lists.infradead.org/mailman/listinfo/linux-phy >
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c index 12dad28cc1bd..2cc819089d71 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c @@ -1757,31 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg qmp_ufs_init_all(qmp, &cfg->tbls_hs_b); } -static int qmp_ufs_com_init(struct qmp_ufs *qmp) -{ - const struct qmp_phy_cfg *cfg = qmp->cfg; - void __iomem *pcs = qmp->pcs; - int ret; - - ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); - if (ret) { - dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); - return ret; - } - - ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks); - if (ret) - goto err_disable_regulators; - - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN); - - return 0; - -err_disable_regulators: - regulator_bulk_disable(cfg->num_vregs, qmp->vregs); - - return ret; -} static int qmp_ufs_com_exit(struct qmp_ufs *qmp) { @@ -1799,10 +1774,27 @@ static int qmp_ufs_com_exit(struct qmp_ufs *qmp) static int qmp_ufs_power_on(struct phy *phy) { struct qmp_ufs *qmp = phy_get_drvdata(phy); + const struct qmp_phy_cfg *cfg = qmp->cfg; + void __iomem *pcs = qmp->pcs; int ret; + dev_vdbg(qmp->dev, "Initializing QMP phy\n"); - ret = qmp_ufs_com_init(qmp); + ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs); + if (ret) { + dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret); + return ret; + } + + ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks); + if (ret) + goto err_disable_regulators; + + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN); + return 0; + +err_disable_regulators: + regulator_bulk_disable(cfg->num_vregs, qmp->vregs); return ret; }
Simplify the qcom ufs phy driver by inlining qmp_ufs_com_init() into qmp_ufs_power_on(). This change removes unnecessary function calls and ensures that the initialization logic is directly within the power-on routine, maintaining the same functionality. Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 44 ++++++++++--------------- 1 file changed, 18 insertions(+), 26 deletions(-) -- 2.48.1