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.
Thanks,
NeilBrown
---
NeilBrown (2):
usb: phy: Add interface to get phy give of device_node.
twl4030_charger: find associated phy by more reliable means.
drivers/power/twl4030_charger.c | 21 ++++----
drivers/usb/phy/phy.c | 97 ++++++++++++++++++++++++++++-----------
include/linux/usb/phy.h | 2 +
3 files changed, 81 insertions(+), 39 deletions(-)
--
Signature
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.
Signed-off-by: NeilBrown <[email protected]>
---
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,
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 <[email protected]>
---
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);
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
On Mon, Mar 23, 2015 at 09:52:48AM +1100, NeilBrown wrote:
> From: NeilBrown <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> ---
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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: NeilBrown <[email protected]>
> > ---
>
> Sebastian, can I get your Acked-by here ?
Acked-By: Sebastian Reichel <[email protected]>
-- Sebastian