From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758411Ab2I1Sck (ORCPT ); Fri, 28 Sep 2012 14:32:40 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:63188 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758069Ab2I1Sci (ORCPT ); Fri, 28 Sep 2012 14:32:38 -0400 Message-ID: <5065ED44.6090302@linaro.org> Date: Fri, 28 Sep 2012 12:32:36 -0600 From: Mathieu Poirier User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: Anton Vorontsov CC: linux-kernel@vger.kernel.org, dwmw2@infradead.org Subject: Re: [PATCH 38/57] power: l9540: Charge only mode fixes References: <1348589574-25655-1-git-send-email-mathieu.poirier@linaro.org> <1348589574-25655-39-git-send-email-mathieu.poirier@linaro.org> <20120928002749.GC20560@lizard> In-Reply-To: <20120928002749.GC20560@lizard> X-Enigmail-Version: 1.5a1pre Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12-09-27 06:27 PM, Anton Vorontsov wrote: > On Tue, Sep 25, 2012 at 10:12:35AM -0600, mathieu.poirier@linaro.org wrote: >> From: Rupesh Kumar >> >> Fix for: charging not getting enabled in >> charge only mode by external charger. > > Subject says l9540.. what is this? > >> Signed-off-by: Rupesh Kumar >> Signed-off-by: Mathieu Poirier >> Reviewed-by: Marcus COOPER >> Reviewed-by: Michel JAOUEN >> Reviewed-by: Philippe LANGLAIS >> Reviewed-by: Philippe LANGLAIS >> --- >> drivers/power/ab8500_charger.c | 42 +++++++++++++++++++++++++++++ >> drivers/power/abx500_chargalg.c | 14 +++++++++ >> include/linux/mfd/abx500/ux500_chargalg.h | 2 + >> 3 files changed, 58 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c >> index 70e7c5e..ebeb068 100644 >> --- a/drivers/power/ab8500_charger.c >> +++ b/drivers/power/ab8500_charger.c >> @@ -15,6 +15,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -94,6 +95,10 @@ >> #define AB8500_SW_CONTROL_FALLBACK 0x03 >> /* Wait for enumeration before charging in us */ >> #define WAIT_ACA_RID_ENUMERATION (5 * 1000) >> +/*External charger control*/ >> +#define AB8500_SYS_CHARGER_CONTROL_REG 0x52 >> +#define EXTERNAL_CHARGER_DISABLE_REG_VAL 0x03 >> +#define EXTERNAL_CHARGER_ENABLE_REG_VAL 0x07 >> >> /* UsbLineStatus register - usb types */ >> enum ab8500_charger_link_status { >> @@ -1672,6 +1677,29 @@ static int ab8500_charger_usb_en(struct ux500_charger *charger, >> return ret; >> } >> >> +static int ab8500_external_charger_prepare(struct notifier_block *charger_nb, >> + unsigned long event, void *data) >> +{ >> + int ret; >> + struct device *dev = data; > > Need an empty line here. > >> + /*Toggle External charger control pin*/ > > Spaces after /* and before */. > >> + ret = abx500_set_register_interruptible(dev, AB8500_SYS_CTRL1_BLOCK, >> + AB8500_SYS_CHARGER_CONTROL_REG, >> + EXTERNAL_CHARGER_DISABLE_REG_VAL); >> + if (ret < 0) { >> + dev_err(dev, "write reg failed %d\n", ret); >> + goto out; > > No need for goto. > >> + } >> + ret = abx500_set_register_interruptible(dev, AB8500_SYS_CTRL1_BLOCK, >> + AB8500_SYS_CHARGER_CONTROL_REG, >> + EXTERNAL_CHARGER_ENABLE_REG_VAL); >> + if (ret < 0) >> + dev_err(dev, "Write reg failed %d\n", ret); >> + >> +out: >> + return ret; >> +} >> + >> /** >> * ab8500_charger_usb_check_enable() - enable usb charging >> * @charger: pointer to the ux500_charger structure >> @@ -3201,6 +3229,10 @@ static int ab8500_charger_suspend(struct platform_device *pdev, >> #define ab8500_charger_resume NULL >> #endif >> >> +static struct notifier_block charger_nb = { >> + .notifier_call = ab8500_external_charger_prepare, >> +}; >> + >> static int __devexit ab8500_charger_remove(struct platform_device *pdev) >> { >> struct ab8500_charger *di = platform_get_drvdata(pdev); >> @@ -3233,6 +3265,11 @@ static int __devexit ab8500_charger_remove(struct platform_device *pdev) >> /* Delete the work queue */ >> destroy_workqueue(di->charger_wq); >> >> + /*Unregister external charger enable notifier*/ > > Spaces. > >> + if (!di->ac_chg.enabled) >> + blocking_notifier_chain_unregister( >> + &charger_notifier_list, &charger_nb); >> + >> flush_scheduled_work(); >> if (di->usb_chg.enabled) >> power_supply_unregister(&di->usb_chg.psy); >> @@ -3307,6 +3344,11 @@ static int __devinit ab8500_charger_probe(struct platform_device *pdev) >> di->ac_chg.enabled = di->pdata->ac_enabled; >> di->ac_chg.external = false; >> >> + /*notifier for external charger enabling*/ > > Spaces. > >> + if (!di->ac_chg.enabled) >> + blocking_notifier_chain_register( >> + &charger_notifier_list, &charger_nb); >> + >> /* USB supply */ >> /* power_supply base class */ >> di->usb_chg.psy.name = "ab8500_usb"; >> diff --git a/drivers/power/abx500_chargalg.c b/drivers/power/abx500_chargalg.c >> index 9568f63..3ca00dd 100644 >> --- a/drivers/power/abx500_chargalg.c >> +++ b/drivers/power/abx500_chargalg.c >> @@ -24,6 +24,7 @@ >> #include >> #include >> #include >> +#include >> >> /* Watchdog kick interval */ >> #define CHG_WD_INTERVAL (6 * HZ) >> @@ -255,6 +256,9 @@ static enum power_supply_property abx500_chargalg_props[] = { >> POWER_SUPPLY_PROP_HEALTH, >> }; >> >> +/*External charger prepare notifier*/ >> +BLOCKING_NOTIFIER_HEAD(charger_notifier_list); > > Can you place the notifier list in di->? Oh, you can't, di is > something different for registrat > > struct abx500_chargalg *di; > struct ab8500_charger *di; > > But having the global list is not friendly to multi-device situations. > > If there's a subtle reason why you do it this way, it have to be described > in the patch description (which is very scarce, btw). Though I did not write this code, it looks to me like any other global variable. Perhaps you can suggest another way to write this ? > > Also, the charger_notifier_list name is in global name space, it's too > generic. Has to be ab8500_chg_notifier_list or something. Ok, will fix. > >> /** >> * abx500_chargalg_safety_timer_expired() - Expiration of the safety timer >> * @data: pointer to the abx500_chargalg structure >> @@ -508,6 +512,8 @@ static int abx500_chargalg_kick_watchdog(struct abx500_chargalg *di) >> static int abx500_chargalg_ac_en(struct abx500_chargalg *di, int enable, >> int vset, int iset) >> { >> + static int ab8500_chargalg_ex_ac_enable_toggle; > > Static? No, this is not multi-device friendly, seems like a hack. > >> + >> if (!di->ac_chg || !di->ac_chg->ops.enable) >> return -ENXIO; >> >> @@ -520,6 +526,14 @@ static int abx500_chargalg_ac_en(struct abx500_chargalg *di, int enable, >> di->chg_info.ac_iset = iset; >> di->chg_info.ac_vset = vset; >> >> + /*enable external charger*/ > > spaces. > >> + if (enable && di->ac_chg->external && >> + !ab8500_chargalg_ex_ac_enable_toggle) { > > Wrong indentation. > >> + blocking_notifier_call_chain(&charger_notifier_list, >> + 0, di->dev); >> + ab8500_chargalg_ex_ac_enable_toggle++; >> + } >> + >> return di->ac_chg->ops.enable(di->ac_chg, enable, vset, iset); >> } >> >> diff --git a/include/linux/mfd/abx500/ux500_chargalg.h b/include/linux/mfd/abx500/ux500_chargalg.h >> index 110d12f..fa831f1 100644 >> --- a/include/linux/mfd/abx500/ux500_chargalg.h >> +++ b/include/linux/mfd/abx500/ux500_chargalg.h >> @@ -41,4 +41,6 @@ struct ux500_charger { >> bool external; >> }; >> >> +extern struct blocking_notifier_head charger_notifier_list; >> + >> #endif >> -- >> 1.7.5.4