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

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

On 11/07/2012 02:35 PM, Vivek Gautam wrote:
>>> @@ -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.
>
>> 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.

>>> +/*
>>> + * 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