From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751069AbeACBEx (ORCPT + 1 other); Tue, 2 Jan 2018 20:04:53 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:47850 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750749AbeACBEv (ORCPT ); Tue, 2 Jan 2018 20:04:51 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout3.samsung.com 20180103010449epoutp03919fee41f2fb01a71cda43f6ce390f57~GJ0IYqZ5h0080900809epoutp03g X-AuditID: b6c32a46-995ff700000010ee-e2-5a4c2c31f9ac MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="utf-8" Message-id: <5A4C2C30.9010502@samsung.com> Date: Wed, 03 Jan 2018 10:04:48 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Hans de Goede , MyungJoo Ham Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] extcon: axp288: Redo charger type dection a couple of seconds after probe() In-reply-to: <5A4C2AC1.8080107@samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrDKsWRmVeSWpSXmKPExsWy7bCmua6hjk+UwfztHBZvjk9nsri8aw6b xe3GFWwOzB7v911l8+jbsorR4/MmuQDmqFSbjNTElNQihdS85PyUzLx0WyXv4HjneFMzA0Nd Q0sLcyWFvMTcVFslF58AXbfMHKBFSgpliTmlQKGAxOJiJX07m6L80pJUhYz84hJbpWhDQyM9 QwNzPSMjIz0T41grI1OgkoTUjG3TP7IW9FhUvDl8g72BsVOni5GDQ0LAROL7o7IuRi4OIYEd jBJvj8xkhHC+M0osuPUByOEEK/p0+A0rRGI3o8Tfmw/BErwCghI/Jt9jAZnELCAvceRSNkiY WUBT4sWXSSwQ9fcYJX4/OA5VryWx8tgJJhCbRUBV4v655WA2G1B8/4sbbCA2v4CixNUfj8Hq RQUiJHbO/8YOYosIBEnc+L6CFWKBgsSve5vAbGGBNIkjc66A3cApoC3x+ZExyF4JgQVsEpfW 9rFAfOkise5dCMQvwhKvjm9hh7ClJZ6t2sgIUd/OKNG+dx4zhDOFUeLc9XtMEFXGEs8WdjFB LOaT6Dj8lx1iKK9ER5sQRImHxOeNS6GB5Sgx79NjaGCtZJLYeKCfeQKj3Cyk8JqFCK9ZSOG1 gJF5FaNYakFxbnpqsVGBkV5xYm5xaV66XnJ+7iZGcMrSctvBuOSczyFGAQ5GJR5ejj7vKCHW xLLiytxDjBIczEoivM+UfaKEeFMSK6tSi/Lji0pzUosPMZoCg3sis5Rocj4wneaVxBuaWBqY mJkZmZtZANOXOG9rgEuUkEB6YklqdmpqQWoRTB8TB6dUA6Pa8jUiorGS90RPFDS8kkzY+dno ROH+g/vDDxpb3H645abAmV3Wio15t6dt3OZkr/GRe2bJitXyucu8p95Xy2yUPd7xt3DHiVe6 JlN3tNbsPCvFueo37y6BKzI9ks4Nwa9MH+1yNLAyWCJr8Ej07VIVu06meR1VjT2TPLg7EhXa z9lUysfwBCuxFGckGmoxFxUnAgDqUTIFbwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrLLMWRmVeSWpSXmKPExsVy+t9jAV0DHZ8og7+bZCzeHJ/OZHF51xw2 i9uNK9gcmD3e77vK5tG3ZRWjx+dNcgHMUVw2Kak5mWWpRfp2CVwZ26Z/ZC3osah4c/gGewNj p04XIyeHhICJxKfDb1i7GLk4hAR2Mkrs+dzADpLgFRCU+DH5HksXIwcHs4C8xJFL2RCmusSU KbkQ5Q8YJWZ9/8sIUa4lsfLYCSYQm0VAVeL+ueVgNhtQfP+LG2wgNr+AosTVH48ZQeaICkRI dJ+oBAmLCARI/DzVD7aVWUBB4te9TawgtrBAmsSGKQ2MELtWMkmseb6FDaSXU0Bb4vMj4wmM ArOQHDoL4dBZCIcuYGRexSiZWlCcm55bbFRglJdarlecmFtcmpeul5yfu4kRGKjbDmv172B8 vCT+EKMAB6MSD2/CBO8oIdbEsuLK3EOMEhzMSiK8z5R9ooR4UxIrq1KL8uOLSnNSiw8xSnOw KInz8ucfixQSSE8sSc1OTS1ILYLJMnFwSjUwrlZ4XSHWaHX0mNGfUN6Pv2btqruvFCv/czZn QmPY5uuJq4T65ILe/5yZdW9W9z2xv6Klm9aLrNlt9sFaY2G01rKuSr7Ztf5ivsJZ3Lof04Nt IsRsm/SSNrAwFivs73pyKGBVC++P3qbfu+v6PrIzXeLZE/CupnahgNT168+O5dj87++0eVui xFKckWioxVxUnAgA4bnkglACAAA= X-CMS-MailID: 20180103010448epcas2p4700317637bd3bde1b2235e8b0c47f983 X-Msg-Generator: CA CMS-TYPE: 102P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20171222123640epcas3p190e0800ab69794c55de84381f8a543fb X-RootMTR: 20171222123640epcas3p190e0800ab69794c55de84381f8a543fb References: <20171222123616.9562-1-hdegoede@redhat.com> <20171222123616.9562-3-hdegoede@redhat.com> <5A4AD84D.600@samsung.com> <7ed18dbe-b1fb-5eb9-fb3f-779c8a461e44@redhat.com> <5A4C2AC1.8080107@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 >>>> --- >>>> 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 >>>> * Copyright (C) 2015 Intel Corporation >>>> * Author: Ramakrishna Pallala >>>> * >>>> @@ -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