[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC without USB-PD support

[PATCH 00/13] power-suppy/i2c/extcon: Add support for cht-wc PMIC without USB-PD support

[Hans de Goede]

On many X86 devices with a Cherry Trail SoC the PMIC / battery charger
support is not fully handled by ACPI. Instead we need to load native
drivers for the PMIC and various chips surrounding the PMIC like the
charger IC. So far we mostly support the AXP288 PMIC this way, as well
as the Intel Whiskey Cove PMIC with a FUSB302 Type-C controller + other
ICs for a USB-PD, USB-3 (superspeed) and DP-altmode capable Type-C
connector, as found on the GPD win and GPD pocket mini laptops.

On the Xiaomi Mi Pad 2 tablet the Whiskey Cove PMIC is used in
a different setup then on the GPD devices, supporting only USB-2 with
BC1.2 spec charger detection on its USB connector.

This series adds support for the charger IC in this setup and for having
the extcon-intel-cht-wc code control the 5V Vbus boost converter and
the USB role-switch, which is done by the FUSB302 Type-C controller driver
in the GPD case.

This fixes the tablet not charging under Linux, host-mode only working
when booted in host-mode, as well as device-mode not working. Note
device-mode still does not work when plugged into a CDP port. I have
identified the cause for this and I plan to submit a fix later.

This series consists of the following parts:

Patch 1 + 2: Add a "intel,cht-wc-setup" device property to the Whiskey Cove
i2c-client so that various WC drivers can use this to identify which setup
they are dealing with.

Patch 3 + 4: bq25890 psy driver bug-fixes

Patch 5 - 8: bq25890 psy support for non-devicetree devices

Patch 9 + 10: bq25890 psy support for registering the builtin Vbus boost
converter as a regulator

Patch 11: Add support to the i2c-cht-wc I2C-controller driver to
instantiate an i2c-client for the attached bq25890 charger

Patch 12 + 13: extcon-intel-cht-wc add support for the USB-2 only with
BC1.2 charger detection setup

Assuming everybody is ok with this series, we need to talk about how
to merge this.

Patch 1 makes some very small changes (just a rename) to
drivers/firmware/efi code, I would like to merging this + patch 2 through
the pdx86 tree (where the real changes are). Ard, can I have your ack
for this please ?

Patch 11 depends on a header file added by patch 10, since the
i2c-cht-wc.c code otherwise sees very little changes I believe it makes
sense for patch 11 to be merged into linux-power-supply.git/for-next
together with patches 3-10. Wolfram can we have you ack for this?

Patch 12 + 13 can be merged through the extcon tree, these have no
(compile time) dependencies on the other patches.

This is all 5.17 material, and I will make sure the pdx86 patches
adding the new property will land in 5.17-rc1, hopefully the rest
will land then too, but if other bits land later that is fine too,
as long as the new property is there behavior won't change on the
GPD win/pocket and we won't get any regressions.

Regards,

Hans

p.s.

Patch 3 and 4 are pure bq25890 bugfixes and should probably be picked up
right away independent of the rest of the series.


Hans de Goede (13):
  platform/x86: Rename touchscreen_dmi to dmi_device_properties
  platform/x86: dmi_device_properties: Add setup info for boards with a
    CHT Whiskey Cove PMIC
  power: supply: bq25890: Fix race causing oops at boot
  power: supply: bq25890: Fix initial setting of the F_CONV_RATE field
  power: supply: bq25890: Add a bq25890_rw_init_data() helper
  power: supply: bq25890: Add support for skipping initialization
  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
  i2c: cht-wc: Add support for devices using a bq25890 charger
  extcon: intel-cht-wc: Check new "intel,cht-wc-setup" device-property
  extcon: intel-cht-wc: Add support for devices with an USB-micro-B
    connector

 MAINTAINERS                                   |   2 +-
 drivers/extcon/extcon-intel-cht-wc.c          | 119 ++++++++--
 drivers/firmware/efi/embedded-firmware.c      |   4 +-
 drivers/i2c/busses/i2c-cht-wc.c               |  77 ++++--
 drivers/platform/x86/Kconfig                  |  20 +-
 drivers/platform/x86/Makefile                 |   2 +-
 ...chscreen_dmi.c => dmi_device_properties.c} |  54 ++++-
 drivers/power/supply/bq25890_charger.c        | 223 ++++++++++++------
 include/linux/efi_embedded_fw.h               |   2 +-
 include/linux/power/bq25890_charger.h         |  15 ++
 10 files changed, 400 insertions(+), 118 deletions(-)
 rename drivers/platform/x86/{touchscreen_dmi.c => dmi_device_properties.c} (96%)
 create mode 100644 include/linux/power/bq25890_charger.h

-- 
2.31.1

[PATCH 01/13] platform/x86: Rename touchscreen_dmi to dmi_device_properties

[Hans de Goede]

The ability to attach device-properties to (I2C) devices based on
a DMI + I2C device-name match to address the hw-description in
some ACPI tables being incomplete is useful for more things then just
touchscreens. Rename the Kconfig option and file to reflect this.

The specific use-case triggering this change is describing the
hardware surrounding the Whiskey Cove PMIC found on several
Cherry Trail based devices. At least 3 configs are known:

1. 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 WC PMIC is connected to a TI BQ25890 charger, paired with
   a TI BQ27520 fuelgauge, for a USB-2 only Type-C port without PD

3. The WC PMIC is connected to a TI BQ25890 charger, paired with
   a TI BQ27542 fuelgauge, for a micro-USB port

And various drivers (extcon-intel-cht-wc.c, i2c-cht-wc.c, ...) need
to know which config they are dealing with.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS                                   |  2 +-
 drivers/firmware/efi/embedded-firmware.c      |  4 ++--
 drivers/platform/x86/Kconfig                  | 20 ++++++++++---------
 drivers/platform/x86/Makefile                 |  2 +-
 ...chscreen_dmi.c => dmi_device_properties.c} |  8 ++++----
 include/linux/efi_embedded_fw.h               |  2 +-
 6 files changed, 20 insertions(+), 18 deletions(-)
 rename drivers/platform/x86/{touchscreen_dmi.c => dmi_device_properties.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 09abc1d84a7f..fe6a952c0232 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17156,7 +17156,7 @@ L:	linux-input@vger.kernel.org
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/input/touchscreen/silead.c
-F:	drivers/platform/x86/touchscreen_dmi.c
+F:	drivers/platform/x86/dmi_device_properties.c
 
 SILICON LABS WIRELESS DRIVERS (for WFxxx series)
 M:	Jérôme Pouiller <jerome.pouiller@silabs.com>
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index f5be8e22305b..f8212af0ba5e 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -21,8 +21,8 @@ bool efi_embedded_fw_checked;
 EXPORT_SYMBOL_NS_GPL(efi_embedded_fw_checked, TEST_FIRMWARE);
 
 static const struct dmi_system_id * const embedded_fw_table[] = {
-#ifdef CONFIG_TOUCHSCREEN_DMI
-	touchscreen_dmi_table,
+#ifdef CONFIG_DMI_DEVICE_PROPERTIES
+	dmi_device_properties,
 #endif
 	NULL
 };
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b110a2e6b8f3..9cb8d33cc6e1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -981,17 +981,19 @@ config MLX_PLATFORM
 
 	  If you have a Mellanox system, say Y or M here.
 
-config TOUCHSCREEN_DMI
-	bool "DMI based touchscreen configuration info"
-	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
+config DMI_DEVICE_PROPERTIES
+	bool "DMI based extra device properties"
+	depends on ACPI && DMI && I2C=y
 	select EFI_EMBEDDED_FIRMWARE if EFI
 	help
-	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
-	  do not have enough data in ACPI tables for the touchscreen driver to
-	  handle the touchscreen properly, as OEMs expect the data to be baked
-	  into the tablet model specific version of the driver shipped with the
-	  the OS-image for the device. This option supplies the missing info.
-	  Enable this for x86 tablets with Silead or Chipone touchscreens.
+	  Sometimes ACPI based x86 devices do not have enough data in their ACPI
+	  tables to fully describe the hardware. This option enables support for
+	  supplying the missing info based on DMI (vendor & model string)
+	  matching for devices where this info has been added to the
+	  dmi-device-properties code.
+
+	  This option is often necessary for correct operation of x86 based
+	  tablets and 2-in-1 devices. If in doubt, say Y here.
 
 config FW_ATTR_CLASS
 	tristate
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 219478061683..3f610332b556 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -107,10 +107,10 @@ obj-$(CONFIG_SYSTEM76_ACPI)	+= system76_acpi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 
 # Platform drivers
+obj-$(CONFIG_DMI_DEVICE_PROPERTIES)	+= dmi_device_properties.o
 obj-$(CONFIG_FW_ATTR_CLASS)		+= firmware_attributes_class.o
 obj-$(CONFIG_I2C_MULTI_INSTANTIATE)	+= i2c-multi-instantiate.o
 obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
-obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
 obj-$(CONFIG_WIRELESS_HOTKEY)		+= wireless-hotkey.o
 
 # Intel uncore drivers
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/dmi_device_properties.c
similarity index 99%
rename from drivers/platform/x86/touchscreen_dmi.c
rename to drivers/platform/x86/dmi_device_properties.c
index b5d007875423..1bcd14a0bc78 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/dmi_device_properties.c
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Touchscreen driver DMI based configuration code
+ * DMI based device-property addition (adding info missing from ACPI tables)
  *
- * Copyright (c) 2017 Red Hat Inc.
+ * Copyright (c) 2017-2021 Red Hat Inc.
  *
  * Red Hat authors:
  * Hans de Goede <hdegoede@redhat.com>
@@ -979,7 +979,7 @@ static const struct ts_dmi_data vinga_twizzle_j116_data = {
 };
 
 /* NOTE: Please keep this table sorted alphabetically */
-const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id dmi_device_properties[] = {
 	{
 		/* Chuwi Hi8 */
 		.driver_data = (void *)&chuwi_hi8_data,
@@ -1633,7 +1633,7 @@ static int __init ts_dmi_init(void)
 	const struct dmi_system_id *dmi_id;
 	int error;
 
-	dmi_id = dmi_first_match(touchscreen_dmi_table);
+	dmi_id = dmi_first_match(dmi_device_properties);
 	if (!dmi_id)
 		return 0; /* Not an error */
 
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index a97a12bb2c9e..01105c38a309 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -34,7 +34,7 @@ struct efi_embedded_fw_desc {
 	u8 sha256[32];
 };
 
-extern const struct dmi_system_id touchscreen_dmi_table[];
+extern const struct dmi_system_id dmi_device_properties[];
 
 int efi_get_embedded_fw(const char *name, const u8 **dat, size_t *sz);
 
-- 
2.31.1

[PATCH 02/13] platform/x86: dmi_device_properties: Add setup info for boards with a CHT Whiskey Cove PMIC

[Hans de Goede]

Add a new "intel,cht-wc-setup" string property to the "INT34D3:00"
i2c_client for the Whiskey Cove PMIC found on several Cherry Trail
based devices. At least 3 setups are known:

1. 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 WC PMIC is connected to a TI BQ25890 charger, paired with
   a TI BQ27520 fuelgauge, for a USB-2 only Type-C port without PD

3. The WC PMIC is connected to a TI BQ25890 charger, paired with
   a TI BQ27542 fuelgauge, for a micro-USB port

Which setup is in use cannot be determined reliably from the ACPI tables
and various drivers (extcon-intel-cht-wc.c, i2c-cht-wc.c, ...) need
to know which setup they are dealing with.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dmi_device_properties.c | 46 ++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/platform/x86/dmi_device_properties.c b/drivers/platform/x86/dmi_device_properties.c
index 1bcd14a0bc78..bd973afbde5f 100644
--- a/drivers/platform/x86/dmi_device_properties.c
+++ b/drivers/platform/x86/dmi_device_properties.c
@@ -387,6 +387,16 @@ static const struct ts_dmi_data gp_electronic_t701_data = {
 	.properties	= gp_electronic_t701_props,
 };
 
+static const struct property_entry gpd_win_pocket_props[] = {
+	PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq24292i,max17047,fusb302,pi3usb30532"),
+	{ }
+};
+
+static const struct ts_dmi_data gpd_win_pocket_data = {
+	.acpi_name      = "INT34D3:00",
+	.properties     = gpd_win_pocket_props,
+};
+
 static const struct property_entry irbis_tw90_props[] = {
 	PROPERTY_ENTRY_U32("touchscreen-size-x", 1720),
 	PROPERTY_ENTRY_U32("touchscreen-size-y", 1138),
@@ -978,6 +988,16 @@ static const struct ts_dmi_data vinga_twizzle_j116_data = {
 	.properties	= vinga_twizzle_j116_props,
 };
 
+static const struct property_entry xiaomi_mi_pad2_props[] = {
+	PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq25890,bq27520"),
+	{ }
+};
+
+static const struct ts_dmi_data xiaomi_mi_pad2_data = {
+	.acpi_name      = "INT34D3:00",
+	.properties     = xiaomi_mi_pad2_props,
+};
+
 /* NOTE: Please keep this table sorted alphabetically */
 const struct dmi_system_id dmi_device_properties[] = {
 	{
@@ -1166,6 +1186,24 @@ const struct dmi_system_id dmi_device_properties[] = {
 			DMI_MATCH(DMI_BIOS_VERSION, "itWORKS.G.WI71C.JGBMRB"),
 		},
 	},
+	{
+		/* GPD win / GPD pocket mini laptops */
+		.driver_data = (void *)&gpd_win_pocket_data,
+		/*
+		 * 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 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"),
+		},
+	},
 	{
 		/* Irbis TW90 */
 		.driver_data = (void *)&irbis_tw90_data,
@@ -1578,6 +1616,14 @@ const struct dmi_system_id dmi_device_properties[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "TW700")
 		},
 	},
+	{
+		/* Xiaomi Mi Pad 2 */
+		.driver_data = (void *)&xiaomi_mi_pad2_data,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Xiaomi Inc"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Mipad2"),
+		},
+	},
 	{
 		/* Yours Y8W81, same case and touchscreen as Chuwi Vi8 */
 		.driver_data = (void *)&chuwi_vi8_data,
-- 
2.31.1

[PATCH 03/13] power: supply: bq25890: Fix race causing oops at boot

[Hans de Goede]

Before this commit the driver was registering its interrupt handler before
it registered the power_supply, causing bq->charger to potentially be NULL
when the interrupt handler runs, triggering a NULL pointer exception in
the interrupt handler:

[   21.213531] BUG: kernel NULL pointer dereference, address: 0000000000000680
...
[   21.213573] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
[   21.213576] RIP: 0010:__lock_acquire+0x5c5/0x1de0
...
[   21.213629] Call Trace:
[   21.213636]  ? disable_irq_nosync+0x10/0x10
[   21.213644]  ? __mutex_unlock_slowpath+0x35/0x260
[   21.213655]  lock_acquire+0xb5/0x2b0
[   21.213661]  ? power_supply_changed+0x23/0x90
[   21.213670]  ? disable_irq_nosync+0x10/0x10
[   21.213676]  _raw_spin_lock_irqsave+0x48/0x60
[   21.213682]  ? power_supply_changed+0x23/0x90
[   21.213687]  power_supply_changed+0x23/0x90
[   21.213697]  __bq25890_handle_irq+0x5e/0xe0 [bq25890_charger]
[   21.213709]  bq25890_irq_handler_thread+0x26/0x40 [bq25890_charger]
[   21.213718]  irq_thread_fn+0x20/0x60
...

Fix this by moving the power_supply_register() call to above the
request_threaded_irq() call.

Note this fix includes making the following 2 (necessary) changes:

1. Switch to the devm version of power_supply_register() to avoid the
need to make the error-handling in probe() more complicated.

2. Rename the "irq_fail" label to "err_unregister_usb_notifier" since
the old name no longer makes sense after this fix.

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

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 945c3257ca93..491d36a3811a 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -734,8 +734,9 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
 	psy_cfg.supplied_to = bq25890_charger_supplied_to;
 	psy_cfg.num_supplicants = ARRAY_SIZE(bq25890_charger_supplied_to);
 
-	bq->charger = power_supply_register(bq->dev, &bq25890_power_supply_desc,
-					    &psy_cfg);
+	bq->charger = devm_power_supply_register(bq->dev,
+						 &bq25890_power_supply_desc,
+						 &psy_cfg);
 
 	return PTR_ERR_OR_ZERO(bq->charger);
 }
@@ -985,22 +986,22 @@ static int bq25890_probe(struct i2c_client *client,
 		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
 	}
 
+	ret = bq25890_power_supply_init(bq);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register power supply\n");
+		goto err_unregister_usb_notifier;
+	}
+
 	ret = devm_request_threaded_irq(dev, client->irq, NULL,
 					bq25890_irq_handler_thread,
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					BQ25890_IRQ_PIN, bq);
 	if (ret)
-		goto irq_fail;
-
-	ret = bq25890_power_supply_init(bq);
-	if (ret < 0) {
-		dev_err(dev, "Failed to register power supply\n");
-		goto irq_fail;
-	}
+		goto err_unregister_usb_notifier;
 
 	return 0;
 
-irq_fail:
+err_unregister_usb_notifier:
 	if (!IS_ERR_OR_NULL(bq->usb_phy))
 		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
 
@@ -1011,8 +1012,6 @@ static int bq25890_remove(struct i2c_client *client)
 {
 	struct bq25890_device *bq = i2c_get_clientdata(client);
 
-	power_supply_unregister(bq->charger);
-
 	if (!IS_ERR_OR_NULL(bq->usb_phy))
 		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
 
-- 
2.31.1

[PATCH 04/13] power: supply: bq25890: Fix initial setting of the F_CONV_RATE field

[Hans de Goede]

The code doing the initial setting of the F_CONV_RATE field based
on the bq->state.online flag. In order for this to work properly,
this must be done after the initial bq25890_get_chip_state() call.

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

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 491d36a3811a..99497fdc73da 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -682,16 +682,16 @@ static int bq25890_hw_init(struct bq25890_device *bq)
 		}
 	}
 
-	/* Configure ADC for continuous conversions when charging */
-	ret = bq25890_field_write(bq, F_CONV_RATE, !!bq->state.online);
+	ret = bq25890_get_chip_state(bq, &bq->state);
 	if (ret < 0) {
-		dev_dbg(bq->dev, "Config ADC failed %d\n", ret);
+		dev_dbg(bq->dev, "Get state failed %d\n", ret);
 		return ret;
 	}
 
-	ret = bq25890_get_chip_state(bq, &bq->state);
+	/* Configure ADC for continuous conversions when charging */
+	ret = bq25890_field_write(bq, F_CONV_RATE, !!bq->state.online);
 	if (ret < 0) {
-		dev_dbg(bq->dev, "Get state failed %d\n", ret);
+		dev_dbg(bq->dev, "Config ADC failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.31.1

[PATCH 05/13] 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 99497fdc73da..1ec93a565631 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -636,29 +636,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);
@@ -673,14 +696,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 06/13] power: supply: bq25890: Add support for skipping initialization

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

At support for a new "ti,skip-init" 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>
---
 drivers/power/supply/bq25890_charger.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 1ec93a565631..280821a6a6c6 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -73,6 +73,7 @@ enum bq25890_fields {
 
 /* initial field values, converted to register values */
 struct bq25890_init_data {
+	u8 write_cfg;	/* write firmware cfg to chip?	*/
 	u8 ichg;	/* charge current		*/
 	u8 vreg;	/* regulation voltage		*/
 	u8 iterm;	/* termination current		*/
@@ -638,7 +639,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->init_data.write_cfg;
 	int ret;
 	int i;
 
@@ -682,10 +683,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->init_data.write_cfg) {
+		ret = bq25890_chip_reset(bq);
+		if (ret < 0) {
+			dev_dbg(bq->dev, "Reset failed %d\n", ret);
+			return ret;
+		}
 	}
 
 	/* disable watchdog */
@@ -920,6 +923,10 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	int ret;
 	struct bq25890_init_data *init = &bq->init_data;
 
+	init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init");
+	if (!init->write_cfg)
+		return 0;
+
 	ret = bq25890_fw_read_u32_props(bq);
 	if (ret < 0)
 		return ret;
@@ -1033,8 +1040,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->init_data.write_cfg) {
+		/* reset all registers to default values */
+		bq25890_chip_reset(bq);
+	}
 
 	return 0;
 }
-- 
2.31.1

[PATCH 07/13] power: supply: bq25890: Enable charging on boards where we skip reset

[Hans de Goede]

On boards where the "ti,skip-init" 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 280821a6a6c6..aa4d04d20cdc 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -689,6 +689,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 08/13] 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 "ti,skip-init" property the driver can actually supports
devices without devicetree and this check no longer makes sense.

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

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index aa4d04d20cdc..163ca5d761aa 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -990,14 +990,10 @@ 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.\n");
-			return ret;
-		}
-	} else {
-		return -ENODEV;
+	ret = bq25890_fw_probe(bq);
+	if (ret < 0) {
+		dev_err(dev, "Cannot read device properties.\n");
+		return ret;
 	}
 
 	ret = bq25890_hw_init(bq);
-- 
2.31.1

[PATCH 09/13] 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 163ca5d761aa..7504e30f1e4d 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -773,6 +773,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;
@@ -782,25 +793,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 10/13] 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>
---
 drivers/power/supply/bq25890_charger.c | 56 ++++++++++++++++++++++++++
 include/linux/power/bq25890_charger.h  | 15 +++++++
 2 files changed, 71 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 7504e30f1e4d..eaadb7a82c67 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>
@@ -817,6 +819,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;
@@ -1018,6 +1059,21 @@ static int bq25890_probe(struct i2c_client *client,
 		INIT_WORK(&bq->usb_work, bq25890_usb_work);
 		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);
diff --git a/include/linux/power/bq25890_charger.h b/include/linux/power/bq25890_charger.h
new file mode 100644
index 000000000000..8a36c0d787c7
--- /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_
+
+#include <linux/regulator/machine.h>
+
+struct bq25890_platform_data {
+	const struct regulator_init_data *regulator_init_data;
+};
+
+#endif
-- 
2.31.1

[PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[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 there has been a rudimentary check to make sure the ACPI tables
are at least somewhat as expected by checking for the presence of an
INT33FE device and sofar the code has assumed that if this INT33FE
device is present that the used charger then is a bq24290i.

But some boards with an INT33FE device in their ACPI tables use a
different charger IC and some boards don't have an INT33FE device at all.

Since the information about the used charger + fuel-gauge + other chips is
necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
which reliably describes the board's setup of the PMIC.

Switch to using the "intel,cht-wc-setup" property and add support for
instantiating an i2c-client for either a bq24292i or a bq25890 charger.

This has been tested on a GPD pocket (which uses the old bq24292i setup)
and on a Xiaomi Mi Pad 2 with a bq25890 charger.

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

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index 1cf68f85b2e1..e7d62af6c39d 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
@@ -304,18 +305,55 @@ static struct bq24190_platform_data bq24190_pdata = {
 	.regulator_init_data = &bq24190_vbus_init_data,
 };
 
+static struct i2c_board_info bq24190_board_info = {
+	.type = "bq24190",
+	.addr = 0x6b,
+	.dev_name = "bq24190",
+	.swnode = &bq24190_node,
+	.platform_data = &bq24190_pdata,
+};
+
+static struct regulator_consumer_supply bq25890_vbus_consumer = {
+	.supply = "vbus",
+	.dev_name = "cht_wcove_pwrsrc",
+};
+
+static const struct regulator_init_data bq25890_vbus_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.consumer_supplies = &bq25890_vbus_consumer,
+	.num_consumer_supplies = 1,
+};
+
+static struct bq25890_platform_data bq25890_pdata = {
+	.regulator_init_data = &bq25890_vbus_init_data,
+};
+
+static const struct property_entry bq25890_props[] = {
+	PROPERTY_ENTRY_BOOL("ti,skip-init"),
+	{ }
+};
+
+static const struct software_node bq25890_node = {
+	.properties = bq25890_props,
+};
+
+static struct i2c_board_info bq25890_board_info = {
+	.type = "bq25890",
+	.addr = 0x6a,
+	.dev_name = "bq25890",
+	.swnode = &bq25890_node,
+	.platform_data = &bq25890_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;
+	const char *str;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -379,17 +417,20 @@ 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);
+	ret = device_property_read_string(pdev->dev.parent, "intel,cht-wc-setup", &str);
+	if (ret)
+		dev_warn(&pdev->dev, "intel,cht-wc-setup not set, not instantiating charger device\n");
+	else if (!strcmp(str, "bq24292i,max17047,fusb302,pi3usb30532"))
+		board_info = &bq24190_board_info;
+	else if (!strcmp(str, "bq25890,bq27520"))
+		board_info = &bq25890_board_info;
+	else
+		dev_warn(&pdev->dev, "Unknown intel,cht-wc-setup value: '%s', not instantiating charger device\n",
+			 str);
+
+	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 12/13] extcon: intel-cht-wc: Check new "intel,cht-wc-setup" device-property

[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 kernel now adds a "intel,cht-wc-setup" string property
to the Whiskey Cove PMIC i2c-client based on DMI matching.

Only poke the CHT_WC_VBUS_GPIO_CTLO GPIO if this new property has the
"bq24292i,max17047,fusb302,pi3usb30532" value 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 | 38 ++++++++++++++++------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 771f6f4cf92e..a7a6b43d699b 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>
 
@@ -338,8 +339,8 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
 	struct cht_wc_extcon_data *ext;
 	unsigned long mask = ~(CHT_WC_PWRSRC_VBUS | CHT_WC_PWRSRC_USBID_MASK);
-	int pwrsrc_sts, id;
-	int irq, ret;
+	int id, irq, pwrsrc_sts, ret;
+	const char *setup;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -358,20 +359,25 @@ 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);
+	ret = device_property_read_string(ext->dev->parent, "intel,cht-wc-setup", &setup);
+	if (ret) {
+		dev_warn(ext->dev, "intel,cht-wc-setup not set\n");
+	} else if (!strcmp(setup, "bq24292i,max17047,fusb302,pi3usb30532")) {
+		/*
+		 * 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);
+	}
 
 	/* Enable sw control */
 	ret = cht_wc_extcon_sw_control(ext, true);
-- 
2.31.1

[PATCH 13/13] extcon: intel-cht-wc: Add support for devices with an USB-micro-B connector

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

These device are identified by "intel,cht-wc-setup" = "bq25890,bq27520",
since there is no Type-C controller on these devices 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/extcon-intel-cht-wc.c | 81 ++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index a7a6b43d699b..d8d0a69f4ddd 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,29 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
 	return ret;
 }
 
+static int cht_wc_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_put_role_sw(void *data)
+{
+	struct cht_wc_extcon_data *ext = data;
+
+	usb_role_switch_put(ext->role_sw);
+}
+
 static int cht_wc_extcon_probe(struct platform_device *pdev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
@@ -377,6 +430,34 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 		 * external 5v boost converter off and leave it off entirely.
 		 */
 		cht_wc_extcon_set_5v_boost(ext, false);
+	} else if (!strcmp(setup, "bq25890,bq27520")) {
+		/*
+		 * Classic micro USB-B setup, this requires controling
+		 * the role-sw and vbus based on the id-pin.
+		 */
+		ret = cht_wc_find_role_sw(ext);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(ext->dev, cht_wc_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");
+		}
 	}
 
 	/* Enable sw control */
-- 
2.31.1

Re: [PATCH 01/13] platform/x86: Rename touchscreen_dmi to dmi_device_properties

[Randy Dunlap]

Hi--

On 10/30/21 11:28 AM, Hans de Goede wrote:
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index b110a2e6b8f3..9cb8d33cc6e1 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -981,17 +981,19 @@ config MLX_PLATFORM
>   
>   	  If you have a Mellanox system, say Y or M here.
>   
> -config TOUCHSCREEN_DMI
> -	bool "DMI based touchscreen configuration info"
> -	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
> +config DMI_DEVICE_PROPERTIES
> +	bool "DMI based extra device properties"

	      DMI-based

> +	depends on ACPI && DMI && I2C=y
>   	select EFI_EMBEDDED_FIRMWARE if EFI
>   	help
> -	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
> -	  do not have enough data in ACPI tables for the touchscreen driver to
> -	  handle the touchscreen properly, as OEMs expect the data to be baked
> -	  into the tablet model specific version of the driver shipped with the
> -	  the OS-image for the device. This option supplies the missing info.
> -	  Enable this for x86 tablets with Silead or Chipone touchscreens.
> +	  Sometimes ACPI based x86 devices do not have enough data in their ACPI

	            ACPI-based

> +	  tables to fully describe the hardware. This option enables support for
> +	  supplying the missing info based on DMI (vendor & model string)
> +	  matching for devices where this info has been added to the
> +	  dmi-device-properties code.
> +
> +	  This option is often necessary for correct operation of x86 based

	                                                          x86-based

> +	  tablets and 2-in-1 devices. If in doubt, say Y here.


-- 
~Randy

Re: [PATCH 02/13] platform/x86: dmi_device_properties: Add setup info for boards with a CHT Whiskey Cove PMIC

[Andy Shevchenko]

On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a new "intel,cht-wc-setup" string property to the "INT34D3:00"
> i2c_client for the Whiskey Cove PMIC found on several Cherry Trail
> based devices. At least 3 setups are known:
>
> 1. 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 WC PMIC is connected to a TI BQ25890 charger, paired with
>    a TI BQ27520 fuelgauge, for a USB-2 only Type-C port without PD
>
> 3. The WC PMIC is connected to a TI BQ25890 charger, paired with
>    a TI BQ27542 fuelgauge, for a micro-USB port
>
> Which setup is in use cannot be determined reliably from the ACPI tables
> and various drivers (extcon-intel-cht-wc.c, i2c-cht-wc.c, ...) need
> to know which setup they are dealing with.

If it's internal property only, I would rather expect it to start with
'linux,' as DWC3 does. And it's also USB related.

...

> +       PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq24292i,max17047,fusb302,pi3usb30532"),

> +       PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq25890,bq27520"),

 Besides that I'm not sure about the name of the property, maybe
'linux,cht-wc-usb-chips' or alike? And since it's a list, can we make
it a string array?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/13] power: supply: bq25890: Add support for skipping initialization

[Andy Shevchenko]

On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.
>
> At support for a new "ti,skip-init" 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.

With 'ti,' prefix it can be a potential collision in name space, for
internal properties I would rather use 'linux,' one.

...

> +       init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init");
> +       if (!init->write_cfg)
> +               return 0;

Why to have double negation here?
I would rather expect that you will have direct value in the structure
and do a respective check in the functions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/13] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Andy Shevchenko]

On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> patch for adding regulator support.

...

>         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;
>         }

While at it,

default:
 break;

?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Oct 30, 2021 at 9:28 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.

...

> @@ -1018,6 +1059,21 @@ static int bq25890_probe(struct i2c_client *client,
>                 INIT_WORK(&bq->usb_work, bq25890_usb_work);
>                 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
>         }

}
#ifdef
else {
  ...
}
#endif

is a bit better to maintain (less error prone in case of new code).

...

> +#ifndef _BQ25890_CHARGER_H_
> +#define _BQ25890_CHARGER_H_

> +#include <linux/regulator/machine.h>

struct regulator_init_data;

should be sufficient, no header is needed.

> +struct bq25890_platform_data {
> +       const struct regulator_init_data *regulator_init_data;
> +};
> +
> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/13] extcon: intel-cht-wc: Add support for devices with an USB-micro-B connector

[Andy Shevchenko]

On Sat, Oct 30, 2021 at 9:29 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.
>
> These device are identified by "intel,cht-wc-setup" = "bq25890,bq27520",
> since there is no Type-C controller on these devices the extcon code must
> control the Vbus 5V boost converter and the USB role switch depending on
> the detected cable-type.

...

> +       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);

if (ret)
 dev_err()
else
 ...

?

> +       }

...


> +               /*
> +                * Classic micro USB-B setup, this requires controling

controlling

> +                * the role-sw and vbus based on the id-pin.
> +                */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[Wolfram Sang]

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

On Sat, Oct 30, 2021 at 08:28:11PM +0200, 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 there has been a rudimentary check to make sure the ACPI tables
> are at least somewhat as expected by checking for the presence of an
> INT33FE device and sofar the code has assumed that if this INT33FE
> device is present that the used charger then is a bq24290i.
> 
> But some boards with an INT33FE device in their ACPI tables use a
> different charger IC and some boards don't have an INT33FE device at all.
> 
> Since the information about the used charger + fuel-gauge + other chips is
> necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
> string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
> which reliably describes the board's setup of the PMIC.
> 
> Switch to using the "intel,cht-wc-setup" property and add support for
> instantiating an i2c-client for either a bq24292i or a bq25890 charger.
> 
> This has been tested on a GPD pocket (which uses the old bq24292i setup)
> and on a Xiaomi Mi Pad 2 with a bq25890 charger.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

In general, fine with me from the I2C side:

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


> +	else if (!strcmp(str, "bq24292i,max17047,fusb302,pi3usb30532"))
> +		board_info = &bq24190_board_info;
> +	else if (!strcmp(str, "bq25890,bq27520"))
> +		board_info = &bq25890_board_info;

Very minor nit: I prefer 'strcmp() == 0' because the above could be read
as 'if not strcmp()' which is sadly misleading. But I am not strict with
it.


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

Re: [PATCH 03/13] power: supply: bq25890: Fix race causing oops at boot

[Sebastian Reichel]

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

Hi,

On Sat, Oct 30, 2021 at 08:28:03PM +0200, Hans de Goede wrote:
> Before this commit the driver was registering its interrupt handler before
> it registered the power_supply, causing bq->charger to potentially be NULL
> when the interrupt handler runs, triggering a NULL pointer exception in
> the interrupt handler:
> 
> [   21.213531] BUG: kernel NULL pointer dereference, address: 0000000000000680
> ...
> [   21.213573] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
> [   21.213576] RIP: 0010:__lock_acquire+0x5c5/0x1de0
> ...
> [   21.213629] Call Trace:
> [   21.213636]  ? disable_irq_nosync+0x10/0x10
> [   21.213644]  ? __mutex_unlock_slowpath+0x35/0x260
> [   21.213655]  lock_acquire+0xb5/0x2b0
> [   21.213661]  ? power_supply_changed+0x23/0x90
> [   21.213670]  ? disable_irq_nosync+0x10/0x10
> [   21.213676]  _raw_spin_lock_irqsave+0x48/0x60
> [   21.213682]  ? power_supply_changed+0x23/0x90
> [   21.213687]  power_supply_changed+0x23/0x90
> [   21.213697]  __bq25890_handle_irq+0x5e/0xe0 [bq25890_charger]
> [   21.213709]  bq25890_irq_handler_thread+0x26/0x40 [bq25890_charger]
> [   21.213718]  irq_thread_fn+0x20/0x60
> ...
> 
> Fix this by moving the power_supply_register() call to above the
> request_threaded_irq() call.
> 
> Note this fix includes making the following 2 (necessary) changes:
> 
> 1. Switch to the devm version of power_supply_register() to avoid the
> need to make the error-handling in probe() more complicated.
> 
> 2. Rename the "irq_fail" label to "err_unregister_usb_notifier" since
> the old name no longer makes sense after this fix.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/bq25890_charger.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 945c3257ca93..491d36a3811a 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -734,8 +734,9 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>  	psy_cfg.supplied_to = bq25890_charger_supplied_to;
>  	psy_cfg.num_supplicants = ARRAY_SIZE(bq25890_charger_supplied_to);
>  
> -	bq->charger = power_supply_register(bq->dev, &bq25890_power_supply_desc,
> -					    &psy_cfg);
> +	bq->charger = devm_power_supply_register(bq->dev,
> +						 &bq25890_power_supply_desc,
> +						 &psy_cfg);
>  
>  	return PTR_ERR_OR_ZERO(bq->charger);
>  }
> @@ -985,22 +986,22 @@ static int bq25890_probe(struct i2c_client *client,
>  		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
>  	}
>  
> +	ret = bq25890_power_supply_init(bq);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register power supply\n");
> +		goto err_unregister_usb_notifier;
> +	}
> +
>  	ret = devm_request_threaded_irq(dev, client->irq, NULL,
>  					bq25890_irq_handler_thread,
>  					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  					BQ25890_IRQ_PIN, bq);
>  	if (ret)
> -		goto irq_fail;
> -
> -	ret = bq25890_power_supply_init(bq);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to register power supply\n");
> -		goto irq_fail;
> -	}
> +		goto err_unregister_usb_notifier;
>  
>  	return 0;
>  
> -irq_fail:
> +err_unregister_usb_notifier:
>  	if (!IS_ERR_OR_NULL(bq->usb_phy))
>  		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
>  
> @@ -1011,8 +1012,6 @@ static int bq25890_remove(struct i2c_client *client)
>  {
>  	struct bq25890_device *bq = i2c_get_clientdata(client);
>  
> -	power_supply_unregister(bq->charger);
> -
>  	if (!IS_ERR_OR_NULL(bq->usb_phy))
>  		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
>  
> -- 
> 2.31.1
> 

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

Re: [PATCH 04/13] power: supply: bq25890: Fix initial setting of the F_CONV_RATE field

[Sebastian Reichel]

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

Hi,

On Sat, Oct 30, 2021 at 08:28:04PM +0200, Hans de Goede wrote:
> The code doing the initial setting of the F_CONV_RATE field based
> on the bq->state.online flag. In order for this to work properly,
> this must be done after the initial bq25890_get_chip_state() call.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/bq25890_charger.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 491d36a3811a..99497fdc73da 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -682,16 +682,16 @@ static int bq25890_hw_init(struct bq25890_device *bq)
>  		}
>  	}
>  
> -	/* Configure ADC for continuous conversions when charging */
> -	ret = bq25890_field_write(bq, F_CONV_RATE, !!bq->state.online);
> +	ret = bq25890_get_chip_state(bq, &bq->state);
>  	if (ret < 0) {
> -		dev_dbg(bq->dev, "Config ADC failed %d\n", ret);
> +		dev_dbg(bq->dev, "Get state failed %d\n", ret);
>  		return ret;
>  	}
>  
> -	ret = bq25890_get_chip_state(bq, &bq->state);
> +	/* Configure ADC for continuous conversions when charging */
> +	ret = bq25890_field_write(bq, F_CONV_RATE, !!bq->state.online);
>  	if (ret < 0) {
> -		dev_dbg(bq->dev, "Get state failed %d\n", ret);
> +		dev_dbg(bq->dev, "Config ADC failed %d\n", ret);
>  		return ret;
>  	}
>  
> -- 
> 2.31.1
> 

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

Re: [PATCH 07/13] power: supply: bq25890: Enable charging on boards where we skip reset

[Yauhen Kharuzhy]

On Sat, Oct 30, 2021 at 08:28:07PM +0200, Hans de Goede wrote:
> On boards where the "ti,skip-init" 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.

We should check if an OTG host mode with powering of bus by bq25890 is
not enabled before enable charging. If we boot with OTG device
connected, this can cause troubles.

> 
> 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 280821a6a6c6..aa4d04d20cdc 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -689,6 +689,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
> 

-- 
Yauhen Kharuzhy

Re: [PATCH 09/13] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Yauhen Kharuzhy]

On Sat, Oct 30, 2021 at 08:28:09PM +0200, Hans de Goede wrote:
> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> patch for adding regulator support.

The same notice here: if we enabling the boost mode to supply USB OTG
device, we should disable charging and vice versa. I don't remember if
enabling of OTG with CHG enabled really caused issues but I avoided this
in my Yoga Book work.

I made quick check — seems that charging can be enabled during of boost
operation, there are no visible side effects in registers and no
limitation in the datasheet, so your approach may be good.

> 
> 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 163ca5d761aa..7504e30f1e4d 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -773,6 +773,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;
> @@ -782,25 +793,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
> 

-- 
Yauhen Kharuzhy

Re: [PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[Yauhen Kharuzhy]

On Sat, Oct 30, 2021 at 08:28:11PM +0200, 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 there has been a rudimentary check to make sure the ACPI tables
> are at least somewhat as expected by checking for the presence of an
> INT33FE device and sofar the code has assumed that if this INT33FE
> device is present that the used charger then is a bq24290i.
> 
> But some boards with an INT33FE device in their ACPI tables use a
> different charger IC and some boards don't have an INT33FE device at all.
> 
> Since the information about the used charger + fuel-gauge + other chips is
> necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
> string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
> which reliably describes the board's setup of the PMIC.
> 
> Switch to using the "intel,cht-wc-setup" property and add support for
> instantiating an i2c-client for either a bq24292i or a bq25890 charger.
> 
> This has been tested on a GPD pocket (which uses the old bq24292i setup)
> and on a Xiaomi Mi Pad 2 with a bq25890 charger.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/busses/i2c-cht-wc.c | 77 +++++++++++++++++++++++++--------
>  1 file changed, 59 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
> index 1cf68f85b2e1..e7d62af6c39d 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
> @@ -304,18 +305,55 @@ static struct bq24190_platform_data bq24190_pdata = {
>  	.regulator_init_data = &bq24190_vbus_init_data,
>  };
>  
> +static struct i2c_board_info bq24190_board_info = {
> +	.type = "bq24190",
> +	.addr = 0x6b,
> +	.dev_name = "bq24190",
> +	.swnode = &bq24190_node,
> +	.platform_data = &bq24190_pdata,
> +};
> +
> +static struct regulator_consumer_supply bq25890_vbus_consumer = {
> +	.supply = "vbus",
> +	.dev_name = "cht_wcove_pwrsrc",
> +};
> +
> +static const struct regulator_init_data bq25890_vbus_init_data = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +	.consumer_supplies = &bq25890_vbus_consumer,
> +	.num_consumer_supplies = 1,
> +};
> +
> +static struct bq25890_platform_data bq25890_pdata = {
> +	.regulator_init_data = &bq25890_vbus_init_data,
> +};
> +
> +static const struct property_entry bq25890_props[] = {
> +	PROPERTY_ENTRY_BOOL("ti,skip-init"),
> +	{ }
> +};

The Lenovo Yoga Book firmware set the IINLIM field to 500 mA at
initialization, we need a way to pass maximum allowed current in the fast
charging mode to driver. I have added 'ti,input-max-current' in my port, for
example.

> +
> +static const struct software_node bq25890_node = {
> +	.properties = bq25890_props,
> +};
> +
> +static struct i2c_board_info bq25890_board_info = {
> +	.type = "bq25890",
> +	.addr = 0x6a,
> +	.dev_name = "bq25890",
> +	.swnode = &bq25890_node,
> +	.platform_data = &bq25890_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;
> +	const char *str;
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> @@ -379,17 +417,20 @@ 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);
> +	ret = device_property_read_string(pdev->dev.parent, "intel,cht-wc-setup", &str);
> +	if (ret)
> +		dev_warn(&pdev->dev, "intel,cht-wc-setup not set, not instantiating charger device\n");
> +	else if (!strcmp(str, "bq24292i,max17047,fusb302,pi3usb30532"))
> +		board_info = &bq24190_board_info;
> +	else if (!strcmp(str, "bq25890,bq27520"))
> +		board_info = &bq25890_board_info;
> +	else
> +		dev_warn(&pdev->dev, "Unknown intel,cht-wc-setup value: '%s', not instantiating charger device\n",
> +			 str);
> +
> +	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
> 

-- 
Yauhen Kharuzhy

Re: [PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[Yauhen Kharuzhy]

On Sun, Nov 07, 2021 at 10:07:27PM +0300, Yauhen Kharuzhy wrote:
> On Sat, Oct 30, 2021 at 08:28:11PM +0200, 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 there has been a rudimentary check to make sure the ACPI tables
> > are at least somewhat as expected by checking for the presence of an
> > INT33FE device and sofar the code has assumed that if this INT33FE
> > device is present that the used charger then is a bq24290i.
> > 
> > But some boards with an INT33FE device in their ACPI tables use a
> > different charger IC and some boards don't have an INT33FE device at all.
> > 
> > Since the information about the used charger + fuel-gauge + other chips is
> > necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
> > string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
> > which reliably describes the board's setup of the PMIC.
> > 
> > Switch to using the "intel,cht-wc-setup" property and add support for
> > instantiating an i2c-client for either a bq24292i or a bq25890 charger.
> > 
> > This has been tested on a GPD pocket (which uses the old bq24292i setup)
> > and on a Xiaomi Mi Pad 2 with a bq25890 charger.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/i2c/busses/i2c-cht-wc.c | 77 +++++++++++++++++++++++++--------
> >  1 file changed, 59 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
> > index 1cf68f85b2e1..e7d62af6c39d 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
> > @@ -304,18 +305,55 @@ static struct bq24190_platform_data bq24190_pdata = {
> >  	.regulator_init_data = &bq24190_vbus_init_data,
> >  };
> >  
> > +static struct i2c_board_info bq24190_board_info = {
> > +	.type = "bq24190",
> > +	.addr = 0x6b,
> > +	.dev_name = "bq24190",
> > +	.swnode = &bq24190_node,
> > +	.platform_data = &bq24190_pdata,
> > +};
> > +
> > +static struct regulator_consumer_supply bq25890_vbus_consumer = {
> > +	.supply = "vbus",
> > +	.dev_name = "cht_wcove_pwrsrc",
> > +};
> > +
> > +static const struct regulator_init_data bq25890_vbus_init_data = {
> > +	.constraints = {
> > +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> > +	},
> > +	.consumer_supplies = &bq25890_vbus_consumer,
> > +	.num_consumer_supplies = 1,
> > +};
> > +
> > +static struct bq25890_platform_data bq25890_pdata = {
> > +	.regulator_init_data = &bq25890_vbus_init_data,
> > +};
> > +
> > +static const struct property_entry bq25890_props[] = {
> > +	PROPERTY_ENTRY_BOOL("ti,skip-init"),
> > +	{ }
> > +};
> 
> The Lenovo Yoga Book firmware set the IINLIM field to 500 mA at
> initialization, we need a way to pass maximum allowed current in the fast
> charging mode to driver. I have added 'ti,input-max-current' in my port, for
> example.

ICHG (charging current limit) is too low also (2A versus 4A in Android
sources for this device), so it should be set by properties.

-- 
Yauhen Kharuzhy

Re: [PATCH 09/13] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Hans de Goede]

Hi,

On 11/7/21 19:49, Yauhen Kharuzhy wrote:
> On Sat, Oct 30, 2021 at 08:28:09PM +0200, Hans de Goede wrote:
>> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>> patch for adding regulator support.
> 
> The same notice here: if we enabling the boost mode to supply USB OTG
> device, we should disable charging and vice versa. I don't remember if
> enabling of OTG with CHG enabled really caused issues but I avoided this
> in my Yoga Book work.
> 
> I made quick check — seems that charging can be enabled during of boost
> operation, there are no visible side effects in registers and no
> limitation in the datasheet, so your approach may be good.

Right, the charging bit can simply be always on, if the OTG boost
converter is enabled then the charger it will automatically stop
charging (AFAIK it is partly re-using the same hw even).

I've no idea why the charging bit is being cleared by the BIOS, there
really is no need for this, with the exception of either:

1. Using an external Vbus boost converter, because that can e.g.
deliver more current, then charging does need to be disabled.

2. Disabling charging for battery health reasons

1. is not the case here; and 2. is something which we could export
as something userspace can request but that is optional.

So for normal use there really is no reason to set the charging
bit to 0 ever (and it is 1 on reset), so just setting it to 1
makes the behavior the same as on many ARM devices where this
charger is also used and the same as how similar chargers
like the bq2419x and bq2429x are used on other X86 devices.

Regards,

Hans




> 
>>
>> 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 163ca5d761aa..7504e30f1e4d 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -773,6 +773,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;
>> @@ -782,25 +793,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
>>
> 

Re: [PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[Hans de Goede]

Hi,

On 11/7/21 20:07, Yauhen Kharuzhy wrote:
> On Sat, Oct 30, 2021 at 08:28:11PM +0200, 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 there has been a rudimentary check to make sure the ACPI tables
>> are at least somewhat as expected by checking for the presence of an
>> INT33FE device and sofar the code has assumed that if this INT33FE
>> device is present that the used charger then is a bq24290i.
>>
>> But some boards with an INT33FE device in their ACPI tables use a
>> different charger IC and some boards don't have an INT33FE device at all.
>>
>> Since the information about the used charger + fuel-gauge + other chips is
>> necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
>> string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
>> which reliably describes the board's setup of the PMIC.
>>
>> Switch to using the "intel,cht-wc-setup" property and add support for
>> instantiating an i2c-client for either a bq24292i or a bq25890 charger.
>>
>> This has been tested on a GPD pocket (which uses the old bq24292i setup)
>> and on a Xiaomi Mi Pad 2 with a bq25890 charger.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/i2c/busses/i2c-cht-wc.c | 77 +++++++++++++++++++++++++--------
>>  1 file changed, 59 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
>> index 1cf68f85b2e1..e7d62af6c39d 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
>> @@ -304,18 +305,55 @@ static struct bq24190_platform_data bq24190_pdata = {
>>  	.regulator_init_data = &bq24190_vbus_init_data,
>>  };
>>  
>> +static struct i2c_board_info bq24190_board_info = {
>> +	.type = "bq24190",
>> +	.addr = 0x6b,
>> +	.dev_name = "bq24190",
>> +	.swnode = &bq24190_node,
>> +	.platform_data = &bq24190_pdata,
>> +};
>> +
>> +static struct regulator_consumer_supply bq25890_vbus_consumer = {
>> +	.supply = "vbus",
>> +	.dev_name = "cht_wcove_pwrsrc",
>> +};
>> +
>> +static const struct regulator_init_data bq25890_vbus_init_data = {
>> +	.constraints = {
>> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +	},
>> +	.consumer_supplies = &bq25890_vbus_consumer,
>> +	.num_consumer_supplies = 1,
>> +};
>> +
>> +static struct bq25890_platform_data bq25890_pdata = {
>> +	.regulator_init_data = &bq25890_vbus_init_data,
>> +};
>> +
>> +static const struct property_entry bq25890_props[] = {
>> +	PROPERTY_ENTRY_BOOL("ti,skip-init"),
>> +	{ }
>> +};
> 
> The Lenovo Yoga Book firmware set the IINLIM field to 500 mA at
> initialization, we need a way to pass maximum allowed current in the fast
> charging mode to driver. I have added 'ti,input-max-current' in my port, for
> example.

500 mA is fine, we will run BC1.2 spec charger-type detection
in the PMIC extcon code and then pass the result of that to
the charger-IC. This is not present in this version of the
patch-set because I did not have a yogabook yet when
I wrote this, v2 will take care of this.

I hope to post a v2 sometime the coming 2 weeks.

Regards,

Hans





> 
>> +
>> +static const struct software_node bq25890_node = {
>> +	.properties = bq25890_props,
>> +};
>> +
>> +static struct i2c_board_info bq25890_board_info = {
>> +	.type = "bq25890",
>> +	.addr = 0x6a,
>> +	.dev_name = "bq25890",
>> +	.swnode = &bq25890_node,
>> +	.platform_data = &bq25890_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;
>> +	const char *str;
>>  
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0)
>> @@ -379,17 +417,20 @@ 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);
>> +	ret = device_property_read_string(pdev->dev.parent, "intel,cht-wc-setup", &str);
>> +	if (ret)
>> +		dev_warn(&pdev->dev, "intel,cht-wc-setup not set, not instantiating charger device\n");
>> +	else if (!strcmp(str, "bq24292i,max17047,fusb302,pi3usb30532"))
>> +		board_info = &bq24190_board_info;
>> +	else if (!strcmp(str, "bq25890,bq27520"))
>> +		board_info = &bq25890_board_info;
>> +	else
>> +		dev_warn(&pdev->dev, "Unknown intel,cht-wc-setup value: '%s', not instantiating charger device\n",
>> +			 str);
>> +
>> +	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
>>
> 

Re: [PATCH 13/13] extcon: intel-cht-wc: Add support for devices with an USB-micro-B connector

[Hans de Goede]

Hi,

On 10/31/21 13:52, Andy Shevchenko wrote:
> On Sat, Oct 30, 2021 at 9:29 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.
>>
>> These device are identified by "intel,cht-wc-setup" = "bq25890,bq27520",
>> since there is no Type-C controller on these devices the extcon code must
>> control the Vbus 5V boost converter and the USB role switch depending on
>> the detected cable-type.
> 
> ...
> 
>> +       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);
> 
> if (ret)
>  dev_err()
> else
>  ...
> 
> ?

When doing if-else branches around an error code I always put the success
handling in the if branch and have the else branch deal with the error
to me that feels as the most natural way to do it the error is the exception
and thus the "else"

> 
>> +       }
> 
> ...
> 
> 
>> +               /*
>> +                * Classic micro USB-B setup, this requires controling
> 
> controlling

Fixed for the next version.

Thanks & Regards,

Hans


> 
>> +                * the role-sw and vbus based on the id-pin.
>> +                */
> 

Re: [PATCH 02/13] platform/x86: dmi_device_properties: Add setup info for boards with a CHT Whiskey Cove PMIC

[Hans de Goede]

Hi Andy,

Thank you for your feedback.

On 10/30/21 23:56, Andy Shevchenko wrote:
> On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a new "intel,cht-wc-setup" string property to the "INT34D3:00"
>> i2c_client for the Whiskey Cove PMIC found on several Cherry Trail
>> based devices. At least 3 setups are known:
>>
>> 1. 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 WC PMIC is connected to a TI BQ25890 charger, paired with
>>    a TI BQ27520 fuelgauge, for a USB-2 only Type-C port without PD
>>
>> 3. The WC PMIC is connected to a TI BQ25890 charger, paired with
>>    a TI BQ27542 fuelgauge, for a micro-USB port
>>
>> Which setup is in use cannot be determined reliably from the ACPI tables
>> and various drivers (extcon-intel-cht-wc.c, i2c-cht-wc.c, ...) need
>> to know which setup they are dealing with.
> 
> If it's internal property only, I would rather expect it to start with
> 'linux,' as DWC3 does. And it's also USB related.
> 
> ...
> 
>> +       PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq24292i,max17047,fusb302,pi3usb30532"),
> 
>> +       PROPERTY_ENTRY_STRING("intel,cht-wc-setup", "bq25890,bq27520"),
> 
>  Besides that I'm not sure about the name of the property, maybe
> 'linux,cht-wc-usb-chips' or alike? And since it's a list, can we make
> it a string array?
> 

So now that I also have a yogabook to test on it has become clear that
we really need to treat each device-model/board with a cht-wc PMIC
differently in the various cht-wc MFD cell drivers.

So instead of using device-properties (patch 1 + 2 from this series)
I've chosen to add a intel_cht_wc_get_model( helper to:
drivers/mfd/intel_soc_pmic_chtwc.c

Which uses DMI matching (in a shared place so that we need the DMI
table only once) and returns an enum value which represents all
known boards.

Regards,

Hans

Re: [PATCH 06/13] power: supply: bq25890: Add support for skipping initialization

[Hans de Goede]

Hi,

On 10/31/21 00:07, Andy Shevchenko wrote:
> On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
>>
>> At support for a new "ti,skip-init" 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.
> 
> With 'ti,' prefix it can be a potential collision in name space, for
> internal properties I would rather use 'linux,' one.

Good point, changed for v2.

> ...
> 
>> +       init->write_cfg = !device_property_read_bool(bq->dev, "ti,skip-init");
>> +       if (!init->write_cfg)
>> +               return 0;
> 
> Why to have double negation here?
> I would rather expect that you will have direct value in the structure
> and do a respective check in the functions.

Because in all places except this one we want to know if we need to
write the cfg to the device, removing the double negation here would
mean adding negation to a init->skip_init check in many places, so this
is cleaner.

Regards,

Hans

Re: [PATCH 09/13] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

[Hans de Goede]

Hi,

On 10/31/21 00:10, Andy Shevchenko wrote:
> On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>> patch for adding regulator support.
> 
> ...
> 
>>         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;
>>         }
> 
> While at it,
> 
> default:
>  break;

bq->usb_event is not an enum, so there is no need for this.

Regards,

Hans

Re: [PATCH 11/13] i2c: cht-wc: Add support for devices using a bq25890 charger

[Hans de Goede]

Hi,

On 10/31/21 18:58, Wolfram Sang wrote:
> On Sat, Oct 30, 2021 at 08:28:11PM +0200, 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 there has been a rudimentary check to make sure the ACPI tables
>> are at least somewhat as expected by checking for the presence of an
>> INT33FE device and sofar the code has assumed that if this INT33FE
>> device is present that the used charger then is a bq24290i.
>>
>> But some boards with an INT33FE device in their ACPI tables use a
>> different charger IC and some boards don't have an INT33FE device at all.
>>
>> Since the information about the used charger + fuel-gauge + other chips is
>> necessary in other places too, the kernel now adds a "intel,cht-wc-setup"
>> string property to the Whiskey Cove PMIC i2c-client based on DMI matching,
>> which reliably describes the board's setup of the PMIC.
>>
>> Switch to using the "intel,cht-wc-setup" property and add support for
>> instantiating an i2c-client for either a bq24292i or a bq25890 charger.
>>
>> This has been tested on a GPD pocket (which uses the old bq24292i setup)
>> and on a Xiaomi Mi Pad 2 with a bq25890 charger.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> In general, fine with me from the I2C side:
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>

Thank you for v2 I've refactored things a bit, enough that I'm going
to drop your Ack, sorry.

>> +	else if (!strcmp(str, "bq24292i,max17047,fusb302,pi3usb30532"))
>> +		board_info = &bq24190_board_info;
>> +	else if (!strcmp(str, "bq25890,bq27520"))
>> +		board_info = &bq25890_board_info;
> 
> Very minor nit: I prefer 'strcmp() == 0' because the above could be read
> as 'if not strcmp()' which is sadly misleading. But I am not strict with
> it.

All the strcmp-s are gone in the refactored version.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 10/31/21 00:13, Andy Shevchenko wrote:
> On Sat, Oct 30, 2021 at 9:28 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.
> 
> ...
> 
>> @@ -1018,6 +1059,21 @@ static int bq25890_probe(struct i2c_client *client,
>>                 INIT_WORK(&bq->usb_work, bq25890_usb_work);
>>                 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
>>         }
> 
> }
> #ifdef
> else {
>   ...
> }
> #endif
> 
> is a bit better to maintain (less error prone in case of new code).
> 
> ...
> 
>> +#ifndef _BQ25890_CHARGER_H_
>> +#define _BQ25890_CHARGER_H_
> 
>> +#include <linux/regulator/machine.h>
> 
> struct regulator_init_data;
> 
> should be sufficient, no header is needed.

Thanks, I've fixed both for v2 of the patch-set.

Regards,

Hans


> 
>> +struct bq25890_platform_data {
>> +       const struct regulator_init_data *regulator_init_data;
>> +};
>> +
>> +#endif
>