On 2021-08-10 09:06, Bjorn Andersson wrote:

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

quoted

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

quoted

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

[..]

quoted

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

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

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?

We have only one clock (edp refclk) and one regulator (phy-0p9) for 
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.

Okay. Thank you.

Regards,
Bjorn

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