LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Yauhen Kharuzhy <jekhor@gmail.com>, linux-kernel@vger.kernel.org
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger
Date: Wed, 20 Feb 2019 16:53:50 +0100	[thread overview]
Message-ID: <19f27109-434d-3e5e-c895-fdec0324572f@redhat.com> (raw)
In-Reply-To: <20190219212441.19391-3-jekhor@gmail.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.

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

  parent reply	other threads:[~2019-02-20 15:53 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-19 21:24 [PATCH v2 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks Yauhen Kharuzhy
2019-02-19 21:24 ` [PATCH v2 1/2] extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode Yauhen Kharuzhy
2019-02-20 12:42   ` Andy Shevchenko
2019-02-20 20:46     ` Yauhen Kharuzhy
2019-02-19 21:24 ` [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger Yauhen Kharuzhy
2019-02-20 13:08   ` Andy Shevchenko
2019-02-20 15:53   ` Hans de Goede [this message]
2019-02-20 21:24     ` Yauhen Kharuzhy
2019-02-22  9:18       ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=19f27109-434d-3e5e-c895-fdec0324572f@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=cw00.choi@samsung.com \
    --cc=jekhor@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=myungjoo.ham@samsung.com \
    --subject='Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).