Re: [PATCH] bq25890_charger.c : add the BQ25896 part

From: Angus Ainslie
Date: Wed Jul 25 2018 - 08:17:49 EST

Hi Krzysztof,

On 2018-07-25 03:58, Krzysztof Kozlowski wrote:
On 23 July 2018 at 15:51, Angus Ainslie <angus@xxxxxxxx> wrote:
Add some debugging to be able to check the proper initialization
of the BQ25896 part.

Hi,

This should be split into separate patchset. Do not mix two features
in one commit.


Ok, I'll take it apart

Enable the BQ25896 part.

Add 2 new parameters "voltage_now" and "model_name".

Signed-off-by: Angus Ainslie <angus.ainslie@xxxxxxx>

Your signed-off-by does not match From address.


That was intentional as I wanted Purism to get credit for it. I'm guessing that's not the correct way of doing it.

---
drivers/power/supply/bq25890_charger.c | 68 ++++++++++++++++++++++----
1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 8e2c41ded171..32cdae15ce40 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -32,6 +32,7 @@
#define BQ25890_IRQ_PIN "bq25890_irq"

#define BQ25890_ID 3
+#define BQ25896_ID 0

enum bq25890_fields {
F_EN_HIZ, F_EN_ILIM, F_IILIM, /* Reg00 */
@@ -153,8 +154,8 @@ static const struct reg_field bq25890_reg_fields[] = {
[F_CONV_RATE] = REG_FIELD(0x02, 6, 6),
[F_BOOSTF] = REG_FIELD(0x02, 5, 5),
[F_ICO_EN] = REG_FIELD(0x02, 4, 4),
- [F_HVDCP_EN] = REG_FIELD(0x02, 3, 3),
- [F_MAXC_EN] = REG_FIELD(0x02, 2, 2),
+ [F_HVDCP_EN] = REG_FIELD(0x02, 3, 3), // reserved on BQ25896
+ [F_MAXC_EN] = REG_FIELD(0x02, 2, 2), // reserved on BQ25896
[F_FORCE_DPM] = REG_FIELD(0x02, 1, 1),
[F_AUTO_DPDM_EN] = REG_FIELD(0x02, 0, 0),
/* REG03 */
@@ -163,6 +164,7 @@ static const struct reg_field bq25890_reg_fields[] = {
[F_OTG_CFG] = REG_FIELD(0x03, 5, 5),
[F_CHG_CFG] = REG_FIELD(0x03, 4, 4),
[F_SYSVMIN] = REG_FIELD(0x03, 1, 3),
+ /* MIN_VBAT_SEL on BQ25896 */
/* REG04 */
[F_PUMPX_EN] = REG_FIELD(0x04, 7, 7),
[F_ICHG] = REG_FIELD(0x04, 0, 6),
@@ -181,7 +183,7 @@ static const struct reg_field bq25890_reg_fields[] = {
[F_CHG_TMR] = REG_FIELD(0x07, 1, 2),
[F_JEITA_ISET] = REG_FIELD(0x07, 0, 0),
/* REG08 */
- [F_BATCMP] = REG_FIELD(0x08, 6, 7),
+ [F_BATCMP] = REG_FIELD(0x08, 6, 7), // 5-7 on BQ25896

So this field is different on BQ25896...


The BATCMP field is not currently used by the driver but it is reference by "bq25890_tables". Should I remove that reference or make a special case for something that isn't used ?

[F_VCLAMP] = REG_FIELD(0x08, 2, 4),
[F_TREG] = REG_FIELD(0x08, 0, 1),
/* REG09 */
@@ -195,12 +197,13 @@ static const struct reg_field bq25890_reg_fields[] = {
[F_PUMPX_DN] = REG_FIELD(0x09, 0, 0),
/* REG0A */
[F_BOOSTV] = REG_FIELD(0x0A, 4, 7),
+ /* PFM_OTG_DIS 3 on BQ25896 */
[F_BOOSTI] = REG_FIELD(0x0A, 0, 2),
/* REG0B */
[F_VBUS_STAT] = REG_FIELD(0x0B, 5, 7),
[F_CHG_STAT] = REG_FIELD(0x0B, 3, 4),
[F_PG_STAT] = REG_FIELD(0x0B, 2, 2),
- [F_SDP_STAT] = REG_FIELD(0x0B, 1, 1),
+ [F_SDP_STAT] = REG_FIELD(0x0B, 1, 1), // reserved on BQ25896
[F_VSYS_STAT] = REG_FIELD(0x0B, 0, 0),
/* REG0C */
[F_WD_FAULT] = REG_FIELD(0x0C, 7, 7),
@@ -401,6 +404,18 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
val->strval = BQ25890_MANUFACTURER;
break;

+ case POWER_SUPPLY_PROP_MODEL_NAME:
+ bq->chip_id = bq25890_field_read(bq, F_PN);

Why do you need to read it again?


Right, it should be set in probe.

+
+ if (bq->chip_id == BQ25890_ID)
+ val->strval = "BQ25890";
+ else if (bq->chip_id == BQ25896_ID)
+ val->strval = "BQ25896";
+ else
+ val->strval = "UNKNOWN";
+
+ break;
+
case POWER_SUPPLY_PROP_ONLINE:
val->intval = state.online;
break;
@@ -453,6 +468,20 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
val->intval = bq25890_find_val(bq->init_data.iterm, TBL_ITERM);
break;

+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ if (!state.online) {
+ val->intval = 0;
+ break;
+ }

This looks like unrelated change. Please split it.


Ok

+
+ ret = bq25890_field_read(bq, F_SYSV); /* read measured value */
+ if (ret < 0)
+ return ret;
+
+ /* converted_val = 2.304V + ADC_val * 20mV (table 10.3.15) */
+ val->intval = 2304000 + ret * 20000;
+ break;
+
default:
return -EINVAL;
}
@@ -608,30 +637,43 @@ static int bq25890_hw_init(struct bq25890_device *bq)
};

ret = bq25890_chip_reset(bq);
- if (ret < 0)
+ if (ret < 0) {
+ dev_dbg(bq->dev, "BQ259x reset failed %d\n", ret);

"BQ259x" prefix is not needed.


Ok

[snip]


Document the compatible. If you run checkpatch.... it would point it.

Sorry, I'll fix all the checkpatch errors.


Best regards,
Krzysztof

Cheers
Angus