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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---
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
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 <[email protected]>
> -#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
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 <[email protected]>
> +#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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> > -#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
ср, 20 февр. 2019 г. в 18:53, Hans de Goede <[email protected]>:
>
> 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 <[email protected]>
> > ---
> > 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
Hi,
On 20-02-19 22:24, Yauhen Kharuzhy wrote:
> ср, 20 февр. 2019 г. в 18:53, Hans de Goede <[email protected]>:
>>
>> 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