Enhancements to twl4030 phy to support better charging.

A construct like:

        if (pm_runtime_suspended(twl->dev))
               pm_runtime_get_sync(twl->dev);

is against the spirit of the runtime_pm interface as it makes the internal refcounting useless.

In this case it is also racy, particularly as 'put_autosuspend' is use to drop a reference.
When that happens a timer is started and the device is runtime-suspended after the timeout.
If the above code runs in this window, the device will not be found to be suspended so no pm_runtime reference is taken. When the timer expires the device will be suspended, which is against the intention of the code.

So be more direct is taking and dropping references. If twl->linkstat is VBUS_VALID or ID_GROUND, then hold a pm_runtime reference, otherwise don't.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/phy/phy-twl4030-usb.c +13 -7

index 8e87f54671f3..97c59074233f 100644
@@ -536,8 +536,13 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 
 	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
+		status_changed =
+			(twl->linkstat == OMAP_MUSB_VBUS_VALID ||
+			 twl->linkstat == OMAP_MUSB_ID_GROUND)
+			!=
+			(status == OMAP_MUSB_VBUS_VALID ||
+			 status == OMAP_MUSB_ID_GROUND);
 		twl->linkstat = status;
-		status_changed = true;
 	}
 	mutex_unlock(&twl->lock);
 

@@ -555,13 +560,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		 */
 		if ((status == OMAP_MUSB_VBUS_VALID) ||
 		    (status == OMAP_MUSB_ID_GROUND)) {
-			if (pm_runtime_suspended(twl->dev))
-				pm_runtime_get_sync(twl->dev);
+			pm_runtime_get_sync(twl->dev);
 		} else {
-			if (pm_runtime_active(twl->dev)) {
-				pm_runtime_mark_last_busy(twl->dev);
-				pm_runtime_put_autosuspend(twl->dev);
-			}
+			pm_runtime_mark_last_busy(twl->dev);
+			pm_runtime_put_autosuspend(twl->dev);
 		}
 		omap_musb_mailbox(status);
 	}

@@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev)
 
 	/* disable complete OTG block */
 	twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB);
+
+	if (twl->linkstat == OMAP_MUSB_VBUS_VALID ||
+	    twl->linkstat == OMAP_MUSB_ID_GROUND)
+		pm_runtime_put_noidle(twl->dev);
 	pm_runtime_mark_last_busy(twl->dev);
 	pm_runtime_put(twl->dev);
 

drivers/phy/phy-twl4030-usb.c +6 -0

+		status_changed =
+			(twl->linkstat == OMAP_MUSB_VBUS_VALID ||
+			 twl->linkstat == OMAP_MUSB_ID_GROUND)
+			!=
+			(status == OMAP_MUSB_VBUS_VALID ||
+			 status == OMAP_MUSB_ID_GROUND);
 		twl->linkstat = status;

Hi!

Quoted 7 lines

drivers/phy/phy-twl4030-usb.c +6 -0

+		status_changed =
+			(twl->linkstat == OMAP_MUSB_VBUS_VALID ||
+			 twl->linkstat == OMAP_MUSB_ID_GROUND)
+			!=
+			(status == OMAP_MUSB_VBUS_VALID ||
+			 status == OMAP_MUSB_ID_GROUND);
 		twl->linkstat = status;

...

drivers/phy/phy-twl4030-usb.c static int twl4030_usb_remove(struct platform_device *pdev) · line 777

Quoted 9 lines

drivers/phy/phy-twl4030-usb.c +4 -0

@@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev)
 
 	/* disable complete OTG block */
 	twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB);
+
+	if (twl->linkstat == OMAP_MUSB_VBUS_VALID ||
+	    twl->linkstat == OMAP_MUSB_ID_GROUND)
+		pm_runtime_put_noidle(twl->dev);
 	pm_runtime_mark_last_busy(twl->dev);

inline function returning (x == OMAP_MUSB_VBUS_VALID || x == OMAP_MUSB_ID_GROUND) would really help readability here.

Thanks,
Pavel

The charger needs to know when a USB gadget has been enumerated and what the agreed maximum current was so that it can adjust charging accordingly.

So define a "set_power()" function to record the permitted draw, and pass a pointer to that when sending USB_EVENT_ENUMERATED notification.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

drivers/phy/phy-twl4030-usb.c +0 -0

index 97c59074233f..023fe150c7a1 100644

drivers/phy/phy-twl4030-usb.c +4 -0

+	 * USB_EVENT_ENUMERATED
+	 */
+	unsigned		vbus_draw;
+
 	struct delayed_work	id_workaround_work;

Quoted 18 lines · NeilBrown

The charger needs to know when a USB gadget has been enumerated and what the agreed maximum current was so that it can adjust charging accordingly.

So define a "set_power()" function to record the permitted draw, and pass a pointer to that when sending USB_EVENT_ENUMERATED notification.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

drivers/phy/phy-twl4030-usb.c +0 -0

index 97c59074233f..023fe150c7a1 100644

drivers/phy/phy-twl4030-usb.c struct twl4030_usb { · line 166

Quoted 4 lines

drivers/phy/phy-twl4030-usb.c +1 -0

@@ -163,6 +163,11 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 
+	/* Permitted vbus draw - only meaningful after

add "in mA"?

drivers/phy/phy-twl4030-usb.c struct twl4030_usb { · line 171

Quoted 5 lines

drivers/phy/phy-twl4030-usb.c +4 -0

+	 * USB_EVENT_ENUMERATED
+	 */
+	unsigned		vbus_draw;
+
 	struct delayed_work	id_workaround_work;

The twl4030 phy can measure, with low precision, the resistance-to-ground of the ID pin.

Add a function to read the value, and export the result via sysfs.

If the read fails, which it does sometimes, try again in 50msec.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

drivers/phy/phy-twl4030-usb.c +3 -0

index 023fe150c7a1..759950898df9 100644
@@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 
+enum twl4030_id_status {
+	TWL4030_GROUND,
+	TWL4030_102K,

Hi!

Quoted 21 lines

The twl4030 phy can measure, with low precision, the resistance-to-ground of the ID pin.

Add a function to read the value, and export the result via sysfs.

If the read fails, which it does sometimes, try again in 50msec.

Signed-off-by NeilBrown <[email protected]>

---
 drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

drivers/phy/phy-twl4030-usb.c +0 -0

index 023fe150c7a1..759950898df9 100644
@@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 

drivers/phy/phy-twl4030-usb.c static void twl4030_i2c_access(struct twl4030_usb *twl, int on) · line 389

Quoted 13 lines

drivers/phy/phy-twl4030-usb.c +13 -0

+enum twl4030_id_status {
+	TWL4030_GROUND,
+	TWL4030_102K,
+	TWL4030_200K,
+	TWL4030_440K,
+	TWL4030_FLOATING,
+	TWL4030_ID_UNKNOWN,
+};
+static char *twl4030_id_names[] = {
+	"ground",
+	"102k",
+	"200k",
+	"440k",

New /sys files should be documented somewhere...?

Does it make sense to change "440k" -> "440KOhm"?

Plus I guess you need to update Documentation/

Acked-by Pavel Machek <[email protected]>

Quoted 24 lines

Hi!

Quoted 21 lines

The twl4030 phy can measure, with low precision, the resistance-to-ground of the ID pin.

Add a function to read the value, and export the result via sysfs.

If the read fails, which it does sometimes, try again in 50msec.

Signed-off-by NeilBrown <[email protected]>

---
drivers/phy/phy-twl4030-usb.c | 63 +++++++++++++++++++++++++++++++++++++++++

1 file changed, 63 insertions(+)

drivers/phy/phy-twl4030-usb.c +0 -0

index 023fe150c7a1..759950898df9 100644
@@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 

Quoted 15 lines

drivers/phy/phy-twl4030-usb.c static void twl4030_i2c_access(struct twl4030_usb *twl, int on) · line 389

Quoted 13 lines

drivers/phy/phy-twl4030-usb.c +13 -0

+enum twl4030_id_status {
+	TWL4030_GROUND,
+	TWL4030_102K,
+	TWL4030_200K,
+	TWL4030_440K,
+	TWL4030_FLOATING,
+	TWL4030_ID_UNKNOWN,
+};
+static char *twl4030_id_names[] = {
+	"ground",
+	"102k",
+	"200k",
+	"440k",

New /sys files should be documented somewhere...?

Preferably with the code...

Quoted 1 line

Quoted 1 line

Does it make sense to change "440k" -> "440KOhm"?

Interesting question. I prefer to avoid including units in files - bare numbers is better. But there is no number to match "floating" unless I spell it out as "infinity", and wouldn't be helpful.

Certainly "K" would be preferred over "k“,

and given that I have "ground"
and "floating", it is more consistent to include the "Ohm"....

These are really names, not measures of resistance. The data sheet calls them:
ID_RES_FLOAT (or sometimes ID_FLOAT)
ID_RES_440K
ID_RES_200K
ID_RES_102K
ID_GND (or sometimes ID_RES_GND)

So using those names is defensible.

I think I'll change them all to upper case, but leave out the "Ohm". My justification is consistency with the data sheet.

Quoted 2 lines

Plus I guess you need to update Documentation/

I guess I'll need to give in to this eventually :-)

Quoted 3 lines

Acked-by Pavel Machek <[email protected]>

Thanks,
NeilBrown
_______________________________________________
Gta04-owner mailing list
[email protected]
http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Am 04.03.2015 um 07:35 schrieb NeilBrown <[email protected]>:

Quoted 50 lines · On Mon, 2 Mar 2015 22:04:31 +0100 Pavel Machek <[email protected]> wrote:

Quoted 24 lines

Hi!

Quoted 21 lines

The twl4030 phy can measure, with low precision, the resistance-to-ground of the ID pin.

Add a function to read the value, and export the result via sysfs.

If the read fails, which it does sometimes, try again in 50msec.

Signed-off-by NeilBrown <[email protected]>

---
drivers/phy/phy-twl4030-usb.c | 63 +++++++++++++++++++++++++++++++++++++++++

1 file changed, 63 insertions(+)

drivers/phy/phy-twl4030-usb.c +0 -0

index 023fe150c7a1..759950898df9 100644
@@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 

Quoted 15 lines

drivers/phy/phy-twl4030-usb.c static void twl4030_i2c_access(struct twl4030_usb *twl, int on) · line 389

Quoted 13 lines

drivers/phy/phy-twl4030-usb.c +13 -0

+enum twl4030_id_status {
+	TWL4030_GROUND,
+	TWL4030_102K,
+	TWL4030_200K,
+	TWL4030_440K,
+	TWL4030_FLOATING,
+	TWL4030_ID_UNKNOWN,
+};
+static char *twl4030_id_names[] = {
+	"ground",
+	"102k",
+	"200k",
+	"440k",

New /sys files should be documented somewhere...?

Preferably with the code...

Quoted 2 lines

Does it make sense to change "440k" -> "440KOhm"?

Interesting question. I prefer to avoid including units in files - bare numbers is better. But there is no number to match "floating" unless I spell it out as "infinity", and wouldn't be helpful.

Quoted 1 line

Certainly "K" would be preferred over "k“,

The international standard for kilo = 1000 is a lower case k. While it is (non-standard) to use K for 1024:

http://en.wikipedia.org/wiki/Kilobyte

So I would keep the string constants lower case to avoid this potential confusion.

Quoted 1 line

New /sys files should be documented somewhere...?

Preferably with the code...

Quoted 1 line

Does it make sense to change "440k" -> "440KOhm"?

Interesting question. I prefer to avoid including units in files - bare numbers is better. But there is no number to match "floating" unless I spell it out as "infinity", and wouldn't be helpful.

Certainly "K" would be preferred over "k", and given that I have "ground" and "floating", it is more consistent to include the "Ohm"....

These are really names, not measures of resistance. The data sheet calls them:
ID_RES_FLOAT (or sometimes ID_FLOAT)
ID_RES_440K
ID_RES_200K
ID_RES_102K
ID_GND (or sometimes ID_RES_GND)

So using those names is defensible.

I think I'll change them all to upper case, but leave out the "Ohm". My justification is consistency with the data sheet.

Quoted 1 line

Plus I guess you need to update Documentation/

I guess I'll need to give in to this eventually :-)

If an 'A' plug is inserted, ID should be pulled to ground. If a 'B' plug, then ID should be floating.

If an Accessory Charger Adapter is inserted, then ID will be neither grounded nor floating. In this case tell the USB subsystem that it is an A plug, and the battery charging subsystem that it is a charger.

Fortunately, this will treat the Openmoko charger (and other similar chargers) as a charger.

Signed-off-by NeilBrown <[email protected]>

drivers/phy/phy-twl4030-usb.c +0 -0

@@ -596,9 +596,31 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	struct twl4030_usb *twl = _twl;
 	enum omap_musb_vbus_id_status status;

Quoted 7 lines · NeilBrown

If an 'A' plug is inserted, ID should be pulled to ground. If a 'B' plug, then ID should be floating.

If an Accessory Charger Adapter is inserted, then ID will be neither grounded nor floating. In this case tell the USB subsystem that it is an A plug, and the battery charging subsystem that it is a charger.

Quoted 4 lines

Fortunately, this will treat the Openmoko charger (and other similar chargers) as a charger.

Signed-off-by NeilBrown <[email protected]>

I guess signed-off-by should be "real name", so I'd add a space..

Acked-by Pavel Machek <[email protected]>

Quoted 11 lines

drivers/phy/phy-twl4030-usb.c +1 -0

@@ -596,9 +596,31 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	struct twl4030_usb *twl = _twl;
 	enum omap_musb_vbus_id_status status;
 	bool status_changed = false;
+	bool found_charger = false;
 
 	status = twl4030_usb_linkstat(twl);
 

drivers/phy/phy-twl4030-usb.c static irqreturn_t twl4030_usb_irq(int irq, void *_twl) · line 607

Quoted 5 lines

drivers/phy/phy-twl4030-usb.c +5 -0

+	if (status == OMAP_MUSB_ID_GROUND ||
+	    status == OMAP_MUSB_VBUS_VALID) {
+		/* We should check the resistance on the ID pin.
+		 * If not a Ground or Floating, then this is
+		 * likely a charger

"charger.", and I guess kernel comments should have /* on separate line.

So it will draw .5A from the charger? 1A? 2A?

Pavel

Hi,
the following 4 patches make some improvements to the twl4030 phy. Together with some other patches I have for twl4030_charger, they allow for better automatic control of charging.

In particular, the status of the ID pin is assessed and the result in communicated to the charger drivers.
There is also support for conveying the max current negotiated by a host to the charger.

Comments most welcome.