| Message ID | 20150416080304.23307.41913.stgit@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); > > > >
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); >>> >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);