On Mon 09 Aug 22:15 CDT 2021, sbillaka@codeaurora.org wrote:

On 2021-05-11 09:49, Bjorn Andersson wrote:

quoted

diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c

[..]

quoted

+#define DP_PHY_AUX_CFG0				0x0020
+#define DP_PHY_AUX_CFG1				0x0024
+#define DP_PHY_AUX_CFG2				0x0028
+#define DP_PHY_AUX_CFG3				0x002c
+#define DP_PHY_AUX_CFG4				0x0030
+#define DP_PHY_AUX_CFG5				0x0034
+#define DP_PHY_AUX_CFG6				0x0038
+#define DP_PHY_AUX_CFG7				0x003c
+#define DP_PHY_AUX_CFG8				0x0040
+#define DP_PHY_AUX_CFG9				0x0044

The DP_PHY_AUX_CFG0 offset for sc8180x eDP phy is 0x0024.
Some of the eDP PHY offset addresses are shifted by 4 address locations,
compared to the DP QMP PHY offset addresses for sc8180x.
The DP_PHY_AUX_CFG* offsets for this eDP phy driver are as below:

#define DP_PHY_AUX_CFG0                         0x0024
#define DP_PHY_AUX_CFG1                         0x0028
#define DP_PHY_AUX_CFG2                         0x002c
#define DP_PHY_AUX_CFG3                         0x0030
#define DP_PHY_AUX_CFG4                         0x0034
#define DP_PHY_AUX_CFG5                         0x0038
#define DP_PHY_AUX_CFG6                         0x003c
#define DP_PHY_AUX_CFG7                         0x0040
#define DP_PHY_AUX_CFG8                         0x0044
#define DP_PHY_AUX_CFG9                         0x0048

I noticed this as well. During development I just used the numbers
directly in the code and I must have screwed up as I replaced them with
defined - and somehow missed this in the testing before posting.

Sorry about that.

[..]

quoted

+static int qcom_edp_phy_init(struct phy *phy)
+{
+	struct qcom_edp *edp = phy_get_drvdata(phy);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies);
+	if (ret)
+		return ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks);
+	if (ret)
+		goto out_disable_supplies;

I think the number of clk and regulator resources can vary based on
platform.

If that's the case we should replace the ARRAY_SIZE() with an integer.
But I prefer to wait with that until the number actually is variable.

[..]

quoted

+static int qcom_edp_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct qcom_edp *edp;
+	int ret;
+
+	edp = devm_kzalloc(dev, sizeof(*edp), GFP_KERNEL);
+	if (!edp)
+		return -ENOMEM;
+
+	edp->dev = dev;
+
+	edp->edp = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(edp->edp))
+		return PTR_ERR(edp->edp);
+
+	edp->tx0 = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(edp->tx0))
+		return PTR_ERR(edp->tx0);
+
+	edp->tx1 = devm_platform_ioremap_resource(pdev, 2);
+	if (IS_ERR(edp->tx1))
+		return PTR_ERR(edp->tx1);
+
+	edp->pll = devm_platform_ioremap_resource(pdev, 3);
+	if (IS_ERR(edp->pll))
+		return PTR_ERR(edp->pll);
+
+	edp->clks[0].id = "aux";
+	edp->clks[1].id = "cfg_ahb";
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(edp->clks), edp->clks);
+	if (ret)
+		return ret;
+
+	edp->supplies[0].supply = "vdda-phy";
+	edp->supplies[1].supply = "vdda-pll";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies),
edp->supplies);
+	if (ret)
+		return ret;

I believe, the combination of the number of regulator and clk resources may
vary based on the platform.
I think we should not fail probe if all these resources are not present in
the device tree file.
I think, these resources can be optional. We can get these resources if they
are present in the device tree file and enable them as required.

It's quite helpful to the DTS writer to actually encode in the driver
which resources the driver expects and provide useful error messages
when these expectations aren't met - so I think in line with most other
drivers this should be decided based on the compatible.

What clocks and regulators do you have on sc7280?


Thanks for the feedback, I see that I have a few more pieces of feedback
from others that I need to incorporate. I'll make sure to do that and
repost this patch shortly.

Regards,
Bjorn

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy