Re: [PATCH v2 2/4] phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs

Hi Philipp,

On 3/24/2020 2:49 AM, Philipp Zabel wrote:

Hi Wesley,

On Mon, Mar 23, 2020 at 01:17:10PM -0700, Wesley Cheng wrote:

quoted

This adds the SNPS FemtoPHY driver used in QCOM SOCs.  There
are potentially multiple instances of this UTMI PHY on the
SOC, all which can utilize this driver.

Signed-off-by: Wesley Cheng <redacted>
---
 drivers/phy/qualcomm/Kconfig             |  10 ++
 drivers/phy/qualcomm/Makefile            |   1 +
 drivers/phy/qualcomm/phy-qcom-snps-7nm.c | 294 +++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-snps-7nm.c

[...]

quoted

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-7nm.c b/drivers/phy/qualcomm/phy-qcom-snps-7nm.c
new file mode 100644
index 0000000..8d4ba53
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-snps-7nm.c

@@ -0,0 +1,294 @@

[...]

quoted

+static int qcom_snps_hsphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_snps_hsphy *hsphy;
+	struct phy_provider *phy_provider;
+	struct phy *generic_phy;
+	struct resource *res;
+	int ret, i;
+	int num;
+
+	hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL);
+	if (!hsphy)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hsphy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(hsphy->base))
+		return PTR_ERR(hsphy->base);
+
+	hsphy->ref_clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(hsphy->ref_clk)) {
+		ret = PTR_ERR(hsphy->ref_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get ref clk, %d\n", ret);
+		return ret;
+	}
+
+	hsphy->phy_reset = devm_reset_control_get_by_index(&pdev->dev, 0);
+	if (IS_ERR(hsphy->phy_reset)) {
+		dev_err(dev, "failed to get phy core reset\n");
+		return PTR_ERR(hsphy->phy_reset);
+	}

There is only a single reset specified, so there is no need for
_by_index. Also please explicitly request exclusive reset control
for this driver, I suggest:

	hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);

If you do want to prepare for future addition of other resets to the
bindings (but if so, why not specify those right now?), you should add
a reset-names property and request the reset control by id string
instead.

regards
Philipp

Thanks for the feedback.  Sure, I will move to using
devm_reset_control_get_exclusive, as we won't be having multiple resets
for this particular PHY.  Will update a new patch series.

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