From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754611AbeAOLci (ORCPT + 1 other); Mon, 15 Jan 2018 06:32:38 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:43591 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751290AbeAOLch (ORCPT ); Mon, 15 Jan 2018 06:32:37 -0500 X-Google-Smtp-Source: ACJfBouLw2kLw2taUXRvzUiuqDWpsUJhPtuTZ4gl4SRKCoa4lH8Kbh1pspujpkc+SbPGCtjYXemibA== Subject: Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected To: Chanwoo Choi , MyungJoo Ham , Chen-Yu Tsai Cc: linux-kernel@vger.kernel.org References: <20180114151021.11432-1-hdegoede@redhat.com> <20180114151021.11432-2-hdegoede@redhat.com> <5A5C3A85.9080109@samsung.com> <996966c8-6a31-eb48-579e-f8ef119a15d5@redhat.com> <5A5C6F7C.4090200@samsung.com> From: Hans de Goede Message-ID: <4176b3af-01f8-9a5e-f6c8-4c446468918f@redhat.com> Date: Mon, 15 Jan 2018 12:32:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <5A5C6F7C.4090200@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 >>>> --- >>>> 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. Regards, Hans