| Message ID | 1478218237-1737-3-git-send-email-spjoshi@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 11/03, Sarangdhar Joshi wrote: > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index d79fecd..844e90d 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); > static int qcom_scm_probe(struct platform_device *pdev) > { > struct qcom_scm *scm; > + uint32_t clks; If this was unsigned long flags; > int ret; > > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); > if (!scm) > return -ENOMEM; > > - scm->core_clk = devm_clk_get(&pdev->dev, "core"); > - if (IS_ERR(scm->core_clk)) { > - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) > - return PTR_ERR(scm->core_clk); > + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev)); then this could just be a cast to unsigned long? > + if (clks & SCM_HAS_CORE_CLK) { > + scm->core_clk = devm_clk_get(&pdev->dev, "core"); > + if (IS_ERR(scm->core_clk)) { > + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) > + return PTR_ERR(scm->core_clk); > > - scm->core_clk = NULL; > + scm->core_clk = NULL; > + } > }
On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote: > On 11/03, Sarangdhar Joshi wrote: > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > index d79fecd..844e90d 100644 > > --- a/drivers/firmware/qcom_scm.c > > +++ b/drivers/firmware/qcom_scm.c > > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); > > static int qcom_scm_probe(struct platform_device *pdev) > > { > > struct qcom_scm *scm; > > + uint32_t clks; > > If this was unsigned long flags; > I did look at this too and could only find a mixture of ways people have done this. Isn't the correct type for this intptr_t? Regards, Bjorn > > int ret; > > > > scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); > > if (!scm) > > return -ENOMEM; > > > > - scm->core_clk = devm_clk_get(&pdev->dev, "core"); > > - if (IS_ERR(scm->core_clk)) { > > - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) > > - return PTR_ERR(scm->core_clk); > > + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev)); > > then this could just be a cast to unsigned long? > > > + if (clks & SCM_HAS_CORE_CLK) { > > + scm->core_clk = devm_clk_get(&pdev->dev, "core"); > > + if (IS_ERR(scm->core_clk)) { > > + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) > > + return PTR_ERR(scm->core_clk); > > > > - scm->core_clk = NULL; > > + scm->core_clk = NULL; > > + } > > } > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09, Bjorn Andersson wrote: > On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote: > > > On 11/03, Sarangdhar Joshi wrote: > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > > index d79fecd..844e90d 100644 > > > --- a/drivers/firmware/qcom_scm.c > > > +++ b/drivers/firmware/qcom_scm.c > > > @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); > > > static int qcom_scm_probe(struct platform_device *pdev) > > > { > > > struct qcom_scm *scm; > > > + uint32_t clks; > > > > If this was unsigned long flags; > > > > I did look at this too and could only find a mixture of ways people have > done this. Isn't the correct type for this intptr_t? > Well unsigned long == sizeof(kernel pointer) on all Linux platforms so it's safe to do the cast. For example, struct timer_list stores data in unsigned long. I guess this is a 'kernelism' though? Perhaps we should update Documentation/CodingStyle if anyone cares.
On 11/09/2016 05:47 PM, Stephen Boyd wrote: > On 11/03, Sarangdhar Joshi wrote: >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index d79fecd..844e90d 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); >> static int qcom_scm_probe(struct platform_device *pdev) >> { >> struct qcom_scm *scm; >> + uint32_t clks; > > If this was unsigned long flags; > >> int ret; >> >> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); >> if (!scm) >> return -ENOMEM; >> >> - scm->core_clk = devm_clk_get(&pdev->dev, "core"); >> - if (IS_ERR(scm->core_clk)) { >> - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) >> - return PTR_ERR(scm->core_clk); >> + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev)); > > then this could just be a cast to unsigned long? I saw quite a few places in kernel where similar type casts are used (e.g gpio drivers) and ended up using uint32_t. uintptr_t is nothing but typedef of an unsigned long. Probably it's a good idea to just use unsigned long instead. > >> + if (clks & SCM_HAS_CORE_CLK) { >> + scm->core_clk = devm_clk_get(&pdev->dev, "core"); >> + if (IS_ERR(scm->core_clk)) { >> + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) >> + return PTR_ERR(scm->core_clk); >> >> - scm->core_clk = NULL; >> + scm->core_clk = NULL; >> + } >> } >
On 11/09/2016 09:55 PM, Bjorn Andersson wrote: > On Wed 09 Nov 17:47 PST 2016, Stephen Boyd wrote: > >> On 11/03, Sarangdhar Joshi wrote: >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index d79fecd..844e90d 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); >>> static int qcom_scm_probe(struct platform_device *pdev) >>> { >>> struct qcom_scm *scm; >>> + uint32_t clks; >> >> If this was unsigned long flags; >> > > I did look at this too and could only find a mixture of ways people have > done this. Isn't the correct type for this intptr_t? That's true. There are lot of variations of how it's done. I had referred one of the gpio driver for this. I think it's safe to use unsigned long instead as Stephen suggested. Btw I don't see intptr_t defined in include/linux/types.h. Regards, Sarang > > Regards, > Bjorn > >>> int ret; >>> >>> scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); >>> if (!scm) >>> return -ENOMEM; >>> >>> - scm->core_clk = devm_clk_get(&pdev->dev, "core"); >>> - if (IS_ERR(scm->core_clk)) { >>> - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) >>> - return PTR_ERR(scm->core_clk); >>> + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev)); >> >> then this could just be a cast to unsigned long? >> >>> + if (clks & SCM_HAS_CORE_CLK) { >>> + scm->core_clk = devm_clk_get(&pdev->dev, "core"); >>> + if (IS_ERR(scm->core_clk)) { >>> + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) >>> + return PTR_ERR(scm->core_clk); >>> >>> - scm->core_clk = NULL; >>> + scm->core_clk = NULL; >>> + } >>> } >> >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> a Linux Foundation Collaborative Project
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index d79fecd..844e90d 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -28,6 +28,10 @@ #include "qcom_scm.h" +#define SCM_HAS_CORE_CLK BIT(0) +#define SCM_HAS_IFACE_CLK BIT(1) +#define SCM_HAS_BUS_CLK BIT(2) + struct qcom_scm { struct device *dev; struct clk *core_clk; @@ -380,32 +384,40 @@ EXPORT_SYMBOL(qcom_scm_is_available); static int qcom_scm_probe(struct platform_device *pdev) { struct qcom_scm *scm; + uint32_t clks; int ret; scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL); if (!scm) return -ENOMEM; - scm->core_clk = devm_clk_get(&pdev->dev, "core"); - if (IS_ERR(scm->core_clk)) { - if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) - return PTR_ERR(scm->core_clk); + clks = (uint32_t)((uintptr_t)of_device_get_match_data(&pdev->dev)); + if (clks & SCM_HAS_CORE_CLK) { + scm->core_clk = devm_clk_get(&pdev->dev, "core"); + if (IS_ERR(scm->core_clk)) { + if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER) + return PTR_ERR(scm->core_clk); - scm->core_clk = NULL; + scm->core_clk = NULL; + } } - if (of_device_is_compatible(pdev->dev.of_node, "qcom,scm")) { + if (clks & SCM_HAS_IFACE_CLK) { scm->iface_clk = devm_clk_get(&pdev->dev, "iface"); if (IS_ERR(scm->iface_clk)) { if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER) - dev_err(&pdev->dev, "failed to acquire iface clk\n"); + dev_err(&pdev->dev, + "failed to acquire iface clk\n"); return PTR_ERR(scm->iface_clk); } + } + if (clks & SCM_HAS_BUS_CLK) { scm->bus_clk = devm_clk_get(&pdev->dev, "bus"); if (IS_ERR(scm->bus_clk)) { if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER) - dev_err(&pdev->dev, "failed to acquire bus clk\n"); + dev_err(&pdev->dev, + "failed to acquire bus clk\n"); return PTR_ERR(scm->bus_clk); } } @@ -429,10 +441,23 @@ static int qcom_scm_probe(struct platform_device *pdev) } static const struct of_device_id qcom_scm_dt_match[] = { - { .compatible = "qcom,scm-apq8064",}, - { .compatible = "qcom,scm-msm8660",}, - { .compatible = "qcom,scm-msm8960",}, - { .compatible = "qcom,scm",}, + { .compatible = "qcom,scm-apq8064", + .data = (void *) SCM_HAS_CORE_CLK, + }, + { .compatible = "qcom,scm-msm8660", + .data = (void *) SCM_HAS_CORE_CLK, + }, + { .compatible = "qcom,scm-msm8960", + .data = (void *) SCM_HAS_CORE_CLK, + }, + { .compatible = "qcom,scm-msm8996", + .data = NULL, /* no clocks */ + }, + { .compatible = "qcom,scm", + .data = (void *)(SCM_HAS_CORE_CLK + | SCM_HAS_IFACE_CLK + | SCM_HAS_BUS_CLK), + }, {} };
Core, iface and bus clocks are not required to be voted from SCM driver for some of the Qualcomm chipsets. Remove dependency on these clocks from driver. Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Sarangdhar Joshi <spjoshi@codeaurora.org> --- drivers/firmware/qcom_scm.c | 49 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 12 deletions(-)