Re: [PATCH v2 2/2] extcon intel-cht-wc: Enable external charger

From: Hans de Goede
Date: Fri Feb 22 2019 - 04:18:15 EST


Hi,

On 20-02-19 22:24, Yauhen Kharuzhy wrote:
ÑÑ, 20 ÑÐÐÑ. 2019 Ð. Ð 18:53, Hans de Goede <hdegoede@xxxxxxxxxx>:

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