From: sbillaka@codeaurora.org To: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Andy Gross <agross@kernel.org>, Kishon Vijay Abraham I <kishon@ti.com>, Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, Rob Clark <robdclark@gmail.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Stephen Boyd <swboyd@chromium.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, kalyan_t@codeaurora.org, abhinavk@codeaurora.org Subject: Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Date: Thu, 12 Aug 2021 05:53:43 +0530 [thread overview] Message-ID: <04267264ac7904733552d6ca02a6797b@codeaurora.org> (raw) In-Reply-To: <YRH0WFvWmemHIHqc@builder.lan> On 2021-08-10 09:06, Bjorn Andersson wrote: > On Mon 09 Aug 22:15 CDT 2021, sbillaka@codeaurora.org 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
WARNING: multiple messages have this Message-ID (diff)
From: sbillaka@codeaurora.org To: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Andy Gross <agross@kernel.org>, Kishon Vijay Abraham I <kishon@ti.com>, Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, Rob Clark <robdclark@gmail.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Stephen Boyd <swboyd@chromium.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, kalyan_t@codeaurora.org, abhinavk@codeaurora.org Subject: Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Date: Thu, 12 Aug 2021 05:53:43 +0530 [thread overview] Message-ID: <04267264ac7904733552d6ca02a6797b@codeaurora.org> (raw) In-Reply-To: <YRH0WFvWmemHIHqc@builder.lan> On 2021-08-10 09:06, Bjorn Andersson wrote: > On Mon 09 Aug 22:15 CDT 2021, sbillaka@codeaurora.org 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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2021-08-12 0:24 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-11 4:19 [PATCH 1/2] dt-bindings: phy: Introduce Qualcomm eDP/DP PHY binding Bjorn Andersson 2021-05-11 4:19 ` Bjorn Andersson 2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson 2021-05-11 4:19 ` Bjorn Andersson 2021-05-11 5:22 ` Stephen Boyd 2021-05-11 5:22 ` Stephen Boyd 2021-05-14 10:38 ` Vinod Koul 2021-05-14 10:38 ` Vinod Koul 2021-05-11 14:24 ` Jeffrey Hugo 2021-05-11 14:24 ` Jeffrey Hugo 2021-05-11 14:46 ` Dmitry Baryshkov 2021-05-11 14:46 ` Dmitry Baryshkov 2021-05-14 10:40 ` Vinod Koul 2021-05-14 10:40 ` Vinod Koul 2021-08-10 3:15 ` sbillaka 2021-08-10 3:15 ` sbillaka 2021-08-10 3:36 ` Bjorn Andersson 2021-08-10 3:36 ` Bjorn Andersson 2021-08-12 0:23 ` sbillaka [this message] 2021-08-12 0:23 ` sbillaka 2021-05-11 17:24 ` [PATCH 1/2] dt-bindings: phy: Introduce Qualcomm eDP/DP PHY binding Stephen Boyd 2021-05-11 17:24 ` Stephen Boyd 2021-05-17 21:46 ` Rob Herring 2021-05-17 21:46 ` Rob Herring
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=04267264ac7904733552d6ca02a6797b@codeaurora.org \ --to=sbillaka@codeaurora.org \ --cc=abhinavk@codeaurora.org \ --cc=agross@kernel.org \ --cc=bjorn.andersson@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=dmitry.baryshkov@linaro.org \ --cc=kalyan_t@codeaurora.org \ --cc=kishon@ti.com \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=robdclark@gmail.com \ --cc=robh+dt@kernel.org \ --cc=swboyd@chromium.org \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.