Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe()

[Chanwoo Choi]

Hi Hans,

On 2018ë 01ì 03ì 09:58, Chanwoo Choi wrote:
> Hi Hans,
> 
> On 2018ë 01ì 03ì 07:44, Hans de Goede wrote:
>> Hi,
>>
>> On 02-01-18 01:54, Chanwoo Choi wrote:
>>> Hi Hans,
>>>
>>> s/dection/detection on patch title.
>>
>> Thank you for all the reviews.
>>
>> I've fixed the typo in my personal tree.
>>
>>> On 2017ë 12ì 22ì 21:36, Hans de Goede wrote:
>>>> The axp288 extcon code depends 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.
>>>>
>>>> This commit starts a second charger-detection cycle a couple of seconds
>>>> after the first one finishes, giving the other drivers time to load and
>>>> do their thing.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>> ---
>>>>   drivers/extcon/extcon-axp288.c | 32 ++++++++++++++++++++++++++++++--
>>>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>>>> index 386afb7d1160..cc7c35c7ff02 100644
>>>> --- a/drivers/extcon/extcon-axp288.c
>>>> +++ b/drivers/extcon/extcon-axp288.c
>>>> @@ -1,6 +1,7 @@
>>>>   /*
>>>>    * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
>>>>    *
>>>> + * Copyright (C) 2016-2017 Hans de Goede <hdegoede@xxxxxxxxxx>
>>>>    * Copyright (C) 2015 Intel Corporation
>>>>    * Author: Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>
>>>>    *
>>>> @@ -97,9 +98,11 @@ 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;
>>>
>>> The first_detect_done is used only one time in the axp288_handle_chrg_det_event().
>>> The other function don't use it. So, you better to define and use
>>> 'static bool first_detect_done' in the axp288_handle_chrg_det_event()
>>> instead of defining the 'first_detect_done' in the struct axp288_extcon_info.
>>
>> But what if a device has 2 axp288 PMICs (unlikely I know) then only the
>> first one to check this will do the re-detect 2 seconds later, unless they
>> race, which is bad in itself too.
>>
>> In general using static function variables is a bad idea and should be
>> avoided, it does not work when their are multiple instances of the device
>> and it is racy. So sorry but I'm not going to make this change.
> 
> You're right. It is my mistake. Please keep your way.
> 
>>
>>>
>>>>   };
>>>>     /* Power up/down reason string array */
>>>> @@ -137,6 +140,25 @@ 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;
>>>> +    }
>>>> +}
>>>
>>> The axp288_chrg_detect_complete() is only used in the axp288_handle_chrg_det_event()
>>> and axp288_chrg_detect_complete() is not a complicate. I think that you don't need
>>> to make the separate function. I'd like you to add the
>>>
>>>> +
>>>>   static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>>>   {
>>>>       int ret, stat, cfg, pwr_stat;
>>>> @@ -201,6 +223,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>>>           info->previous_cable = cable;
>>>>       }
>>>>   +    axp288_chrg_detect_complete(info);
>>>
>>> As I commented, you better to add the code directly instead of separate function.
>>
>> I would prefer to keep this as a separate function, that keeps the main
>> flow of the axp288_handle_chrg_det_event function a lot more readable IMHO.
>>
>> axp288_handle_chrg_det_event already has a non trivial code flow adding more
>> conditional code to it only makes it harder to read.
>>
>> But if you insist I can move the code inside the function for v2. Note that
>> this will not make a difference for the code generated by the compiler, the
>> compiler will auto-inline it anyways.
> 
> I didn't mention the result from compiler. I focus on the readability.
> On v1, the developer always check what to do in axp288_chrg_detect_complete()
> even if this function is used only one time like '__init'.
> Actually, axp288_chrg_detect_complete() is not complicate and short.
> 
> But, I will not be forced because it is not a critical issue.
> If you want to keep the separate function, you just send v2 with only fixing the typo.

You don't need to send v2. I'll fix the typo and apply your patch v1
As I mentioned, it is not a critical issue. Thanks.

> 
>>
>>
>>>
>>>> +
>>>>       return 0;
>>>>     dev_det_ret:
>>>> @@ -222,8 +246,11 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>>>>       return IRQ_HANDLED;
>>>>   }
>>>>   -static void axp288_extcon_enable(struct axp288_extcon_info *info)
>>>> +static void axp288_extcon_det_work(struct work_struct *work)
>>>>   {
>>>> +    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 */
>>>> @@ -245,6 +272,7 @@ 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);
>>>>   @@ -287,7 +315,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>>>>       }
>>>>         /* Start charger cable type detection */
>>>> -    axp288_extcon_enable(info);
>>>> +    queue_delayed_work(system_wq, &info->det_work, 0);
>>>>         return 0;
>>>>   }
>>>>
>>>
>>>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics