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

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

[NeilBrown]

Hi Kishon,
 this is a slightly modified version of the series I sent earlier.

 I've removed that patches which use the old 'notifier chain' and will
 solve the issue of communicating current available separately.

 I've added a couple of patches which fix some pm_runtime issues.

 In particular:
     phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.

 Fixes a bug which causes the usb phy to remain permanently powered
 on, hence the Cc to Tony.

 If these could be queued for some future merge window, I would really
 appreciate it.

Thanks,
NeilBrown


---

NeilBrown (6):
      phy: twl4030-usb: make runtime pm more reliable.
      phy: twl4030-usb: remove pointless 'suspended' test in 'suspend' callback.
      phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.
      phy: twl4030-usb: add ABI documentation
      phy: twl4030-usb: add support for reading resistor on ID pin.
      phy: twl4030-usb: add extcon to report cable connections.


 .../ABI/testing/sysfs-platform-twl4030-usb         |   30 ++++
 drivers/phy/phy-twl4030-usb.c                      |  164 ++++++++++++++++++--
 2 files changed, 180 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb

--
Signature

[PATCH 4/6] phy: twl4030-usb: add ABI documentation

[NeilBrown]

From: NeilBrown <neilb@suse.de>

This driver device one local attribute: vbus.
Describe that in Documentation/ABI/testing/sysfs-platform/twl4030-usb.

Signed-off-by: NeilBrown <neil@brown.name>
---
 .../ABI/testing/sysfs-platform-twl4030-usb         |    8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb

diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
new file mode 100644
index 000000000000..512c51be64ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
@@ -0,0 +1,8 @@
+What: /sys/bus/platform/devices/*twl4030-usb/vbus
+Description:
+	Read-only status reporting if VBUS (approx 5V)
+	is being supplied by the USB bus.
+
+	Possible values: "on", "off".
+
+	Changes are notified via select/poll.

Re: [PATCH 4/6] phy: twl4030-usb: add ABI documentation

[Pavel Machek]

On Thu 2015-04-16 18:03:04, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
> 
> This driver device one local attribute: vbus.
> Describe that in Documentation/ABI/testing/sysfs-platform/twl4030-usb.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  .../ABI/testing/sysfs-platform-twl4030-usb         |    8 ++++++++
>  1 file changed, 8 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> new file mode 100644
> index 000000000000..512c51be64ae
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> @@ -0,0 +1,8 @@
> +What: /sys/bus/platform/devices/*twl4030-usb/vbus
> +Description:
> +	Read-only status reporting if VBUS (approx 5V)
> +	is being supplied by the USB bus.
> +
> +	Possible values: "on", "off".

Would bit be better to have values "0" and "1"? Kernel usually does
that for booleans...

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

Re: [PATCH 4/6] phy: twl4030-usb: add ABI documentation

[NeilBrown]

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

On Sat, 18 Apr 2015 00:14:36 +0200 Pavel Machek <pavel@ucw.cz> wrote:

> On Thu 2015-04-16 18:03:04, NeilBrown wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > This driver device one local attribute: vbus.
> > Describe that in Documentation/ABI/testing/sysfs-platform/twl4030-usb.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  .../ABI/testing/sysfs-platform-twl4030-usb         |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > new file mode 100644
> > index 000000000000..512c51be64ae
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > @@ -0,0 +1,8 @@
> > +What: /sys/bus/platform/devices/*twl4030-usb/vbus
> > +Description:
> > +	Read-only status reporting if VBUS (approx 5V)
> > +	is being supplied by the USB bus.
> > +
> > +	Possible values: "on", "off".
> 
> Would bit be better to have values "0" and "1"? Kernel usually does
> that for booleans...

1/ The code  already uses "on" and "off", so changing would be an ABI
breakage.

2/ No it doesn't.
 For modules params, the kernel uses "Y" and "N"

  git grep '? "on" : "off"'  | wc

 find 172 matches.

NeilBrown

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

[PATCH 6/6] phy: twl4030-usb: add extcon to report cable connections.

[NeilBrown]

From: NeilBrown <neilb@suse.de>

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

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 1d6f3e70193e..c42153d43ec2 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -40,6 +40,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/extcon.h>
 
 /* Register defines */
 
@@ -173,6 +174,9 @@ struct twl4030_usb {
 	enum omap_musb_vbus_id_status linkstat;
 	bool			vbus_supplied;
 
+	/* cable connection */
+	struct extcon_dev	edev;
+
 	struct delayed_work	id_workaround_work;
 };
 
@@ -592,6 +596,54 @@ static ssize_t twl4030_usb_id_show(struct device *dev,
 }
 static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
 
+static const char *usb_cables[] = {
+	"USB", /* id is floating */
+	"Charger-downstream", /* id is floating and D+ shorted to D- */
+	"USB-Host", /* id is ground */
+	"USB-ACA", /* "Accessory Charger Adapter" id is something else */
+	NULL
+};
+enum {
+	TWL_CABLE_USB,
+	TWL_CABLE_CHARGER,	/* Not used - twl4030 can detect charger,
+				 * but driver cannot yet */
+	TWL_CABLE_OTG,
+	TWL_CABLE_ACA,
+};
+static u32 all_exclusive[] =  {0xFFFFFFFF, 0};
+
+static void twl4030_usb_report_cable(struct twl4030_usb *twl)
+{
+	enum twl4030_id_status sts;
+
+	if (!cable_present(twl->linkstat)) {
+		extcon_set_state(&twl->edev, 0);
+		return;
+	}
+
+	sts = twl4030_get_id(twl);
+
+	switch (sts) {
+	case TWL4030_FLOATING: /* USB downstream */
+		extcon_update_state(&twl->edev,
+				    1<<TWL_CABLE_USB,
+				    1<<TWL_CABLE_USB);
+		break;
+	case TWL4030_GROUND: /* USB host */
+		extcon_update_state(&twl->edev,
+				    1<<TWL_CABLE_OTG,
+				    1<<TWL_CABLE_OTG);
+		break;
+	default: /* Some resistor */
+		extcon_update_state(&twl->edev,
+				    1<<TWL_CABLE_ACA,
+				    1<<TWL_CABLE_ACA);
+		/* An ACA should set port to 'host' mode */
+		twl->linkstat = OMAP_MUSB_ID_GROUND;
+		break;
+	}
+}
+
 static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 {
 	struct twl4030_usb *twl = _twl;
@@ -628,6 +680,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 			pm_runtime_put_autosuspend(twl->dev);
 		}
 		omap_musb_mailbox(status);
+		twl4030_usb_report_cable(twl);
 	}
 
 	/* don't schedule during sleep - irq works right then */
@@ -766,6 +819,20 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	}
 	usb_add_phy_dev(&twl->phy);
 
+	twl->edev.name = devm_kasprintf(twl->dev, GFP_KERNEL, "%s-usb",
+					dev_name(twl->dev->parent));
+	twl->edev.supported_cable = usb_cables;
+	twl->edev.mutually_exclusive = all_exclusive;
+	twl->edev.print_name = NULL; /* why would you change this? */
+	twl->edev.print_state = NULL; /* probably want to change this */
+
+	twl->edev.dev.parent = &pdev->dev;
+	err = extcon_dev_register(&twl->edev);
+	if (err) {
+		dev_err(&pdev->dev, "register extcon failed\n");
+		return err;
+	}
+
 	platform_set_drvdata(pdev, twl);
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");

Re: [PATCH 6/6] phy: twl4030-usb: add extcon to report cable connections.

[Kishon Vijay Abraham I]

+extcon MAINTAINERS

Hi,

On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>

commit msg pls.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   drivers/phy/phy-twl4030-usb.c |   67 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
>
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 1d6f3e70193e..c42153d43ec2 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -40,6 +40,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/err.h>
>   #include <linux/slab.h>
> +#include <linux/extcon.h>
>
>   /* Register defines */
>
> @@ -173,6 +174,9 @@ struct twl4030_usb {
>   	enum omap_musb_vbus_id_status linkstat;
>   	bool			vbus_supplied;
>
> +	/* cable connection */
> +	struct extcon_dev	edev;
> +
>   	struct delayed_work	id_workaround_work;
>   };
>
> @@ -592,6 +596,54 @@ static ssize_t twl4030_usb_id_show(struct device *dev,
>   }
>   static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
>
> +static const char *usb_cables[] = {
> +	"USB", /* id is floating */
> +	"Charger-downstream", /* id is floating and D+ shorted to D- */
> +	"USB-Host", /* id is ground */
> +	"USB-ACA", /* "Accessory Charger Adapter" id is something else */
> +	NULL
> +};
> +enum {
> +	TWL_CABLE_USB,
> +	TWL_CABLE_CHARGER,	/* Not used - twl4030 can detect charger,
> +				 * but driver cannot yet */
> +	TWL_CABLE_OTG,
> +	TWL_CABLE_ACA,
> +};
> +static u32 all_exclusive[] =  {0xFFFFFFFF, 0};
> +
> +static void twl4030_usb_report_cable(struct twl4030_usb *twl)
> +{
> +	enum twl4030_id_status sts;
> +
> +	if (!cable_present(twl->linkstat)) {
> +		extcon_set_state(&twl->edev, 0);
> +		return;
> +	}
> +
> +	sts = twl4030_get_id(twl);
> +
> +	switch (sts) {
> +	case TWL4030_FLOATING: /* USB downstream */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_USB,
> +				    1<<TWL_CABLE_USB);
> +		break;
> +	case TWL4030_GROUND: /* USB host */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_OTG,
> +				    1<<TWL_CABLE_OTG);
> +		break;
> +	default: /* Some resistor */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_ACA,
> +				    1<<TWL_CABLE_ACA);
> +		/* An ACA should set port to 'host' mode */
> +		twl->linkstat = OMAP_MUSB_ID_GROUND;
> +		break;
> +	}
> +}
> +
>   static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>   {
>   	struct twl4030_usb *twl = _twl;
> @@ -628,6 +680,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>   			pm_runtime_put_autosuspend(twl->dev);
>   		}
>   		omap_musb_mailbox(status);
> +		twl4030_usb_report_cable(twl);
>   	}
>
>   	/* don't schedule during sleep - irq works right then */
> @@ -766,6 +819,20 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>   	}
>   	usb_add_phy_dev(&twl->phy);
>
> +	twl->edev.name = devm_kasprintf(twl->dev, GFP_KERNEL, "%s-usb",
> +					dev_name(twl->dev->parent));
> +	twl->edev.supported_cable = usb_cables;
> +	twl->edev.mutually_exclusive = all_exclusive;
> +	twl->edev.print_name = NULL; /* why would you change this? */
> +	twl->edev.print_state = NULL; /* probably want to change this */

Not sure about those two callbacks. Chanwoo?

Thanks
Kishon

Re: [PATCH 6/6] phy: twl4030-usb: add extcon to report cable connections.

[Kishon Vijay Abraham I]

+extcon MAINTAINERS

Hi,

On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>

commit msg pls.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   drivers/phy/phy-twl4030-usb.c |   67 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
>
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 1d6f3e70193e..c42153d43ec2 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -40,6 +40,7 @@
>   #include <linux/regulator/consumer.h>
>   #include <linux/err.h>
>   #include <linux/slab.h>
> +#include <linux/extcon.h>
>
>   /* Register defines */
>
> @@ -173,6 +174,9 @@ struct twl4030_usb {
>   	enum omap_musb_vbus_id_status linkstat;
>   	bool			vbus_supplied;
>
> +	/* cable connection */
> +	struct extcon_dev	edev;
> +
>   	struct delayed_work	id_workaround_work;
>   };
>
> @@ -592,6 +596,54 @@ static ssize_t twl4030_usb_id_show(struct device *dev,
>   }
>   static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
>
> +static const char *usb_cables[] = {
> +	"USB", /* id is floating */
> +	"Charger-downstream", /* id is floating and D+ shorted to D- */
> +	"USB-Host", /* id is ground */
> +	"USB-ACA", /* "Accessory Charger Adapter" id is something else */
> +	NULL
> +};
> +enum {
> +	TWL_CABLE_USB,
> +	TWL_CABLE_CHARGER,	/* Not used - twl4030 can detect charger,
> +				 * but driver cannot yet */
> +	TWL_CABLE_OTG,
> +	TWL_CABLE_ACA,
> +};
> +static u32 all_exclusive[] =  {0xFFFFFFFF, 0};
> +
> +static void twl4030_usb_report_cable(struct twl4030_usb *twl)
> +{
> +	enum twl4030_id_status sts;
> +
> +	if (!cable_present(twl->linkstat)) {
> +		extcon_set_state(&twl->edev, 0);
> +		return;
> +	}
> +
> +	sts = twl4030_get_id(twl);
> +
> +	switch (sts) {
> +	case TWL4030_FLOATING: /* USB downstream */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_USB,
> +				    1<<TWL_CABLE_USB);
> +		break;
> +	case TWL4030_GROUND: /* USB host */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_OTG,
> +				    1<<TWL_CABLE_OTG);
> +		break;
> +	default: /* Some resistor */
> +		extcon_update_state(&twl->edev,
> +				    1<<TWL_CABLE_ACA,
> +				    1<<TWL_CABLE_ACA);
> +		/* An ACA should set port to 'host' mode */
> +		twl->linkstat = OMAP_MUSB_ID_GROUND;
> +		break;
> +	}
> +}
> +
>   static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>   {
>   	struct twl4030_usb *twl = _twl;
> @@ -628,6 +680,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>   			pm_runtime_put_autosuspend(twl->dev);
>   		}
>   		omap_musb_mailbox(status);
> +		twl4030_usb_report_cable(twl);
>   	}
>
>   	/* don't schedule during sleep - irq works right then */
> @@ -766,6 +819,20 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>   	}
>   	usb_add_phy_dev(&twl->phy);
>
> +	twl->edev.name = devm_kasprintf(twl->dev, GFP_KERNEL, "%s-usb",
> +					dev_name(twl->dev->parent));
> +	twl->edev.supported_cable = usb_cables;
> +	twl->edev.mutually_exclusive = all_exclusive;
> +	twl->edev.print_name = NULL; /* why would you change this? */
> +	twl->edev.print_state = NULL; /* probably want to change this */

Not sure about those two callbacks. Chanwoo?

Thanks
Kishon

Re: [PATCH 6/6] phy: twl4030-usb: add extcon to report cable connections.

[Chanwoo Choi]

Hi,

On Mon, May 11, 2015 at 10:38 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> +extcon MAINTAINERS
>
> Hi,
>
> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>
>> From: NeilBrown <neilb@suse.de>
>
>
> commit msg pls.
>
>>
>> Signed-off-by: NeilBrown <neilb@suse.de>
>> ---
>>   drivers/phy/phy-twl4030-usb.c |   67
>> +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 67 insertions(+)
>>
>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>> index 1d6f3e70193e..c42153d43ec2 100644
>> --- a/drivers/phy/phy-twl4030-usb.c
>> +++ b/drivers/phy/phy-twl4030-usb.c
>> @@ -40,6 +40,7 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/err.h>
>>   #include <linux/slab.h>
>> +#include <linux/extcon.h>
>>
>>   /* Register defines */
>>
>> @@ -173,6 +174,9 @@ struct twl4030_usb {
>>         enum omap_musb_vbus_id_status linkstat;
>>         bool                    vbus_supplied;
>>
>> +       /* cable connection */
>> +       struct extcon_dev       edev;
>> +
>>         struct delayed_work     id_workaround_work;
>>   };
>>
>> @@ -592,6 +596,54 @@ static ssize_t twl4030_usb_id_show(struct device
>> *dev,
>>   }
>>   static DEVICE_ATTR(id, 0444, twl4030_usb_id_show, NULL);
>>
>> +static const char *usb_cables[] = {
>> +       "USB", /* id is floating */
>> +       "Charger-downstream", /* id is floating and D+ shorted to D- */
>> +       "USB-Host", /* id is ground */
>> +       "USB-ACA", /* "Accessory Charger Adapter" id is something else */
>> +       NULL

Current extcon core use the string for external connector when
registering the extcon device.
But, It is not clear. So, I'm working to implement it with the unique
id for each external connectors
instead of previous string name. You can refer to ongoing patch[1].

[1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=v4.2/extcon-next&id=ab73c3cb76816f904d91191b13b9140f3684fa35

I recommend that you stop the implementation of this patch until
finishing the update[2] of extcon core.
After complete the update[2], you'd better to implement this path
again with new method
to register the extcon device.
[2] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/log/?h=v4.2/extcon-next


>> +};
>> +enum {
>> +       TWL_CABLE_USB,
>> +       TWL_CABLE_CHARGER,      /* Not used - twl4030 can detect charger,
>> +                                * but driver cannot yet */
>> +       TWL_CABLE_OTG,
>> +       TWL_CABLE_ACA,
>> +};
>> +static u32 all_exclusive[] =  {0xFFFFFFFF, 0};
>> +
>> +static void twl4030_usb_report_cable(struct twl4030_usb *twl)
>> +{
>> +       enum twl4030_id_status sts;
>> +
>> +       if (!cable_present(twl->linkstat)) {
>> +               extcon_set_state(&twl->edev, 0);
>> +               return;
>> +       }
>> +
>> +       sts = twl4030_get_id(twl);
>> +
>> +       switch (sts) {
>> +       case TWL4030_FLOATING: /* USB downstream */
>> +               extcon_update_state(&twl->edev,
>> +                                   1<<TWL_CABLE_USB,
>> +                                   1<<TWL_CABLE_USB);
>> +               break;
>> +       case TWL4030_GROUND: /* USB host */
>> +               extcon_update_state(&twl->edev,
>> +                                   1<<TWL_CABLE_OTG,
>> +                                   1<<TWL_CABLE_OTG);
>> +               break;
>> +       default: /* Some resistor */
>> +               extcon_update_state(&twl->edev,
>> +                                   1<<TWL_CABLE_ACA,
>> +                                   1<<TWL_CABLE_ACA);
>> +               /* An ACA should set port to 'host' mode */
>> +               twl->linkstat = OMAP_MUSB_ID_GROUND;
>> +               break;
>> +       }

I don't prefer to use the extcon_update_state() function because
the extcon_update_state() function make the difficult code to handle
the extcon device driver. So, I'm planning to handle the extcon_update_state()
in only extcon core. Instead, extcon driver can use the
extcon_set_cable_{state|state_}
to update the state of cable.

>> +}
>> +
>>   static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>>   {
>>         struct twl4030_usb *twl = _twl;
>> @@ -628,6 +680,7 @@ static irqreturn_t twl4030_usb_irq(int irq, void
>> *_twl)
>>                         pm_runtime_put_autosuspend(twl->dev);
>>                 }
>>                 omap_musb_mailbox(status);
>> +               twl4030_usb_report_cable(twl);
>>         }
>>
>>         /* don't schedule during sleep - irq works right then */
>> @@ -766,6 +819,20 @@ static int twl4030_usb_probe(struct platform_device
>> *pdev)
>>         }
>>         usb_add_phy_dev(&twl->phy);
>>
>> +       twl->edev.name = devm_kasprintf(twl->dev, GFP_KERNEL, "%s-usb",
>> +                                       dev_name(twl->dev->parent));

The name of extcon device should be set by extcon core. you can refer
to patch[3]
about the extcon device's name.
[3] https://lkml.org/lkml/2015/4/27/258
- extcon: Modify the device name as extcon[X] for sysfs

>> +       twl->edev.supported_cable = usb_cables;
>> +       twl->edev.mutually_exclusive = all_exclusive;
>> +       twl->edev.print_name = NULL; /* why would you change this? */
>> +       twl->edev.print_state = NULL; /* probably want to change this */

I don't agree that you store some variable to extcon_dev structure directly.

There are both extcon provider driver and extcon consumer driver. So,
the extcon_dev structure should be handled on extcon provider driver
which included in drivers/extcon. Namely, current extcon subsystem
has the problem about this. Also, I'm working to resolve this problem.

I don't prefer to implement the new extcon provider driver on
non-'drivers/extcon' directory.

>
>
> Not sure about those two callbacks. Chanwoo?
>
> Thanks
> Kishon


Best Regards,
Chanwoo Choi

[PATCH 3/6] phy: twl4030-usb: remove incorrect pm_runtime_get_sync() in probe function.

[NeilBrown]

The USB phy should initialize with power-off, and will be powered on
by the USB system when a cable connection is detected.

Having this pm_runtime_get_sync() during probe causes the phy to
*always* be powered on.
Removing it returns to sensible power management.

Fixes: 96be39ab34b77c6f6f5cd6ae03aac6c6449ee5c4
Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/phy/phy-twl4030-usb.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 590c2b1c1a94..3a707dd14238 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -715,7 +715,6 @@ static int twl4030_usb_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
 
 	/* Our job is to use irqs and status from the power module
 	 * to keep the transceiver disabled when nothing's connected.

[PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[NeilBrown]

From: NeilBrown <neilb@suse.de>

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.

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
 drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
index 512c51be64ae..425d23676f8a 100644
--- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
+++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
@@ -6,3 +6,25 @@ Description:
 	Possible values: "on", "off".
 
 	Changes are notified via select/poll.
+
+What: /sys/bus/platform/devices/*twl4030-usb/id
+Description:
+	Read-only report on measurement of USB-OTG ID pin.
+
+	The ID pin may be floating, grounded, or pulled to
+	ground by a resistor.
+
+	A very course grained reading of the resistance is
+	available.  The numbers given in kilo-ohms are roughly
+	the center-point of the detected range.
+
+	Possible values are:
+		ground
+		102k
+		200k
+		440k
+		floating
+		unknown
+
+	"unknown" indicates a problem with trying to detect
+	the resistance.
diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 3a707dd14238..1d6f3e70193e 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -379,6 +379,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);
@@ -532,6 +582,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;
@@ -709,6 +769,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);
 
@@ -753,6 +815,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 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Kishon Vijay Abraham I]

Hi,

On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
>
> 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.

Little sceptical about adding new sysfs entries. Do you have a good reason to 
add this?

Thanks
Kishon
>
> If the read fails, which it does sometimes, try again in 50msec.
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>   drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>   2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> index 512c51be64ae..425d23676f8a 100644
> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> @@ -6,3 +6,25 @@ Description:
>   	Possible values: "on", "off".
>
>   	Changes are notified via select/poll.
> +
> +What: /sys/bus/platform/devices/*twl4030-usb/id
> +Description:
> +	Read-only report on measurement of USB-OTG ID pin.
> +
> +	The ID pin may be floating, grounded, or pulled to
> +	ground by a resistor.
> +
> +	A very course grained reading of the resistance is
> +	available.  The numbers given in kilo-ohms are roughly
> +	the center-point of the detected range.
> +
> +	Possible values are:
> +		ground
> +		102k
> +		200k
> +		440k
> +		floating
> +		unknown
> +
> +	"unknown" indicates a problem with trying to detect
> +	the resistance.
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 3a707dd14238..1d6f3e70193e 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -379,6 +379,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);
> @@ -532,6 +582,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;
> @@ -709,6 +769,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);
>
> @@ -753,6 +815,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 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Kishon Vijay Abraham I]

Hi,

On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
>
> 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.

Little sceptical about adding new sysfs entries. Do you have a good reason to 
add this?

Thanks
Kishon
>
> If the read fails, which it does sometimes, try again in 50msec.
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>   drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>   2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> index 512c51be64ae..425d23676f8a 100644
> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> @@ -6,3 +6,25 @@ Description:
>   	Possible values: "on", "off".
>
>   	Changes are notified via select/poll.
> +
> +What: /sys/bus/platform/devices/*twl4030-usb/id
> +Description:
> +	Read-only report on measurement of USB-OTG ID pin.
> +
> +	The ID pin may be floating, grounded, or pulled to
> +	ground by a resistor.
> +
> +	A very course grained reading of the resistance is
> +	available.  The numbers given in kilo-ohms are roughly
> +	the center-point of the detected range.
> +
> +	Possible values are:
> +		ground
> +		102k
> +		200k
> +		440k
> +		floating
> +		unknown
> +
> +	"unknown" indicates a problem with trying to detect
> +	the resistance.
> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> index 3a707dd14238..1d6f3e70193e 100644
> --- a/drivers/phy/phy-twl4030-usb.c
> +++ b/drivers/phy/phy-twl4030-usb.c
> @@ -379,6 +379,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);
> @@ -532,6 +582,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;
> @@ -709,6 +769,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);
>
> @@ -753,6 +815,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 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[NeilBrown]

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

On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
wrote:

> Hi,
> 
> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> > From: NeilBrown <neilb@suse.de>
> >
> > 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.
> 
> Little sceptical about adding new sysfs entries. Do you have a good reason to 
> add this?

The hardware can report the value, so why not present it to user-space?

I originally used this with a udev rule which would configure the maximum
current based on the resistance measure - to work with the particular charger
hardware I have.

More recent patches try to do all of the max-current configuration in the
kernel, so I could live without exporting the value via sysfs if that is a
show-stopper.

I can't see where the scepticism comes from though.  It is a well defined
and cleary documented feature of the hardware.  Why not expose it?

Thanks,
NeilBrown


> 
> Thanks
> Kishon
> >
> > If the read fails, which it does sometimes, try again in 50msec.
> >
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
> >   drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
> >   2 files changed, 85 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > index 512c51be64ae..425d23676f8a 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > @@ -6,3 +6,25 @@ Description:
> >   	Possible values: "on", "off".
> >
> >   	Changes are notified via select/poll.
> > +
> > +What: /sys/bus/platform/devices/*twl4030-usb/id
> > +Description:
> > +	Read-only report on measurement of USB-OTG ID pin.
> > +
> > +	The ID pin may be floating, grounded, or pulled to
> > +	ground by a resistor.
> > +
> > +	A very course grained reading of the resistance is
> > +	available.  The numbers given in kilo-ohms are roughly
> > +	the center-point of the detected range.
> > +
> > +	Possible values are:
> > +		ground
> > +		102k
> > +		200k
> > +		440k
> > +		floating
> > +		unknown
> > +
> > +	"unknown" indicates a problem with trying to detect
> > +	the resistance.
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 3a707dd14238..1d6f3e70193e 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -379,6 +379,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);
> > @@ -532,6 +582,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;
> > @@ -709,6 +769,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);
> >
> > @@ -753,6 +815,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);
> >
> >


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

Re: [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[NeilBrown]

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

On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
wrote:

> Hi,
> 
> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> > From: NeilBrown <neilb@suse.de>
> >
> > 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.
> 
> Little sceptical about adding new sysfs entries. Do you have a good reason to 
> add this?

The hardware can report the value, so why not present it to user-space?

I originally used this with a udev rule which would configure the maximum
current based on the resistance measure - to work with the particular charger
hardware I have.

More recent patches try to do all of the max-current configuration in the
kernel, so I could live without exporting the value via sysfs if that is a
show-stopper.

I can't see where the scepticism comes from though.  It is a well defined
and cleary documented feature of the hardware.  Why not expose it?

Thanks,
NeilBrown


> 
> Thanks
> Kishon
> >
> > If the read fails, which it does sometimes, try again in 50msec.
> >
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
> >   drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
> >   2 files changed, 85 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > index 512c51be64ae..425d23676f8a 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
> > @@ -6,3 +6,25 @@ Description:
> >   	Possible values: "on", "off".
> >
> >   	Changes are notified via select/poll.
> > +
> > +What: /sys/bus/platform/devices/*twl4030-usb/id
> > +Description:
> > +	Read-only report on measurement of USB-OTG ID pin.
> > +
> > +	The ID pin may be floating, grounded, or pulled to
> > +	ground by a resistor.
> > +
> > +	A very course grained reading of the resistance is
> > +	available.  The numbers given in kilo-ohms are roughly
> > +	the center-point of the detected range.
> > +
> > +	Possible values are:
> > +		ground
> > +		102k
> > +		200k
> > +		440k
> > +		floating
> > +		unknown
> > +
> > +	"unknown" indicates a problem with trying to detect
> > +	the resistance.
> > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
> > index 3a707dd14238..1d6f3e70193e 100644
> > --- a/drivers/phy/phy-twl4030-usb.c
> > +++ b/drivers/phy/phy-twl4030-usb.c
> > @@ -379,6 +379,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);
> > @@ -532,6 +582,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;
> > @@ -709,6 +769,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);
> >
> > @@ -753,6 +815,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);
> >
> >


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

Re: [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Kishon Vijay Abraham I]

Hi,

On Tuesday 02 June 2015 03:07 AM, NeilBrown wrote:
> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> wrote:
>
>> Hi,
>>
>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>> From: NeilBrown <neilb@suse.de>
>>>
>>> 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.
>>
>> Little sceptical about adding new sysfs entries. Do you have a good reason to
>> add this?
>
> The hardware can report the value, so why not present it to user-space?
>
> I originally used this with a udev rule which would configure the maximum
> current based on the resistance measure - to work with the particular charger
> hardware I have.
>
> More recent patches try to do all of the max-current configuration in the
> kernel, so I could live without exporting the value via sysfs if that is a
> show-stopper.
>
> I can't see where the scepticism comes from though.  It is a well defined
> and cleary documented feature of the hardware.  Why not expose it?

ABI can never be removed or modified later. So should be really careful before 
adding it.

Thanks
Kishon

>
> Thanks,
> NeilBrown
>
>
>>
>> Thanks
>> Kishon
>>>
>>> If the read fails, which it does sometimes, try again in 50msec.
>>>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>    .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>>>    drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>>>    2 files changed, 85 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> index 512c51be64ae..425d23676f8a 100644
>>> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> @@ -6,3 +6,25 @@ Description:
>>>    	Possible values: "on", "off".
>>>
>>>    	Changes are notified via select/poll.
>>> +
>>> +What: /sys/bus/platform/devices/*twl4030-usb/id
>>> +Description:
>>> +	Read-only report on measurement of USB-OTG ID pin.
>>> +
>>> +	The ID pin may be floating, grounded, or pulled to
>>> +	ground by a resistor.
>>> +
>>> +	A very course grained reading of the resistance is
>>> +	available.  The numbers given in kilo-ohms are roughly
>>> +	the center-point of the detected range.
>>> +
>>> +	Possible values are:
>>> +		ground
>>> +		102k
>>> +		200k
>>> +		440k
>>> +		floating
>>> +		unknown
>>> +
>>> +	"unknown" indicates a problem with trying to detect
>>> +	the resistance.
>>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>>> index 3a707dd14238..1d6f3e70193e 100644
>>> --- a/drivers/phy/phy-twl4030-usb.c
>>> +++ b/drivers/phy/phy-twl4030-usb.c
>>> @@ -379,6 +379,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);
>>> @@ -532,6 +582,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;
>>> @@ -709,6 +769,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);
>>>
>>> @@ -753,6 +815,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 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Kishon Vijay Abraham I]

Hi,

On Tuesday 02 June 2015 03:07 AM, NeilBrown wrote:
> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> wrote:
>
>> Hi,
>>
>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>> From: NeilBrown <neilb@suse.de>
>>>
>>> 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.
>>
>> Little sceptical about adding new sysfs entries. Do you have a good reason to
>> add this?
>
> The hardware can report the value, so why not present it to user-space?
>
> I originally used this with a udev rule which would configure the maximum
> current based on the resistance measure - to work with the particular charger
> hardware I have.
>
> More recent patches try to do all of the max-current configuration in the
> kernel, so I could live without exporting the value via sysfs if that is a
> show-stopper.
>
> I can't see where the scepticism comes from though.  It is a well defined
> and cleary documented feature of the hardware.  Why not expose it?

ABI can never be removed or modified later. So should be really careful before 
adding it.

Thanks
Kishon

>
> Thanks,
> NeilBrown
>
>
>>
>> Thanks
>> Kishon
>>>
>>> If the read fails, which it does sometimes, try again in 50msec.
>>>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>    .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>>>    drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>>>    2 files changed, 85 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> index 512c51be64ae..425d23676f8a 100644
>>> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> @@ -6,3 +6,25 @@ Description:
>>>    	Possible values: "on", "off".
>>>
>>>    	Changes are notified via select/poll.
>>> +
>>> +What: /sys/bus/platform/devices/*twl4030-usb/id
>>> +Description:
>>> +	Read-only report on measurement of USB-OTG ID pin.
>>> +
>>> +	The ID pin may be floating, grounded, or pulled to
>>> +	ground by a resistor.
>>> +
>>> +	A very course grained reading of the resistance is
>>> +	available.  The numbers given in kilo-ohms are roughly
>>> +	the center-point of the detected range.
>>> +
>>> +	Possible values are:
>>> +		ground
>>> +		102k
>>> +		200k
>>> +		440k
>>> +		floating
>>> +		unknown
>>> +
>>> +	"unknown" indicates a problem with trying to detect
>>> +	the resistance.
>>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>>> index 3a707dd14238..1d6f3e70193e 100644
>>> --- a/drivers/phy/phy-twl4030-usb.c
>>> +++ b/drivers/phy/phy-twl4030-usb.c
>>> @@ -379,6 +379,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);
>>> @@ -532,6 +582,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;
>>> @@ -709,6 +769,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);
>>>
>>> @@ -753,6 +815,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: [Gta04-owner] [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Dr. H. Nikolaus Schaller]

Hi,

Am 02.06.2015 um 15:49 schrieb Kishon Vijay Abraham I <kishon@ti.com>:

> Hi,
> 
> On Tuesday 02 June 2015 03:07 AM, NeilBrown wrote:
>> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
>> wrote:
>> 
>>> Hi,
>>> 
>>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>>> From: NeilBrown <neilb@suse.de>
>>>> 
>>>> 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.
>>> 
>>> Little sceptical about adding new sysfs entries. Do you have a good reason to
>>> add this?
>> 
>> The hardware can report the value, so why not present it to user-space?
>> 
>> I originally used this with a udev rule which would configure the maximum
>> current based on the resistance measure - to work with the particular charger
>> hardware I have.
>> 
>> More recent patches try to do all of the max-current configuration in the
>> kernel, so I could live without exporting the value via sysfs if that is a
>> show-stopper.
>> 
>> I can't see where the scepticism comes from though.  It is a well defined
>> and cleary documented feature of the hardware.  Why not expose it?
> 
> ABI can never be removed or modified later. So should be really careful before adding it.

Is /sys considered ABI? It is permanently changing. At least in what I see.

User space developers are always reminded not to rely on /sys nodes.
Or if they do they have to follow kernel changes at their own risk.

And if something is useful (and has a use case as Neil mentioned), why shouldn’t
it be provided.

There are use cases where user space needs to know the value. Udev rule being
an example. E.g. to make LEDs show the state.

Or see it as a debugging tool. Just cat /sys/…path…/id to check if your 3 types
of charger are recognised properly.

Or write a tool that displays the charger type.

So isn’t that a little too narrow view applied here?

Just my opinion.

BR,
Nikolaus

> 
> Thanks
> Kishon
> 
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>>> 
>>> Thanks
>>> Kishon
>>>> 
>>>> If the read fails, which it does sometimes, try again in 50msec.
>>>> 
>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>> ---
>>>>   .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>>>>   drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>>>>   2 files changed, 85 insertions(+)
>>>> 
>>>> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>>> index 512c51be64ae..425d23676f8a 100644
>>>> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>>> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>>> @@ -6,3 +6,25 @@ Description:
>>>>   	Possible values: "on", "off".
>>>> 
>>>>   	Changes are notified via select/poll.
>>>> +
>>>> +What: /sys/bus/platform/devices/*twl4030-usb/id
>>>> +Description:
>>>> +	Read-only report on measurement of USB-OTG ID pin.
>>>> +
>>>> +	The ID pin may be floating, grounded, or pulled to
>>>> +	ground by a resistor.
>>>> +
>>>> +	A very course grained reading of the resistance is
>>>> +	available.  The numbers given in kilo-ohms are roughly
>>>> +	the center-point of the detected range.
>>>> +
>>>> +	Possible values are:
>>>> +		ground
>>>> +		102k
>>>> +		200k
>>>> +		440k
>>>> +		floating
>>>> +		unknown
>>>> +
>>>> +	"unknown" indicates a problem with trying to detect
>>>> +	the resistance.
>>>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>>>> index 3a707dd14238..1d6f3e70193e 100644
>>>> --- a/drivers/phy/phy-twl4030-usb.c
>>>> +++ b/drivers/phy/phy-twl4030-usb.c
>>>> @@ -379,6 +379,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);
>>>> @@ -532,6 +582,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;
>>>> @@ -709,6 +769,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);
>>>> 
>>>> @@ -753,6 +815,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);
>>>> 
>>>> 
>> 
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Re: [Gta04-owner] [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Pavel Machek]

On Tue 2015-06-02 16:06:47, Dr. H. Nikolaus Schaller wrote:
> Hi,
> 
> Am 02.06.2015 um 15:49 schrieb Kishon Vijay Abraham I <kishon@ti.com>:
> 
> > Hi,
> > 
> > On Tuesday 02 June 2015 03:07 AM, NeilBrown wrote:
> >> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> >> wrote:
> >> 
> >>> Hi,
> >>> 
> >>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> >>>> From: NeilBrown <neilb@suse.de>
> >>>> 
> >>>> 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.
> >>> 
> >>> Little sceptical about adding new sysfs entries. Do you have a good reason to
> >>> add this?
> >> 
> >> The hardware can report the value, so why not present it to user-space?
> >> 
> >> I originally used this with a udev rule which would configure the maximum
> >> current based on the resistance measure - to work with the particular charger
> >> hardware I have.
> >> 
> >> More recent patches try to do all of the max-current configuration in the
> >> kernel, so I could live without exporting the value via sysfs if that is a
> >> show-stopper.
> >> 
> >> I can't see where the scepticism comes from though.  It is a well defined
> >> and cleary documented feature of the hardware.  Why not expose it?

Is it well defined enough that it will work on other chargers, too?

> > ABI can never be removed or modified later. So should be really careful before adding it.
> 
> Is /sys considered ABI?

Yes.

> User space developers are always reminded not to rely on /sys nodes.

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

Re: [Gta04-owner] [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Dr. H. Nikolaus Schaller]

Hi,

Am 02.06.2015 um 22:11 schrieb Pavel Machek <pavel@ucw.cz>:

> On Tue 2015-06-02 16:06:47, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 02.06.2015 um 15:49 schrieb Kishon Vijay Abraham I <kishon@ti.com>:
>> 
>>> Hi,
>>> 
>>> On Tuesday 02 June 2015 03:07 AM, NeilBrown wrote:
>>>> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
>>>> wrote:
>>>> 
>>>>> Hi,
>>>>> 
>>>>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>>>>> From: NeilBrown <neilb@suse.de>
>>>>>> 
>>>>>> 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.
>>>>> 
>>>>> Little sceptical about adding new sysfs entries. Do you have a good reason to
>>>>> add this?
>>>> 
>>>> The hardware can report the value, so why not present it to user-space?
>>>> 
>>>> I originally used this with a udev rule which would configure the maximum
>>>> current based on the resistance measure - to work with the particular charger
>>>> hardware I have.
>>>> 
>>>> More recent patches try to do all of the max-current configuration in the
>>>> kernel, so I could live without exporting the value via sysfs if that is a
>>>> show-stopper.
>>>> 
>>>> I can't see where the scepticism comes from though.  It is a well defined
>>>> and cleary documented feature of the hardware.  Why not expose it?
> 
> Is it well defined enough that it will work on other chargers, too?

It reports the resistance of the charger’s ID pin. So that works for all chargers connected
to a twl4030. As long as the ID pin goes to a 5 pin USB socket.

Other charger drivers do not need to expose a similar attribute since each twl4030 has its
unique path within the /sys tree.

> 
>>> ABI can never be removed or modified later. So should be really careful before adding it.
>> 
>> Is /sys considered ABI?
> 
> Yes.

You are right: https://lwn.net/Articles/172986/

But I am as well with my doubts: https://lwn.net/Articles/173093/

> 
>> User space developers are always reminded not to rely on /sys nodes.
> 
> No.

Then please explain why I have the impression that it is quite unstable.

BR,
Nikolaus

Re: [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Pavel Machek]

On Tue 2015-06-02 07:37:31, NeilBrown wrote:
> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> wrote:
> 
> > Hi,
> > 
> > On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> > > From: NeilBrown <neilb@suse.de>
> > >
> > > 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.
> > 
> > Little sceptical about adding new sysfs entries. Do you have a good reason to 
> > add this?
> 
> The hardware can report the value, so why not present it to user-space?
> 
> I originally used this with a udev rule which would configure the maximum
> current based on the resistance measure - to work with the particular charger
> hardware I have.
> 
> More recent patches try to do all of the max-current configuration in the
> kernel, so I could live without exporting the value via sysfs if that is a
> show-stopper.
> 
> I can't see where the scepticism comes from though.  It is a well defined
> and cleary documented feature of the hardware.  Why not expose it?

sysfs interface is supposed to work for different chargers, without userspace
noticing.

Are these values the ones that are likely to be useful there?

> > > +	Possible values are:
> > > +		ground
> > > +		102k
> > > +		200k
> > > +		440k
> > > +		floating
> > > +		unknown

...or would it make more sense to export for example numerical ohms, as that's
what are other chargers likely to provide?

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

Re: [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Felipe Balbi]

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

On Sat, Jun 06, 2015 at 03:10:09PM +0200, Pavel Machek wrote:
> On Tue 2015-06-02 07:37:31, NeilBrown wrote:
> > On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> > wrote:
> > 
> > > Hi,
> > > 
> > > On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> > > > From: NeilBrown <neilb@suse.de>
> > > >
> > > > 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.
> > > 
> > > Little sceptical about adding new sysfs entries. Do you have a good reason to 
> > > add this?
> > 
> > The hardware can report the value, so why not present it to user-space?
> > 
> > I originally used this with a udev rule which would configure the maximum
> > current based on the resistance measure - to work with the particular charger
> > hardware I have.
> > 
> > More recent patches try to do all of the max-current configuration in the
> > kernel, so I could live without exporting the value via sysfs if that is a
> > show-stopper.
> > 
> > I can't see where the scepticism comes from though.  It is a well defined
> > and cleary documented feature of the hardware.  Why not expose it?
> 
> sysfs interface is supposed to work for different chargers, without userspace
> noticing.
> 
> Are these values the ones that are likely to be useful there?

These values come from Battery Charger specification 1.1+, IIRC, so no
other values should show up unless documented in future BC revisions.

> > > > +	Possible values are:
> > > > +		ground
> > > > +		102k
> > > > +		200k
> > > > +		440k
> > > > +		floating
> > > > +		unknown
> 
> ...or would it make more sense to export for example numerical ohms, as that's
> what are other chargers likely to provide?

How do you expose "floating" in Ohms ? UINT_MAX might be one way, but
that would have to be documented.

-- 
balbi

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

Re: [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Felipe Balbi]

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

On Sat, Jun 06, 2015 at 03:10:09PM +0200, Pavel Machek wrote:
> On Tue 2015-06-02 07:37:31, NeilBrown wrote:
> > On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> > wrote:
> > 
> > > Hi,
> > > 
> > > On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
> > > > From: NeilBrown <neilb@suse.de>
> > > >
> > > > 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.
> > > 
> > > Little sceptical about adding new sysfs entries. Do you have a good reason to 
> > > add this?
> > 
> > The hardware can report the value, so why not present it to user-space?
> > 
> > I originally used this with a udev rule which would configure the maximum
> > current based on the resistance measure - to work with the particular charger
> > hardware I have.
> > 
> > More recent patches try to do all of the max-current configuration in the
> > kernel, so I could live without exporting the value via sysfs if that is a
> > show-stopper.
> > 
> > I can't see where the scepticism comes from though.  It is a well defined
> > and cleary documented feature of the hardware.  Why not expose it?
> 
> sysfs interface is supposed to work for different chargers, without userspace
> noticing.
> 
> Are these values the ones that are likely to be useful there?

These values come from Battery Charger specification 1.1+, IIRC, so no
other values should show up unless documented in future BC revisions.

> > > > +	Possible values are:
> > > > +		ground
> > > > +		102k
> > > > +		200k
> > > > +		440k
> > > > +		floating
> > > > +		unknown
> 
> ...or would it make more sense to export for example numerical ohms, as that's
> what are other chargers likely to provide?

How do you expose "floating" in Ohms ? UINT_MAX might be one way, but
that would have to be documented.

-- 
balbi

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

Re: [Gta04-owner] [PATCH 5/6] phy: twl4030-usb: add support for reading resistor on ID pin.

[Dr. H. Nikolaus Schaller]

Hi Neil,

Am 01.06.2015 um 23:37 schrieb NeilBrown <neilb@suse.de>:

> On Mon, 1 Jun 2015 19:06:52 +0530 Kishon Vijay Abraham I <kishon@ti.com>
> wrote:
> 
>> Hi,
>> 
>> On Thursday 16 April 2015 01:33 PM, NeilBrown wrote:
>>> From: NeilBrown <neilb@suse.de>
>>> 
>>> 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.
>> 
>> Little sceptical about adding new sysfs entries. Do you have a good reason to 
>> add this?
> 
> The hardware can report the value, so why not present it to user-space?

I just had another idea how to present the value to user space.

The TWL6030 has connected the USB ID pin to one of the GPADC channels:

	http://lxr.free-electrons.com/source/drivers/iio/adc/twl6030-gpadc.c#L235

And therefore automatically presents the ID pin voltage through iio.

Would it be possible and useful to provide an iio interface for the resistance-to-ground
of the tw4030 ID pin as well?

This would resemble a 6 or 7 level ADC with non-linear scale, but better than nothing.

And to avoid the “floating” issue, it could also present some voltage value (assuming
a defined current).

So that “floating” is reported as some maximum voltage (e.g. 3.3V) and “ground” as 0V.

What do you think?

BR,
Nikolaus

> 
> I originally used this with a udev rule which would configure the maximum
> current based on the resistance measure - to work with the particular charger
> hardware I have.
> 
> More recent patches try to do all of the max-current configuration in the
> kernel, so I could live without exporting the value via sysfs if that is a
> show-stopper.
> 
> I can't see where the scepticism comes from though.  It is a well defined
> and cleary documented feature of the hardware.  Why not expose it?
> 
> Thanks,
> NeilBrown
> 
> 
>> 
>> Thanks
>> Kishon
>>> 
>>> If the read fails, which it does sometimes, try again in 50msec.
>>> 
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>  .../ABI/testing/sysfs-platform-twl4030-usb         |   22 +++++++
>>>  drivers/phy/phy-twl4030-usb.c                      |   63 ++++++++++++++++++++
>>>  2 files changed, 85 insertions(+)
>>> 
>>> diff --git a/Documentation/ABI/testing/sysfs-platform-twl4030-usb b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> index 512c51be64ae..425d23676f8a 100644
>>> --- a/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> +++ b/Documentation/ABI/testing/sysfs-platform-twl4030-usb
>>> @@ -6,3 +6,25 @@ Description:
>>>  	Possible values: "on", "off".
>>> 
>>>  	Changes are notified via select/poll.
>>> +
>>> +What: /sys/bus/platform/devices/*twl4030-usb/id
>>> +Description:
>>> +	Read-only report on measurement of USB-OTG ID pin.
>>> +
>>> +	The ID pin may be floating, grounded, or pulled to
>>> +	ground by a resistor.
>>> +
>>> +	A very course grained reading of the resistance is
>>> +	available.  The numbers given in kilo-ohms are roughly
>>> +	the center-point of the detected range.
>>> +
>>> +	Possible values are:
>>> +		ground
>>> +		102k
>>> +		200k
>>> +		440k
>>> +		floating
>>> +		unknown
>>> +
>>> +	"unknown" indicates a problem with trying to detect
>>> +	the resistance.
>>> diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
>>> index 3a707dd14238..1d6f3e70193e 100644
>>> --- a/drivers/phy/phy-twl4030-usb.c
>>> +++ b/drivers/phy/phy-twl4030-usb.c
>>> @@ -379,6 +379,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);
>>> @@ -532,6 +582,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;
>>> @@ -709,6 +769,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);
>>> 
>>> @@ -753,6 +815,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);
>>> 
>>> 
> 
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

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

[NeilBrown]

From: NeilBrown <neilb@suse.de>

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 used 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.
Define "cable_present()" to test for this condition.

Tested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/phy/phy-twl4030-usb.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index bc42d6a8939f..3078f80bf520 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -144,6 +144,16 @@
 #define PMBR1				0x0D
 #define GPIO_USB_4PIN_ULPI_2430C	(3 << 0)
 
+/*
+ * If VBUS is valid or ID is ground, then we know a
+ * cable is present and we need to be runtime-enabled
+ */
+static inline bool cable_present(enum omap_musb_vbus_id_status stat)
+{
+	return stat == OMAP_MUSB_VBUS_VALID ||
+		stat == OMAP_MUSB_ID_GROUND;
+}
+
 struct twl4030_usb {
 	struct usb_phy		phy;
 	struct device		*dev;
@@ -536,8 +546,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 
 	mutex_lock(&twl->lock);
 	if (status >= 0 && status != twl->linkstat) {
+		status_changed =
+			cable_present(twl->linkstat) !=
+			cable_present(status);
 		twl->linkstat = status;
-		status_changed = true;
 	}
 	mutex_unlock(&twl->lock);
 
@@ -553,15 +565,11 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		 * USB_LINK_VBUS state.  musb_hdrc won't care until it
 		 * starts to handle softconnect right.
 		 */
-		if ((status == OMAP_MUSB_VBUS_VALID) ||
-		    (status == OMAP_MUSB_ID_GROUND)) {
-			if (pm_runtime_suspended(twl->dev))
-				pm_runtime_get_sync(twl->dev);
+		if (cable_present(status)) {
+			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);
 	}
@@ -767,6 +775,9 @@ 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 (cable_present(twl->linkstat))
+		pm_runtime_put_noidle(twl->dev);
 	pm_runtime_mark_last_busy(twl->dev);
 	pm_runtime_put(twl->dev);
 

[PATCH 2/6] phy: twl4030-usb: remove pointless 'suspended' test in 'suspend' callback.

[NeilBrown]

When the runtime_suspend callback is running, 'runtime_status'
is always RPM_SUSPENDING, so pm_runtime_suspended() will always
fail.
Similarly while the runtime_resume callback is running
'runtime_status' is RPM_RESUMING, so pm_runtime_active() will
always fail.

So remove these two pointless tests.

Signed-off-by: NeilBrown <neil@brown.name>
---
 drivers/phy/phy-twl4030-usb.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c
index 3078f80bf520..590c2b1c1a94 100644
--- a/drivers/phy/phy-twl4030-usb.c
+++ b/drivers/phy/phy-twl4030-usb.c
@@ -396,8 +396,6 @@ static int twl4030_usb_runtime_suspend(struct device *dev)
 	struct twl4030_usb *twl = dev_get_drvdata(dev);
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	if (pm_runtime_suspended(dev))
-		return 0;
 
 	__twl4030_phy_power(twl, 0);
 	regulator_disable(twl->usb1v5);
@@ -413,8 +411,6 @@ static int twl4030_usb_runtime_resume(struct device *dev)
 	int res;
 
 	dev_dbg(twl->dev, "%s\n", __func__);
-	if (pm_runtime_active(dev))
-		return 0;
 
 	res = regulator_enable(twl->usb3v1);
 	if (res)