Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250

On 11/07/2012 02:35 PM, Vivek Gautam wrote:

quoted

quoted

@@ -180,10 +273,12 @@ enum samsung_cpu_type {
   /*
    * struct samsung_usbphy - transceiver driver state
    * @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
    * @plat: platform data
    * @dev: The parent device supplied to the probe function
    * @clk: usb phy clock
    * @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
    * @ref_clk_freq: reference clock frequency selection
    * @cpu_type: machine identifier
    * @phy_type: It keeps track of the PHY type.

@@ -191,10 +286,12 @@ enum samsung_cpu_type {
    */
   struct samsung_usbphy {
         struct usb_phy  phy;
+       struct usb_phy  phy3;
         struct samsung_usbphy_data *plat;
         struct device   *dev;
         struct clk      *clk;
         void __iomem    *regs;
+       void __iomem    *regs_phy3;

Wouldn't it be better to create a new data structure for USB 3.0
and embedding it here, rather than adding multiple fields with "3"
suffix ? E.g.

         struct {
                 void __iomem    *phy_regs;
                 struct usb_phy  phy;
         } usb3;
?

Yes, thanks for suggesting. This way things will look clearer.
Will update this.

quoted

And why do you need to duplicate those fields in first place ?
I guess phy and phy3 are dependant and you can't register 2 PHYs
separately ?

Controllers like DWC3 needs two different PHYs. One of USB2 type and
one of USB3 type. So we needed to register two separate PHYs.

OK, I was just wondering if there is any dependency between those two phys
so you need to aggregate them in one struct samsung_usbphy, rather than
creating two separate struct samsung_usbphy objects for them.

quoted

quoted

+/*
+ * The function passed to the usb driver for phy initialization
+ */
   static int samsung_usbphy_init(struct usb_phy *phy)
   {
         struct samsung_usbphy *sphy;

@@ -570,6 +872,8 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
         struct device *dev =&pdev->dev;

         struct resource *phy_mem;
         void __iomem    *phy_base;
+       struct resource *phy3_mem;
+       void __iomem    *phy3_base = NULL;
         struct clk *clk;
         int     ret = 0;

@@ -618,7 +922,38 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)

         sphy->clk = clk;

+       if (sphy->cpu_type == TYPE_EXYNOS5250) {
+               phy3_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+               if (!phy3_mem) {
+                       dev_err(dev, "%s: missing mem resource\n", __func__);
+                       return -ENODEV;
+               }
+
+               phy3_base = devm_request_and_ioremap(dev, phy3_mem);
+               if (!phy3_base) {
+                       dev_err(dev, "%s: register mapping failed\n", __func__);
+                       return -ENXIO;
+               }
+       }
+
+       sphy->regs_phy3         = phy3_base;
+       sphy->phy3.dev          = sphy->dev;
+       sphy->phy3.label        = "samsung-usbphy3";
+       sphy->phy3.init         = samsung_usbphy3_init;
+       sphy->phy3.shutdown     = samsung_usbphy3_shutdown;
+
         ret = usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
+       if (ret)
+               return ret;
+
+       if (sphy->cpu_type != TYPE_EXYNOS5250) {
+               dev_warn(dev, "Not a valid cpu_type for USB 3.0\n");
+       } else {
+               ret = usb_add_phy(&sphy->phy3, USB_PHY_TYPE_USB3);
+               if (ret)
+                       return ret;

2 redundant lines here.

Will two returns under if return not error codes ? The last return
actually returns success.
If still it needs modification, will do that.

It's up to you how you structure it. The last return returns whatever
value ret has. I can't see what is an advantage of doing something like:

	if (ret)
		return ret;

	return ret;
--

Thanks,
Sylwester