Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933139AbbD0OCt (ORCPT ); Mon, 27 Apr 2015 10:02:49 -0400 Received: from mga02.intel.com ([134.134.136.20]:16855 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932692AbbD0OCp convert rfc822-to-8bit (ORCPT ); Mon, 27 Apr 2015 10:02:45 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,657,1422950400"; d="scan'208";a="719876170" From: "Nicolae, Anda-maria" To: =?iso-8859-2?Q?Krzysztof_Koz=B3owski?= CC: "sre@kernel.org" , "dbaryshkov@gmail.com" , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "dwmw2@infradead.org" , "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: RE: [PATCH] power_supply: Add support for Richtek rt9455 battery charger Thread-Topic: [PATCH] power_supply: Add support for Richtek rt9455 battery charger Thread-Index: AQHQfykLWKTPlgb780GKsnLE6kC9x51g1eQ3///5pgCAABYQ+w== Date: Mon, 27 Apr 2015 14:02:17 +0000 Message-ID: <11982715D62E664DBACA4351A418B0581437DA7C@IRSMSX107.ger.corp.intel.com> References: <1429862240-21619-1-git-send-email-anda-maria.nicolae@intel.com> <11982715D62E664DBACA4351A418B0581437DA51@IRSMSX107.ger.corp.intel.com>, In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.3.86.137] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5632 Lines: 107 Hello Krzysztof, Thank you for your prompt answer :). More comments inline. Thanks, Anda ________________________________________ From: Krzysztof Koz?owski [k.kozlowski.k@gmail.com] Sent: Monday, April 27, 2015 4:37 PM To: Nicolae, Anda-maria Cc: sre@kernel.org; dbaryshkov@gmail.com; robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org; dwmw2@infradead.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org Subject: Re: [PATCH] power_supply: Add support for Richtek rt9455 battery charger 2015-04-27 22:06 GMT+09:00 Nicolae, Anda-maria : (...) >> + >> +static void rt9455_batt_presence_work_callback(struct work_struct *work) >> +{ >> + struct rt9455_info *info = container_of(work, struct rt9455_info, >> + batt_presence_work.work); >> + struct device *dev = &info->client->dev; >> + u8 irq1; >> + int ret; >> + >> + ret = rt9455_read(info, RT9455_REG_IRQ1, &irq1); >> + if (ret) { >> + dev_err(dev, "Failed to read IRQ1 register\n"); >> + return; >> + } >> + >> + /* If the battery is still absent, batt_presence_work is rescheduled. >> + Otherwise, max_charging_time is scheduled. >> + */ >> + if (irq1 & RT9455_REG_IRQ1_BATAB_MASK) { >> + schedule_delayed_work(&info->batt_presence_work, >> + RT9455_BATT_PRESENCE_DELAY * HZ); >> + } else { >> + schedule_delayed_work(&info->max_charging_time_work, >> + RT9455_MAX_CHARGING_TIME * HZ); >> + } > > Do all of these schedules have to be executed on system_wq? Maybe > power efficient workqueue could be used? I don't see any locking so > probably not... but still it is worth investigating. > > Ok, I will use queue_delayed_work(system_power_efficient_wq, ...). I'm not sure if I understand exactly what do you mean by "I don't see any locking so probably not". I can see that __queue_work() uses spinlocks. And other drivers that use system_power_efficient_wq also do not use additional locking. > I intend to replace schedule_delayed_work() with queue_delayed_work(system_power_efficient_wq, ...), without adding any locking in the driver. Please let me know if I understood correctly what you meant . You understood me correctly. I was thinking about possibility of concurrent execution of work callbacks... but actually this should not be impacted by unbound or bound workqueues. So never mind. So, I will stick to replacing schedule_delayed_work() with queue_delayed_work(system_power_efficient_wq, ...), without adding any locking in the driver. However after looking at the code I got different question. You are reading here and in few other places the RT9455_REG_IRQ1. Isn't the register auto-cleared? No, RT9455_REG_IRQ1 is not cleared after reading. Let me give you an example. Suppose the battery is missing, so BATAB bit is set. This bit remains set until the battery is reconnected to the charger, independent of the number of i2c_reads over RT9455_REG_IRQ1. Unfortunately, no interrupt is triggered when the battery is reconnected (and BATAB bit is cleared). (...) >> + >> +#if IS_ENABLED(CONFIG_USB_PHY) >> + info->usb_phy = usb_get_phy(USB_PHY_TYPE_USB2); >> + if (IS_ERR_OR_NULL(info->usb_phy)) { >> + dev_err(dev, "Failed to get USB transceiver\n"); >> + } else { >> + info->nb.notifier_call = rt9455_usb_event; >> + ret = usb_register_notifier(info->usb_phy, &info->nb); >> + if (ret) { >> + dev_err(dev, "Failed to register USB notifier\n"); >> + usb_put_phy(info->usb_phy); >> + } >> + } >> +#endif >> + >> + INIT_DELAYED_WORK(&info->pwr_rdy_work, rt9455_pwr_rdy_work_callback); >> + INIT_DELAYED_WORK(&info->max_charging_time_work, >> + rt9455_max_charging_time_work_callback); >> + INIT_DELAYED_WORK(&info->batt_presence_work, >> + rt9455_batt_presence_work_callback); > > Maybe any of these could be made as DEFERRABLE_WORK? Just thinking out loud... > > I cannot use DEFERRABLE_WORK instead of DELAYED_WORK. DEFERRABLE_WORK runs in interrupt context, while DELAYED_WORK runs in process context. All handlers for DELAYED_WORK from the driver perform read/write operations via I2C, which may sleep. This is the reason why DELAYED_WORK is used. AFAIK, the context is the same. The only difference is the type of timer used for scheduling the work. In case of deferrable timer it won't wake up a sleeping CPU. Which means that it may be executed some unspecified time in the future. This has energy conserving benefits but may not be appropriate for your case because you could want to poll the battery status in exact intervals. Just to understand correctly, you mean that if I use DEFERRABLE_WORK, I will not get kernel panic with message "scheduling while atomic"? This is what happens if I use timers and I thought that I would get the same result with DEFERRABLE_WORK. Actually, the timers are not critical and status of the charger may be checked some fraction of a second later. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/