linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Allow twl4030_charger to find phy reliably.
@ 2015-03-22 22:52 NeilBrown
  2015-03-22 22:52 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
  2015-03-22 22:52 ` [PATCH 1/2] usb: phy: Add interface to get phy give of device_node NeilBrown
  0 siblings, 2 replies; 8+ messages in thread
From: NeilBrown @ 2015-03-22 22:52 UTC (permalink / raw)
  To: Mark Rutland, Pawel Moll, Ian Campbell, Sebastian Reichel,
	Felipe Balbi, Rob Herring, Kumar Gala
  Cc: devicetree, linux-usb, linux-kernel, Pavel Machek, linux-pm

The twl4030_charger is physically paired with the twl4030 USB phy,
so the drivers need to be able to reliably find each other.

twl4030_charger currently uses usb_get_phy(), which works if there is
only one phy to choose from, but is not reliable in more complex
configurations.

These patches add a new interface to allow a phy to be found given a
device node, and then use that interface in twl4030_charger so that
it finds its sibling in the devicetree, and gets the phy associated
with that.

===

This is a resend with improved documentation.  I'm hoping this can go
in to the next merge window, though should it go through usb/phy or
power???

Thanks,
NeilBrown



---

NeilBrown (2):
      usb: phy: Add interface to get phy give of device_node.
      twl4030_charger: find associated phy by more reliable means.


 .../devicetree/bindings/power/twl-charger.txt      |   10 ++
 .../devicetree/bindings/usb/twlxxxx-usb.txt        |    3 +
 drivers/power/twl4030_charger.c                    |   21 ++--
 drivers/usb/phy/phy.c                              |   97 ++++++++++++++------
 include/linux/usb/phy.h                            |    2 
 5 files changed, 94 insertions(+), 39 deletions(-)

--
Signature


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] usb: phy: Add interface to get phy give of device_node.
  2015-03-22 22:52 [PATCH 0/2] Allow twl4030_charger to find phy reliably NeilBrown
  2015-03-22 22:52 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
@ 2015-03-22 22:52 ` NeilBrown
  1 sibling, 0 replies; 8+ messages in thread
From: NeilBrown @ 2015-03-22 22:52 UTC (permalink / raw)
  To: Mark Rutland, Pawel Moll, Ian Campbell, Sebastian Reichel,
	Felipe Balbi, Rob Herring, Kumar Gala
  Cc: devicetree, linux-pm, NeilBrown, linux-usb, linux-kernel, Pavel Machek

From: NeilBrown <neilb@suse.de>

Split the "get phy from device_node" functionality out of
"get phy by phandle" so it can be used directly.

This is useful when a battery-charger is intimately associated with a
particular phy but handled by a separate driver.  The charger
can find the device_node based on sibling relationships
without the need for a redundant declaration in the devicetree
description.

As a peripheral that gets a phy will often want to register a
notifier block, and de-register it later, that functionality
is included so the de-registration is automatic.

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/usb/phy/phy.c   |   97 ++++++++++++++++++++++++++++++++++-------------
 include/linux/usb/phy.h |    2 +
 2 files changed, 72 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 2f9735b35338..6b089e19e8db 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -22,6 +22,11 @@ static LIST_HEAD(phy_list);
 static LIST_HEAD(phy_bind_list);
 static DEFINE_SPINLOCK(phy_lock);
 
+struct phy_devm {
+	struct usb_phy *phy;
+	struct notifier_block *nb;
+};
+
 static struct usb_phy *__usb_find_phy(struct list_head *list,
 	enum usb_phy_type type)
 {
@@ -79,6 +84,15 @@ static void devm_usb_phy_release(struct device *dev, void *res)
 	usb_put_phy(phy);
 }
 
+static void devm_usb_phy_release2(struct device *dev, void *_res)
+{
+	struct phy_devm *res = _res;
+
+	if (res->nb)
+		usb_unregister_notifier(res->phy, res->nb);
+	usb_put_phy(res->phy);
+}
+
 static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
 {
 	return res == match_data;
@@ -151,40 +165,30 @@ err0:
 EXPORT_SYMBOL_GPL(usb_get_phy);
 
 /**
- * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * devm_usb_get_phy_by_node - find the USB PHY by device_node
  * @dev - device that requests this phy
- * @phandle - name of the property holding the phy phandle value
- * @index - the index of the phy
+ * @node - the device_node for the phy device.
+ * @nb - a notifier_block to register with the phy.
  *
- * Returns the phy driver associated with the given phandle value,
+ * Returns the phy driver associated with the given device_node,
  * after getting a refcount to it, -ENODEV if there is no such phy or
- * -EPROBE_DEFER if there is a phandle to the phy, but the device is
- * not yet loaded. While at that, it also associates the device with
+ * -EPROBE_DEFER if the device is not yet loaded. While at that, it
+ * also associates the device with
  * the phy using devres. On driver detach, release function is invoked
  * on the devres data, then, devres data is freed.
  *
- * For use by USB host and peripheral drivers.
+ * For use by peripheral drivers for devices related to a phy,
+ * such as a charger.
  */
-struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
-	const char *phandle, u8 index)
+struct  usb_phy *devm_usb_get_phy_by_node(struct device *dev,
+					  struct device_node *node,
+					  struct notifier_block *nb)
 {
-	struct usb_phy	*phy = ERR_PTR(-ENOMEM), **ptr;
+	struct usb_phy	*phy = ERR_PTR(-ENOMEM);
+	struct phy_devm	*ptr;
 	unsigned long	flags;
-	struct device_node *node;
 
-	if (!dev->of_node) {
-		dev_dbg(dev, "device does not have a device node entry\n");
-		return ERR_PTR(-EINVAL);
-	}
-
-	node = of_parse_phandle(dev->of_node, phandle, index);
-	if (!node) {
-		dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
-			dev->of_node->full_name);
-		return ERR_PTR(-ENODEV);
-	}
-
-	ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
+	ptr = devres_alloc(devm_usb_phy_release2, sizeof(*ptr), GFP_KERNEL);
 	if (!ptr) {
 		dev_dbg(dev, "failed to allocate memory for devres\n");
 		goto err0;
@@ -203,8 +207,10 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 		devres_free(ptr);
 		goto err1;
 	}
-
-	*ptr = phy;
+	if (nb)
+		usb_register_notifier(phy, nb);
+	ptr->phy = phy;
+	ptr->nb = nb;
 	devres_add(dev, ptr);
 
 	get_device(phy->dev);
@@ -213,10 +219,47 @@ err1:
 	spin_unlock_irqrestore(&phy_lock, flags);
 
 err0:
-	of_node_put(node);
 
 	return phy;
 }
+EXPORT_SYMBOL_GPL(devm_usb_get_phy_by_node);
+
+/**
+ * devm_usb_get_phy_by_phandle - find the USB PHY by phandle
+ * @dev - device that requests this phy
+ * @phandle - name of the property holding the phy phandle value
+ * @index - the index of the phy
+ *
+ * Returns the phy driver associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such phy or
+ * -EPROBE_DEFER if there is a phandle to the phy, but the device is
+ * not yet loaded. While at that, it also associates the device with
+ * the phy using devres. On driver detach, release function is invoked
+ * on the devres data, then, devres data is freed.
+ *
+ * For use by USB host and peripheral drivers.
+ */
+struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
+	const char *phandle, u8 index)
+{
+	struct device_node *node;
+	struct usb_phy	*phy;
+
+	if (!dev->of_node) {
+		dev_dbg(dev, "device does not have a device node entry\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	node = of_parse_phandle(dev->of_node, phandle, index);
+	if (!node) {
+		dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
+			dev->of_node->full_name);
+		return ERR_PTR(-ENODEV);
+	}
+	phy = devm_usb_get_phy_by_node(dev, node, NULL);
+	of_node_put(node);
+	return phy;
+}
 EXPORT_SYMBOL_GPL(devm_usb_get_phy_by_phandle);
 
 /**
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index bc91b5d380fd..8ed1e29ef329 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -205,6 +205,8 @@ extern struct usb_phy *usb_get_phy_dev(struct device *dev, u8 index);
 extern struct usb_phy *devm_usb_get_phy_dev(struct device *dev, u8 index);
 extern struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 	const char *phandle, u8 index);
+extern struct usb_phy *devm_usb_get_phy_by_node(struct device *dev,
+	struct device_node *node, struct notifier_block *nb);
 extern void usb_put_phy(struct usb_phy *);
 extern void devm_usb_put_phy(struct device *dev, struct usb_phy *x);
 extern int usb_bind_phy(const char *dev_name, u8 index,



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-03-22 22:52 [PATCH 0/2] Allow twl4030_charger to find phy reliably NeilBrown
@ 2015-03-22 22:52 ` NeilBrown
  2015-05-08 16:12   ` Felipe Balbi
  2015-03-22 22:52 ` [PATCH 1/2] usb: phy: Add interface to get phy give of device_node NeilBrown
  1 sibling, 1 reply; 8+ messages in thread
From: NeilBrown @ 2015-03-22 22:52 UTC (permalink / raw)
  To: Mark Rutland, Pawel Moll, Ian Campbell, Sebastian Reichel,
	Felipe Balbi, Rob Herring, Kumar Gala
  Cc: devicetree, linux-pm, NeilBrown, linux-usb, linux-kernel, Pavel Machek

From: NeilBrown <neilb@suse.de>

twl4030_charger currently finds the associated phy
using usb_get_phy() which will return the first USB2 phy.
If your platform has multiple such phys (as mine does),
this is not reliable (and reliably fails on the GTA04).

Change to use devm_usb_get_phy_by_node(), having found the
node by looking for an appropriately named sibling in
device-tree.

This makes usb-charging dependent on correct device-tree
configuration.

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../devicetree/bindings/power/twl-charger.txt      |   10 ++++++++++
 .../devicetree/bindings/usb/twlxxxx-usb.txt        |    3 +++
 drivers/power/twl4030_charger.c                    |   21 +++++++++-----------
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/twl-charger.txt b/Documentation/devicetree/bindings/power/twl-charger.txt
index d5c706216df5..3b4ea1b73b38 100644
--- a/Documentation/devicetree/bindings/power/twl-charger.txt
+++ b/Documentation/devicetree/bindings/power/twl-charger.txt
@@ -1,5 +1,15 @@
 TWL BCI (Battery Charger Interface)
 
+The battery charger needs to interact with the USB phy in order
+to know when charging is permissible, and when there is a connection
+or disconnection.
+
+The choice of phy cannot be configured at a hardware level, so there
+is no value in explicit configuration in device-tree.  Rather
+if there is a sibling of the BCI node which is compatible with
+"ti,twl4030-usb", then that is used to determine when and how
+use USB power for charging.
+
 Required properties:
 - compatible:
   - "ti,twl4030-bci"
diff --git a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
index 0aee0ad3f035..17327a296110 100644
--- a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
@@ -30,6 +30,9 @@ TWL4030 USB PHY AND COMPARATOR
  - usb_mode : The mode used by the phy to connect to the controller. "1"
    specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
 
+If a sibling node is compatible "ti,twl4030-bci", then it will find
+this device and query it for USB power status.
+
 twl4030-usb {
 	compatible = "ti,twl4030-usb";
 	interrupts = < 10 4 >;
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index d35b83e635b5..b07f4e2f2dde 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -629,10 +629,15 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
 
-	bci->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
-		usb_register_notifier(bci->transceiver, &bci->usb_nb);
+	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
+	if (bci->dev->of_node) {
+		struct device_node *phynode;
+
+		phynode = of_find_compatible_node(bci->dev->of_node->parent,
+						  NULL, "ti,twl4030-usb");
+		if (phynode)
+			bci->transceiver = devm_usb_get_phy_by_node(
+				bci->dev, phynode, &bci->usb_nb);
 	}
 
 	/* Enable interrupts now. */
@@ -662,10 +667,6 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 	return 0;
 
 fail_unmask_interrupts:
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
-		usb_put_phy(bci->transceiver);
-	}
 	free_irq(bci->irq_bci, bci);
 fail_bci_irq:
 	free_irq(bci->irq_chg, bci);
@@ -694,10 +695,6 @@ static int __exit twl4030_bci_remove(struct platform_device *pdev)
 	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
 			 TWL4030_INTERRUPTS_BCIIMR2A);
 
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
-		usb_put_phy(bci->transceiver);
-	}
 	free_irq(bci->irq_bci, bci);
 	free_irq(bci->irq_chg, bci);
 	power_supply_unregister(&bci->usb);



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-03-22 22:52 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
@ 2015-05-08 16:12   ` Felipe Balbi
  2015-05-17  9:56     ` Sebastian Reichel
  0 siblings, 1 reply; 8+ messages in thread
From: Felipe Balbi @ 2015-05-08 16:12 UTC (permalink / raw)
  To: NeilBrown
  Cc: Mark Rutland, Pawel Moll, Ian Campbell, Sebastian Reichel,
	Felipe Balbi, Rob Herring, Kumar Gala, devicetree, linux-pm,
	NeilBrown, linux-usb, linux-kernel, Pavel Machek

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

On Mon, Mar 23, 2015 at 09:52:48AM +1100, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
> 
> twl4030_charger currently finds the associated phy
> using usb_get_phy() which will return the first USB2 phy.
> If your platform has multiple such phys (as mine does),
> this is not reliable (and reliably fails on the GTA04).
> 
> Change to use devm_usb_get_phy_by_node(), having found the
> node by looking for an appropriately named sibling in
> device-tree.
> 
> This makes usb-charging dependent on correct device-tree
> configuration.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---

Sebastian, can I get your Acked-by here ?

>  .../devicetree/bindings/power/twl-charger.txt      |   10 ++++++++++
>  .../devicetree/bindings/usb/twlxxxx-usb.txt        |    3 +++
>  drivers/power/twl4030_charger.c                    |   21 +++++++++-----------
>  3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/twl-charger.txt b/Documentation/devicetree/bindings/power/twl-charger.txt
> index d5c706216df5..3b4ea1b73b38 100644
> --- a/Documentation/devicetree/bindings/power/twl-charger.txt
> +++ b/Documentation/devicetree/bindings/power/twl-charger.txt
> @@ -1,5 +1,15 @@
>  TWL BCI (Battery Charger Interface)
>  
> +The battery charger needs to interact with the USB phy in order
> +to know when charging is permissible, and when there is a connection
> +or disconnection.
> +
> +The choice of phy cannot be configured at a hardware level, so there
> +is no value in explicit configuration in device-tree.  Rather
> +if there is a sibling of the BCI node which is compatible with
> +"ti,twl4030-usb", then that is used to determine when and how
> +use USB power for charging.
> +
>  Required properties:
>  - compatible:
>    - "ti,twl4030-bci"
> diff --git a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
> index 0aee0ad3f035..17327a296110 100644
> --- a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
> @@ -30,6 +30,9 @@ TWL4030 USB PHY AND COMPARATOR
>   - usb_mode : The mode used by the phy to connect to the controller. "1"
>     specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
>  
> +If a sibling node is compatible "ti,twl4030-bci", then it will find
> +this device and query it for USB power status.
> +
>  twl4030-usb {
>  	compatible = "ti,twl4030-usb";
>  	interrupts = < 10 4 >;
> diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> index d35b83e635b5..b07f4e2f2dde 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -629,10 +629,15 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
>  
>  	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>  
> -	bci->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
> -	if (!IS_ERR_OR_NULL(bci->transceiver)) {
> -		bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
> -		usb_register_notifier(bci->transceiver, &bci->usb_nb);
> +	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
> +	if (bci->dev->of_node) {
> +		struct device_node *phynode;
> +
> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> +						  NULL, "ti,twl4030-usb");
> +		if (phynode)
> +			bci->transceiver = devm_usb_get_phy_by_node(
> +				bci->dev, phynode, &bci->usb_nb);
>  	}
>  
>  	/* Enable interrupts now. */
> @@ -662,10 +667,6 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_unmask_interrupts:
> -	if (!IS_ERR_OR_NULL(bci->transceiver)) {
> -		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
> -		usb_put_phy(bci->transceiver);
> -	}
>  	free_irq(bci->irq_bci, bci);
>  fail_bci_irq:
>  	free_irq(bci->irq_chg, bci);
> @@ -694,10 +695,6 @@ static int __exit twl4030_bci_remove(struct platform_device *pdev)
>  	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
>  			 TWL4030_INTERRUPTS_BCIIMR2A);
>  
> -	if (!IS_ERR_OR_NULL(bci->transceiver)) {
> -		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
> -		usb_put_phy(bci->transceiver);
> -	}
>  	free_irq(bci->irq_bci, bci);
>  	free_irq(bci->irq_chg, bci);
>  	power_supply_unregister(&bci->usb);
> 
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-05-08 16:12   ` Felipe Balbi
@ 2015-05-17  9:56     ` Sebastian Reichel
  0 siblings, 0 replies; 8+ messages in thread
From: Sebastian Reichel @ 2015-05-17  9:56 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: NeilBrown, Mark Rutland, Pawel Moll, Ian Campbell, Rob Herring,
	Kumar Gala, devicetree, linux-pm, NeilBrown, linux-usb,
	linux-kernel, Pavel Machek

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

Hi,

On Fri, May 08, 2015 at 11:12:05AM -0500, Felipe Balbi wrote:
> On Mon, Mar 23, 2015 at 09:52:48AM +1100, NeilBrown wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > twl4030_charger currently finds the associated phy
> > using usb_get_phy() which will return the first USB2 phy.
> > If your platform has multiple such phys (as mine does),
> > this is not reliable (and reliably fails on the GTA04).
> > 
> > Change to use devm_usb_get_phy_by_node(), having found the
> > node by looking for an appropriately named sibling in
> > device-tree.
> > 
> > This makes usb-charging dependent on correct device-tree
> > configuration.
> > 
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> 
> Sebastian, can I get your Acked-by here ?

Acked-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-02-25 21:13   ` Sebastian Reichel
@ 2015-03-04  9:14     ` NeilBrown
  0 siblings, 0 replies; 8+ messages in thread
From: NeilBrown @ 2015-03-04  9:14 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Dmitry Eremin-Solenikov, Greg Kroah-Hartman, David Woodhouse,
	Felipe Balbi, GTA04 owners, linux-usb, linux-kernel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

On Wed, 25 Feb 2015 22:13:51 +0100 Sebastian Reichel <sre@kernel.org> wrote:

> Hi Neil,
> 
> On Tue, Feb 24, 2015 at 03:01:29PM +1100, NeilBrown wrote:
> > twl4030_charger currently finds the associated phy
> > using usb_get_phy() which will return the first USB2 phy.
> > If your platform has multiple such phys (as mine does),
> > this is not reliable (and reliably fails on the GTA04).
> > 
> > Change to use devm_usb_get_phy_by_node(), having found the
> > node by looking for an appropriately named sibling in
> > device-tree.
> > 
> > This makes usb-charging dependent on correct device-tree
> > configuration.
> 
> The patch looks ok to me, but you should update the DT documentation
> in Documentation/devicetree/bindings/power/twl-charger.txt regarding
> the sibling dependency.
> 
> Apart from that DT binding maintainers should be CC'd.
> 
> -- Sebastian

Thanks.
I've added the following.  I've also changed the code to use
of_find_compatible_node() and find the USB phy based on 'compatible' rather
than on the node name.

I'll Cc DT maintainers when I resubmit.

Thanks,
NeilBrown

diff --git a/Documentation/devicetree/bindings/power/twl-charger.txt b/Documentation/devicetree/bindings/power/twl-charger.txt
index d5c706216df5..3b4ea1b73b38 100644
--- a/Documentation/devicetree/bindings/power/twl-charger.txt
+++ b/Documentation/devicetree/bindings/power/twl-charger.txt
@@ -1,5 +1,15 @@
 TWL BCI (Battery Charger Interface)
 
+The battery charger needs to interact with the USB phy in order
+to know when charging is permissible, and when there is a connection
+or disconnection.
+
+The choice of phy cannot be configured at a hardware level, so there
+is no value in explicit configuration in device-tree.  Rather
+if there is a sibling of the BCI node which is compatible with
+"ti,twl4030-usb", then that is used to determine when and how
+use USB power for charging.
+
 Required properties:
 - compatible:
   - "ti,twl4030-bci"
diff --git a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
index 0aee0ad3f035..17327a296110 100644
--- a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
@@ -30,6 +30,9 @@ TWL4030 USB PHY AND COMPARATOR
  - usb_mode : The mode used by the phy to connect to the controller. "1"
    specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
 
+If a sibling node is compatible "ti,twl4030-bci", then it will find
+this device and query it for USB power status.
+
 twl4030-usb {
 	compatible = "ti,twl4030-usb";
 	interrupts = < 10 4 >;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-02-24  4:01 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
@ 2015-02-25 21:13   ` Sebastian Reichel
  2015-03-04  9:14     ` NeilBrown
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Reichel @ 2015-02-25 21:13 UTC (permalink / raw)
  To: NeilBrown
  Cc: Dmitry Eremin-Solenikov, Greg Kroah-Hartman, David Woodhouse,
	Felipe Balbi, GTA04 owners, linux-usb, linux-kernel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

Hi Neil,

On Tue, Feb 24, 2015 at 03:01:29PM +1100, NeilBrown wrote:
> twl4030_charger currently finds the associated phy
> using usb_get_phy() which will return the first USB2 phy.
> If your platform has multiple such phys (as mine does),
> this is not reliable (and reliably fails on the GTA04).
> 
> Change to use devm_usb_get_phy_by_node(), having found the
> node by looking for an appropriately named sibling in
> device-tree.
> 
> This makes usb-charging dependent on correct device-tree
> configuration.

The patch looks ok to me, but you should update the DT documentation
in Documentation/devicetree/bindings/power/twl-charger.txt regarding
the sibling dependency.

Apart from that DT binding maintainers should be CC'd.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] twl4030_charger: find associated phy by more reliable means.
  2015-02-24  4:01 [PATCH 0/2] Allow twl4030_charger to find phy reliably NeilBrown
@ 2015-02-24  4:01 ` NeilBrown
  2015-02-25 21:13   ` Sebastian Reichel
  0 siblings, 1 reply; 8+ messages in thread
From: NeilBrown @ 2015-02-24  4:01 UTC (permalink / raw)
  To: Dmitry Eremin-Solenikov, Greg Kroah-Hartman, David Woodhouse,
	Sebastian Reichel, Felipe Balbi
  Cc: GTA04 owners, linux-usb, linux-kernel, linux-pm

twl4030_charger currently finds the associated phy
using usb_get_phy() which will return the first USB2 phy.
If your platform has multiple such phys (as mine does),
this is not reliable (and reliably fails on the GTA04).

Change to use devm_usb_get_phy_by_node(), having found the
node by looking for an appropriately named sibling in
device-tree.

This makes usb-charging dependent on correct device-tree
configuration.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/power/twl4030_charger.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index d35b83e635b5..4cf5ffbc904a 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -629,10 +629,15 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
 
-	bci->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
-		usb_register_notifier(bci->transceiver, &bci->usb_nb);
+	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
+	if (bci->dev->of_node) {
+		struct device_node *phynode;
+
+		phynode = of_get_child_by_name(bci->dev->of_node->parent,
+					       "twl4030-usb");
+		if (phynode)
+			bci->transceiver = devm_usb_get_phy_by_node(
+				bci->dev, phynode, &bci->usb_nb);
 	}
 
 	/* Enable interrupts now. */
@@ -662,10 +667,6 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 	return 0;
 
 fail_unmask_interrupts:
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
-		usb_put_phy(bci->transceiver);
-	}
 	free_irq(bci->irq_bci, bci);
 fail_bci_irq:
 	free_irq(bci->irq_chg, bci);
@@ -694,10 +695,6 @@ static int __exit twl4030_bci_remove(struct platform_device *pdev)
 	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
 			 TWL4030_INTERRUPTS_BCIIMR2A);
 
-	if (!IS_ERR_OR_NULL(bci->transceiver)) {
-		usb_unregister_notifier(bci->transceiver, &bci->usb_nb);
-		usb_put_phy(bci->transceiver);
-	}
 	free_irq(bci->irq_bci, bci);
 	free_irq(bci->irq_chg, bci);
 	power_supply_unregister(&bci->usb);



^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-05-17  9:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-22 22:52 [PATCH 0/2] Allow twl4030_charger to find phy reliably NeilBrown
2015-03-22 22:52 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
2015-05-08 16:12   ` Felipe Balbi
2015-05-17  9:56     ` Sebastian Reichel
2015-03-22 22:52 ` [PATCH 1/2] usb: phy: Add interface to get phy give of device_node NeilBrown
  -- strict thread matches above, loose matches on Subject: below --
2015-02-24  4:01 [PATCH 0/2] Allow twl4030_charger to find phy reliably NeilBrown
2015-02-24  4:01 ` [PATCH 2/2] twl4030_charger: find associated phy by more reliable means NeilBrown
2015-02-25 21:13   ` Sebastian Reichel
2015-03-04  9:14     ` NeilBrown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).