[PATCH 1/4] bq25890_charger: Rename IILIM field to IINLIM

[PATCH 1/4] bq25890_charger: Rename IILIM field to IINLIM

[Yauhen Kharuzhy]

Rename the Input Current Limit field in the REG00 from IILIM to IINLIM
accordingly with the bq2589x datasheet. This is just cosmetical change
to reduce confusion.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/power/supply/bq25890_charger.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 34ec186a2e9a..34467bfb9537 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -40,7 +40,7 @@ static const char *const bq25890_chip_name[] = {
 };
 
 enum bq25890_fields {
-	F_EN_HIZ, F_EN_ILIM, F_IILIM,				     /* Reg00 */
+	F_EN_HIZ, F_EN_ILIM, F_IINLIM,				     /* Reg00 */
 	F_BHOT, F_BCOLD, F_VINDPM_OFS,				     /* Reg01 */
 	F_CONV_START, F_CONV_RATE, F_BOOSTF, F_ICO_EN,
 	F_HVDCP_EN, F_MAXC_EN, F_FORCE_DPM, F_AUTO_DPDM_EN,	     /* Reg02 */
@@ -153,7 +153,7 @@ static const struct reg_field bq25890_reg_fields[] = {
 	/* REG00 */
 	[F_EN_HIZ]		= REG_FIELD(0x00, 7, 7),
 	[F_EN_ILIM]		= REG_FIELD(0x00, 6, 6),
-	[F_IILIM]		= REG_FIELD(0x00, 0, 5),
+	[F_IINLIM]		= REG_FIELD(0x00, 0, 5),
 	/* REG01 */
 	[F_BHOT]		= REG_FIELD(0x01, 6, 7),
 	[F_BCOLD]		= REG_FIELD(0x01, 5, 5),
@@ -256,7 +256,7 @@ enum bq25890_table_ids {
 	/* range tables */
 	TBL_ICHG,
 	TBL_ITERM,
-	TBL_IILIM,
+	TBL_IINLIM,
 	TBL_VREG,
 	TBL_BOOSTV,
 	TBL_SYSVMIN,
@@ -299,7 +299,7 @@ static const union {
 	/* TODO: BQ25896 has max ICHG 3008 mA */
 	[TBL_ICHG] =	{ .rt = {0,	  5056000, 64000} },	 /* uA */
 	[TBL_ITERM] =	{ .rt = {64000,   1024000, 64000} },	 /* uA */
-	[TBL_IILIM] =   { .rt = {100000,  3250000, 50000} },	 /* uA */
+	[TBL_IINLIM] =  { .rt = {100000,  3250000, 50000} },	 /* uA */
 	[TBL_VREG] =	{ .rt = {3840000, 4608000, 16000} },	 /* uV */
 	[TBL_BOOSTV] =	{ .rt = {4550000, 5510000, 64000} },	 /* uV */
 	[TBL_SYSVMIN] = { .rt = {3000000, 3700000, 100000} },	 /* uV */
@@ -503,11 +503,11 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
-		ret = bq25890_field_read(bq, F_IILIM);
+		ret = bq25890_field_read(bq, F_IINLIM);
 		if (ret < 0)
 			return ret;
 
-		val->intval = bq25890_find_val(ret, TBL_IILIM);
+		val->intval = bq25890_find_val(ret, TBL_IINLIM);
 		break;
 
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-- 
2.33.1

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

[Yauhen Kharuzhy]

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.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 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,
-- 
2.33.1

[PATCH 3/4] power supply bq25890-charger: Handle temperature faults

[Yauhen Kharuzhy]

Add debug info about thermal failure to message in
bq25890_get_chip_state().

Take into account possible thermal failure when calculating a
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX value as it described in
datasheet (in cold conditions a current limit is decreased to 20% or 50% of
ICHG field value depended on JEITA_ISET field).

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/power/supply/bq25890_charger.c | 33 +++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 1c43555d5bd8..fb2f1578503c 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -95,6 +95,7 @@ struct bq25890_state {
 	u8 vsys_status;
 	u8 boost_fault;
 	u8 bat_fault;
+	u8 ntc_fault;
 };
 
 struct bq25890_device {
@@ -384,6 +385,14 @@ enum bq25890_chrg_fault {
 	CHRG_FAULT_TIMER_EXPIRED,
 };
 
+enum bq25890_ntc_fault {
+	NTC_FAULT_NORMAL = 0,
+	NTC_FAULT_WARM = 2,
+	NTC_FAULT_COOL = 3,
+	NTC_FAULT_COLD = 5,
+	NTC_FAULT_HOT = 6,
+};
+
 static bool bq25890_is_adc_property(enum power_supply_property psp)
 {
 	switch (psp) {
@@ -474,7 +483,19 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		ret = bq25890_field_read(bq, F_JEITA_ISET);
+		if (ret < 0)
+			return ret;
+
 		val->intval = bq25890_find_val(bq->init_data.ichg, TBL_ICHG);
+
+		/* When temperature is too low, charge current is decreased */
+		if (bq->state.ntc_fault == NTC_FAULT_COOL) {
+			if (ret)
+				val->intval /= 5;
+			else
+				val->intval /= 2;
+		}
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
@@ -487,6 +508,10 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 
+		ret = bq25890_field_read(bq, F_JEITA_VSET);
+		if (ret < 0)
+			return ret;
+
 		/* converted_val = 2.304V + ADC_val * 20mV (table 10.3.15) */
 		val->intval = 2304000 + ret * 20000;
 		break;
@@ -550,7 +575,8 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
 		{F_VSYS_STAT,	&state->vsys_status},
 		{F_BOOST_FAULT, &state->boost_fault},
 		{F_BAT_FAULT,	&state->bat_fault},
-		{F_CHG_FAULT,	&state->chrg_fault}
+		{F_CHG_FAULT,	&state->chrg_fault},
+		{F_CHG_FAULT,	&state->ntc_fault}
 	};
 
 	for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
@@ -561,9 +587,10 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
 		*state_fields[i].data = ret;
 	}
 
-	dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT=%d/%d/%d\n",
+	dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT/NTC=%d/%d/%d/%d\n",
 		state->chrg_status, state->online, state->vsys_status,
-		state->chrg_fault, state->boost_fault, state->bat_fault);
+		state->chrg_fault, state->boost_fault, state->bat_fault,
+		state->ntc_fault);
 
 	return 0;
 }
-- 
2.33.1

[PATCH 4/4] bq25890_charger: Enable continuous conversion for ADC at charging

[Yauhen Kharuzhy]

Instead of one shot run of ADC at beginning of charging, run continuous
conversion to ensure that all charging-related values are monitored
properly (input voltage, input current, themperature etc.).

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/power/supply/bq25890_charger.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index fb2f1578503c..26ac7cde8c02 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -609,12 +609,12 @@ static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq)
 
 	if (!new_state.online && bq->state.online) {	    /* power removed */
 		/* disable ADC */
-		ret = bq25890_field_write(bq, F_CONV_START, 0);
+		ret = bq25890_field_write(bq, F_CONV_RATE, 0);
 		if (ret < 0)
 			goto error;
 	} else if (new_state.online && !bq->state.online) { /* power inserted */
 		/* enable ADC, to have control of charge current/voltage */
-		ret = bq25890_field_write(bq, F_CONV_START, 1);
+		ret = bq25890_field_write(bq, F_CONV_RATE, 1);
 		if (ret < 0)
 			goto error;
 	}
-- 
2.33.1

Re: [PATCH 1/4] bq25890_charger: Rename IILIM field to IINLIM

[Hans de Goede]

Hi,

On 11/7/21 21:19, Yauhen Kharuzhy wrote:
> Rename the Input Current Limit field in the REG00 from IILIM to IINLIM
> accordingly with the bq2589x datasheet. This is just cosmetical change
> to reduce confusion.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>

Thank you, this typo was annoying me too :)

Patch subject prefix should be: "power: supply: bq25890: "

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/bq25890_charger.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 34ec186a2e9a..34467bfb9537 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -40,7 +40,7 @@ static const char *const bq25890_chip_name[] = {
>  };
>  
>  enum bq25890_fields {
> -	F_EN_HIZ, F_EN_ILIM, F_IILIM,				     /* Reg00 */
> +	F_EN_HIZ, F_EN_ILIM, F_IINLIM,				     /* Reg00 */
>  	F_BHOT, F_BCOLD, F_VINDPM_OFS,				     /* Reg01 */
>  	F_CONV_START, F_CONV_RATE, F_BOOSTF, F_ICO_EN,
>  	F_HVDCP_EN, F_MAXC_EN, F_FORCE_DPM, F_AUTO_DPDM_EN,	     /* Reg02 */
> @@ -153,7 +153,7 @@ static const struct reg_field bq25890_reg_fields[] = {
>  	/* REG00 */
>  	[F_EN_HIZ]		= REG_FIELD(0x00, 7, 7),
>  	[F_EN_ILIM]		= REG_FIELD(0x00, 6, 6),
> -	[F_IILIM]		= REG_FIELD(0x00, 0, 5),
> +	[F_IINLIM]		= REG_FIELD(0x00, 0, 5),
>  	/* REG01 */
>  	[F_BHOT]		= REG_FIELD(0x01, 6, 7),
>  	[F_BCOLD]		= REG_FIELD(0x01, 5, 5),
> @@ -256,7 +256,7 @@ enum bq25890_table_ids {
>  	/* range tables */
>  	TBL_ICHG,
>  	TBL_ITERM,
> -	TBL_IILIM,
> +	TBL_IINLIM,
>  	TBL_VREG,
>  	TBL_BOOSTV,
>  	TBL_SYSVMIN,
> @@ -299,7 +299,7 @@ static const union {
>  	/* TODO: BQ25896 has max ICHG 3008 mA */
>  	[TBL_ICHG] =	{ .rt = {0,	  5056000, 64000} },	 /* uA */
>  	[TBL_ITERM] =	{ .rt = {64000,   1024000, 64000} },	 /* uA */
> -	[TBL_IILIM] =   { .rt = {100000,  3250000, 50000} },	 /* uA */
> +	[TBL_IINLIM] =  { .rt = {100000,  3250000, 50000} },	 /* uA */
>  	[TBL_VREG] =	{ .rt = {3840000, 4608000, 16000} },	 /* uV */
>  	[TBL_BOOSTV] =	{ .rt = {4550000, 5510000, 64000} },	 /* uV */
>  	[TBL_SYSVMIN] = { .rt = {3000000, 3700000, 100000} },	 /* uV */
> @@ -503,11 +503,11 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
>  		break;
>  
>  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> -		ret = bq25890_field_read(bq, F_IILIM);
> +		ret = bq25890_field_read(bq, F_IINLIM);
>  		if (ret < 0)
>  			return ret;
>  
> -		val->intval = bq25890_find_val(ret, TBL_IILIM);
> +		val->intval = bq25890_find_val(ret, TBL_IINLIM);
>  		break;
>  
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> 

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

Re: [PATCH 3/4] power supply bq25890-charger: Handle temperature faults

[Hans de Goede]

Hi,

On 11/7/21 21:20, Yauhen Kharuzhy wrote:
> Add debug info about thermal failure to message in
> bq25890_get_chip_state().
> 
> Take into account possible thermal failure when calculating a
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX value as it described in
> datasheet (in cold conditions a current limit is decreased to 20% or 50% of
> ICHG field value depended on JEITA_ISET field).
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>

Thank you for your patch,

Patch subject prefix should be: "power: supply: bq25890: "


> ---
>  drivers/power/supply/bq25890_charger.c | 33 +++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 1c43555d5bd8..fb2f1578503c 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -95,6 +95,7 @@ struct bq25890_state {
>  	u8 vsys_status;
>  	u8 boost_fault;
>  	u8 bat_fault;
> +	u8 ntc_fault;
>  };
>  
>  struct bq25890_device {
> @@ -384,6 +385,14 @@ enum bq25890_chrg_fault {
>  	CHRG_FAULT_TIMER_EXPIRED,
>  };
>  
> +enum bq25890_ntc_fault {
> +	NTC_FAULT_NORMAL = 0,
> +	NTC_FAULT_WARM = 2,
> +	NTC_FAULT_COOL = 3,
> +	NTC_FAULT_COLD = 5,
> +	NTC_FAULT_HOT = 6,
> +};
> +
>  static bool bq25890_is_adc_property(enum power_supply_property psp)
>  {
>  	switch (psp) {
> @@ -474,7 +483,19 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
>  		break;
>  
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		ret = bq25890_field_read(bq, F_JEITA_ISET);
> +		if (ret < 0)
> +			return ret;
> +

You only use the read value if bq->state.ntc_fault == NTC_FAULT_COOL
and i2c reads are somewhat expensive (or at least slow), can you
please move this to inside the
"if (bq->state.ntc_fault == NTC_FAULT_COOL) {" block ?

This is esp. relevant since this condition will normally be false,
so normally this will save us an i2c_read for every read of the
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX property.

>  		val->intval = bq25890_find_val(bq->init_data.ichg, TBL_ICHG);
> +
> +		/* When temperature is too low, charge current is decreased */
> +		if (bq->state.ntc_fault == NTC_FAULT_COOL) {
> +			if (ret)
> +				val->intval /= 5;
> +			else
> +				val->intval /= 2;
> +		}
>  		break;
>  
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> @@ -487,6 +508,10 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  
> +		ret = bq25890_field_read(bq, F_JEITA_VSET);
> +		if (ret < 0)
> +			return ret;
> +
>  		/* converted_val = 2.304V + ADC_val * 20mV (table 10.3.15) */
>  		val->intval = 2304000 + ret * 20000;
>  		break;
> @@ -550,7 +575,8 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
>  		{F_VSYS_STAT,	&state->vsys_status},
>  		{F_BOOST_FAULT, &state->boost_fault},
>  		{F_BAT_FAULT,	&state->bat_fault},
> -		{F_CHG_FAULT,	&state->chrg_fault}
> +		{F_CHG_FAULT,	&state->chrg_fault},
> +		{F_CHG_FAULT,	&state->ntc_fault}

Copy paste error? Surely the field in the second line must be F_NTG_FAULT ?

>  	};
>  
>  	for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
> @@ -561,9 +587,10 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
>  		*state_fields[i].data = ret;
>  	}
>  
> -	dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT=%d/%d/%d\n",
> +	dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT/NTC=%d/%d/%d/%d\n",
>  		state->chrg_status, state->online, state->vsys_status,
> -		state->chrg_fault, state->boost_fault, state->bat_fault);
> +		state->chrg_fault, state->boost_fault, state->bat_fault,
> +		state->ntc_fault);
>  
>  	return 0;
>  }
> 

Regards,

Hans

Re: [PATCH 4/4] bq25890_charger: Enable continuous conversion for ADC at charging

[Hans de Goede]

Hi,

On 11/7/21 21:20, Yauhen Kharuzhy wrote:
> Instead of one shot run of ADC at beginning of charging, run continuous
> conversion to ensure that all charging-related values are monitored
> properly (input voltage, input current, themperature etc.).
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>

Thank you for finding this! This explains why all the ADC returned
values like current_now where 0 when charging, I thought this was
just something which was only supported while not charging, heh.

As before, the patch subject prefix should be: "power: supply: bq25890: "

Otherwise the patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Sebastian, this really is a pure bug-fix patch, any chance you
can pick this up for a future 5.16-rc# pull-req ? 

Regards,

Hans




> ---
>  drivers/power/supply/bq25890_charger.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index fb2f1578503c..26ac7cde8c02 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -609,12 +609,12 @@ static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq)
>  
>  	if (!new_state.online && bq->state.online) {	    /* power removed */
>  		/* disable ADC */
> -		ret = bq25890_field_write(bq, F_CONV_START, 0);
> +		ret = bq25890_field_write(bq, F_CONV_RATE, 0);
>  		if (ret < 0)
>  			goto error;
>  	} else if (new_state.online && !bq->state.online) { /* power inserted */
>  		/* enable ADC, to have control of charge current/voltage */
> -		ret = bq25890_field_write(bq, F_CONV_START, 1);
> +		ret = bq25890_field_write(bq, F_CONV_RATE, 1);
>  		if (ret < 0)
>  			goto error;
>  	}
> 

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

[Yauhen Kharuzhy]

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

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

[Hans de Goede]

Hi,

On 11/7/21 22:08, Yauhen Kharuzhy wrote:
> 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.

Yeah 5A is bad, I find it a bit weird that drivers/usb/phy/phy.c
uses that, but my plan for X86 devices with this charger is
to not use the drivers/usb/phy/phy.c code, that is not really
a good fit for X86 devices.

For the axp288 code (found on many other X86 Cherry Trail
devices) we use 2A when detecting a DCP.

And the same for the full-featured Type-C case with a
C to A cable identified with a Pull Up resistor indicating
max 0.5, combined with BC1.2 DCP detection, then we too
set the input-current-limit to 2A.

Also I believe that the ILIM pin limits things to 2A
on the yogabook, I've seen the firmware set IINLIM to
3.25A and even with a 3A capable charger it only
draws 2A.

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

Re: [PATCH 4/4] bq25890_charger: Enable continuous conversion for ADC at charging

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Hi,

On Sun, Nov 07, 2021 at 09:48:38PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/7/21 21:20, Yauhen Kharuzhy wrote:
> > Instead of one shot run of ADC at beginning of charging, run continuous
> > conversion to ensure that all charging-related values are monitored
> > properly (input voltage, input current, themperature etc.).
> > 
> > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> 
> Thank you for finding this! This explains why all the ADC returned
> values like current_now where 0 when charging, I thought this was
> just something which was only supported while not charging, heh.
> 
> As before, the patch subject prefix should be: "power: supply: bq25890: "
> 
> Otherwise the patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Sebastian, this really is a pure bug-fix patch, any chance you
> can pick this up for a future 5.16-rc# pull-req ? 
> 
> Regards,
> 
> Hans

Thanks, queued to power-supply's fixes branch.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] bq25890_charger: Enable continuous conversion for ADC at charging

[Hans de Goede]

Hi,

On 11/15/21 16:23, Sebastian Reichel wrote:
> Hi,
> 
> On Sun, Nov 07, 2021 at 09:48:38PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/7/21 21:20, Yauhen Kharuzhy wrote:
>>> Instead of one shot run of ADC at beginning of charging, run continuous
>>> conversion to ensure that all charging-related values are monitored
>>> properly (input voltage, input current, themperature etc.).
>>>
>>> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
>>
>> Thank you for finding this! This explains why all the ADC returned
>> values like current_now where 0 when charging, I thought this was
>> just something which was only supported while not charging, heh.
>>
>> As before, the patch subject prefix should be: "power: supply: bq25890: "
>>
>> Otherwise the patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Sebastian, this really is a pure bug-fix patch, any chance you
>> can pick this up for a future 5.16-rc# pull-req ? 
>>
>> Regards,
>>
>> Hans
> 
> Thanks, queued to power-supply's fixes branch.

Thank you.

Note I also added this to my:

[PATCH v2 00/20] power-suppy/i2c/extcon: Fix charger setup on Xiaomi Mi Pad 2 and Lenovo Yogabook

Series, together with 2 of the 3 other patches from this series
(with some minor fixes applied to one of them).

So you can skip:

[PATCH v2 02/20] power: supply: bq25890: Fix ADC continuous conversion setting when charging

from that series (I reworded the commit msg subject a bit,
but it is the exact same patch).

If I end up doing a v3, I'll drop this patch.

Regards,

Hans