| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120928001317.GB20560@lizard>
Date: Thu, 27 Sep 2012 17:13:18 -0700
From: Anton Vorontsov <anton.vorontsov@...aro.org>
To: mathieu.poirier@...aro.org
Cc: linux-kernel@...r.kernel.org, dwmw2@...radead.org
Subject: Re: [PATCH 37/57] power: ab8500_bm: Quick re-attach charging
behaviour
On Tue, Sep 25, 2012 at 10:12:34AM -0600, mathieu.poirier@...aro.org wrote:
> From: Kalle Komierowski <karl.komierowski@...ricsson.com>
>
> Due to a bug in some AB8500 ASICs charger removal cannot always
> be detected if the removal and reinsertion is done to close in time.
> This patch detects above described case and handles the situation
> so that charging will be kept turned on.
>
> Signed-off-by: Kalle Komierowski <karl.komierowski@...ricsson.com>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
> Reviewed-by: Marcus COOPER <marcus.xm.cooper@...ricsson.com>
> Reviewed-by: Jonas ABERG <jonas.aberg@...ricsson.com>
> Reviewed-by: Philippe LANGLAIS <philippe.langlais@...ricsson.com>
> ---
> drivers/power/ab8500_charger.c | 105 ++++++++++++++++++++++++++++-
> drivers/power/abx500_chargalg.c | 31 ++++++++-
> include/linux/mfd/abx500/ux500_chargalg.h | 1 +
> 3 files changed, 134 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
> index 8137ea5..70e7c5e 100644
> --- a/drivers/power/ab8500_charger.c
> +++ b/drivers/power/ab8500_charger.c
> @@ -49,6 +49,7 @@
> #define VBUS_DET_DBNC100 0x02
> #define VBUS_DET_DBNC1 0x01
> #define OTP_ENABLE_WD 0x01
> +#define DROP_COUNT_RESET 0x01
>
> #define MAIN_CH_INPUT_CURR_SHIFT 4
> #define VBUS_IN_CURR_LIM_SHIFT 4
> @@ -1672,6 +1673,105 @@ static int ab8500_charger_usb_en(struct ux500_charger *charger,
> }
>
> /**
> + * ab8500_charger_usb_check_enable() - enable usb charging
> + * @charger: pointer to the ux500_charger structure
> + * @vset: charging voltage
> + * @iset: charger output current
> + *
> + * Check if the VBUS charger has been disconnected and reconnected without
> + * AB8500 rising an interrupt. Returns 0 on success.
> + */
> +static int ab8500_charger_usb_check_enable(struct ux500_charger *charger,
> + int vset, int iset)
Wrong indentation (should be at least two tabs; sometimes the driver
aligning to opening parenthesis, sometimes not... it's better to be
consistent).
> +{
> + u8 usbch_ctrl1 = 0;
> + int ret = 0;
> +
No need for this empty line.
> + struct ab8500_charger *di = to_ab8500_charger_usb_device_info(charger);
> +
> + if (!di->usb.charger_connected)
> + return ret;
> +
> + ret = abx500_get_register_interruptible(di->dev, AB8500_CHARGER,
> + AB8500_USBCH_CTRL1_REG, &usbch_ctrl1);
> + if (ret < 0) {
> + dev_err(di->dev, "ab8500 read failed %d\n", __LINE__);
> + return ret;
> + }
> + dev_dbg(di->dev, "USB charger ctrl: 0x%02x\n", usbch_ctrl1);
> +
> + if (!(usbch_ctrl1 & USB_CH_ENA)) {
By returning early you can greatly reduce indentation and make the
code more readable.
if (usbch_ctrl1 & USB_CH_ENA)
return ret;
...the rest...
> + dev_info(di->dev, "Charging has been disabled abnormally and will be re-enabled\n");
> +
> + ret = abx500_mask_and_set_register_interruptible(di->dev,
> + AB8500_CHARGER, AB8500_CHARGER_CTRL,
> + DROP_COUNT_RESET, DROP_COUNT_RESET);
> + if (ret < 0) {
> + dev_err(di->dev, "ab8500 write failed %d\n", __LINE__);
> + return ret;
> + }
> +
> + ret = ab8500_charger_usb_en(&di->usb_chg, true, vset, iset);
> + if (ret < 0) {
> + dev_err(di->dev, "Failed to enable VBUS charger %d\n",
> + __LINE__);
> + return ret;
> + }
> + }
> + return ret;
> +}
> +
> +/**
> + * ab8500_charger_ac_check_enable() - enable usb charging
> + * @charger: pointer to the ux500_charger structure
> + * @vset: charging voltage
> + * @iset: charger output current
> + *
> + * Check if the AC charger has been disconnected and reconnected without
> + * AB8500 rising an interrupt. Returns 0 on success.
> + */
> +static int ab8500_charger_ac_check_enable(struct ux500_charger *charger,
> + int vset, int iset)
This function is very very similar to the previous one. The only
difference seem to be whether you use MAIN_CH_ENA or USB_CH_ENA..
So would it make sense to introduce a helper function that accepts one
additional parameter:
ab8500_charger_check_enable(....., int ch_ena)
?
[...]
> +static int abx500_chargalg_check_charger_enable(struct abx500_chargalg *di)
> +{
> + switch (di->charge_state) {
> + case STATE_NORMAL:
> + case STATE_MAINTENANCE_A:
> + case STATE_MAINTENANCE_B:
> + break;
> + default:
> + return 0;
> + }
> +
> + if (di->chg_info.charger_type & USB_CHG) {
> + return di->usb_chg->ops.check_enable(di->usb_chg,
> + di->bat->bat_type[di->bat->batt_id].normal_vol_lvl,
> + di->bat->bat_type[di->bat->batt_id].normal_cur_lvl);
> + } else if (di->chg_info.charger_type & AC_CHG) {
> + return di->ac_chg->ops.check_enable(di->ac_chg,
> + di->bat->bat_type[di->bat->batt_id].normal_vol_lvl,
> + di->bat->bat_type[di->bat->batt_id].normal_cur_lvl);
di->bat is very repetative, and can be avoided by introducing a variable.
(Not that it would greatly help readability, but a little, yes.)
> + }
> + return -ENXIO;
> +}
> +
> /**
> * abx500_chargalg_check_charger_connection() - Check charger connection change
> * @di: pointer to the abx500_chargalg structure
> @@ -1220,6 +1243,7 @@ static void abx500_chargalg_external_power_changed(struct power_supply *psy)
> static void abx500_chargalg_algorithm(struct abx500_chargalg *di)
> {
> int charger_status;
> + int ret;
>
> /* Collect data from all power_supply class devices */
> class_for_each_device(power_supply_class, NULL,
> @@ -1228,8 +1252,13 @@ static void abx500_chargalg_algorithm(struct abx500_chargalg *di)
> abx500_chargalg_end_of_charge(di);
> abx500_chargalg_check_temp(di);
> abx500_chargalg_check_charger_voltage(di);
> -
Stray change.
> charger_status = abx500_chargalg_check_charger_connection(di);
> +
> + ret = abx500_chargalg_check_charger_enable(di);
> + if (ret < 0)
> + dev_err(di->dev, "Checking charger if enabled error: %d line: %d\n",
> + ret, __LINE__);
> +
> /*
> * First check if we have a charger connected.
> * Also we don't allow charging of unknown batteries if configured
> diff --git a/include/linux/mfd/abx500/ux500_chargalg.h b/include/linux/mfd/abx500/ux500_chargalg.h
> index d43ac0f..110d12f 100644
> --- a/include/linux/mfd/abx500/ux500_chargalg.h
> +++ b/include/linux/mfd/abx500/ux500_chargalg.h
> @@ -17,6 +17,7 @@ struct ux500_charger;
>
> struct ux500_charger_ops {
> int (*enable) (struct ux500_charger *, int, int, int);
> + int (*check_enable) (struct ux500_charger *, int, int);
> int (*kick_wd) (struct ux500_charger *);
> int (*update_curr) (struct ux500_charger *, int);
> };
> --
> 1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists