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?
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
