| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Feb 2019 16:53:50 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Yauhen Kharuzhy <jekhor@...il.com>, linux-kernel@...r.kernel.org
Cc: MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger
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@...il.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
Powered by blists - more mailing lists