Messages in this thread | | | Subject | Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected | From | Hans de Goede <> | Date | Thu, 25 Jan 2018 18:06:58 +0100 |
| |
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
| |