On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote: > As explained in the similar ufs-exynos.c change, PHY consumer drivers > should not look at the phy->power_count, because in the general case > there might also be other consumers who have called phy_power_on() too, > so the fact that the power_count is non-zero does not mean that we did. > > Moreover, struct phy will become opaque soon, so the qcom UFS driver > will not be able to apply this pattern. Keep parallel track of the PHY > power state, instead of looking at a field which will become unavailable > (phy->power_count). > > About treating the phy_power_off() return code: from an API perspective, > this should have probably returned void, otherwise consumers would be > stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c, > does return 0 in its power_off() implementation. I consider it safe to > discard potential errors from phy_power_off() instead of complicating > the phy_powered_on logic. >
You could even simplify the code by getting rid of the 'phy_powered_on' check altogether. There is no real need to track the PHY power state in this driver. It is safe to call phy_power_off() without any checks. - Mani > Signed-off-by: Vladimir Oltean <[email protected]> > --- > Cc: "James E.J. Bottomley" <[email protected]> > Cc: Manivannan Sadhasivam <[email protected]> > Cc: "Martin K. Petersen" <[email protected]> > Cc: Nitin Rawat <[email protected]> > > v4->v5: patch is new > --- > drivers/ufs/host/ufs-qcom.c | 9 +++++++-- > drivers/ufs/host/ufs-qcom.h | 1 + > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 375fd24ba458..3b8bd9968235 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -508,9 +508,10 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba > *hba) > if (ret) > return ret; > > - if (phy->power_count) > + if (host->phy_powered_on) { > phy_power_off(phy); > - > + host->phy_powered_on = false; > + } > > /* phy initialization - calibrate the phy */ > ret = phy_init(phy); > @@ -531,6 +532,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > __func__, ret); > goto out_disable_phy; > } > + host->phy_powered_on = true; > > ret = phy_calibrate(phy); > if (ret) { > @@ -1268,6 +1270,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, > bool on, > dev_err(hba->dev, "phy power off failed, > ret=%d\n", err); > return err; > } > + host->phy_powered_on = false; > } > break; > case POST_CHANGE: > @@ -1277,6 +1280,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, > bool on, > dev_err(hba->dev, "phy power on failed, ret = > %d\n", err); > return err; > } > + host->phy_powered_on = true; > > /* enable the device ref clock for HS mode*/ > if (ufshcd_is_hs_mode(&hba->pwr_info)) > @@ -1467,6 +1471,7 @@ static void ufs_qcom_exit(struct ufs_hba *hba) > > ufs_qcom_disable_lane_clks(host); > phy_power_off(host->generic_phy); > + host->phy_powered_on = false; > phy_exit(host->generic_phy); > } > > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 1111ab34da01..72ce0687fa42 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -282,6 +282,7 @@ struct ufs_qcom_host { > struct clk_bulk_data *clks; > u32 num_clks; > bool is_lane_clks_enabled; > + bool phy_powered_on; > > struct icc_path *icc_ddr; > struct icc_path *icc_cpu; > -- > 2.43.0 > -- மணிவண்ணன் சதாசிவம்
