[PATCH 0/4] Enhancements to twl4030 phy to support better charging.

[PATCH 0/4] Enhancements to twl4030 phy to support better charging.

[NeilBrown]

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.

thanks,
NeilBrown


---

NeilBrown (4):
      usb: phy: twl4030: make runtime pm more reliable.
      usb: phy: twl4030: allow charger to see usb current draw limits.
      usb: phy: twl4030: add support for reading restore on ID pin.
      usb: phy: twl4030: test ID resistance to see if charger is present.


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

--
Signature

[PATCH 1/4] usb: phy: twl4030: make runtime pm more reliable.

[NeilBrown]

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 <neilb@suse.de>
---
 drivers/phy/phy-twl4030-usb.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 8e87f54671f3..97c59074233f 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -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);
 

Re: [PATCH 1/4] usb: phy: twl4030: make runtime pm more reliable.

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [150223 19:45]:
> 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.

Looks good to me, thanks for fixing it. I've tested this series
on beagleboard-xm by plugging and unplugging devices and
switching between host and peripheral mode, things still
idle properly for off-idle. So please feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>
 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  drivers/phy/phy-twl4030-usb.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 8e87f54671f3..97c59074233f 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -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);
>  
> 
> 

Re: [PATCH 1/4] usb: phy: twl4030: make runtime pm more reliable.

[Pavel Machek]

Hi!

> +		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;
...
> @@ -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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/4] usb: phy: twl4030: make runtime pm more reliable.

[NeilBrown]

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

On Mon, 2 Mar 2015 22:03:59 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > +		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;
> ...
> > @@ -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

Good idea.  I've done that.  The function is called "cable_present()".

Thanks,
NeilBrown

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

[PATCH 2/4] usb: phy: twl4030: allow charger to see usb current draw limits.

[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 <neilb@suse.de>
---
 drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 97c59074233f..023fe150c7a1 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -163,6 +163,11 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 
+	/* Permitted vbus draw - only meaningful after
+	 * USB_EVENT_ENUMERATED
+	 */
+	unsigned		vbus_draw;
+
 	struct delayed_work	id_workaround_work;
 };
 
@@ -547,12 +552,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	mutex_unlock(&twl->lock);
 
 	if (status_changed) {
-		/* FIXME add a set_power() method so that B-devices can
-		 * configure the charger appropriately.  It's not always
-		 * correct to consume VBUS power, and how much current to
-		 * consume is a function of the USB configuration chosen
-		 * by the host.
-		 *
+		/*
 		 * REVISIT usb_gadget_vbus_connect(...) as needed, ditto
 		 * its disconnect() sibling, when changing to/from the
 		 * USB_LINK_VBUS state.  musb_hdrc won't care until it
@@ -625,6 +625,20 @@ static int twl4030_set_host(struct usb_otg *otg, struct usb_bus *host)
 	return 0;
 }
 
+static int twl4030_set_power(struct usb_phy *phy, unsigned mA)
+{
+	struct twl4030_usb *twl = phy_to_twl(phy);
+
+	if (twl->vbus_draw != mA) {
+		phy->last_event = USB_EVENT_ENUMERATED;
+		twl->vbus_draw = mA;
+		atomic_notifier_call_chain(&phy->notifier,
+					   USB_EVENT_ENUMERATED,
+					   &twl->vbus_draw);
+	}
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= twl4030_phy_init,
 	.power_on	= twl4030_phy_power_on,
@@ -675,6 +689,7 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	twl->phy.label		= "twl4030";
 	twl->phy.otg		= otg;
 	twl->phy.type		= USB_PHY_TYPE_USB2;
+	twl->phy.set_power	= twl4030_set_power;
 
 	otg->usb_phy		= &twl->phy;
 	otg->set_host		= twl4030_set_host;

Re: [PATCH 2/4] usb: phy: twl4030: allow charger to see usb current draw limits.

[Pavel Machek]

On Tue 2015-02-24 14:40:37, NeilBrown wrote:
> 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 <neilb@suse.de>
> ---
>  drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 97c59074233f..023fe150c7a1 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -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"?

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/4] usb: phy: twl4030: allow charger to see usb current draw limits.

[NeilBrown]

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

On Mon, 2 Mar 2015 22:03:55 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> On Tue 2015-02-24 14:40:37, NeilBrown wrote:
> > 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 <neilb@suse.de>
> > ---
> >  drivers/phy/phy-twl4030-usb.c |   27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 97c59074233f..023fe150c7a1 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -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"?
> 
> > +	 * USB_EVENT_ENUMERATED
> > +	 */
> > +	unsigned		vbus_draw;
> > +
> >  	struct delayed_work	id_workaround_work;
> 

Yes.  I make it 'unsigned int' too.

Thanks,
NeilBrown

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

[PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[NeilBrown]

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 <neilb@suse.de>
---
 drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 023fe150c7a1..759950898df9 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
 	}
 }
 
+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",
+	"floating",
+	"unknown"
+};
+
+enum twl4030_id_status twl4030_get_id(struct twl4030_usb *twl)
+{
+	int ret;
+
+	pm_runtime_get_sync(twl->dev);
+	if (twl->usb_mode == T2_USB_MODE_ULPI)
+		twl4030_i2c_access(twl, 1);
+	ret = twl4030_usb_read(twl, ULPI_OTG_CTRL);
+	if (ret < 0 || !(ret & ULPI_OTG_ID_PULLUP)) {
+		/* Need pull-up to read ID */
+		twl4030_usb_set_bits(twl, ULPI_OTG_CTRL,
+				     ULPI_OTG_ID_PULLUP);
+		mdelay(50);
+	}
+	ret = twl4030_usb_read(twl, ID_STATUS);
+	if (ret < 0 || (ret & 0x1f) == 0) {
+		mdelay(50);
+		ret = twl4030_usb_read(twl, ID_STATUS);
+	}
+
+	if (twl->usb_mode == T2_USB_MODE_ULPI)
+		twl4030_i2c_access(twl, 0);
+	pm_runtime_put_autosuspend(twl->dev);
+
+	if (ret < 0)
+		return TWL4030_ID_UNKNOWN;
+	ret = ffs(ret) - 1;
+	if (ret < TWL4030_GROUND || ret > TWL4030_FLOATING)
+		return TWL4030_ID_UNKNOWN;
+
+	return ret;
+}
+
 static void __twl4030_phy_power(struct twl4030_usb *twl, int on)
 {
 	u8 pwr = twl4030_usb_read(twl, PHY_PWR_CTRL);
@@ -531,6 +581,16 @@ static ssize_t twl4030_usb_vbus_show(struct device *dev,
 }
 static DEVICE_ATTR(vbus, 0444, twl4030_usb_vbus_show, NULL);
 
+static ssize_t twl4030_usb_id_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct twl4030_usb *twl = dev_get_drvdata(dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n",
+			 twl4030_id_names[twl4030_get_id(twl)]);
+}
+static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
+
 static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 {
 	struct twl4030_usb *twl = _twl;
@@ -723,6 +783,8 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, twl);
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
+	if (device_create_file(&pdev->dev, &dev_attr_id))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);
 
@@ -768,6 +830,7 @@ static int twl4030_usb_remove(struct platform_device *pdev)
 	pm_runtime_get_sync(twl->dev);
 	cancel_delayed_work(&twl->id_workaround_work);
 	device_remove_file(twl->dev, &dev_attr_vbus);
+	device_remove_file(twl->dev, &dev_attr_id);
 
 	/* set transceiver mode to power on defaults */
 	twl4030_usb_set_mode(twl, -1);

Re: [PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[Pavel Machek]

Hi!

> 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 <neilb@suse.de>
> ---
>  drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 023fe150c7a1..759950898df9 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
>  	}
>  }
>  
> +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 <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[NeilBrown]

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

On Mon, 2 Mar 2015 22:04:31 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > 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 <neilb@suse.de>
> > ---
> >  drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 023fe150c7a1..759950898df9 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
> >  	}
> >  }
> >  
> > +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...

> 
> 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.




> 
> Plus I guess you need to update Documentation/

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

> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 

Thanks,
NeilBrown

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

Re: [Gta04-owner] [PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[Dr. H. Nikolaus Schaller]

Am 04.03.2015 um 07:35 schrieb NeilBrown <neilb@suse.de>:

> On Mon, 2 Mar 2015 22:04:31 +0100 Pavel Machek <pavel@ucw.cz> wrote:
> 
>> Hi!
>> 
>>> 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 <neilb@suse.de>
>>> ---
>>> drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 63 insertions(+)
>>> 
>>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>>> index 023fe150c7a1..759950898df9 100644
>>> --- a/drivers/phy/phy-twl4030-usb.c
>>> +++ b/drivers/phy/phy-twl4030-usb.c
>>> @@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
>>> 	}
>>> }
>>> 
>>> +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...
> 
>> 
>> 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“,

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.

> 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.
> 
> 
> 
> 
>> 
>> Plus I guess you need to update Documentation/
> 
> I guess I'll need to give in to this eventually :-)
> 
>> 
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> 
> 
> Thanks,
> NeilBrown
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Re: [Gta04-owner] [PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[NeilBrown]

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

On Wed, 4 Mar 2015 07:54:41 +0100 "Dr. H. Nikolaus Schaller"
<hns@goldelico.com> wrote:

> 
> Am 04.03.2015 um 07:35 schrieb NeilBrown <neilb@suse.de>:
> 
> > On Mon, 2 Mar 2015 22:04:31 +0100 Pavel Machek <pavel@ucw.cz> wrote:
> > 
> >> Hi!
> >> 
> >>> 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 <neilb@suse.de>
> >>> ---
> >>> drivers/phy/phy-twl4030-usb.c |   63 +++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 63 insertions(+)
> >>> 
> >>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> >>> index 023fe150c7a1..759950898df9 100644
> >>> --- a/drivers/phy/phy-twl4030-usb.c
> >>> +++ b/drivers/phy/phy-twl4030-usb.c
> >>> @@ -374,6 +374,56 @@ static void twl4030_i2c_access(struct twl4030_usb *twl, int on)
> >>> 	}
> >>> }
> >>> 
> >>> +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...
> > 
> >> 
> >> 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“,
> 
> 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.

Yes, I got that backwards, didn't I.

I think I'll leave it as lower-case.
I won't include the word "ohm" - it is very uncommon to have units explicit
in sysfs attribute values.

Thanks,
NeilBrown

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

Re: [PATCH 3/4] usb: phy: twl4030: add support for reading restore on ID pin.

[Pavel Machek]

Hi!

> > New /sys files should be documented somewhere...?
> 
> Preferably with the code...
> 
> > 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.

Does it make sense to use "_ohm" in the attribute name, then? (And
yes, I was wrong with the "K", "k" is actually right.)

> > Plus I guess you need to update Documentation/
> 
> I guess I'll need to give in to this eventually :-)

Yes please. It was useful in past.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 4/4] usb: phy: twl4030: test ID resistance to see if charger is present.

[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.

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

Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/phy/phy-twl4030-usb.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 759950898df9..8a43080cdbd7 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -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);
 
+	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
+		 */
+		enum twl4030_id_status sts = twl4030_get_id(twl);
+		if (sts > TWL4030_GROUND &&
+		    sts < TWL4030_FLOATING) {
+			/*
+			 * This might be a charger, or an
+			 * Accessory Charger Adapter.
+			 * In either case we can charge, and it
+			 * makes sense to tell the USB system
+			 * that we might be acting as a HOST.
+			 */
+			status = OMAP_MUSB_ID_GROUND;
+			found_charger = true;
+		}
+	}
+
 	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
 		status_changed =
@@ -627,6 +649,12 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		}
 		omap_musb_mailbox(status);
 	}
+	if (found_charger && twl->phy.last_event != USB_EVENT_CHARGER) {
+		twl->phy.last_event = USB_EVENT_CHARGER;
+		atomic_notifier_call_chain(&twl->phy.notifier,
+					   USB_EVENT_CHARGER,
+					   NULL);
+	}
 
 	/* don't schedule during sleep - irq works right then */
 	if (status == OMAP_MUSB_ID_GROUND && pm_runtime_active(twl->dev)) {

Re: [PATCH 4/4] usb: phy: twl4030: test ID resistance to see if charger is present.

[Pavel Machek]

On Tue 2015-02-24 14:40:37, NeilBrown wrote:
> 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 <neilb@suse.de>

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

Acked-by: Pavel Machek <pavel@ucw.cz>

> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -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);
>  
> +	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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 4/4] usb: phy: twl4030: test ID resistance to see if charger is present.

[NeilBrown]

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

On Mon, 2 Mar 2015 22:04:44 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> On Tue 2015-02-24 14:40:37, NeilBrown wrote:
> > 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 <neilb@suse.de>
> 
> I guess signed-off-by should be "real name", so I'd add a space..

This is how I always sign-off my name (2858 times in "git log") so I don't
think I'll change it now.

> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks.

> 
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -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);
> >  
> > +	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.

Yep.

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

That is up to the charger driver.  The phy just detects what is there, it
doesn't decide what to do with it.

Thanks,
NeilBrown

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

Re: [PATCH 0/4] Enhancements to twl4030 phy to support better charging.

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [150223 19:45]:
> 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.

Looks good to me, for the whole series, please feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>