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 <[email protected]> >>>>> --- >>>>> 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. -- Best Regards, Chanwoo Choi Samsung Electronics
