From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753551AbdDDKwp (ORCPT ); Tue, 4 Apr 2017 06:52:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10169 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752879AbdDDKwf (ORCPT ); Tue, 4 Apr 2017 06:52:35 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 205E53D945 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 205E53D945 Subject: Re: [PATCH 2/3] extcon: intel-cht-wc: Disable external 5v boost converter on probe To: Chanwoo Choi , MyungJoo Ham References: <20170403112630.30625-1-hdegoede@redhat.com> <20170403112630.30625-2-hdegoede@redhat.com> <58E33DCC.6020905@samsung.com> Cc: Takashi Iwai , linux-kernel@vger.kernel.org From: Hans de Goede Message-ID: <12b64560-0380-ced3-2231-3668f54b2a6a@redhat.com> Date: Tue, 4 Apr 2017 12:52:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <58E33DCC.6020905@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 04 Apr 2017 10:52:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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) >> > >