[PATCH] Revert "extcon: axp288: Redo charger type detection a couple of seconds after probe()"

[PATCH] Revert "extcon: axp288: Redo charger type detection a couple of seconds after probe()"

[Hans de Goede]

Redoing the charger type detection to give the usb-role-switch code time
to properly set the role-switch is no good for mainline, since the
usb-role-switch code is not yet in mainline (my bad, sorry).

Also once we've that code there are better ways to fix this which are
not prone to racing as doing a retry after 2 seconds is.

This reverts commit 50082c17bb1455acacd376ae30dff92f2e1addbd.

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 9abc99e8c0a5..901eddd3994b 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -1,7 +1,6 @@
 /*
  * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
  *
- * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
  * Copyright (C) 2015 Intel Corporation
  * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
  *
@@ -98,11 +97,9 @@ struct axp288_extcon_info {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *regmap_irqc;
-	struct delayed_work det_work;
 	int irq[EXTCON_IRQ_END];
 	struct extcon_dev *edev;
 	unsigned int previous_cable;
-	bool first_detect_done;
 };
 
 /* Power up/down reason string array */
@@ -140,25 +137,6 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
 	regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask);
 }
 
-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.
-	 *
-	 * So we schedule a second cable type detection 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));
-		info->first_detect_done = true;
-	}
-}
-
 static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 {
 	int ret, stat, cfg, pwr_stat;
@@ -223,8 +201,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 		info->previous_cable = cable;
 	}
 
-	axp288_chrg_detect_complete(info);
-
 	return 0;
 
 dev_det_ret:
@@ -246,11 +222,8 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void axp288_extcon_det_work(struct work_struct *work)
+static void axp288_extcon_enable(struct axp288_extcon_info *info)
 {
-	struct axp288_extcon_info *info =
-		container_of(work, struct axp288_extcon_info, det_work.work);
-
 	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
 						BC_GLOBAL_RUN, 0);
 	/* Enable the charger detection logic */
@@ -272,7 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	info->regmap = axp20x->regmap;
 	info->regmap_irqc = axp20x->regmap_irqc;
 	info->previous_cable = EXTCON_NONE;
-	INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work);
 
 	platform_set_drvdata(pdev, info);
 
@@ -315,7 +287,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	}
 
 	/* Start charger cable type detection */
-	queue_delayed_work(system_wq, &info->det_work, 0);
+	axp288_extcon_enable(info);
 
 	return 0;
 }
-- 
2.14.3

Re: [PATCH] Revert "extcon: axp288: Redo charger type detection a couple of seconds after probe()"

[Chanwoo Choi]

Hi Hans,

Why don't modify the extcon-axp288.c after revert it?
Instead, you sent the patch[1]. Are you going to send other patch for extcon-axp288.c?
[1] [PATCH] extcon: int3496: process id-pin first so that we start with the right status


On 2018년 01월 26일 02:10, Hans de Goede wrote:
> Redoing the charger type detection to give the usb-role-switch code time
> to properly set the role-switch is no good for mainline, since the
> usb-role-switch code is not yet in mainline (my bad, sorry).
> 
> Also once we've that code there are better ways to fix this which are
> not prone to racing as doing a retry after 2 seconds is.
> 
> This reverts commit 50082c17bb1455acacd376ae30dff92f2e1addbd.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 32 ++------------------------------
>  1 file changed, 2 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index 9abc99e8c0a5..901eddd3994b 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -1,7 +1,6 @@
>  /*
>   * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
>   *
> - * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
>   * Copyright (C) 2015 Intel Corporation
>   * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>   *
> @@ -98,11 +97,9 @@ struct axp288_extcon_info {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	struct regmap_irq_chip_data *regmap_irqc;
> -	struct delayed_work det_work;
>  	int irq[EXTCON_IRQ_END];
>  	struct extcon_dev *edev;
>  	unsigned int previous_cable;
> -	bool first_detect_done;
>  };
>  
>  /* Power up/down reason string array */
> @@ -140,25 +137,6 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
>  	regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask);
>  }
>  
> -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.
> -	 *
> -	 * So we schedule a second cable type detection 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));
> -		info->first_detect_done = true;
> -	}
> -}
> -
>  static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  {
>  	int ret, stat, cfg, pwr_stat;
> @@ -223,8 +201,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  		info->previous_cable = cable;
>  	}
>  
> -	axp288_chrg_detect_complete(info);
> -
>  	return 0;
>  
>  dev_det_ret:
> @@ -246,11 +222,8 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void axp288_extcon_det_work(struct work_struct *work)
> +static void axp288_extcon_enable(struct axp288_extcon_info *info)
>  {
> -	struct axp288_extcon_info *info =
> -		container_of(work, struct axp288_extcon_info, det_work.work);
> -
>  	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>  						BC_GLOBAL_RUN, 0);
>  	/* Enable the charger detection logic */
> @@ -272,7 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  	info->regmap = axp20x->regmap;
>  	info->regmap_irqc = axp20x->regmap_irqc;
>  	info->previous_cable = EXTCON_NONE;
> -	INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work);
>  
>  	platform_set_drvdata(pdev, info);
>  
> @@ -315,7 +287,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Start charger cable type detection */
> -	queue_delayed_work(system_wq, &info->det_work, 0);
> +	axp288_extcon_enable(info);
>  
>  	return 0;
>  }
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] Revert "extcon: axp288: Redo charger type detection a couple of seconds after probe()"

[Hans de Goede]

Hi,

On 01-02-18 22:22, Chanwoo Choi wrote:
> Hi Hans,
> 
> Why don't modify the extcon-axp288.c after revert it?

Fixing the axp288 extcon code properly requires usb-role-switch support
which is not yet in the mainline kernel, I will submit the fix for it
once usb-role-switch support is in the mainline.

> Instead, you sent the patch[1].

Yes this is a preparation patch for the pending extcon-axp288 fix.

Regards,

Hans



  Are you going to send other patch for extcon-axp288.c?
> [1] [PATCH] extcon: int3496: process id-pin first so that we start with the right status
> 
> 
> On 2018년 01월 26일 02:10, Hans de Goede wrote:
>> Redoing the charger type detection to give the usb-role-switch code time
>> to properly set the role-switch is no good for mainline, since the
>> usb-role-switch code is not yet in mainline (my bad, sorry).
>>
>> Also once we've that code there are better ways to fix this which are
>> not prone to racing as doing a retry after 2 seconds is.
>>
>> This reverts commit 50082c17bb1455acacd376ae30dff92f2e1addbd.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/extcon/extcon-axp288.c | 32 ++------------------------------
>>   1 file changed, 2 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>> index 9abc99e8c0a5..901eddd3994b 100644
>> --- a/drivers/extcon/extcon-axp288.c
>> +++ b/drivers/extcon/extcon-axp288.c
>> @@ -1,7 +1,6 @@
>>   /*
>>    * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
>>    *
>> - * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
>>    * Copyright (C) 2015 Intel Corporation
>>    * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>    *
>> @@ -98,11 +97,9 @@ struct axp288_extcon_info {
>>   	struct device *dev;
>>   	struct regmap *regmap;
>>   	struct regmap_irq_chip_data *regmap_irqc;
>> -	struct delayed_work det_work;
>>   	int irq[EXTCON_IRQ_END];
>>   	struct extcon_dev *edev;
>>   	unsigned int previous_cable;
>> -	bool first_detect_done;
>>   };
>>   
>>   /* Power up/down reason string array */
>> @@ -140,25 +137,6 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
>>   	regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask);
>>   }
>>   
>> -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.
>> -	 *
>> -	 * So we schedule a second cable type detection 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));
>> -		info->first_detect_done = true;
>> -	}
>> -}
>> -
>>   static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>   {
>>   	int ret, stat, cfg, pwr_stat;
>> @@ -223,8 +201,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>   		info->previous_cable = cable;
>>   	}
>>   
>> -	axp288_chrg_detect_complete(info);
>> -
>>   	return 0;
>>   
>>   dev_det_ret:
>> @@ -246,11 +222,8 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> -static void axp288_extcon_det_work(struct work_struct *work)
>> +static void axp288_extcon_enable(struct axp288_extcon_info *info)
>>   {
>> -	struct axp288_extcon_info *info =
>> -		container_of(work, struct axp288_extcon_info, det_work.work);
>> -
>>   	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>>   						BC_GLOBAL_RUN, 0);
>>   	/* Enable the charger detection logic */
>> @@ -272,7 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>   	info->regmap = axp20x->regmap;
>>   	info->regmap_irqc = axp20x->regmap_irqc;
>>   	info->previous_cable = EXTCON_NONE;
>> -	INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work);
>>   
>>   	platform_set_drvdata(pdev, info);
>>   
>> @@ -315,7 +287,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	/* Start charger cable type detection */
>> -	queue_delayed_work(system_wq, &info->det_work, 0);
>> +	axp288_extcon_enable(info);
>>   
>>   	return 0;
>>   }
>>
> 

Re: [PATCH] Revert "extcon: axp288: Redo charger type detection a couple of seconds after probe()"

[Chanwoo Choi]

Hi,

On 2018년 02월 02일 08:48, Hans de Goede wrote:
> Hi,
> 
> On 01-02-18 22:22, Chanwoo Choi wrote:
>> Hi Hans,
>>
>> Why don't modify the extcon-axp288.c after revert it?
> 
> Fixing the axp288 extcon code properly requires usb-role-switch support
> which is not yet in the mainline kernel, I will submit the fix for it
> once usb-role-switch support is in the mainline.
> 
>> Instead, you sent the patch[1].
> 
> Yes this is a preparation patch for the pending extcon-axp288 fix.

OK. Applied it.

Thanks,
Chanwoo Choi

> 
> Regards,
> 
> Hans
> 
> 
> 
>  Are you going to send other patch for extcon-axp288.c?
>> [1] [PATCH] extcon: int3496: process id-pin first so that we start with the right status
>>
>>
>> On 2018년 01월 26일 02:10, Hans de Goede wrote:
>>> Redoing the charger type detection to give the usb-role-switch code time
>>> to properly set the role-switch is no good for mainline, since the
>>> usb-role-switch code is not yet in mainline (my bad, sorry).
>>>
>>> Also once we've that code there are better ways to fix this which are
>>> not prone to racing as doing a retry after 2 seconds is.
>>>
>>> This reverts commit 50082c17bb1455acacd376ae30dff92f2e1addbd.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/extcon/extcon-axp288.c | 32 ++------------------------------
>>>   1 file changed, 2 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>>> index 9abc99e8c0a5..901eddd3994b 100644
>>> --- a/drivers/extcon/extcon-axp288.c
>>> +++ b/drivers/extcon/extcon-axp288.c
>>> @@ -1,7 +1,6 @@
>>>   /*
>>>    * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
>>>    *
>>> - * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
>>>    * Copyright (C) 2015 Intel Corporation
>>>    * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>>    *
>>> @@ -98,11 +97,9 @@ struct axp288_extcon_info {
>>>       struct device *dev;
>>>       struct regmap *regmap;
>>>       struct regmap_irq_chip_data *regmap_irqc;
>>> -    struct delayed_work det_work;
>>>       int irq[EXTCON_IRQ_END];
>>>       struct extcon_dev *edev;
>>>       unsigned int previous_cable;
>>> -    bool first_detect_done;
>>>   };
>>>     /* Power up/down reason string array */
>>> @@ -140,25 +137,6 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
>>>       regmap_write(info->regmap, AXP288_PS_BOOT_REASON_REG, clear_mask);
>>>   }
>>>   -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.
>>> -     *
>>> -     * So we schedule a second cable type detection 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));
>>> -        info->first_detect_done = true;
>>> -    }
>>> -}
>>> -
>>>   static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>>   {
>>>       int ret, stat, cfg, pwr_stat;
>>> @@ -223,8 +201,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>>           info->previous_cable = cable;
>>>       }
>>>   -    axp288_chrg_detect_complete(info);
>>> -
>>>       return 0;
>>>     dev_det_ret:
>>> @@ -246,11 +222,8 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>>>       return IRQ_HANDLED;
>>>   }
>>>   -static void axp288_extcon_det_work(struct work_struct *work)
>>> +static void axp288_extcon_enable(struct axp288_extcon_info *info)
>>>   {
>>> -    struct axp288_extcon_info *info =
>>> -        container_of(work, struct axp288_extcon_info, det_work.work);
>>> -
>>>       regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>>>                           BC_GLOBAL_RUN, 0);
>>>       /* Enable the charger detection logic */
>>> @@ -272,7 +245,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>>       info->regmap = axp20x->regmap;
>>>       info->regmap_irqc = axp20x->regmap_irqc;
>>>       info->previous_cable = EXTCON_NONE;
>>> -    INIT_DELAYED_WORK(&info->det_work, axp288_extcon_det_work);
>>>         platform_set_drvdata(pdev, info);
>>>   @@ -315,7 +287,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>>       }
>>>         /* Start charger cable type detection */
>>> -    queue_delayed_work(system_wq, &info->det_work, 0);
>>> +    axp288_extcon_enable(info);
>>>         return 0;
>>>   }
>>>
>>
> 
> 
>