lkml.org 
[lkml]   [2018]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected
From
Date
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@redhat.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




\
 
 \ /
  Last update: 2018-01-25 18:07    [W:0.107 / U:2.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site