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

[PATCH v2 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].

v2 changes:
- Disable HW control mode of CHGDISCTRL at driver probing and restore
  initial state at exit.
- Switch CE output off if OTG host mode is enabled.
- Save and restore CHGRCTRL0 register also.

[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 | 129 ++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH v2 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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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 v2 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 12:24:40AM +0300, 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.

Some minor comments below.

Otherwise,

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> -#define CHT_WC_CHGRCTRL1		0x5e17
> +#define CHT_WC_CHGRCTRL1			0x5e17

Not related change?

> +#define CHT_WC_CHGRCTRL1_DBPEN_MASK		BIT(7)

Drop the _MASK, it's one bit anyway.

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

I think better to keep ascending order.

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

Redundant parens.

> +
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL1, chgrctrl1);
> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error writing CHGRCTRL1 OTG mode bit: %d\n", ret);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Yauhen Kharuzhy]

On Wed, Feb 20, 2019 at 02:42:06PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 20, 2019 at 12:24:40AM +0300, 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.
> 
> Some minor comments below.
> 
> Otherwise,
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > -#define CHT_WC_CHGRCTRL1		0x5e17
> > +#define CHT_WC_CHGRCTRL1			0x5e17
> 
> Not related change?

just alignment, yes.

> 
> > +#define CHT_WC_CHGRCTRL1_DBPEN_MASK		BIT(7)
> 
> Drop the _MASK, it's one bit anyway.
OK.

> 
> > +#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)
> 
> I think better to keep ascending order.

OK.

> 
> > +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);
> 
> Redundant parens.

Hmm... Why I didn't use regmap_update_bits() here... I will simplify this
piece with it.

Thanks!

-- 
Yauhen Kharuzhy

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

[Yauhen Kharuzhy]

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

Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
them at driver unbind to re-enable hardware charging control if it was
enabled before.

Tested at Lenovo Yoga Book (YB1-X91L).

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

diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
index 4f6ba249bc30..ac009929d244 100644
--- a/drivers/extcon/extcon-intel-cht-wc.c
+++ b/drivers/extcon/extcon-intel-cht-wc.c
@@ -57,6 +57,16 @@
 #define CHT_WC_USBSRC_TYPE_OTHER	8
 #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
 
+#define CHT_WC_CHGDISCTRL		0x5e2f
+#define CHT_WC_CHGDISCTRL_OUTPUT	BIT(0)
+/* 0 - open drain, 1 - regular output */
+#define CHT_WC_CHGDISCTRL_DRV_OD_DIS	BIT(4)
+#define CHT_WC_CHGDISCTRL_MODE_HW	BIT(6)
+
+#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
@@ -103,6 +113,8 @@ struct cht_wc_extcon_data {
 	struct regmap *regmap;
 	struct extcon_dev *edev;
 	unsigned int previous_cable;
+	unsigned int chgdisctrl_saved;
+	unsigned int chgrctrl0_saved;
 	bool usb_host;
 };
 
@@ -230,6 +242,20 @@ 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 val;
+	int ret;
+
+	val = enable ? 0 : CHT_WC_CHGDISCTRL_OUTPUT;
+
+	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
+				 CHT_WC_CHGDISCTRL_OUTPUT, val);
+	if (ret)
+		dev_err(ext->dev, "Error updating CHGDISCTRL reg: %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)
@@ -254,6 +280,7 @@ 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_enable_charging(ext, false);
 		cht_wc_extcon_set_otgmode(ext, true);
 
 		/* The 5v boost causes a false VBUS / SDP detect, skip */
@@ -261,6 +288,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
 	}
 
 	cht_wc_extcon_set_otgmode(ext, false);
+	cht_wc_extcon_enable_charging(ext, true);
 
 	/* Plugged into a host/charger or not connected? */
 	if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
@@ -314,6 +342,14 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
 {
 	int ret, mask, val;
 
+	val = enable ? 0 : CHT_WC_CHGDISCTRL_MODE_HW;
+	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
+			CHT_WC_CHGDISCTRL_MODE_HW, val);
+	if (ret)
+		dev_err(ext->dev,
+			"Error setting sw control for charger enable: %d\n",
+			ret);
+
 	mask = CHT_WC_CHGRCTRL0_SWCONTROL | CHT_WC_CHGRCTRL0_CCSM_OFF;
 	val = enable ? mask : 0;
 	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGRCTRL0, mask, val);
@@ -323,6 +359,52 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
 	return ret;
 }
 
+static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
+{
+	int ret;
+
+	/*
+	 * Save the external charger control output state for restoring it at
+	 * driver unbinding
+	 */
+	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
+			  &ext->chgdisctrl_saved);
+	if (ret) {
+		dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
+			  &ext->chgrctrl0_saved);
+	if (ret) {
+		dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cht_wc_restore_initial_state(struct cht_wc_extcon_data *ext)
+{
+	int ret;
+
+	ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL,
+			   ext->chgdisctrl_saved);
+	if (ret)
+		dev_err(ext->dev, "Error restoring of CHGDISCTRL reg: %d\n",
+			ret);
+
+	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL0,
+			   ext->chgrctrl0_saved);
+	if (ret)
+		dev_err(ext->dev, "Error restoring of CHGRCTRL0 reg: %d\n",
+			ret);
+
+	return ret;
+}
+
 static int cht_wc_extcon_probe(struct platform_device *pdev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
@@ -347,6 +429,8 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	if (IS_ERR(ext->edev))
 		return PTR_ERR(ext->edev);
 
+	cht_wc_save_initial_state(ext);
+
 	/*
 	 * When a host-cable is detected the BIOS enables an external 5v boost
 	 * converter to power connected devices there are 2 problems with this:
@@ -365,7 +449,10 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 	/* Enable sw control */
 	ret = cht_wc_extcon_sw_control(ext, true);
 	if (ret)
-		return ret;
+		goto disable_sw_control;
+
+	/* Disable charging by external battery charger */
+	cht_wc_extcon_enable_charging(ext, false);
 
 	/* Register extcon device */
 	ret = devm_extcon_dev_register(ext->dev, ext->edev);
@@ -400,6 +487,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
 
 disable_sw_control:
 	cht_wc_extcon_sw_control(ext, false);
+	cht_wc_restore_initial_state(ext);
 	return ret;
 }
 
@@ -408,6 +496,7 @@ static int cht_wc_extcon_remove(struct platform_device *pdev)
 	struct cht_wc_extcon_data *ext = platform_get_drvdata(pdev);
 
 	cht_wc_extcon_sw_control(ext, false);
+	cht_wc_restore_initial_state(ext);
 
 	return 0;
 }
-- 
2.20.1

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

[Andy Shevchenko]

On Wed, Feb 20, 2019 at 12:24:41AM +0300, Yauhen Kharuzhy wrote:
> In some configuration external charger "#charge enable" signal is
> connected to PMIC. Enable it at device probing to allow charging.
> 
> Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
> them at driver unbind to re-enable hardware charging control if it was
> enabled before.
> 
> Tested at Lenovo Yoga Book (YB1-X91L).

My comments below.
After addressing them,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> +#define CHT_WC_CHGDISCTRL_OUTPUT	BIT(0)

Simple _OUT as per documentation.

> +/* 0 - open drain, 1 - regular output */

Regular means push-pull.

> +#define CHT_WC_CHGDISCTRL_DRV_OD_DIS	BIT(4)

_DRV as per documentation.

> +#define CHT_WC_CHGDISCTRL_MODE_HW	BIT(6)

_FN as per documentation.

> +static void cht_wc_extcon_enable_charging(struct cht_wc_extcon_data *ext,
> +					  bool enable)
> +{
> +	unsigned int val;
> +	int ret;
> +

> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_OUTPUT;

Can be assigned in definition block.

> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +				 CHT_WC_CHGDISCTRL_OUTPUT, val);
> +	if (ret)
> +		dev_err(ext->dev, "Error updating CHGDISCTRL reg: %d\n", ret);
> +}

> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_MODE_HW;

> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +			CHT_WC_CHGDISCTRL_MODE_HW, val);

Indentation.

> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error setting sw control for charger enable: %d\n",
> +			ret);

> +static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
> +{
> +	int ret;
> +
> +	/*
> +	 * Save the external charger control output state for restoring it at
> +	 * driver unbinding

You may move "at" to the next line and add a period at the end.

> +	 */
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
> +			  &ext->chgdisctrl_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
> +			  &ext->chgrctrl0_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 2/19/19 10:24 PM, Yauhen Kharuzhy wrote:
> In some configuration external charger "#charge enable" signal is
> connected to PMIC. Enable it at device probing to allow charging.
> 
> Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
> them at driver unbind to re-enable hardware charging control if it was
> enabled before.
> 
> Tested at Lenovo Yoga Book (YB1-X91L).
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 91 +++++++++++++++++++++++++++-
>   1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 4f6ba249bc30..ac009929d244 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -57,6 +57,16 @@
>   #define CHT_WC_USBSRC_TYPE_OTHER	8
>   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
>   
> +#define CHT_WC_CHGDISCTRL		0x5e2f
> +#define CHT_WC_CHGDISCTRL_OUTPUT	BIT(0)
> +/* 0 - open drain, 1 - regular output */
> +#define CHT_WC_CHGDISCTRL_DRV_OD_DIS	BIT(4)
> +#define CHT_WC_CHGDISCTRL_MODE_HW	BIT(6)
> +
> +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
> +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00
> +#define CHT_WC_CHGDISCTRL_CCSM_MASK	0x51
> +

You no longer need the last 3 defines and IMHO keeping them
will only confuse the reader of the code, please drop these 3.

>   #define CHT_WC_PWRSRC_IRQ		0x6e03
>   #define CHT_WC_PWRSRC_IRQ_MASK		0x6e0f
>   #define CHT_WC_PWRSRC_STS		0x6e1e
> @@ -103,6 +113,8 @@ struct cht_wc_extcon_data {
>   	struct regmap *regmap;
>   	struct extcon_dev *edev;
>   	unsigned int previous_cable;
> +	unsigned int chgdisctrl_saved;
> +	unsigned int chgrctrl0_saved;
>   	bool usb_host;
>   };
>   
> @@ -230,6 +242,20 @@ 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 val;
> +	int ret;
> +
> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_OUTPUT;
> +
> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +				 CHT_WC_CHGDISCTRL_OUTPUT, val);
> +	if (ret)
> +		dev_err(ext->dev, "Error updating CHGDISCTRL reg: %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)
> @@ -254,6 +280,7 @@ 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_enable_charging(ext, false);
>   		cht_wc_extcon_set_otgmode(ext, true);
>   
>   		/* The 5v boost causes a false VBUS / SDP detect, skip */
> @@ -261,6 +288,7 @@ static void cht_wc_extcon_pwrsrc_event(struct cht_wc_extcon_data *ext)
>   	}
>   
>   	cht_wc_extcon_set_otgmode(ext, false);
> +	cht_wc_extcon_enable_charging(ext, true);
>   
>   	/* Plugged into a host/charger or not connected? */
>   	if (!(pwrsrc_sts & CHT_WC_PWRSRC_VBUS)) {
> @@ -314,6 +342,14 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
>   {
>   	int ret, mask, val;
>   
> +	val = enable ? 0 : CHT_WC_CHGDISCTRL_MODE_HW;
> +	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGDISCTRL,
> +			CHT_WC_CHGDISCTRL_MODE_HW, val);
> +	if (ret)
> +		dev_err(ext->dev,
> +			"Error setting sw control for charger enable: %d\n",
> +			ret);
> +
>   	mask = CHT_WC_CHGRCTRL0_SWCONTROL | CHT_WC_CHGRCTRL0_CCSM_OFF;
>   	val = enable ? mask : 0;
>   	ret = regmap_update_bits(ext->regmap, CHT_WC_CHGRCTRL0, mask, val);
> @@ -323,6 +359,52 @@ static int cht_wc_extcon_sw_control(struct cht_wc_extcon_data *ext, bool enable)
>   	return ret;
>   }
>   
> +static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
> +{
> +	int ret;
> +
> +	/*
> +	 * Save the external charger control output state for restoring it at
> +	 * driver unbinding
> +	 */
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
> +			  &ext->chgdisctrl_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
> +			  &ext->chgrctrl0_saved);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cht_wc_restore_initial_state(struct cht_wc_extcon_data *ext)
> +{
> +	int ret;
> +
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL,
> +			   ext->chgdisctrl_saved);
> +	if (ret)
> +		dev_err(ext->dev, "Error restoring of CHGDISCTRL reg: %d\n",
> +			ret);
> +
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL0,
> +			   ext->chgrctrl0_saved);
> +	if (ret)
> +		dev_err(ext->dev, "Error restoring of CHGRCTRL0 reg: %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +
>   static int cht_wc_extcon_probe(struct platform_device *pdev)
>   {
>   	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> @@ -347,6 +429,8 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	if (IS_ERR(ext->edev))
>   		return PTR_ERR(ext->edev);
>   
> +	cht_wc_save_initial_state(ext);
> +
>   	/*
>   	 * When a host-cable is detected the BIOS enables an external 5v boost
>   	 * converter to power connected devices there are 2 problems with this:
> @@ -365,7 +449,10 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	/* Enable sw control */
>   	ret = cht_wc_extcon_sw_control(ext, true);
>   	if (ret)
> -		return ret;
> +		goto disable_sw_control;
> +
> +	/* Disable charging by external battery charger */
> +	cht_wc_extcon_enable_charging(ext, false);
>   
>   	/* Register extcon device */
>   	ret = devm_extcon_dev_register(ext->dev, ext->edev);
> @@ -400,6 +487,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   
>   disable_sw_control:
>   	cht_wc_extcon_sw_control(ext, false);
> +	cht_wc_restore_initial_state(ext);

The restore_initial_state will clober al changes made by the
cht_wc_extcon_sw_control call, so we do not need both. Thinking a bit
more about this I think it might be best to drop the state save/restore
code and just enable hw-control on exit unconditionally. We cannot be
sure that te initial state is sane, so just switching to hw-control on
exit seem best and requires less code.  Andy, what is your take on this?

>   	return ret;
>   }
>   
> @@ -408,6 +496,7 @@ static int cht_wc_extcon_remove(struct platform_device *pdev)
>   	struct cht_wc_extcon_data *ext = platform_get_drvdata(pdev);
>   
>   	cht_wc_extcon_sw_control(ext, false);
> +	cht_wc_restore_initial_state(ext);

Idem / same as my prvious comment.

Regards,

Hans

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

[Yauhen Kharuzhy]

ср, 20 февр. 2019 г. в 18:53, Hans de Goede <hdegoede@redhat.com>:
>
> Hi,
>
> On 2/19/19 10:24 PM, Yauhen Kharuzhy wrote:
> > In some configuration external charger "#charge enable" signal is
> > connected to PMIC. Enable it at device probing to allow charging.
> >
> > Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
> > them at driver unbind to re-enable hardware charging control if it was
> > enabled before.
> >
> > Tested at Lenovo Yoga Book (YB1-X91L).
> >
> > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > ---
> >   drivers/extcon/extcon-intel-cht-wc.c | 91 +++++++++++++++++++++++++++-
> >   1 file changed, 90 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> > index 4f6ba249bc30..ac009929d244 100644
> > --- a/drivers/extcon/extcon-intel-cht-wc.c
> > +++ b/drivers/extcon/extcon-intel-cht-wc.c
> > @@ -57,6 +57,16 @@
> >   #define CHT_WC_USBSRC_TYPE_OTHER    8
> >   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY       9
> >
> > +#define CHT_WC_CHGDISCTRL            0x5e2f
> > +#define CHT_WC_CHGDISCTRL_OUTPUT     BIT(0)
> > +/* 0 - open drain, 1 - regular output */
> > +#define CHT_WC_CHGDISCTRL_DRV_OD_DIS BIT(4)
> > +#define CHT_WC_CHGDISCTRL_MODE_HW    BIT(6)
> > +
> > +#define CHT_WC_CHGDISCTRL_CCSM_DIS   0x11
> > +#define CHT_WC_CHGDISCTRL_CCSM_EN    0x00
> > +#define CHT_WC_CHGDISCTRL_CCSM_MASK  0x51
> > +
>
> You no longer need the last 3 defines and IMHO keeping them
> will only confuse the reader of the code, please drop these 3.

My mistake, thanks.

> > +static int cht_wc_save_initial_state(struct cht_wc_extcon_data *ext)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * Save the external charger control output state for restoring it at
> > +      * driver unbinding
> > +      */
> > +     ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL,
> > +                       &ext->chgdisctrl_saved);
> > +     if (ret) {
> > +             dev_err(ext->dev, "Error reading CHGDISCTRL: %d\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = regmap_read(ext->regmap, CHT_WC_CHGRCTRL0,
> > +                       &ext->chgrctrl0_saved);
> > +     if (ret) {
> > +             dev_err(ext->dev, "Error reading CHGRCTRL0: %d\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cht_wc_restore_initial_state(struct cht_wc_extcon_data *ext)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL,
> > +                        ext->chgdisctrl_saved);
> > +     if (ret)
> > +             dev_err(ext->dev, "Error restoring of CHGDISCTRL reg: %d\n",
> > +                     ret);
> > +
> > +     ret = regmap_write(ext->regmap, CHT_WC_CHGRCTRL0,
> > +                        ext->chgrctrl0_saved);
> > +     if (ret)
> > +             dev_err(ext->dev, "Error restoring of CHGRCTRL0 reg: %d\n",
> > +                     ret);
> > +
> > +     return ret;
> > +}
> > +
> >   static int cht_wc_extcon_probe(struct platform_device *pdev)
> >   {
> >       struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> > @@ -347,6 +429,8 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >       if (IS_ERR(ext->edev))
> >               return PTR_ERR(ext->edev);
> >
> > +     cht_wc_save_initial_state(ext);
> > +
> >       /*
> >        * When a host-cable is detected the BIOS enables an external 5v boost
> >        * converter to power connected devices there are 2 problems with this:
> > @@ -365,7 +449,10 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >       /* Enable sw control */
> >       ret = cht_wc_extcon_sw_control(ext, true);
> >       if (ret)
> > -             return ret;
> > +             goto disable_sw_control;
> > +
> > +     /* Disable charging by external battery charger */
> > +     cht_wc_extcon_enable_charging(ext, false);
> >
> >       /* Register extcon device */
> >       ret = devm_extcon_dev_register(ext->dev, ext->edev);
> > @@ -400,6 +487,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
> >
> >   disable_sw_control:
> >       cht_wc_extcon_sw_control(ext, false);
> > +     cht_wc_restore_initial_state(ext);
>
> The restore_initial_state will clober al changes made by the
> cht_wc_extcon_sw_control call, so we do not need both. Thinking a bit
> more about this I think it might be best to drop the state save/restore
> code and just enable hw-control on exit unconditionally. We cannot be
> sure that te initial state is sane, so just switching to hw-control on
> exit seem best and requires less code.  Andy, what is your take on this?

On the other hand we don't know if HW mode is suitable for given
hardware configuration.

We can suppose that if existing code with unconditionally switching to
HW mode didn't cause
any issues before than we can safely leave this for future discussions
and add CHGDISCTRL restoring only.

-- 
Yauhen Kharuzhy

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

[Hans de Goede]

Hi,

On 20-02-19 22:24, Yauhen Kharuzhy wrote:
> ср, 20 февр. 2019 г. в 18:53, Hans de Goede <hdegoede@redhat.com>:
>>
>> Hi,
>>
>> On 2/19/19 10:24 PM, Yauhen Kharuzhy wrote:
>>> In some configuration external charger "#charge enable" signal is
>>> connected to PMIC. Enable it at device probing to allow charging.
>>>
>>> Save CHGRCTRL0 and CHGDISCTR registers at driver probing and restore
>>> them at driver unbind to re-enable hardware charging control if it was
>>> enabled before.
>>>
>>> Tested at Lenovo Yoga Book (YB1-X91L).

<snip>

>>> @@ -400,6 +487,7 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>>>
>>>    disable_sw_control:
>>>        cht_wc_extcon_sw_control(ext, false);
>>> +     cht_wc_restore_initial_state(ext);
>>
>> The restore_initial_state will clober al changes made by the
>> cht_wc_extcon_sw_control call, so we do not need both. Thinking a bit
>> more about this I think it might be best to drop the state save/restore
>> code and just enable hw-control on exit unconditionally. We cannot be
>> sure that te initial state is sane, so just switching to hw-control on
>> exit seem best and requires less code.  Andy, what is your take on this?
> 
> On the other hand we don't know if HW mode is suitable for given
> hardware configuration.
> 
> We can suppose that if existing code with unconditionally switching to
> HW mode didn't cause
> any issues before than we can safely leave this for future discussions
> and add CHGDISCTRL restoring only.

The code would be a lot simpler if we also unconditionally put the chrgdis
pin back in hw control mode, then we only need your modifications to
cht_wc_extcon_sw_control() and we don't need the save / restore state
functions.

On my hardware with a whiskey cove PMIC the firmware comes up with 0x50 in
the CHGDISCTRL register, which means it is under hw control, so we would
normally end up restoring that. I believe your Yoga Book is similar, so
there really is no need for the save / restore state functions.

Regards,

Hans