Re: [PATCH 2/4] bq25890: Add max input current limit property

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/7/21 22:08, Yauhen Kharuzhy wrote:
> On Sun, Nov 07, 2021 at 09:41:55PM +0100, Hans de Goede wrote:
>> Hi Yauhen,
>>
>> On 11/7/21 21:19, Yauhen Kharuzhy wrote:
>>> Add property 'ti,input-max-current' to define input current limit if
>>> needed. It will be applied if automatic charger type detection is
>>> disabled and using of ILIM pin is disabled or such pin defines greater
>>> limit than IINLIM field.
>>
>> Sorry, but this makes no sense, as the datasheet says the charger
>> itself updates iinlim dynamically when it has done charger-type
>> detection (for the bq25890 version) and for the bq25892 version
>> which does not have the D+ + D- USB pins for BC1.2 detection,
>> the iinlim should be updated based on the charger detection
>> done elsewhere (by the Whiskey Cove PMIC in case of the Yoga Book).
> 
> For Yoga Book, charger detection in the bq25892 is disabled and done in
> the extcon driver.
> 
>> My plan for this is to have drivers/extcon/extcon-intel-cht-wc.c
>> also register a power_supply device which models the detected
>> charger / negotiated external charger/power-brick settings and
>> which is the supplier of the bq25892 charger.
>>
>> Then an external_power_changed handler can be added to the
>> bq25892_charger code using the
>> power_supply_set_input_current_limit_from_supplier()
>> helper to dynamically set iinlim based on the detected
>> "power-brick"/external-charger.
>>
>> This is also how this is done for (X86) devices with an
>> full-featured USB Type-C port where this needs to be handled
>> by the kernel (rather then it being done in firmware) in
>> this case the current-max property of the Type-C power-supply
>> class device gets set either based on the Type-C pull-up
>> resistor in the charger (setting 0.5A / 1.5A / 3A), with
>> a fallback to BC1.2 for the 0.5A case, or based on the
>> USB-PD negotiated max-current.
> 
> Agree, looks reasonable.
> 
>>
>> Since we will need this mechanism to dynamically set
>> iinlim based on the PMIC-s charger-detection it seems
>> to me that setting it at boot is both unnecessary and a bad
>> idea, since we don't know the correct value to set at boot.
>>
>> The extcon code will start a charger-detection cycle
>> as soon as it loads (if there is Vbus present) and then
>> trigger the external_power_changed handler .
>>
>> TL;DR: I don't really see a need for this ?
> 
> Hmm... I think you are rigth. The only case when such property can be
> needed – if the device may be damaged by maximum current supported by
> charging source. I use it to limit current by 2A when original Lenovo adapter
> is connected because the linux kernel has default max current limit
> 5A for DCP (drivers/usb/phy/phy.c) but adapter supports only 2A.

Yeah 5A is bad, I find it a bit weird that drivers/usb/phy/phy.c
uses that, but my plan for X86 devices with this charger is
to not use the drivers/usb/phy/phy.c code, that is not really
a good fit for X86 devices.

For the axp288 code (found on many other X86 Cherry Trail
devices) we use 2A when detecting a DCP.

And the same for the full-featured Type-C case with a
C to A cable identified with a Pull Up resistor indicating
max 0.5, combined with BC1.2 DCP detection, then we too
set the input-current-limit to 2A.

Also I believe that the ILIM pin limits things to 2A
on the yogabook, I've seen the firmware set IINLIM to
3.25A and even with a 3A capable charger it only
draws 2A.

Regards,

Hans



>>>  drivers/power/supply/bq25890_charger.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>>> index 34467bfb9537..1c43555d5bd8 100644
>>> --- a/drivers/power/supply/bq25890_charger.c
>>> +++ b/drivers/power/supply/bq25890_charger.c
>>> @@ -85,6 +85,7 @@ struct bq25890_init_data {
>>>  	u8 treg;	/* thermal regulation threshold */
>>>  	u8 rbatcomp;	/* IBAT sense resistor value    */
>>>  	u8 vclamp;	/* IBAT compensation voltage limit */
>>> +	u8 iinlim_max;	/* maximum input current limit allowed */
>>>  };
>>>  
>>>  struct bq25890_state {
>>> @@ -657,6 +658,7 @@ static int bq25890_hw_init(struct bq25890_device *bq)
>>>  		{F_TREG,	 bq->init_data.treg},
>>>  		{F_BATCMP,	 bq->init_data.rbatcomp},
>>>  		{F_VCLAMP,	 bq->init_data.vclamp},
>>> +		{F_IINLIM,	 bq->init_data.iinlim_max},
>>>  	};
>>>  
>>>  	ret = bq25890_chip_reset(bq);
>>> @@ -870,11 +872,13 @@ static int bq25890_fw_read_u32_props(struct bq25890_device *bq)
>>>  		{"ti,thermal-regulation-threshold", true, TBL_TREG, &init->treg},
>>>  		{"ti,ibatcomp-micro-ohms", true, TBL_RBATCOMP, &init->rbatcomp},
>>>  		{"ti,ibatcomp-clamp-microvolt", true, TBL_VBATCOMP, &init->vclamp},
>>> +		{"ti,input-max-current", true, TBL_IINLIM, &init->iinlim_max},
>>>  	};
>>>  
>>>  	/* initialize data for optional properties */
>>>  	init->treg = 3; /* 120 degrees Celsius */
>>>  	init->rbatcomp = init->vclamp = 0; /* IBAT compensation disabled */
>>> +	init->iinlim_max = 0x3f;
>>>  
>>>  	for (i = 0; i < ARRAY_SIZE(props); i++) {
>>>  		ret = device_property_read_u32(bq->dev, props[i].name,
>>>
>>
>