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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YYhAUH2nvj1b0uCW@jeknote.loshitsa1.net>
Date:   Mon, 8 Nov 2021 00:08:32 +0300
From:   Yauhen Kharuzhy <jekhor@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] bq25890: Add max input current limit property

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.

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

-- 
Yauhen Kharuzhy