Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] extcon intel-cht-wc: Enable external charger
To:     Yauhen Kharuzhy <jekhor@gmail.com>, linux-kernel@vger.kernel.org
Cc:     MyungJoo Ham <myungjoo.ham@samsung.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
References: <20190210203649.21691-1-jekhor@gmail.com>
 <20190210203649.21691-3-jekhor@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <1b2f04fc-05a0-4f09-c84e-dc7adc63ec63@redhat.com>
Date:   Thu, 14 Feb 2019 17:31:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
MIME-Version: 1.0
In-Reply-To: <20190210203649.21691-3-jekhor@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> In some configuration external charge "#charge enable" signal is
> connected to PMIC. Enable it at device probing to allow charging.
> 
> Tested at Lenovo Yoga Book (YB1-X91L).
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
>   drivers/extcon/extcon-intel-cht-wc.c | 33 ++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-intel-cht-wc.c b/drivers/extcon/extcon-intel-cht-wc.c
> index 4f6ba249bc30..00cb3084955e 100644
> --- a/drivers/extcon/extcon-intel-cht-wc.c
> +++ b/drivers/extcon/extcon-intel-cht-wc.c
> @@ -57,6 +57,11 @@
>   #define CHT_WC_USBSRC_TYPE_OTHER	8
>   #define CHT_WC_USBSRC_TYPE_DCP_EXTPHY	9
>   
> +#define CHT_WC_CHGDISCTRL		0x5e2f
> +#define CHT_WC_CHGDISCTRL_CCSM_DIS	0x11
> +#define CHT_WC_CHGDISCTRL_CCSM_EN	0x00

Hmm, the enable mask here does not match the enable mask from:

https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch

Which has:

#define CHGDISFN_EN_CCSM_VAL           0x50
#define CHGDISFN_DIS_CCSM_VAL          0x11
#define CHGDISFN_CCSM_MASK             0x51

Where as on my hardware, the PMIC comes up with 0x50
in the 0x5e2f register, exactly matching the values
from that patch.

Why did you change this value ?

It would be interesting to get a dump of the
charger's i2c registers using i2cdump before
and after writing this new value to register
0x5e2f. Note you can simply leave the patch adding
the write out of the kernel, and then do:

i2cdump <charger>
i2cget -y -f 6 0x5e 0x2f
# check 0x00 is the correct val
i2cset -y -f 6 0x5e 0x2f 0x00
i2cdump <charger>

To see if any reg values of the charger-ic have changed.

Note bus 6 is the right bus for the PMIC on my systems,
but it might be different, to find the right bus do:

ls -l /sys/bus/devices/i2c-INT34D3:00

And look at what the i2c bus-number is in that symlink.


Also can you please provide i2cdump output for the
0x5e and 0x6e devices of the PMIC, preferably from a
clean boot, without your patches, so that we can
see what the value of the 0x5e2f register is before
your code modifies it.

i2cdump -y -f 6 0x5e
i2cdump -y -f 6 0x6e

Regards,

Hans


> +#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
> @@ -230,6 +235,31 @@ 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 chgdisctrl;
> +	int ret;
> +
> +	ret = regmap_read(ext->regmap, CHT_WC_CHGDISCTRL, &chgdisctrl);
> +	if (ret) {
> +		dev_err(ext->dev, "Error reading CHGDISCTRL reg: %d\n", ret);
> +		return;
> +	}
> +
> +	chgdisctrl &= CHT_WC_CHGDISCTRL_CCSM_MASK;
> +
> +	if (enable)
> +		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_EN;
> +	else
> +		chgdisctrl |= CHT_WC_CHGDISCTRL_CCSM_DIS;
> +
> +	dev_dbg(ext->dev, "Writing CHGDISCTRL: 0x%02x\n", chgdisctrl);
> +	ret = regmap_write(ext->regmap, CHT_WC_CHGDISCTRL, chgdisctrl);
> +	if (ret)
> +		dev_err(ext->dev, "Error writing CHGDISCTRL: %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)
> @@ -362,6 +392,9 @@ static int cht_wc_extcon_probe(struct platform_device *pdev)
>   	 */
>   	cht_wc_extcon_set_5v_boost(ext, false);
>   
> +	/* Allow charging by external battery charger*/
> +	cht_wc_extcon_enable_charging(ext, true);
> +
>   	/* Enable sw control */
>   	ret = cht_wc_extcon_sw_control(ext, true);
>   	if (ret)
>