| Message ID | 1403606121-6368-8-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
On Tuesday, June 24, 2014 7:35 PM, Antoine Tenart wrote: > > Add a generic ChipIdea driver, with optional PHY and clock, to support > ChipIdea controllers that doesn't need specific functions. s/doesn't/don't > > Needed for the Marvell Berlin SoCs SUB controllers. s/SUB/USB > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/usb/chipidea/Makefile | 1 + > drivers/usb/chipidea/ci_hdrc_generic.c | 108 +++++++++++++++++++++++++++++++++ > 2 files changed, 109 insertions(+) > create mode 100644 drivers/usb/chipidea/ci_hdrc_generic.c > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 2f099c7df7b5..c705f0fe7a00 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -20,4 +20,5 @@ endif > > ifneq ($(CONFIG_OF),) > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > endif > diff --git a/drivers/usb/chipidea/ci_hdrc_generic.c b/drivers/usb/chipidea/ci_hdrc_generic.c > new file mode 100644 > index 000000000000..27520710a1f7 > --- /dev/null > +++ b/drivers/usb/chipidea/ci_hdrc_generic.c [...] > + > +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > +MODULE_LICENSE("GPL"); How about "GPL v2"? Best regards, Jingoo Han > -- > 1.9.1
On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > Add a generic ChipIdea driver, with optional PHY and clock, to support > ChipIdea controllers that doesn't need specific functions. > > Needed for the Marvell Berlin SoCs SUB controllers. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/usb/chipidea/Makefile | 1 + > drivers/usb/chipidea/ci_hdrc_generic.c | 108 +++++++++++++++++++++++++++++++++ > 2 files changed, 109 insertions(+) > create mode 100644 drivers/usb/chipidea/ci_hdrc_generic.c > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 2f099c7df7b5..c705f0fe7a00 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -20,4 +20,5 @@ endif > > ifneq ($(CONFIG_OF),) > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > endif As a generic driver, you may need to support both dt and non-dt solution. > diff --git a/drivers/usb/chipidea/ci_hdrc_generic.c b/drivers/usb/chipidea/ci_hdrc_generic.c > new file mode 100644 > index 000000000000..27520710a1f7 > --- /dev/null > +++ b/drivers/usb/chipidea/ci_hdrc_generic.c > @@ -0,0 +1,108 @@ > +/* > + * Copyright (C) 2014 Marvell Technology Group Ltd. > + * > + * Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/usb/chipidea.h> > +#include <linux/usb/hcd.h> > +#include <linux/usb/ulpi.h> > + > +#include "ci.h" > + > +struct ci_hdrc_generic_priv { > + struct platform_device *ci_pdev; > + struct clk *clk; > +}; > + > +static int ci_hdrc_generic_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct ci_hdrc_generic_priv *priv; > + struct ci_hdrc_platform_data ci_pdata = { > + .name = "ci_hdrc", How about this using dev_name(&pdev->dev) for name? > + .capoffset = DEF_CAPOFFSET, > + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | > + CI_HDRC_DISABLE_STREAMING, > + }; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (!IS_ERR(priv->clk)) { > + ret = clk_prepare_enable(priv->clk); > + if (ret) { > + dev_err(dev, "failed to enable the clock: %d\n", ret); > + return ret; > + } > + } > + > + ci_pdata.phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > + if (IS_ERR(ci_pdata.phy)) > + /* PHY is optional */ > + ci_pdata.phy = NULL; > + > + priv->ci_pdev = ci_hdrc_add_device(dev, pdev->resource, > + pdev->num_resources, &ci_pdata); > + if (IS_ERR(priv->ci_pdev)) { > + ret = PTR_ERR(priv->ci_pdev); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, > + "failed to register ci_hdrc platform device: %d\n", > + ret); > + goto clk_err; > + } > + > + platform_set_drvdata(pdev, priv); > + > + pm_runtime_no_callbacks(dev); > + pm_runtime_enable(dev); > + > + return 0; > + > +clk_err: > + clk_disable_unprepare(priv->clk); You may need to add "if (!IS_ERR(priv->clk))" > + return ret; > +} > + > +static int ci_hdrc_generic_remove(struct platform_device *pdev) > +{ > + struct ci_hdrc_generic_priv *priv = platform_get_drvdata(pdev); > + > + pm_runtime_disable(&pdev->dev); > + ci_hdrc_remove_device(priv->ci_pdev); > + clk_disable_unprepare(priv->clk); > + > + return 0; > +} > + > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > + { .compatible = "chipidea-usb" }, > + { } > +}; Even as a generic driver, you can also use your own compatible string. > +MODULE_DEVICE_TABLE(of, ci_hdrc_generic_of_match); > + > +static struct platform_driver ci_hdrc_generic_driver = { > + .probe = ci_hdrc_generic_probe, > + .remove = ci_hdrc_generic_remove, > + .driver = { > + .name = "chipidea-usb", > + .owner = THIS_MODULE, > + .of_match_table = ci_hdrc_generic_of_match, > + }, > +}; > +module_platform_driver(ci_hdrc_generic_driver); > + > +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > +MODULE_LICENSE("GPL"); > -- > 1.9.1 >
On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: > On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > > Add a generic ChipIdea driver, with optional PHY and clock, to support > > ChipIdea controllers that doesn't need specific functions. > > > > + return 0; > > +} > > + > > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > > + { .compatible = "chipidea-usb" }, > > + { } > > +}; > > Even as a generic driver, you can also use your own compatible string. > > > +MODULE_DEVICE_TABLE(of, ci_hdrc_generic_of_match); > > + > > +static struct platform_driver ci_hdrc_generic_driver = { > > + .probe = ci_hdrc_generic_probe, > > + .remove = ci_hdrc_generic_remove, > > + .driver = { > > + .name = "chipidea-usb", > > + .owner = THIS_MODULE, > > + .of_match_table = ci_hdrc_generic_of_match, > > + }, > > +}; > > +module_platform_driver(ci_hdrc_generic_driver); > > + > > +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); > > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > > +MODULE_LICENSE("GPL"); > > -- > > 1.9.1 > > > > -- > Besides, I haven't seen dma_coerce_mask_and_coherent API calling, where you set your dma mask?
Peter, On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: > On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > > > > ifneq ($(CONFIG_OF),) > > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > > endif > > As a generic driver, you may need to support both dt and non-dt > solution. Since the dt is now the best practice and since there is no need (yet) for a non-dt usage of this driver shouldn't we let anyone needing it implement it when the time comes? > > +static int ci_hdrc_generic_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct ci_hdrc_generic_priv *priv; > > + struct ci_hdrc_platform_data ci_pdata = { > > + .name = "ci_hdrc", > > How about this using dev_name(&pdev->dev) for name? Yes, why not. I don't have a strong preference. > > + > > +clk_err: > > + clk_disable_unprepare(priv->clk); > > You may need to add "if (!IS_ERR(priv->clk))" Right! I'll update this. > > + > > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > > + { .compatible = "chipidea-usb" }, > > + { } > > +}; > > Even as a generic driver, you can also use your own compatible string. Well, there is nothing specific about the Berlin CI. Some subsystems use the 'generic' keyword in these cases. Do you see a particular reason I should use some Berlin related compatible here? Thanks for the review! Antoine
Hello, On Tue, Jun 24, 2014 at 07:51:01PM +0900, Jingoo Han wrote: > On Tuesday, June 24, 2014 7:35 PM, Antoine Tenart wrote: > > > > Add a generic ChipIdea driver, with optional PHY and clock, to support > > ChipIdea controllers that doesn't need specific functions. > > s/doesn't/don't > > > > > Needed for the Marvell Berlin SoCs SUB controllers. > > s/SUB/USB Right, I'll fix these. > > > + > > +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); > > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > > +MODULE_LICENSE("GPL"); > > How about "GPL v2"? Well, "GPL" stands for "GNU Public License v2 or later" as documented in: http://lxr.free-electrons.com/source/include/linux/module.h#L100 Is there a reason I should use "GPLv2"? Or is this a best practice? Thanks! Antoine
On Mon, Jun 30, 2014 at 03:33:13PM +0200, Antoine Ténart wrote: > Peter, > > On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: > > On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > > > > > > ifneq ($(CONFIG_OF),) > > > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > > > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > > > endif > > > > As a generic driver, you may need to support both dt and non-dt > > solution. > > Since the dt is now the best practice and since there is no need (yet) > for a non-dt usage of this driver shouldn't we let anyone needing it > implement it when the time comes? > No, at least your code structure should support both dt and non-dt, and let the compile pass for non-dt platform if you don't have one. Then, someone with non-dt platform can change few to support it. A good example is: drivers/usb/host/ehci-platform.c > > > +static int ci_hdrc_generic_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct ci_hdrc_generic_priv *priv; > > > + struct ci_hdrc_platform_data ci_pdata = { > > > + .name = "ci_hdrc", > > > > How about this using dev_name(&pdev->dev) for name? > > Yes, why not. I don't have a strong preference. > > > > + > > > +clk_err: > > > + clk_disable_unprepare(priv->clk); > > > > You may need to add "if (!IS_ERR(priv->clk))" > > Right! I'll update this. > > > > + > > > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > > > + { .compatible = "chipidea-usb" }, > > > + { } > > > +}; > > > > Even as a generic driver, you can also use your own compatible string. > > Well, there is nothing specific about the Berlin CI. Some subsystems > use the 'generic' keyword in these cases. Do you see a particular reason > I should use some Berlin related compatible here? > Not must, one suggestion is: can you change the compatible string to "chipidea-usb-generic"?
Peter, On Tue, Jul 01, 2014 at 08:21:14AM +0800, Peter Chen wrote: > On Mon, Jun 30, 2014 at 03:33:13PM +0200, Antoine Ténart wrote: > > On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: > > > On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > > > > > > > > ifneq ($(CONFIG_OF),) > > > > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > > > > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > > > > endif > > > > > > As a generic driver, you may need to support both dt and non-dt > > > solution. > > > > Since the dt is now the best practice and since there is no need (yet) > > for a non-dt usage of this driver shouldn't we let anyone needing it > > implement it when the time comes? > > > > No, at least your code structure should support both dt and non-dt, > and let the compile pass for non-dt platform if you don't have one. > Then, someone with non-dt platform can change few to support it. > A good example is: drivers/usb/host/ehci-platform.c Ok. I'll isolate dt-specific code in the probe, and remove the CONFIG_OF dependency. > > > > > +static int ci_hdrc_generic_probe(struct platform_device *pdev) > > > > +{ > > > > + struct device *dev = &pdev->dev; > > > > + struct ci_hdrc_generic_priv *priv; > > > > + struct ci_hdrc_platform_data ci_pdata = { > > > > + .name = "ci_hdrc", > > > > > > How about this using dev_name(&pdev->dev) for name? > > > > Yes, why not. I don't have a strong preference. > > > > > > + > > > > +clk_err: > > > > + clk_disable_unprepare(priv->clk); > > > > > > You may need to add "if (!IS_ERR(priv->clk))" > > > > Right! I'll update this. > > > > > > + > > > > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > > > > + { .compatible = "chipidea-usb" }, > > > > + { } > > > > +}; > > > > > > Even as a generic driver, you can also use your own compatible string. > > > > Well, there is nothing specific about the Berlin CI. Some subsystems > > use the 'generic' keyword in these cases. Do you see a particular reason > > I should use some Berlin related compatible here? > > > > Not must, one suggestion is: can you change the compatible string > to "chipidea-usb-generic"? Sounds good, I'll update. Antoine
On Tue, Jul 01, 2014 at 10:55:37AM +0200, Sebastian Hesselbarth wrote: > On 07/01/2014 02:21 AM, Peter Chen wrote: > >On Mon, Jun 30, 2014 at 03:33:13PM +0200, Antoine Ténart wrote: > >>On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: > >>>On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: > >>>>+ > >>>>+static const struct of_device_id ci_hdrc_generic_of_match[] = { > >>>>+ { .compatible = "chipidea-usb" }, > >>>>+ { } > >>>>+}; > >>> > >>>Even as a generic driver, you can also use your own compatible string. > >> > >>Well, there is nothing specific about the Berlin CI. Some subsystems > >>use the 'generic' keyword in these cases. Do you see a particular reason > >>I should use some Berlin related compatible here? > > > >Not must, one suggestion is: can you change the compatible string > >to "chipidea-usb-generic"? > > I don't know about ChipIdea/ARC/DW's product portfolio but I guess > the compatible should also carry '2.0' or 'usb2' in it. Or we just > use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. > > Sebastian > The recommended format for compatible string is: "manufacturer,model", I agree with "chipidea,ci13xxx", thanks.
On 07/01/2014 02:21 AM, Peter Chen wrote: > On Mon, Jun 30, 2014 at 03:33:13PM +0200, Antoine Ténart wrote: >> On Fri, Jun 27, 2014 at 11:25:07AM +0800, Peter Chen wrote: >>> On Tue, Jun 24, 2014 at 12:35:16PM +0200, Antoine Ténart wrote: >>>> + >>>> +static const struct of_device_id ci_hdrc_generic_of_match[] = { >>>> + { .compatible = "chipidea-usb" }, >>>> + { } >>>> +}; >>> >>> Even as a generic driver, you can also use your own compatible string. >> >> Well, there is nothing specific about the Berlin CI. Some subsystems >> use the 'generic' keyword in these cases. Do you see a particular reason >> I should use some Berlin related compatible here? > > Not must, one suggestion is: can you change the compatible string > to "chipidea-usb-generic"? I don't know about ChipIdea/ARC/DW's product portfolio but I guess the compatible should also carry '2.0' or 'usb2' in it. Or we just use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. Sebastian
On 01/07/2014 at 16:30:08 +0800, Peter Chen wrote : > > >>Well, there is nothing specific about the Berlin CI. Some subsystems > > >>use the 'generic' keyword in these cases. Do you see a particular reason > > >>I should use some Berlin related compatible here? > > > > > >Not must, one suggestion is: can you change the compatible string > > >to "chipidea-usb-generic"? > > > > I don't know about ChipIdea/ARC/DW's product portfolio but I guess > > the compatible should also carry '2.0' or 'usb2' in it. Or we just > > use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. > > > > The recommended format for compatible string is: "manufacturer,model", > I agree with "chipidea,ci13xxx", thanks. > I think we should probably avoid using wildcards in the compatible string.
Hello. On 07/01/2014 02:42 PM, Alexandre Belloni wrote: >>>>> Well, there is nothing specific about the Berlin CI. Some subsystems >>>>> use the 'generic' keyword in these cases. Do you see a particular reason >>>>> I should use some Berlin related compatible here? >>>> Not must, one suggestion is: can you change the compatible string >>>> to "chipidea-usb-generic"? >>> I don't know about ChipIdea/ARC/DW's product portfolio but I guess >>> the compatible should also carry '2.0' or 'usb2' in it. Or we just >>> use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. >> The recommended format for compatible string is: "manufacturer,model", >> I agree with "chipidea,ci13xxx", thanks. > I think we should probably avoid using wildcards in the compatible > string. I'm sure wildcards shouldn't be allowed there. :-) WBR, Sergei
> > Hello. > > On 07/01/2014 02:42 PM, Alexandre Belloni wrote: > > >>>>> Well, there is nothing specific about the Berlin CI. Some > >>>>> subsystems use the 'generic' keyword in these cases. Do you see a > >>>>> particular reason I should use some Berlin related compatible here? > > >>>> Not must, one suggestion is: can you change the compatible string > >>>> to "chipidea-usb-generic"? > > >>> I don't know about ChipIdea/ARC/DW's product portfolio but I guess > >>> the compatible should also carry '2.0' or 'usb2' in it. Or we just > >>> use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. > > >> The recommended format for compatible string is: > >> "manufacturer,model", I agree with "chipidea,ci13xxx", thanks. > > > I think we should probably avoid using wildcards in the compatible > > string. > > I'm sure wildcards shouldn't be allowed there. :-) Then, what's your guys recommend, how about "chipidea,usb2-generic"? Peter
Since its a generic driver, support for configuring the dma_mask using dma_coerce_mask_and_coherent would be good. Regards, Punnaiah On Tue, Jun 24, 2014 at 4:05 PM, Antoine Ténart <antoine.tenart@free-electrons.com> wrote: > Add a generic ChipIdea driver, with optional PHY and clock, to support > ChipIdea controllers that doesn't need specific functions. > > Needed for the Marvell Berlin SoCs SUB controllers. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/usb/chipidea/Makefile | 1 + > drivers/usb/chipidea/ci_hdrc_generic.c | 108 +++++++++++++++++++++++++++++++++ > 2 files changed, 109 insertions(+) > create mode 100644 drivers/usb/chipidea/ci_hdrc_generic.c > > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile > index 2f099c7df7b5..c705f0fe7a00 100644 > --- a/drivers/usb/chipidea/Makefile > +++ b/drivers/usb/chipidea/Makefile > @@ -20,4 +20,5 @@ endif > > ifneq ($(CONFIG_OF),) > obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o > + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o > endif > diff --git a/drivers/usb/chipidea/ci_hdrc_generic.c b/drivers/usb/chipidea/ci_hdrc_generic.c > new file mode 100644 > index 000000000000..27520710a1f7 > --- /dev/null > +++ b/drivers/usb/chipidea/ci_hdrc_generic.c > @@ -0,0 +1,108 @@ > +/* > + * Copyright (C) 2014 Marvell Technology Group Ltd. > + * > + * Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/clk.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/usb/chipidea.h> > +#include <linux/usb/hcd.h> > +#include <linux/usb/ulpi.h> > + > +#include "ci.h" > + > +struct ci_hdrc_generic_priv { > + struct platform_device *ci_pdev; > + struct clk *clk; > +}; > + > +static int ci_hdrc_generic_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct ci_hdrc_generic_priv *priv; > + struct ci_hdrc_platform_data ci_pdata = { > + .name = "ci_hdrc", > + .capoffset = DEF_CAPOFFSET, > + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | > + CI_HDRC_DISABLE_STREAMING, > + }; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (!IS_ERR(priv->clk)) { > + ret = clk_prepare_enable(priv->clk); > + if (ret) { > + dev_err(dev, "failed to enable the clock: %d\n", ret); > + return ret; > + } > + } > + > + ci_pdata.phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > + if (IS_ERR(ci_pdata.phy)) > + /* PHY is optional */ > + ci_pdata.phy = NULL; > + > + priv->ci_pdev = ci_hdrc_add_device(dev, pdev->resource, > + pdev->num_resources, &ci_pdata); > + if (IS_ERR(priv->ci_pdev)) { > + ret = PTR_ERR(priv->ci_pdev); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, > + "failed to register ci_hdrc platform device: %d\n", > + ret); > + goto clk_err; > + } > + > + platform_set_drvdata(pdev, priv); > + > + pm_runtime_no_callbacks(dev); > + pm_runtime_enable(dev); > + > + return 0; > + > +clk_err: > + clk_disable_unprepare(priv->clk); > + return ret; > +} > + > +static int ci_hdrc_generic_remove(struct platform_device *pdev) > +{ > + struct ci_hdrc_generic_priv *priv = platform_get_drvdata(pdev); > + > + pm_runtime_disable(&pdev->dev); > + ci_hdrc_remove_device(priv->ci_pdev); > + clk_disable_unprepare(priv->clk); > + > + return 0; > +} > + > +static const struct of_device_id ci_hdrc_generic_of_match[] = { > + { .compatible = "chipidea-usb" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ci_hdrc_generic_of_match); > + > +static struct platform_driver ci_hdrc_generic_driver = { > + .probe = ci_hdrc_generic_probe, > + .remove = ci_hdrc_generic_remove, > + .driver = { > + .name = "chipidea-usb", > + .owner = THIS_MODULE, > + .of_match_table = ci_hdrc_generic_of_match, > + }, > +}; > +module_platform_driver(ci_hdrc_generic_driver); > + > +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); > +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); > +MODULE_LICENSE("GPL"); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi guys, On Wed, Jul 02, 2014 at 01:10:00AM +0000, Peter Chen wrote: > > On 07/01/2014 02:42 PM, Alexandre Belloni wrote: > > > > >>>>> Well, there is nothing specific about the Berlin CI. Some > > >>>>> subsystems use the 'generic' keyword in these cases. Do you see a > > >>>>> particular reason I should use some Berlin related compatible here? > > > > >>>> Not must, one suggestion is: can you change the compatible string > > >>>> to "chipidea-usb-generic"? > > > > >>> I don't know about ChipIdea/ARC/DW's product portfolio but I guess > > >>> the compatible should also carry '2.0' or 'usb2' in it. Or we just > > >>> use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. > > > > >> The recommended format for compatible string is: > > >> "manufacturer,model", I agree with "chipidea,ci13xxx", thanks. > > > > > I think we should probably avoid using wildcards in the compatible > > > string. > > > > I'm sure wildcards shouldn't be allowed there. :-) > > Then, what's your guys recommend, how about "chipidea,usb2-generic"? So what do you think? "chipidea,ci13", "chipidea,usb2-generic" or something else? I tend to prefer something without the 'generic' keyword. After updating the compatible I'll send a v3 based on USB changes introducing the generic PHY support[1]. [1] https://lkml.org/lkml/2014/7/15/330 Antoine
Hi, On Thu, Jul 03, 2014 at 08:17:34AM +0530, punnaiah choudary kalluri wrote: > Since its a generic driver, support for configuring the dma_mask using > dma_coerce_mask_and_coherent would be good. > Should I add a DMA mask dt property? Or just not add this and wait for someone needing it in his case? Antoine
On Tue, Jul 15, 2014 at 05:22:30PM +0200, Antoine Ténart wrote: > Hi guys, > > On Wed, Jul 02, 2014 at 01:10:00AM +0000, Peter Chen wrote: > > > On 07/01/2014 02:42 PM, Alexandre Belloni wrote: > > > > > > >>>>> Well, there is nothing specific about the Berlin CI. Some > > > >>>>> subsystems use the 'generic' keyword in these cases. Do you see a > > > >>>>> particular reason I should use some Berlin related compatible here? > > > > > > >>>> Not must, one suggestion is: can you change the compatible string > > > >>>> to "chipidea-usb-generic"? > > > > > > >>> I don't know about ChipIdea/ARC/DW's product portfolio but I guess > > > >>> the compatible should also carry '2.0' or 'usb2' in it. Or we just > > > >>> use some version number like 'chipidea,ci13000' or 'chipidea,ci13xxx'. > > > > > > >> The recommended format for compatible string is: > > > >> "manufacturer,model", I agree with "chipidea,ci13xxx", thanks. > > > > > > > I think we should probably avoid using wildcards in the compatible > > > > string. > > > > > > I'm sure wildcards shouldn't be allowed there. :-) > > > > Then, what's your guys recommend, how about "chipidea,usb2-generic"? > > So what do you think? "chipidea,ci13", "chipidea,usb2-generic" or > something else? > Do you agree to use "chipidea,usb2", And add ci13xxx at MODULE_DESCRIPTION?
On Tue, Jul 15, 2014 at 05:24:37PM +0200, Antoine Ténart wrote: > Hi, > > On Thu, Jul 03, 2014 at 08:17:34AM +0530, punnaiah choudary kalluri wrote: > > Since its a generic driver, support for configuring the dma_mask using > > dma_coerce_mask_and_coherent would be good. > > > > Should I add a DMA mask dt property? Or just not add this and wait for > someone needing it in his case? > As a dt property, since most of platforms will use it. For PIO mode, the property can be NULL.
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile index 2f099c7df7b5..c705f0fe7a00 100644 --- a/drivers/usb/chipidea/Makefile +++ b/drivers/usb/chipidea/Makefile @@ -20,4 +20,5 @@ endif ifneq ($(CONFIG_OF),) obj-$(CONFIG_USB_CHIPIDEA) += usbmisc_imx.o ci_hdrc_imx.o + obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc_generic.o endif diff --git a/drivers/usb/chipidea/ci_hdrc_generic.c b/drivers/usb/chipidea/ci_hdrc_generic.c new file mode 100644 index 000000000000..27520710a1f7 --- /dev/null +++ b/drivers/usb/chipidea/ci_hdrc_generic.c @@ -0,0 +1,108 @@ +/* + * Copyright (C) 2014 Marvell Technology Group Ltd. + * + * Antoine Ténart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/usb/chipidea.h> +#include <linux/usb/hcd.h> +#include <linux/usb/ulpi.h> + +#include "ci.h" + +struct ci_hdrc_generic_priv { + struct platform_device *ci_pdev; + struct clk *clk; +}; + +static int ci_hdrc_generic_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct ci_hdrc_generic_priv *priv; + struct ci_hdrc_platform_data ci_pdata = { + .name = "ci_hdrc", + .capoffset = DEF_CAPOFFSET, + .flags = CI_HDRC_REQUIRE_TRANSCEIVER | + CI_HDRC_DISABLE_STREAMING, + }; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->clk = devm_clk_get(dev, NULL); + if (!IS_ERR(priv->clk)) { + ret = clk_prepare_enable(priv->clk); + if (ret) { + dev_err(dev, "failed to enable the clock: %d\n", ret); + return ret; + } + } + + ci_pdata.phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); + if (IS_ERR(ci_pdata.phy)) + /* PHY is optional */ + ci_pdata.phy = NULL; + + priv->ci_pdev = ci_hdrc_add_device(dev, pdev->resource, + pdev->num_resources, &ci_pdata); + if (IS_ERR(priv->ci_pdev)) { + ret = PTR_ERR(priv->ci_pdev); + if (ret != -EPROBE_DEFER) + dev_err(dev, + "failed to register ci_hdrc platform device: %d\n", + ret); + goto clk_err; + } + + platform_set_drvdata(pdev, priv); + + pm_runtime_no_callbacks(dev); + pm_runtime_enable(dev); + + return 0; + +clk_err: + clk_disable_unprepare(priv->clk); + return ret; +} + +static int ci_hdrc_generic_remove(struct platform_device *pdev) +{ + struct ci_hdrc_generic_priv *priv = platform_get_drvdata(pdev); + + pm_runtime_disable(&pdev->dev); + ci_hdrc_remove_device(priv->ci_pdev); + clk_disable_unprepare(priv->clk); + + return 0; +} + +static const struct of_device_id ci_hdrc_generic_of_match[] = { + { .compatible = "chipidea-usb" }, + { } +}; +MODULE_DEVICE_TABLE(of, ci_hdrc_generic_of_match); + +static struct platform_driver ci_hdrc_generic_driver = { + .probe = ci_hdrc_generic_probe, + .remove = ci_hdrc_generic_remove, + .driver = { + .name = "chipidea-usb", + .owner = THIS_MODULE, + .of_match_table = ci_hdrc_generic_of_match, + }, +}; +module_platform_driver(ci_hdrc_generic_driver); + +MODULE_DESCRIPTION("Generic ChipIdea HDRC USB binding"); +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); +MODULE_LICENSE("GPL");
Add a generic ChipIdea driver, with optional PHY and clock, to support ChipIdea controllers that doesn't need specific functions. Needed for the Marvell Berlin SoCs SUB controllers. Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/usb/chipidea/Makefile | 1 + drivers/usb/chipidea/ci_hdrc_generic.c | 108 +++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 drivers/usb/chipidea/ci_hdrc_generic.c