Hi,


On 01/06/2017 12:48 PM, Bjorn Andersson wrote:

On Tue 20 Dec 09:03 PST 2016, Vivek Gautam wrote:

quoted

diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c

[..]

quoted

+static int qcom_qmp_phy_poweron(struct phy *phy)

[..]

quoted

+
+err3:

Rather than naming your labels errX, it's idiomatic to give them
descriptive names, e.g. "disable_ref_clk" here.

Sure will change these labels and any such occurrence further in the code.

quoted

+	clk_disable_unprepare(qphy->ref_clk);
+err2:
+	regulator_disable(qphy->vddp_ref_clk);
+err1:
+	regulator_disable(qphy->vdda_pll);
+err:
+	regulator_disable(qphy->vdda_phy);
+	return ret;
+}

[..]

quoted

+static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
+{
+	const struct qmp_phy_cfg *cfg = qphy->cfg;
+	void __iomem *serdes = qphy->serdes;
+	int ret;
+
+	mutex_lock(&qphy->phy_mutex);
+	if (qphy->init_count++) {
+		mutex_unlock(&qphy->phy_mutex);
+		return 0;
+	}

As far as I can see phy_init() and phy_exit() already keep reference
count on the initialization and you only call this function from
phy_ops->init, so you should be able to drop this.

This is an intermediary function that does the common block initialization.
PHYs like PCIe have a separate common block (apart from SerDes)
for all phy channels. We shouldn't program this common block
multiple times for each channel. That's why this init_count.

[..]

quoted

+
+/* PHY Initialization */
+static int qcom_qmp_phy_init(struct phy *phy)
+{

[..]

quoted

+	/* phy power down delay; given in PCIE phy programming guide only */
+	if (qphy->cfg->type == PHY_TYPE_PCIE)

Rather than basing this off "is this pcie" it's preferred if you have a
bool power_down_delay; (or optionally carrying the timeout value) to
control this.

Sure, will modify this.

quoted

+		usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
+
+	/* start SerDes and Phy-Coding-Sublayer */
+	qphy_setbits(pcs + QPHY_START_CTRL, cfg->start_ctrl);
+
+	/* Pull PHY out of reset state */
+	qphy_clrbits(pcs + QPHY_SW_RESET, SW_RESET);
+
+	status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
+	mask = cfg->mask_pcs_ready;
+
+	ret = !mask ? 0 : readl_poll_timeout(status, val, !(val & mask), 1,
+						PHY_INIT_COMPLETE_TIMEOUT);

I presume it's not okay to read the status register even once for pcie?
If it is you can skip the conditional as !(val & 0) will break the poll
after the first read.

Yea, it is okay to read the status register for pcie.

Will leave just readl_poll_timeout() that exits on !(val & mask).
We anyways don't set mask_pcs_ready for pcie.

[..]

quoted

+static int qcom_qmp_phy_exit(struct phy *phy)
+{

[..]

quoted

+}
+
+

Extra blank line

Will remove it.

quoted

+static int qcom_qmp_phy_regulator_init(struct device *dev)

[..]

quoted

+static
+struct qmp_phy_desc *qcom_qmp_phy_create(struct platform_device *pdev, int id)
+{

[..]

quoted

+	phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
+	if (IS_ERR(phy_desc->pipe_clk)) {
+		if (qphy->cfg->type == PHY_TYPE_PCIE ||
+		    qphy->cfg->type == PHY_TYPE_USB3) {

Is this check adding any value?

The USB3 and PCIe phys are PIPE based phys, and hence the pipe
clock is mandatory for them. Other phys don't care about pipe clock.

quoted

+			ret = PTR_ERR(phy_desc->pipe_clk);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev,
+					"failed to get lane%d pipe_clk, %d\n",
+					id, ret);
+			return ERR_PTR(ret);
+		}
+		phy_desc->pipe_clk = NULL;

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project