Hi Alan, Thank you for the review. Please find my replies inline. > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] > Sent: Thursday, December 05, 2013 7:53 PM > > On Thu, 5 Dec 2013, Kamil Debski wrote: > > > Change the phy provider used from the old usb phy specific to a new > > one using the generic phy framework. > > > > Signed-off-by: Kamil Debski <k.debski@xxxxxxxxxxx> > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > > > --- a/drivers/usb/host/ehci-exynos.c > > +++ b/drivers/usb/host/ehci-exynos.c > > > @@ -42,10 +42,10 @@ > > static const char hcd_name[] = "ehci-exynos"; static struct > > hc_driver __read_mostly exynos_ehci_hc_driver; > > > > +#define PHY_NUMBER 3 > > struct exynos_ehci_hcd { > > struct clk *clk; > > - struct usb_phy *phy; > > - struct usb_otg *otg; > > Are you sure you want to remove that line? Yes, I am. The new generic phy interface does not have the otg field in it. > > + struct phy *phy[PHY_NUMBER]; > > }; > > > > #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd > > *)(hcd_to_ehci(hcd)->priv) > > > @@ -102,13 +132,24 @@ static int exynos_ehci_probe(struct > platform_device *pdev) > > "samsung,exynos5440-ehci")) > > goto skip_phy; > > > > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); > > - if (IS_ERR(phy)) { > > - usb_put_hcd(hcd); > > - dev_warn(&pdev->dev, "no platform data or transceiver > defined\n"); > > - return -EPROBE_DEFER; > > - } else { > > - exynos_ehci->phy = phy; > > + for_each_available_child_of_node(pdev->dev.of_node, child) { > > + err = of_property_read_u32(child, "reg", &phy_number); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to parse device tree\n"); > > + return err; > > + } > > + if (phy_number >= PHY_NUMBER) { > > + dev_err(&pdev->dev, "Failed to parse device tree - > number out of range\n"); > > + return -EINVAL; > > Do you need to call of_node_put(child) before each of these return > statements? You are right, thank you for spotting this. > > > + } > > + phy = devm_of_phy_get(&pdev->dev, child, 0); > > + of_node_put(child); > > + if (IS_ERR(phy)) { > > + dev_err(&pdev->dev, "Failed to get phy number %d", > > + phy_number); > > + return PTR_ERR(phy); > > + } > > + exynos_ehci->phy[phy_number] = phy; > > exynos_ehci->otg = phy->otg; > > Did you intend to remove this line? Above, you removed the > exynos_ehci->otg field. I can't see how this patch would ever compile > without an error. Yes, I had this in a separate fix patch which I forgot to squash. Sorry for this. > > } > > > > @@ -149,11 +190,11 @@ skip_phy: > > goto fail_io; > > } > > > > - if (exynos_ehci->otg) > > - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); > > - > > - if (exynos_ehci->phy) > > - usb_phy_init(exynos_ehci->phy); > > + err = exynos_phys_on(exynos_ehci->phy); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to enabled phys\n"); > > + goto fail_phys_on; > > Why add a new statement label? Just goto fail_io. To me it seemed better to add a new label. I will drop it and use goto fail_io, as you suggested. > > > + } > > > > ehci = hcd_to_ehci(hcd); > > ehci->caps = hcd->regs; > > @@ -172,8 +213,8 @@ skip_phy: > > return 0; > > > > fail_add_hcd: > > - if (exynos_ehci->phy) > > - usb_phy_shutdown(exynos_ehci->phy); > > + exynos_phys_off(exynos_ehci->phy); > > +fail_phys_on: > > fail_io: > > clk_disable_unprepare(exynos_ehci->clk); > > fail_clk: > > Alan Stern Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
