linux-kernel - Re: [PATCH v2 1/2] power: supply: bq25980 Apply datasheet revision changes

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: <20210405160947.l2i2yl6gbihbmv3g@earth.universe>
Date:   Mon, 5 Apr 2021 18:09:47 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Ricardo Rivera-Matos <r-rivera-matos@...com>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] power: supply: bq25980 Apply datasheet revision
 changes

Hi,

On Thu, Feb 11, 2021 at 08:36:03AM +0100, Krzysztof Kozlowski wrote:
> On Wed, Feb 10, 2021 at 04:56:45PM -0600, Ricardo Rivera-Matos wrote:
> > The latest datasheet revision for BQ25980, BQ25975, and BQ25960 changed
> > 
> > various register step sizes and offset values. 
> > 
> > This patch changes the following header file
> > 
> > values for POWER_SUPPLY_PROP_CURRENT_NOW, 
> > 
> > POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> > 
> > POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> > 
> > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > 
> > POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT, and POWER_SUPPLY_PROP_VOLTAGE_NOW.
> > 
> > Additionally, this patch adjusts bq25980_get_input_curr_lim(),
> > 
> > bq25980_set_input_curr_lim(), bq25980_get_const_charge_curr(), and
> > 
> > bq25980_set_const_charge_curr() to perform the get/set math correctly.
> 
> Your formatting is so odd, it is not readable. Please open "git log" and
> try to write something similar to existing commits, e.g. without
> additional blank line between lines.

Ack. This is a paint to read.

> 
> > 
> > Fixes: 5069185fc18e ("power: supply: bq25980: Add support for the BQ259xx family")
> > Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@...com>
> > ---
> >  drivers/power/supply/bq25980_charger.c | 141 ++++++++++++++++++++-----
> >  drivers/power/supply/bq25980_charger.h |  77 ++++++++++----
> >  2 files changed, 173 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq25980_charger.c b/drivers/power/supply/bq25980_charger.c
> > index 530ff4025b31..7c489a9e8877 100644
> > --- a/drivers/power/supply/bq25980_charger.c
> > +++ b/drivers/power/supply/bq25980_charger.c
> > @@ -52,6 +52,10 @@ struct bq25980_chip_info {
> >  	int busocp_byp_max;
> >  	int busocp_sc_min;
> >  	int busocp_byp_min;
> > +	int busocp_sc_step;
> > +	int busocp_byp_step;
> > +	int busocp_sc_offset;
> > +	int busocp_byp_offset;
> 
> Does not look like related to changing offsets of register values in
> header.

well looking at the change as a whole the problem is that each
chip has different offset/step size now. Arguably this is an
atomic change. I would have queued it (fixing the commit message
while applying), but the patch also does some unrelated changes:

 - introduced usage of clamp instead of min()/max() checks
   (which is great, but should be separate commit!)
 - rename variables to something more sensible in
   bq25980_get_adc_vbat (again a great change, but should be
   separate commit)

Ricardo, please submit a new version with these changes splitted
out.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)