Re: [PATCH 12/12] extcon: axp288: Set USB role where necessary

From: Hans de Goede
Date: Sun Feb 25 2018 - 08:50:15 EST


Hi,

On 16-02-18 14:19, Andy Shevchenko wrote:
On Fri, Feb 16, 2018 at 12:47 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
The AXP288 BC1.2 charger detection / extcon code may seem like a strange
place to add code to control the USB role-switch on devices with an AXP288,
but there are 2 reasons to do this inside the axp288 extcon code:

1) On many devices the USB role is controlled by ACPI AML code, but the AML
code only switches between the host and none roles, because of Windows
not really using device mode. To make device mode work we need to toggle
between the none/device roles based on VBus presence, and the axp288
extcon gets interrupts on VBus insertion / removal.

2) In order for our BC1.2 charger detection to work properly the role
mux must be properly set to device mode before we do the detection.

Also note the Kconfig help-text / obsolete depends on USB_PHY which are
remnants from older never upstreamed code also controlling the mux from
the axp288 extcon code.

This commit also adds code to get notifications from the INT3496 extcon
device, which is used on some devices to notify the kernel about id-pin
changes instead of them being handled through AML code.

This fixes:
-Device mode not working on most CHT devices with an AXP288
-Host mode not working on devices with an INT3496 ACPI device
-Charger-type misdetection (always SDP) on devices with an INT3496 when the
USB role (always) gets initialized as host

Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>

config EXTCON_AXP288
tristate "X-Power AXP288 EXTCON support"
- depends on MFD_AXP20X && USB_PHY
+ depends on MFD_AXP20X && USB_SUPPORT
+ select USB_ROLE_SWITCH

Is it supposed to work outside of x86 world?..

No.

+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>

...if yes, this should go under CONFIG_X86 along with accompanying parts.

...if no, put corresponding dependency to Kconfig.

Ack, added the dependency for v2 of the patch-set.

+ if (info->role_sw) {
+ ret = devm_add_action_or_reset(dev, axp288_put_role_sw, info);
+ if (ret)
+ return ret;
+
+ if (acpi_dev_present("INT3496", NULL, -1)) {
+ info->id_extcon = extcon_get_extcon_dev("INT3496:00");

Please use instance found by acpi_dev_present(). Okay, actually new
helper is here:
acpi_dev_get_first_match_name().

Good call, I've switched to acpi_dev_get_first_match_name() for v2.



+ if (!info->id_extcon)
+ return -EPROBE_DEFER;
+
+ dev_info(dev, "controlling USB role\n");
+ } else {
+ dev_info(dev, "controlling USB role based on vbus presence\n");
+ }
+ }
+


Andy, Thank you for all the reviews!

Regards,

Hans