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

2021-04-05 Thread Sebastian Reichel 
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 
> > ---
> >  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


signature.asc
Description: PGP signature

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

2021-02-10 Thread Krzysztof Kozlowski 
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.

> 
> Fixes: 5069185fc18e ("power: supply: bq25980: Add support for the BQ259xx 
> family")
> Signed-off-by: Ricardo Rivera-Matos 
> ---
>  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.

>  
>   int busovp_sc_def;
>   int busovp_byp_def;
> @@ -73,6 +77,20 @@ struct bq25980_chip_info {
>  
>   int batocp_def;
>   int batocp_max;
> + int batocp_min;
> + int batocp_step;
> +
> + int vbus_adc_step;
> + int vbus_adc_offset;
> +
> + int ibus_adc_step;
> + int ibus_adc_offset;
> +
> + int vbat_adc_step;
> + int vbat_adc_offset;
> +
> + int ibat_adc_step;
> + int ibat_adc_offset;
>  };
>  

Does not look like related to changing offsets of register values in
header.

>  struct bq25980_init_data {
> @@ -275,13 +293,22 @@ static int bq25980_watchdog_time[BQ25980_NUM_WD_VAL] = 
> {5000, 1, 5,
>  static int bq25980_get_input_curr_lim(struct bq25980_device *bq)
>  {
>   unsigned int busocp_reg_code;
> + int offset, step;
>   int ret;
>  
> + if (bq->state.bypass) {
> + step = bq->chip_info->busocp_byp_step;
> + offset = bq->chip_info->busocp_byp_offset;
> + } else {
> + step = bq->chip_info->busocp_sc_step;
> + offset = bq->chip_info->busocp_sc_offset;
> + }
> +

Does not look like related to changing offsets of register values in
header.

and so on... Fix one thing at a time.

Best regards,
Krzysztof

[PATCH v2 1/2] power: supply: bq25980 Apply datasheet revision changes

2021-02-10 Thread Ricardo Rivera-Matos 
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.

Fixes: 5069185fc18e ("power: supply: bq25980: Add support for the BQ259xx 
family")
Signed-off-by: Ricardo Rivera-Matos 
---
 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;
 
int busovp_sc_def;
int busovp_byp_def;
@@ -73,6 +77,20 @@ struct bq25980_chip_info {
 
int batocp_def;
int batocp_max;
+   int batocp_min;
+   int batocp_step;
+
+   int vbus_adc_step;
+   int vbus_adc_offset;
+
+   int ibus_adc_step;
+   int ibus_adc_offset;
+
+   int vbat_adc_step;
+   int vbat_adc_offset;
+
+   int ibat_adc_step;
+   int ibat_adc_offset;
 };
 
 struct bq25980_init_data {
@@ -275,13 +293,22 @@ static int bq25980_watchdog_time[BQ25980_NUM_WD_VAL] = 
{5000, 1, 5,
 static int bq25980_get_input_curr_lim(struct bq25980_device *bq)
 {
unsigned int busocp_reg_code;
+   int offset, step;
int ret;
 
+   if (bq->state.bypass) {
+   step = bq->chip_info->busocp_byp_step;
+   offset = bq->chip_info->busocp_byp_offset;
+   } else {
+   step = bq->chip_info->busocp_sc_step;
+   offset = bq->chip_info->busocp_sc_offset;
+   }
+
ret = regmap_read(bq->regmap, BQ25980_BUSOCP, _reg_code);
if (ret)
return ret;
 
-   return (busocp_reg_code * BQ25980_BUSOCP_STEP_uA) + 
BQ25980_BUSOCP_OFFSET_uA;
+   return (busocp_reg_code * step) + offset;
 }
 
 static int bq25980_set_hiz(struct bq25980_device *bq, int setting)
@@ -293,6 +320,7 @@ static int bq25980_set_hiz(struct bq25980_device *bq, int 
setting)
 static int bq25980_set_input_curr_lim(struct bq25980_device *bq, int busocp)
 {
unsigned int busocp_reg_code;
+   int step, offset;
int ret;
 
if (!busocp)
@@ -303,13 +331,17 @@ static int bq25980_set_input_curr_lim(struct 
bq25980_device *bq, int busocp)
if (busocp < BQ25980_BUSOCP_MIN_uA)
busocp = BQ25980_BUSOCP_MIN_uA;
 
-   if (bq->state.bypass)
+   if (bq->state.bypass) {
busocp = min(busocp, bq->chip_info->busocp_sc_max);
-   else
+   step = bq->chip_info->busocp_byp_step;
+   offset = bq->chip_info->busocp_byp_offset;
+   } else {
busocp = min(busocp, bq->chip_info->busocp_byp_max);
+   step = bq->chip_info->busocp_sc_step;
+   offset = bq->chip_info->busocp_sc_offset;
+   }
 
-   busocp_reg_code = (busocp - BQ25980_BUSOCP_OFFSET_uA)
-   / BQ25980_BUSOCP_STEP_uA;
+   busocp_reg_code = (busocp - offset) / step;
 
ret = regmap_write(bq->regmap, BQ25980_BUSOCP, busocp_reg_code);
if (ret)
@@ -374,6 +406,7 @@ static int bq25980_set_input_volt_lim(struct bq25980_device 
*bq, int busovp)
 
 static int bq25980_get_const_charge_curr(struct bq25980_device *bq)
 {
+   int step = bq->chip_info->batocp_step;
unsigned int batocp_reg_code;
int ret;
 
@@ -381,19 +414,20 @@ static int bq25980_get_const_charge_curr(struct 
bq25980_device *bq)
if (ret)
return ret;
 
-   return (batocp_reg_code & BQ25980_BATOCP_MASK) *
-   BQ25980_BATOCP_STEP_uA;
+   return (batocp_reg_code & BQ25980_BATOCP_MASK) * step;
 }
 
 static int bq25980_set_const_charge_curr(struct bq25980_device *bq, int batocp)
 {
+   int step = bq->chip_info->batocp_step;
+   int max = bq->chip_info->batocp_max;
+   int min = bq->chip_info->batocp_min;
unsigned int batocp_reg_code;
int ret;
 
-   batocp = max(batocp, BQ25980_BATOCP_MIN_uA);
-   batocp = min(batocp, bq->chip_info->batocp_max);
+   batocp = clamp(batocp, min, max);
 
-