lkml.org 
[lkml]   [2017]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] extcon: intel-cht-wc: Disable external 5v boost converter on probe
From
Date
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)
>>
>
>

\
 
 \ /
  Last update: 2017-04-04 12:53    [W:0.053 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site