Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752882AbbGBHhi (ORCPT ); Thu, 2 Jul 2015 03:37:38 -0400 Received: from mga01.intel.com ([192.55.52.88]:9127 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752111AbbGBHha (ORCPT ); Thu, 2 Jul 2015 03:37:30 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,390,1432623600"; d="scan'208";a="754600663" Message-ID: <5594EAE9.8010506@intel.com> Date: Thu, 02 Jul 2015 10:40:25 +0300 From: Anda-Maria Nicolae User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Krzysztof Kozlowski CC: sre@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace about charging events References: <1434558539-30591-1-git-send-email-anda-maria.nicolae@intel.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6971 Lines: 140 Hello Krzysztof, The information provided to userspace is not accurate in current driver implementation and it is accurate with this patch. Also, when the battery is reconnected to the charger (after it has been disconnected from the charger), the charger does not trigger any interrupt. Since power_supply_changed() is called only in interrupt handler, the userspace is never notified that the battery is reconnected. Let me explain to you why power_supply_changed() is called in rt9455_pwr_rdy_work_callback() and not in interrupt handler, when CHRVPI interrupt occurs. I created a userspace tool that waits for notifications from the driver. A notification is sent when the driver calls power_supply_changed(). When my userspace tool receives a notification, it reads the files from /sys/class/power_supply/rt9455-charger/ folder. The tool reads each file this to figure it out what has been changed, since the driver notifies the tool only when a change has been made. These files include /sys/class/power_supply/rt9455-charger/online. This file displays 1 if the power source is connected to the charger and 0 otherwise. When the userspace tool reads /sys/class/power_supply/rt9455-charger/online, rt9455_charger_get_online() function from the driver is called. rt9455_charger_get_online() returns the value of PWR_RDY bit. But whenever the power source is connected to / disconnected from the charger, RT9455 first sets CHRVPI interrupt bit, triggers an interrupt, and after almost 1-2 seconds, the charger also updates PWR_RDY bit. So, if we check PWR_RDY bit immediately after CHRVPI bit is set, we will not obtain accurate info about the change that has been made (i.e.: if the power source was connected or disconnected). This is why the driver has struct delayed_work pwr_rdy_work, which is scheduled whenever CHRVPI interrupt bit is set. This is why in this patch, in rt9455_pwr_rdy_work_callback(), power_supply_changed() is called, and not in interrupt handler. Let me explain to you why power_supply_changed() is called in rt9455_batt_presence_work_callback(). As previously stated, the userspace tool is never notified that the battery is reconnected. But since the driver uses struct delayed_work batt_presence_work, to determine whether the battery has been reconnected, we can call power_supply_changed() whenever the driver determines the battery is reconnected (i.e. BATAB interrupt bit is cleared). This is why in this patch, in rt9455_batt_presence_work_callback(), power_supply_changed() is called. On 07/02/2015 03:49 AM, Krzysztof Kozlowski wrote: > 2015-06-18 1:28 GMT+09:00 Anda-Maria Nicolae : >> Do not call power_supply_changed() when CHRVPI interrupt has occurred. > What I cannot find here is the answer to: why? I am not familiar with > the driver so this is just guessing that you wanted to notify > userspace about connected/disconnected charger. In both cases (before > and after the patch) this is achieved, isn't it? > > Best regards, > Krzysztof > > >> CHRVPI interrupt occurs when the charger is connected to or disconnected >> from the power source. Call power_supply_changed() after PWR_RDY bit is >> read, to distinguish between connection to or disconnection from the power >> source. >> Also, call power_supply_changed() after the battery is reconnected to the >> charger, to notify userspace that the battery is no longer absent. >> >> Signed-off-by: Anda-Maria Nicolae >> --- >> drivers/power/rt9455_charger.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/power/rt9455_charger.c b/drivers/power/rt9455_charger.c >> index 08baac6..a49a9d4 100644 >> --- a/drivers/power/rt9455_charger.c >> +++ b/drivers/power/rt9455_charger.c >> @@ -973,7 +973,6 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info, >> >> if (irq2 & GET_MASK(F_CHRVPI)) { >> dev_dbg(dev, "Charger fault occurred\n"); >> - alert_userspace = true; >> /* >> * CHRVPI bit is set in 2 cases: >> * 1. when the power source is connected to the charger. >> @@ -981,6 +980,9 @@ static int rt9455_irq_handler_check_irq2_register(struct rt9455_info *info, >> * To identify the case, PWR_RDY bit is checked. Because >> * PWR_RDY bit is set / cleared after CHRVPI interrupt is >> * triggered, it is used delayed_work to later read PWR_RDY bit. >> + * Also, do not set to true alert_userspace, because there is no >> + * need to notify userspace when CHRVPI interrupt has occurred. >> + * Userspace will be notified after PWR_RDY bit is read. >> */ >> queue_delayed_work(system_power_efficient_wq, >> &info->pwr_rdy_work, >> @@ -1178,7 +1180,7 @@ static irqreturn_t rt9455_irq_handler_thread(int irq, void *data) >> /* >> * Sometimes, an interrupt occurs while rt9455_probe() function >> * is executing and power_supply_register() is not yet called. >> - * Do not call power_supply_charged() in this case. >> + * Do not call power_supply_changed() in this case. >> */ >> if (info->charger) >> power_supply_changed(info->charger); >> @@ -1478,6 +1480,11 @@ static void rt9455_pwr_rdy_work_callback(struct work_struct *work) >> RT9455_MAX_CHARGING_TIME * HZ); >> break; >> } >> + /* >> + * Notify userspace that the charger has been either connected to or >> + * disconnected from the power source. >> + */ >> + power_supply_changed(info->charger); >> } >> >> static void rt9455_max_charging_time_work_callback(struct work_struct *work) >> @@ -1533,6 +1540,11 @@ static void rt9455_batt_presence_work_callback(struct work_struct *work) >> if (ret) >> dev_err(dev, "Failed to unmask BATAB interrupt\n"); >> } >> + /* >> + * Notify userspace that the battery is now connected to the >> + * charger. >> + */ >> + power_supply_changed(info->charger); >> } >> } >> >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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/