Message-ID: <7ed18dbe-b1fb-5eb9-fb3f-779c8a461e44@redhat.com>
Date:   Tue, 2 Jan 2018 23:44:46 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of
 seconds after probe()

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

> 
>>   };
>>   
>>   /* 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.


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

Powered by blists - more mailing lists