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

[Hans de Goede]

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).

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.

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 ?

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,
>