Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
On Wed, Dec 19, 2012 at 10:37 PM, Vivek Gautam wrote: > > On Thu, Dec 20, 2012 at 5:00 AM, Doug Anderson wrote: >> >> On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam >> wrote: >>> +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) >>> +{ >>> + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); >>> + >>> + if (s5p_ehci->phy) { >>> + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); >> >> This confuses me. Why are you setting the type to host here? >> > Being in host controller, before calling usb_phy_init() we set type to > Host since, with certain SOCs > like 4210, same register has different bit settings for HOST type and > device type. So setting this > to Host type here make the flow of usb_phy_init to go in the direction of > Host. OK. I think I need to study the code more... >>> >>> + phy = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2); >>> + if (IS_ERR_OR_NULL(phy)) { >>> + /* Fallback to pdata */ >>> + if (!pdata) { >>> + dev_err(>dev, "no platform data or >>> transceiver defined\n"); >>> + return -EPROBE_DEFER; >> >> Shouldn't you return -EINVAL like the old code did? > > We are deferring the probe since the usb-phy transceiver may get > probed after ehci/ohci controllers. > And if we return -EINVAL like the previous code, we would end up not > setting the phy. OK. Something is wrong here then, since this really isn't an error: * It should be commented about why you're deferring. * You shouldn't have a dev_err for something that's not fatal. Ideally we'd want something that would force probing to happen in the right order. I spent a little time looking and didn't see anything, but maybe I'm missing something obvious. If nothing pops out, the defer seems OK as long as it is commented well. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
On Wed, Dec 19, 2012 at 10:37 PM, Vivek Gautam gautamvivek1...@gmail.com wrote: On Thu, Dec 20, 2012 at 5:00 AM, Doug Anderson diand...@chromium.org wrote: On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam gautam.vi...@samsung.com wrote: +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); This confuses me. Why are you setting the type to host here? Being in host controller, before calling usb_phy_init() we set type to Host since, with certain SOCs like 4210, same register has different bit settings for HOST type and device type. So setting this to Host type here make the flow of usb_phy_init to go in the direction of Host. OK. I think I need to study the code more... + phy = devm_usb_get_phy(pdev-dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(pdev-dev, no platform data or transceiver defined\n); + return -EPROBE_DEFER; Shouldn't you return -EINVAL like the old code did? We are deferring the probe since the usb-phy transceiver may get probed after ehci/ohci controllers. And if we return -EINVAL like the previous code, we would end up not setting the phy. OK. Something is wrong here then, since this really isn't an error: * It should be commented about why you're deferring. * You shouldn't have a dev_err for something that's not fatal. Ideally we'd want something that would force probing to happen in the right order. I spent a little time looking and didn't see anything, but maybe I'm missing something obvious. If nothing pops out, the defer seems OK as long as it is commented well. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Hi Doug, On Thu, Dec 20, 2012 at 5:00 AM, Doug Anderson wrote: > Vivek, > > Again, not a high-level review, but... > Thanks for reviewing. :-) > > On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam > wrote: >> Adding the phy driver to ehci-s5p. Keeping the platform data >> for continuing the smooth operation for boards which still uses it >> >> Signed-off-by: Vivek Gautam >> Acked-by: Jingoo Han >> --- >> drivers/usb/host/ehci-s5p.c | 70 >> ++- >> 1 files changed, 49 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c >> index 46ca5ef..50c93af 100644 >> --- a/drivers/usb/host/ehci-s5p.c >> +++ b/drivers/usb/host/ehci-s5p.c >> @@ -17,6 +17,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { >> struct device *dev; >> struct usb_hcd *hcd; >> struct clk *clk; >> + struct usb_phy *phy; >> + struct s5p_ehci_platdata *pdata; >> }; >> >> static const struct hc_driver s5p_ehci_hc_driver = { >> @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { >> .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, >> }; >> >> +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) >> +{ >> + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); >> + >> + if (s5p_ehci->phy) { >> + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); > > This confuses me. Why are you setting the type to host here? > Being in host controller, before calling usb_phy_init() we set type to Host since, with certain SOCs like 4210, same register has different bit settings for HOST type and device type. So setting this to Host type here make the flow of usb_phy_init to go in the direction of Host. >> + usb_phy_init(s5p_ehci->phy); >> + } else if (s5p_ehci->pdata->phy_init) { >> + s5p_ehci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST); >> + } >> +} >> + >> +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) >> +{ >> + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); >> + >> + if (s5p_ehci->phy) { >> + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); > > ...and why set it to host here? > Same here... >> + usb_phy_shutdown(s5p_ehci->phy); >> + } else if (s5p_ehci->pdata->phy_exit) { >> + s5p_ehci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); >> + } >> +} >> + >> static void s5p_setup_vbus_gpio(struct platform_device *pdev) >> { >> int err; >> @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); >> >> static int s5p_ehci_probe(struct platform_device *pdev) >> { >> - struct s5p_ehci_platdata *pdata; >> + struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; >> struct s5p_ehci_hcd *s5p_ehci; >> struct usb_hcd *hcd; >> struct ehci_hcd *ehci; >> struct resource *res; >> + struct usb_phy *phy; >> int irq; >> int err; >> >> - pdata = pdev->dev.platform_data; >> - if (!pdata) { >> - dev_err(>dev, "No platform data defined\n"); >> - return -EINVAL; >> - } >> - >> /* >> * Right now device-tree probed devices don't get dma_mask set. >> * Since shared usb code relies on it, set it here for now. >> @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) >> if (!s5p_ehci) >> return -ENOMEM; >> >> + phy = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2); >> + if (IS_ERR_OR_NULL(phy)) { >> + /* Fallback to pdata */ >> + if (!pdata) { >> + dev_err(>dev, "no platform data or transceiver >> defined\n"); >> + return -EPROBE_DEFER; > > Shouldn't you return -EINVAL like the old code did? We are deferring the probe since the usb-phy transceiver may get probed after ehci/ohci controllers. And if we return -EINVAL like the previous code, we would end up not setting the phy. > >> + } else { >> + s5p_ehci->pdata = pdata; >> + } >> + } else { >> + s5p_ehci->phy = phy; >> + } >> + >> s5p_ehci->dev = >dev; >> >> hcd = usb_create_hcd(_ehci_hc_driver, >dev, >> @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) >> goto fail_io; >> } >> >> - if (pdata->phy_init) >> - pdata->phy_init(pdev, USB_PHY_TYPE_HOST); >> + s5p_ehci_phy_enable(s5p_ehci); >> >> ehci = hcd_to_ehci(hcd); >> ehci->caps = hcd->regs; >> @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) >> err = usb_add_hcd(hcd, irq, IRQF_SHARED); >> if (err) { >>
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Vivek, Again, not a high-level review, but... On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam wrote: > Adding the phy driver to ehci-s5p. Keeping the platform data > for continuing the smooth operation for boards which still uses it > > Signed-off-by: Vivek Gautam > Acked-by: Jingoo Han > --- > drivers/usb/host/ehci-s5p.c | 70 > ++- > 1 files changed, 49 insertions(+), 21 deletions(-) > > diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c > index 46ca5ef..50c93af 100644 > --- a/drivers/usb/host/ehci-s5p.c > +++ b/drivers/usb/host/ehci-s5p.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { > struct device *dev; > struct usb_hcd *hcd; > struct clk *clk; > + struct usb_phy *phy; > + struct s5p_ehci_platdata *pdata; > }; > > static const struct hc_driver s5p_ehci_hc_driver = { > @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { > .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, > }; > > +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) > +{ > + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); > + > + if (s5p_ehci->phy) { > + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); This confuses me. Why are you setting the type to host here? > + usb_phy_init(s5p_ehci->phy); > + } else if (s5p_ehci->pdata->phy_init) { > + s5p_ehci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST); > + } > +} > + > +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) > +{ > + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); > + > + if (s5p_ehci->phy) { > + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); ...and why set it to host here? > + usb_phy_shutdown(s5p_ehci->phy); > + } else if (s5p_ehci->pdata->phy_exit) { > + s5p_ehci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); > + } > +} > + > static void s5p_setup_vbus_gpio(struct platform_device *pdev) > { > int err; > @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); > > static int s5p_ehci_probe(struct platform_device *pdev) > { > - struct s5p_ehci_platdata *pdata; > + struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; > struct s5p_ehci_hcd *s5p_ehci; > struct usb_hcd *hcd; > struct ehci_hcd *ehci; > struct resource *res; > + struct usb_phy *phy; > int irq; > int err; > > - pdata = pdev->dev.platform_data; > - if (!pdata) { > - dev_err(>dev, "No platform data defined\n"); > - return -EINVAL; > - } > - > /* > * Right now device-tree probed devices don't get dma_mask set. > * Since shared usb code relies on it, set it here for now. > @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) > if (!s5p_ehci) > return -ENOMEM; > > + phy = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2); > + if (IS_ERR_OR_NULL(phy)) { > + /* Fallback to pdata */ > + if (!pdata) { > + dev_err(>dev, "no platform data or transceiver > defined\n"); > + return -EPROBE_DEFER; Shouldn't you return -EINVAL like the old code did? > + } else { > + s5p_ehci->pdata = pdata; > + } > + } else { > + s5p_ehci->phy = phy; > + } > + > s5p_ehci->dev = >dev; > > hcd = usb_create_hcd(_ehci_hc_driver, >dev, > @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) > goto fail_io; > } > > - if (pdata->phy_init) > - pdata->phy_init(pdev, USB_PHY_TYPE_HOST); > + s5p_ehci_phy_enable(s5p_ehci); > > ehci = hcd_to_ehci(hcd); > ehci->caps = hcd->regs; > @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) > err = usb_add_hcd(hcd, irq, IRQF_SHARED); > if (err) { > dev_err(>dev, "Failed to add USB HCD\n"); > - goto fail_io; > + goto fail_add_hcd; > } > > platform_set_drvdata(pdev, s5p_ehci); > > return 0; > > +fail_add_hcd: > + s5p_ehci_phy_disable(s5p_ehci); > fail_io: > clk_disable_unprepare(s5p_ehci->clk); > fail_clk: > @@ -192,14 +228,12 @@ fail_clk: > > static int s5p_ehci_remove(struct platform_device *pdev) > { > - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; > struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); > struct usb_hcd *hcd = s5p_ehci->hcd; > > usb_remove_hcd(hcd); > > - if (pdata && pdata->phy_exit) > -
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Vivek, Again, not a high-level review, but... On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam gautam.vi...@samsung.com wrote: Adding the phy driver to ehci-s5p. Keeping the platform data for continuing the smooth operation for boards which still uses it Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Acked-by: Jingoo Han jg1@samsung.com --- drivers/usb/host/ehci-s5p.c | 70 ++- 1 files changed, 49 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c index 46ca5ef..50c93af 100644 --- a/drivers/usb/host/ehci-s5p.c +++ b/drivers/usb/host/ehci-s5p.c @@ -17,6 +17,7 @@ #include linux/platform_device.h #include linux/of_gpio.h #include linux/platform_data/usb-ehci-s5p.h +#include linux/usb/phy.h #include linux/usb/samsung_usb_phy.h #include plat/usb-phy.h @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { struct device *dev; struct usb_hcd *hcd; struct clk *clk; + struct usb_phy *phy; + struct s5p_ehci_platdata *pdata; }; static const struct hc_driver s5p_ehci_hc_driver = { @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, }; +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); This confuses me. Why are you setting the type to host here? + usb_phy_init(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_init) { + s5p_ehci-pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + } +} + +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); ...and why set it to host here? + usb_phy_shutdown(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_exit) { + s5p_ehci-pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + } +} + static void s5p_setup_vbus_gpio(struct platform_device *pdev) { int err; @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); static int s5p_ehci_probe(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata; + struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; + struct usb_phy *phy; int irq; int err; - pdata = pdev-dev.platform_data; - if (!pdata) { - dev_err(pdev-dev, No platform data defined\n); - return -EINVAL; - } - /* * Right now device-tree probed devices don't get dma_mask set. * Since shared usb code relies on it, set it here for now. @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) if (!s5p_ehci) return -ENOMEM; + phy = devm_usb_get_phy(pdev-dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(pdev-dev, no platform data or transceiver defined\n); + return -EPROBE_DEFER; Shouldn't you return -EINVAL like the old code did? + } else { + s5p_ehci-pdata = pdata; + } + } else { + s5p_ehci-phy = phy; + } + s5p_ehci-dev = pdev-dev; hcd = usb_create_hcd(s5p_ehci_hc_driver, pdev-dev, @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) goto fail_io; } - if (pdata-phy_init) - pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_enable(s5p_ehci); ehci = hcd_to_ehci(hcd); ehci-caps = hcd-regs; @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) { dev_err(pdev-dev, Failed to add USB HCD\n); - goto fail_io; + goto fail_add_hcd; } platform_set_drvdata(pdev, s5p_ehci); return 0; +fail_add_hcd: + s5p_ehci_phy_disable(s5p_ehci); fail_io: clk_disable_unprepare(s5p_ehci-clk); fail_clk: @@ -192,14 +228,12 @@ fail_clk: static int s5p_ehci_remove(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); struct usb_hcd *hcd = s5p_ehci-hcd; usb_remove_hcd(hcd); - if (pdata
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Hi Doug, On Thu, Dec 20, 2012 at 5:00 AM, Doug Anderson diand...@chromium.org wrote: Vivek, Again, not a high-level review, but... Thanks for reviewing. :-) On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam gautam.vi...@samsung.com wrote: Adding the phy driver to ehci-s5p. Keeping the platform data for continuing the smooth operation for boards which still uses it Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Acked-by: Jingoo Han jg1@samsung.com --- drivers/usb/host/ehci-s5p.c | 70 ++- 1 files changed, 49 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c index 46ca5ef..50c93af 100644 --- a/drivers/usb/host/ehci-s5p.c +++ b/drivers/usb/host/ehci-s5p.c @@ -17,6 +17,7 @@ #include linux/platform_device.h #include linux/of_gpio.h #include linux/platform_data/usb-ehci-s5p.h +#include linux/usb/phy.h #include linux/usb/samsung_usb_phy.h #include plat/usb-phy.h @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { struct device *dev; struct usb_hcd *hcd; struct clk *clk; + struct usb_phy *phy; + struct s5p_ehci_platdata *pdata; }; static const struct hc_driver s5p_ehci_hc_driver = { @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, }; +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); This confuses me. Why are you setting the type to host here? Being in host controller, before calling usb_phy_init() we set type to Host since, with certain SOCs like 4210, same register has different bit settings for HOST type and device type. So setting this to Host type here make the flow of usb_phy_init to go in the direction of Host. + usb_phy_init(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_init) { + s5p_ehci-pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + } +} + +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); ...and why set it to host here? Same here... + usb_phy_shutdown(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_exit) { + s5p_ehci-pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + } +} + static void s5p_setup_vbus_gpio(struct platform_device *pdev) { int err; @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); static int s5p_ehci_probe(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata; + struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; + struct usb_phy *phy; int irq; int err; - pdata = pdev-dev.platform_data; - if (!pdata) { - dev_err(pdev-dev, No platform data defined\n); - return -EINVAL; - } - /* * Right now device-tree probed devices don't get dma_mask set. * Since shared usb code relies on it, set it here for now. @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) if (!s5p_ehci) return -ENOMEM; + phy = devm_usb_get_phy(pdev-dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(pdev-dev, no platform data or transceiver defined\n); + return -EPROBE_DEFER; Shouldn't you return -EINVAL like the old code did? We are deferring the probe since the usb-phy transceiver may get probed after ehci/ohci controllers. And if we return -EINVAL like the previous code, we would end up not setting the phy. + } else { + s5p_ehci-pdata = pdata; + } + } else { + s5p_ehci-phy = phy; + } + s5p_ehci-dev = pdev-dev; hcd = usb_create_hcd(s5p_ehci_hc_driver, pdev-dev, @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) goto fail_io; } - if (pdata-phy_init) - pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_enable(s5p_ehci); ehci = hcd_to_ehci(hcd); ehci-caps = hcd-regs; @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) { dev_err(pdev-dev, Failed to add USB
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
CC: Doug Anderson. On Tue, Dec 18, 2012 at 8:13 PM, Vivek Gautam wrote: > Adding the phy driver to ehci-s5p. Keeping the platform data > for continuing the smooth operation for boards which still uses it > > Signed-off-by: Vivek Gautam > Acked-by: Jingoo Han > --- > drivers/usb/host/ehci-s5p.c | 70 > ++- > 1 files changed, 49 insertions(+), 21 deletions(-) > > diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c > index 46ca5ef..50c93af 100644 > --- a/drivers/usb/host/ehci-s5p.c > +++ b/drivers/usb/host/ehci-s5p.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { > struct device *dev; > struct usb_hcd *hcd; > struct clk *clk; > + struct usb_phy *phy; > + struct s5p_ehci_platdata *pdata; > }; > > static const struct hc_driver s5p_ehci_hc_driver = { > @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { > .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, > }; > > +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) > +{ > + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); > + > + if (s5p_ehci->phy) { > + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); > + usb_phy_init(s5p_ehci->phy); > + } else if (s5p_ehci->pdata->phy_init) { > + s5p_ehci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST); > + } > +} > + > +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) > +{ > + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); > + > + if (s5p_ehci->phy) { > + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); > + usb_phy_shutdown(s5p_ehci->phy); > + } else if (s5p_ehci->pdata->phy_exit) { > + s5p_ehci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); > + } > +} > + > static void s5p_setup_vbus_gpio(struct platform_device *pdev) > { > int err; > @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); > > static int s5p_ehci_probe(struct platform_device *pdev) > { > - struct s5p_ehci_platdata *pdata; > + struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; > struct s5p_ehci_hcd *s5p_ehci; > struct usb_hcd *hcd; > struct ehci_hcd *ehci; > struct resource *res; > + struct usb_phy *phy; > int irq; > int err; > > - pdata = pdev->dev.platform_data; > - if (!pdata) { > - dev_err(>dev, "No platform data defined\n"); > - return -EINVAL; > - } > - > /* > * Right now device-tree probed devices don't get dma_mask set. > * Since shared usb code relies on it, set it here for now. > @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) > if (!s5p_ehci) > return -ENOMEM; > > + phy = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2); > + if (IS_ERR_OR_NULL(phy)) { > + /* Fallback to pdata */ > + if (!pdata) { > + dev_err(>dev, "no platform data or transceiver > defined\n"); > + return -EPROBE_DEFER; > + } else { > + s5p_ehci->pdata = pdata; > + } > + } else { > + s5p_ehci->phy = phy; > + } > + > s5p_ehci->dev = >dev; > > hcd = usb_create_hcd(_ehci_hc_driver, >dev, > @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) > goto fail_io; > } > > - if (pdata->phy_init) > - pdata->phy_init(pdev, USB_PHY_TYPE_HOST); > + s5p_ehci_phy_enable(s5p_ehci); > > ehci = hcd_to_ehci(hcd); > ehci->caps = hcd->regs; > @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) > err = usb_add_hcd(hcd, irq, IRQF_SHARED); > if (err) { > dev_err(>dev, "Failed to add USB HCD\n"); > - goto fail_io; > + goto fail_add_hcd; > } > > platform_set_drvdata(pdev, s5p_ehci); > > return 0; > > +fail_add_hcd: > + s5p_ehci_phy_disable(s5p_ehci); > fail_io: > clk_disable_unprepare(s5p_ehci->clk); > fail_clk: > @@ -192,14 +228,12 @@ fail_clk: > > static int s5p_ehci_remove(struct platform_device *pdev) > { > - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; > struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); > struct usb_hcd *hcd = s5p_ehci->hcd; > > usb_remove_hcd(hcd); > > - if (pdata && pdata->phy_exit) > - pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); > + s5p_ehci_phy_disable(s5p_ehci); > > clk_disable_unprepare(s5p_ehci->clk); > > @@ -223,14 +257,11 @@ static int
[PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Adding the phy driver to ehci-s5p. Keeping the platform data for continuing the smooth operation for boards which still uses it Signed-off-by: Vivek Gautam Acked-by: Jingoo Han --- drivers/usb/host/ehci-s5p.c | 70 ++- 1 files changed, 49 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c index 46ca5ef..50c93af 100644 --- a/drivers/usb/host/ehci-s5p.c +++ b/drivers/usb/host/ehci-s5p.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { struct device *dev; struct usb_hcd *hcd; struct clk *clk; + struct usb_phy *phy; + struct s5p_ehci_platdata *pdata; }; static const struct hc_driver s5p_ehci_hc_driver = { @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, }; +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); + + if (s5p_ehci->phy) { + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); + usb_phy_init(s5p_ehci->phy); + } else if (s5p_ehci->pdata->phy_init) { + s5p_ehci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST); + } +} + +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci->dev); + + if (s5p_ehci->phy) { + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST); + usb_phy_shutdown(s5p_ehci->phy); + } else if (s5p_ehci->pdata->phy_exit) { + s5p_ehci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); + } +} + static void s5p_setup_vbus_gpio(struct platform_device *pdev) { int err; @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); static int s5p_ehci_probe(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata; + struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; struct s5p_ehci_hcd *s5p_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; + struct usb_phy *phy; int irq; int err; - pdata = pdev->dev.platform_data; - if (!pdata) { - dev_err(>dev, "No platform data defined\n"); - return -EINVAL; - } - /* * Right now device-tree probed devices don't get dma_mask set. * Since shared usb code relies on it, set it here for now. @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) if (!s5p_ehci) return -ENOMEM; + phy = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(>dev, "no platform data or transceiver defined\n"); + return -EPROBE_DEFER; + } else { + s5p_ehci->pdata = pdata; + } + } else { + s5p_ehci->phy = phy; + } + s5p_ehci->dev = >dev; hcd = usb_create_hcd(_ehci_hc_driver, >dev, @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) goto fail_io; } - if (pdata->phy_init) - pdata->phy_init(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_enable(s5p_ehci); ehci = hcd_to_ehci(hcd); ehci->caps = hcd->regs; @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) { dev_err(>dev, "Failed to add USB HCD\n"); - goto fail_io; + goto fail_add_hcd; } platform_set_drvdata(pdev, s5p_ehci); return 0; +fail_add_hcd: + s5p_ehci_phy_disable(s5p_ehci); fail_io: clk_disable_unprepare(s5p_ehci->clk); fail_clk: @@ -192,14 +228,12 @@ fail_clk: static int s5p_ehci_remove(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); struct usb_hcd *hcd = s5p_ehci->hcd; usb_remove_hcd(hcd); - if (pdata && pdata->phy_exit) - pdata->phy_exit(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_disable(s5p_ehci); clk_disable_unprepare(s5p_ehci->clk); @@ -223,14 +257,11 @@ static int s5p_ehci_suspend(struct device *dev) struct s5p_ehci_hcd *s5p_ehci = dev_get_drvdata(dev); struct usb_hcd *hcd = s5p_ehci->hcd; bool do_wakeup = device_may_wakeup(dev); - struct platform_device *pdev = to_platform_device(dev); - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data; int rc; rc =
[PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Adding the phy driver to ehci-s5p. Keeping the platform data for continuing the smooth operation for boards which still uses it Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Acked-by: Jingoo Han jg1@samsung.com --- drivers/usb/host/ehci-s5p.c | 70 ++- 1 files changed, 49 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c index 46ca5ef..50c93af 100644 --- a/drivers/usb/host/ehci-s5p.c +++ b/drivers/usb/host/ehci-s5p.c @@ -17,6 +17,7 @@ #include linux/platform_device.h #include linux/of_gpio.h #include linux/platform_data/usb-ehci-s5p.h +#include linux/usb/phy.h #include linux/usb/samsung_usb_phy.h #include plat/usb-phy.h @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { struct device *dev; struct usb_hcd *hcd; struct clk *clk; + struct usb_phy *phy; + struct s5p_ehci_platdata *pdata; }; static const struct hc_driver s5p_ehci_hc_driver = { @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, }; +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); + usb_phy_init(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_init) { + s5p_ehci-pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + } +} + +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); + usb_phy_shutdown(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_exit) { + s5p_ehci-pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + } +} + static void s5p_setup_vbus_gpio(struct platform_device *pdev) { int err; @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); static int s5p_ehci_probe(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata; + struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; + struct usb_phy *phy; int irq; int err; - pdata = pdev-dev.platform_data; - if (!pdata) { - dev_err(pdev-dev, No platform data defined\n); - return -EINVAL; - } - /* * Right now device-tree probed devices don't get dma_mask set. * Since shared usb code relies on it, set it here for now. @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) if (!s5p_ehci) return -ENOMEM; + phy = devm_usb_get_phy(pdev-dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(pdev-dev, no platform data or transceiver defined\n); + return -EPROBE_DEFER; + } else { + s5p_ehci-pdata = pdata; + } + } else { + s5p_ehci-phy = phy; + } + s5p_ehci-dev = pdev-dev; hcd = usb_create_hcd(s5p_ehci_hc_driver, pdev-dev, @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) goto fail_io; } - if (pdata-phy_init) - pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_enable(s5p_ehci); ehci = hcd_to_ehci(hcd); ehci-caps = hcd-regs; @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) { dev_err(pdev-dev, Failed to add USB HCD\n); - goto fail_io; + goto fail_add_hcd; } platform_set_drvdata(pdev, s5p_ehci); return 0; +fail_add_hcd: + s5p_ehci_phy_disable(s5p_ehci); fail_io: clk_disable_unprepare(s5p_ehci-clk); fail_clk: @@ -192,14 +228,12 @@ fail_clk: static int s5p_ehci_remove(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); struct usb_hcd *hcd = s5p_ehci-hcd; usb_remove_hcd(hcd); - if (pdata pdata-phy_exit) - pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_disable(s5p_ehci); clk_disable_unprepare(s5p_ehci-clk); @@ -223,14 +257,11 @@ static int s5p_ehci_suspend(struct device *dev) struct s5p_ehci_hcd *s5p_ehci = dev_get_drvdata(dev); struct usb_hcd *hcd = s5p_ehci-hcd; bool do_wakeup = device_may_wakeup(dev); -
Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
CC: Doug Anderson. On Tue, Dec 18, 2012 at 8:13 PM, Vivek Gautam gautam.vi...@samsung.com wrote: Adding the phy driver to ehci-s5p. Keeping the platform data for continuing the smooth operation for boards which still uses it Signed-off-by: Vivek Gautam gautam.vi...@samsung.com Acked-by: Jingoo Han jg1@samsung.com --- drivers/usb/host/ehci-s5p.c | 70 ++- 1 files changed, 49 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c index 46ca5ef..50c93af 100644 --- a/drivers/usb/host/ehci-s5p.c +++ b/drivers/usb/host/ehci-s5p.c @@ -17,6 +17,7 @@ #include linux/platform_device.h #include linux/of_gpio.h #include linux/platform_data/usb-ehci-s5p.h +#include linux/usb/phy.h #include linux/usb/samsung_usb_phy.h #include plat/usb-phy.h @@ -33,6 +34,8 @@ struct s5p_ehci_hcd { struct device *dev; struct usb_hcd *hcd; struct clk *clk; + struct usb_phy *phy; + struct s5p_ehci_platdata *pdata; }; static const struct hc_driver s5p_ehci_hc_driver = { @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = { .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, }; +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); + usb_phy_init(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_init) { + s5p_ehci-pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + } +} + +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci) +{ + struct platform_device *pdev = to_platform_device(s5p_ehci-dev); + + if (s5p_ehci-phy) { + samsung_usbphy_set_type(s5p_ehci-phy, USB_PHY_TYPE_HOST); + usb_phy_shutdown(s5p_ehci-phy); + } else if (s5p_ehci-pdata-phy_exit) { + s5p_ehci-pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + } +} + static void s5p_setup_vbus_gpio(struct platform_device *pdev) { int err; @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32); static int s5p_ehci_probe(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata; + struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; + struct usb_phy *phy; int irq; int err; - pdata = pdev-dev.platform_data; - if (!pdata) { - dev_err(pdev-dev, No platform data defined\n); - return -EINVAL; - } - /* * Right now device-tree probed devices don't get dma_mask set. * Since shared usb code relies on it, set it here for now. @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev) if (!s5p_ehci) return -ENOMEM; + phy = devm_usb_get_phy(pdev-dev, USB_PHY_TYPE_USB2); + if (IS_ERR_OR_NULL(phy)) { + /* Fallback to pdata */ + if (!pdata) { + dev_err(pdev-dev, no platform data or transceiver defined\n); + return -EPROBE_DEFER; + } else { + s5p_ehci-pdata = pdata; + } + } else { + s5p_ehci-phy = phy; + } + s5p_ehci-dev = pdev-dev; hcd = usb_create_hcd(s5p_ehci_hc_driver, pdev-dev, @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev) goto fail_io; } - if (pdata-phy_init) - pdata-phy_init(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_enable(s5p_ehci); ehci = hcd_to_ehci(hcd); ehci-caps = hcd-regs; @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev) err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) { dev_err(pdev-dev, Failed to add USB HCD\n); - goto fail_io; + goto fail_add_hcd; } platform_set_drvdata(pdev, s5p_ehci); return 0; +fail_add_hcd: + s5p_ehci_phy_disable(s5p_ehci); fail_io: clk_disable_unprepare(s5p_ehci-clk); fail_clk: @@ -192,14 +228,12 @@ fail_clk: static int s5p_ehci_remove(struct platform_device *pdev) { - struct s5p_ehci_platdata *pdata = pdev-dev.platform_data; struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev); struct usb_hcd *hcd = s5p_ehci-hcd; usb_remove_hcd(hcd); - if (pdata pdata-phy_exit) - pdata-phy_exit(pdev, USB_PHY_TYPE_HOST); + s5p_ehci_phy_disable(s5p_ehci); clk_disable_unprepare(s5p_ehci-clk); @@ -223,14