[PATCH] usb: dwc3: Fix the error paths in usb3-phy PHY configuration

[Michal Simek]

+Jan

On 5/18/22 12:18, Aswath Govindraju wrote:
> [CAUTION: External Email]
> 
> generic_phy_power_off(), needs to be called in dwc3_glue_remove() while
> exiting as we powering on the phy in the dwc3_glue_probe(). Therefore,
> instantiate struct phy in dwc3_glue_data to use in dwc3_glue_probe() as
> well as dwc3_glue_remove().
> 
> In cases where "usb3-phy" is not present in the phy-names property,
> generic_phy_get_by_name() returns -ENODATA. Therefore, add condition to not
> return when the error code is -ENODATA too.
> 
> Also, generic_phy_init() and generic_phy_power_on() functions, have checks
> to verify if the struct phy argument passed is valid. Therefore, remove
> additional checks added for these in the dwc3_glue_probe().
> 
> Fixes: 142d50fbce7c ("usb: dwc3: Add support for usb3-phy PHY configuration")
> Signed-off-by: Aswath Govindraju <a-govindraju at ti.com>
> ---
>   drivers/usb/dwc3/dwc3-generic.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c
> index 6e1a1d066b40..f74d710a2447 100644
> --- a/drivers/usb/dwc3/dwc3-generic.c
> +++ b/drivers/usb/dwc3/dwc3-generic.c
> @@ -30,6 +30,7 @@
>   struct dwc3_glue_data {
>          struct clk_bulk         clks;
>          struct reset_ctl_bulk   resets;
> +       struct phy phy;
>          fdt_addr_t regs;
>   };
> 
> @@ -458,21 +459,21 @@ static int dwc3_glue_probe(struct udevice *dev)
>   {
>          struct dwc3_glue_ops *ops = (struct dwc3_glue_ops *)dev_get_driver_data(dev);
>          struct dwc3_glue_data *glue = dev_get_plat(dev);
> +       struct phy *phy = &glue->phy;
>          struct udevice *child = NULL;
>          int index = 0;
>          int ret;
> -       struct phy phy;
> 
> -       ret = generic_phy_get_by_name(dev, "usb3-phy", &phy);
> -       if (!ret) {
> -               ret = generic_phy_init(&phy);
> -               if (ret)
> -                       return ret;
> -       } else if (ret != -ENOENT) {
> +       ret = generic_phy_get_by_name(dev, "usb3-phy", phy);
> +       if (ret && ret != -ENOENT && ret != -ENODATA) {
>                  debug("could not get phy (err %d)\n", ret);
>                  return ret;
>          }
> 
> +       ret = generic_phy_init(phy);
> +       if (ret)
> +               return ret;
> +
>          glue->regs = dev_read_addr(dev);
> 
>          ret = dwc3_glue_clk_init(dev, glue);
> @@ -483,11 +484,9 @@ static int dwc3_glue_probe(struct udevice *dev)
>          if (ret)
>                  return ret;
> 
> -       if (phy.dev) {
> -               ret = generic_phy_power_on(&phy);
> -               if (ret)
> -                       return ret;
> -       }
> +       ret = generic_phy_power_on(phy);
> +       if (ret)
> +               return ret;
> 
>          ret = device_find_first_child(dev, &child);
>          if (ret)
> @@ -516,6 +515,8 @@ static int dwc3_glue_remove(struct udevice *dev)
>   {
>          struct dwc3_glue_data *glue = dev_get_plat(dev);
> 
> +       generic_phy_power_off(&glue->phy);
> +
>          reset_release_bulk(&glue->resets);
> 
>          clk_release_bulk(&glue->clks);
> --
> 2.17.1
> 

Jan sent one patch for this to handle ENODATA case. You don't need to fill NULL 
phy.dev to NULL because in your case it should be NUll already.
https://lore.kernel.org/all/c5a71c30-e55d-c8ab-d372-e5aaa859cf2a@siemens.com/

That's why I am fine with this patch too.

Acked-by: Michal Simek <michal.simek at amd.com>
Tested-by: Michal Simek <michal.simek at amd.com>

Thanks,
Michal