Message-ID: <641a1738-c73d-0d9e-dde6-d43c74202bd1@redhat.com>
Date:   Thu, 25 Jan 2018 18:06:58 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chen-Yu Tsai <wens@...e.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at
 boot when a SDP is detected

Hi,

On 17-01-18 02:04, Chanwoo Choi wrote:
> On 2018년 01월 16일 18:33, Hans de Goede wrote:
>> Hi,
>>
>> On 16-01-18 00:43, Chanwoo Choi wrote:
>>> On 2018년 01월 15일 20:32, Hans de Goede wrote:
>>>> HI,
>>>>
>>>> On 15-01-18 10:08, Chanwoo Choi wrote:
>>>>> On 2018년 01월 15일 17:36, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 15-01-18 06:22, Chanwoo Choi wrote:
>>>>>>> On 2018년 01월 15일 00:10, Hans de Goede wrote:
>>>>>>>> The only misdetection which can happen at boot due to data-lines mux issues
>>>>>>>> is detecting a non SDP as SDP, so we only need to retry if we detect a SDP
>>>>>>>> on our first detection.
>>>>>>>>
>>>>>>>> Note Vbus misdetection is not a problem, as soon as the drivers controlling
>>>>>>>> the Vbus path set it correctly we will get an interrupt which reschedules
>>>>>>>> the charger-detection.
>>>>>>>>
>>>>>>>> Also update the comment about the re-detection to reflect this.
>>>>>>>>
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>>>>>>> ---
>>>>>>>>      drivers/extcon/extcon-axp288.c | 14 +++++++-------
>>>>>>>>      1 file changed, 7 insertions(+), 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>>>>>>>> index 63b99d5becd7..17e6808af0d1 100644
>>>>>>>> --- a/drivers/extcon/extcon-axp288.c
>>>>>>>> +++ b/drivers/extcon/extcon-axp288.c
>>>>>>>> @@ -161,16 +161,16 @@ static void axp288_chrg_detect_complete(struct axp288_extcon_info *info,
>>>>>>>>          /*
>>>>>>>>           * We depend on other drivers to do things like mux the data lines,
>>>>>>>>           * enable/disable vbus based on the id-pin, etc. Sometimes the BIOS has
>>>>>>>> -     * not set these things up correctly resulting in the initial charger
>>>>>>>> -     * cable type detection giving a wrong result and we end up not charging
>>>>>>>> -     * or charging at only 0.5A.
>>>>>>>> +     * not set these things up correctly resulting in a wrong result for the
>>>>>>>> +     * initial charger type detection and we end up charging at only 0.5A.
>>>>>>>>           *
>>>>>>>> -     * So we schedule a second cable type detection after 2 seconds to
>>>>>>>> -     * give the other drivers time to load and do their thing.
>>>>>>>> +     * If our first detect detects an SDP charger-type, we try again after
>>>>>>>> +     * 2 seconds to give the other drivers time to load and do their thing.
>>>>>>>>           */
>>>>>>>>          if (!info->first_detect_done) {
>>>>>>>> -        queue_delayed_work(system_wq, &info->det_work,
>>>>>>>> -                   msecs_to_jiffies(2000));
>>>>>>>> +        if (info->previous_cable == EXTCON_CHG_USB_SDP)
>>>>>>>> +            queue_delayed_work(system_wq, &info->det_work,
>>>>>>>> +                       msecs_to_jiffies(2000));
>>>>>>>>              info->first_detect_done = true;
>>>>>>>>          }
>>>>>>>>     
>>>>>>>
>>>>>>> I understand why you add the second delayed_work because of dependency
>>>>>>> of other consumer driver. But, this patch is not proper method. It looks
>>>>>>> like the workaround.
>>>>>>>
>>>>>>> We need to consider the fundamental solution such as using OF graph
>>>>>>> or sending the pending notification when consumer driver is probed.
>>>>>>
>>>>>> I agree that having some sort of proper probe ordering here would be
>>>>>> better. But on these ACPI systems that is going to be quite tricky todo,
>>>>>> since we've no control over the firmware there.
>>>>>>
>>>>>> Note that you've already merged the workaround, this patch merely changes
>>>>>> the workaround to avoid it in cases where it is not necessary, so I would
>>>>>> really like to see this get merged.
>>>>>
>>>>> I merged your patch because I knew this issue related to dependency.
>>>>>
>>>>> But, I don't want to merge this patch until developing the fundamental
>>>>> method. All extcon provider driver have the same issue. I'll try to
>>>>> resolve this issue thro extcon framework.
>>>>
>>>> I really don't see how holding this very simple patch hostage is going to
>>>> help (or deter) finding a better solution for this.
>>>>
>>>> In the mean time my original patch seems to cause mis-detection of CDP ports
>>>> as SDP ports which this fixes.
>>>
>>> I disagree this patch for only one specific connector. Instead of adding second
>>> delayed_work for detection, you better to extend the time from 2 sec to 4 sec
>>> on first time detection.
>>
>> You are misreading the patch, it is changing the code to only do the first
>> detection, unless the result of the first detection is SDP. So I'm not adding
>> a second retry I'm not retrying at all unless really necessary.
> 
> Because I don't prefer to add the second detection for only specific connector
> as I commented, I suggested the extending the delay time of first detection.
> 
> You mentioned on following comment. If extcon provider driver detects the kind
> of connector after the enough delay time,  you don't need to add the second detection.
> You can detect the correct type of connector with only first detection.

You are right. Also I just realized that my previous fix adding the retry
relies on there actually being code changing the usb role-switch which
currently is still not in mainline, so it is useless for mainline and
I've come up with a better fix for once we've that code
(Heikki is mainlining this and I'm helping him):

https://github.com/jwrdegoede/linux-sunxi/commit/7c9c2747fc3c9bb96908b2590bd3292d52f3f003

So for now lets revert my original fix for this, sorry.

I will submit a revert-patch right away.

Regards,

Hans

Powered by blists - more mailing lists