| Message ID | 1410550088-8754-3-git-send-email-agross@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Sep 13, 2014 at 01:44:25AM +0530, Pramod Gurav wrote: > Andy, > Couple of minor comments. > > On Sat, Sep 13, 2014 at 12:58 AM, Andy Gross <agross@codeaurora.org> wrote: > > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > DWC3 glue layer is hardware layer around Synopsys DesignWare > > USB3 core. Its purpose is to supply Synopsys IP with required > > clocks, voltages and interface it with the rest of the SoC. > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > Signed-off-by: Andy Gross <agross@codeaurora.org> > > --- > > drivers/usb/dwc3/Kconfig | 8 +++ > > drivers/usb/dwc3/Makefile | 1 + > > drivers/usb/dwc3/dwc3-qcom.c | 131 > > ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 140 insertions(+) > > create mode 100644 drivers/usb/dwc3/dwc3-qcom.c > > > > > <..> > > > > +#include <linux/platform_device.h> > > + > > +struct dwc3_qcom { > > + struct device *dev; > > + > > > Extra new line here. that's not an issue however. > > + struct clk *core_clk; > > + struct clk *iface_clk; > > + struct clk *sleep_clk; > > +}; > > + > > +static int dwc3_qcom_probe(struct platform_device *pdev) > > +{ > > + struct device_node *node = pdev->dev.of_node; > > + struct dwc3_qcom *qdwc; > > + int ret = 0; > > > Initialization not required. I'll fix this one as I'm already applying this patch. > > + > > + qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); > > + if (!qdwc) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, qdwc); > > + > > + qdwc->dev = &pdev->dev; > > + > > + qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); > > + if (IS_ERR(qdwc->core_clk)) { > > + dev_err(qdwc->dev, "failed to get core clock\n"); > > + return PTR_ERR(qdwc->core_clk); > > + } > > + > > + qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); > > + if (IS_ERR(qdwc->iface_clk)) { > > + dev_dbg(qdwc->dev, "failed to get optional iface clock\n"); > > + qdwc->iface_clk = NULL; > > + } > > + > > + qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); > > + if (IS_ERR(qdwc->sleep_clk)) { > > + dev_dbg(qdwc->dev, "failed to get optional sleep clock\n"); > > + qdwc->sleep_clk = NULL; > > + } > > + > > + ret = clk_prepare_enable(qdwc->core_clk); > > + if (ret) { > > + dev_err(qdwc->dev, "failed to enable core clock\n"); > > + goto err_core; > > + } > > + > > + ret = clk_prepare_enable(qdwc->iface_clk); > > > Should not we check if qdwc->iface_clk is valid? read the sources luke. > > +err_clks: > > + clk_disable_unprepare(qdwc->sleep_clk); > > > IS_ERR check before above statement not needed as we have continued with > probe even after failure og devm_clk_get? read more carefully, there's a detail which you're missing.
Hi Felipe, On 13-09-2014 01:50 AM, Felipe Balbi wrote: > On Sat, Sep 13, 2014 at 01:44:25AM +0530, Pramod Gurav wrote: >> Andy, >> Couple of minor comments. >> >> On Sat, Sep 13, 2014 at 12:58 AM, Andy Gross <agross@codeaurora.org> wrote: >> >>> From: "Ivan T. Ivanov" <iivanov@mm-sol.com> >>> >>> DWC3 glue layer is hardware layer around Synopsys DesignWare >>> USB3 core. Its purpose is to supply Synopsys IP with required >>> clocks, voltages and interface it with the rest of the SoC. >>> >>> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> >>> Signed-off-by: Andy Gross <agross@codeaurora.org> >>> --- >>> drivers/usb/dwc3/Kconfig | 8 +++ >>> drivers/usb/dwc3/Makefile | 1 + >>> drivers/usb/dwc3/dwc3-qcom.c | 131 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 140 insertions(+) >>> create mode 100644 drivers/usb/dwc3/dwc3-qcom.c >>> >>> >> <..> >> >> >>> +#include <linux/platform_device.h> >>> + >>> +struct dwc3_qcom { >>> + struct device *dev; >>> + >>> >> Extra new line here. > > that's not an issue however. > >>> + struct clk *core_clk; >>> + struct clk *iface_clk; >>> + struct clk *sleep_clk; >>> +}; >>> + >>> +static int dwc3_qcom_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *node = pdev->dev.of_node; >>> + struct dwc3_qcom *qdwc; >>> + int ret = 0; >>> >> Initialization not required. > > I'll fix this one as I'm already applying this patch. > >>> + >>> + qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); >>> + if (!qdwc) >>> + return -ENOMEM; >>> + >>> + platform_set_drvdata(pdev, qdwc); >>> + >>> + qdwc->dev = &pdev->dev; >>> + >>> + qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); >>> + if (IS_ERR(qdwc->core_clk)) { >>> + dev_err(qdwc->dev, "failed to get core clock\n"); >>> + return PTR_ERR(qdwc->core_clk); >>> + } >>> + >>> + qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); >>> + if (IS_ERR(qdwc->iface_clk)) { >>> + dev_dbg(qdwc->dev, "failed to get optional iface clock\n"); >>> + qdwc->iface_clk = NULL; >>> + } >>> + >>> + qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); >>> + if (IS_ERR(qdwc->sleep_clk)) { >>> + dev_dbg(qdwc->dev, "failed to get optional sleep clock\n"); >>> + qdwc->sleep_clk = NULL; >>> + } >>> + >>> + ret = clk_prepare_enable(qdwc->core_clk); >>> + if (ret) { >>> + dev_err(qdwc->dev, "failed to enable core clock\n"); >>> + goto err_core; >>> + } >>> + >>> + ret = clk_prepare_enable(qdwc->iface_clk); >>> >> Should not we check if qdwc->iface_clk is valid? > > read the sources luke. Now I read that its initialized to NULL in fail case but should we call prepare_enable at all if its NULL? > >>> +err_clks: >>> + clk_disable_unprepare(qdwc->sleep_clk); >>> >> IS_ERR check before above statement not needed as we have continued with >> probe even after failure og devm_clk_get? > > read more carefully, there's a detail which you're missing. >
Hi, On Sat, Sep 13, 2014 at 01:55:50AM +0530, Pramod Gurav wrote: > >>> + qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); > >>> + if (!qdwc) > >>> + return -ENOMEM; > >>> + > >>> + platform_set_drvdata(pdev, qdwc); > >>> + > >>> + qdwc->dev = &pdev->dev; > >>> + > >>> + qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); > >>> + if (IS_ERR(qdwc->core_clk)) { > >>> + dev_err(qdwc->dev, "failed to get core clock\n"); > >>> + return PTR_ERR(qdwc->core_clk); > >>> + } > >>> + > >>> + qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); > >>> + if (IS_ERR(qdwc->iface_clk)) { > >>> + dev_dbg(qdwc->dev, "failed to get optional iface clock\n"); > >>> + qdwc->iface_clk = NULL; > >>> + } > >>> + > >>> + qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); > >>> + if (IS_ERR(qdwc->sleep_clk)) { > >>> + dev_dbg(qdwc->dev, "failed to get optional sleep clock\n"); > >>> + qdwc->sleep_clk = NULL; > >>> + } > >>> + > >>> + ret = clk_prepare_enable(qdwc->core_clk); > >>> + if (ret) { > >>> + dev_err(qdwc->dev, "failed to enable core clock\n"); > >>> + goto err_core; > >>> + } > >>> + > >>> + ret = clk_prepare_enable(qdwc->iface_clk); > >>> > >> Should not we check if qdwc->iface_clk is valid? > > > > read the sources luke. > Now I read that its initialized to NULL in fail case but should we call > prepare_enable at all if its NULL? now read the source of clk_enable() and clk_prepare() ;-) NULL is a valid clock, it just returns 0. This is better than sprinkling IS_ERR() all over the place.
On 13-09-2014 01:59 AM, Felipe Balbi wrote: > Hi, > > On Sat, Sep 13, 2014 at 01:55:50AM +0530, Pramod Gurav wrote: >>>>> + qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); >>>>> + if (!qdwc) >>>>> + return -ENOMEM; >>>>> + >>>>> + platform_set_drvdata(pdev, qdwc); >>>>> + >>>>> + qdwc->dev = &pdev->dev; >>>>> + >>>>> + qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); >>>>> + if (IS_ERR(qdwc->core_clk)) { >>>>> + dev_err(qdwc->dev, "failed to get core clock\n"); >>>>> + return PTR_ERR(qdwc->core_clk); >>>>> + } >>>>> + >>>>> + qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); >>>>> + if (IS_ERR(qdwc->iface_clk)) { >>>>> + dev_dbg(qdwc->dev, "failed to get optional iface clock\n"); >>>>> + qdwc->iface_clk = NULL; >>>>> + } >>>>> + >>>>> + qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); >>>>> + if (IS_ERR(qdwc->sleep_clk)) { >>>>> + dev_dbg(qdwc->dev, "failed to get optional sleep clock\n"); >>>>> + qdwc->sleep_clk = NULL; >>>>> + } >>>>> + >>>>> + ret = clk_prepare_enable(qdwc->core_clk); >>>>> + if (ret) { >>>>> + dev_err(qdwc->dev, "failed to enable core clock\n"); >>>>> + goto err_core; >>>>> + } >>>>> + >>>>> + ret = clk_prepare_enable(qdwc->iface_clk); >>>>> >>>> Should not we check if qdwc->iface_clk is valid? >>> >>> read the sources luke. >> Now I read that its initialized to NULL in fail case but should we call >> prepare_enable at all if its NULL? > > now read the source of clk_enable() and clk_prepare() ;-) NULL is a > valid clock, it just returns 0. This is better than sprinkling IS_ERR() > all over the place. Seen that. I was wrong about IS_ERR. Thanks. :) >
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 785510a..a816fe4 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -80,6 +80,14 @@ config USB_DWC3_KEYSTONE Support of USB2/3 functionality in TI Keystone2 platforms. Say 'Y' or 'M' here if you have one such device +config USB_DWC3_QCOM + tristate "Qualcomm Platforms" + depends on ARCH_QCOM || COMPILE_TEST + default USB_DWC3 + help + Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside, + say 'Y' or 'M' if you have one such device. + comment "Debugging features" config USB_DWC3_DEBUG diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index 10ac3e7..0da8e75 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -33,3 +33,4 @@ obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o obj-$(CONFIG_USB_DWC3_EXYNOS) += dwc3-exynos.o obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o obj-$(CONFIG_USB_DWC3_KEYSTONE) += dwc3-keystone.o +obj-$(CONFIG_USB_DWC3_QCOM) += dwc3-qcom.o diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c new file mode 100644 index 0000000..f255335 --- /dev/null +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -0,0 +1,131 @@ +/* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +struct dwc3_qcom { + struct device *dev; + + struct clk *core_clk; + struct clk *iface_clk; + struct clk *sleep_clk; +}; + +static int dwc3_qcom_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + struct dwc3_qcom *qdwc; + int ret = 0; + + qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); + if (!qdwc) + return -ENOMEM; + + platform_set_drvdata(pdev, qdwc); + + qdwc->dev = &pdev->dev; + + qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); + if (IS_ERR(qdwc->core_clk)) { + dev_err(qdwc->dev, "failed to get core clock\n"); + return PTR_ERR(qdwc->core_clk); + } + + qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); + if (IS_ERR(qdwc->iface_clk)) { + dev_dbg(qdwc->dev, "failed to get optional iface clock\n"); + qdwc->iface_clk = NULL; + } + + qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); + if (IS_ERR(qdwc->sleep_clk)) { + dev_dbg(qdwc->dev, "failed to get optional sleep clock\n"); + qdwc->sleep_clk = NULL; + } + + ret = clk_prepare_enable(qdwc->core_clk); + if (ret) { + dev_err(qdwc->dev, "failed to enable core clock\n"); + goto err_core; + } + + ret = clk_prepare_enable(qdwc->iface_clk); + if (ret) { + dev_err(qdwc->dev, "failed to enable optional iface clock\n"); + goto err_iface; + } + + ret = clk_prepare_enable(qdwc->sleep_clk); + if (ret) { + dev_err(qdwc->dev, "failed to enable optional sleep clock\n"); + goto err_sleep; + } + + ret = of_platform_populate(node, NULL, NULL, qdwc->dev); + if (ret) { + dev_err(qdwc->dev, "failed to register core - %d\n", ret); + goto err_clks; + } + + return 0; + +err_clks: + clk_disable_unprepare(qdwc->sleep_clk); +err_sleep: + clk_disable_unprepare(qdwc->iface_clk); +err_iface: + clk_disable_unprepare(qdwc->core_clk); +err_core: + return ret; +} + +static int dwc3_qcom_remove(struct platform_device *pdev) +{ + struct dwc3_qcom *qdwc = platform_get_drvdata(pdev); + + of_platform_depopulate(&pdev->dev); + + clk_disable_unprepare(qdwc->sleep_clk); + clk_disable_unprepare(qdwc->iface_clk); + clk_disable_unprepare(qdwc->core_clk); + + return 0; +} + +static const struct of_device_id of_dwc3_match[] = { + { .compatible = "qcom,dwc3" }, + { /* Sentinel */ } +}; +MODULE_DEVICE_TABLE(of, of_dwc3_match); + +static struct platform_driver dwc3_qcom_driver = { + .probe = dwc3_qcom_probe, + .remove = dwc3_qcom_remove, + .driver = { + .name = "qcom-dwc3", + .owner = THIS_MODULE, + .of_match_table = of_dwc3_match, + }, +}; + +module_platform_driver(dwc3_qcom_driver); + +MODULE_ALIAS("platform:qcom-dwc3"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("DesignWare USB3 QCOM Glue Layer"); +MODULE_AUTHOR("Ivan T. Ivanov <iivanov@mm-sol.com>");