* [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc}
@ 2018-03-24 14:21 Martin Blumenstingl
0 siblings, 0 replies; 5+ messages in thread
From: Martin Blumenstingl @ 2018-03-24 14:21 UTC (permalink / raw)
To: linux-usb
Cc: gregkh, matthias.bgg, stern, rogerq, linux-mediatek,
linux-amlogic, j-keerthy, d-gerlach, kishon, chunfeng.yun,
Martin Blumenstingl
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@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);
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);
^ permalink raw reply related [flat|nested] 5+ messages in thread* [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc}
@ 2018-03-26 3:37 Chunfeng Yun
0 siblings, 0 replies; 5+ messages in thread
From: Chunfeng Yun @ 2018-03-26 3:37 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-usb, gregkh, matthias.bgg, stern, rogerq, linux-mediatek,
linux-amlogic, j-keerthy, d-gerlach, kishon
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@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);
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc}
@ 2018-03-26 9:57 Roger Quadros
0 siblings, 0 replies; 5+ messages in thread
From: Roger Quadros @ 2018-03-26 9:57 UTC (permalink / raw)
To: Martin Blumenstingl, linux-usb
Cc: gregkh, matthias.bgg, stern, linux-mediatek, linux-amlogic,
j-keerthy, d-gerlach, kishon, chunfeng.yun
On 24/03/18 16:21, 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@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;
>
We missed doing a
INIT_LIST_HEAD(&roothub_entry->list);
> 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);
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc}
@ 2018-03-26 20:31 Martin Blumenstingl
0 siblings, 0 replies; 5+ messages in thread
From: Martin Blumenstingl @ 2018-03-26 20:31 UTC (permalink / raw)
To: Chunfeng Yun
Cc: linux-usb, gregkh, matthias.bgg, stern, rogerq, linux-mediatek,
linux-amlogic, j-keerthy, d-gerlach, kishon
Hi Chunfeng,
On Mon, Mar 26, 2018 at 5:37 AM, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> 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@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.
good catch - thank you!
I think (but I don't remember exactly) my idea was to catch the case
where CONFIG_GENERIC_PHY was disabled. however, in that case the PHY
framework returns ERR_PTR(-ENOSYS);
let me know if I should send a patch for this issue or if you want to
send a patch yourself
however, I'd suggest waiting until this RFC is finished/merged because
this issue already exist in the code before this series
>> 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);
>
>
Regards
Martin
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc}
@ 2018-03-27 0:50 Chunfeng Yun
0 siblings, 0 replies; 5+ messages in thread
From: Chunfeng Yun @ 2018-03-27 0:50 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-usb, gregkh, matthias.bgg, stern, rogerq, linux-mediatek,
linux-amlogic, j-keerthy, d-gerlach, kishon
On Mon, 2018-03-26 at 22:31 +0200, Martin Blumenstingl wrote:
> Hi Chunfeng,
>
> On Mon, Mar 26, 2018 at 5:37 AM, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > 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@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.
> good catch - thank you!
> I think (but I don't remember exactly) my idea was to catch the case
> where CONFIG_GENERIC_PHY was disabled. however, in that case the PHY
> framework returns ERR_PTR(-ENOSYS);
In the earlier patch, phy is got by name for each port, and may not
exist.
>
> let me know if I should send a patch for this issue or if you want to
> send a patch yourself
> however, I'd suggest waiting until this RFC is finished/merged because
> this issue already exist in the code before this series
Ok, I'll send a patch if need after this RFC is merged
>
> >> 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);
> >
> >
>
>
> Regards
> Martin
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-27 0:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-24 14:21 [RFC,usb-next,v2,1/2] usb: core: split usb_phy_roothub_{init,alloc} Martin Blumenstingl
-- strict thread matches above, loose matches on Subject: below --
2018-03-26 3:37 Chunfeng Yun
2018-03-26 9:57 Roger Quadros
2018-03-26 20:31 Martin Blumenstingl
2018-03-27 0:50 Chunfeng Yun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox