Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471790 > unrolled thread

[PATCHv6 0/3] USB Type-C Connector class

Back to article view | Back to linux.kernel

Contents

  [PATCHv6 0/3] USB Type-C Connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-29 14:40 +0200
    [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on WhiskeyCove Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-29 14:40 +0200
      Re: [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB  Type-C PHY on WhiskeyCove Lee Jones <lee.jones@linaro.org> - 2016-08-30 11:00 +0200
        Re: [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB  Type-C PHY on WhiskeyCove Lee Jones <lee.jones@linaro.org> - 2016-08-30 15:40 +0200
    Re: [PATCHv6 0/3] USB Type-C Connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-29 15:10 +0200
      Re: [PATCHv6 0/3] USB Type-C Connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-29 15:50 +0200
    Re: [PATCHv6 1/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2016-08-30 11:40 +0200
      Re: [PATCHv6 1/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-30 12:10 +0200
        Re: [PATCHv6 1/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2016-08-30 13:20 +0200
          Re: [PATCHv6 1/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-30 14:00 +0200
            Re: [PATCHv6 1/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-30 15:20 +0200
              Re: [PATCHv6 1/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-30 15:50 +0200
                Re: [PATCHv6 1/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-31 13:20 +0200
                  Re: [PATCHv6 1/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-31 15:20 +0200
        Re: [PATCHv6 1/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-30 15:50 +0200
          Re: [PATCHv6 1/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-30 16:30 +0200
        Re: [PATCHv6 1/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-30 17:50 +0200

#1471790 — [PATCHv6 0/3] USB Type-C Connector class

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.


Changes since v6:
- current_vconn_role attr renamed to vconn_source (no API changes)
- Small documentation improvements proposed by Vincent Palatin

Changes since v5:
- Only updating the roles based on driver notifications
- Added MODULE_ALIAS for the WhiskeyCove module
- Including the patch that creates the actual platform device for the
  WhiskeyCove Type-C PHY in this series.

Changes since v4:
- Remove the port lock completely

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.


Heikki Krogerus (3):
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY
  mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on
    WhiskeyCove

 Documentation/ABI/testing/sysfs-class-typec |  205 +++++
 Documentation/usb/typec.txt                 |  103 +++
 MAINTAINERS                                 |    9 +
 drivers/mfd/intel_soc_pmic_bxtwc.c          |   11 +
 drivers/usb/Kconfig                         |    2 +
 drivers/usb/Makefile                        |    2 +
 drivers/usb/typec/Kconfig                   |   21 +
 drivers/usb/typec/Makefile                  |    2 +
 drivers/usb/typec/typec.c                   | 1091 +++++++++++++++++++++++++++
 drivers/usb/typec/typec_wcove.c             |  372 +++++++++
 include/linux/usb/typec.h                   |  260 +++++++
 11 files changed, 2078 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.8.1

[toc] | [next] | [standalone]

#1471791 — [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on WhiskeyCove

Intel WhiskeyCove PMIC has also a USB Type-C PHY, so let's
create a device for it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/intel_soc_pmic_bxtwc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
index b942876..0e61dde 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -84,6 +84,7 @@ enum bxtwc_irqs_level2 {
 	BXTWC_THRM2_IRQ,
 	BXTWC_BCU_IRQ,
 	BXTWC_ADC_IRQ,
+	BXTWC_USBC_IRQ,
 	BXTWC_CHGR0_IRQ,
 	BXTWC_CHGR1_IRQ,
 	BXTWC_GPIO0_IRQ,
@@ -109,6 +110,7 @@ static const struct regmap_irq bxtwc_regmap_irqs_level2[] = {
 	REGMAP_IRQ_REG(BXTWC_THRM2_IRQ, 2, 0xff),
 	REGMAP_IRQ_REG(BXTWC_BCU_IRQ, 3, 0x1f),
 	REGMAP_IRQ_REG(BXTWC_ADC_IRQ, 4, 0xff),
+	REGMAP_IRQ_REG(BXTWC_USBC_IRQ, 5, BIT(5)),
 	REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x1f),
 	REGMAP_IRQ_REG(BXTWC_CHGR1_IRQ, 6, 0x1f),
 	REGMAP_IRQ_REG(BXTWC_GPIO0_IRQ, 7, 0xff),
@@ -143,6 +145,10 @@ static struct resource adc_resources[] = {
 	DEFINE_RES_IRQ_NAMED(BXTWC_ADC_IRQ, "ADC"),
 };
 
+static struct resource usbc_resources[] = {
+	DEFINE_RES_IRQ(BXTWC_USBC_IRQ),
+};
+
 static struct resource charger_resources[] = {
 	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR0_IRQ, "CHARGER"),
 	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR1_IRQ, "CHARGER1"),
@@ -170,6 +176,11 @@ static struct mfd_cell bxt_wc_dev[] = {
 		.resources = thermal_resources,
 	},
 	{
+		.name = "bxt_wcove_usbc",
+		.num_resources = ARRAY_SIZE(usbc_resources),
+		.resources = usbc_resources,
+	},
+	{
 		.name = "bxt_wcove_ext_charger",
 		.num_resources = ARRAY_SIZE(charger_resources),
 		.resources = charger_resources,
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1472331 — Re: [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on WhiskeyCove

On Mon, 29 Aug 2016, Heikki Krogerus wrote:

> Intel WhiskeyCove PMIC has also a USB Type-C PHY, so let's
> create a device for it.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/intel_soc_pmic_bxtwc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
> index b942876..0e61dde 100644
> --- a/drivers/mfd/intel_soc_pmic_bxtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
> @@ -84,6 +84,7 @@ enum bxtwc_irqs_level2 {
>  	BXTWC_THRM2_IRQ,
>  	BXTWC_BCU_IRQ,
>  	BXTWC_ADC_IRQ,
> +	BXTWC_USBC_IRQ,
>  	BXTWC_CHGR0_IRQ,
>  	BXTWC_CHGR1_IRQ,
>  	BXTWC_GPIO0_IRQ,
> @@ -109,6 +110,7 @@ static const struct regmap_irq bxtwc_regmap_irqs_level2[] = {
>  	REGMAP_IRQ_REG(BXTWC_THRM2_IRQ, 2, 0xff),
>  	REGMAP_IRQ_REG(BXTWC_BCU_IRQ, 3, 0x1f),
>  	REGMAP_IRQ_REG(BXTWC_ADC_IRQ, 4, 0xff),
> +	REGMAP_IRQ_REG(BXTWC_USBC_IRQ, 5, BIT(5)),
>  	REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x1f),
>  	REGMAP_IRQ_REG(BXTWC_CHGR1_IRQ, 6, 0x1f),
>  	REGMAP_IRQ_REG(BXTWC_GPIO0_IRQ, 7, 0xff),
> @@ -143,6 +145,10 @@ static struct resource adc_resources[] = {
>  	DEFINE_RES_IRQ_NAMED(BXTWC_ADC_IRQ, "ADC"),
>  };
>  
> +static struct resource usbc_resources[] = {
> +	DEFINE_RES_IRQ(BXTWC_USBC_IRQ),
> +};
> +
>  static struct resource charger_resources[] = {
>  	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR0_IRQ, "CHARGER"),
>  	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR1_IRQ, "CHARGER1"),
> @@ -170,6 +176,11 @@ static struct mfd_cell bxt_wc_dev[] = {
>  		.resources = thermal_resources,
>  	},
>  	{
> +		.name = "bxt_wcove_usbc",
> +		.num_resources = ARRAY_SIZE(usbc_resources),
> +		.resources = usbc_resources,
> +	},
> +	{
>  		.name = "bxt_wcove_ext_charger",
>  		.num_resources = ARRAY_SIZE(charger_resources),
>  		.resources = charger_resources,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1472487 — Re: [PATCHv6 3/3] mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on WhiskeyCove

On Tue, 30 Aug 2016, Lee Jones wrote:

> On Mon, 29 Aug 2016, Heikki Krogerus wrote:
> 
> > Intel WhiskeyCove PMIC has also a USB Type-C PHY, so let's
> > create a device for it.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/mfd/intel_soc_pmic_bxtwc.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> Applied, thanks.

This patch conflicts with ...

 "mfd: intel_soc_pmic_bxtwc: Add bxt_wcove_usbc device"

Please re-base your changes on my tree (or -next) and resubmit if this
change is still relevant.

> > diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
> > index b942876..0e61dde 100644
> > --- a/drivers/mfd/intel_soc_pmic_bxtwc.c
> > +++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
> > @@ -84,6 +84,7 @@ enum bxtwc_irqs_level2 {
> >  	BXTWC_THRM2_IRQ,
> >  	BXTWC_BCU_IRQ,
> >  	BXTWC_ADC_IRQ,
> > +	BXTWC_USBC_IRQ,
> >  	BXTWC_CHGR0_IRQ,
> >  	BXTWC_CHGR1_IRQ,
> >  	BXTWC_GPIO0_IRQ,
> > @@ -109,6 +110,7 @@ static const struct regmap_irq bxtwc_regmap_irqs_level2[] = {
> >  	REGMAP_IRQ_REG(BXTWC_THRM2_IRQ, 2, 0xff),
> >  	REGMAP_IRQ_REG(BXTWC_BCU_IRQ, 3, 0x1f),
> >  	REGMAP_IRQ_REG(BXTWC_ADC_IRQ, 4, 0xff),
> > +	REGMAP_IRQ_REG(BXTWC_USBC_IRQ, 5, BIT(5)),
> >  	REGMAP_IRQ_REG(BXTWC_CHGR0_IRQ, 5, 0x1f),
> >  	REGMAP_IRQ_REG(BXTWC_CHGR1_IRQ, 6, 0x1f),
> >  	REGMAP_IRQ_REG(BXTWC_GPIO0_IRQ, 7, 0xff),
> > @@ -143,6 +145,10 @@ static struct resource adc_resources[] = {
> >  	DEFINE_RES_IRQ_NAMED(BXTWC_ADC_IRQ, "ADC"),
> >  };
> >  
> > +static struct resource usbc_resources[] = {
> > +	DEFINE_RES_IRQ(BXTWC_USBC_IRQ),
> > +};
> > +
> >  static struct resource charger_resources[] = {
> >  	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR0_IRQ, "CHARGER"),
> >  	DEFINE_RES_IRQ_NAMED(BXTWC_CHGR1_IRQ, "CHARGER1"),
> > @@ -170,6 +176,11 @@ static struct mfd_cell bxt_wc_dev[] = {
> >  		.resources = thermal_resources,
> >  	},
> >  	{
> > +		.name = "bxt_wcove_usbc",
> > +		.num_resources = ARRAY_SIZE(usbc_resources),
> > +		.resources = usbc_resources,
> > +	},
> > +	{
> >  		.name = "bxt_wcove_ext_charger",
> >  		.num_resources = ARRAY_SIZE(charger_resources),
> >  		.resources = charger_resources,
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1471825

On 08/29/2016 05:36 AM, Heikki Krogerus wrote:
> The USB Type-C class is meant to provide unified interface to the
> userspace to present the USB Type-C ports in a system.
>
The subject says "v6".

Guenter

>
> Changes since v6:
> - current_vconn_role attr renamed to vconn_source (no API changes)
> - Small documentation improvements proposed by Vincent Palatin
>
> Changes since v5:
> - Only updating the roles based on driver notifications
> - Added MODULE_ALIAS for the WhiskeyCove module
> - Including the patch that creates the actual platform device for the
>   WhiskeyCove Type-C PHY in this series.
>
> Changes since v4:
> - Remove the port lock completely
>
> Changes since v3:
> - Documentation cleanup as proposed by Roger Quadros
> - Setting partner altmodes member to NULL on removal and fixing a
>   warning, as proposed by Guenter Roeck
> - Added the following attributes for partners and cables:
>   * supports_usb_power_delivery
>   * id_header_vdo
> - "id_header_vdo" is visible only when the partner or cable supports
>   USB Power Delivery communication.
> - Partner attribute "accessory" is hidden when the partner type is not
>   "Accessory".
>
> Changes since v2:
> - Notification on role and alternate mode changes
> - cleanups
>
> Changes since v1:
> - Completely rewrote alternate mode support
> - Patners, cables and cable plugs presented as devices.
>
>
> Heikki Krogerus (3):
>   usb: USB Type-C connector class
>   usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY
>   mfd: intel_soc_pmic_bxtwc: add support for USB Type-C PHY on
>     WhiskeyCove
>
>  Documentation/ABI/testing/sysfs-class-typec |  205 +++++
>  Documentation/usb/typec.txt                 |  103 +++
>  MAINTAINERS                                 |    9 +
>  drivers/mfd/intel_soc_pmic_bxtwc.c          |   11 +
>  drivers/usb/Kconfig                         |    2 +
>  drivers/usb/Makefile                        |    2 +
>  drivers/usb/typec/Kconfig                   |   21 +
>  drivers/usb/typec/Makefile                  |    2 +
>  drivers/usb/typec/typec.c                   | 1091 +++++++++++++++++++++++++++
>  drivers/usb/typec/typec_wcove.c             |  372 +++++++++
>  include/linux/usb/typec.h                   |  260 +++++++
>  11 files changed, 2078 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 drivers/usb/typec/typec_wcove.c
>  create mode 100644 include/linux/usb/typec.h
>

[toc] | [prev] | [next] | [standalone]

#1471858

On Mon, Aug 29, 2016 at 06:06:39AM -0700, Guenter Roeck wrote:
> On 08/29/2016 05:36 AM, Heikki Krogerus wrote:
> > The USB Type-C class is meant to provide unified interface to the
> > userspace to present the USB Type-C ports in a system.
> > 
> The subject says "v6".

True. I used the wrong reroll-count when formatting. I'm sorry about
that.

Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1472363 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> +What:          /sys/class/typec/<port>/current_data_role
> +Date:          June 2016
> +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +               The current USB data role the port is operating in.
> This
> +               attribute can be used for requesting data role
> swapping on the
> +               port. Swapping is only supported as an asynchronous
> operation
> +               and requires polling of the attribute in order to know
> the
> +               result, so successful write operation does not mean
> successful
> +               swap.
> +

That is badly formulated. Does it mean that poll() or select()
can be used or does the value need to be repearedly read?
And how would you learn about an error?

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1472383 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

Hi Oliver,

On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
> On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> > +What:          /sys/class/typec/<port>/current_data_role
> > +Date:          June 2016
> > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +               The current USB data role the port is operating in.
> > This
> > +               attribute can be used for requesting data role
> > swapping on the
> > +               port. Swapping is only supported as an asynchronous
> > operation
> > +               and requires polling of the attribute in order to know
> > the
> > +               result, so successful write operation does not mean
> > successful
> > +               swap.
> > +
> 
> That is badly formulated. Does it mean that poll() or select()
> can be used or does the value need to be repearedly read?

Does polling not always mean poll/select?

> And how would you learn about an error?

This is what I'm also really worried about. I'm now wondering did I
give up too easily on this to Guenter in hope to move this thing
forward. He said it's problematic to do these calls synchronously for
him. Was it something related to potential conflicting role swaps from
both ends?

Guenter, can you please elaborate? And how do you plan to report
failures with the swaps?


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1472419 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On Tue, 2016-08-30 at 13:04 +0300, Heikki Krogerus wrote:
> On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:

Hi,

> > On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> > > +What:          /sys/class/typec/<port>/current_data_role
> > > +Date:          June 2016
> > > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +               The current USB data role the port is operating in.
> > > This
> > > +               attribute can be used for requesting data role
> > > swapping on the
> > > +               port. Swapping is only supported as an asynchronous
> > > operation
> > > +               and requires polling of the attribute in order to know
> > > the
> > > +               result, so successful write operation does not mean
> > > successful
> > > +               swap.
> > > +
> > 
> > That is badly formulated. Does it mean that poll() or select()
> > can be used or does the value need to be repearedly read?
> 
> Does polling not always mean poll/select?

No, it does not.

> > And how would you learn about an error?
> 
> This is what I'm also really worried about. I'm now wondering did I
> give up too easily on this to Guenter in hope to move this thing
> forward. He said it's problematic to do these calls synchronously for

Error reporting does not require a synchronous operation. Reporting
it in the next read() or write() and making it pollable is perfectly
viable. It just must not be silently dropped.

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1472435 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
> On Tue, 2016-08-30 at 13:04 +0300, Heikki Krogerus wrote:
> > On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
> 
> Hi,
> 
> > > On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> > > > +What:          /sys/class/typec/<port>/current_data_role
> > > > +Date:          June 2016
> > > > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > +Description:
> > > > +               The current USB data role the port is operating in.
> > > > This
> > > > +               attribute can be used for requesting data role
> > > > swapping on the
> > > > +               port. Swapping is only supported as an asynchronous
> > > > operation
> > > > +               and requires polling of the attribute in order to know
> > > > the
> > > > +               result, so successful write operation does not mean
> > > > successful
> > > > +               swap.
> > > > +
> > > 
> > > That is badly formulated. Does it mean that poll() or select()
> > > can be used or does the value need to be repearedly read?
> > 
> > Does polling not always mean poll/select?
> 
> No, it does not.
> 
> > > And how would you learn about an error?
> > 
> > This is what I'm also really worried about. I'm now wondering did I
> > give up too easily on this to Guenter in hope to move this thing
> > forward. He said it's problematic to do these calls synchronously for
> 
> Error reporting does not require a synchronous operation. Reporting
> it in the next read() or write() and making it pollable is perfectly
> viable. It just must not be silently dropped.

OK, I think I got it. I need to document that. I'll also add
get_pr/dr/vconn hooks to the API for getting the status.


Thanks Oliver,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1472485 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Aug 30, 2016 at 02:49:50PM +0300, Heikki Krogerus wrote:
> On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
> > Error reporting does not require a synchronous operation. Reporting
> > it in the next read() or write() and making it pollable is perfectly
> > viable. It just must not be silently dropped.
> 
> OK, I think I got it. I need to document that. I'll also add
> get_pr/dr/vconn hooks to the API for getting the status.

Would the attached diff do the trick? It also includes the other
suggestions from Guenter.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1472490 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On 08/30/2016 06:11 AM, Heikki Krogerus wrote:
> Hi,
>
> On Tue, Aug 30, 2016 at 02:49:50PM +0300, Heikki Krogerus wrote:
>> On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
>>> Error reporting does not require a synchronous operation. Reporting
>>> it in the next read() or write() and making it pollable is perfectly
>>> viable. It just must not be silently dropped.
>>
>> OK, I think I got it. I need to document that. I'll also add
>> get_pr/dr/vconn hooks to the API for getting the status.
>
> Would the attached diff do the trick? It also includes the other
> suggestions from Guenter.
>

It is not at all what I meant or asked for :-(. I'll have a closer
look into the latest patch set later today.

Guenter

[toc] | [prev] | [next] | [standalone]

#1473331 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

[Multipart message — attachments visible in raw view] — view raw

Hi guys,

On Tue, Aug 30, 2016 at 06:47:41AM -0700, Guenter Roeck wrote:
> On 08/30/2016 06:11 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Tue, Aug 30, 2016 at 02:49:50PM +0300, Heikki Krogerus wrote:
> > > On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
> > > > Error reporting does not require a synchronous operation. Reporting
> > > > it in the next read() or write() and making it pollable is perfectly
> > > > viable. It just must not be silently dropped.
> > > 
> > > OK, I think I got it. I need to document that. I'll also add
> > > get_pr/dr/vconn hooks to the API for getting the status.
> > 
> > Would the attached diff do the trick? It also includes the other
> > suggestions from Guenter.
> > 
> 
> It is not at all what I meant or asked for :-(. I'll have a closer
> look into the latest patch set later today.

An other attempt. This one has just the suggestions from you Guenter
(including the supports_usb_power_delivery attibute for cables as
well), and update to the ABI document.

Let me know if it's OK.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1473455 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On 08/31/2016 04:09 AM, Heikki Krogerus wrote:
> Hi guys,
>
> On Tue, Aug 30, 2016 at 06:47:41AM -0700, Guenter Roeck wrote:
>> On 08/30/2016 06:11 AM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Tue, Aug 30, 2016 at 02:49:50PM +0300, Heikki Krogerus wrote:
>>>> On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
>>>>> Error reporting does not require a synchronous operation. Reporting
>>>>> it in the next read() or write() and making it pollable is perfectly
>>>>> viable. It just must not be silently dropped.
>>>>
>>>> OK, I think I got it. I need to document that. I'll also add
>>>> get_pr/dr/vconn hooks to the API for getting the status.
>>>
>>> Would the attached diff do the trick? It also includes the other
>>> suggestions from Guenter.
>>>
>>
>> It is not at all what I meant or asked for :-(. I'll have a closer
>> look into the latest patch set later today.
>
> An other attempt. This one has just the suggestions from you Guenter
> (including the supports_usb_power_delivery attibute for cables as
> well), and update to the ABI document.
>
> Let me know if it's OK.
>

Yes, much better.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1472491 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On 08/30/2016 03:04 AM, Heikki Krogerus wrote:
> Hi Oliver,
>
> On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
>> On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
>>> +What:          /sys/class/typec/<port>/current_data_role
>>> +Date:          June 2016
>>> +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> +Description:
>>> +               The current USB data role the port is operating in.
>>> This
>>> +               attribute can be used for requesting data role
>>> swapping on the
>>> +               port. Swapping is only supported as an asynchronous
>>> operation
>>> +               and requires polling of the attribute in order to know
>>> the
>>> +               result, so successful write operation does not mean
>>> successful
>>> +               swap.
>>> +
>>
>> That is badly formulated. Does it mean that poll() or select()
>> can be used or does the value need to be repearedly read?
>
> Does polling not always mean poll/select?
>
>> And how would you learn about an error?
>
> This is what I'm also really worried about. I'm now wondering did I
> give up too easily on this to Guenter in hope to move this thing
> forward. He said it's problematic to do these calls synchronously for
> him. Was it something related to potential conflicting role swaps from
> both ends?
>
> Guenter, can you please elaborate? And how do you plan to report
> failures with the swaps?
>

I thought we had this sorted out. When I said "asynchronous", I did not mean
that the sysfs operation would not wait for the operation to complete. I meant
that the Type-C state machine operates in a different context than the sysfs/class
code. Since the state machine operates in a different context, it may have
to execute a callback into the class code at any time, independently of
any pending role changes triggered through sysfs. Please have a look into
the patch set I submitted for details. Roughly it works as follows.

Class code context				State machine context

User requests role change
Class code calls {dr,pr,vconn}_set
{dr,pr,vconn}_set code validates request
{dr,pr,vconn}_set code sends role change
	request to state machine		State machine gets role change request
{dr,pr,vconn}_set code waits for completion
						State machine sends role change request
						to link partner
						Partner reports Accept or Reject
						State machine changes state as requested
						State machine reports new role to class code
							via callbacks
						State machine informs Class code that request
						is complete
{dr,pr,vconn}_set code gets results
	and returns to caller
Class code reports results to user

 From user perspective, everything is synchronous. However, the state machine has to be
able to run independently and report role and other state changes to the class code while
a role change request from the class code is pending. For example, it has to be able to
handle incoming role change requests from the link partner, and it has to be able to
handle link state changes. All those have to be reported to the class code. This is
impossible if the class code holds a lock while a role change triggered from user space
is pending, which is why I asked for the locks in the class code to be removed.

Maybe my use of the term "asynchronous" was misleading, and I should have said "operates
in a different context" instead. My apologies.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1472515 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

On Tue, Aug 30, 2016 at 06:46:24AM -0700, Guenter Roeck wrote:
> On 08/30/2016 03:04 AM, Heikki Krogerus wrote:
> > Hi Oliver,
> > 
> > On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
> > > On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> > > > +What:          /sys/class/typec/<port>/current_data_role
> > > > +Date:          June 2016
> > > > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > +Description:
> > > > +               The current USB data role the port is operating in.
> > > > This
> > > > +               attribute can be used for requesting data role
> > > > swapping on the
> > > > +               port. Swapping is only supported as an asynchronous
> > > > operation
> > > > +               and requires polling of the attribute in order to know
> > > > the
> > > > +               result, so successful write operation does not mean
> > > > successful
> > > > +               swap.
> > > > +
> > > 
> > > That is badly formulated. Does it mean that poll() or select()
> > > can be used or does the value need to be repearedly read?
> > 
> > Does polling not always mean poll/select?
> > 
> > > And how would you learn about an error?
> > 
> > This is what I'm also really worried about. I'm now wondering did I
> > give up too easily on this to Guenter in hope to move this thing
> > forward. He said it's problematic to do these calls synchronously for
> > him. Was it something related to potential conflicting role swaps from
> > both ends?
> > 
> > Guenter, can you please elaborate? And how do you plan to report
> > failures with the swaps?
> > 
> 
> I thought we had this sorted out. When I said "asynchronous", I did not mean
> that the sysfs operation would not wait for the operation to complete. I meant
> that the Type-C state machine operates in a different context than the sysfs/class
> code. Since the state machine operates in a different context, it may have
> to execute a callback into the class code at any time, independently of
> any pending role changes triggered through sysfs. Please have a look into
> the patch set I submitted for details. Roughly it works as follows.
> 
> Class code context				State machine context
> 
> User requests role change
> Class code calls {dr,pr,vconn}_set
> {dr,pr,vconn}_set code validates request
> {dr,pr,vconn}_set code sends role change
> 	request to state machine		State machine gets role change request
> {dr,pr,vconn}_set code waits for completion
> 						State machine sends role change request
> 						to link partner
> 						Partner reports Accept or Reject
> 						State machine changes state as requested
> 						State machine reports new role to class code
> 							via callbacks
> 						State machine informs Class code that request
> 						is complete
> {dr,pr,vconn}_set code gets results
> 	and returns to caller
> Class code reports results to user
> 
> From user perspective, everything is synchronous. However, the state machine has to be
> able to run independently and report role and other state changes to the class code while
> a role change request from the class code is pending. For example, it has to be able to
> handle incoming role change requests from the link partner, and it has to be able to
> handle link state changes. All those have to be reported to the class code. This is
> impossible if the class code holds a lock while a role change triggered from user space
> is pending, which is why I asked for the locks in the class code to be removed.
> 
> Maybe my use of the term "asynchronous" was misleading, and I should have said "operates
> in a different context" instead. My apologies.

Thanks for the explanation. I remember you did explain this before I
started my parental leave, but I forgot all about it.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1472571 — Re: [PATCHv6 1/3] usb: USB Type-C connector class

Heikki,

On Tue, Aug 30, 2016 at 01:04:37PM +0300, Heikki Krogerus wrote:
> Hi Oliver,
> 
> On Tue, Aug 30, 2016 at 11:32:01AM +0200, Oliver Neukum wrote:
> > On Mon, 2016-08-29 at 15:36 +0300, Heikki Krogerus wrote:
> > > +What:          /sys/class/typec/<port>/current_data_role
> > > +Date:          June 2016
> > > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +               The current USB data role the port is operating in.
> > > This
> > > +               attribute can be used for requesting data role
> > > swapping on the
> > > +               port. Swapping is only supported as an asynchronous
> > > operation
> > > +               and requires polling of the attribute in order to know
> > > the
> > > +               result, so successful write operation does not mean
> > > successful
> > > +               swap.
> > > +
> > 
> > That is badly formulated. Does it mean that poll() or select()
> > can be used or does the value need to be repearedly read?
> 
> Does polling not always mean poll/select?
> 
> > And how would you learn about an error?
> 
> This is what I'm also really worried about. I'm now wondering did I
> give up too easily on this to Guenter in hope to move this thing
> forward. He said it's problematic to do these calls synchronously for
> him. Was it something related to potential conflicting role swaps from
> both ends?
> 
> Guenter, can you please elaborate? And how do you plan to report
> failures with the swaps?
> 
Following up on this again. For the record, I never meant to suggest that the
ABI to userspace should be asynchronous. On the contrary, I think it should be
synchronous. Reason is that it simplifies user space for the general case, where
user space does not mind the wait and wants to get a valid return code as part
of the operation.

The more complex case, where user space _does_ mind the wait, can always be
handled by implementing a separate thread to write into sysfs attributes and
wait for the result. This can be hidden in an application library which
can also implement callbacks or signals to other threads as needed. It can
also be tied with udev event handling to observe actual role changes (which
will probably be necessary anyway).

While this may sound complicated, the code necessary to implement and support
an asynchronous kernel ABI would be at least as complicated, and would probably
also require a polling thread and callbacks to report results to other threads.
So I don't really see a gain for user space by providing an asynchronous
kernel ABI.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web