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

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

[Hans de Goede]

Hi All,

This is version 2 of my series previously titled:
"[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC
without USB-PD support".

So far almost all the kernel code surrounding the Cherry Trail Whiskey Cove
PMIC has been developed on the GPD win / pocket devices and it has various
assumption based on that. In the mean time I've learned (and gotten access
to) about 2 more designs and none of the 3 now known designs use a single
standard setup for the charger, fuel-gauge and other chips surrounding the
PMIC / charging+data USB port:

1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
but the Pocket re-uses the GPD Win's design in a different housing:

The WC PMIC is connected to a TI BQ24292i charger, paired with
a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
a PI3USB30532 USB switch, for a fully functional Type-C port.

2. The Xiaomi Mi Pad 2:

The WC PMIC is connected to a TI BQ25890 charger, paired with
a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
detection, for a USB-2 only Type-C port without PD.

3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:

The WC PMIC is connected to a TI BQ25892 charger, paired with
a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
detection and using the BQ25892's Mediatek Pump Express+ (1.0)

###

Unlike what is normal on X86 this diversity in designs is not handled /
abstracted away by the ACPI tables.

This series takes care of making sure that charging and device/host mode
switching also works on the Xiaomi Mi Pad 2 and the Lenovo Yogabook.

New in version 2 of this patch-set:
- This is all about Whiskey Cove based designs, instead of going roundabout
  and (ab)using drivers/platform/x86/touchscreen_dmi.c to add
  device-properties on the WC I2C-dev and then check for that, just add a new
  intel_cht_wc_get_model() helper to the intel_soc_pmic_chtwc.c MFD-driver.
- Extend the series to not only fix things on the Mi Pad 2, but also on the
  Lenovo Yogabook YB1-X90*/-X91* models.

Patches  1-13: Prepare the bq25890 power_supply driver to fully support
               the Mi Pad 2 and the Yogabook
               Note this includes a new version of the 3 bq25890 cleanup /
               fixes patches send earlier by Yauhen Kharuzhy
Patch 14:      Adds the intel_cht_wc_get_model() helper
Patch 15:      Uses this intel_cht_wc_get_model() value to instantiate an
               i2c-client with the right type and properties for the charger
               IC used on the board (instead of harcoding the GPD values)
Patches 16-20: Modify the extcon code to provide charger-detection results
               to the charger driver and to take care of the Vbus boost
               regulator control (for host-mode) and device/host mode
               switching

I've tried to keep the power_supply patches as generic as possible while
focussing some of the special handling these boards need in the
WC PMIC MFD and cell drivers, which will only get loaded on these boards.

Since some of the later patches depend on some of the power_supply changes;
and since the Whiskey Cove MFD and cell drivers generally do not see much
changes I believe that it would be best to merge the entire series through
Sebastian's linux-power-supply tree.

Lee, Wolfram and Chanwoo, may we please have your Ack for merging this
entire series through Sebastian's linux-power-supply tree?

Regards,

Hans


Hans de Goede (16):
  power: supply: core: Refactor
    power_supply_set_input_current_limit_from_supplier()
  power: supply: bq25890: Add a bq25890_rw_init_data() helper
  power: supply: bq25890: Add support to skip reset at probe() /
    remove()
  power: supply: bq25890: Add support to read back the settings from the
    chip
  power: supply: bq25890: Enable charging on boards where we skip reset
  power: supply: bq25890: Drop dev->platform_data == NULL check
  power: supply: bq25890: Add bq25890_set_otg_cfg() helper
  power: supply: bq25890: Add support for registering the Vbus boost
    converter as a regulator
  power: supply: bq25890: On the bq25892 set the IINLIM based on
    external charger detection
  mfd: intel_soc_pmic_chtwc: Add intel_cht_wc_get_model() helper
    function
  i2c: cht-wc: Make charger i2c-client instantiation board/device-model
    specific
  extcon: intel-cht-wc: Use new intel_cht_wc_get_model() helper
  extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C
    connectors
  extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()
  extcon: intel-cht-wc: Add support for registering a power_supply
    class-device
  extcon: intel-cht-wc: Report RID_A for ACA adapters

Yauhen Kharuzhy (4):
  power: supply: bq25890: Fix ADC continuous conversion setting when
    charging
  power: supply: bq25890: Rename IILIM field to IINLIM
  power: supply: bq25890: Reduce reported CONSTANT_CHARGE_CURRENT_MAX
    for low temperatures
  power: supply: bq25890: Support higher charging voltages through Pump
    Express+ protocol

 drivers/extcon/Kconfig                   |   3 +-
 drivers/extcon/extcon-intel-cht-wc.c     | 242 +++++++++++++--
 drivers/i2c/busses/i2c-cht-wc.c          | 120 ++++++--
 drivers/mfd/intel_soc_pmic_chtwc.c       |  46 +++
 drivers/power/supply/bq24190_charger.c   |  10 +-
 drivers/power/supply/bq25890_charger.c   | 374 +++++++++++++++++++----
 drivers/power/supply/power_supply_core.c |  57 ++--
 include/linux/mfd/intel_soc_pmic.h       |   9 +
 include/linux/power/bq25890_charger.h    |  15 +
 include/linux/power_supply.h             |   5 +-
 10 files changed, 744 insertions(+), 137 deletions(-)
 create mode 100644 include/linux/power/bq25890_charger.h

-- 
2.31.1

[PATCH v2 01/20] power: supply: core: Refactor power_supply_set_input_current_limit_from_supplier()

[Hans de Goede]

Some (USB) charger ICs have variants with USB D+ and D- pins to do their
own builtin charger-type detection, like e.g. the bq24190 and bq25890 and
also variants which lack this functionality, e.g. the bq24192 and bq25892.

In case the charger-type; and thus the input-current-limit detection is
done outside the charger IC then we need some way to communicate this to
the charger IC. In the past extcon was used for this, but if the external
detection does e.g. full USB PD negotiation then the extcon cable-types do
not convey enough information.

For these setups it was decided to model the external charging "brick"
and the parameters negotiated with it as a power_supply class-device
itself; and power_supply_set_input_current_limit_from_supplier() was
introduced to allow drivers to get the input-current-limit this way.

But in some cases psy drivers may want to know other properties, e.g. the
bq25892 can do "quick-charge" negotiation by pulsing its current draw,
but this should only be done if the usb_type psy-property of its supplier
is set to DCP (and device-properties indicate the board allows higher
voltages).

Instead of adding extra helper functions for each property which
a psy-driver wants to query from its supplier, refactor
power_supply_set_input_current_limit_from_supplier() into a
more generic power_supply_get_property_from_supplier() function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c   | 10 ++++-
 drivers/power/supply/power_supply_core.c | 57 +++++++++++++-----------
 include/linux/power_supply.h             |  5 ++-
 3 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 35ff0c8fe96f..0c3d5caaef0c 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1206,8 +1206,16 @@ static void bq24190_input_current_limit_work(struct work_struct *work)
 	struct bq24190_dev_info *bdi =
 		container_of(work, struct bq24190_dev_info,
 			     input_current_limit_work.work);
+	union power_supply_propval val;
+	int ret;
 
-	power_supply_set_input_current_limit_from_supplier(bdi->charger);
+	ret = power_supply_get_property_from_supplier(bdi->charger,
+						      POWER_SUPPLY_PROP_CURRENT_MAX,
+						      &val);
+	if (ret == 0)
+		bq24190_charger_set_property(bdi->charger,
+					     POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+					     &val);
 }
 
 /* Sync the input-current-limit with our parent supply (if we have one) */
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index fc12a4f407f4..fb956a9a1827 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -375,46 +375,49 @@ int power_supply_is_system_supplied(void)
 }
 EXPORT_SYMBOL_GPL(power_supply_is_system_supplied);
 
-static int __power_supply_get_supplier_max_current(struct device *dev,
-						   void *data)
+struct psy_get_supplier_prop_data {
+	struct power_supply *psy;
+	enum power_supply_property psp;
+	union power_supply_propval *val;
+};
+
+static int __power_supply_get_supplier_property(struct device *dev, void *_data)
 {
-	union power_supply_propval ret = {0,};
 	struct power_supply *epsy = dev_get_drvdata(dev);
-	struct power_supply *psy = data;
+	struct psy_get_supplier_prop_data *data = _data;
 
-	if (__power_supply_is_supplied_by(epsy, psy))
-		if (!epsy->desc->get_property(epsy,
-					      POWER_SUPPLY_PROP_CURRENT_MAX,
-					      &ret))
-			return ret.intval;
+	if (__power_supply_is_supplied_by(epsy, data->psy))
+		if (!epsy->desc->get_property(epsy, data->psp, data->val))
+			return 1; /* Success */
 
-	return 0;
+	return 0; /* Continue iterating */
 }
 
-int power_supply_set_input_current_limit_from_supplier(struct power_supply *psy)
+int power_supply_get_property_from_supplier(struct power_supply *psy,
+					    enum power_supply_property psp,
+					    union power_supply_propval *val)
 {
-	union power_supply_propval val = {0,};
-	int curr;
-
-	if (!psy->desc->set_property)
-		return -EINVAL;
+	struct psy_get_supplier_prop_data data = {
+		.psy = psy,
+		.psp = psp,
+		.val = val,
+	};
+	int ret;
 
 	/*
 	 * This function is not intended for use with a supply with multiple
-	 * suppliers, we simply pick the first supply to report a non 0
-	 * max-current.
+	 * suppliers, we simply pick the first supply to report the psp.
 	 */
-	curr = class_for_each_device(power_supply_class, NULL, psy,
-				      __power_supply_get_supplier_max_current);
-	if (curr <= 0)
-		return (curr == 0) ? -ENODEV : curr;
-
-	val.intval = curr;
+	ret = class_for_each_device(power_supply_class, NULL, &data,
+				    __power_supply_get_supplier_property);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -ENODEV;
 
-	return psy->desc->set_property(psy,
-				POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val);
+	return 0;
 }
-EXPORT_SYMBOL_GPL(power_supply_set_input_current_limit_from_supplier);
+EXPORT_SYMBOL_GPL(power_supply_get_property_from_supplier);
 
 int power_supply_set_battery_charged(struct power_supply *psy)
 {
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 9ca1f120a211..0735b8963e0a 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -420,8 +420,9 @@ power_supply_temp2resist_simple(struct power_supply_resistance_temp_table *table
 				int table_len, int temp);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
-extern int power_supply_set_input_current_limit_from_supplier(
-					 struct power_supply *psy);
+int power_supply_get_property_from_supplier(struct power_supply *psy,
+					    enum power_supply_property psp,
+					    union power_supply_propval *val);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
 
 #ifdef CONFIG_POWER_SUPPLY
-- 
2.31.1

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

[Hans de Goede]

From: Yauhen Kharuzhy <jekhor@gmail.com>

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.).

Fixes: 4aeae9cb0dad ("power_supply: Add support for TI BQ25890 charger chip")
Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.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 34ec186a2e9a..b7eac5428083 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -581,12 +581,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.31.1

[PATCH v2 03/20] power: supply: bq25890: Rename IILIM field to IINLIM

[Hans de Goede]

From: Yauhen Kharuzhy <jekhor@gmail.com>

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>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.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 b7eac5428083..b208cc2193b8 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.31.1

[PATCH v2 04/20] power: supply: bq25890: Reduce reported CONSTANT_CHARGE_CURRENT_MAX for low temperatures

[Hans de Goede]

From: Yauhen Kharuzhy <jekhor@gmail.com>

Take into account possible current reduction due to low-temperature when
reading POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX. As described in
the datasheet in cool (0-20° Celcius) conditions the current limit is
decreased to 20% or 50% of ICHG field value depended on JEITA_ISET field.

Also add NTC_FAULT field value to the debug message in
bq25890_get_chip_state().

Changed by Hans de Goede:
- Fix reading F_CHG_FAULT instead of F_NTC_FIELD for state->ntc_fault
- Only read JEITA_ISET field if necessary
- Tweak commit message a bit

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.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 b208cc2193b8..617a653221ab 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -94,6 +94,7 @@ struct bq25890_state {
 	u8 vsys_status;
 	u8 boost_fault;
 	u8 bat_fault;
+	u8 ntc_fault;
 };
 
 struct bq25890_device {
@@ -383,6 +384,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,6 +483,18 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		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) {
+			ret = bq25890_field_read(bq, F_JEITA_ISET);
+			if (ret < 0)
+				return ret;
+
+			if (ret)
+				val->intval /= 5;
+			else
+				val->intval /= 2;
+		}
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
@@ -486,6 +507,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;
@@ -549,7 +574,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_NTC_FAULT,	&state->ntc_fault}
 	};
 
 	for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
@@ -560,9 +586,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.31.1

[PATCH v2 05/20] power: supply: bq25890: Add a bq25890_rw_init_data() helper

[Hans de Goede]

On most X86/ACPI devices there is no devicetree to supply the necessary
init-data. Instead the firmware already fully initializes the bq25890
charger at boot.

Factor out the current code to write all the init_data from devicetree
into a new bq25890_rw_init_data() helper which can both write the data
to the charger (the current behavior) as well as read it back from
the charger into the init_data struct.

This is a preparation patch for adding support for X86/ACPI device's
where the init_data must be read back from the bq25890 charger.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 62 +++++++++++++++++---------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 617a653221ab..704cc15b4403 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -663,29 +663,52 @@ static int bq25890_chip_reset(struct bq25890_device *bq)
 	return 0;
 }
 
-static int bq25890_hw_init(struct bq25890_device *bq)
+static int bq25890_rw_init_data(struct bq25890_device *bq)
 {
+	bool write = true;
 	int ret;
 	int i;
 
 	const struct {
 		enum bq25890_fields id;
-		u32 value;
+		u8 *value;
 	} init_data[] = {
-		{F_ICHG,	 bq->init_data.ichg},
-		{F_VREG,	 bq->init_data.vreg},
-		{F_ITERM,	 bq->init_data.iterm},
-		{F_IPRECHG,	 bq->init_data.iprechg},
-		{F_SYSVMIN,	 bq->init_data.sysvmin},
-		{F_BOOSTV,	 bq->init_data.boostv},
-		{F_BOOSTI,	 bq->init_data.boosti},
-		{F_BOOSTF,	 bq->init_data.boostf},
-		{F_EN_ILIM,	 bq->init_data.ilim_en},
-		{F_TREG,	 bq->init_data.treg},
-		{F_BATCMP,	 bq->init_data.rbatcomp},
-		{F_VCLAMP,	 bq->init_data.vclamp},
+		{F_ICHG,	 &bq->init_data.ichg},
+		{F_VREG,	 &bq->init_data.vreg},
+		{F_ITERM,	 &bq->init_data.iterm},
+		{F_IPRECHG,	 &bq->init_data.iprechg},
+		{F_SYSVMIN,	 &bq->init_data.sysvmin},
+		{F_BOOSTV,	 &bq->init_data.boostv},
+		{F_BOOSTI,	 &bq->init_data.boosti},
+		{F_BOOSTF,	 &bq->init_data.boostf},
+		{F_EN_ILIM,	 &bq->init_data.ilim_en},
+		{F_TREG,	 &bq->init_data.treg},
+		{F_BATCMP,	 &bq->init_data.rbatcomp},
+		{F_VCLAMP,	 &bq->init_data.vclamp},
 	};
 
+	for (i = 0; i < ARRAY_SIZE(init_data); i++) {
+		if (write) {
+			ret = bq25890_field_write(bq, init_data[i].id,
+						  *init_data[i].value);
+		} else {
+			ret = bq25890_field_read(bq, init_data[i].id);
+			if (ret >= 0)
+				*init_data[i].value = ret;
+		}
+		if (ret < 0) {
+			dev_dbg(bq->dev, "Accessing init data failed %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int bq25890_hw_init(struct bq25890_device *bq)
+{
+	int ret;
+
 	ret = bq25890_chip_reset(bq);
 	if (ret < 0) {
 		dev_dbg(bq->dev, "Reset failed %d\n", ret);
@@ -700,14 +723,9 @@ static int bq25890_hw_init(struct bq25890_device *bq)
 	}
 
 	/* initialize currents/voltages and other parameters */
-	for (i = 0; i < ARRAY_SIZE(init_data); i++) {
-		ret = bq25890_field_write(bq, init_data[i].id,
-					  init_data[i].value);
-		if (ret < 0) {
-			dev_dbg(bq->dev, "Writing init data failed %d\n", ret);
-			return ret;
-		}
-	}
+	ret = bq25890_rw_init_data(bq);
+	if (ret)
+		return ret;
 
 	ret = bq25890_get_chip_state(bq, &bq->state);
 	if (ret < 0) {
-- 
2.31.1

[PATCH v2 06/20] power: supply: bq25890: Add support to skip reset at probe() / remove()

[Hans de Goede]

On most X86/ACPI devices the firmware already fully initializes
the bq25890 charger at boot, in this case it is best to not reset
it at probe() time.

At support for a new "linux,skip-reset" boolean property to support this.
So far this new property is only used on X86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added
to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- New patch in v2 of this series, together with the "linux,read-back-settings"
  patch, this patch replaces the "ti,skip-init" patch from v1
---
 drivers/power/supply/bq25890_charger.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 704cc15b4403..864904d7eaeb 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -110,6 +110,7 @@ struct bq25890_device {
 	struct regmap *rmap;
 	struct regmap_field *rmap_fields[F_MAX_FIELDS];
 
+	bool skip_reset;
 	enum bq25890_chip_version chip_version;
 	struct bq25890_init_data init_data;
 	struct bq25890_state state;
@@ -709,10 +710,12 @@ static int bq25890_hw_init(struct bq25890_device *bq)
 {
 	int ret;
 
-	ret = bq25890_chip_reset(bq);
-	if (ret < 0) {
-		dev_dbg(bq->dev, "Reset failed %d\n", ret);
-		return ret;
+	if (!bq->skip_reset) {
+		ret = bq25890_chip_reset(bq);
+		if (ret < 0) {
+			dev_dbg(bq->dev, "Reset failed %d\n", ret);
+			return ret;
+		}
 	}
 
 	/* disable watchdog */
@@ -946,6 +949,8 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	int ret;
 	struct bq25890_init_data *init = &bq->init_data;
 
+	bq->skip_reset = device_property_read_bool(bq->dev, "linux,skip-reset");
+
 	ret = bq25890_fw_read_u32_props(bq);
 	if (ret < 0)
 		return ret;
@@ -1058,8 +1063,10 @@ static int bq25890_remove(struct i2c_client *client)
 	if (!IS_ERR_OR_NULL(bq->usb_phy))
 		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
 
-	/* reset all registers to default values */
-	bq25890_chip_reset(bq);
+	if (!bq->skip_reset) {
+		/* reset all registers to default values */
+		bq25890_chip_reset(bq);
+	}
 
 	return 0;
 }
-- 
2.31.1

[PATCH v2 07/20] power: supply: bq25890: Add support to read back the settings from the chip

[Hans de Goede]

On most X86/ACPI devices there is no devicetree to supply the necessary
init-data. Instead the firmware already fully initializes the bq25890
charger at boot. To support this, add support for reading back the
settings from the chip through a new "linux,read-back-settings" boolean.

So far this new property is only used on X86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added
to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- New patch in v2 of this series, together with the "linux,skip-reset" patch
  this patch replaces the "ti,skip-init" patch from v1
---
 drivers/power/supply/bq25890_charger.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 864904d7eaeb..b7c38d2c5ede 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -111,6 +111,7 @@ struct bq25890_device {
 	struct regmap_field *rmap_fields[F_MAX_FIELDS];
 
 	bool skip_reset;
+	bool read_back_init_data;
 	enum bq25890_chip_version chip_version;
 	struct bq25890_init_data init_data;
 	struct bq25890_state state;
@@ -666,7 +667,7 @@ static int bq25890_chip_reset(struct bq25890_device *bq)
 
 static int bq25890_rw_init_data(struct bq25890_device *bq)
 {
-	bool write = true;
+	bool write = !bq->read_back_init_data;
 	int ret;
 	int i;
 
@@ -950,6 +951,10 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	struct bq25890_init_data *init = &bq->init_data;
 
 	bq->skip_reset = device_property_read_bool(bq->dev, "linux,skip-reset");
+	bq->read_back_init_data = device_property_read_bool(bq->dev,
+						"linux,read-back-settings");
+	if (bq->read_back_init_data)
+		return 0;
 
 	ret = bq25890_fw_read_u32_props(bq);
 	if (ret < 0)
-- 
2.31.1

[PATCH v2 08/20] power: supply: bq25890: Enable charging on boards where we skip reset

[Hans de Goede]

On boards where the "linux,skip-reset" boolean property is set we don't
reset the charger; and on some boards where the fw takes care of
initalizition F_CHG_CFG is set to 0 before handing control over to the OS.

Explicitly set F_CHG_CFG to 1 on boards where we don't reset the charger,
so that charging is always enabled on these boards, like it is always
enabled on boards where we do reset the charger.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index b7c38d2c5ede..a69a2173e31a 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -717,6 +717,17 @@ static int bq25890_hw_init(struct bq25890_device *bq)
 			dev_dbg(bq->dev, "Reset failed %d\n", ret);
 			return ret;
 		}
+	} else {
+		/*
+		 * Ensure charging is enabled, on some boards where the fw
+		 * takes care of initalizition F_CHG_CFG is set to 0 before
+		 * handing control over to the OS.
+		 */
+		ret = bq25890_field_write(bq, F_CHG_CFG, 1);
+		if (ret < 0) {
+			dev_dbg(bq->dev, "Enabling charging failed %d\n", ret);
+			return ret;
+		}
 	}
 
 	/* disable watchdog */
-- 
2.31.1

[PATCH v2 09/20] power: supply: bq25890: Drop dev->platform_data == NULL check

[Hans de Goede]

Drop the "if (!dev->platform_data)" check, this seems to be an attempt
for allowing loading the driver on devices without devicetree stemming
from the initial commit of the driver (with the presumed intention being
the "return -ENODEV" else branch getting replaced with something else).

With the new "linux,skip-init" and "linux,read-back-settings" properties
the driver can actually supports devices without devicetree and this
check no longer makes sense.

While at it also switch to dev_err_probe(), which is already used in
various other places in the driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index a69a2173e31a..2bdfb58cda75 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1017,16 +1017,9 @@ static int bq25890_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (!dev->platform_data) {
-		ret = bq25890_fw_probe(bq);
-		if (ret < 0) {
-			dev_err(dev, "Cannot read device properties: %d\n",
-				ret);
-			return ret;
-		}
-	} else {
-		return -ENODEV;
-	}
+	ret = bq25890_fw_probe(bq);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "reading device properties\n");
 
 	ret = bq25890_hw_init(bq);
 	if (ret < 0) {
-- 
2.31.1

[PATCH v2 10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Hans de Goede]

Add a bq25890_set_otg_cfg() helper function, this is a preparation
patch for adding regulator support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 2bdfb58cda75..3c41fe86b3d3 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
 	return PTR_ERR_OR_ZERO(bq->charger);
 }
 
+static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
+{
+	int ret;
+
+	ret = bq25890_field_write(bq, F_OTG_CFG, val);
+	if (ret < 0)
+		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
+
+	return ret;
+}
+
 static void bq25890_usb_work(struct work_struct *data)
 {
 	int ret;
@@ -810,25 +821,16 @@ static void bq25890_usb_work(struct work_struct *data)
 	switch (bq->usb_event) {
 	case USB_EVENT_ID:
 		/* Enable boost mode */
-		ret = bq25890_field_write(bq, F_OTG_CFG, 1);
-		if (ret < 0)
-			goto error;
+		bq25890_set_otg_cfg(bq, 1);
 		break;
 
 	case USB_EVENT_NONE:
 		/* Disable boost mode */
-		ret = bq25890_field_write(bq, F_OTG_CFG, 0);
-		if (ret < 0)
-			goto error;
-
-		power_supply_changed(bq->charger);
+		ret = bq25890_set_otg_cfg(bq, 0);
+		if (ret == 0)
+			power_supply_changed(bq->charger);
 		break;
 	}
-
-	return;
-
-error:
-	dev_err(bq->dev, "Error switching to boost/charger mode.\n");
 }
 
 static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
-- 
2.31.1

[PATCH v2 11/20] power: supply: bq25890: Add support for registering the Vbus boost converter as a regulator

[Hans de Goede]

The bq25890_charger code supports enabling/disabling the boost converter
based on usb-phy notifications. But the usb-phy framework is not used on
all boards/platforms. At support for registering the Vbus boost converter
as a standard regulator when there is no usb-phy on the board.

Also add support for providing regulator_init_data through platform_data
for use on boards where device-tree is not used and the platform code must
thus provide the regulator_init_data.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- When the usb-phy framework is not used, turn off the Vboost regulator
  on shutdown
- Some minor code-tweaks based on Andy's review
---
 drivers/power/supply/bq25890_charger.c | 80 ++++++++++++++++++++++++++
 include/linux/power/bq25890_charger.h  | 15 +++++
 2 files changed, 95 insertions(+)
 create mode 100644 include/linux/power/bq25890_charger.h

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 3c41fe86b3d3..e06ca7b0eb3e 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -8,7 +8,9 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/power_supply.h>
+#include <linux/power/bq25890_charger.h>
 #include <linux/regmap.h>
+#include <linux/regulator/driver.h>
 #include <linux/types.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
@@ -845,6 +847,45 @@ static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
 	return NOTIFY_OK;
 }
 
+#ifdef CONFIG_REGULATOR
+static int bq25890_vbus_enable(struct regulator_dev *rdev)
+{
+	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+
+	return bq25890_set_otg_cfg(bq, 1);
+}
+
+static int bq25890_vbus_disable(struct regulator_dev *rdev)
+{
+	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+
+	return bq25890_set_otg_cfg(bq, 0);
+}
+
+static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
+{
+	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+
+	return bq25890_field_read(bq, F_OTG_CFG);
+}
+
+static const struct regulator_ops bq25890_vbus_ops = {
+	.enable = bq25890_vbus_enable,
+	.disable = bq25890_vbus_disable,
+	.is_enabled = bq25890_vbus_is_enabled,
+};
+
+static const struct regulator_desc bq25890_vbus_desc = {
+	.name = "usb_otg_vbus",
+	.of_match = "usb-otg-vbus",
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.ops = &bq25890_vbus_ops,
+	.fixed_uV = 5000000,
+	.n_voltages = 1,
+};
+#endif
+
 static int bq25890_get_chip_version(struct bq25890_device *bq)
 {
 	int id, rev;
@@ -1044,6 +1085,22 @@ static int bq25890_probe(struct i2c_client *client,
 		bq->usb_nb.notifier_call = bq25890_usb_notifier;
 		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
 	}
+#ifdef CONFIG_REGULATOR
+	else {
+		struct bq25890_platform_data *pdata = dev_get_platdata(dev);
+		struct regulator_config cfg = { };
+		struct regulator_dev *reg;
+
+		cfg.dev = dev;
+		cfg.driver_data = bq;
+		if (pdata)
+			cfg.init_data = pdata->regulator_init_data;
+
+		reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
+		if (IS_ERR(reg))
+			return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
+	}
+#endif
 
 	ret = bq25890_power_supply_init(bq);
 	if (ret < 0) {
@@ -1082,6 +1139,28 @@ static int bq25890_remove(struct i2c_client *client)
 	return 0;
 }
 
+static void bq25890_shutdown(struct i2c_client *client)
+{
+	struct bq25890_device *bq = i2c_get_clientdata(client);
+
+	/*
+	 * TODO this if + return should probably be removed, but that would
+	 * introduce a function change for boards using the usb-phy framework.
+	 * This needs to be tested on such a board before making this change.
+	 */
+	if (!IS_ERR_OR_NULL(bq->usb_phy))
+		return;
+
+	/*
+	 * Turn off the 5v Boost regulator which outputs Vbus to the device's
+	 * Micro-USB or Type-C USB port. Leaving this on drains power and
+	 * this avoids the PMIC on some device-models seeing this as Vbus
+	 * getting inserted after shutdown, causing the device to immediately
+	 * power-up again.
+	 */
+	bq25890_set_otg_cfg(bq, 0);
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int bq25890_suspend(struct device *dev)
 {
@@ -1161,6 +1240,7 @@ static struct i2c_driver bq25890_driver = {
 	},
 	.probe = bq25890_probe,
 	.remove = bq25890_remove,
+	.shutdown = bq25890_shutdown,
 	.id_table = bq25890_i2c_ids,
 };
 module_i2c_driver(bq25890_driver);
diff --git a/include/linux/power/bq25890_charger.h b/include/linux/power/bq25890_charger.h
new file mode 100644
index 000000000000..c706ddb77a08
--- /dev/null
+++ b/include/linux/power/bq25890_charger.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Platform data for the TI bq25890 battery charger driver.
+ */
+
+#ifndef _BQ25890_CHARGER_H_
+#define _BQ25890_CHARGER_H_
+
+struct regulator_init_data;
+
+struct bq25890_platform_data {
+	const struct regulator_init_data *regulator_init_data;
+};
+
+#endif
-- 
2.31.1

[PATCH v2 12/20] power: supply: bq25890: On the bq25892 set the IINLIM based on external charger detection

[Hans de Goede]

The bq25892 does not have builtin charger-type detection like the bq25980,
there might be some external charger detection capability, which will be
modelled as a power_supply class-device supplying the bq25892.

Use the usb_type property value from the supplier psy-device to set the
input-current-limit (when available).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 33 ++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index e06ca7b0eb3e..57e4034bc9cd 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -564,6 +564,38 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 	return 0;
 }
 
+/* On the BQ25892 try to get charger-type info from our supplier */
+static void bq25890_charger_external_power_changed(struct power_supply *psy)
+{
+	struct bq25890_device *bq = power_supply_get_drvdata(psy);
+	union power_supply_propval val;
+	int input_current_limit, ret;
+
+	if (bq->chip_version != BQ25892)
+		return;
+
+	ret = power_supply_get_property_from_supplier(bq->charger,
+						      POWER_SUPPLY_PROP_USB_TYPE,
+						      &val);
+	if (ret)
+		return;
+
+	switch (val.intval) {
+	case POWER_SUPPLY_USB_TYPE_DCP:
+		input_current_limit = bq25890_find_idx(2000000, TBL_IINLIM);
+		break;
+	case POWER_SUPPLY_USB_TYPE_CDP:
+	case POWER_SUPPLY_USB_TYPE_ACA:
+		input_current_limit = bq25890_find_idx(1500000, TBL_IINLIM);
+		break;
+	case POWER_SUPPLY_USB_TYPE_SDP:
+	default:
+		input_current_limit = bq25890_find_idx(500000, TBL_IINLIM);
+	}
+
+	bq25890_field_write(bq, F_IINLIM, input_current_limit);
+}
+
 static int bq25890_get_chip_state(struct bq25890_device *bq,
 				  struct bq25890_state *state)
 {
@@ -787,6 +819,7 @@ static const struct power_supply_desc bq25890_power_supply_desc = {
 	.properties = bq25890_power_supply_props,
 	.num_properties = ARRAY_SIZE(bq25890_power_supply_props),
 	.get_property = bq25890_power_supply_get_property,
+	.external_power_changed	= bq25890_charger_external_power_changed,
 };
 
 static int bq25890_power_supply_init(struct bq25890_device *bq)
-- 
2.31.1

[PATCH v2 13/20] power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol

[Hans de Goede]

From: Yauhen Kharuzhy <jekhor@gmail.com>

Add a "linux,pump-express-vbus-max" property which indicates if the Pump
Express+ protocol should be used to increase the charging protocol.

If this new property is set and a DCP charger is detected then request
a higher charging voltage through the Pump Express+ protocol.

So far this new property is only used on X86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added
to the existing devicetree-bindings.

Changes by Hans de Goede:
- Port to my bq25890 patch-series + various cleanups
- Make behavior configurable through a new "linux,pump-express-vbus-max"
  device-property
- Sleep 1 second before re-checking the Vbus voltage after requesting
  it to be raised, to ensure that the ADC has time to sampled the new Vbus
- Add VBUSV bq25890_tables[] entry and use it in bq25890_get_vbus_voltage()
- Tweak commit message

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 74 ++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 57e4034bc9cd..1c59d4f71389 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -27,6 +27,10 @@
 #define BQ25895_ID			7
 #define BQ25896_ID			0
 
+#define PUMP_EXPRESS_START_DELAY	(5 * HZ)
+#define PUMP_EXPRESS_MAX_TRIES		6
+#define PUMP_EXPRESS_VBUS_MARGIN	1000000
+
 enum bq25890_chip_version {
 	BQ25890,
 	BQ25892,
@@ -107,6 +111,7 @@ struct bq25890_device {
 	struct usb_phy *usb_phy;
 	struct notifier_block usb_nb;
 	struct work_struct usb_work;
+	struct delayed_work pump_express_work;
 	unsigned long usb_event;
 
 	struct regmap *rmap;
@@ -114,6 +119,7 @@ struct bq25890_device {
 
 	bool skip_reset;
 	bool read_back_init_data;
+	u32 pump_express_vbus_max;
 	enum bq25890_chip_version chip_version;
 	struct bq25890_init_data init_data;
 	struct bq25890_state state;
@@ -265,6 +271,7 @@ enum bq25890_table_ids {
 	TBL_VREG,
 	TBL_BOOSTV,
 	TBL_SYSVMIN,
+	TBL_VBUSV,
 	TBL_VBATCOMP,
 	TBL_RBATCOMP,
 
@@ -308,6 +315,7 @@ static const union {
 	[TBL_VREG] =	{ .rt = {3840000, 4608000, 16000} },	 /* uV */
 	[TBL_BOOSTV] =	{ .rt = {4550000, 5510000, 64000} },	 /* uV */
 	[TBL_SYSVMIN] = { .rt = {3000000, 3700000, 100000} },	 /* uV */
+	[TBL_VBUSV] =	{ .rt = {2600000,15300000, 100000} },	 /* uV */
 	[TBL_VBATCOMP] ={ .rt = {0,        224000, 32000} },	 /* uV */
 	[TBL_RBATCOMP] ={ .rt = {0,        140000, 20000} },	 /* uOhm */
 
@@ -410,6 +418,17 @@ static bool bq25890_is_adc_property(enum power_supply_property psp)
 
 static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq);
 
+static int bq25890_get_vbus_voltage(struct bq25890_device *bq)
+{
+	int ret;
+
+	ret = bq25890_field_read(bq, F_VBUSV);
+	if (ret < 0)
+		return ret;
+
+	return bq25890_find_val(ret, TBL_VBUSV);
+}
+
 static int bq25890_power_supply_get_property(struct power_supply *psy,
 					     enum power_supply_property psp,
 					     union power_supply_propval *val)
@@ -583,6 +602,11 @@ static void bq25890_charger_external_power_changed(struct power_supply *psy)
 	switch (val.intval) {
 	case POWER_SUPPLY_USB_TYPE_DCP:
 		input_current_limit = bq25890_find_idx(2000000, TBL_IINLIM);
+		if (bq->pump_express_vbus_max) {
+			queue_delayed_work(system_power_efficient_wq,
+					   &bq->pump_express_work,
+					   PUMP_EXPRESS_START_DELAY);
+		}
 		break;
 	case POWER_SUPPLY_USB_TYPE_CDP:
 	case POWER_SUPPLY_USB_TYPE_ACA:
@@ -847,6 +871,50 @@ static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
 	return ret;
 }
 
+static void bq25890_pump_express_work(struct work_struct *data)
+{
+	struct bq25890_device *bq =
+		container_of(data, struct bq25890_device, pump_express_work.work);
+	int voltage, i, ret;
+
+	dev_dbg(bq->dev, "Start to request input voltage increasing\n");
+
+	/* Enable current pulse voltage control protocol */
+	ret = bq25890_field_write(bq, F_PUMPX_EN, 1);
+	if (ret < 0)
+		goto error;
+
+	for (i = 0; i < PUMP_EXPRESS_MAX_TRIES; i++) {
+		voltage = bq25890_get_vbus_voltage(bq);
+		if (voltage < 0)
+			goto error;
+		dev_dbg(bq->dev, "input voltage = %d mV\n", voltage);
+
+		if ((voltage + PUMP_EXPRESS_VBUS_MARGIN) >
+					bq->pump_express_vbus_max)
+			break;
+
+		ret = bq25890_field_write(bq, F_PUMPX_UP, 1);
+		if (ret < 0)
+			goto error;
+
+		while (bq25890_field_read(bq, F_PUMPX_UP) == 1)
+			msleep(100);
+
+		/* Make sure ADC has sampled Vbus before checking again */
+		msleep(1000);
+	}
+
+	bq25890_field_write(bq, F_PUMPX_EN, 0);
+
+	dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n",
+		 voltage);
+
+	return;
+error:
+	dev_err(bq->dev, "Failed to request hi-voltage charging\n");
+}
+
 static void bq25890_usb_work(struct work_struct *data)
 {
 	int ret;
@@ -1037,6 +1105,11 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	int ret;
 	struct bq25890_init_data *init = &bq->init_data;
 
+	ret = device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
+				       &bq->pump_express_vbus_max);
+	if (ret < 0)
+		bq->pump_express_vbus_max = 0;
+
 	bq->skip_reset = device_property_read_bool(bq->dev, "linux,skip-reset");
 	bq->read_back_init_data = device_property_read_bool(bq->dev,
 						"linux,read-back-settings");
@@ -1069,6 +1142,7 @@ static int bq25890_probe(struct i2c_client *client,
 	bq->dev = dev;
 
 	mutex_init(&bq->lock);
+	INIT_DELAYED_WORK(&bq->pump_express_work, bq25890_pump_express_work);
 
 	bq->rmap = devm_regmap_init_i2c(client, &bq25890_regmap_config);
 	if (IS_ERR(bq->rmap))
-- 
2.31.1

[PATCH v2 14/20] mfd: intel_soc_pmic_chtwc: Add intel_cht_wc_get_model() helper function

[Hans de Goede]

Tablet / laptop designs using an Intel Cherry Trail x86 main SoC with
an Intel Whiskey Cove PMIC do not use a single standard setup for
the charger, fuel-gauge and other chips surrounding the PMIC /
charging+data USB port.

Unlike what is normal on X86 this diversity in designs is not handled
by the ACPI tables. On 2 of the 3 known designs there are no standard
(PNP0C0A) ACPI battery devices and on the 3th design the ACPI battery
device does not work under Linux due to it requiring non-standard
and undocumented ACPI behavior.

So to make things work under Linux we use native charger and fuel-gauge
drivers on these devices, re-using the native drivers used on ARM boards
with the same charger / fuel-gauge ICs.

This requires various MFD-cell drivers for the CHT-WC PMIC cells to
know which model they are exactly running on so that they can e.g.
instantiate an I2C-client for the right model charger-IC (the charger
is connected to an I2C-controller which is part of the PMIC).

Rather then duplicating DMI-id matching to check which model we are
running on in each MFD-cell driver add a helper function for this
and make this id all 3 known models:

1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
but the Pocket re-uses the GPD Win's design in a different housing:

The WC PMIC is connected to a TI BQ24292i charger, paired with
a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
a PI3USB30532 USB switch, for a fully functional Type-C port.

2. The Xiaomi Mi Pad 2:

The WC PMIC is connected to a TI BQ25890 charger, paired with
a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
detection, for a USB-2 only Type-C port without PD.

3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:

The WC PMIC is connected to a TI BQ25892 charger, paired with
a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
detection and using the BQ25892's Mediatek Pump Express+ (1.0)
support to enable charging with up to 12V through a micro-USB port.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 46 ++++++++++++++++++++++++++++++
 include/linux/mfd/intel_soc_pmic.h |  9 ++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index 49c5f71664bc..3432bd18f5d0 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -134,6 +135,51 @@ static const struct regmap_irq_chip cht_wc_regmap_irq_chip = {
 	.num_regs = 1,
 };
 
+static const struct dmi_system_id cht_wc_model_dmi_ids[] = {
+	{	/* GPD win / GPD pocket mini laptops */
+		.driver_data = (void *)(long)INTEL_CHT_WC_GPD_WIN_POCKET,
+		/*
+		 * Note this may not seem like a very unique match, but in the
+		 * 24000+ DMI decode dumps from linux-hardware.org only 42 have
+		 * a board_vendor value of "AMI Corporation" and of those 42
+		 * only 1 (the GPD win/pocket entry) has a board_name of
+		 * "Default string". Also very few devices have both board_ and
+		 * product_name not set.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+			DMI_MATCH(DMI_BOARD_NAME, "Default string"),
+			DMI_MATCH(DMI_BOARD_SERIAL, "Default string"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
+		},
+	}, {	/* Xiaomi Mi Pad 2 */
+		.driver_data = (void *)(long)INTEL_CHT_WC_XIAOMI_MIPAD2,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+		},
+	}, {	/* Lenovo Yoga Book X90F / X91F / X91L */
+		.driver_data = (void *)(long)INTEL_CHT_WC_LENOVO_YOGABOOK1,
+		.matches = {
+		  /* Non exact match to match all versions */
+		  DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X9"),
+		},
+	},
+	{ } /* Terminating empty */
+};
+
+enum intel_cht_wc_models intel_cht_wc_get_model(void)
+{
+	const struct dmi_system_id *id;
+
+	id = dmi_first_match(cht_wc_model_dmi_ids);
+	if (!id)
+		return INTEL_CHT_WC_UNKNOWN;
+
+	return (long)id->driver_data;
+}
+EXPORT_SYMBOL_GPL(intel_cht_wc_get_model);
+
 static int cht_wc_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
index 6a88e34cb955..dd17d7f82434 100644
--- a/include/linux/mfd/intel_soc_pmic.h
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -41,7 +41,16 @@ struct intel_soc_pmic {
 	struct intel_scu_ipc_dev *scu;
 };
 
+enum intel_cht_wc_models {
+	INTEL_CHT_WC_UNKNOWN,
+	INTEL_CHT_WC_GPD_WIN_POCKET,
+	INTEL_CHT_WC_XIAOMI_MIPAD2,
+	INTEL_CHT_WC_LENOVO_YOGABOOK1,
+};
+
 int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
 					      u32 value, u32 mask);
 
+enum intel_cht_wc_models intel_cht_wc_get_model(void);
+
 #endif	/* __INTEL_SOC_PMIC_H__ */
-- 
2.31.1

[PATCH v2 15/20] i2c: cht-wc: Make charger i2c-client instantiation board/device-model specific

[Hans de Goede]

The i2c-controller on the Cherry Trail - Whiskey Cove PMIC is special
in that it is always connected to the I2C charger IC of the board on
which the PMIC is used; and the charger IC is not described in ACPI,
so the i2c-cht-wc code needs to instantiate an i2c-client for it itself.

So far this was hardcoded to instantiate an i2c-client for the
bq24292i, with all properties, etc. set to match how this charger
is used on the GPD win and GPD pcoket devices.

There is a rudimentary check to make sure the ACPI tables are at least
somewhat as expected, but this is far from accurate, leading to
a wrong i2c-client being instantiated for the charger on some boards.

Switch to the new DMI based intel_cht_wc_get_model() helper which is
exported by the MFD driver for the CHT Whiskey Cove PMIC to help PMIC
cell drivers like the i2c-cht-wc code reliably detect which board
they are running on.

And add board_info for the charger ICs as found on the other 2 known
boards with a Whisky Cove PMIC.

This has been tested on all 3 known boards.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-cht-wc.c | 120 +++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index 1cf68f85b2e1..e700373bf388 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/power/bq24190_charger.h>
+#include <linux/power/bq25890_charger.h>
 #include <linux/slab.h>
 
 #define CHT_WC_I2C_CTRL			0x5e24
@@ -270,6 +271,7 @@ static const struct irq_chip cht_wc_i2c_irq_chip = {
 	.name			= "cht_wc_ext_chrg_irq_chip",
 };
 
+/********** GPD Win / Pocket charger IC settings **********/
 static const char * const bq24190_suppliers[] = {
 	"tcpm-source-psy-i2c-fusb302" };
 
@@ -304,17 +306,93 @@ static struct bq24190_platform_data bq24190_pdata = {
 	.regulator_init_data = &bq24190_vbus_init_data,
 };
 
+static struct i2c_board_info gpd_win_board_info = {
+	.type = "bq24190",
+	.addr = 0x6b,
+	.dev_name = "bq24190",
+	.swnode = &bq24190_node,
+	.platform_data = &bq24190_pdata,
+};
+
+/********** Xiaomi Mi Pad 2 charger IC settings  **********/
+static struct regulator_consumer_supply bq2589x_vbus_consumer = {
+	.supply = "vbus",
+	.dev_name = "cht_wcove_pwrsrc",
+};
+
+static const struct regulator_init_data bq2589x_vbus_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.consumer_supplies = &bq2589x_vbus_consumer,
+	.num_consumer_supplies = 1,
+};
+
+static struct bq25890_platform_data bq2589x_pdata = {
+	.regulator_init_data = &bq2589x_vbus_init_data,
+};
+
+static const struct property_entry xiaomi_mipad2_props[] = {
+	PROPERTY_ENTRY_BOOL("linux,skip-reset"),
+	PROPERTY_ENTRY_BOOL("linux,read-back-settings"),
+	{ }
+};
+
+static const struct software_node xiaomi_mipad2_node = {
+	.properties = xiaomi_mipad2_props,
+};
+
+static struct i2c_board_info xiaomi_mipad2_board_info = {
+	.type = "bq25890",
+	.addr = 0x6a,
+	.dev_name = "bq25890",
+	.swnode = &xiaomi_mipad2_node,
+	.platform_data = &bq2589x_pdata,
+};
+
+/********** Lenovo Yogabook YB1-X90F/-X91F/-X91L charger settings **********/
+static const char * const lenovo_yb1_bq25892_suppliers[] = {
+	"cht_wcove_pwrsrc" };
+
+static const struct property_entry lenovo_yb1_bq25892_props[] = {
+	PROPERTY_ENTRY_STRING_ARRAY("supplied-from",
+				    lenovo_yb1_bq25892_suppliers),
+	PROPERTY_ENTRY_U32("linux,pump-express-vbus-max", 12000000),
+	PROPERTY_ENTRY_BOOL("linux,skip-reset"),
+	/*
+	 * The firmware sets everything to the defaults, which leads to a
+	 * somewhat low charge-current of 2048mA and worse to a batter-voltage
+	 * of 4.2V instead of 4.35V (when booted without a charger connected).
+	 * Use our own values instead of "linux,read-back-settings" to fix this.
+	 */
+	PROPERTY_ENTRY_U32("ti,charge-current", 4224000),
+	PROPERTY_ENTRY_U32("ti,battery-regulation-voltage", 4352000),
+	PROPERTY_ENTRY_U32("ti,termination-current", 256000),
+	PROPERTY_ENTRY_U32("ti,precharge-current", 128000),
+	PROPERTY_ENTRY_U32("ti,minimum-sys-voltage", 3500000),
+	PROPERTY_ENTRY_U32("ti,boost-voltage", 4998000),
+	PROPERTY_ENTRY_U32("ti,boost-max-current", 1400000),
+	PROPERTY_ENTRY_BOOL("ti,use-ilim-pin"),
+	{ }
+};
+
+static const struct software_node lenovo_yb1_bq25892_node = {
+	.properties = lenovo_yb1_bq25892_props,
+};
+
+static struct i2c_board_info lenovo_yogabook1_board_info = {
+	.type = "bq25892",
+	.addr = 0x6b,
+	.dev_name = "bq25892",
+	.swnode = &lenovo_yb1_bq25892_node,
+	.platform_data = &bq2589x_pdata,
+};
+
 static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	struct i2c_board_info *board_info = NULL;
 	struct cht_wc_i2c_adap *adap;
-	struct i2c_board_info board_info = {
-		.type = "bq24190",
-		.addr = 0x6b,
-		.dev_name = "bq24190",
-		.swnode = &bq24190_node,
-		.platform_data = &bq24190_pdata,
-	};
 	int ret, reg, irq;
 
 	irq = platform_get_irq(pdev, 0);
@@ -379,17 +457,23 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		goto remove_irq_domain;
 
-	/*
-	 * Normally the Whiskey Cove PMIC is paired with a TI bq24292i charger,
-	 * connected to this i2c bus, and a max17047 fuel-gauge and a fusb302
-	 * USB Type-C controller connected to another i2c bus. In this setup
-	 * the max17047 and fusb302 devices are enumerated through an INT33FE
-	 * ACPI device. If this device is present register an i2c-client for
-	 * the TI bq24292i charger.
-	 */
-	if (acpi_dev_present("INT33FE", NULL, -1)) {
-		board_info.irq = adap->client_irq;
-		adap->client = i2c_new_client_device(&adap->adapter, &board_info);
+	switch (intel_cht_wc_get_model()) {
+	case INTEL_CHT_WC_GPD_WIN_POCKET:
+		board_info = &gpd_win_board_info;
+		break;
+	case INTEL_CHT_WC_XIAOMI_MIPAD2:
+		board_info = &xiaomi_mipad2_board_info;
+		break;
+	case INTEL_CHT_WC_LENOVO_YOGABOOK1:
+		board_info = &lenovo_yogabook1_board_info;
+		break;
+	default:
+		dev_warn(&pdev->dev, "Unknown model, not instantiating charger device\n");
+	}
+
+	if (board_info) {
+		board_info->irq = adap->client_irq;
+		adap->client = i2c_new_client_device(&adap->adapter, board_info);
 		if (IS_ERR(adap->client)) {
 			ret = PTR_ERR(adap->client);
 			goto del_adapter;
-- 
2.31.1

[PATCH v2 16/20] extcon: intel-cht-wc: Use new intel_cht_wc_get_model() helper

[Hans de Goede]

The CHT_WC_VBUS_GPIO_CTLO GPIO actually driving an external 5V Vboost
converter for Vbus depends on the board on which the Cherry Trail -
Whiskey Cove PMIC is actually used.

Since the information about the exact PMIC setup is necessary in other
places too, the drivers/mfd/intel_soc_pmic_chtwc.c code now has a new
intel_cht_wc_get_model() helper.

Only poke the CHT_WC_VBUS_GPIO_CTLO GPIO if this new helper returns
INTEL_CHT_WC_GPD_WIN_POCKET, which indicates the Type-C (with PD and
DP-altmode) setup used on the GPD pocket and GPD win; and on which
this GPIO actually controls an external 5V Vboost converter.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 35 +++++++++++++++++-----------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 771f6f4cf92e..a5aeeecc44fb 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
@@ -358,20 +359,26 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	if (IS_ERR(ext->edev))
 		return PTR_ERR(ext->edev);
 
-	/*
-	 * When a host-cable is detected the BIOS enables an external 5v boost
-	 * converter to power connected devices there are 2 problems with this:
-	 * 1) This gets seen by the external battery charger as a valid Vbus
-	 *    supply and it then tries to feed Vsys from this creating a
-	 *    feedback loop which causes aprox. 300 mA extra battery drain
-	 *    (and unless we drive the external-charger-disable pin high it
-	 *    also tries to charge the battery causing even more feedback).
-	 * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
-	 * Since the external battery charger has its own 5v boost converter
-	 * which does not have these issues, we simply turn the separate
-	 * external 5v boost converter off and leave it off entirely.
-	 */
-	cht_wc_extcon_set_5v_boost(ext, false);
+	switch (intel_cht_wc_get_model()) {
+	case INTEL_CHT_WC_GPD_WIN_POCKET:
+		/*
+		 * When a host-cable is detected the BIOS enables an external 5v boost
+		 * converter to power connected devices there are 2 problems with this:
+		 * 1) This gets seen by the external battery charger as a valid Vbus
+		 *    supply and it then tries to feed Vsys from this creating a
+		 *    feedback loop which causes aprox. 300 mA extra battery drain
+		 *    (and unless we drive the external-charger-disable pin high it
+		 *    also tries to charge the battery causing even more feedback).
+		 * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
+		 * Since the external battery charger has its own 5v boost converter
+		 * which does not have these issues, we simply turn the separate
+		 * external 5v boost converter off and leave it off entirely.
+		 */
+		cht_wc_extcon_set_5v_boost(ext, false);
+		break;
+	default:
+		break;
+	}
 
 	/* Enable sw control */
 	ret = cht_wc_extcon_sw_control(ext, true);
-- 
2.31.1

[PATCH v2 17/20] extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C connectors

[Hans de Goede]

So far the extcon-intel-cht-wc code has only been tested on devices with
a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support
through a FUSB302 Type-C controller.

Some devices with the intel-cht-wc PMIC however come with an USB-micro-B
connector, or an USB-2 only Type-C connector without USB-PD.

Which device-model we are running on can be identified with the new
intel_cht_wc_get_model() helper and on models without a Type-C controller
the extcon code must control the Vbus 5V boost converter and the USB role
switch depending on the detected cable-type.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/Kconfig               |  3 +-
 drivers/extcon/extcon-intel-cht-wc.c | 91 ++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index c69d40ae5619..fc733689cfad 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -60,7 +60,8 @@ config EXTCON_INTEL_INT3496
 
 config EXTCON_INTEL_CHT_WC
 	tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver"
-	depends on INTEL_SOC_PMIC_CHTWC
+	depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT
+	select USB_ROLE_SWITCH
 	help
 	  Say Y here to enable extcon support for charger detection / control
 	  on the Intel Cherrytrail Whiskey Cove PMIC.
diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index a5aeeecc44fb..119b83793123 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -16,7 +16,9 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/usb/role.h>
 
 #include "extcon-intel.h"
 
@@ -102,8 +104,11 @@ struct cht_wc_extcon_data {
 	struct device *dev;
 	struct regmap *regmap;
 	struct extcon_dev *edev;
+	struct usb_role_switch *role_sw;
+	struct regulator *vbus_boost;
 	unsigned int previous_cable;
 	bool usb_host;
+	bool vbus_boost_enabled;
 };
 
 static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
@@ -217,6 +222,18 @@ static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
 				 CHT_WC_CHGRCTRL1_OTGMODE, val);
 	if (ret)
 		dev_err(ext->dev, "Error updating CHGRCTRL1 reg: %d\n", ret);
+
+	if (ext->vbus_boost && ext->vbus_boost_enabled != enable) {
+		if (enable)
+			ret = regulator_enable(ext->vbus_boost);
+		else
+			ret = regulator_disable(ext->vbus_boost);
+
+		if (ret == 0)
+			ext->vbus_boost_enabled = enable;
+		else
+			dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret);
+	}
 }
 
 static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext,
@@ -246,6 +263,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 	unsigned int cable = EXTCON_NONE;
 	/* Ignore errors in host mode, as the 5v boost converter is on then */
 	bool ignore_get_charger_errors = ext->usb_host;
+	enum usb_role role;
 
 	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
 	if (ret) {
@@ -289,6 +307,18 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 
 	ext->usb_host = ((id == INTEL_USB_ID_GND) || (id == INTEL_USB_RID_A));
 	extcon_set_state_sync(ext->edev, EXTCON_USB_HOST, ext->usb_host);
+
+	if (ext->usb_host)
+		role = USB_ROLE_HOST;
+	else if (pwrsrc_sts & CHT_WC_PWRSRC_VBUS)
+		role = USB_ROLE_DEVICE;
+	else
+		role = USB_ROLE_NONE;
+
+	/* Note: this is a no-op when ext->role_sw is NULL */
+	ret = usb_role_switch_set_role(ext->role_sw, role);
+	if (ret)
+		dev_err(ext->dev, "Error setting USB-role: %d\n", ret);
 }
 
 static irqreturn_t cht_wc_extcon_isr(int irq, void *data)
@@ -334,6 +364,61 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
 	return ret;
 }
 
+static int cht_wc_extcon_find_role_sw(struct cht_wc_extcon_data *ext)
+{
+	const struct software_node *swnode;
+	struct fwnode_handle *fwnode;
+
+	swnode = software_node_find_by_name(NULL, "intel-xhci-usb-sw");
+	if (!swnode)
+		return -EPROBE_DEFER;
+
+	fwnode = software_node_fwnode(swnode);
+	ext->role_sw = usb_role_switch_find_by_fwnode(fwnode);
+	fwnode_handle_put(fwnode);
+
+	return ext->role_sw ? 0 : -EPROBE_DEFER;
+}
+
+static void cht_wc_extcon_put_role_sw(void *data)
+{
+	struct cht_wc_extcon_data *ext = data;
+
+	usb_role_switch_put(ext->role_sw);
+}
+
+/* Some boards require controlling the role-sw and vbus based on the id-pin */
+static int cht_wc_extcon_get_role_sw_and_regulator(struct cht_wc_extcon_data *ext)
+{
+	int ret;
+
+	ret = cht_wc_extcon_find_role_sw(ext);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(ext->dev, cht_wc_extcon_put_role_sw, ext);
+	if (ret)
+		return ret;
+
+	/*
+	 * On x86/ACPI platforms the regulator <-> consumer link is provided
+	 * by platform_data passed to the regulator driver. This means that
+	 * this info is not available before the regulator driver has bound.
+	 * Use devm_regulator_get_optional() to avoid getting a dummy
+	 * regulator and wait for the regulator to show up if necessary.
+	 */
+	ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus");
+	if (IS_ERR(ext->vbus_boost)) {
+		ret = PTR_ERR(ext->vbus_boost);
+		if (ret == -ENODEV)
+			ret = -EPROBE_DEFER;
+
+		return dev_err_probe(ext->dev, ret, "getting vbus regulator");
+	}
+
+	return 0;
+}
+
 static int cht_wc_extcon_probe(struct platform_device *pdev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
@@ -376,6 +461,12 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 		 */
 		cht_wc_extcon_set_5v_boost(ext, false);
 		break;
+	case INTEL_CHT_WC_LENOVO_YOGABOOK1:
+	case INTEL_CHT_WC_XIAOMI_MIPAD2:
+		ret = cht_wc_extcon_get_role_sw_and_regulator(ext);
+		if (ret)
+			return ret;
+		break;
 	default:
 		break;
 	}
-- 
2.31.1

[PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

[Hans de Goede]

Refactor cht_wc_extcon_get_charger() to have all the returns are in
the "switch (usbsrc)" cases.

This is a preparation patch for adding support for registering
a power_supply class device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 119b83793123..f2b93a99a625 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
 	} while (time_before(jiffies, timeout));
 
 	if (status != CHT_WC_USBSRC_STS_SUCCESS) {
-		if (ignore_errors)
-			return EXTCON_CHG_USB_SDP; /* Save fallback */
+		if (!ignore_errors) {
+			if (status == CHT_WC_USBSRC_STS_FAIL)
+				dev_warn(ext->dev, "Could not detect charger type\n");
+			else
+				dev_warn(ext->dev, "Timeout detecting charger type\n");
+		}
 
-		if (status == CHT_WC_USBSRC_STS_FAIL)
-			dev_warn(ext->dev, "Could not detect charger type\n");
-		else
-			dev_warn(ext->dev, "Timeout detecting charger type\n");
-		return EXTCON_CHG_USB_SDP; /* Save fallback */
+		/* Save fallback */
+		usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
 	}
 
 	usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
-- 
2.31.1

[PATCH v2 19/20] extcon: intel-cht-wc: Add support for registering a power_supply class-device

[Hans de Goede]

The bq25890 used on the Yogabook YB1-X90 / -X91 models relies on
the extcon-driver's BC-1.2 charger detection, and the bq25890 driver
expect this info to be  available through a parent power_supply
class-device which models the detected charger (idem to how the Type-C
TCPM code registers a power_supply classdev for the connected charger).

Add support for registering the power_supply class-device expected
by this setup.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 83 ++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index f2b93a99a625..1030610a3494 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
+#include <linux/power_supply.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -106,6 +107,8 @@ struct cht_wc_extcon_data {
 	struct extcon_dev *edev;
 	struct usb_role_switch *role_sw;
 	struct regulator *vbus_boost;
+	struct power_supply *psy;
+	enum power_supply_usb_type usb_type;
 	unsigned int previous_cable;
 	bool usb_host;
 	bool vbus_boost_enabled;
@@ -170,18 +173,23 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
 		dev_warn(ext->dev,
 			"Unhandled charger type %d, defaulting to SDP\n",
 			 ret);
+		ext->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
 		return EXTCON_CHG_USB_SDP;
 	case CHT_WC_USBSRC_TYPE_SDP:
 	case CHT_WC_USBSRC_TYPE_FLOATING:
 	case CHT_WC_USBSRC_TYPE_OTHER:
+		ext->usb_type = POWER_SUPPLY_USB_TYPE_SDP;
 		return EXTCON_CHG_USB_SDP;
 	case CHT_WC_USBSRC_TYPE_CDP:
+		ext->usb_type = POWER_SUPPLY_USB_TYPE_CDP;
 		return EXTCON_CHG_USB_CDP;
 	case CHT_WC_USBSRC_TYPE_DCP:
 	case CHT_WC_USBSRC_TYPE_DCP_EXTPHY:
 	case CHT_WC_USBSRC_TYPE_MHL: /* MHL2+ delivers upto 2A, treat as DCP */
+		ext->usb_type = POWER_SUPPLY_USB_TYPE_DCP;
 		return EXTCON_CHG_USB_DCP;
 	case CHT_WC_USBSRC_TYPE_ACA:
+		ext->usb_type = POWER_SUPPLY_USB_TYPE_ACA;
 		return EXTCON_CHG_USB_ACA;
 	}
 }
@@ -266,6 +274,8 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 	bool ignore_get_charger_errors = ext->usb_host;
 	enum usb_role role;
 
+	ext->usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+
 	ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_STS, &pwrsrc_sts);
 	if (ret) {
 		dev_err(ext->dev, "Error reading pwrsrc status: %d\n", ret);
@@ -320,6 +330,9 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 	ret = usb_role_switch_set_role(ext->role_sw, role);
 	if (ret)
 		dev_err(ext->dev, "Error setting USB-role: %d\n", ret);
+
+	if (ext->psy)
+		power_supply_changed(ext->psy);
 }
 
 static irqreturn_t cht_wc_extcon_isr(int irq, void *data)
@@ -420,6 +433,61 @@ static int cht_wc_extcon_get_role_sw_and_regulator(struct cht_wc_extcon_data *ex
 	return 0;
 }
 
+static int cht_wc_extcon_psy_get_prop(struct power_supply *psy,
+				      enum power_supply_property psp,
+				      union power_supply_propval *val)
+{
+	struct cht_wc_extcon_data *ext = power_supply_get_drvdata(psy);
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_USB_TYPE:
+		val->intval = ext->usb_type;
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = ext->usb_type ? 1 : 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static const enum power_supply_usb_type cht_wc_extcon_psy_usb_types[] = {
+	POWER_SUPPLY_USB_TYPE_SDP,
+	POWER_SUPPLY_USB_TYPE_CDP,
+	POWER_SUPPLY_USB_TYPE_DCP,
+	POWER_SUPPLY_USB_TYPE_ACA,
+	POWER_SUPPLY_USB_TYPE_UNKNOWN,
+};
+
+static const enum power_supply_property cht_wc_extcon_psy_props[] = {
+	POWER_SUPPLY_PROP_USB_TYPE,
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static const struct power_supply_desc cht_wc_extcon_psy_desc = {
+	.name = "cht_wcove_pwrsrc",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.usb_types = cht_wc_extcon_psy_usb_types,
+	.num_usb_types = ARRAY_SIZE(cht_wc_extcon_psy_usb_types),
+	.properties = cht_wc_extcon_psy_props,
+	.num_properties = ARRAY_SIZE(cht_wc_extcon_psy_props),
+	.get_property = cht_wc_extcon_psy_get_prop,
+};
+
+static int cht_wc_extcon_register_psy(struct cht_wc_extcon_data *ext)
+{
+	struct power_supply_config psy_cfg = { .drv_data = ext };
+
+	ext->psy = devm_power_supply_register(ext->dev,
+					      &cht_wc_extcon_psy_desc,
+					      &psy_cfg);
+	return PTR_ERR_OR_ZERO(ext->psy);
+}
+
 static int cht_wc_extcon_probe(struct platform_device *pdev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
@@ -463,6 +531,21 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 		cht_wc_extcon_set_5v_boost(ext, false);
 		break;
 	case INTEL_CHT_WC_LENOVO_YOGABOOK1:
+		/* Do this first, as it may very well return -EPROBE_DEFER. */
+		ret = cht_wc_extcon_get_role_sw_and_regulator(ext);
+		if (ret)
+			return ret;
+		/*
+		 * The bq25890 used here relies on this driver's BC-1.2 charger
+		 * detection, and the bq25890 driver expect this info to be
+		 * available through a parent power_supply class device which
+		 * models the detected charger (idem to how the Type-C TCPM code
+		 * registers a power_supply classdev for the connected charger).
+		 */
+		ret = cht_wc_extcon_register_psy(ext);
+		if (ret)
+			return ret;
+		break;
 	case INTEL_CHT_WC_XIAOMI_MIPAD2:
 		ret = cht_wc_extcon_get_role_sw_and_regulator(ext);
 		if (ret)
-- 
2.31.1

[PATCH v2 20/20] extcon: intel-cht-wc: Report RID_A for ACA adapters

[Hans de Goede]

Make cht_wc_extcon_get_id() report RID_A for ACA adapters, instead of
reporting ID_FLOAT.

According to the spec. we should read the USB-ID pin ADC value
to determine the resistance of the used pull-down resister and
then return RID_A / RID_B / RID_C based on this. But all "Accessory
Charger Adapter"s (ACAs) which users can actually buy always use
a combination of a charging port with one or more USB-A ports, so
they should always use a resistor indicating RID_A. But the spec
is hard to read / badly-worded so some of them actually indicate
they are a RID_B ACA evnen though they clearly are a RID_A ACA.

To workaround this simply always return INTEL_USB_RID_A, which
matches all the ACAs which users can actually buy.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 1030610a3494..c92bc98cb428 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -121,13 +121,21 @@ static int cht_wc_extcon_get_id(struct cht_wc_extcon_data *ext, int pwrsrc_sts)
 		return INTEL_USB_ID_GND;
 	case CHT_WC_PWRSRC_RID_FLOAT:
 		return INTEL_USB_ID_FLOAT;
+	/*
+	 * According to the spec. we should read the USB-ID pin ADC value here
+	 * to determine the resistance of the used pull-down resister and then
+	 * return RID_A / RID_B / RID_C based on this. But all "Accessory
+	 * Charger Adapter"s (ACAs) which users can actually buy always use
+	 * a combination of a charging port with one or more USB-A ports, so
+	 * they should always use a resistor indicating RID_A. But the spec
+	 * is hard to read / badly-worded so some of them actually indicate
+	 * they are a RID_B ACA evnen though they clearly are a RID_A ACA.
+	 * To workaround this simply always return INTEL_USB_RID_A, which
+	 * matches all the ACAs which users can actually buy.
+	 */
 	case CHT_WC_PWRSRC_RID_ACA:
+		return INTEL_USB_RID_A;
 	default:
-		/*
-		 * Once we have IIO support for the GPADC we should read
-		 * the USBID GPADC channel here and determine ACA role
-		 * based on that.
-		 */
 		return INTEL_USB_ID_FLOAT;
 	}
 }
-- 
2.31.1

Re: [PATCH v2 10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Yauhen Kharuzhy]

On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> patch for adding regulator support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 2bdfb58cda75..3c41fe86b3d3 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>  	return PTR_ERR_OR_ZERO(bq->charger);
>  }
>  
> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
> +{
> +	int ret;
> +
> +	ret = bq25890_field_write(bq, F_OTG_CFG, val);
> +	if (ret < 0)
> +		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);

Just a note: if a connected USB device has relative big capacitor
at power wires inside, then a starting current pulse may be enough to
overload the boost reguator and VBUS will not be powered. I met this
at Yoga Book: the firmware set boost current limit to 1.4 A (default
value for bq25892) but when USB hub connected, the BOOST_FAULT event
appeared.

To avoid this, Lenovo uses following trick in its kernel: set a boost
current limit to big value (2.1 A), wait some time (500 ms) and set
the current limit to right value (1.4A). This provides enough current to
charge capacitors in the connected device but saves desired long-time limit
to prevent overloading if the device consumes too much power itself.



-- 
Yauhen Kharuzhy

Re: [PATCH v2 10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Hans de Goede]

Hi Yauhen,

On 11/15/21 11:11, Yauhen Kharuzhy wrote:
> On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
>> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>> patch for adding regulator support.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 2bdfb58cda75..3c41fe86b3d3 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>>  	return PTR_ERR_OR_ZERO(bq->charger);
>>  }
>>  
>> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = bq25890_field_write(bq, F_OTG_CFG, val);
>> +	if (ret < 0)
>> +		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
> 
> Just a note: if a connected USB device has relative big capacitor
> at power wires inside, then a starting current pulse may be enough to
> overload the boost reguator and VBUS will not be powered. I met this
> at Yoga Book: the firmware set boost current limit to 1.4 A (default
> value for bq25892) but when USB hub connected, the BOOST_FAULT event
> appeared.
> 
> To avoid this, Lenovo uses following trick in its kernel: set a boost
> current limit to big value (2.1 A), wait some time (500 ms) and set
> the current limit to right value (1.4A). This provides enough current to
> charge capacitors in the connected device but saves desired long-time limit
> to prevent overloading if the device consumes too much power itself.

Right I saw this in your git repo, but I cannot reproduce the issue (1)
I was hoping that since you can reproduce this, that you can rebase
your fix on top of my patch-set ?

Also I'm wondering if this behavior should be the default, I believe
that the max. boost current may also be dependent on some external
factors, so maybe we should make this behavior conditional on a
new device-property ?

Regards,

Hans



1) I must admit I did not try really hard, I guess I could try an
USB powered hdd enclosure with a spinning disk

What device are you seeing this with?

Re: [PATCH v2 04/20] power: supply: bq25890: Reduce reported CONSTANT_CHARGE_CURRENT_MAX for low temperatures

[Hans de Goede]

Hi,

On 11/14/21 18:03, Hans de Goede wrote:
> From: Yauhen Kharuzhy <jekhor@gmail.com>
> 
> Take into account possible current reduction due to low-temperature when
> reading POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX. As described in
> the datasheet in cool (0-20° Celcius) conditions the current limit is
> decreased to 20% or 50% of ICHG field value depended on JEITA_ISET field.
> 
> Also add NTC_FAULT field value to the debug message in
> bq25890_get_chip_state().
> 
> Changed by Hans de Goede:
> - Fix reading F_CHG_FAULT instead of F_NTC_FIELD for state->ntc_fault
> - Only read JEITA_ISET field if necessary
> - Tweak commit message a bit
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.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 b208cc2193b8..617a653221ab 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -94,6 +94,7 @@ struct bq25890_state {
>  	u8 vsys_status;
>  	u8 boost_fault;
>  	u8 bat_fault;
> +	u8 ntc_fault;
>  };
>  
>  struct bq25890_device {
> @@ -383,6 +384,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,6 +483,18 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
>  
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>  		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) {
> +			ret = bq25890_field_read(bq, F_JEITA_ISET);
> +			if (ret < 0)
> +				return ret;
> +
> +			if (ret)
> +				val->intval /= 5;
> +			else
> +				val->intval /= 2;
> +		}
>  		break;
>  
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> @@ -486,6 +507,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;

Ugh, this should not be here. I guess this is a leftover from an attempt to
also apply temperature correction to the POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE
prop.

I'll drop this for v3 of the series. Note I'll first wait a week or so for
other feedback.

Regards,

Hans




> @@ -549,7 +574,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_NTC_FAULT,	&state->ntc_fault}
>  	};
>  
>  	for (i = 0; i < ARRAY_SIZE(state_fields); i++) {
> @@ -560,9 +586,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;
>  }
> 

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

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:03 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> This is version 2 of my series previously titled:
> "[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC
> without USB-PD support".
>
> So far almost all the kernel code surrounding the Cherry Trail Whiskey Cove
> PMIC has been developed on the GPD win / pocket devices and it has various
> assumption based on that. In the mean time I've learned (and gotten access
> to) about 2 more designs and none of the 3 now known designs use a single
> standard setup for the charger, fuel-gauge and other chips surrounding the
> PMIC / charging+data USB port:
>
> 1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
> but the Pocket re-uses the GPD Win's design in a different housing:
>
> The WC PMIC is connected to a TI BQ24292i charger, paired with
> a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
> a PI3USB30532 USB switch, for a fully functional Type-C port.
>
> 2. The Xiaomi Mi Pad 2:
>
> The WC PMIC is connected to a TI BQ25890 charger, paired with
> a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
> detection, for a USB-2 only Type-C port without PD.
>
> 3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:
>
> The WC PMIC is connected to a TI BQ25892 charger, paired with
> a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
> detection and using the BQ25892's Mediatek Pump Express+ (1.0)
>
> ###
>
> Unlike what is normal on X86 this diversity in designs is not handled /
> abstracted away by the ACPI tables.

I will briefly look into it, right now two observations (or nit-picks):
- you may utilize Co-developed-by tag when it makes sense
- I would rather see "x86/ACPI" in all texts (note small "x")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/20] power: supply: bq25890: Drop dev->platform_data == NULL check

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Drop the "if (!dev->platform_data)" check, this seems to be an attempt
> for allowing loading the driver on devices without devicetree stemming
> from the initial commit of the driver (with the presumed intention being
> the "return -ENODEV" else branch getting replaced with something else).
>
> With the new "linux,skip-init" and "linux,read-back-settings" properties
> the driver can actually supports devices without devicetree and this
> check no longer makes sense.
>
> While at it also switch to dev_err_probe(), which is already used in

"While at it, also ..."

> various other places in the driver.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq25890_charger.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index a69a2173e31a..2bdfb58cda75 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1017,16 +1017,9 @@ static int bq25890_probe(struct i2c_client *client,
>                 return ret;
>         }
>
> -       if (!dev->platform_data) {
> -               ret = bq25890_fw_probe(bq);
> -               if (ret < 0) {
> -                       dev_err(dev, "Cannot read device properties: %d\n",
> -                               ret);
> -                       return ret;
> -               }
> -       } else {
> -               return -ENODEV;
> -       }
> +       ret = bq25890_fw_probe(bq);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "reading device properties\n");
>
>         ret = bq25890_hw_init(bq);
>         if (ret < 0) {
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 11/16/21 12:00, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:03 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> This is version 2 of my series previously titled:
>> "[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC
>> without USB-PD support".
>>
>> So far almost all the kernel code surrounding the Cherry Trail Whiskey Cove
>> PMIC has been developed on the GPD win / pocket devices and it has various
>> assumption based on that. In the mean time I've learned (and gotten access
>> to) about 2 more designs and none of the 3 now known designs use a single
>> standard setup for the charger, fuel-gauge and other chips surrounding the
>> PMIC / charging+data USB port:
>>
>> 1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
>> but the Pocket re-uses the GPD Win's design in a different housing:
>>
>> The WC PMIC is connected to a TI BQ24292i charger, paired with
>> a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
>> a PI3USB30532 USB switch, for a fully functional Type-C port.
>>
>> 2. The Xiaomi Mi Pad 2:
>>
>> The WC PMIC is connected to a TI BQ25890 charger, paired with
>> a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
>> detection, for a USB-2 only Type-C port without PD.
>>
>> 3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:
>>
>> The WC PMIC is connected to a TI BQ25892 charger, paired with
>> a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
>> detection and using the BQ25892's Mediatek Pump Express+ (1.0)
>>
>> ###
>>
>> Unlike what is normal on X86 this diversity in designs is not handled /
>> abstracted away by the ACPI tables.
> 
> I will briefly look into it, right now two observations (or nit-picks):

Thank you.

> - you may utilize Co-developed-by tag when it makes sense

Right, I intended to do so in patch 13/20, but I now see that I
somehow forgot that :)

> - I would rather see "x86/ACPI" in all texts (note small "x")

Ack.

Regards,

Hans

Re: [PATCH v2 11/20] power: supply: bq25890: Add support for registering the Vbus boost converter as a regulator

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The bq25890_charger code supports enabling/disabling the boost converter
> based on usb-phy notifications. But the usb-phy framework is not used on
> all boards/platforms. At support for registering the Vbus boost converter
> as a standard regulator when there is no usb-phy on the board.
>
> Also add support for providing regulator_init_data through platform_data
> for use on boards where device-tree is not used and the platform code must
> thus provide the regulator_init_data.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

One remark below.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - When the usb-phy framework is not used, turn off the Vboost regulator
>   on shutdown
> - Some minor code-tweaks based on Andy's review
> ---
>  drivers/power/supply/bq25890_charger.c | 80 ++++++++++++++++++++++++++
>  include/linux/power/bq25890_charger.h  | 15 +++++
>  2 files changed, 95 insertions(+)
>  create mode 100644 include/linux/power/bq25890_charger.h
>
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 3c41fe86b3d3..e06ca7b0eb3e 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -8,7 +8,9 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/power_supply.h>
> +#include <linux/power/bq25890_charger.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
>  #include <linux/types.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
> @@ -845,6 +847,45 @@ static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
>         return NOTIFY_OK;
>  }
>
> +#ifdef CONFIG_REGULATOR
> +static int bq25890_vbus_enable(struct regulator_dev *rdev)
> +{
> +       struct bq25890_device *bq = rdev_get_drvdata(rdev);
> +
> +       return bq25890_set_otg_cfg(bq, 1);
> +}
> +
> +static int bq25890_vbus_disable(struct regulator_dev *rdev)
> +{
> +       struct bq25890_device *bq = rdev_get_drvdata(rdev);
> +
> +       return bq25890_set_otg_cfg(bq, 0);
> +}
> +
> +static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
> +{
> +       struct bq25890_device *bq = rdev_get_drvdata(rdev);
> +
> +       return bq25890_field_read(bq, F_OTG_CFG);
> +}
> +
> +static const struct regulator_ops bq25890_vbus_ops = {
> +       .enable = bq25890_vbus_enable,
> +       .disable = bq25890_vbus_disable,
> +       .is_enabled = bq25890_vbus_is_enabled,
> +};
> +
> +static const struct regulator_desc bq25890_vbus_desc = {
> +       .name = "usb_otg_vbus",
> +       .of_match = "usb-otg-vbus",
> +       .type = REGULATOR_VOLTAGE,
> +       .owner = THIS_MODULE,
> +       .ops = &bq25890_vbus_ops,
> +       .fixed_uV = 5000000,
> +       .n_voltages = 1,
> +};
> +#endif
> +
>  static int bq25890_get_chip_version(struct bq25890_device *bq)
>  {
>         int id, rev;
> @@ -1044,6 +1085,22 @@ static int bq25890_probe(struct i2c_client *client,
>                 bq->usb_nb.notifier_call = bq25890_usb_notifier;
>                 usb_register_notifier(bq->usb_phy, &bq->usb_nb);
>         }
> +#ifdef CONFIG_REGULATOR
> +       else {
> +               struct bq25890_platform_data *pdata = dev_get_platdata(dev);
> +               struct regulator_config cfg = { };
> +               struct regulator_dev *reg;
> +
> +               cfg.dev = dev;
> +               cfg.driver_data = bq;
> +               if (pdata)
> +                       cfg.init_data = pdata->regulator_init_data;
> +
> +               reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
> +               if (IS_ERR(reg))
> +                       return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
> +       }
> +#endif

I just realized that you may use even

} else {
#ifdef
...
#endif
}

w/o any compiler warning or so. But it's up to you.

>         ret = bq25890_power_supply_init(bq);
>         if (ret < 0) {
> @@ -1082,6 +1139,28 @@ static int bq25890_remove(struct i2c_client *client)
>         return 0;
>  }
>
> +static void bq25890_shutdown(struct i2c_client *client)
> +{
> +       struct bq25890_device *bq = i2c_get_clientdata(client);
> +
> +       /*
> +        * TODO this if + return should probably be removed, but that would
> +        * introduce a function change for boards using the usb-phy framework.
> +        * This needs to be tested on such a board before making this change.
> +        */
> +       if (!IS_ERR_OR_NULL(bq->usb_phy))
> +               return;
> +
> +       /*
> +        * Turn off the 5v Boost regulator which outputs Vbus to the device's
> +        * Micro-USB or Type-C USB port. Leaving this on drains power and
> +        * this avoids the PMIC on some device-models seeing this as Vbus
> +        * getting inserted after shutdown, causing the device to immediately
> +        * power-up again.
> +        */
> +       bq25890_set_otg_cfg(bq, 0);
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int bq25890_suspend(struct device *dev)
>  {
> @@ -1161,6 +1240,7 @@ static struct i2c_driver bq25890_driver = {
>         },
>         .probe = bq25890_probe,
>         .remove = bq25890_remove,
> +       .shutdown = bq25890_shutdown,
>         .id_table = bq25890_i2c_ids,
>  };
>  module_i2c_driver(bq25890_driver);
> diff --git a/include/linux/power/bq25890_charger.h b/include/linux/power/bq25890_charger.h
> new file mode 100644
> index 000000000000..c706ddb77a08
> --- /dev/null
> +++ b/include/linux/power/bq25890_charger.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Platform data for the TI bq25890 battery charger driver.
> + */
> +
> +#ifndef _BQ25890_CHARGER_H_
> +#define _BQ25890_CHARGER_H_
> +
> +struct regulator_init_data;
> +
> +struct bq25890_platform_data {
> +       const struct regulator_init_data *regulator_init_data;
> +};
> +
> +#endif
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/20] power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Yauhen Kharuzhy <jekhor@gmail.com>
>
> Add a "linux,pump-express-vbus-max" property which indicates if the Pump
> Express+ protocol should be used to increase the charging protocol.
>
> If this new property is set and a DCP charger is detected then request
> a higher charging voltage through the Pump Express+ protocol.
>
> So far this new property is only used on X86/ACPI (non devicetree) devs,
> IOW it is not used in actual devicetree files. The devicetree-bindings
> maintainers have requested properties like these to not be added to the
> devicetree-bindings, so the new property is deliberately not added
> to the existing devicetree-bindings.
>
> Changes by Hans de Goede:
> - Port to my bq25890 patch-series + various cleanups
> - Make behavior configurable through a new "linux,pump-express-vbus-max"
>   device-property
> - Sleep 1 second before re-checking the Vbus voltage after requesting
>   it to be raised, to ensure that the ADC has time to sampled the new Vbus
> - Add VBUSV bq25890_tables[] entry and use it in bq25890_get_vbus_voltage()
> - Tweak commit message

...

> +#define PUMP_EXPRESS_START_DELAY       (5 * HZ)
> +#define PUMP_EXPRESS_MAX_TRIES         6
> +#define PUMP_EXPRESS_VBUS_MARGIN       1000000

Units? Perhaps "_uV"?

...

> +               dev_dbg(bq->dev, "input voltage = %d mV\n", voltage);

Just to be sure, is it indeed "mV" and not "uV"?

...

> +               while (bq25890_field_read(bq, F_PUMPX_UP) == 1)
> +                       msleep(100);

Infinite loop?

Sounds like a good candidate to switch to read_poll_timeout() // note
it accepts any type of (op) with a variadic number of args.

...

> +error:

error_print: ?

> +       dev_err(bq->dev, "Failed to request hi-voltage charging\n");

...

> +       ret = device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
> +                                      &bq->pump_express_vbus_max);
> +       if (ret < 0)
> +               bq->pump_express_vbus_max = 0;

Isn't it 0 by default?

Anyway, for all optional properties one may use

bq->...property... = $default;
device_property_read_u32(bq->dev, "linux,...property...", &bq->...property...);

I.e. no conditional needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/20] mfd: intel_soc_pmic_chtwc: Add intel_cht_wc_get_model() helper function

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Tablet / laptop designs using an Intel Cherry Trail x86 main SoC with
> an Intel Whiskey Cove PMIC do not use a single standard setup for

does not

> the charger, fuel-gauge and other chips surrounding the PMIC /
> charging+data USB port.
>
> Unlike what is normal on X86 this diversity in designs is not handled
> by the ACPI tables. On 2 of the 3 known designs there are no standard
> (PNP0C0A) ACPI battery devices and on the 3th design the ACPI battery
> device does not work under Linux due to it requiring non-standard
> and undocumented ACPI behavior.
>
> So to make things work under Linux we use native charger and fuel-gauge
> drivers on these devices, re-using the native drivers used on ARM boards
> with the same charger / fuel-gauge ICs.
>
> This requires various MFD-cell drivers for the CHT-WC PMIC cells to
> know which model they are exactly running on so that they can e.g.
> instantiate an I2C-client for the right model charger-IC (the charger
> is connected to an I2C-controller which is part of the PMIC).
>
> Rather then duplicating DMI-id matching to check which model we are
> running on in each MFD-cell driver add a helper function for this
> and make this id all 3 known models:
>
> 1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
> but the Pocket re-uses the GPD Win's design in a different housing:
>
> The WC PMIC is connected to a TI BQ24292i charger, paired with
> a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
> a PI3USB30532 USB switch, for a fully functional Type-C port.
>
> 2. The Xiaomi Mi Pad 2:
>
> The WC PMIC is connected to a TI BQ25890 charger, paired with
> a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
> detection, for a USB-2 only Type-C port without PD.
>
> 3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:
>
> The WC PMIC is connected to a TI BQ25892 charger, paired with
> a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
> detection and using the BQ25892's Mediatek Pump Express+ (1.0)
> support to enable charging with up to 12V through a micro-USB port.

...

> +               /*
> +                * Note this may not seem like a very unique match, but in the
> +                * 24000+ DMI decode dumps from linux-hardware.org only 42 have

Can you add https:// (or is it gopher? :)

> +                * a board_vendor value of "AMI Corporation" and of those 42
> +                * only 1 (the GPD win/pocket entry) has a board_name of
> +                * "Default string". Also very few devices have both board_ and
> +                * product_name not set.
> +                */

...

> +enum intel_cht_wc_models intel_cht_wc_get_model(void)
> +{
> +       const struct dmi_system_id *id;
> +
> +       id = dmi_first_match(cht_wc_model_dmi_ids);
> +       if (!id)
> +               return INTEL_CHT_WC_UNKNOWN;
> +
> +       return (long)id->driver_data;

Why not proper casting, i.e. (enum intel_...)?

> +}
> +EXPORT_SYMBOL_GPL(intel_cht_wc_get_model);

Are you planning to use EXPORT_SYMBOL_GPL_NS()? If not, please consider it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 15/20] i2c: cht-wc: Make charger i2c-client instantiation board/device-model specific

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The i2c-controller on the Cherry Trail - Whiskey Cove PMIC is special
> in that it is always connected to the I2C charger IC of the board on
> which the PMIC is used; and the charger IC is not described in ACPI,
> so the i2c-cht-wc code needs to instantiate an i2c-client for it itself.
>
> So far this was hardcoded to instantiate an i2c-client for the
> bq24292i, with all properties, etc. set to match how this charger
> is used on the GPD win and GPD pcoket devices.

"pocket" I believe?

> There is a rudimentary check to make sure the ACPI tables are at least
> somewhat as expected, but this is far from accurate, leading to
> a wrong i2c-client being instantiated for the charger on some boards.
>
> Switch to the new DMI based intel_cht_wc_get_model() helper which is
> exported by the MFD driver for the CHT Whiskey Cove PMIC to help PMIC
> cell drivers like the i2c-cht-wc code reliably detect which board
> they are running on.
>
> And add board_info for the charger ICs as found on the other 2 known
> boards with a Whisky Cove PMIC.
>
> This has been tested on all 3 known boards.

...

> +/********** Lenovo Yogabook YB1-X90F/-X91F/-X91L charger settings **********/
> +static const char * const lenovo_yb1_bq25892_suppliers[] = {
> +       "cht_wcove_pwrsrc" };

Something went wrong with indentation...

...

> +       /*
> +        * The firmware sets everything to the defaults, which leads to a
> +        * somewhat low charge-current of 2048mA and worse to a batter-voltage

battery?

> +        * of 4.2V instead of 4.35V (when booted without a charger connected).
> +        * Use our own values instead of "linux,read-back-settings" to fix this.
> +        */

...

> +       switch (intel_cht_wc_get_model()) {
> +       case INTEL_CHT_WC_GPD_WIN_POCKET:
> +               board_info = &gpd_win_board_info;
> +               break;
> +       case INTEL_CHT_WC_XIAOMI_MIPAD2:
> +               board_info = &xiaomi_mipad2_board_info;
> +               break;
> +       case INTEL_CHT_WC_LENOVO_YOGABOOK1:
> +               board_info = &lenovo_yogabook1_board_info;
> +               break;
> +       default:
> +               dev_warn(&pdev->dev, "Unknown model, not instantiating charger device\n");

break;

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 17/20] extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C connectors

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> So far the extcon-intel-cht-wc code has only been tested on devices with
> a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support
> through a FUSB302 Type-C controller.
>
> Some devices with the intel-cht-wc PMIC however come with an USB-micro-B
> connector, or an USB-2 only Type-C connector without USB-PD.
>
> Which device-model we are running on can be identified with the new
> intel_cht_wc_get_model() helper and on models without a Type-C controller
> the extcon code must control the Vbus 5V boost converter and the USB role
> switch depending on the detected cable-type.

...

>  config EXTCON_INTEL_CHT_WC
>         tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver"
> -       depends on INTEL_SOC_PMIC_CHTWC

> +       depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT

Having these two in one expression sounds a bit alogical to me, can
you just add a separate "depends on"?

> +       select USB_ROLE_SWITCH

...

> +       if (ext->vbus_boost && ext->vbus_boost_enabled != enable) {
> +               if (enable)
> +                       ret = regulator_enable(ext->vbus_boost);
> +               else
> +                       ret = regulator_disable(ext->vbus_boost);

Redundant blank line here (but it's up to you)

> +               if (ret == 0)
> +                       ext->vbus_boost_enabled = enable;
> +               else
> +                       dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret);

Why not a traditional pattern, i.e. error handling first?

> +       }

...

> +/* Some boards require controlling the role-sw and vbus based on the id-pin */

Vbus ? VBUS? Here and there the inconsistency of some terms...

...

> +       ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus");
> +       if (IS_ERR(ext->vbus_boost)) {
> +               ret = PTR_ERR(ext->vbus_boost);
> +               if (ret == -ENODEV)
> +                       ret = -EPROBE_DEFER;
> +
> +               return dev_err_probe(ext->dev, ret, "getting vbus regulator");

Can be also written as

    if (PTR_ERR(ext->vbus_boost) == -ENODEV ||
PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER)
        return dev_err_probe(ext->dev, -EPROBE_DEFER, "getting vbus regulator");

    return PTR_ERR(ext->vbus_boost);

but up to you.

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 17/20] extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C connectors

[Andy Shevchenko]

On Tue, Nov 16, 2021 at 1:28 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > +       ext->vbus_boost = devm_regulator_get_optional(ext->dev, "vbus");
> > +       if (IS_ERR(ext->vbus_boost)) {
> > +               ret = PTR_ERR(ext->vbus_boost);
> > +               if (ret == -ENODEV)
> > +                       ret = -EPROBE_DEFER;
> > +
> > +               return dev_err_probe(ext->dev, ret, "getting vbus regulator");
>
> Can be also written as
>
>     if (PTR_ERR(ext->vbus_boost) == -ENODEV ||
> PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER)
>         return dev_err_probe(ext->dev, -EPROBE_DEFER, "getting vbus regulator");
>
>     return PTR_ERR(ext->vbus_boost);

Oops, other way around, of course.

  if (PTR_ERR(ext->vbus_boost) == -ENODEV ||
      PTR_ERR(ext->vbus_boost) == -EPROBE_DEFER)
        return -EPROBE_DEFER;

  return dev_err_probe(ext->dev, PTR_ERR(ext->vbus_boost), "getting
vbus regulator");

> but up to you.
>
> > +       }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 17/20] extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C connectors

[Andy Shevchenko]

On Tue, Nov 16, 2021 at 1:31 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 16, 2021 at 1:28 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:

> > Can be also written as

> Oops, other way around, of course.

Yeah, scratch it. We have nice debugfs which will be missed...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/20] power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol

[Hans de Goede]

Hi,

On 11/16/21 12:14, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Yauhen Kharuzhy <jekhor@gmail.com>
>>
>> Add a "linux,pump-express-vbus-max" property which indicates if the Pump
>> Express+ protocol should be used to increase the charging protocol.
>>
>> If this new property is set and a DCP charger is detected then request
>> a higher charging voltage through the Pump Express+ protocol.
>>
>> So far this new property is only used on X86/ACPI (non devicetree) devs,
>> IOW it is not used in actual devicetree files. The devicetree-bindings
>> maintainers have requested properties like these to not be added to the
>> devicetree-bindings, so the new property is deliberately not added
>> to the existing devicetree-bindings.
>>
>> Changes by Hans de Goede:
>> - Port to my bq25890 patch-series + various cleanups
>> - Make behavior configurable through a new "linux,pump-express-vbus-max"
>>   device-property
>> - Sleep 1 second before re-checking the Vbus voltage after requesting
>>   it to be raised, to ensure that the ADC has time to sampled the new Vbus
>> - Add VBUSV bq25890_tables[] entry and use it in bq25890_get_vbus_voltage()
>> - Tweak commit message
> 
> ...
> 
>> +#define PUMP_EXPRESS_START_DELAY       (5 * HZ)
>> +#define PUMP_EXPRESS_MAX_TRIES         6
>> +#define PUMP_EXPRESS_VBUS_MARGIN       1000000
> 
> Units? Perhaps "_uV"?
> 
> ...
> 
>> +               dev_dbg(bq->dev, "input voltage = %d mV\n", voltage);
> 
> Just to be sure, is it indeed "mV" and not "uV"?

It is uV, will fix for the next version.

> 
> ...
> 
>> +               while (bq25890_field_read(bq, F_PUMPX_UP) == 1)
>> +                       msleep(100);
> 
> Infinite loop?
> 
> Sounds like a good candidate to switch to read_poll_timeout() // note> it accepts any type of (op) with a variadic number of args.

Good catch, will fix.

> 
> ...
> 
>> +error:
> 
> error_print: ?
> 
>> +       dev_err(bq->dev, "Failed to request hi-voltage charging\n");
> 
> ...
> 
>> +       ret = device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
>> +                                      &bq->pump_express_vbus_max);
>> +       if (ret < 0)
>> +               bq->pump_express_vbus_max = 0;
> 
> Isn't it 0 by default?
> 
> Anyway, for all optional properties one may use
> 
> bq->...property... = $default;
> device_property_read_u32(bq->dev, "linux,...property...", &bq->...property...);
> 
> I.e. no conditional needed.

Ack, will fix.

Regards,

Hans

Re: [PATCH v2 14/20] mfd: intel_soc_pmic_chtwc: Add intel_cht_wc_get_model() helper function

[Hans de Goede]

Hi,

On 11/16/21 12:18, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Tablet / laptop designs using an Intel Cherry Trail x86 main SoC with
>> an Intel Whiskey Cove PMIC do not use a single standard setup for
> 
> does not
> 
>> the charger, fuel-gauge and other chips surrounding the PMIC /
>> charging+data USB port.
>>
>> Unlike what is normal on X86 this diversity in designs is not handled
>> by the ACPI tables. On 2 of the 3 known designs there are no standard
>> (PNP0C0A) ACPI battery devices and on the 3th design the ACPI battery
>> device does not work under Linux due to it requiring non-standard
>> and undocumented ACPI behavior.
>>
>> So to make things work under Linux we use native charger and fuel-gauge
>> drivers on these devices, re-using the native drivers used on ARM boards
>> with the same charger / fuel-gauge ICs.
>>
>> This requires various MFD-cell drivers for the CHT-WC PMIC cells to
>> know which model they are exactly running on so that they can e.g.
>> instantiate an I2C-client for the right model charger-IC (the charger
>> is connected to an I2C-controller which is part of the PMIC).
>>
>> Rather then duplicating DMI-id matching to check which model we are
>> running on in each MFD-cell driver add a helper function for this
>> and make this id all 3 known models:
>>
>> 1. The GPD Win and GPD Pocket mini-laptops, these are really 2 models
>> but the Pocket re-uses the GPD Win's design in a different housing:
>>
>> The WC PMIC is connected to a TI BQ24292i charger, paired with
>> a Maxim MAX17047 fuelgauge + a FUSB302 USB Type-C Controller +
>> a PI3USB30532 USB switch, for a fully functional Type-C port.
>>
>> 2. The Xiaomi Mi Pad 2:
>>
>> The WC PMIC is connected to a TI BQ25890 charger, paired with
>> a TI BQ27520 fuelgauge, using the TI BQ25890 for BC1.2 charger type
>> detection, for a USB-2 only Type-C port without PD.
>>
>> 3. The Lenovo Yoga Book YB1-X90 / Lenovo Yoga Book YB1-X91 series:
>>
>> The WC PMIC is connected to a TI BQ25892 charger, paired with
>> a TI BQ27542 fuelgauge, using the WC PMIC for BC1.2 charger type
>> detection and using the BQ25892's Mediatek Pump Express+ (1.0)
>> support to enable charging with up to 12V through a micro-USB port.
> 
> ...
> 
>> +               /*
>> +                * Note this may not seem like a very unique match, but in the
>> +                * 24000+ DMI decode dumps from linux-hardware.org only 42 have
> 
> Can you add https:// (or is it gopher? :)

linux-hardware.org is intended here as an identifier of the projects, not an
URL. The DMI decode database lives here:

https://github.com/linuxhw/DMI.git

But I don't believe that adding the exact URL in the comment is important,
esp. since that may change over time.

> 
>> +                * a board_vendor value of "AMI Corporation" and of those 42
>> +                * only 1 (the GPD win/pocket entry) has a board_name of
>> +                * "Default string". Also very few devices have both board_ and
>> +                * product_name not set.
>> +                */
> 
> ...
> 
>> +enum intel_cht_wc_models intel_cht_wc_get_model(void)
>> +{
>> +       const struct dmi_system_id *id;
>> +
>> +       id = dmi_first_match(cht_wc_model_dmi_ids);
>> +       if (!id)
>> +               return INTEL_CHT_WC_UNKNOWN;
>> +
>> +       return (long)id->driver_data;
> 
> Why not proper casting, i.e. (enum intel_...)?

Because sizeof(enum) != sizeof(void *) so then the compiler will
complain. Where as sizeof(long) == sizeof(void *)

> 
>> +}
>> +EXPORT_SYMBOL_GPL(intel_cht_wc_get_model);
> 
> Are you planning to use EXPORT_SYMBOL_GPL_NS()? If not, please consider it.

No I was not planning on this and it seems overkill for just a single
exported symbol.

Regards,

Hans

Re: [PATCH v2 15/20] i2c: cht-wc: Make charger i2c-client instantiation board/device-model specific

[Hans de Goede]

Hi,

On 11/16/21 12:20, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The i2c-controller on the Cherry Trail - Whiskey Cove PMIC is special
>> in that it is always connected to the I2C charger IC of the board on
>> which the PMIC is used; and the charger IC is not described in ACPI,
>> so the i2c-cht-wc code needs to instantiate an i2c-client for it itself.
>>
>> So far this was hardcoded to instantiate an i2c-client for the
>> bq24292i, with all properties, etc. set to match how this charger
>> is used on the GPD win and GPD pcoket devices.
> 
> "pocket" I believe?
> 
>> There is a rudimentary check to make sure the ACPI tables are at least
>> somewhat as expected, but this is far from accurate, leading to
>> a wrong i2c-client being instantiated for the charger on some boards.
>>
>> Switch to the new DMI based intel_cht_wc_get_model() helper which is
>> exported by the MFD driver for the CHT Whiskey Cove PMIC to help PMIC
>> cell drivers like the i2c-cht-wc code reliably detect which board
>> they are running on.
>>
>> And add board_info for the charger ICs as found on the other 2 known
>> boards with a Whisky Cove PMIC.
>>
>> This has been tested on all 3 known boards.
> 
> ...
> 
>> +/********** Lenovo Yogabook YB1-X90F/-X91F/-X91L charger settings **********/
>> +static const char * const lenovo_yb1_bq25892_suppliers[] = {
>> +       "cht_wcove_pwrsrc" };
> 
> Something went wrong with indentation...
> 
> ...
> 
>> +       /*
>> +        * The firmware sets everything to the defaults, which leads to a
>> +        * somewhat low charge-current of 2048mA and worse to a batter-voltage
> 
> battery?
> 
>> +        * of 4.2V instead of 4.35V (when booted without a charger connected).
>> +        * Use our own values instead of "linux,read-back-settings" to fix this.
>> +        */
> 
> ...

Ack to all of the above remarks.

> 
>> +       switch (intel_cht_wc_get_model()) {
>> +       case INTEL_CHT_WC_GPD_WIN_POCKET:
>> +               board_info = &gpd_win_board_info;
>> +               break;
>> +       case INTEL_CHT_WC_XIAOMI_MIPAD2:
>> +               board_info = &xiaomi_mipad2_board_info;
>> +               break;
>> +       case INTEL_CHT_WC_LENOVO_YOGABOOK1:
>> +               board_info = &lenovo_yogabook1_board_info;
>> +               break;
>> +       default:
>> +               dev_warn(&pdev->dev, "Unknown model, not instantiating charger device\n");
> 
> break;

Why ?  Having a default without a break at the end of a switch-case
is quite a normal thing to do.

> 
>> +       }
> 

Regards,

Hams

Re: [PATCH v2 17/20] extcon: intel-cht-wc: Support devs with Micro-B / USB-2 only Type-C connectors

[Hans de Goede]

Hi,

On 11/16/21 12:28, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> So far the extcon-intel-cht-wc code has only been tested on devices with
>> a Type-C connector with USB-PD, USB3 (superspeed) and DP-altmode support
>> through a FUSB302 Type-C controller.
>>
>> Some devices with the intel-cht-wc PMIC however come with an USB-micro-B
>> connector, or an USB-2 only Type-C connector without USB-PD.
>>
>> Which device-model we are running on can be identified with the new
>> intel_cht_wc_get_model() helper and on models without a Type-C controller
>> the extcon code must control the Vbus 5V boost converter and the USB role
>> switch depending on the detected cable-type.
> 
> ...
> 
>>  config EXTCON_INTEL_CHT_WC
>>         tristate "Intel Cherrytrail Whiskey Cove PMIC extcon driver"
>> -       depends on INTEL_SOC_PMIC_CHTWC
> 
>> +       depends on INTEL_SOC_PMIC_CHTWC && USB_SUPPORT
> 
> Having these two in one expression sounds a bit alogical to me, can
> you just add a separate "depends on"?

Sure.

> 
>> +       select USB_ROLE_SWITCH
> 
> ...
> 
>> +       if (ext->vbus_boost && ext->vbus_boost_enabled != enable) {
>> +               if (enable)
>> +                       ret = regulator_enable(ext->vbus_boost);
>> +               else
>> +                       ret = regulator_disable(ext->vbus_boost);
> 
> Redundant blank line here (but it's up to you)
> 
>> +               if (ret == 0)
>> +                       ext->vbus_boost_enabled = enable;
>> +               else
>> +                       dev_err(ext->dev, "Error updating Vbus boost regulator: %d\n", ret);
> 
> Why not a traditional pattern, i.e. error handling first?

As I've mentioned before (to a very similar remark) error handling
first is not the traditional pattern, at least not for me.

Traditionally (to me) the else case is the error case. This
is just how humans work. E.g. if I need help for something
saying something like:

"If you have time can you help me with this please? Otherwise
I'm afraid that I am never going to solve this."

Feels natural, where as saying it like this:

"If you do not have time I'm afraid I am never going to solve
this, otherwise can you help me with this please ?"

Feels quite unnatural, at least to me.

>> +       }
> 
> ...
> 
>> +/* Some boards require controlling the role-sw and vbus based on the id-pin */
> 
> Vbus ? VBUS? Here and there the inconsistency of some terms...

"Vbus", I'll try to fix this up everywhere.

Regards,

Hans

Re: [PATCH v2 19/20] extcon: intel-cht-wc: Add support for registering a power_supply class-device

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 7:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The bq25890 used on the Yogabook YB1-X90 / -X91 models relies on
> the extcon-driver's BC-1.2 charger detection, and the bq25890 driver
> expect this info to be  available through a parent power_supply
> class-device which models the detected charger (idem to how the Type-C
> TCPM code registers a power_supply classdev for the connected charger).
>
> Add support for registering the power_supply class-device expected
> by this setup.

...

> +       struct cht_wc_extcon_data *ext = power_supply_get_drvdata(psy);

> +       int ret = 0;

Not needed.

> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_USB_TYPE:
> +               val->intval = ext->usb_type;
> +               break;
> +       case POWER_SUPPLY_PROP_ONLINE:
> +               val->intval = ext->usb_type ? 1 : 0;
> +               break;
> +       default:

> +               ret = -EINVAL;
> +               break;

return -EINVAL;

> +       }
> +
> +       return ret;

return 0;

> +}

P.S. And here you have a "break", which is nice!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 16/20] extcon: intel-cht-wc: Use new intel_cht_wc_get_model() helper

[Chanwoo Choi]

On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
> The CHT_WC_VBUS_GPIO_CTLO GPIO actually driving an external 5V Vboost
> converter for Vbus depends on the board on which the Cherry Trail -
> Whiskey Cove PMIC is actually used.
> 
> Since the information about the exact PMIC setup is necessary in other
> places too, the drivers/mfd/intel_soc_pmic_chtwc.c code now has a new
> intel_cht_wc_get_model() helper.
> 
> Only poke the CHT_WC_VBUS_GPIO_CTLO GPIO if this new helper returns
> INTEL_CHT_WC_GPD_WIN_POCKET, which indicates the Type-C (with PD and
> DP-altmode) setup used on the GPD pocket and GPD win; and on which
> this GPIO actually controls an external 5V Vboost converter.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 35 +++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 771f6f4cf92e..a5aeeecc44fb 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -14,6 +14,7 @@
>   #include <linux/module.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/platform_device.h>
> +#include <linux/property.h>
>   #include <linux/regmap.h>
>   #include <linux/slab.h>
>   
> @@ -358,20 +359,26 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	if (IS_ERR(ext->edev))
>   		return PTR_ERR(ext->edev);
>   
> -	/*
> -	 * When a host-cable is detected the BIOS enables an external 5v boost
> -	 * converter to power connected devices there are 2 problems with this:
> -	 * 1) This gets seen by the external battery charger as a valid Vbus
> -	 *    supply and it then tries to feed Vsys from this creating a
> -	 *    feedback loop which causes aprox. 300 mA extra battery drain
> -	 *    (and unless we drive the external-charger-disable pin high it
> -	 *    also tries to charge the battery causing even more feedback).
> -	 * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
> -	 * Since the external battery charger has its own 5v boost converter
> -	 * which does not have these issues, we simply turn the separate
> -	 * external 5v boost converter off and leave it off entirely.
> -	 */
> -	cht_wc_extcon_set_5v_boost(ext, false);
> +	switch (intel_cht_wc_get_model()) {

intel_cht_wc_get_model() is defined in driver/mfd/intel_soc_pmic_chtwc.c

Usually, mfd drivers share the data structure such as struct 
intel_soc_pmic. But, didn't call the exported function for only
specific driver between linux kernel framework (extcon vs. mfd).

So that I think that you better to update the mode information
to 'struct intel_soc_pmic' data structure and then use it
instead of using the exported function which may make the confusion.

> +	case INTEL_CHT_WC_GPD_WIN_POCKET:
> +		/*
> +		 * When a host-cable is detected the BIOS enables an external 5v boost
> +		 * converter to power connected devices there are 2 problems with this:
> +		 * 1) This gets seen by the external battery charger as a valid Vbus
> +		 *    supply and it then tries to feed Vsys from this creating a
> +		 *    feedback loop which causes aprox. 300 mA extra battery drain
> +		 *    (and unless we drive the external-charger-disable pin high it
> +		 *    also tries to charge the battery causing even more feedback).
> +		 * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
> +		 * Since the external battery charger has its own 5v boost converter
> +		 * which does not have these issues, we simply turn the separate
> +		 * external 5v boost converter off and leave it off entirely.
> +		 */
> +		cht_wc_extcon_set_5v_boost(ext, false);
> +		break;
> +	default:
> +		break;
> +	}
>   
>   	/* Enable sw control */
>   	ret = cht_wc_extcon_sw_control(ext, true);
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

[Chanwoo Choi]

Hello,

I think that you need to squash it with patch21
I'm not sure that this patch is either atomic or not because
you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
without explaining why it is no problem. Just mention that
pass the role to next 'switch' cases. But, before this change,
there were any reason to return the type of charger cable
before switch statement.

According to your patch description, you don't need
to make the separate patch of it. Please squash it with patch21.

On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
> Refactor cht_wc_extcon_get_charger() to have all the returns are in
> the "switch (usbsrc)" cases.
> 
> This is a preparation patch for adding support for registering
> a power_supply class device.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 119b83793123..f2b93a99a625 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>   	} while (time_before(jiffies, timeout));
>   
>   	if (status != CHT_WC_USBSRC_STS_SUCCESS) {
> -		if (ignore_errors)
> -			return EXTCON_CHG_USB_SDP; /* Save fallback */
> +		if (!ignore_errors) {
> +			if (status == CHT_WC_USBSRC_STS_FAIL)
> +				dev_warn(ext->dev, "Could not detect charger type\n");
> +			else
> +				dev_warn(ext->dev, "Timeout detecting charger type\n");
> +		}
>   
> -		if (status == CHT_WC_USBSRC_STS_FAIL)
> -			dev_warn(ext->dev, "Could not detect charger type\n");
> -		else
> -			dev_warn(ext->dev, "Timeout detecting charger type\n");
> -		return EXTCON_CHG_USB_SDP; /* Save fallback */
> +		/* Save fallback */
> +		usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>   	}
>   
>   	usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

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

[Sebastian Reichel]

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

Hi,

On Sun, Nov 14, 2021 at 06:03:15PM +0100, Hans de Goede wrote:
> This is version 2 of my series previously titled:
> "[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC
> without USB-PD support". [...]

Apart from the already pointed out things the power-supply changes LGTM.

-- Sebastian

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

Re: [PATCH v2 16/20] extcon: intel-cht-wc: Use new intel_cht_wc_get_model() helper

[Hans de Goede]

Hi,

On 11/17/21 07:47, Chanwoo Choi wrote:
> On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
>> The CHT_WC_VBUS_GPIO_CTLO GPIO actually driving an external 5V Vboost
>> converter for Vbus depends on the board on which the Cherry Trail -
>> Whiskey Cove PMIC is actually used.
>>
>> Since the information about the exact PMIC setup is necessary in other
>> places too, the drivers/mfd/intel_soc_pmic_chtwc.c code now has a new
>> intel_cht_wc_get_model() helper.
>>
>> Only poke the CHT_WC_VBUS_GPIO_CTLO GPIO if this new helper returns
>> INTEL_CHT_WC_GPD_WIN_POCKET, which indicates the Type-C (with PD and
>> DP-altmode) setup used on the GPD pocket and GPD win; and on which
>> this GPIO actually controls an external 5V Vboost converter.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/extcon/extcon-intel-cht-wc.c | 35 +++++++++++++++++-----------
>>   1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>> index 771f6f4cf92e..a5aeeecc44fb 100644
>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/module.h>
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/property.h>
>>   #include <linux/regmap.h>
>>   #include <linux/slab.h>
>>   @@ -358,20 +359,26 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>>       if (IS_ERR(ext->edev))
>>           return PTR_ERR(ext->edev);
>>   -    /*
>> -     * When a host-cable is detected the BIOS enables an external 5v boost
>> -     * converter to power connected devices there are 2 problems with this:
>> -     * 1) This gets seen by the external battery charger as a valid Vbus
>> -     *    supply and it then tries to feed Vsys from this creating a
>> -     *    feedback loop which causes aprox. 300 mA extra battery drain
>> -     *    (and unless we drive the external-charger-disable pin high it
>> -     *    also tries to charge the battery causing even more feedback).
>> -     * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
>> -     * Since the external battery charger has its own 5v boost converter
>> -     * which does not have these issues, we simply turn the separate
>> -     * external 5v boost converter off and leave it off entirely.
>> -     */
>> -    cht_wc_extcon_set_5v_boost(ext, false);
>> +    switch (intel_cht_wc_get_model()) {
> 
> intel_cht_wc_get_model() is defined in driver/mfd/intel_soc_pmic_chtwc.c
> 
> Usually, mfd drivers share the data structure such as struct intel_soc_pmic. But, didn't call the exported function for only
> specific driver between linux kernel framework (extcon vs. mfd).
> 
> So that I think that you better to update the mode information
> to 'struct intel_soc_pmic' data structure and then use it
> instead of using the exported function which may make the confusion.

That is a good idea, thanks.

I've implemented this suggestion for the upcoming v3 of the patch-set.

Regards,

Hans


> 
>> +    case INTEL_CHT_WC_GPD_WIN_POCKET:
>> +        /*
>> +         * When a host-cable is detected the BIOS enables an external 5v boost
>> +         * converter to power connected devices there are 2 problems with this:
>> +         * 1) This gets seen by the external battery charger as a valid Vbus
>> +         *    supply and it then tries to feed Vsys from this creating a
>> +         *    feedback loop which causes aprox. 300 mA extra battery drain
>> +         *    (and unless we drive the external-charger-disable pin high it
>> +         *    also tries to charge the battery causing even more feedback).
>> +         * 2) This gets seen by the pwrsrc block as a SDP USB Vbus supply
>> +         * Since the external battery charger has its own 5v boost converter
>> +         * which does not have these issues, we simply turn the separate
>> +         * external 5v boost converter off and leave it off entirely.
>> +         */
>> +        cht_wc_extcon_set_5v_boost(ext, false);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>>         /* Enable sw control */
>>       ret = cht_wc_extcon_sw_control(ext, true);
>>
> 
> 

Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

[Hans de Goede]

Hi,

On 11/17/21 08:15, Chanwoo Choi wrote:
> Hello,
> 
> I think that you need to squash it with patch21
> I'm not sure that this patch is either atomic or not because
> you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
> without explaining why it is no problem. Just mention that
> pass the role to next 'switch' cases. But, before this change,
> there were any reason to return the type of charger cable
> before switch statement.

The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
will make the following switch-case return EXTCON_CHG_USB_SDP
just as before, so there is no functional change.

> According to your patch description, you don't need
> to make the separate patch of it. Please squash it with patch21.

Having this refactoring in a separate patch makes it easier
to see what is going on in patch 21. So I'm going to keep this
as a separate patch for v3 of this series.

Thanks & Regards,

Hans



> On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
>> Refactor cht_wc_extcon_get_charger() to have all the returns are in
>> the "switch (usbsrc)" cases.
>>
>> This is a preparation patch for adding support for registering
>> a power_supply class device.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>> index 119b83793123..f2b93a99a625 100644
>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>>       } while (time_before(jiffies, timeout));
>>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
>> -        if (ignore_errors)
>> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
>> +        if (!ignore_errors) {
>> +            if (status == CHT_WC_USBSRC_STS_FAIL)
>> +                dev_warn(ext->dev, "Could not detect charger type\n");
>> +            else
>> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
>> +        }
>>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
>> -            dev_warn(ext->dev, "Could not detect charger type\n");
>> -        else
>> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
>> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
>> +        /* Save fallback */
>> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>>       }
>>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
>>
> 
> 

Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

[Chanwoo Choi]

Hi,

On Thu, Nov 18, 2021 at 7:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/17/21 08:15, Chanwoo Choi wrote:
> > Hello,
> >
> > I think that you need to squash it with patch21
> > I'm not sure that this patch is either atomic or not because
> > you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
> > without explaining why it is no problem. Just mention that
> > pass the role to next 'switch' cases. But, before this change,
> > there were any reason to return the type of charger cable
> > before switch statement.
>
> The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
> will make the following switch-case return EXTCON_CHG_USB_SDP
> just as before, so there is no functional change.
>
> > According to your patch description, you don't need
> > to make the separate patch of it. Please squash it with patch21.
>
> Having this refactoring in a separate patch makes it easier
> to see what is going on in patch 21. So I'm going to keep this
> as a separate patch for v3 of this series.

If you want to keep this  patch, please remove the following description.
Instead, just mention to focus on refactor it without behavior changes.

'This is a preparation patch for adding support for registering
a power_supply class device.'

>
>
> > On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
> >> Refactor cht_wc_extcon_get_charger() to have all the returns are in
> >> the "switch (usbsrc)" cases.
> >>
> >> This is a preparation patch for adding support for registering
> >> a power_supply class device.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
> >>   1 file changed, 8 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> >> index 119b83793123..f2b93a99a625 100644
> >> --- a/drivers/extcon/extcon-intel-cht-wc.c
> >> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> >> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
> >>       } while (time_before(jiffies, timeout));
> >>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
> >> -        if (ignore_errors)
> >> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
> >> +        if (!ignore_errors) {
> >> +            if (status == CHT_WC_USBSRC_STS_FAIL)
> >> +                dev_warn(ext->dev, "Could not detect charger type\n");
> >> +            else
> >> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
> >> +        }
> >>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
> >> -            dev_warn(ext->dev, "Could not detect charger type\n");
> >> -        else
> >> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
> >> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
> >> +        /* Save fallback */
> >> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
> >>       }
> >>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
> >>
> >
> >
>


-- 
Best Regards,
Chanwoo Choi

Re: [PATCH v2 15/20] i2c: cht-wc: Make charger i2c-client instantiation board/device-model specific

[Wolfram Sang]

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

On Sun, Nov 14, 2021 at 06:03:30PM +0100, Hans de Goede wrote:
> The i2c-controller on the Cherry Trail - Whiskey Cove PMIC is special
> in that it is always connected to the I2C charger IC of the board on
> which the PMIC is used; and the charger IC is not described in ACPI,
> so the i2c-cht-wc code needs to instantiate an i2c-client for it itself.
> 
> So far this was hardcoded to instantiate an i2c-client for the
> bq24292i, with all properties, etc. set to match how this charger
> is used on the GPD win and GPD pcoket devices.
> 
> There is a rudimentary check to make sure the ACPI tables are at least
> somewhat as expected, but this is far from accurate, leading to
> a wrong i2c-client being instantiated for the charger on some boards.
> 
> Switch to the new DMI based intel_cht_wc_get_model() helper which is
> exported by the MFD driver for the CHT Whiskey Cove PMIC to help PMIC
> cell drivers like the i2c-cht-wc code reliably detect which board
> they are running on.
> 
> And add board_info for the charger ICs as found on the other 2 known
> boards with a Whisky Cove PMIC.
> 
> This has been tested on all 3 known boards.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Here is my Ack to take this via some other tree:

Acked-by: Wolfram Sang <wsa@kernel.org>

No need to send further versions of this series to the i2c-list, I'd
think.


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

Re: [PATCH v2 14/20] mfd: intel_soc_pmic_chtwc: Add intel_cht_wc_get_model() helper function

[Hans de Goede]

Hi,

On 11/16/21 12:18, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 7:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Tablet / laptop designs using an Intel Cherry Trail x86 main SoC with
>> an Intel Whiskey Cove PMIC do not use a single standard setup for
> 
> does not

designs ... do not. Design_s_ so more then one, so "do not" is correct.

<snip>

>> +}
>> +EXPORT_SYMBOL_GPL(intel_cht_wc_get_model);
> 
> Are you planning to use EXPORT_SYMBOL_GPL_NS()? If not, please consider it.

This will be gone in v3, since this info is now stored in struct intel_soc_pmic
as requested by Chanwoo.

Regards,

Hans

Re: [PATCH v2 15/20] i2c: cht-wc: Make charger i2c-client instantiation board/device-model specific

[Hans de Goede]

Hi,

On 11/23/21 09:24, Wolfram Sang wrote:
> On Sun, Nov 14, 2021 at 06:03:30PM +0100, Hans de Goede wrote:
>> The i2c-controller on the Cherry Trail - Whiskey Cove PMIC is special
>> in that it is always connected to the I2C charger IC of the board on
>> which the PMIC is used; and the charger IC is not described in ACPI,
>> so the i2c-cht-wc code needs to instantiate an i2c-client for it itself.
>>
>> So far this was hardcoded to instantiate an i2c-client for the
>> bq24292i, with all properties, etc. set to match how this charger
>> is used on the GPD win and GPD pcoket devices.
>>
>> There is a rudimentary check to make sure the ACPI tables are at least
>> somewhat as expected, but this is far from accurate, leading to
>> a wrong i2c-client being instantiated for the charger on some boards.
>>
>> Switch to the new DMI based intel_cht_wc_get_model() helper which is
>> exported by the MFD driver for the CHT Whiskey Cove PMIC to help PMIC
>> cell drivers like the i2c-cht-wc code reliably detect which board
>> they are running on.
>>
>> And add board_info for the charger ICs as found on the other 2 known
>> boards with a Whisky Cove PMIC.
>>
>> This has been tested on all 3 known boards.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Here is my Ack to take this via some other tree:
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>

Great, thank you.

> No need to send further versions of this series to the i2c-list, I'd
> think.

Ok I will drop the i2c-list from the Cc for further versions of this series.

Regards,

Hans

Re: [PATCH v2 18/20] extcon: intel-cht-wc: Refactor cht_wc_extcon_get_charger()

[Hans de Goede]

Hi,

On 11/19/21 16:44, Chanwoo Choi wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 7:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/17/21 08:15, Chanwoo Choi wrote:
>>> Hello,
>>>
>>> I think that you need to squash it with patch21
>>> I'm not sure that this patch is either atomic or not because
>>> you remove the 'return EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_SDP'
>>> without explaining why it is no problem. Just mention that
>>> pass the role to next 'switch' cases. But, before this change,
>>> there were any reason to return the type of charger cable
>>> before switch statement.
>>
>> The setting of usbsrc to "CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT"
>> will make the following switch-case return EXTCON_CHG_USB_SDP
>> just as before, so there is no functional change.
>>
>>> According to your patch description, you don't need
>>> to make the separate patch of it. Please squash it with patch21.
>>
>> Having this refactoring in a separate patch makes it easier
>> to see what is going on in patch 21. So I'm going to keep this
>> as a separate patch for v3 of this series.
> 
> If you want to keep this  patch, please remove the following description.
> Instead, just mention to focus on refactor it without behavior changes.
> 
> 'This is a preparation patch for adding support for registering
> a power_supply class device.'

Ok, done for v3 of the patch-series.

Regards,

Hans



>>> On 21. 11. 15. 오전 2:03, Hans de Goede wrote:
>>>> Refactor cht_wc_extcon_get_charger() to have all the returns are in
>>>> the "switch (usbsrc)" cases.
>>>>
>>>> This is a preparation patch for adding support for registering
>>>> a power_supply class device.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   drivers/extcon/extcon-intel-cht-wc.c | 15 ++++++++-------
>>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>>>> index 119b83793123..f2b93a99a625 100644
>>>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>>>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>>>> @@ -153,14 +153,15 @@ static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext,
>>>>       } while (time_before(jiffies, timeout));
>>>>         if (status != CHT_WC_USBSRC_STS_SUCCESS) {
>>>> -        if (ignore_errors)
>>>> -            return EXTCON_CHG_USB_SDP; /* Save fallback */
>>>> +        if (!ignore_errors) {
>>>> +            if (status == CHT_WC_USBSRC_STS_FAIL)
>>>> +                dev_warn(ext->dev, "Could not detect charger type\n");
>>>> +            else
>>>> +                dev_warn(ext->dev, "Timeout detecting charger type\n");
>>>> +        }
>>>>   -        if (status == CHT_WC_USBSRC_STS_FAIL)
>>>> -            dev_warn(ext->dev, "Could not detect charger type\n");
>>>> -        else
>>>> -            dev_warn(ext->dev, "Timeout detecting charger type\n");
>>>> -        return EXTCON_CHG_USB_SDP; /* Save fallback */
>>>> +        /* Save fallback */
>>>> +        usbsrc = CHT_WC_USBSRC_TYPE_SDP << CHT_WC_USBSRC_TYPE_SHIFT;
>>>>       }
>>>>         usbsrc = (usbsrc & CHT_WC_USBSRC_TYPE_MASK) >> CHT_WC_USBSRC_TYPE_SHIFT;
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH v2 10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Hans de Goede]

Hi,

On 11/16/21 12:07, Yauhen Kharuzhy wrote:
> 
> 
> аў, 16 ліс 2021, 12:33 карыстальнік Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> напісаў:
> 
>     Hi Yauhen,
> 
>     On 11/15/21 11:11, Yauhen Kharuzhy wrote:
>     > On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
>     >> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>     >> patch for adding regulator support.
>     >>
>     >> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>     >> ---
>     >>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>     >>  1 file changed, 15 insertions(+), 13 deletions(-)
>     >>
>     >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>     >> index 2bdfb58cda75..3c41fe86b3d3 100644
>     >> --- a/drivers/power/supply/bq25890_charger.c
>     >> +++ b/drivers/power/supply/bq25890_charger.c
>     >> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>     >>      return PTR_ERR_OR_ZERO(bq->charger);
>     >>  }
>     >> 
>     >> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
>     >> +{
>     >> +    int ret;
>     >> +
>     >> +    ret = bq25890_field_write(bq, F_OTG_CFG, val);
>     >> +    if (ret < 0)
>     >> +            dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
>     >
>     > Just a note: if a connected USB device has relative big capacitor
>     > at power wires inside, then a starting current pulse may be enough to
>     > overload the boost reguator and VBUS will not be powered. I met this
>     > at Yoga Book: the firmware set boost current limit to 1.4 A (default
>     > value for bq25892) but when USB hub connected, the BOOST_FAULT event
>     > appeared.
>     >
>     > To avoid this, Lenovo uses following trick in its kernel: set a boost
>     > current limit to big value (2.1 A), wait some time (500 ms) and set
>     > the current limit to right value (1.4A). This provides enough current to
>     > charge capacitors in the connected device but saves desired long-time limit
>     > to prevent overloading if the device consumes too much power itself.
> 
>     Right I saw this in your git repo, but I cannot reproduce the issue (1)
>     I was hoping that since you can reproduce this, that you can rebase
>     your fix on top of my patch-set ?
> 
>     Also I'm wondering if this behavior should be the default, I believe
>     that the max. boost current may also be dependent on some external
>     factors, so maybe we should make this behavior conditional on a
>     new device-property ?
> 
> Yes, defining of max VBUS current may be a good idea. Another possible approach may be to use some empirical multiplier, like 150% of max 'long time' current limit setting. I almost sure that all hardware will work with short impulse of such current, its usual condition at device connection.
> 
> 
>     Regards,
> 
>     Hans
> 
> 
> 
>     1) I must admit I did not try really hard, I guess I could try an
>     USB powered hdd enclosure with a spinning disk
> 
>     What device are you seeing this with?
> 
> I cannot remember exactly device but this was a USB hub, possible with keyboard, mouse receiver and USB dongle inserted. I can recheck this issue but one week after, when will return home.

So as I mentioned before I've just tried to reproduce this problem, but
I cannot reproduce it with an 2.5" USB disk enclosure with a spinning
disk, which typically will cause a nice current-peak when spinning up.

I think this might also require an almost empty battery to reproduce ?

Regards,

Hans

Re: [PATCH v2 10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Yauhen Kharuzhy]

On Sun, Nov 28, 2021 at 04:02:06PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/16/21 12:07, Yauhen Kharuzhy wrote:
> > 
> > 
> > аў, 16 ліс 2021, 12:33 карыстальнік Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> напісаў:
> > 
> >     Hi Yauhen,
> > 
> >     On 11/15/21 11:11, Yauhen Kharuzhy wrote:
> >     > On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
> >     >> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> >     >> patch for adding regulator support.
> >     >>
> >     >> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
> >     >> ---
> >     >>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
> >     >>  1 file changed, 15 insertions(+), 13 deletions(-)
> >     >>
> >     >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> >     >> index 2bdfb58cda75..3c41fe86b3d3 100644
> >     >> --- a/drivers/power/supply/bq25890_charger.c
> >     >> +++ b/drivers/power/supply/bq25890_charger.c
> >     >> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
> >     >>      return PTR_ERR_OR_ZERO(bq->charger);
> >     >>  }
> >     >> 
> >     >> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
> >     >> +{
> >     >> +    int ret;
> >     >> +
> >     >> +    ret = bq25890_field_write(bq, F_OTG_CFG, val);
> >     >> +    if (ret < 0)
> >     >> +            dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
> >     >
> >     > Just a note: if a connected USB device has relative big capacitor
> >     > at power wires inside, then a starting current pulse may be enough to
> >     > overload the boost reguator and VBUS will not be powered. I met this
> >     > at Yoga Book: the firmware set boost current limit to 1.4 A (default
> >     > value for bq25892) but when USB hub connected, the BOOST_FAULT event
> >     > appeared.
> >     >
> >     > To avoid this, Lenovo uses following trick in its kernel: set a boost
> >     > current limit to big value (2.1 A), wait some time (500 ms) and set
> >     > the current limit to right value (1.4A). This provides enough current to
> >     > charge capacitors in the connected device but saves desired long-time limit
> >     > to prevent overloading if the device consumes too much power itself.
> > 
> >     Right I saw this in your git repo, but I cannot reproduce the issue (1)
> >     I was hoping that since you can reproduce this, that you can rebase
> >     your fix on top of my patch-set ?
> > 
> >     Also I'm wondering if this behavior should be the default, I believe
> >     that the max. boost current may also be dependent on some external
> >     factors, so maybe we should make this behavior conditional on a
> >     new device-property ?
> > 
> > Yes, defining of max VBUS current may be a good idea. Another possible approach may be to use some empirical multiplier, like 150% of max 'long time' current limit setting. I almost sure that all hardware will work with short impulse of such current, its usual condition at device connection.
> > 
> > 
> >     Regards,
> > 
> >     Hans
> > 
> > 
> > 
> >     1) I must admit I did not try really hard, I guess I could try an
> >     USB powered hdd enclosure with a spinning disk
> > 
> >     What device are you seeing this with?
> > 
> > I cannot remember exactly device but this was a USB hub, possible with keyboard, mouse receiver and USB dongle inserted. I can recheck this issue but one week after, when will return home.
> 
> So as I mentioned before I've just tried to reproduce this problem, but
> I cannot reproduce it with an 2.5" USB disk enclosure with a spinning
> disk, which typically will cause a nice current-peak when spinning up.
> 
> I think this might also require an almost empty battery to reproduce ?

Hi.

I tried to reproduce just now with success.

I have the UNITEK Y-2165B OTG USB hub with cardreader. It has three
100 uF capacitors connected to the VBUS inside.

If the boost current limit is set to 1.4A, boot failure condition is appeared
when the hub connected, there is no USB device detected.

If the limit is set to 2.15A, the device detected and works, there is no
fault condition appeared.

So, I think that you may add 1-3 100uF capacitors to
any USB device or cable and try to reproduce.

Debug I2C dumps are below.

Before connection of the hub:

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 0a 42 13 80 cc 03 44 73 02 00 8d 58 57    .?.?B????Ds?.?XW
10: 4e 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    N...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

Immediately after connection (look to 0x0c register):

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 2a 42 13 82 cc 03 44 73 e2 40 8d 57 57    .?.*B????Ds?@?WW
10: 4d 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    M...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

Increase current limit manually and restart the boost converter:

root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x0a 0x76
root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x03 0x0a
root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x03 0x2a

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 2a 42 13 82 cc 03 44 76 e2 00 8d 57 57    .?.*B????Dv?.?WW
10: 4d 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    M...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................



> 
> Regards,
> 
> Hans
> 

-- 
Yauhen Kharuzhy