From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751168AbeACBAR (ORCPT + 1 other); Tue, 2 Jan 2018 20:00:17 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:20906 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751064AbeACBAP (ORCPT ); Tue, 2 Jan 2018 20:00:15 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.samsung.com 20180103010013epoutp0101af70c6f334e494aa6f52632121bc7a~GJwHpaQMv0568605686epoutp01H X-AuditID: b6c32a36-3d5ff7000000117d-02-5a4c2ac1593a MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="utf-8" Message-id: <5A4C2AC1.8080107@samsung.com> Date: Wed, 03 Jan 2018 09:58:41 +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: <7ed18dbe-b1fb-5eb9-fb3f-779c8a461e44@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrHKsWRmVeSWpSXmKPExsWy7bCmvu5BLZ8og6/nTC3eHJ/OZHF51xw2 i9uNK9gcmD3e77vK5tG3ZRWjx+dNcgHMUak2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6 hpYW5koKeYm5qbZKLj4Bum6ZOUCLlBTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StGGhkZ6 hgbmekZGRnomxrFWRqZAJQmpGWeuPGcsWGBa8enmPqYGxnOaXYycHBICJhJv1q1m62Lk4hAS 2MEo0b7nFxOE851R4uilFawwVf/ft0AldjNKnD67GyzBKyAo8WPyPZYuRg4OZgF5iSOXskHC zAKaEi++TGKBqL/HKPG7exEjRL2WxIHbP9hAbBYBVYntqzrA4mxA8f0vboDF+QUUJa7+eAwW FxWIkNg5/xs7iC0iECRx4zvEQcwCChK/7m0Cs4UF0iSOzLnCAmJzCthJPL1+ghVksYTAAjaJ ta/vM0N84CLR/+AuE4QtLPHq+BZ2kKMlBKQlLh21hahvB3p/7zxmCGcKo8S56/egGowlni3s YoLYzCfx7msPK0Qzr0RHmxBEiYfE541LGSFsR4l5nx6zQnzfwSSxbM8TlgmMcrOQAmwWIsBm IQXYAkbmVYxiqQXFuempxYYFRnrFibnFpXnpesn5uZsYwWlLy2wH46JzPocYBTgYlXh4EyZ4 RwmxJpYVV+YeYpTgYFYS4X2m7BMlxJuSWFmVWpQfX1Sak1p8iNEUGN4TmaVEk/OBKTWvJN7Q xNLAxMwImL4sDQ2VxHkDAlyihATSE0tSs1NTC1KLYPqYODilGhgPHBc5evPWvQtrb245fj01 Y0Zi0d3pLov5FtyUXBl/NTX+z3L2qW787x3X1HyLFgp33uzU73XpBc9K11C7fZfONm6/Lnqr oiVnX9zctdsNXXLm3Xl3s+DU9TxhuQK9E43cgpq+LzhPqr7s28Y0yW6C8i9mL/NtHomWshFz fjq6HuYOCU7kebJFiaU4I9FQi7moOBEABG/PxHEDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrHLMWRmVeSWpSXmKPExsVy+t9jAd2DWj5RBt93KFm8OT6dyeLyrjls FrcbV7A5MHu833eVzaNvyypGj8+b5AKYo7hsUlJzMstSi/TtErgyzlx5zliwwLTi0819TA2M 5zS7GDk5JARMJP6/b2HqYuTiEBLYySixp/c3O0iCV0BQ4sfkeyxdjBwczALyEkcuZUOY6hJT puRClD9glJi1ZB0LRLmWxIHbP9hAbBYBVYntqzoYQWw2oPj+FzfA4vwCihJXfzxmBJkjKhAh 0X2iEiQsIhAg8fNUP9hWZgEFiV/3NrGC2MICaRIbpjQwQuzqYJJYO28n2ExOATuJp9dPsE5g FJiF5NJZCJfOQrh0ASPzKkbJ1ILi3PTcYqMCw7zUcr3ixNzi0rx0veT83E2MwFDddlirbwfj /SXxhxgFOBiVeHg5+ryjhFgTy4orcw8xSnAwK4nwPlP2iRLiTUmsrEotyo8vKs1JLT7EKM3B oiTOezvvWKSQQHpiSWp2ampBahFMlomDU6qBsUdh1swsYd1vZ9hviZv9V+5SuHzbaJfsxZNH pK5MTbG6wmJZcvsD35mS5RObIhWsszWBnurSzEr/Z1QXe3LWA42PQVyTCqM8Ap9Nj6q1237r 6u2bs7eYdZwy25tcnKX0emlPdMQLmZu50w/af/RfxRmXdtVgovHCgglBu0551x3Lbbl+/LWS EktxRqKhFnNRcSIA0TjyXFECAAA= X-CMS-MailID: 20180103005841epcas1p2929d69ec354c8d592e54c6a3c7794bfc X-Msg-Generator: CA CMS-TYPE: 101P 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> 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일 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. > > >> >>> + >>> 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