Messages in this thread | | | Date | Sun, 7 Nov 2021 21:41:55 +0100 | Subject | Re: [PATCH 2/4] bq25890: Add max input current limit property | From | 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, >
| |