Messages in this thread | | | Subject | Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger | From | Hans de Goede <> | Date | Wed, 20 Feb 2019 16:53:50 +0100 |
| |
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
| |