[PATCH 1/2] extcon: axp288: Constify the axp288_pwr_up_down_info array

[PATCH 1/2] extcon: axp288: Constify the axp288_pwr_up_down_info array

[Hans de Goede]

Make the axp288_pwr_up_down_info array const char * const, this leads
to the following section size changes:

.text     0x674 -> 0x664
.data     0x148 -> 0x0f0
.rodata   0x0b4 -> 0x114

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-axp288.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 081d8a1b4fd7..63b99d5becd7 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -118,7 +118,7 @@ static const struct x86_cpu_id cherry_trail_cpu_ids[] = {
 };
 
 /* Power up/down reason string array */
-static char *axp288_pwr_up_down_info[] = {
+static const char * const axp288_pwr_up_down_info[] = {
 	"Last wake caused by user pressing the power button",
 	"Last wake caused by a charger insertion",
 	"Last wake caused by a battery insertion",
@@ -136,7 +136,7 @@ static char *axp288_pwr_up_down_info[] = {
  */
 static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
 {
-	char **rsi;
+	const char * const *rsi;
 	unsigned int val, i, clear_mask = 0;
 	int ret;
 
-- 
2.14.3

[PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

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;
 	}
 
-- 
2.14.3

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Chanwoo Choi]

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 don't know what is best solution right now. I'll consider the appropriate
method for all extcon provider drivers.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/2] extcon: axp288: Constify the axp288_pwr_up_down_info array

[Chanwoo Choi]

On 2018년 01월 15일 00:10, Hans de Goede wrote:
> Make the axp288_pwr_up_down_info array const char * const, this leads
> to the following section size changes:
> 
> .text     0x674 -> 0x664
> .data     0x148 -> 0x0f0
> .rodata   0x0b4 -> 0x114
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index 081d8a1b4fd7..63b99d5becd7 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -118,7 +118,7 @@ static const struct x86_cpu_id cherry_trail_cpu_ids[] = {
>  };
>  
>  /* Power up/down reason string array */
> -static char *axp288_pwr_up_down_info[] = {
> +static const char * const axp288_pwr_up_down_info[] = {
>  	"Last wake caused by user pressing the power button",
>  	"Last wake caused by a charger insertion",
>  	"Last wake caused by a battery insertion",
> @@ -136,7 +136,7 @@ static char *axp288_pwr_up_down_info[] = {
>   */
>  static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
>  {
> -	char **rsi;
> +	const char * const *rsi;
>  	unsigned int val, i, clear_mask = 0;
>  	int ret;
>  
> 

Applied it to v4.17
because I had finished the extcon pull request for v4.16.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

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.

Regards,

Hans

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Chanwoo Choi]

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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

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.

Regards,

Hans

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Chanwoo Choi]

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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

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.

Regards,

Hans




> 

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Chanwoo Choi]

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.

>>>>>>> +     * 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.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: axp288: Only reschedule charger-detection at boot when a SDP is detected

[Hans de Goede]

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