[PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Yauhen Kharuzhy]

At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
based with Whiskey Cove PMIC), two pitfalls were found:

- for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
  should be set in 0 (and set to 1 for Host mode). Pick up its definition
  and logic from from Intel code drop[1];

- "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
  connected to one of PMIC outputs controlled by CHGDISCTRL register.
  Enable charging at driver initialization. Pick up this from Lenovo's code
  drop[2,3].

Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
are based on some kind of reverse engineering and suppositions, correct me if
this semantic is wrong for common case.

[1]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
[2]. https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
[3]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch

Yauhen Kharuzhy (2):
  extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode
  extcon intel-cht-wc: Enable external charger

 drivers/extcon/extcon-intel-cht-wc.c | 71 +++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Yauhen Kharuzhy]

Whiskey Cove Cherry Trail PMIC requires disabling OTG host mode before
of charger detection procedure. Do this by manipulationg of CHGRCTRL1
register.

Source: APCI DSDT code of Lenovo Yoga Book YB1-X91L and open-sourced
Intel's drivers.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 38 +++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 5ef215297101..4f6ba249bc30 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -29,7 +29,16 @@
 #define CHT_WC_CHGRCTRL0_DBPOFF		BIT(6)
 #define CHT_WC_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
 
-#define CHT_WC_CHGRCTRL1		0x5e17
+#define CHT_WC_CHGRCTRL1			0x5e17
+#define CHT_WC_CHGRCTRL1_DBPEN_MASK		BIT(7)
+#define CHT_WC_CHGRCTRL1_OTGMODE		BIT(6)
+#define CHT_WC_CHGRCTRL1_FTEMP_EVENT		BIT(5)
+#define CHT_WC_CHGRCTRL1_FUSB_INLMT_1500	BIT(4)
+#define CHT_WC_CHGRCTRL1_FUSB_INLMT_900		BIT(3)
+#define CHT_WC_CHGRCTRL1_FUSB_INLMT_500		BIT(2)
+#define CHT_WC_CHGRCTRL1_FUSB_INLMT_150		BIT(1)
+#define CHT_WC_CHGRCTRL1_FUSB_INLMT_100		BIT(0)
+
 
 #define CHT_WC_USBSRC			0x5e29
 #define CHT_WC_USBSRC_STS_MASK		GENMASK(1, 0)
@@ -198,6 +207,29 @@ static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext,
 		dev_err(ext->dev, "Error writing Vbus GPIO CTLO: %d\n", ret);
 }
 
+static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
+				      bool enable)
+{
+	unsigned int chgrctrl1;
+	int ret;
+
+	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL1, &chgrctrl1);
+	if (ret) {
+		dev_err(ext->dev, "Error reading CHGRCTRL1 reg: %d\n", ret);
+		return;
+	}
+
+	if (enable)
+		chgrctrl1 |= CHT_WC_CHGRCTRL1_OTGMODE;
+	else
+		chgrctrl1 &= ~(CHT_WC_CHGRCTRL1_OTGMODE);
+
+	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL1, chgrctrl1);
+	if (ret)
+		dev_err(ext->dev,
+			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
+}
+
 /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */
 static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext,
 				    unsigned int cable, bool state)
@@ -222,10 +254,14 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 
 	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
 	if (id == USB_ID_GND) {
+		cht_wc_extcon_set_otgmode(ext, true);
+
 		/* The 5v boost causes a false VBUS / SDP detect, skip */
 		goto charger_det_done;
 	}
 
+	cht_wc_extcon_set_otgmode(ext, false);
+
 	/* Plugged into a host/charger or not connected? */
 	if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
 		/* Route D+ and D- to PMIC for future charger detection */
-- 
2.20.1

Re: [PATCH 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Hans de Goede]

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> Whiskey Cove Cherry Trail PMIC requires disabling OTG host mode before
> of charger detection procedure. Do this by manipulationg of CHGRCTRL1
> register.
> 
> Source: APCI DSDT code of Lenovo Yoga Book YB1-X91L and open-sourced
> Intel's drivers.

Hmm, of the ACPI code is updating the otg-mode, then there should be
no reason for us to do this, can you provide an acpidump of your
device please?

Regards,

Hans



> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 38 +++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 5ef215297101..4f6ba249bc30 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -29,7 +29,16 @@
>   #define CHT_WC_CHGRCTRL0_DBPOFF		BIT(6)
>   #define CHT_WC_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
>   
> -#define CHT_WC_CHGRCTRL1		0x5e17
> +#define CHT_WC_CHGRCTRL1			0x5e17
> +#define CHT_WC_CHGRCTRL1_DBPEN_MASK		BIT(7)
> +#define CHT_WC_CHGRCTRL1_OTGMODE		BIT(6)
> +#define CHT_WC_CHGRCTRL1_FTEMP_EVENT		BIT(5)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_1500	BIT(4)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_900		BIT(3)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_500		BIT(2)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_150		BIT(1)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_100		BIT(0)
> +
>   
>   #define CHT_WC_USBSRC			0x5e29
>   #define CHT_WC_USBSRC_STS_MASK		GENMASK(1, 0)
> @@ -198,6 +207,29 @@ static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext,
>   		dev_err(ext->dev, "Error writing Vbus GPIO CTLO: %d\n", ret);
>   }
>   
> +static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
> +				      bool enable)
> +{
> +	unsigned int chgrctrl1;
> +	int ret;
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL1, &chgrctrl1);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGRCTRL1 reg: %d\n", ret);
> +		return;
> +	}
> +
> +	if (enable)
> +		chgrctrl1 |= CHT_WC_CHGRCTRL1_OTGMODE;
> +	else
> +		chgrctrl1 &= ~(CHT_WC_CHGRCTRL1_OTGMODE);
> +
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL1, chgrctrl1);
> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
> +}
> +
>   /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */
>   static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext,
>   				    unsigned int cable, bool state)
> @@ -222,10 +254,14 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
>   
>   	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
>   	if (id == USB_ID_GND) {
> +		cht_wc_extcon_set_otgmode(ext, true);
> +
>   		/* The 5v boost causes a false VBUS / SDP detect, skip */
>   		goto charger_det_done;
>   	}
>   
> +	cht_wc_extcon_set_otgmode(ext, false);
> +
>   	/* Plugged into a host/charger or not connected? */
>   	if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
>   		/* Route D+ and D- to PMIC for future charger detection */
> 

Re: [PATCH 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Yauhen Kharuzhy]

On Thu, Feb 14, 2019 at 12:15:00AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> > Whiskey Cove Cherry Trail PMIC requires disabling OTG host mode before
> > of charger detection procedure. Do this by manipulationg of CHGRCTRL1
> > register.
> > 
> > Source: APCI DSDT code of Lenovo Yoga Book YB1-X91L and open-sourced
> > Intel's drivers.
> 
> Hmm, of the ACPI code is updating the otg-mode, then there should be
> no reason for us to do this, can you provide an acpidump of your
> device please?

sure: https://github.com/jekhor/yogabook-linux/blob/master/yoga/acpi/yoga.acpidump.log

Yes, there is ACPI routine for switching this but it is not called at
IRQ handling because it is handled by extcon driver. See my notes here:
https://github.com/jekhor/yogabook-linux/blob/master/yoga/acpi/investigate.txt#L364


-- 
Yauhen Kharuzhy

Re: [PATCH 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Hans de Goede]

Hi,

On 14-02-19 08:09, Yauhen Kharuzhy wrote:
> On Thu, Feb 14, 2019 at 12:15:00AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
>>> Whiskey Cove Cherry Trail PMIC requires disabling OTG host mode before
>>> of charger detection procedure. Do this by manipulationg of CHGRCTRL1
>>> register.
>>>
>>> Source: APCI DSDT code of Lenovo Yoga Book YB1-X91L and open-sourced
>>> Intel's drivers.
>>
>> Hmm, of the ACPI code is updating the otg-mode, then there should be
>> no reason for us to do this, can you provide an acpidump of your
>> device please?
> 
> sure: https://github.com/jekhor/yogabook-linux/blob/master/yoga/acpi/yoga.acpidump.log
> 
> Yes, there is ACPI routine for switching this but it is not called at
> IRQ handling because it is handled by extcon driver. See my notes here:
> https://github.com/jekhor/yogabook-linux/blob/master/yoga/acpi/investigate.txt#L364

The reason the _E12 and/or _E1F methods are not called is because
they are GPIO interrupt handlers for virtual GPIOs on the PMIC,
this is another abomination on these devices, that the ACPI GPIO
interface is abused to make it look like the PMIC has more GPIOs
then it really does and a bunch if them are used either to create
event handlers for no-one knows which events exactly, where as
the 0x20 - 0x55 ones are write-only virtual GPIOs where writing
them is supposed to do no-one knows what.  For some reason the
people who engineered this decided it was not a good idea to have
the AML code actually just directly talk to the hardware (except
that in other cases it does of course) and it goes through these
fake GPIOs which presumably are implemented by the Windows GPIO
driver for the PMIC

Note that we do implement the custom opregion 0x8F for the INT34D3
/ PMI5 device, so the GET/SET calls could work, except that we
have the following bug:

  -drivers/acpi/pmic/intel_pmic.c intel_pmic_regs_handler is wrong,
   assumes everything is a write, read of address 3 should return last value
   read on writing 0 to address 4, see the GET method on INT34D3 / PMI5

Even if we fix that bug though, there still is the issues that we:

1) Do not know when to execute the _E12 and/or _E1F methods
2) Do not know what to do of any of the "virtual" GPIOs are written
3) There is no valid ACPI battery interface for querying battery status

So we are better of just not executing the _E12 and/or _E1F methods
ever and directly talking to the hardware.

<grumble mode on>
If only the engineers who did the firmware interface for these
devices had actually known what they were doing.
</grumble mode>

Regards,

Hans

Re: [PATCH 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Hans de Goede]

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> Whiskey Cove Cherry Trail PMIC requires disabling OTG host mode before
> of charger detection procedure. Do this by manipulationg of CHGRCTRL1
> register.
> 
> Source: APCI DSDT code of Lenovo Yoga Book YB1-X91L and open-sourced
> Intel's drivers.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>

Ok, so this patch should work fine on the existing cht-wc using devices
I am aware of; and otherwise looks good to:

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

Regards,

Hans



> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 38 +++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 5ef215297101..4f6ba249bc30 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -29,7 +29,16 @@
>   #define CHT_WC_CHGRCTRL0_DBPOFF		BIT(6)
>   #define CHT_WC_CHGRCTRL0_CHR_WDT_NOKICK	BIT(7)
>   
> -#define CHT_WC_CHGRCTRL1		0x5e17
> +#define CHT_WC_CHGRCTRL1			0x5e17
> +#define CHT_WC_CHGRCTRL1_DBPEN_MASK		BIT(7)
> +#define CHT_WC_CHGRCTRL1_OTGMODE		BIT(6)
> +#define CHT_WC_CHGRCTRL1_FTEMP_EVENT		BIT(5)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_1500	BIT(4)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_900		BIT(3)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_500		BIT(2)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_150		BIT(1)
> +#define CHT_WC_CHGRCTRL1_FUSB_INLMT_100		BIT(0)
> +
>   
>   #define CHT_WC_USBSRC			0x5e29
>   #define CHT_WC_USBSRC_STS_MASK		GENMASK(1, 0)
> @@ -198,6 +207,29 @@ static void cht_wc_extcon_set_5v_boost(struct cht_wc_extcon_data *ext,
>   		dev_err(ext->dev, "Error writing Vbus GPIO CTLO: %d\n", ret);
>   }
>   
> +static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
> +				      bool enable)
> +{
> +	unsigned int chgrctrl1;
> +	int ret;
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL1, &chgrctrl1);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGRCTRL1 reg: %d\n", ret);
> +		return;
> +	}
> +
> +	if (enable)
> +		chgrctrl1 |= CHT_WC_CHGRCTRL1_OTGMODE;
> +	else
> +		chgrctrl1 &= ~(CHT_WC_CHGRCTRL1_OTGMODE);
> +
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL1, chgrctrl1);
> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
> +}
> +
>   /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */
>   static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext,
>   				    unsigned int cable, bool state)
> @@ -222,10 +254,14 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
>   
>   	id = cht_wc_extcon_get_id(ext, pwrsrc_sts);
>   	if (id == USB_ID_GND) {
> +		cht_wc_extcon_set_otgmode(ext, true);
> +
>   		/* The 5v boost causes a false VBUS / SDP detect, skip */
>   		goto charger_det_done;
>   	}
>   
> +	cht_wc_extcon_set_otgmode(ext, false);
> +
>   	/* Plugged into a host/charger or not connected? */
>   	if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
>   		/* Route D+ and D- to PMIC for future charger detection */
> 

[PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

In some configuration external charge "#charge enable" signal is
connected to PMIC. Enable it at device probing to allow charging.

Tested at Lenovo Yoga Book (YB1-X91L).

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 4f6ba249bc30..00cb3084955e 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -57,6 +57,11 @@
 #define CHT_WC_USBSRC_TYPE_OTHER	8
 #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
 
+#define CHT_WC_CHGDISCTRL		0x5e2f
+#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
+#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00
+#define CHT_WC_CHGDISCTRL_CCSM_MASK	0x51
+
 #define CHT_WC_PWRSRC_IRQ		0x6e03
 #define CHT_WC_PWRSRC_IRQ_MASK		0x6e0f
 #define CHT_WC_PWRSRC_STS		0x6e1e
@@ -230,6 +235,31 @@ static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
 			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
 }
 
+static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext,
+					  bool enable)
+{
+	unsigned int chgdisctrl;
+	int ret;
+
+	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL, &chgdisctrl);
+	if (ret) {
+		dev_err(ext->dev, "Error reading CHGDISCTRL reg: %d\n", ret);
+		return;
+	}
+
+	chgdisctrl &= CHT_WC_CHGDISCTRL_CCSM_MASK;
+
+	if (enable)
+		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_EN;
+	else
+		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_DIS;
+
+	dev_dbg(ext->dev, "Writing CHGDISCTRL: 0x%02x\n", chgdisctrl);
+	ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL, chgdisctrl);
+	if (ret)
+		dev_err(ext->dev, "Error writing CHGDISCTRL: %d\n", ret);
+}
+
 /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */
 static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext,
 				    unsigned int cable, bool state)
@@ -362,6 +392,9 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	 */
 	cht_wc_extcon_set_5v_boost(ext, false);
 
+	/* Allow charging by external battery charger*/
+	cht_wc_extcon_enable_charging(ext, true);
+
 	/* Enable sw control */
 	ret = cht_wc_extcon_sw_control(ext, true);
 	if (ret)
-- 
2.20.1

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Hans de Goede]

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> In some configuration external charge "#charge enable" signal is
> connected to PMIC. Enable it at device probing to allow charging.
> 
> Tested at Lenovo Yoga Book (YB1-X91L).
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 4f6ba249bc30..00cb3084955e 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -57,6 +57,11 @@
>   #define CHT_WC_USBSRC_TYPE_OTHER	8
>   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
>   
> +#define CHT_WC_CHGDISCTRL		0x5e2f
> +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
> +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00

Hmm, the enable mask here does not match the enable mask from:

https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch

Which has:

#define CHGDISFN_EN_CCSM_VAL           0x50
#define CHGDISFN_DIS_CCSM_VAL          0x11
#define CHGDISFN_CCSM_MASK             0x51

Where as on my hardware, the PMIC comes up with 0x50
in the 0x5e2f register, exactly matching the values
from that patch.

Why did you change this value ?

It would be interesting to get a dump of the
charger's i2c registers using i2cdump before
and after writing this new value to register
0x5e2f. Note you can simply leave the patch adding
the write out of the kernel, and then do:

i2cdump <charger>
i2cget -y -f 6 0x5e 0x2f
# check 0x00 is the correct val
i2cset -y -f 6 0x5e 0x2f 0x00
i2cdump <charger>

To see if any reg values of the charger-ic have changed.

Note bus 6 is the right bus for the PMIC on my systems,
but it might be different, to find the right bus do:

ls -l /sys/bus/devices/i2c-INT34D3:00

And look at what the i2c bus-number is in that symlink.


Also can you please provide i2cdump output for the
0x5e and 0x6e devices of the PMIC, preferably from a
clean boot, without your patches, so that we can
see what the value of the 0x5e2f register is before
your code modifies it.

i2cdump -y -f 6 0x5e
i2cdump -y -f 6 0x6e

Regards,

Hans





> +#define CHT_WC_CHGDISCTRL_CCSM_MASK	0x51
> +
>   #define CHT_WC_PWRSRC_IRQ		0x6e03
>   #define CHT_WC_PWRSRC_IRQ_MASK		0x6e0f
>   #define CHT_WC_PWRSRC_STS		0x6e1e
> @@ -230,6 +235,31 @@ static void cht_wc_extcon_set_otgmode(struct cht_wc_extcon_data *ext,
>   			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
>   }
>   
> +static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext,
> +					  bool enable)
> +{
> +	unsigned int chgdisctrl;
> +	int ret;
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL, &chgdisctrl);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGDISCTRL reg: %d\n", ret);
> +		return;
> +	}
> +
> +	chgdisctrl &= CHT_WC_CHGDISCTRL_CCSM_MASK;
> +
> +	if (enable)
> +		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_EN;
> +	else
> +		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_DIS;
> +
> +	dev_dbg(ext->dev, "Writing CHGDISCTRL: 0x%02x\n", chgdisctrl);
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL, chgdisctrl);
> +	if (ret)
> +		dev_err(ext->dev, "Error writing CHGDISCTRL: %d\n", ret);
> +}
> +
>   /* Small helper to sync EXTCON_CHG_USB_SDP and EXTCON_USB state */
>   static void cht_wc_extcon_set_state(struct cht_wc_extcon_data *ext,
>   				    unsigned int cable, bool state)
> @@ -362,6 +392,9 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	 */
>   	cht_wc_extcon_set_5v_boost(ext, false);
>   
> +	/* Allow charging by external battery charger*/
> +	cht_wc_extcon_enable_charging(ext, true);
> +
>   	/* Enable sw control */
>   	ret = cht_wc_extcon_sw_control(ext, true);
>   	if (ret)
> 

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

On Thu, Feb 14, 2019 at 05:31:48PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> > In some configuration external charge "#charge enable" signal is
> > connected to PMIC. Enable it at device probing to allow charging.
> > 
> > Tested at Lenovo Yoga Book (YB1-X91L).
> > 
> > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > ---
> >   drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> > index 4f6ba249bc30..00cb3084955e 100644
> > --- a/drivers/extcon/extcon-intel-cht-wc.c
> > +++ b/drivers/extcon/extcon-intel-cht-wc.c
> > @@ -57,6 +57,11 @@
> >   #define CHT_WC_USBSRC_TYPE_OTHER	8
> >   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
> > +#define CHT_WC_CHGDISCTRL		0x5e2f
> > +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
> > +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00
> 
> Hmm, the enable mask here does not match the enable mask from:
> 
> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> 
> Which has:
> 
> #define CHGDISFN_EN_CCSM_VAL           0x50
> #define CHGDISFN_DIS_CCSM_VAL          0x11
> #define CHGDISFN_CCSM_MASK             0x51
> 
> Where as on my hardware, the PMIC comes up with 0x50
> in the 0x5e2f register, exactly matching the values
> from that patch.
> 
> Why did you change this value ?

Good question... I found this values in Lenovo's sources and use them
'as is':
https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/intel_pmic_ccsm.h#L255

I don't remember if charger worked with Intel's value, I need to
re-check this.

(...why does Intel close documentation for this piece of HW?...)

> 
> It would be interesting to get a dump of the
> charger's i2c registers using i2cdump before
> and after writing this new value to register
> 0x5e2f. Note you can simply leave the patch adding
> the write out of the kernel, and then do:
> 
> i2cdump <charger>
> i2cget -y -f 6 0x5e 0x2f
> # check 0x00 is the correct val
> i2cset -y -f 6 0x5e 0x2f 0x00
> i2cdump <charger>
> 
> To see if any reg values of the charger-ic have changed.
> 
> Note bus 6 is the right bus for the PMIC on my systems,
> but it might be different, to find the right bus do:
> 
> ls -l /sys/bus/devices/i2c-INT34D3:00
> 
> And look at what the i2c bus-number is in that symlink.
> 
> 
> Also can you please provide i2cdump output for the
> 0x5e and 0x6e devices of the PMIC, preferably from a
> clean boot, without your patches, so that we can
> see what the value of the 0x5e2f register is before
> your code modifies it.
> 
> i2cdump -y -f 6 0x5e
> i2cdump -y -f 6 0x6e

OK, I will send this dumps soon. I used i2get/i2cset for checking this
values and PMIC behaviour already but don't remember exactly results.


-- 
Yauhen Kharuzhy

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

On Fri, Feb 15, 2019 at 09:32:50AM +0300, Yauhen Kharuzhy wrote:
> On Thu, Feb 14, 2019 at 05:31:48PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> > > In some configuration external charge "#charge enable" signal is
> > > connected to PMIC. Enable it at device probing to allow charging.
> > > 
> > > Tested at Lenovo Yoga Book (YB1-X91L).
> > > 
> > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > > ---
> > >   drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> > > index 4f6ba249bc30..00cb3084955e 100644
> > > --- a/drivers/extcon/extcon-intel-cht-wc.c
> > > +++ b/drivers/extcon/extcon-intel-cht-wc.c
> > > @@ -57,6 +57,11 @@
> > >   #define CHT_WC_USBSRC_TYPE_OTHER	8
> > >   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
> > > +#define CHT_WC_CHGDISCTRL		0x5e2f
> > > +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
> > > +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00
> > 
> > Hmm, the enable mask here does not match the enable mask from:
> > 
> > https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> > 
> > Which has:
> > 
> > #define CHGDISFN_EN_CCSM_VAL           0x50
> > #define CHGDISFN_DIS_CCSM_VAL          0x11
> > #define CHGDISFN_CCSM_MASK             0x51
> > 
> > Where as on my hardware, the PMIC comes up with 0x50
> > in the 0x5e2f register, exactly matching the values
> > from that patch.
> > 
> > Why did you change this value ?
> 
> Good question... I found this values in Lenovo's sources and use them
> 'as is':
> https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/intel_pmic_ccsm.h#L255
> 
> I don't remember if charger worked with Intel's value, I need to
> re-check this.

As we know now, value 0x50 meaning is:
- HW mode
- regular output type
- low level at output

0x00 meaning is:
- SW mode
- open-drain output
- low level at output

I don't know what exactly "HW/SW mode" means but I suppose that this pin
can be controlled by PMIC internal charge state algorithm (if it is
enabled) or by software (extcon driver).

So, if we set 0x50 value and disable HW-controlled charging entirely (as
extcon driver does), we don't set 'charger enable' signal to low as
expected. Its exactly state is unknown, but I checked – it is HIGH.
Charger doesn't start charging with this value and starts with 0x00.

0x0b register of 0x6b device on bus 7 – it is the status register of BQ25892
charger, where bits 3,4 describe charging state, 10b means fast charging,
00b means not charging.

root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x00
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x16
root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x01
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x06
root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x10
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x16
root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x11
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x06
root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x50
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x06
root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x51
root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
0x06


After reading of Intel's and Lenovo's sources i understood that Intel's
meaning of CHGDISFN_EN_CCSM_VAL is 'enable HW control of charging' and
Lenovo's is 'enable charging'. Lenovo set this value immediately after
probing and after resume, Intel – only after resume. HW-controlled
charging is disabled entirely at probing, so I don't know how Intel's
driver enables charging.

So, I propose:

1) save initial value of CHGDIS register for restoring it at driver
remove (because the extcon-intel-cht-wc driver restores HW control in
cht_wc_extcon_remove()).
2) at driver start, enable SW control of CHGDIS and set its value to 0.
3) at driver removing, restore the saved state.

Q: In theory, enabling of 'charge enable' output without of properly
configuration of external charger can cause some problems (USB overload,
battery overcurrent etc.). I think that there are no such stupidly
designed devices exist but we cannot be sure. What should we do with this?


> 
> > 
> > It would be interesting to get a dump of the
> > charger's i2c registers using i2cdump before
> > and after writing this new value to register
> > 0x5e2f. Note you can simply leave the patch adding
> > the write out of the kernel, and then do:
> > 
> > i2cdump <charger>
> > i2cget -y -f 6 0x5e 0x2f
> > # check 0x00 is the correct val
> > i2cset -y -f 6 0x5e 0x2f 0x00
> > i2cdump <charger>
> > 
> > To see if any reg values of the charger-ic have changed.
> > 
> > Note bus 6 is the right bus for the PMIC on my systems,
> > but it might be different, to find the right bus do:
> > 
> > ls -l /sys/bus/devices/i2c-INT34D3:00
> > 
> > And look at what the i2c bus-number is in that symlink.
> > 
> > 
> > Also can you please provide i2cdump output for the
> > 0x5e and 0x6e devices of the PMIC, preferably from a
> > clean boot, without your patches, so that we can
> > see what the value of the 0x5e2f register is before
> > your code modifies it.
> > 
> > i2cdump -y -f 6 0x5e
> > i2cdump -y -f 6 0x6e

linux-next (5.0.0-rc5-next-20190208+), extcon module is disabled,
charger is connected before power-on:

root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 ff 00 XX XX 03 00 00 50 00 XX XX 09 XX 00 XX    ...XX?..P.XX?X.X
10: 00 00 4a 01 0b 0b 10 80 01 29 01 XX 00 74 04 1b    ..J??????)?X.t??
20: 06 ff 00 6b 00 6b 0a 03 73 8a 03 ff 6c 6c 00 10    ?..k.k??s??.ll.?
30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX

root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 01 02 00 00 00 80 00 18 0d XX 11 00 00 80 af 00    ??...?.??X?..??.
10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 15 00    ..?.?.X?X`?X.X?.
20: 00 00 00 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ...???Z?????+..X
30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 b0 XX    ??==XXXXX.???.?X
e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X

root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 06 46 00 00 28 02 00 00 00 00 00 00 00 92 08 04    ?F..(?.......???
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
20: 12 00 01 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
30: 65 31 65 05 8e 00 20 b8 00 0c 00 38 00 46 00 ff    e1e??. ?.?.8.F..
40: 00 00 00 d8 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
60: 10 65 05 de 20 b8 20 b8 1f 00 00 00 00 00 00 00    ?e?? ? ??.......
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
b0: 00 00 XX XX XX 00 00 06 00 14 18 3f 3f 2c 3f 3f    ..XXX..?.????,??
c0: 04 1f 1f 04 10 05 00 a8 8e b3 00 1f 04 72 00 83    ??????.???.??r.?
d0: 00 81 17 00 XX 00 00 00 00 00 94 7a b7 XX XX 19    .??.X.....?z?XX?
e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
f0: 00 00 00 00 00 e9 00 50 00 02 00 01 XX XX 01 00    .....?.P.?.?XX?.


The same, without charger connected:

root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 ff 00 XX XX 03 00 00 50 00 XX XX 09 XX 00 XX    ...XX?..P.XX?X.X
10: 00 00 4a 01 0b 0b 10 80 01 0b 01 XX 00 74 04 1b    ..J????????X.t??
20: 06 ff 00 6b 00 6b 0a 03 73 43 00 ff 6c 6c 00 10    ?..k.k??sC..ll.?
30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 01 02 01 05 00 80 00 18 0d XX 10 00 00 80 af 00    ????.?.??X?..??.
10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 14 00    ..?.?.X?X`?X.X?.
20: 00 00 01 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ..????Z?????+..X
30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 a0 XX    ??==XXXXX.???.?X
e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X
root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 06 46 00 00 a6 02 00 00 00 00 00 00 00 92 08 03    ?F..??.......???
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
20: 12 00 01 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
30: 65 31 65 05 8e 00 20 b8 00 6f 00 49 00 5f 00 ff    e1e??. ?.o.I._..
40: 00 00 00 d1 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
60: 10 65 05 de 20 b8 20 b8 08 00 00 00 00 00 00 00    ?e?? ? ??.......
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
b0: 00 00 XX XX XX 00 00 06 00 14 31 3f 3f 2f 3f 3f    ..XXX..?.?1??/??
c0: 04 1f 1f 04 10 05 00 75 8f b3 00 1f 04 72 00 83    ??????.u??.??r.?
d0: 00 81 17 00 XX 00 00 00 00 00 b2 90 7a XX XX 19    .??.X.....??zXX?
e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
f0: 00 00 00 00 00 9c 00 ac 00 02 00 01 XX XX 01 00    .....?.?.?.?XX?.


The same, with USB hub connected (OTG host mode):

root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 ff 00 XX XX 03 00 01 50 00 XX XX 09 XX 00 XX    ...XX?.?P.XX?X.X
10: 00 00 4a 01 0b 0b 10 c0 01 0f 01 XX 00 74 04 1b    ..J????????X.t??
20: 06 ff 00 6b 00 6b 0a 2a 73 00 00 ff 6c 6c 00 50    ?..k.k?*s...ll.P
30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 01 02 01 15 00 00 00 18 0d XX 00 00 00 80 af 00    ????...??X...??.
10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 0d 00    ..?.?.X?X`?X.X?.
20: 00 00 01 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ..????Z?????+..X
30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 c0 XX    ??==XXXXX.???.?X
e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X
root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 06 46 00 00 99 02 00 00 00 00 00 00 00 92 08 04    ?F..??.......???
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
20: 12 00 07 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
30: 65 31 65 05 8e 00 20 b8 00 d9 00 a2 00 c6 00 ff    e1e??. ?.?.?.?..
40: 00 00 00 cc 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
60: 10 65 05 de 20 b8 20 b8 08 00 00 00 00 00 00 00    ?e?? ? ??.......
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
b0: 00 00 XX XX XX 00 00 06 00 14 39 3f 3f 36 3f 3f    ..XXX..?.?9??6??
c0: 04 1f 1f 04 10 05 00 21 91 b3 00 1f 04 72 00 83    ??????.!??.??r.?
d0: 00 81 17 00 XX 00 00 00 00 00 b2 92 67 XX XX 19    .??.X.....??gXX?
e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
f0: 00 00 00 00 00 94 00 99 00 03 00 03 XX XX 01 00    .....?.?.?.?XX?.

-- 
Yauhen Kharuzhy

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Hans de Goede]

Hi,

On 17-02-19 22:52, Yauhen Kharuzhy wrote:
> On Fri, Feb 15, 2019 at 09:32:50AM +0300, Yauhen Kharuzhy wrote:
>> On Thu, Feb 14, 2019 at 05:31:48PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
>>>> In some configuration external charge "#charge enable" signal is
>>>> connected to PMIC. Enable it at device probing to allow charging.
>>>>
>>>> Tested at Lenovo Yoga Book (YB1-X91L).
>>>>
>>>> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
>>>> ---
>>>>    drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
>>>>    1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
>>>> index 4f6ba249bc30..00cb3084955e 100644
>>>> --- a/drivers/extcon/extcon-intel-cht-wc.c
>>>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
>>>> @@ -57,6 +57,11 @@
>>>>    #define CHT_WC_USBSRC_TYPE_OTHER	8
>>>>    #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
>>>> +#define CHT_WC_CHGDISCTRL		0x5e2f
>>>> +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
>>>> +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00
>>>
>>> Hmm, the enable mask here does not match the enable mask from:
>>>
>>> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
>>>
>>> Which has:
>>>
>>> #define CHGDISFN_EN_CCSM_VAL           0x50
>>> #define CHGDISFN_DIS_CCSM_VAL          0x11
>>> #define CHGDISFN_CCSM_MASK             0x51
>>>
>>> Where as on my hardware, the PMIC comes up with 0x50
>>> in the 0x5e2f register, exactly matching the values
>>> from that patch.
>>>
>>> Why did you change this value ?
>>
>> Good question... I found this values in Lenovo's sources and use them
>> 'as is':
>> https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/intel_pmic_ccsm.h#L255
>>
>> I don't remember if charger worked with Intel's value, I need to
>> re-check this.
> 
> As we know now, value 0x50 meaning is:
> - HW mode
> - regular output type
> - low level at output
> 
> 0x00 meaning is:
> - SW mode
> - open-drain output
> - low level at output
> 
> I don't know what exactly "HW/SW mode" means but I suppose that this pin
> can be controlled by PMIC internal charge state algorithm (if it is
> enabled) or by software (extcon driver).
> 
> So, if we set 0x50 value and disable HW-controlled charging entirely (as
> extcon driver does), we don't set 'charger enable' signal to low as
> expected. Its exactly state is unknown, but I checked – it is HIGH.
> Charger doesn't start charging with this value and starts with 0x00.
> 
> 0x0b register of 0x6b device on bus 7 – it is the status register of BQ25892
> charger, where bits 3,4 describe charging state, 10b means fast charging,
> 00b means not charging.
> 
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x00
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x16
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x01
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x06
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x10
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x16
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x11
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x06
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x50
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x06
> root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x51
> root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> 0x06
> 
> 
> After reading of Intel's and Lenovo's sources i understood that Intel's
> meaning of CHGDISFN_EN_CCSM_VAL is 'enable HW control of charging' and
> Lenovo's is 'enable charging'. Lenovo set this value immediately after
> probing and after resume, Intel – only after resume. HW-controlled
> charging is disabled entirely at probing, so I don't know how Intel's
> driver enables charging.
> 
> So, I propose:
> 
> 1) save initial value of CHGDIS register for restoring it at driver
> remove (because the extcon-intel-cht-wc driver restores HW control in
> cht_wc_extcon_remove()).
> 2) at driver start, enable SW control of CHGDIS and set its value to 0.
> 3) at driver removing, restore the saved state.

This sounds good to me, note I believe the extcon code should not touch
bit 4 (open-drain,vs regular), but since we disable the charger state-machine
we should obviously then switch the pin to sw mode and drive the pin low
so that the charger still works.

I believe that on the GPD win / GPD pocket the chgdis pin of the PMIC
is not connected to the charger, since writing different values to reg
5e2f has no effect there.

I do wonder how this interacts with inserting an otg-host cable into
the micro-usb, I mean a cable like this:

https://alexnld.com/wp-content/uploads/2013/11/S-UDC-103.jpg

A cable like this will short the id-pin to ground and at this point
we should disable the charger and enable a 5V boost converter to
supply 5V to the device connected to the USB-A connector.

On the GPD win / GPD pocket this is controlled through the Type-C
framework and the V5 boost converter is actually part of the
charger-IC, so charging gets disabled automatically when we tell
the charger-IC to do enable its 5V boost converter.

Do you already have an idea how to deal with this, it might be good
to have at least an idea how we want to handle 5V boost before we
merge the patch to control the CHGDIS pin, since we may need to
disable the charger when the id-pin is connected to GND, so that
the charger does not try to charge from the output of the 5V boost
converter, which happens when an external 5v boost converter is used.

I also wonder if you've considered just disabling the extcon driver
for the PMIC leaving it in automatic mode. Unlike the GPD win / pocket
with their Type-C connector, your device seems to actually be using
the PMIC as it was designed, so the automatic mode might just work
and not touching the PMIC at all might be best.

> Q: In theory, enabling of 'charge enable' output without of properly
> configuration of external charger can cause some problems (USB overload,
> battery overcurrent etc.). I think that there are no such stupidly
> designed devices exist but we cannot be sure. What should we do with this?

This should not be a problem, the input-current-limit of the charger
will already be setup by the firmware at boot and if a charger gets
plugged in later then the input-current-limit will reset to 500mA.

Likewise the max charging current for the battery should already
be configured properly by the firmware (this must be the case since
the device will also charge while off) and we don't even know what
the max charging current for the battery is, so we just have to rely
on the firmware/BIOS here.

>>> It would be interesting to get a dump of the
>>> charger's i2c registers using i2cdump before
>>> and after writing this new value to register
>>> 0x5e2f. Note you can simply leave the patch adding
>>> the write out of the kernel, and then do:
>>>
>>> i2cdump <charger>
>>> i2cget -y -f 6 0x5e 0x2f
>>> # check 0x00 is the correct val
>>> i2cset -y -f 6 0x5e 0x2f 0x00
>>> i2cdump <charger>
>>>
>>> To see if any reg values of the charger-ic have changed.
>>>
>>> Note bus 6 is the right bus for the PMIC on my systems,
>>> but it might be different, to find the right bus do:
>>>
>>> ls -l /sys/bus/devices/i2c-INT34D3:00
>>>
>>> And look at what the i2c bus-number is in that symlink.
>>>
>>>
>>> Also can you please provide i2cdump output for the
>>> 0x5e and 0x6e devices of the PMIC, preferably from a
>>> clean boot, without your patches, so that we can
>>> see what the value of the 0x5e2f register is before
>>> your code modifies it.
>>>
>>> i2cdump -y -f 6 0x5e
>>> i2cdump -y -f 6 0x6e
> 
> linux-next (5.0.0-rc5-next-20190208+), extcon module is disabled,
> charger is connected before power-on:
> 
> root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 00 ff 00 XX XX 03 00 00 50 00 XX XX 09 XX 00 XX    ...XX?..P.XX?X.X
> 10: 00 00 4a 01 0b 0b 10 80 01 29 01 XX 00 74 04 1b    ..J??????)?X.t??
> 20: 06 ff 00 6b 00 6b 0a 03 73 8a 03 ff 6c 6c 00 10    ?..k.k??s??.ll.?
> 30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
> 40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
> 50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
> 60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
> 70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
> 80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
> 90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
> a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
> b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 
> root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 01 02 00 00 00 80 00 18 0d XX 11 00 00 80 af 00    ??...?.??X?..??.
> 10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 15 00    ..?.?.X?X`?X.X?.
> 20: 00 00 00 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ...???Z?????+..X
> 30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
> 40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
> 50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
> 60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
> 70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
> 90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
> a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
> b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
> c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
> d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 b0 XX    ??==XXXXX.???.?X
> e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
> f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X
> 
> root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 06 46 00 00 28 02 00 00 00 00 00 00 00 92 08 04    ?F..(?.......???
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
> 20: 12 00 01 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
> 30: 65 31 65 05 8e 00 20 b8 00 0c 00 38 00 46 00 ff    e1e??. ?.?.8.F..
> 40: 00 00 00 d8 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
> 50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
> 60: 10 65 05 de 20 b8 20 b8 1f 00 00 00 00 00 00 00    ?e?? ? ??.......
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
> 90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
> b0: 00 00 XX XX XX 00 00 06 00 14 18 3f 3f 2c 3f 3f    ..XXX..?.????,??
> c0: 04 1f 1f 04 10 05 00 a8 8e b3 00 1f 04 72 00 83    ??????.???.??r.?
> d0: 00 81 17 00 XX 00 00 00 00 00 94 7a b7 XX XX 19    .??.X.....?z?XX?
> e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
> f0: 00 00 00 00 00 e9 00 50 00 02 00 01 XX XX 01 00    .....?.P.?.?XX?.
> 
> 
> The same, without charger connected:
> 
> root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 00 ff 00 XX XX 03 00 00 50 00 XX XX 09 XX 00 XX    ...XX?..P.XX?X.X
> 10: 00 00 4a 01 0b 0b 10 80 01 0b 01 XX 00 74 04 1b    ..J????????X.t??
> 20: 06 ff 00 6b 00 6b 0a 03 73 43 00 ff 6c 6c 00 10    ?..k.k??sC..ll.?
> 30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
> 40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
> 50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
> 60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
> 70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
> 80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
> 90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
> a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
> b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 01 02 01 05 00 80 00 18 0d XX 10 00 00 80 af 00    ????.?.??X?..??.
> 10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 14 00    ..?.?.X?X`?X.X?.
> 20: 00 00 01 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ..????Z?????+..X
> 30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
> 40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
> 50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
> 60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
> 70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
> 90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
> a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
> b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
> c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
> d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 a0 XX    ??==XXXXX.???.?X
> e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
> f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X
> root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 06 46 00 00 a6 02 00 00 00 00 00 00 00 92 08 03    ?F..??.......???
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
> 20: 12 00 01 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
> 30: 65 31 65 05 8e 00 20 b8 00 6f 00 49 00 5f 00 ff    e1e??. ?.o.I._..
> 40: 00 00 00 d1 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
> 50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
> 60: 10 65 05 de 20 b8 20 b8 08 00 00 00 00 00 00 00    ?e?? ? ??.......
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
> 90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
> b0: 00 00 XX XX XX 00 00 06 00 14 31 3f 3f 2f 3f 3f    ..XXX..?.?1??/??
> c0: 04 1f 1f 04 10 05 00 75 8f b3 00 1f 04 72 00 83    ??????.u??.??r.?
> d0: 00 81 17 00 XX 00 00 00 00 00 b2 90 7a XX XX 19    .??.X.....??zXX?
> e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
> f0: 00 00 00 00 00 9c 00 ac 00 02 00 01 XX XX 01 00    .....?.?.?.?XX?.
> 
> 
> The same, with USB hub connected (OTG host mode):
> 
> root@yogabook:/home/jek# i2cdump -f -y 6 0x5e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 00 ff 00 XX XX 03 00 01 50 00 XX XX 09 XX 00 XX    ...XX?.?P.XX?X.X
> 10: 00 00 4a 01 0b 0b 10 c0 01 0f 01 XX 00 74 04 1b    ..J????????X.t??
> 20: 06 ff 00 6b 00 6b 0a 2a 73 00 00 ff 6c 6c 00 50    ?..k.k?*s...ll.P
> 30: 02 0e 00 fe 00 00 03 ff 00 51 01 3b 01 a7 01 62    ??.?..?..Q?;???b
> 40: 00 42 00 42 00 9f 00 9f 00 9f 00 d2 00 d2 00 d2    .B.B.?.?.?.?.?.?
> 50: 01 09 00 4d 00 8e 00 d3 01 11 01 56 02 XX XX XX    ??.M.?.????V?XXX
> 60: XX 02 0b ff 6a 6c 0e 26 34 44 52 60 XX XX XX XX    X??.jl?&4DR`XXXX
> 70: 00 00 00 00 00 00 00 00 00 00 00 XX XX XX XX XX    ...........XXXXX
> 80: XX 00 00 00 XX XX 00 00 00 00 00 00 00 00 00 00    X...XX..........
> 90: 00 XX XX XX XX XX XX ff ff ff ff ff ff ff ff ff    .XXXXXX.........
> a0: ff ff XX XX XX XX XX XX 00 00 XX XX XX XX XX XX    ..XXXXXX..XXXXXX
> b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> root@yogabook:/home/jek# i2cdump -f -y 6 0x6e
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 01 02 01 15 00 00 00 18 0d XX 00 00 00 80 af 00    ????...??X...??.
> 10: 00 00 b0 00 1a ff XX 11 XX 60 03 XX 00 XX 0d 00    ..?.?.X?X`?X.X?.
> 20: 00 00 01 0c ac 0b 5a 11 10 0c 1f 18 2b 00 00 XX    ..????Z?????+..X
> 30: 4a 4a XX 08 09 00 09 01 09 08 08 00 38 XX XX XX    JJX??.?????.8XXX
> 40: XX XX XX 09 08 XX XX XX XX XX XX XX XX XX XX XX    XXX??XXXXXXXXXXX
> 50: XX XX XX 0f XX XX 07 00 07 01 01 9b 63 01 07 01    XXX?XX?.????c???
> 60: XX XX XX 02 06 06 06 00 06 20 00 00 66 00 XX XX    XXX????.? ..f.XX
> 70: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX    XXXXXXXXXXXXXXXX
> 80: XX XX XX XX XX XX XX XX XX XX XX 37 XX XX XX XX    XXXXXXXXXXX7XXXX
> 90: 01 01 XX XX XX 01 XX XX XX 01 01 01 01 00 01 01    ??XXX?XXX????.??
> a0: 01 01 01 01 XX XX XX XX XX XX XX XX XX XX XX XX    ????XXXXXXXXXXXX
> b0: XX XX XX XX 12 14 ad XX 00 00 00 18 00 XX XX XX    XXXX???X...?.XXX
> c0: 1f 1f 1f 14 14 1f 3d 34 3d 33 33 3d 3d 1f 1f 3d    ??????=4=33==??=
> d0: 1f 1f 3d 3d XX XX XX XX XX 00 f0 02 05 00 c0 XX    ??==XXXXX.???.?X
> e0: XX XX XX 00 00 00 7d 04 XX XX XX XX XX XX XX XX    XXX...}?XXXXXXXX
> f0: 81 08 XX XX XX XX XX XX 0f XX 03 06 06 06 2f XX    ??XXXXXX?X????/X
> root@yogabook:/home/jek# i2cdump -f -y 6 0x4f
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 06 46 00 00 99 02 00 00 00 00 00 00 00 92 08 04    ?F..??.......???
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 02 11 52    .............??R
> 20: 12 00 07 20 b8 10 87 20 b8 10 87 20 b8 10 87 31    ?.? ??? ??? ???1
> 30: 65 31 65 05 8e 00 20 b8 00 d9 00 a2 00 c6 00 ff    e1e??. ?.?.?.?..
> 40: 00 00 00 cc 25 a1 15 12 11 2d 10 fe 10 b8 27 45    ...?%????-????'E
> 50: 10 87 00 89 06 2f 00 59 00 59 00 59 10 65 10 65    ??.??/.Y.Y.Y?e?e
> 60: 10 65 05 de 20 b8 20 b8 08 00 00 00 00 00 00 00    ?e?? ? ??.......
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 80: 00 00 77 07 77 77 77 07 00 00 00 00 00 00 00 00    ..w?www?........
> 90: 00 00 00 00 00 00 00 00 00 00 00 ff ff ff ff ff    ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00 00    ................
> b0: 00 00 XX XX XX 00 00 06 00 14 39 3f 3f 36 3f 3f    ..XXX..?.?9??6??
> c0: 04 1f 1f 04 10 05 00 21 91 b3 00 1f 04 72 00 83    ??????.!??.??r.?
> d0: 00 81 17 00 XX 00 00 00 00 00 b2 92 67 XX XX 19    .??.X.....??gXX?
> e0: 06 ff 00 23 3e 3b ff ff 00 02 1f 03 00 08 00 00    ?..#>;...???.?..
> f0: 00 00 00 00 00 94 00 99 00 03 00 03 XX XX 01 00    .....?.?.?.?XX?.

Thank you.

Regards,

Hans

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

пн, 18 февр. 2019 г. в 12:24, Hans de Goede <hdegoede@redhat.com>:
>
> Hi,
>
> On 17-02-19 22:52, Yauhen Kharuzhy wrote:
> > On Fri, Feb 15, 2019 at 09:32:50AM +0300, Yauhen Kharuzhy wrote:
> >> On Thu, Feb 14, 2019 at 05:31:48PM +0100, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> >>>> In some configuration external charge "#charge enable" signal is
> >>>> connected to PMIC. Enable it at device probing to allow charging.
> >>>>
> >>>> Tested at Lenovo Yoga Book (YB1-X91L).
> >>>>
> >>>> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> >>>> ---
> >>>>    drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
> >>>>    1 file changed, 33 insertions(+)
> >>>>
> >>>> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> >>>> index 4f6ba249bc30..00cb3084955e 100644
> >>>> --- a/drivers/extcon/extcon-intel-cht-wc.c
> >>>> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> >>>> @@ -57,6 +57,11 @@
> >>>>    #define CHT_WC_USBSRC_TYPE_OTHER        8
> >>>>    #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY   9
> >>>> +#define CHT_WC_CHGDISCTRL         0x5e2f
> >>>> +#define CHT_WC_CHGDISCTRL_CCSM_DIS        0x11
> >>>> +#define CHT_WC_CHGDISCTRL_CCSM_EN 0x00
> >>>
> >>> Hmm, the enable mask here does not match the enable mask from:
> >>>
> >>> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> >>>
> >>> Which has:
> >>>
> >>> #define CHGDISFN_EN_CCSM_VAL           0x50
> >>> #define CHGDISFN_DIS_CCSM_VAL          0x11
> >>> #define CHGDISFN_CCSM_MASK             0x51
> >>>
> >>> Where as on my hardware, the PMIC comes up with 0x50
> >>> in the 0x5e2f register, exactly matching the values
> >>> from that patch.
> >>>
> >>> Why did you change this value ?
> >>
> >> Good question... I found this values in Lenovo's sources and use them
> >> 'as is':
> >> https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/intel_pmic_ccsm.h#L255
> >>
> >> I don't remember if charger worked with Intel's value, I need to
> >> re-check this.
> >
> > As we know now, value 0x50 meaning is:
> > - HW mode
> > - regular output type
> > - low level at output
> >
> > 0x00 meaning is:
> > - SW mode
> > - open-drain output
> > - low level at output
> >
> > I don't know what exactly "HW/SW mode" means but I suppose that this pin
> > can be controlled by PMIC internal charge state algorithm (if it is
> > enabled) or by software (extcon driver).
> >
> > So, if we set 0x50 value and disable HW-controlled charging entirely (as
> > extcon driver does), we don't set 'charger enable' signal to low as
> > expected. Its exactly state is unknown, but I checked – it is HIGH.
> > Charger doesn't start charging with this value and starts with 0x00.
> >
> > 0x0b register of 0x6b device on bus 7 – it is the status register of BQ25892
> > charger, where bits 3,4 describe charging state, 10b means fast charging,
> > 00b means not charging.
> >
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x00
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x16
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x01
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x06
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x10
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x16
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x11
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x06
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x50
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x06
> > root@yogabook:/home/jek# i2cset -y -f 6 0x5e 0x2f 0x51
> > root@yogabook:/home/jek# i2cget -y -f 7 0x6b 0x0b
> > 0x06
> >
> >
> > After reading of Intel's and Lenovo's sources i understood that Intel's
> > meaning of CHGDISFN_EN_CCSM_VAL is 'enable HW control of charging' and
> > Lenovo's is 'enable charging'. Lenovo set this value immediately after
> > probing and after resume, Intel – only after resume. HW-controlled
> > charging is disabled entirely at probing, so I don't know how Intel's
> > driver enables charging.
> >
> > So, I propose:
> >
> > 1) save initial value of CHGDIS register for restoring it at driver
> > remove (because the extcon-intel-cht-wc driver restores HW control in
> > cht_wc_extcon_remove()).
> > 2) at driver start, enable SW control of CHGDIS and set its value to 0.
> > 3) at driver removing, restore the saved state.
>
> This sounds good to me, note I believe the extcon code should not touch
> bit 4 (open-drain,vs regular), but since we disable the charger state-machine
> we should obviously then switch the pin to sw mode and drive the pin low
> so that the charger still works.


Agree.

>
>
> I believe that on the GPD win / GPD pocket the chgdis pin of the PMIC
> is not connected to the charger, since writing different values to reg
> 5e2f has no effect there.
>
> I do wonder how this interacts with inserting an otg-host cable into
> the micro-usb, I mean a cable like this:
>
> https://alexnld.com/wp-content/uploads/2013/11/S-UDC-103.jpg
>
> A cable like this will short the id-pin to ground and at this point
> we should disable the charger and enable a 5V boost converter to
> supply 5V to the device connected to the USB-A connector.
>
> On the GPD win / GPD pocket this is controlled through the Type-C
> framework and the V5 boost converter is actually part of the
> charger-IC, so charging gets disabled automatically when we tell
> the charger-IC to do enable its 5V boost converter.
>
> Do you already have an idea how to deal with this, it might be good
> to have at least an idea how we want to handle 5V boost before we
> merge the patch to control the CHGDIS pin, since we may need to
> disable the charger when the id-pin is connected to GND, so that
> the charger does not try to charge from the output of the 5V boost
> converter, which happens when an external 5v boost converter is used.

 I implemented this in external charger driver by sending extcon
notifications to it.
The BQ25892 charger provides boost converter for OTG and doesn't try to charge
from its own VBUS output. But additional disabling of charging via CHGDIS pin
shouldn't break anything.

> I also wonder if you've considered just disabling the extcon driver
> for the PMIC leaving it in automatic mode. Unlike the GPD win / pocket
> with their Type-C connector, your device seems to actually be using
> the PMIC as it was designed, so the automatic mode might just work
> and not touching the PMIC at all might be best.

Hm. We need to detect charger type (which can be kind of ACA) and set charging
limit properly, control OTG boost converter of the charger, request
hi-voltage charging etc.
I am not sure that this is possible without of software intervention.
Mixing of software
events handling with hardware charging control will be a source of
errors and instability.
So, no, I didn't think about HW-controlled charging when Linux is
running. But this may
be subject of future investigation.

> > Q: In theory, enabling of 'charge enable' output without of properly
> > configuration of external charger can cause some problems (USB overload,
> > battery overcurrent etc.). I think that there are no such stupidly
> > designed devices exist but we cannot be sure. What should we do with this?
>
> This should not be a problem, the input-current-limit of the charger
> will already be setup by the firmware at boot and if a charger gets
> plugged in later then the input-current-limit will reset to 500mA.
>
> Likewise the max charging current for the battery should already
> be configured properly by the firmware (this must be the case since
> the device will also charge while off) and we don't even know what
> the max charging current for the battery is, so we just have to rely
> on the firmware/BIOS here.

In the Lenovo Yoga Book the firmware seems to set safe input current limit
only (500 mA). Fast charging can be enabled by software and exactly value
of limits for this are known from Lenovo's sources only...


-- 
Yauhen Kharuzhy

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Hans de Goede]

Hi,

On 18-02-19 16:07, Yauhen Kharuzhy wrote:
> пн, 18 февр. 2019 г. в 12:24, Hans de Goede <hdegoede@redhat.com>:
<snip>

>>> So, I propose:
>>>
>>> 1) save initial value of CHGDIS register for restoring it at driver
>>> remove (because the extcon-intel-cht-wc driver restores HW control in
>>> cht_wc_extcon_remove()).
>>> 2) at driver start, enable SW control of CHGDIS and set its value to 0.
>>> 3) at driver removing, restore the saved state.
>>
>> This sounds good to me, note I believe the extcon code should not touch
>> bit 4 (open-drain,vs regular), but since we disable the charger state-machine
>> we should obviously then switch the pin to sw mode and drive the pin low
>> so that the charger still works.
> 
> 
> Agree.
> 
>>
>>
>> I believe that on the GPD win / GPD pocket the chgdis pin of the PMIC
>> is not connected to the charger, since writing different values to reg
>> 5e2f has no effect there.
>>
>> I do wonder how this interacts with inserting an otg-host cable into
>> the micro-usb, I mean a cable like this:
>>
>> https://alexnld.com/wp-content/uploads/2013/11/S-UDC-103.jpg
>>
>> A cable like this will short the id-pin to ground and at this point
>> we should disable the charger and enable a 5V boost converter to
>> supply 5V to the device connected to the USB-A connector.
>>
>> On the GPD win / GPD pocket this is controlled through the Type-C
>> framework and the V5 boost converter is actually part of the
>> charger-IC, so charging gets disabled automatically when we tell
>> the charger-IC to do enable its 5V boost converter.
>>
>> Do you already have an idea how to deal with this, it might be good
>> to have at least an idea how we want to handle 5V boost before we
>> merge the patch to control the CHGDIS pin, since we may need to
>> disable the charger when the id-pin is connected to GND, so that
>> the charger does not try to charge from the output of the 5V boost
>> converter, which happens when an external 5v boost converter is used.
> 
>   I implemented this in external charger driver by sending extcon
> notifications to it.
> The BQ25892 charger provides boost converter for OTG and doesn't try to charge
> from its own VBUS output. But additional disabling of charging via CHGDIS pin
> shouldn't break anything.

Ok.

>> I also wonder if you've considered just disabling the extcon driver
>> for the PMIC leaving it in automatic mode. Unlike the GPD win / pocket
>> with their Type-C connector, your device seems to actually be using
>> the PMIC as it was designed, so the automatic mode might just work
>> and not touching the PMIC at all might be best.
> 
> Hm. We need to detect charger type (which can be kind of ACA) and set charging
> limit properly, control OTG boost converter of the charger, request
> hi-voltage charging etc.
> I am not sure that this is possible without of software intervention.
> Mixing of software
> events handling with hardware charging control will be a source of
> errors and instability.
> So, no, I didn't think about HW-controlled charging when Linux is
> running. But this may
> be subject of future investigation.

Ok, I was hoping that the CCSM hardware would automatically do charger-type
detection and set the input-current-limit accordingly.

>>> Q: In theory, enabling of 'charge enable' output without of properly
>>> configuration of external charger can cause some problems (USB overload,
>>> battery overcurrent etc.). I think that there are no such stupidly
>>> designed devices exist but we cannot be sure. What should we do with this?
>>
>> This should not be a problem, the input-current-limit of the charger
>> will already be setup by the firmware at boot and if a charger gets
>> plugged in later then the input-current-limit will reset to 500mA.
>>
>> Likewise the max charging current for the battery should already
>> be configured properly by the firmware (this must be the case since
>> the device will also charge while off) and we don't even know what
>> the max charging current for the battery is, so we just have to rely
>> on the firmware/BIOS here.
> 
> In the Lenovo Yoga Book the firmware seems to set safe input current limit
> only (500 mA). Fast charging can be enabled by software and exactly value
> of limits for this are known from Lenovo's sources only...

The input-current-limit only specifies how much current the charger may
draw from the micro-usb for both supplying the laptop as well as for
charging the battery combined. You can safely set this as high as
the charger can handle (2A for a dedicated charger).

The BQ25892 should have a *separate* setting for the max current to
use while charging the battery (assuming that the input current allows
drawing enough current in the first place). I would hope that those bits
have some sane value set from the firmware...

ON the BQ24292i datasheet the charge current limit is in a register
called "REG02 Charge Current Control Register" and the bits are described
as controlling the "FAST CHARGE CURRENT LIMIT" aka ICHG.

Where as the input current limit is in "REG00 Input Source Control Register"
and the bits are described as "INPUT CURRENT LIMIT" ala INLIM.

Regards,

Hans

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

On Tue, Feb 19, 2019 at 02:39:55PM +0100, Hans de Goede wrote:
> > > I also wonder if you've considered just disabling the extcon driver
> > > for the PMIC leaving it in automatic mode. Unlike the GPD win / pocket
> > > with their Type-C connector, your device seems to actually be using
> > > the PMIC as it was designed, so the automatic mode might just work
> > > and not touching the PMIC at all might be best.
> > 
> > Hm. We need to detect charger type (which can be kind of ACA) and set charging
> > limit properly, control OTG boost converter of the charger, request
> > hi-voltage charging etc.
> > I am not sure that this is possible without of software intervention.
> > Mixing of software
> > events handling with hardware charging control will be a source of
> > errors and instability.
> > So, no, I didn't think about HW-controlled charging when Linux is
> > running. But this may
> > be subject of future investigation.
> 
> Ok, I was hoping that the CCSM hardware would automatically do charger-type
> detection and set the input-current-limit accordingly.

I have checked this. UEFI firmware configures PMIC to SW-controlled
mode if no OTG cable connected at start. If change PMIC mode to
HW-controlled, charging works but no IINLIM control by PMIC is observed
(ILIM pin is disabled and 0x00 register value is not changed). When OTG
cable is connected, bq25892 registers are not changed also (OTG boost
converter is disabled).

So, I consider that this machine is designed for software charging and
OTG mode control.


> > > > Q: In theory, enabling of 'charge enable' output without of properly
> > > > configuration of external charger can cause some problems (USB overload,
> > > > battery overcurrent etc.). I think that there are no such stupidly
> > > > designed devices exist but we cannot be sure. What should we do with this?
> > > 
> > > This should not be a problem, the input-current-limit of the charger
> > > will already be setup by the firmware at boot and if a charger gets
> > > plugged in later then the input-current-limit will reset to 500mA.
> > > 
> > > Likewise the max charging current for the battery should already
> > > be configured properly by the firmware (this must be the case since
> > > the device will also charge while off) and we don't even know what
> > > the max charging current for the battery is, so we just have to rely
> > > on the firmware/BIOS here.
> > 
> > In the Lenovo Yoga Book the firmware seems to set safe input current limit
> > only (500 mA). Fast charging can be enabled by software and exactly value
> > of limits for this are known from Lenovo's sources only...
> 
> The input-current-limit only specifies how much current the charger may
> draw from the micro-usb for both supplying the laptop as well as for
> charging the battery combined. You can safely set this as high as
> the charger can handle (2A for a dedicated charger).
> 
> The BQ25892 should have a *separate* setting for the max current to
> use while charging the battery (assuming that the input current allows
> drawing enough current in the first place). I would hope that those bits
> have some sane value set from the firmware...

Yes, the charger has separate battery current limit but firmware doesn't
change its default value (2048 mA) while Lenovo's software driver does.
It set battery charging limit to 4 A and input limit to 2 A (it makes
sense because Lenovo adapter and BQ25892 both support voltage increasing
upto 12V).


-- 
Yauhen Kharuzhy

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Hans de Goede]

Hi,

On 2/19/19 9:20 PM, Yauhen Kharuzhy wrote:
> On Tue, Feb 19, 2019 at 02:39:55PM +0100, Hans de Goede wrote:
>>>> I also wonder if you've considered just disabling the extcon driver
>>>> for the PMIC leaving it in automatic mode. Unlike the GPD win / pocket
>>>> with their Type-C connector, your device seems to actually be using
>>>> the PMIC as it was designed, so the automatic mode might just work
>>>> and not touching the PMIC at all might be best.
>>>
>>> Hm. We need to detect charger type (which can be kind of ACA) and set charging
>>> limit properly, control OTG boost converter of the charger, request
>>> hi-voltage charging etc.
>>> I am not sure that this is possible without of software intervention.
>>> Mixing of software
>>> events handling with hardware charging control will be a source of
>>> errors and instability.
>>> So, no, I didn't think about HW-controlled charging when Linux is
>>> running. But this may
>>> be subject of future investigation.
>>
>> Ok, I was hoping that the CCSM hardware would automatically do charger-type
>> detection and set the input-current-limit accordingly.
> 
> I have checked this. UEFI firmware configures PMIC to SW-controlled
> mode if no OTG cable connected at start. If change PMIC mode to
> HW-controlled, charging works but no IINLIM control by PMIC is observed
> (ILIM pin is disabled and 0x00 register value is not changed). When OTG
> cable is connected, bq25892 registers are not changed also (OTG boost
> converter is disabled).
> 
> So, I consider that this machine is designed for software charging and
> OTG mode control.

Ok, that is similar to my experience with the GPD devices with
TypeC connector.

>>>>> Q: In theory, enabling of 'charge enable' output without of properly
>>>>> configuration of external charger can cause some problems (USB overload,
>>>>> battery overcurrent etc.). I think that there are no such stupidly
>>>>> designed devices exist but we cannot be sure. What should we do with this?
>>>>
>>>> This should not be a problem, the input-current-limit of the charger
>>>> will already be setup by the firmware at boot and if a charger gets
>>>> plugged in later then the input-current-limit will reset to 500mA.
>>>>
>>>> Likewise the max charging current for the battery should already
>>>> be configured properly by the firmware (this must be the case since
>>>> the device will also charge while off) and we don't even know what
>>>> the max charging current for the battery is, so we just have to rely
>>>> on the firmware/BIOS here.
>>>
>>> In the Lenovo Yoga Book the firmware seems to set safe input current limit
>>> only (500 mA). Fast charging can be enabled by software and exactly value
>>> of limits for this are known from Lenovo's sources only...
>>
>> The input-current-limit only specifies how much current the charger may
>> draw from the micro-usb for both supplying the laptop as well as for
>> charging the battery combined. You can safely set this as high as
>> the charger can handle (2A for a dedicated charger).
>>
>> The BQ25892 should have a *separate* setting for the max current to
>> use while charging the battery (assuming that the input current allows
>> drawing enough current in the first place). I would hope that those bits
>> have some sane value set from the firmware...
> 
> Yes, the charger has separate battery current limit but firmware doesn't
> change its default value (2048 mA) while Lenovo's software driver does.
> It set battery charging limit to 4 A and input limit to 2 A (it makes
> sense because Lenovo adapter and BQ25892 both support voltage increasing
> upto 12V).

Hmm, I guess your device uses a separate power-barrel charging conector
then? 12v over micro-usb requires special negotiation which the Whiskey Cove
PMIC does not support AFAIK.

In either case if you want to increase the max battery current to 4A
in the kernel, then this will have to be guarded by a DMI check.

I beieve the way to do this wuld be throuh a device-property on the
charger which gets set from drivers/i2c/busses/i2c-cht-wc.c, but as
said this needs to be behind a DMI check, e cannot just g and boost
the max charge current to 4A everywhere.

Regards,

Hans

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Yauhen Kharuzhy]

On Wed, Feb 20, 2019 at 05:42:28PM +0100, Hans de Goede wrote:
> > > The input-current-limit only specifies how much current the charger may
> > > draw from the micro-usb for both supplying the laptop as well as for
> > > charging the battery combined. You can safely set this as high as
> > > the charger can handle (2A for a dedicated charger).
> > > 
> > > The BQ25892 should have a *separate* setting for the max current to
> > > use while charging the battery (assuming that the input current allows
> > > drawing enough current in the first place). I would hope that those bits
> > > have some sane value set from the firmware...
> > 
> > Yes, the charger has separate battery current limit but firmware doesn't
> > change its default value (2048 mA) while Lenovo's software driver does.
> > It set battery charging limit to 4 A and input limit to 2 A (it makes
> > sense because Lenovo adapter and BQ25892 both support voltage increasing
> > upto 12V).
> 
> Hmm, I guess your device uses a separate power-barrel charging conector
> then? 12v over micro-usb requires special negotiation which the Whiskey Cove
> PMIC does not support AFAIK.

The Yoga Book supports kind of quick charging by negotiate voltage with
'current pulse protocol' supported by BQ25892 and Lenovo's wall cube.
I think that PMIC is not connected to VBUS directly. After charging
started, BQ25892 driver can ask power adapter to increase voltage
number of times upto 12V. See
https://github.com/jekhor/yogabook-linux-kernel/blob/master/drivers/power/supply/bq25890_charger.c#L918
for example.

> 
> In either case if you want to increase the max battery current to 4A
> in the kernel, then this will have to be guarded by a DMI check.

Yes.

> 
> I beieve the way to do this wuld be throuh a device-property on the
> charger which gets set from drivers/i2c/busses/i2c-cht-wc.c, but as
> said this needs to be behind a DMI check, e cannot just g and boost
> the max charge current to 4A everywhere.

Yes, I use such method already in the my kernel:
https://github.com/jekhor/yogabook-linux-kernel/blob/master/drivers/i2c/busses/i2c-cht-wc.c#L247

Anyway, charger tweaking will be next iteration, now I want to complete
current extcon stuff/

> Regards,
> 
> Hans

-- 
Yauhen Kharuzhy

Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger

[Hans de Goede]

Hi,

On 20-02-19 21:28, Yauhen Kharuzhy wrote:
> On Wed, Feb 20, 2019 at 05:42:28PM +0100, Hans de Goede wrote:
>>>> The input-current-limit only specifies how much current the charger may
>>>> draw from the micro-usb for both supplying the laptop as well as for
>>>> charging the battery combined. You can safely set this as high as
>>>> the charger can handle (2A for a dedicated charger).
>>>>
>>>> The BQ25892 should have a *separate* setting for the max current to
>>>> use while charging the battery (assuming that the input current allows
>>>> drawing enough current in the first place). I would hope that those bits
>>>> have some sane value set from the firmware...
>>>
>>> Yes, the charger has separate battery current limit but firmware doesn't
>>> change its default value (2048 mA) while Lenovo's software driver does.
>>> It set battery charging limit to 4 A and input limit to 2 A (it makes
>>> sense because Lenovo adapter and BQ25892 both support voltage increasing
>>> upto 12V).
>>
>> Hmm, I guess your device uses a separate power-barrel charging conector
>> then? 12v over micro-usb requires special negotiation which the Whiskey Cove
>> PMIC does not support AFAIK.
> 
> The Yoga Book supports kind of quick charging by negotiate voltage with
> 'current pulse protocol' supported by BQ25892 and Lenovo's wall cube.
> I think that PMIC is not connected to VBUS directly. After charging
> started, BQ25892 driver can ask power adapter to increase voltage
> number of times upto 12V. See
> https://github.com/jekhor/yogabook-linux-kernel/blob/master/drivers/power/supply/bq25890_charger.c#L918
> for example.
> 
>>
>> In either case if you want to increase the max battery current to 4A
>> in the kernel, then this will have to be guarded by a DMI check.
> 
> Yes.
> 
>>
>> I beieve the way to do this wuld be throuh a device-property on the
>> charger which gets set from drivers/i2c/busses/i2c-cht-wc.c, but as
>> said this needs to be behind a DMI check, e cannot just g and boost
>> the max charge current to 4A everywhere.
> 
> Yes, I use such method already in the my kernel:
> https://github.com/jekhor/yogabook-linux-kernel/blob/master/drivers/i2c/busses/i2c-cht-wc.c#L247
> 
> Anyway, charger tweaking will be next iteration, now I want to complete
> current extcon stuff/

Ok, sounds good.

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Hans de Goede]

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
> based with Whiskey Cove PMIC), two pitfalls were found:
> 
> - for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
>    should be set in 0 (and set to 1 for Host mode). Pick up its definition
>    and logic from from Intel code drop[1];
> 
> - "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
>    connected to one of PMIC outputs controlled by CHGDISCTRL register.
>    Enable charging at driver initialization. Pick up this from Lenovo's code
>    drop[2,3].
> 
> Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
> are based on some kind of reverse engineering and suppositions, correct me if
> this semantic is wrong for common case.
> 
> [1]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
> [2]. https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
> [3]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch

Thank you for these patches, besides your Lenovo Yoga Book I'm aware of
only 2 other device models using the CHT Whiskey Cove PMIC, the GPD win
and GPD pocket devices. These both work fine without the changes your
patches introduce.

I need to check if your changes do not cause regressions on these 2
devices, which are used with Linux by quite a few people. I will try
to make some time for testing this sometime next week.

A kind request to the platform-x86 driver maintainers (hi Andy): Please
do not apply these patches until I've been able to test they don't cause
issues elsewhere.

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Hans de Goede]

Hi,

On 14-02-19 00:00, Hans de Goede wrote:
> Hi,
> 
> On 10-02-19 21:36, Yauhen Kharuzhy wrote:
>> At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
>> based with Whiskey Cove PMIC), two pitfalls were found:
>>
>> - for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
>>    should be set in 0 (and set to 1 for Host mode). Pick up its definition
>>    and logic from from Intel code drop[1];
>>
>> - "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
>>    connected to one of PMIC outputs controlled by CHGDISCTRL register.
>>    Enable charging at driver initialization. Pick up this from Lenovo's code
>>    drop[2,3].
>>
>> Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
>> are based on some kind of reverse engineering and suppositions, correct me if
>> this semantic is wrong for common case.
>>
>> [1]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
>> [2]. https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
>> [3]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> 
> Thank you for these patches, besides your Lenovo Yoga Book I'm aware of
> only 2 other device models using the CHT Whiskey Cove PMIC, the GPD win
> and GPD pocket devices. These both work fine without the changes your
> patches introduce.
> 
> I need to check if your changes do not cause regressions on these 2
> devices, which are used with Linux by quite a few people. I will try
> to make some time for testing this sometime next week.
> 
> A kind request to the platform-x86 driver maintainers (hi Andy): Please
> do not apply these patches until I've been able to test they don't cause
> issues elsewhere.

Erm, I just realized these are note platform-x86 driver patches at all.

Anyways same request to the extcon maintainers, please do not apply this
until I've had a chance to test this (which I'm doing right now).

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Andy Shevchenko]

On Thu, Feb 14, 2019 at 12:00:44AM +0100, Hans de Goede wrote:
> On 10-02-19 21:36, Yauhen Kharuzhy wrote:

> A kind request to the platform-x86 driver maintainers (hi Andy): Please
> do not apply these patches until I've been able to test they don't cause
> issues elsewhere.

Yes, that's my plan from the day one.

I also asked Yauhen to keep you in Cc list for all patches regarding the
platform he is enabling. On his GH page you may find, btw, a pile of patches.
I hope we will not get a patch bomb at once.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Hans de Goede]

Hi,

On 14-02-19 15:15, Yauhen Kharuzhy wrote:
> 
> 
> чц, 14 лют 2019, 15.47: карыстальнік Andy Shevchenko <andriy.shevchenko@linux.intel.com <mailto:andriy.shevchenko@linux.intel.com>> напісаў:
> 
>     On Thu, Feb 14, 2019 at 12:00:44AM +0100, Hans de Goede wrote:
>      > On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> 
>      > A kind request to the platform-x86 driver maintainers (hi Andy): Please
>      > do not apply these patches until I've been able to test they don't cause
>      > issues elsewhere.
> 
>     Yes, that's my plan from the day one.
> 
>     I also asked Yauhen to keep you in Cc list for all patches regarding the
>     platform he is enabling. On his GH page you may find, btw, a pile of patches.
>     I hope we will not get a patch bomb at once.
> 
> 
> 
> Yes, I plan to propose remained patches only after discussing and accepting already sent series and some reworking.
> 
> 
> The charger-related part will be very discussable.

Yes I just took a look at the patches from your kernel-tree at github,
it seems this is another quite "interesting" Cherry Trail device.

Oh if only the engineers who designed these had just use ACPI as intended
instead of doing a bunch of spaghetti code and duck-taping it all together
with proprietary / vendor-specific ACPI opregions. Ah well.

Note I see that your DSDT does not have any *valid* ACPI battery device
(PNP0C0A dev), so we need to directly talk to the fuel-gauge. I also see
that you already have some WIP code for this, good.

Taking a quick peek I also noticed the changes you did for the
drivers/i2c/busses/i2c-cht-wc.c code instantiating the charger device.

I think it would be better to instead of using DMI matching, to
actually probe which device is there, you can create a dummy
client on the adapter after the i2c_add_adapter call using:
i2c_new_dummy() and then you can do an smbus byte read from
register 0x14, on the bq24292i which the other devices with a wcove
pmic have you will get 0xff then since the addresses on the
bq24292i only go up to 0x0a and on the bq2589x your device has
you should then actually be able to check the device id you expect.

If you do this, please also read and check the 0x0a device-id register
of the bq24292i if the 0x14 check fails, I can test this. For the
bq24292i the expectation is for bits 3-5 to encode the value 3 (011).

If you go this route, I would also advice to change the:

if (acpi_dev_present("INT33FE", NULL, -1)) {
	....
}

To:

if (!acpi_dev_present("INT33FE", NULL, -1))
	goto done;

So that you don't get a too deep indentation level, making
the end result look something like this:


if (!acpi_dev_present("INT33FE", NULL, -1))
	goto done;

test_client = i2c_new_dummy(&adap->adapter, 0x14);
// test for bq25892 or bq24292i
i2c_unregister_device(test_client);
// register correct device, this must be done after
// unregister-ing the dummy to avoid an EBUSY error on the address

done:
platform_set_drvdata(pdev, adap);
return 0;

###

I would do something similar with the fuel-gauge in
drivers/platform/x86/intel_cht_int33fe.c, one option would
be to simply count the number of resources in the ACPI
resource table for the INT33FE device, versions with
the Type-C port have 7 resources, where as your INT33FE
device has only 3.

I'm even thinking that it might be best to rename
intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
a check for the resource table having 7 entries there, then
you can make a intel_cht_int33fe_micro_usb.c copy and strip
that mostly empty. Both would bind to the same "INT33FE"
id and they would both silently bail with -ENODEV if the
resource-count (or the PTYP value) don't match.

The reason I'm thinking of having 2 drivers is because
the current intel_cht_int33fe.c is quite special / ugly and
already has enough ifs.

If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
hopefully be much simpler.

Andy what is your take on having separate intel_cht_int33fe_typec.c
and intel_cht_int33fe_micro_usb.c drivers, both binding to
the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?

Having 2 drivers bind to the same id and exit silently with -ENODEV
is somewhat normal for USB ids where we also sometimes have these
kinda ID clashes with different devices hiding behind the same id.

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Yauhen Kharuzhy]

On Thu, Feb 14, 2019 at 04:05:26PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-02-19 15:15, Yauhen Kharuzhy wrote:
> > 
> > 
> > чц, 14 лют 2019, 15.47: карыстальнік Andy Shevchenko <andriy.shevchenko@linux.intel.com <mailto:andriy.shevchenko@linux.intel.com>> напісаў:
> > 
> >     On Thu, Feb 14, 2019 at 12:00:44AM +0100, Hans de Goede wrote:
> >      > On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> > 
> >      > A kind request to the platform-x86 driver maintainers (hi Andy): Please
> >      > do not apply these patches until I've been able to test they don't cause
> >      > issues elsewhere.
> > 
> >     Yes, that's my plan from the day one.
> > 
> >     I also asked Yauhen to keep you in Cc list for all patches regarding the
> >     platform he is enabling. On his GH page you may find, btw, a pile of patches.
> >     I hope we will not get a patch bomb at once.
> > 
> > 
> > 
> > Yes, I plan to propose remained patches only after discussing and accepting already sent series and some reworking.
> > 
> > 
> > The charger-related part will be very discussable.
> 
> Yes I just took a look at the patches from your kernel-tree at github,
> it seems this is another quite "interesting" Cherry Trail device.
> 
> Oh if only the engineers who designed these had just use ACPI as intended
> instead of doing a bunch of spaghetti code and duck-taping it all together
> with proprietary / vendor-specific ACPI opregions. Ah well.
> 
> Note I see that your DSDT does not have any *valid* ACPI battery device
> (PNP0C0A dev), so we need to directly talk to the fuel-gauge. I also see
> that you already have some WIP code for this, good.
> 
> Taking a quick peek I also noticed the changes you did for the
> drivers/i2c/busses/i2c-cht-wc.c code instantiating the charger device.
> 
> I think it would be better to instead of using DMI matching, to
> actually probe which device is there, you can create a dummy
> client on the adapter after the i2c_add_adapter call using:
> i2c_new_dummy() and then you can do an smbus byte read from
> register 0x14, on the bq24292i which the other devices with a wcove
> pmic have you will get 0xff then since the addresses on the
> bq24292i only go up to 0x0a and on the bq2589x your device has
> you should then actually be able to check the device id you expect.
> 
> If you do this, please also read and check the 0x0a device-id register
> of the bq24292i if the 0x14 check fails, I can test this. For the
> bq24292i the expectation is for bits 3-5 to encode the value 3 (011).

Sounds reasonable. I don't like approach when we store information about
I2C devices inside of I2C bus driver but this kind of autodetection
seems to be lesser evil than a DMI matching.

> 
> If you go this route, I would also advice to change the:
> 
> if (acpi_dev_present("INT33FE", NULL, -1)) {
> 	....
> }
> 
> To:
> 
> if (!acpi_dev_present("INT33FE", NULL, -1))
> 	goto done;
> 
> So that you don't get a too deep indentation level, making
> the end result look something like this:
> 
> 
> if (!acpi_dev_present("INT33FE", NULL, -1))
> 	goto done;
> 
> test_client = i2c_new_dummy(&adap->adapter, 0x14);
> // test for bq25892 or bq24292i
> i2c_unregister_device(test_client);
> // register correct device, this must be done after
> // unregister-ing the dummy to avoid an EBUSY error on the address
> 
> done:
> platform_set_drvdata(pdev, adap);
> return 0;
> 
> ###
> 
> I would do something similar with the fuel-gauge in
> drivers/platform/x86/intel_cht_int33fe.c, one option would
> be to simply count the number of resources in the ACPI
> resource table for the INT33FE device, versions with
> the Type-C port have 7 resources, where as your INT33FE
> device has only 3.
> 
> I'm even thinking that it might be best to rename
> intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
> a check for the resource table having 7 entries there, then
> you can make a intel_cht_int33fe_micro_usb.c copy and strip
> that mostly empty. Both would bind to the same "INT33FE"
> id and they would both silently bail with -ENODEV if the
> resource-count (or the PTYP value) don't match.
> 
> The reason I'm thinking of having 2 drivers is because
> the current intel_cht_int33fe.c is quite special / ugly and
> already has enough ifs.
> 
> If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
> hopefully be much simpler.
> 
> Andy what is your take on having separate intel_cht_int33fe_typec.c
> and intel_cht_int33fe_micro_usb.c drivers, both binding to
> the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?
> 
> Having 2 drivers bind to the same id and exit silently with -ENODEV
> is somewhat normal for USB ids where we also sometimes have these
> kinda ID clashes with different devices hiding behind the same id.

Hmm... And we need to handle case when all three INT33FE devices are
enabled in the DSDT...

Instead of separating of driver to two (and more when we will find new
CHT device...) I think about some kind of configuration variants table
with selection by PTYP/resource count/etc. But typeC devices will
require to define interconnections for example and we will get yet
another hardly readable code with quirks, autodetection magic and big
constant tables...

OK, I plan to start to make some experiments with this at weekend.

-- 
Yauhen Kharuzhy

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Hans de Goede]

Hi,

On 15-02-19 08:01, Yauhen Kharuzhy wrote:
> On Thu, Feb 14, 2019 at 04:05:26PM +0100, Hans de Goede wrote:

<snip>

>> I would do something similar with the fuel-gauge in
>> drivers/platform/x86/intel_cht_int33fe.c, one option would
>> be to simply count the number of resources in the ACPI
>> resource table for the INT33FE device, versions with
>> the Type-C port have 7 resources, where as your INT33FE
>> device has only 3.
>>
>> I'm even thinking that it might be best to rename
>> intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
>> a check for the resource table having 7 entries there, then
>> you can make a intel_cht_int33fe_micro_usb.c copy and strip
>> that mostly empty. Both would bind to the same "INT33FE"
>> id and they would both silently bail with -ENODEV if the
>> resource-count (or the PTYP value) don't match.
>>
>> The reason I'm thinking of having 2 drivers is because
>> the current intel_cht_int33fe.c is quite special / ugly and
>> already has enough ifs.
>>
>> If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
>> hopefully be much simpler.
>>
>> Andy what is your take on having separate intel_cht_int33fe_typec.c
>> and intel_cht_int33fe_micro_usb.c drivers, both binding to
>> the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?
>>
>> Having 2 drivers bind to the same id and exit silently with -ENODEV
>> is somewhat normal for USB ids where we also sometimes have these
>> kinda ID clashes with different devices hiding behind the same id.
> 
> Hmm... And we need to handle case when all three INT33FE devices are
> enabled in the DSDT...

I would not worry about that, I've never seen that and even then
counting the resources + the PTYP check should catch this.

> Instead of separating of driver to two (and more when we will find new
> CHT device...) I think about some kind of configuration variants table
> with selection by PTYP/resource count/etc. But typeC devices will
> require to define interconnections for example and we will get yet
> another hardly readable code with quirks, autodetection magic and big
> constant tables...

Right, the issue is the code for the variant with the Type-C connector
is just quite ugly, also with the cht_int33fe_check_for_max17047 firmware
bug workaround, I would prefer to keep that isolated to a single
file for just the devices which need all these kludges.

The version for your device should be much cleaner and if we need
another variant we can maybe make your version be a generic version
for this with some config table, but adding more special casing to
the current code is not a good idea IMHO.

> OK, I plan to start to make some experiments with this at weekend.

Sounds good.

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Andy Shevchenko]

On Fri, Feb 15, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 14-02-19 15:15, Yauhen Kharuzhy wrote:

> I would do something similar with the fuel-gauge in
> drivers/platform/x86/intel_cht_int33fe.c, one option would
> be to simply count the number of resources in the ACPI
> resource table for the INT33FE device, versions with
> the Type-C port have 7 resources, where as your INT33FE
> device has only 3.
>
> I'm even thinking that it might be best to rename
> intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
> a check for the resource table having 7 entries there, then
> you can make a intel_cht_int33fe_micro_usb.c copy and strip
> that mostly empty. Both would bind to the same "INT33FE"
> id and they would both silently bail with -ENODEV if the
> resource-count (or the PTYP value) don't match.
>
> The reason I'm thinking of having 2 drivers is because
> the current intel_cht_int33fe.c is quite special / ugly and
> already has enough ifs.
>
> If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
> hopefully be much simpler.
>
> Andy what is your take on having separate intel_cht_int33fe_typec.c
> and intel_cht_int33fe_micro_usb.c drivers, both binding to
> the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?

Depends on how code would look better, though I care about users that
they will not get additional Kconfig option and broken their
configurations when new piece of code landed up. So, from mine, as
user, prospective, we may split driver as we wish, but we should get
it working as previously for the existing cases.

> Having 2 drivers bind to the same id and exit silently with -ENODEV
> is somewhat normal for USB ids where we also sometimes have these
> kinda ID clashes with different devices hiding behind the same id.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Hans de Goede]

Hi,

On 15-02-19 10:29, Andy Shevchenko wrote:
> On Fri, Feb 15, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 14-02-19 15:15, Yauhen Kharuzhy wrote:
> 
>> I would do something similar with the fuel-gauge in
>> drivers/platform/x86/intel_cht_int33fe.c, one option would
>> be to simply count the number of resources in the ACPI
>> resource table for the INT33FE device, versions with
>> the Type-C port have 7 resources, where as your INT33FE
>> device has only 3.
>>
>> I'm even thinking that it might be best to rename
>> intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
>> a check for the resource table having 7 entries there, then
>> you can make a intel_cht_int33fe_micro_usb.c copy and strip
>> that mostly empty. Both would bind to the same "INT33FE"
>> id and they would both silently bail with -ENODEV if the
>> resource-count (or the PTYP value) don't match.
>>
>> The reason I'm thinking of having 2 drivers is because
>> the current intel_cht_int33fe.c is quite special / ugly and
>> already has enough ifs.
>>
>> If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
>> hopefully be much simpler.
>>
>> Andy what is your take on having separate intel_cht_int33fe_typec.c
>> and intel_cht_int33fe_micro_usb.c drivers, both binding to
>> the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?
> 
> Depends on how code would look better,

Well the existing drivers/platform/x86/intel_cht_int33fe.c file,
which already is full of kludges would not get even more code-paths
added; and the new file which Yauhen will wrote should be nice and
clean with only 1 straight code-path pretty much.

> though I care about users that
> they will not get additional Kconfig option and broken their
> configurations when new piece of code landed up. So, from mine, as
> user, prospective, we may split driver as we wish, but we should get
> it working as previously for the existing cases.

That is a valid point, I'm not a fan of having even more Kconfig
options either, so we can simply enable/disable both modules through
the same Kconfig option.

Regards,

Hans

Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

[Chanwoo Choi]

Hi Yauhen,

You are missing me on cc list. I didn't know this patchset.
On next, please use the get_maintainer script in order to
send the correct list.

On 19. 2. 11. 오전 5:36, Yauhen Kharuzhy wrote:
> At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
> based with Whiskey Cove PMIC), two pitfalls were found:
> 
> - for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
>   should be set in 0 (and set to 1 for Host mode). Pick up its definition
>   and logic from from Intel code drop[1];
> 
> - "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
>   connected to one of PMIC outputs controlled by CHGDISCTRL register.
>   Enable charging at driver initialization. Pick up this from Lenovo's code
>   drop[2,3].
> 
> Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
> are based on some kind of reverse engineering and suppositions, correct me if
> this semantic is wrong for common case.
> 
> [1]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
> [2]. https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
> [3]. https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> 
> Yauhen Kharuzhy (2):
>   extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode
>   extcon intel-cht-wc: Enable external charger
> 
>  drivers/extcon/extcon-intel-cht-wc.c | 71 +++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 1 deletion(-)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics