Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
From: sbillaka
Date: Wed Aug 11 2021 - 20:24:02 EST
On 2021-08-10 09:06, Bjorn Andersson wrote:
On Mon 09 Aug 22:15 CDT 2021, sbillaka@xxxxxxxxxxxxxx wrote:
On 2021-05-11 09:49, Bjorn Andersson wrote:
> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
[..]
> +#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.
[..]
> +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.
[..]
> +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