[RFC usb-next v2 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Chunfeng Yun]

On Sat, 2018-03-24 at 15:21 +0100, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 51 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..d1861c5a74de 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}

>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +43,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +53,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
BTW:

static int usb_phy_roothub_add_phy(struct device *dev, int index,
                   struct list_head *list)
{
    struct usb_phy_roothub *roothub_entry;
    struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node,
index);

    if (IS_ERR_OR_NULL(phy)) {
        if (!phy || PTR_ERR(phy) == -ENODEV)
            return 0;
        else
            return PTR_ERR(phy);
    }

changed as:
    if (IS_ERR(phy))
         return PTR_ERR(phy);

The checking of return code of devm_of_phy_get_by_index() called by
usb_phy_roothub_add_phy() need be fixed? because it should exist for
index @i.

>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +86,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);