[PATCH 0/4] usb: typec: fixes for Cherry Trails

[PATCH 0/4] usb: typec: fixes for Cherry Trails

[Heikki Krogerus]

Hi,

I now have a GPD Win device - Intel Cherry Trail based board with
FUSB302 USB Type-C PHY - that I started using for testing my alternate
mode patches. I noticed a few dependency issues, and some other
problems with the fusb302 driver. These were the most obvious ones. 
Hans, I included the patch for the xhci usb role mux to this series.

On top of these, the driver for INT34D3 (the PMIC) needs CHT GPIO
driver in order for it to work at least on GPD Win. If that is the
case with all CHT based platforms, we probable should make the driver
depend on the CHT GPIO, but I don't know if it is?

With CHT, the driver for fusb302 also depends in practice on the
extcon_intel_cht_wc extcon device. It would probable be possible to
check it in Kconfig, but I'm not proposing anything for that here
either. I wonder if that thing needs to be an extcon device at all?


Heikki Krogerus (4):
  platform: x86: intel_cht_int33fe: Fix dependencies
  usb: typec: tcpm: Release the role mux when exiting
  usb: typec: fusb302: Fix debugfs issue
  usb: roles: intel_xhci: Always allow user control

 drivers/i2c/busses/Kconfig                     |  3 +--
 drivers/platform/x86/Kconfig                   |  4 ++--
 drivers/usb/roles/intel-xhci-usb-role-switch.c | 15 +--------------
 drivers/usb/typec/fusb302/fusb302.c            |  9 ++++-----
 drivers/usb/typec/tcpm.c                       |  1 +
 5 files changed, 9 insertions(+), 23 deletions(-)

-- 
2.17.0

[PATCH 1/4] platform: x86: intel_cht_int33fe: Fix dependencies

[Heikki Krogerus]

The driver will not probe unless bq24190 is loaded, so
making it a dependency.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/Kconfig   | 3 +--
 drivers/platform/x86/Kconfig | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8d21b9825d71..fce9f2ca0570 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -202,8 +202,7 @@ config I2C_CHT_WC
 
 	  Note this controller is hooked up to a TI bq24292i charger-IC,
 	  combined with a FUSB302 Type-C port-controller as such it is advised
-	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
-	  (the fusb302 driver currently is in drivers/staging).
+	  to also select CONFIG_TYPEC_FUSB302=m.
 
 config I2C_NFORCE2
 	tristate "Nvidia nForce2, nForce3 and nForce4"
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 39d06dd1f63a..1dbd9d547e60 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -866,6 +866,7 @@ config ACPI_CMPC
 config INTEL_CHT_INT33FE
 	tristate "Intel Cherry Trail ACPI INT33FE Driver"
 	depends on X86 && ACPI && I2C && REGULATOR
+	depends on CHARGER_BQ24190
 	---help---
 	  This driver add support for the INT33FE ACPI device found on
 	  some Intel Cherry Trail devices.
@@ -877,8 +878,7 @@ config INTEL_CHT_INT33FE
 	  i2c drivers for these chips can bind to the them.
 
 	  If you enable this driver it is advised to also select
-	  CONFIG_TYPEC_FUSB302=m, CONFIG_CHARGER_BQ24190=m and
-	  CONFIG_BATTERY_MAX17042=m.
+	  CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m.
 
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
-- 
2.17.0

Re: [PATCH 1/4] platform: x86: intel_cht_int33fe: Fix dependencies

[Hans de Goede]

Hi,

On 30-04-18 14:41, Heikki Krogerus wrote:
> The driver will not probe unless bq24190 is loaded, so
> making it a dependency.

Hmm, the dependency is pure run-time, with this Kconfig
change if a user now decides to builtin the intel_cht_int33fe
driver, the bq24190 driver must also be builtin, I'm not sure
if that is a good thing.

Regards,

Hans


> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/i2c/busses/Kconfig   | 3 +--
>   drivers/platform/x86/Kconfig | 4 ++--
>   2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8d21b9825d71..fce9f2ca0570 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -202,8 +202,7 @@ config I2C_CHT_WC
>   
>   	  Note this controller is hooked up to a TI bq24292i charger-IC,
>   	  combined with a FUSB302 Type-C port-controller as such it is advised
> -	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
> -	  (the fusb302 driver currently is in drivers/staging).
> +	  to also select CONFIG_TYPEC_FUSB302=m.
>   
>   config I2C_NFORCE2
>   	tristate "Nvidia nForce2, nForce3 and nForce4"
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 39d06dd1f63a..1dbd9d547e60 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -866,6 +866,7 @@ config ACPI_CMPC
>   config INTEL_CHT_INT33FE
>   	tristate "Intel Cherry Trail ACPI INT33FE Driver"
>   	depends on X86 && ACPI && I2C && REGULATOR
> +	depends on CHARGER_BQ24190
>   	---help---
>   	  This driver add support for the INT33FE ACPI device found on
>   	  some Intel Cherry Trail devices.
> @@ -877,8 +878,7 @@ config INTEL_CHT_INT33FE
>   	  i2c drivers for these chips can bind to the them.
>   
>   	  If you enable this driver it is advised to also select
> -	  CONFIG_TYPEC_FUSB302=m, CONFIG_CHARGER_BQ24190=m and
> -	  CONFIG_BATTERY_MAX17042=m.
> +	  CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m.
>   
>   config INTEL_INT0002_VGPIO
>   	tristate "Intel ACPI INT0002 Virtual GPIO driver"
> 

Re: [PATCH 1/4] platform: x86: intel_cht_int33fe: Fix dependencies

[Heikki Krogerus]

On Tue, May 01, 2018 at 11:53:24AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 30-04-18 14:41, Heikki Krogerus wrote:
> > The driver will not probe unless bq24190 is loaded, so
> > making it a dependency.
> 
> Hmm, the dependency is pure run-time, with this Kconfig
> change if a user now decides to builtin the intel_cht_int33fe
> driver, the bq24190 driver must also be builtin, I'm not sure
> if that is a good thing.

OK. That should be possible to sort out with something like this:

depends on CHARGER_BQ24190=y || CHARGER_BQ24190=INTEL_CHT_INT33FE

I'll fix this one.

> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: Darren Hart <dvhart@infradead.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/i2c/busses/Kconfig   | 3 +--
> >   drivers/platform/x86/Kconfig | 4 ++--
> >   2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 8d21b9825d71..fce9f2ca0570 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -202,8 +202,7 @@ config I2C_CHT_WC
> >   	  Note this controller is hooked up to a TI bq24292i charger-IC,
> >   	  combined with a FUSB302 Type-C port-controller as such it is advised
> > -	  to also select CONFIG_CHARGER_BQ24190=m and CONFIG_TYPEC_FUSB302=m
> > -	  (the fusb302 driver currently is in drivers/staging).
> > +	  to also select CONFIG_TYPEC_FUSB302=m.
> >   config I2C_NFORCE2
> >   	tristate "Nvidia nForce2, nForce3 and nForce4"
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 39d06dd1f63a..1dbd9d547e60 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -866,6 +866,7 @@ config ACPI_CMPC
> >   config INTEL_CHT_INT33FE
> >   	tristate "Intel Cherry Trail ACPI INT33FE Driver"
> >   	depends on X86 && ACPI && I2C && REGULATOR
> > +	depends on CHARGER_BQ24190
> >   	---help---
> >   	  This driver add support for the INT33FE ACPI device found on
> >   	  some Intel Cherry Trail devices.
> > @@ -877,8 +878,7 @@ config INTEL_CHT_INT33FE
> >   	  i2c drivers for these chips can bind to the them.
> >   	  If you enable this driver it is advised to also select
> > -	  CONFIG_TYPEC_FUSB302=m, CONFIG_CHARGER_BQ24190=m and
> > -	  CONFIG_BATTERY_MAX17042=m.
> > +	  CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m.
> >   config INTEL_INT0002_VGPIO
> >   	tristate "Intel ACPI INT0002 Virtual GPIO driver"
> > 

Thanks,

-- 
heikki

[PATCH 2/4] usb: typec: tcpm: Release the role mux when exiting

[Heikki Krogerus]

The ref count for the USB role switch device must be
released after we are done using the switch.

Fixes: c6962c29729c ("usb: typec: tcpm: Set USB role switch to device mode when configured as such")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/tcpm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 1ee259bc14a5..66dc0773b9bf 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -4652,6 +4652,7 @@ void tcpm_unregister_port(struct tcpm_port *port)
 	for (i = 0; i < ARRAY_SIZE(port->port_altmode); i++)
 		typec_unregister_altmode(port->port_altmode[i]);
 	typec_unregister_port(port->typec_port);
+	usb_role_switch_put(port->role_sw);
 	tcpm_debugfs_exit(port);
 	destroy_workqueue(port->wq);
 }
-- 
2.17.0

Re: [PATCH 2/4] usb: typec: tcpm: Release the role mux when exiting

[Guenter Roeck]

On 04/30/2018 05:41 AM, Heikki Krogerus wrote:
> The ref count for the USB role switch device must be
> released after we are done using the switch.
> 
> Fixes: c6962c29729c ("usb: typec: tcpm: Set USB role switch to device mode when configured as such")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/tcpm.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 1ee259bc14a5..66dc0773b9bf 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -4652,6 +4652,7 @@ void tcpm_unregister_port(struct tcpm_port *port)
>   	for (i = 0; i < ARRAY_SIZE(port->port_altmode); i++)
>   		typec_unregister_altmode(port->port_altmode[i]);
>   	typec_unregister_port(port->typec_port);
> +	usb_role_switch_put(port->role_sw);
>   	tcpm_debugfs_exit(port);
>   	destroy_workqueue(port->wq);
>   }
> 

Re: [PATCH 2/4] usb: typec: tcpm: Release the role mux when exiting

[Hans de Goede]

Hi,

On 30-04-18 14:41, Heikki Krogerus wrote:
> The ref count for the USB role switch device must be
> released after we are done using the switch.
> 
> Fixes: c6962c29729c ("usb: typec: tcpm: Set USB role switch to device mode when configured as such")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Makes sense:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>   drivers/usb/typec/tcpm.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 1ee259bc14a5..66dc0773b9bf 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -4652,6 +4652,7 @@ void tcpm_unregister_port(struct tcpm_port *port)
>   	for (i = 0; i < ARRAY_SIZE(port->port_altmode); i++)
>   		typec_unregister_altmode(port->port_altmode[i]);
>   	typec_unregister_port(port->typec_port);
> +	usb_role_switch_put(port->role_sw);
>   	tcpm_debugfs_exit(port);
>   	destroy_workqueue(port->wq);
>   }
> 

[PATCH 3/4] usb: typec: fusb302: Fix debugfs issue

[Heikki Krogerus]

Removing the "fusb302" debugfs directory when unloading
the driver. That allows the driver to be loaded more then
ones.

This fixes an issue where the driver, if unloaded, can't be
re-loaded, as the "fusb302" debugfs directory already
exists. While here, removing useless condition when creating
the debugfs directory.

Fixes: 76f0c53d08b9 ("usb: typec: fusb302: Move out of staging")
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/fusb302/fusb302.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
index eba6bb890b17..0e5d0aa052f5 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ -218,11 +218,9 @@ static struct dentry *rootdir;
 static int fusb302_debugfs_init(struct fusb302_chip *chip)
 {
 	mutex_init(&chip->logbuffer_lock);
-	if (!rootdir) {
-		rootdir = debugfs_create_dir("fusb302", NULL);
-		if (!rootdir)
-			return -ENOMEM;
-	}
+	rootdir = debugfs_create_dir("fusb302", NULL);
+	if (!rootdir)
+		return -ENOMEM;
 
 	chip->dentry = debugfs_create_file(dev_name(chip->dev),
 					   S_IFREG | 0444, rootdir,
@@ -234,6 +232,7 @@ static int fusb302_debugfs_init(struct fusb302_chip *chip)
 static void fusb302_debugfs_exit(struct fusb302_chip *chip)
 {
 	debugfs_remove(chip->dentry);
+	debugfs_remove(rootdir);
 }
 
 #else
-- 
2.17.0

Re: [PATCH 3/4] usb: typec: fusb302: Fix debugfs issue

[Guenter Roeck]

On 04/30/2018 05:41 AM, Heikki Krogerus wrote:
> Removing the "fusb302" debugfs directory when unloading
> the driver. That allows the driver to be loaded more then
> ones.
> 
> This fixes an issue where the driver, if unloaded, can't be
> re-loaded, as the "fusb302" debugfs directory already
> exists. While here, removing useless condition when creating
> the debugfs directory.
> 
> Fixes: 76f0c53d08b9 ("usb: typec: fusb302: Move out of staging")
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>   drivers/usb/typec/fusb302/fusb302.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index eba6bb890b17..0e5d0aa052f5 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -218,11 +218,9 @@ static struct dentry *rootdir;
>   static int fusb302_debugfs_init(struct fusb302_chip *chip)
>   {
>   	mutex_init(&chip->logbuffer_lock);
> -	if (!rootdir) {
> -		rootdir = debugfs_create_dir("fusb302", NULL);
> -		if (!rootdir)
> -			return -ENOMEM;
> -	}

I think the idea here was to permit more than one instance of the driver,
and have all debugfs file entries under the fusb302 directory. Loading the
second instance will fail after this patch is applied.

> +	rootdir = debugfs_create_dir("fusb302", NULL);
> +	if (!rootdir)
> +		return -ENOMEM;
>   
>   	chip->dentry = debugfs_create_file(dev_name(chip->dev),
>   					   S_IFREG | 0444, rootdir,
> @@ -234,6 +232,7 @@ static int fusb302_debugfs_init(struct fusb302_chip *chip)
>   static void fusb302_debugfs_exit(struct fusb302_chip *chip)
>   {
>   	debugfs_remove(chip->dentry);
> +	debugfs_remove(rootdir); >   }
>   
>   #else
> 

Re: [PATCH 3/4] usb: typec: fusb302: Fix debugfs issue

[Heikki Krogerus]

On Mon, Apr 30, 2018 at 06:34:03AM -0700, Guenter Roeck wrote:
> On 04/30/2018 05:41 AM, Heikki Krogerus wrote:
> > Removing the "fusb302" debugfs directory when unloading
> > the driver. That allows the driver to be loaded more then
> > ones.
> > 
> > This fixes an issue where the driver, if unloaded, can't be
> > re-loaded, as the "fusb302" debugfs directory already
> > exists. While here, removing useless condition when creating
> > the debugfs directory.
> > 
> > Fixes: 76f0c53d08b9 ("usb: typec: fusb302: Move out of staging")
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >   drivers/usb/typec/fusb302/fusb302.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> > index eba6bb890b17..0e5d0aa052f5 100644
> > --- a/drivers/usb/typec/fusb302/fusb302.c
> > +++ b/drivers/usb/typec/fusb302/fusb302.c
> > @@ -218,11 +218,9 @@ static struct dentry *rootdir;
> >   static int fusb302_debugfs_init(struct fusb302_chip *chip)
> >   {
> >   	mutex_init(&chip->logbuffer_lock);
> > -	if (!rootdir) {
> > -		rootdir = debugfs_create_dir("fusb302", NULL);
> > -		if (!rootdir)
> > -			return -ENOMEM;
> > -	}
> 
> I think the idea here was to permit more than one instance of the driver,
> and have all debugfs file entries under the fusb302 directory. Loading the
> second instance will fail after this patch is applied.

OK, need to come up with something else for this issue then.

> > +	rootdir = debugfs_create_dir("fusb302", NULL);
> > +	if (!rootdir)
> > +		return -ENOMEM;
> >   	chip->dentry = debugfs_create_file(dev_name(chip->dev),
> >   					   S_IFREG | 0444, rootdir,
> > @@ -234,6 +232,7 @@ static int fusb302_debugfs_init(struct fusb302_chip *chip)
> >   static void fusb302_debugfs_exit(struct fusb302_chip *chip)
> >   {
> >   	debugfs_remove(chip->dentry);
> > +	debugfs_remove(rootdir); >   }
> >   #else
> > 

Thanks,

-- 
heikki

[PATCH 4/4] usb: roles: intel_xhci: Always allow user control

[Heikki Krogerus]

Trying to determine the USB port type with this mux is very
difficult. To simplify the situation, always allowing user
control, even if the port is USB Type-C port.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/roles/intel-xhci-usb-role-switch.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index 28102127b9d5..6482eee6fd45 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -43,15 +43,6 @@ struct intel_xhci_acpi_match {
 	int hrv;
 };
 
-/*
- * ACPI IDs for PMICs which do not support separate data and power role
- * detection (USB ACA detection for micro USB OTG), we allow userspace to
- * change the role manually on these.
- */
-static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
-	{ "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
-};
-
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 {
 	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
@@ -137,7 +128,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct intel_xhci_usb_data *data;
 	struct resource *res;
-	int i;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -150,10 +140,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
 	if (!data->base)
 		return -ENOMEM;
 
-	for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
-		if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
-				     allow_userspace_ctrl_ids[i].hrv))
-			sw_desc.allow_userspace_control = true;
+	sw_desc.allow_userspace_control = true;
 
 	platform_set_drvdata(pdev, data);
 
-- 
2.17.0

Re: [PATCH 4/4] usb: roles: intel_xhci: Always allow user control

[Hans de Goede]

Hi,

On 30-04-18 14:41, Heikki Krogerus wrote:
> Trying to determine the USB port type with this mux is very
> difficult. To simplify the situation, always allowing user
> control, even if the port is USB Type-C port.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > ---
>   drivers/usb/roles/intel-xhci-usb-role-switch.c | 15 +--------------
>   1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> index 28102127b9d5..6482eee6fd45 100644
> --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> @@ -43,15 +43,6 @@ struct intel_xhci_acpi_match {
>   	int hrv;
>   };
>   
> -/*
> - * ACPI IDs for PMICs which do not support separate data and power role
> - * detection (USB ACA detection for micro USB OTG), we allow userspace to
> - * change the role manually on these.
> - */
> -static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
> -	{ "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
> -};
> -
>   static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
>   {
>   	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> @@ -137,7 +128,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct intel_xhci_usb_data *data;
>   	struct resource *res;
> -	int i;
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
> @@ -150,10 +140,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
>   	if (!data->base)
>   		return -ENOMEM;
>   
> -	for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
> -		if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
> -				     allow_userspace_ctrl_ids[i].hrv))
> -			sw_desc.allow_userspace_control = true;
> +	sw_desc.allow_userspace_control = true;

It would be better to add:

	.allow_userspace_control = true,

To the initialization of the struct and also make the struct const since
we are now no longer changing it and usb_role_switch_register() accepts
it being const.

With that changed:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH 4/4] usb: roles: intel_xhci: Always allow user control

[Heikki Krogerus]

On Tue, May 01, 2018 at 11:57:55AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 30-04-18 14:41, Heikki Krogerus wrote:
> > Trying to determine the USB port type with this mux is very
> > difficult. To simplify the situation, always allowing user
> > control, even if the port is USB Type-C port.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > ---
> >   drivers/usb/roles/intel-xhci-usb-role-switch.c | 15 +--------------
> >   1 file changed, 1 insertion(+), 14 deletions(-)
> > 
> > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > index 28102127b9d5..6482eee6fd45 100644
> > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
> > @@ -43,15 +43,6 @@ struct intel_xhci_acpi_match {
> >   	int hrv;
> >   };
> > -/*
> > - * ACPI IDs for PMICs which do not support separate data and power role
> > - * detection (USB ACA detection for micro USB OTG), we allow userspace to
> > - * change the role manually on these.
> > - */
> > -static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
> > -	{ "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
> > -};
> > -
> >   static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
> >   {
> >   	struct intel_xhci_usb_data *data = dev_get_drvdata(dev);
> > @@ -137,7 +128,6 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> >   	struct device *dev = &pdev->dev;
> >   	struct intel_xhci_usb_data *data;
> >   	struct resource *res;
> > -	int i;
> >   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >   	if (!data)
> > @@ -150,10 +140,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev)
> >   	if (!data->base)
> >   		return -ENOMEM;
> > -	for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
> > -		if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
> > -				     allow_userspace_ctrl_ids[i].hrv))
> > -			sw_desc.allow_userspace_control = true;
> > +	sw_desc.allow_userspace_control = true;
> 
> It would be better to add:
> 
> 	.allow_userspace_control = true,
> 
> To the initialization of the struct and also make the struct const since
> we are now no longer changing it and usb_role_switch_register() accepts
> it being const.

Makes sense.

> With that changed:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks,

-- 
heikki