lkml.org 
[lkml]   [2018]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe()
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@redhat.com>
>>> ---
>>> 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@redhat.com>
>>> * Copyright (C) 2015 Intel Corporation
>>> * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>> *
>>> @@ -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.

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

\
 
 \ /
  Last update: 2018-01-03 02:00    [W:0.055 / U:12.012 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site