Messages in this thread | | | Subject | Re: [PATCH 2/3] extcon: intel-cht-wc: Disable external 5v boost converter on probe | From | Hans de Goede <> | Date | Tue, 4 Apr 2017 12:52:32 +0200 |
| |
Hi,
On 04-04-17 08:31, Chanwoo Choi wrote: > Hi, > > On 2017년 04월 03일 20:26, Hans de Goede wrote: >> Disable the 5v boost converter on probe in case it was left on by >> the BIOS, this fixes 2 problems: >> >> 1) This gets seen by the external battery charger as a valid Vbus >> supply and it then tries to feed Vsys from this creating a >> feedback loop which causes aprox. 300 mA extra battery drain >> (and unless we drive the external-charger-disable pin high it >> also tries to charge the battery causing even more feedback). >> 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply >> >> Since the external battery charger has its own 5v boost converter >> which does not have these issues, we simply turn the separate >> external 5v boost converter off and leave it off entirely. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/extcon/extcon-intel-cht-wc.c | 38 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c >> index f1c43af..8651f63 100644 >> --- a/drivers/extcon/extcon-intel-cht-wc.c >> +++ b/drivers/extcon/extcon-intel-cht-wc.c >> @@ -64,6 +64,11 @@ >> #define CHT_WC_PWRSRC_ID_GND BIT(3) >> #define CHT_WC_PWRSRC_ID_FLOAT BIT(4) >> >> +#define CHT_WC_GPIO0_CTLO 0x6e2b >> +#define CHT_WC_GPIO_CTLO_OUTPUT BIT(0) > > CHT_WC_GPIO0_CTLO or CHT_WC_GPIO_CTLO ? > Because CHT_WC_GPIO_CTLO_OUTPUT defintion use just 'GPIO' instead of 'GPIO0'. > I prefer to change the definition name as following with '_MASK' postfix.
All the other BIT definitions don't use _MASK either so that would be inconsistent.
> - CHT_WC_GPIO_CTLO_OUTPUT -> CHT_WC_GPIO_CTLO_OUTPUT_MASK > > >> +#define CHT_WC_GPIO0_CTLO 0x6e2b >> + >> +#define CHT_WC_VBUS_ENABLE_GPIO 2 > > I think that 'CHT_WC_GPIO0_CTLO + CHT_WC_VBUS_ENABLE_GPIO' is a separate > register address. Maybe you can define the separate register offset > instead of the separate the CHT_WC_VBUS_ENABLE_GPIO offset.
Ok, I will send you a version 2 using:
#define CHT_WC_VBUS_GPIO_CTLO 0x6e2d #define CHT_WC_VBUS_GPIO_CTLO_OUTPUT BIT(0)
Regards,
Hans
> >> + >> enum cht_wc_usb_id { >> USB_ID_OTG, >> USB_ID_GND, >> @@ -170,6 +175,24 @@ static void cht_wc_extcon_set_phymux(struct cht_wc_extcon_data *ext, u8 state) >> dev_err(ext->dev, "Error writing phyctrl: %d\n", ret); >> } >> >> +static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext, >> + bool enable) >> +{ >> + int ret, val; >> + >> + val = enable ? CHT_WC_GPIO_CTLO_OUTPUT : 0; >> + >> + /* >> + * The 5V boost converter is enabled through a gpio on the PMIC, since >> + * there currently is no gpio driver we access the gpio reg directly. >> + */ >> + ret = regmap_update_bits(ext->regmap, >> + CHT_WC_GPIO0_CTLO + CHT_WC_VBUS_ENABLE_GPIO, >> + CHT_WC_GPIO_CTLO_OUTPUT, val); >> + if (ret) >> + dev_err(ext->dev, "Error writing Vbus GPIO CTLO: %d\n", ret); >> +} >> + >> /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */ >> static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext, >> unsigned int cable, bool state) >> @@ -280,6 +303,21 @@ static int cht_wc_extcon_probe(struct platform_device *pdev) >> if (IS_ERR(ext->edev)) >> return PTR_ERR(ext->edev); >> >> + /* >> + * When a host-cable is detected the BIOS enables an external 5v boost >> + * converter to power connected devices there are 2 problems with this: >> + * 1) This gets seen by the external battery charger as a valid Vbus >> + * supply and it then tries to feed Vsys from this creating a >> + * feedback loop which causes aprox. 300 mA extra battery drain >> + * (and unless we drive the external-charger-disable pin high it >> + * also tries to charge the battery causing even more feedback). >> + * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply >> + * Since the external battery charger has its own 5v boost converter >> + * which does not have these issues, we simply turn the separate >> + * external 5v boost converter off and leave it off entirely. >> + */ >> + cht_wc_extcon_set_5v_boost(ext, false); >> + >> /* Enable sw control */ >> ret = cht_wc_extcon_sw_control(ext, true); >> if (ret) >> > >
| |