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 <hdego...@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 <hdego...@redhat.com>
>>>> * Copyright (C) 2015 Intel Corporation
>>>> * Author: Ramakrishna Pallala <ramakrishna.pall...@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.
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
