Hi Anton<div> Thanks for your review comments and advice, I will amend them and update again. </div><div>Thanks<br><br><div class="gmail_quote">2012/7/14 Anton Vorontsov <span dir="ltr"><<a href="mailto:cbouatmailru@gmail.com" target="_blank">cbouatmailru@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Jul 06, 2012 at 11:02:09AM +0800, Jett.Zhou wrote:<br>
> There are charger and battery measurement feature for 88pm860x PMIC.<br>
><br>
> For charger, it can support pre-charge with small current when battery is<br>
> nearly exausted and then changed into fast-charge with CC&CV mode.<br>
><br>
> For battery monitor, it can support battery measurement such as<br>
> vbat,vsys,vchg and ibat etc,it can aslo accumulating the Coulomb value<br>
> charged or discharged from battery based on Conlomb Counter, we use it<br>
> to estimate battery capacity.<br>
><br>
> Signed-off-by: Jett.Zhou <<a href="mailto:jtzhou@marvell.com">jtzhou@marvell.com</a>><br>
> ---<br>
<br>
</div>Thanks for the patches, Jett! I see it is a huge and complex driver,<br>
so no wonder it takes quite a bit of iterations to get it merged.<br>
It's all right, and you're making a good progress!<br>
<br>
[...]<br>
<div class="im">> diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c<br>
> index d09918c..c8911f4 100644<br>
> --- a/drivers/mfd/88pm860x-core.c<br>
> +++ b/drivers/mfd/88pm860x-core.c<br>
> @@ -18,6 +18,7 @@<br>
</div>[...]<br>
<div class="im">> static struct mfd_cell rtc_devs[] = {<br>
> @@ -791,6 +798,19 @@ static void __devinit device_power_init(struct pm860x_chip *chip,<br>
> &preg_resources[0], chip->irq_base);<br>
> if (ret < 0)<br>
> dev_err(chip->dev, "Failed to add preg subdev\n");<br>
> +<br>
> + if (pdata->chg_desc) {<br>
> + pdata->chg_desc->charger_regulators =<br>
> + &chg_desc_regulator_data[0];<br>
> + pdata->chg_desc->num_charger_regulators =<br>
> + ARRAY_SIZE(chg_desc_regulator_data),<br>
> + power_devs[3].platform_data = pdata->chg_desc;<br>
> + power_devs[3].pdata_size = sizeof(struct charger_desc);<br>
<br>
</div>Please use sizeof() of a variable, not a type. I.e.<br>
sizeof(*pdata->chg_desc). This is usually preferred in the kernel code.<br>
<div><div class="h5"><br>
> + ret = mfd_add_devices(chip->dev, 0, &power_devs[3], 1,<br>
> + NULL, chip->irq_base);<br>
> + if (ret < 0)<br>
> + dev_err(chip->dev, "Failed to add chg-manager subdev\n");<br>
> + }<br>
> }<br>
><br>
> static void __devinit device_onkey_init(struct pm860x_chip *chip,<br>
> diff --git a/drivers/power/88pm860x_battery.c b/drivers/power/88pm860x_battery.c<br>
> new file mode 100644<br>
> index 0000000..132e2ee<br>
> --- /dev/null<br>
> +++ b/drivers/power/88pm860x_battery.c<br>
> @@ -0,0 +1,1026 @@<br>
> +/*<br>
> + * Battery driver for Marvell 88PM860x PMIC<br>
> + *<br>
> + * Copyright (c) 2012 Marvell International Ltd.<br>
> + * Author: Jett Zhou <<a href="mailto:jtzhou@marvell.com">jtzhou@marvell.com</a>><br>
> + * Haojian Zhuang <<a href="mailto:haojian.zhuang@marvell.com">haojian.zhuang@marvell.com</a>><br>
> + *<br>
> + * This program is free software; you can redistribute it and/or modify<br>
> + * it under the terms of the GNU General Public License version 2 as<br>
> + * published by the Free Software Foundation.<br>
> + */<br>
> +<br>
> +#include <linux/kernel.h><br>
> +#include <linux/module.h><br>
> +#include <linux/platform_device.h><br>
> +#include <linux/slab.h><br>
> +#include <linux/mutex.h><br>
> +#include <linux/string.h><br>
> +#include <linux/power_supply.h><br>
> +#include <linux/mfd/88pm860x.h><br>
> +#include <linux/delay.h><br>
> +<br>
> +/* bit definitions of Status Query Interface 2 */<br>
> +#define STATUS2_CHG (1 << 2)<br>
> +#define STATUS2_BAT (1 << 3)<br>
> +#define STATUS2_VBUS (1 << 4)<br>
> +<br>
> +/* bit definitions of Measurement Enable 1 Register */<br>
> +#define MEAS1_TINT (1 << 3)<br>
> +#define MEAS1_GP1 (1 << 5)<br>
> +<br>
> +/* bit definitions of Measurement Enable 3 Register */<br>
> +#define MEAS3_IBAT (1 << 0)<br>
> +#define MEAS3_BAT_DET (1 << 1)<br>
> +#define MEAS3_CC (1 << 2)<br>
> +<br>
> +/* bit definitions of Measurement Off Time Register */<br>
> +#define MEAS_OFF_SLEEP_EN (1 << 1)<br>
> +<br>
> +/* bit definitions of GPADC Bias Current 2 Register */<br>
> +#define GPBIAS2_GPADC1_SET (2 << 4)<br>
> +/* GPADC1 Bias Current value in uA unit */<br>
> +#define GPBIAS2_GPADC1_UA ((GPBIAS2_GPADC1_SET >> 4) * 5 + 1)<br>
> +<br>
> +/* bit definitions of GPADC Misc 1 Register */<br>
> +#define GPMISC1_GPADC_EN (1 << 0)<br>
> +<br>
> +/* bit definitions of Charger Control 6 Register */<br>
> +#define CC6_BAT_DET_GPADC1 1<br>
> +<br>
> +/* bit definitions of Coulomb Counter Reading Register */<br>
> +#define CCNT_AVG_SEL (4 << 3)<br>
> +<br>
> +/* bit definitions of RTC miscellaneous Register1 */<br>
> +#define RTC_SOC_5LSB (0x1F << 3)<br>
> +<br>
> +/* bit definitions of RTC Register1 */<br>
> +#define RTC_SOC_3MSB (0x7)<br>
> +<br>
> +/* bit definitions of Power up Log register */<br>
> +#define BAT_WU_LOG (1<<6)<br>
> +<br>
> +/* coulomb counter index */<br>
> +#define CCNT_POS1 0<br>
> +#define CCNT_POS2 1<br>
> +#define CCNT_NEG1 2<br>
> +#define CCNT_NEG2 3<br>
> +#define CCNT_SPOS 4<br>
> +#define CCNT_SNEG 5<br>
> +<br>
> +/* OCV -- Open Circuit Voltage */<br>
> +#define OCV_MODE_ACTIVE 0<br>
> +#define OCV_MODE_SLEEP 1<br>
> +<br>
> +/* Vbat range of CC for measuring Rbat */<br>
> +#define LOW_BAT_THRESHOLD 3600<br>
> +#define VBATT_RESISTOR_MIN 3800<br>
> +#define VBATT_RESISTOR_MAX 4100<br>
> +<br>
> +/* TBAT for batt, TINT for chip itself */<br>
> +#define PM860X_TEMP_TINT (0)<br>
> +#define PM860X_TEMP_TBAT (1)<br>
> +<br>
> +/*<br>
> + * Battery temperature based on NTC resistor, defined<br>
> + * corresponding resistor value -- Ohm / C degeree.<br>
> + */<br>
> +#define TBAT_NEG_25D 127773 /* -25 */<br>
> +#define TBAT_NEG_10D 54564 /* -10 */<br>
> +#define TBAT_0D 32330 /* 0 */<br>
> +#define TBAT_10D 19785 /* 10 */<br>
> +#define TBAT_20D 12468 /* 20 */<br>
> +#define TBAT_30D 8072 /* 30 */<br>
> +#define TBAT_40D 5356 /* 40 */<br>
> +<br>
> +struct pm860x_battery_info {<br>
> + struct pm860x_chip *chip;<br>
> + struct i2c_client *i2c;<br>
> + struct device *dev;<br>
> +<br>
> + struct power_supply battery;<br>
> + struct mutex lock;<br>
> + int status;<br>
> + int irq_cc;<br>
> + int irq_batt;<br>
> + int max_capacity;<br>
> + int resistor; /* Battery Internal Resistor */<br>
> + int last_capacity;<br>
> + int start_soc;<br>
> + unsigned present:1;<br>
> + unsigned temp_type:1; /* TINT or TBAT */<br>
> +};<br>
> +<br>
> +struct ccnt {<br>
> + unsigned long long int pos;<br>
> + unsigned long long int neg;<br>
> + unsigned int spos;<br>
> + unsigned int sneg;<br>
> +<br>
> + int total_chg; /* mAh(3.6C) */<br>
> + int total_dischg; /* mAh(3.6C) */<br>
> +};<br>
> +<br>
> +/*<br>
> + * State of Charge.<br>
> + * The first number is mAh(=3.6C), and the second number is percent point.<br>
> + */<br>
> +int array_soc[][2] = { {4170, 100},<br>
> +{4154, 99}, {4136, 98}, {4122, 97}, {4107, 96},<br>
<br>
</div></div>Maybe indent it properly? I.e.<br>
<br>
int array_soc[][2] = {<br>
{...}, {...}, {...},<br>
{...}, {...}, {...},<br>
{...}, {...}, {...},<br>
<div class="im">};<br>
<br>
> +{4102, 95}, {4088, 94}, {4081, 93}, {4070, 92},<br>
> +{4060, 91}, {4053, 90}, {4044, 89}, {4035, 88},<br>
</div>[...]<br>
<div class="im">> +{3637, 7}, {3622, 6}, {3609, 5}, {3580, 4},<br>
> +{3558, 3}, {3540, 2}, {3510, 1}, {3429, 0}<br>
> +};<br>
> +<br>
> +static struct ccnt ccnt_data;<br>
> +<br>
> +static int calc_ccnt(struct pm860x_battery_info *info, struct ccnt *ccnt);<br>
> +static int calc_soc(struct pm860x_battery_info *info, int state, int *soc);<br>
> +static int clear_ccnt(struct pm860x_battery_info *info, struct ccnt *ccnt);<br>
> +static int measure_current(struct pm860x_battery_info *info, int *data);<br>
<br>
</div>Do you really need all these forward declarations, maybe it's possible<br>
to rearrange the functions and variables and thus avoid forward decls?<br>
<div><div class="h5"><br>
> +<br>
> +static irqreturn_t pm860x_coulomb_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_battery_info *info = data;<br>
> +<br>
> + calc_ccnt(info, &ccnt_data);<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_batt_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_battery_info *info = data;<br>
> + int ret;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + ret = pm860x_reg_read(info->i2c, PM8607_STATUS_2);<br>
> + if (ret & STATUS2_BAT) {<br>
> + info->present = 1;<br>
> + info->temp_type = PM860X_TEMP_TBAT;<br>
> + } else {<br>
> + info->present = 0;<br>
> + info->temp_type = PM860X_TEMP_TINT;<br>
> + }<br>
> + mutex_unlock(&info->lock);<br>
> + /* clear ccnt since battery is attached or dettached */<br>
> + clear_ccnt(info, &ccnt_data);<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static void pm860x_init_battery(struct pm860x_battery_info *info)<br>
> +{<br>
> + unsigned char buf[2];<br>
> + int ret, data;<br>
> + int bat_remove, soc;<br>
<br>
</div></div>Please use one line per variable declaration, i.e.<br>
<br>
int ret;<br>
int data;<br>
int bat_remove;<br>
int soc;<br>
<br>
This is per linux coding style.<br>
<div><div class="h5"><br>
> +<br>
> + /* measure enable on GPADC1 */<br>
> + data = MEAS1_GP1;<br>
> + if (info->temp_type == PM860X_TEMP_TINT)<br>
> + data |= MEAS1_TINT;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_MEAS_EN1, data, data);<br>
> + if (ret)<br>
> + goto out;<br>
> +<br>
> + /* measure enable on IBAT, BAT_DET, CC. IBAT is depend on CC. */<br>
> + data = MEAS3_IBAT | MEAS3_BAT_DET | MEAS3_CC;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_MEAS_EN3, data, data);<br>
> + if (ret)<br>
> + goto out;<br>
> +<br>
> + /* measure disable CC in sleep time */<br>
> + ret = pm860x_reg_write(info->i2c, PM8607_MEAS_OFF_TIME1, 0x82);<br>
> + if (ret)<br>
> + goto out;<br>
> + ret = pm860x_reg_write(info->i2c, PM8607_MEAS_OFF_TIME2, 0x6c);<br>
> + if (ret)<br>
> + goto out;<br>
> +<br>
> + /* enable GPADC */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_GPADC_MISC1,<br>
> + GPMISC1_GPADC_EN, GPMISC1_GPADC_EN);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> +<br>
> + /* detect battery via GPADC1 */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL6,<br>
> + CC6_BAT_DET_GPADC1, CC6_BAT_DET_GPADC1);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> +<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CCNT, 7 << 3,<br>
> + CCNT_AVG_SEL);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> +<br>
> + /* set GPADC1 bias */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_GP_BIAS2, 0xF << 4,<br>
> + GPBIAS2_GPADC1_SET);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> +<br>
> + /* check whether battery present) */<br>
> + mutex_lock(&info->lock);<br>
> + ret = pm860x_reg_read(info->i2c, PM8607_STATUS_2);<br>
> + if (ret < 0) {<br>
> + mutex_unlock(&info->lock);<br>
> + goto out;<br>
> + }<br>
> + if (ret & STATUS2_BAT) {<br>
> + info->present = 1;<br>
> + info->temp_type = PM860X_TEMP_TBAT;<br>
> + } else {<br>
> + info->present = 0;<br>
> + info->temp_type = PM860X_TEMP_TINT;<br>
> + }<br>
> + mutex_unlock(&info->lock);<br>
> +<br>
> + calc_soc(info, OCV_MODE_ACTIVE, &soc);<br>
> +<br>
> + data = pm860x_reg_read(info->i2c, PM8607_POWER_UP_LOG);<br>
> + bat_remove = data & BAT_WU_LOG;<br>
> +<br>
> + dev_dbg(info->dev, "battery wake up? %s\n",<br>
> + bat_remove != 0 ? "yes" : "no");<br>
> +<br>
> + /* restore SOC from RTC domain register */<br>
> + if (bat_remove == 0) {<br>
> + buf[0] = pm860x_reg_read(info->i2c, PM8607_RTC_MISC2);<br>
> + buf[1] = pm860x_reg_read(info->i2c, PM8607_RTC1);<br>
> + data = ((buf[1] & 0x3) << 5) | ((buf[0] >> 3) & 0x1F);<br>
> + if (data > soc + 15)<br>
> + info->start_soc = soc;<br>
> + else if (data < soc - 15)<br>
> + info->start_soc = soc;<br>
> + else<br>
> + info->start_soc = data;<br>
> + dev_dbg(info->dev, "soc_rtc %d, soc_ocv :%d\n", data, soc);<br>
> + } else {<br>
> + pm860x_set_bits(info->i2c, PM8607_POWER_UP_LOG,<br>
> + BAT_WU_LOG, BAT_WU_LOG);<br>
> + info->start_soc = soc;<br>
> + }<br>
> + info->last_capacity = info->start_soc;<br>
> + dev_dbg(info->dev, "init soc : %d\n", info->last_capacity);<br>
> +out:<br>
> + return;<br>
> +}<br>
> +<br>
> +/*<br>
> + * register 1 bit[7:0] -- bit[11:4] of measured value of voltage<br>
> + * register 0 bit[3:0] -- bit[3:0] of measured value of voltage<br>
> + */<br>
> +static int measure_12bit_voltage(struct pm860x_battery_info *info,<br>
> + int offset, int *data)<br>
> +{<br>
> + unsigned char buf[2];<br>
> + int ret;<br>
> +<br>
> + if (!data)<br>
> + return -EINVAL;<br>
<br>
</div></div>The condition is never true, thus it can be removed.<br>
<div class="im"><br>
> +<br>
> + ret = pm860x_bulk_read(info->i2c, offset, 2, buf);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> +<br>
> + *data = ((buf[0] & 0xff) << 4) | (buf[1] & 0x0f);<br>
> + /* V_MEAS(mV) = data * 1.8 * 1000 / (2^12) */<br>
> + *data = ((*data & 0xfff) * 9 * 25) >> 9;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int measure_vbatt(struct pm860x_battery_info *info, int state,<br>
> + int *data)<br>
> +{<br>
> + unsigned char buf[5];<br>
> + int ret = 0;<br>
<br>
</div>There is no need for the initializer value here.<br>
<div><div class="h5"><br>
> +<br>
> + switch (state) {<br>
> + case OCV_MODE_ACTIVE:<br>
> + ret = measure_12bit_voltage(info, PM8607_VBAT_MEAS1, data);<br>
> + if (ret)<br>
> + return ret;<br>
> + /* V_BATT_MEAS(mV) = value * 3 * 1.8 * 1000 / (2^12) */<br>
> + *data *= 3;<br>
> + break;<br>
> + case OCV_MODE_SLEEP:<br>
> + /*<br>
> + * voltage value of VBATT in sleep mode is saved in different<br>
> + * registers.<br>
> + * bit[11:10] -- bit[7:6] of LDO9(0x18)<br>
> + * bit[9:8] -- bit[7:6] of LDO8(0x17)<br>
> + * bit[7:6] -- bit[7:6] of LDO7(0x16)<br>
> + * bit[5:4] -- bit[7:6] of LDO6(0x15)<br>
> + * bit[3:0] -- bit[7:4] of LDO5(0x14)<br>
> + */<br>
> + ret = pm860x_bulk_read(info->i2c, PM8607_LDO5, 5, buf);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> + ret = ((buf[4] >> 6) << 10) | ((buf[3] >> 6) << 8)<br>
> + | ((buf[2] >> 6) << 6) | ((buf[1] >> 6) << 4)<br>
> + | (buf[0] >> 4);<br>
> + /* V_BATT_MEAS(mV) = data * 3 * 1.8 * 1000 / (2^12) */<br>
> + *data = ((*data & 0xff) * 27 * 25) >> 9;<br>
> + break;<br>
> + default:<br>
> + return -EINVAL;<br>
> + }<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static void set_temp_threshold(struct pm860x_battery_info *info,<br>
> + int min, int max)<br>
> +{<br>
> + int data;<br>
> +<br>
> + /* (tmp << 8) / 1800 */<br>
> + if (min <= 0)<br>
> + data = 0;<br>
> + else<br>
> + data = (min << 8) / 1800;<br>
> + pm860x_reg_write(info->i2c, PM8607_GPADC1_HIGHTH, data);<br>
> + dev_dbg(info->dev, "TEMP_HIGHTH : min: %d, 0x%x\n", min, data);<br>
> +<br>
> + if (max <= 0)<br>
> + data = 0xff;<br>
> + else<br>
> + data = (max << 8) / 1800;<br>
> + pm860x_reg_write(info->i2c, PM8607_GPADC1_LOWTH, data);<br>
> + dev_dbg(info->dev, "TEMP_LOWTH:max : %d, 0x%x\n", max, data);<br>
> +}<br>
> +<br>
> +static int measure_temp(struct pm860x_battery_info *info, int *data)<br>
> +{<br>
> + int ret, temp;<br>
> + int min, max;<br>
<br>
</div></div>int ret;<br>
int temp;<br>
int min;<br>
int max;<br>
<div class="im"><br>
> +<br>
> + if (!data)<br>
> + return -EINVAL;<br>
<br>
</div>This condition is not possible, so you can remove it.<br>
<div><div class="h5"><br>
> + if (info->temp_type == PM860X_TEMP_TINT) {<br>
> + ret = measure_12bit_voltage(info, PM8607_TINT_MEAS1, data);<br>
> + if (ret)<br>
> + return ret;<br>
> + *data = (*data - 884) * 1000 / 3611;<br>
> + } else {<br>
> + ret = measure_12bit_voltage(info, PM8607_GPADC1_MEAS1, data);<br>
> + if (ret)<br>
> + return ret;<br>
> + /* meausered Vtbat(mV) / Ibias_current(11uA)*/<br>
> + *data = (*data * 1000) / GPBIAS2_GPADC1_UA;<br>
> +<br>
> + if (*data > TBAT_NEG_25D) {<br>
> + temp = -30; /* over cold , suppose -30 roughly */<br>
> + max = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, 0, max);<br>
> + } else if (*data > TBAT_NEG_10D) {<br>
> + temp = -15; /* -15 degree, code */<br>
> + max = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, 0, max);<br>
> + } else if (*data > TBAT_0D) {<br>
> + temp = -5; /* -5 degree */<br>
> + min = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + max = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, max);<br>
> + } else if (*data > TBAT_10D) {<br>
> + temp = 5; /* in range of (0, 10) */<br>
> + min = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + max = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, max);<br>
> + } else if (*data > TBAT_20D) {<br>
> + temp = 15; /* in range of (10, 20) */<br>
> + min = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + max = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, max);<br>
> + } else if (*data > TBAT_30D) {<br>
> + temp = 25; /* in range of (20, 30) */<br>
> + min = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + max = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, max);<br>
> + } else if (*data > TBAT_40D) {<br>
> + temp = 35; /* in range of (30, 40) */<br>
> + min = TBAT_NEG_10D * GPBIAS2_GPADC1_UA / 1000;<br>
> + max = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, max);<br>
> + } else {<br>
> + min = TBAT_40D * GPBIAS2_GPADC1_UA / 1000;<br>
> + set_temp_threshold(info, min, 0);<br>
> + temp = 45; /* over heat ,suppose 45 roughly */<br>
> + }<br>
> +<br>
> + dev_dbg(info->dev, "temp_C:%d C,temp_mv:%d mv\n", temp, *data);<br>
> + *data = temp;<br>
> + }<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/*<br>
> + * Return value is signed data.<br>
> + * Negative value means discharging, and positive value means charging.<br>
> + */<br>
> +static int measure_current(struct pm860x_battery_info *info, int *data)<br>
> +{<br>
> + unsigned char buf[2];<br>
> + short s;<br>
> + int ret;<br>
> +<br>
> + if (!data)<br>
> + return -EINVAL;<br>
<br>
</div></div>The condition is never true, so can be removed.<br>
<div><div class="h5"><br>
> +<br>
> + ret = pm860x_bulk_read(info->i2c, PM8607_IBAT_MEAS1, 2, buf);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> +<br>
> + s = ((buf[0] & 0xff) << 8) | (buf[1] & 0xff);<br>
> + /* current(mA) = value * 0.125 */<br>
> + *data = s >> 3;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int set_charger_current(struct pm860x_battery_info *info, int data,<br>
> + int *old)<br>
> +{<br>
> + int ret;<br>
> +<br>
> + if (data < 50 || data > 1600 || !old)<br>
> + return -EINVAL;<br>
> +<br>
> + data = ((data - 50) / 50) & 0x1f;<br>
> + *old = pm860x_reg_read(info->i2c, PM8607_CHG_CTRL2);<br>
> + *old = (*old & 0x1f) * 50 + 50;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL2, 0x1f, data);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int calc_resistor(struct pm860x_battery_info *info)<br>
> +{<br>
> + int ret, i, data;<br>
> + int vbatt_sum1, vbatt_sum2, chg_current;<br>
> + int ibatt_sum1, ibatt_sum2;<br>
<br>
</div></div>One variable declaration per line please.<br>
<div class="im"><br>
> +<br>
> + ret = measure_current(info, &data);<br>
> + /* make sure that charging is launched by data > 0 */<br>
> + if (ret || data < 0)<br>
> + goto out;<br>
> +<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + /* calculate resistor only in CC charge mode */<br>
> + if (data < VBATT_RESISTOR_MIN || data > VBATT_RESISTOR_MAX)<br>
> + goto out;<br>
> +<br>
> + /* current is saved */<br>
> + if (set_charger_current(info, 500, &chg_current))<br>
> + goto out;<br>
> +<br>
> + msleep(500);<br>
<br>
</div>Maybe add a comment why there's an msleep, any why it's exactly 500 ms?<br>
<div><div class="h5"><br>
> +<br>
> + for (i = 0, vbatt_sum1 = 0, ibatt_sum1 = 0; i < 10; i++) {<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out_meas;<br>
> + vbatt_sum1 += data;<br>
> + ret = measure_current(info, &data);<br>
> + if (ret)<br>
> + goto out_meas;<br>
> +<br>
> + if (data < 0)<br>
> + ibatt_sum1 = ibatt_sum1 - data; /* discharging */<br>
> + else<br>
> + ibatt_sum1 = ibatt_sum1 + data; /* charging */<br>
> + }<br>
> +<br>
> + if (set_charger_current(info, 100, &ret))<br>
> + goto out_meas;<br>
> +<br>
> + msleep(500);<br>
> +<br>
> + for (i = 0, vbatt_sum2 = 0, ibatt_sum2 = 0; i < 10; i++) {<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out_meas;<br>
> + vbatt_sum2 += data;<br>
> + ret = measure_current(info, &data);<br>
> + if (ret)<br>
> + goto out_meas;<br>
> +<br>
> + if (data < 0)<br>
> + ibatt_sum2 = ibatt_sum2 - data; /* discharging */<br>
> + else<br>
> + ibatt_sum2 = ibatt_sum2 + data; /* charging */<br>
> + }<br>
> +<br>
> + /* restore current setting */<br>
> + if (set_charger_current(info, chg_current, &ret))<br>
> + goto out_meas;<br>
> +<br>
> + if ((vbatt_sum1 > vbatt_sum2) && (ibatt_sum1 > ibatt_sum2)<br>
> + && (ibatt_sum2 > 0)) {<br>
> + /* calculate resistor in discharging case */<br>
> + data = 1000 * (vbatt_sum1 - vbatt_sum2)<br>
> + / (ibatt_sum1 - ibatt_sum2);<br>
> + if ((data - info->resistor > 0)<br>
> + && (data - info->resistor < info->resistor))<br>
> + info->resistor = data;<br>
> + if ((info->resistor - data > 0)<br>
> + && (info->resistor - data < data))<br>
> + info->resistor = data;<br>
> + }<br>
> + return 0;<br>
> +<br>
> +out_meas:<br>
> + set_charger_current(info, chg_current, &ret);<br>
> +out:<br>
> + return -EINVAL;<br>
> +}<br>
> +<br>
> +static int read_ccnt(struct pm860x_battery_info *info, int offset,<br>
> + int *ccnt)<br>
> +{<br>
> + unsigned char buf[2];<br>
> + int ret;<br>
> +<br>
> + if (!ccnt)<br>
> + return -EINVAL;<br>
<br>
</div></div>ccnt is always !NULL, so the check is unnecessary.<br>
<div class="im"><br>
> +<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CCNT, 7, offset & 7);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_bulk_read(info->i2c, PM8607_CCNT_MEAS1, 2, buf);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + *ccnt = ((buf[0] & 0xff) << 8) | (buf[1] & 0xff);<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int calc_ccnt(struct pm860x_battery_info *info, struct ccnt *ccnt)<br>
> +{<br>
> + unsigned int sum;<br>
> + int ret, data;<br>
<br>
</div>int ret;<br>
int data;<br>
<div><div class="h5"><br>
> +<br>
> + ret = read_ccnt(info, CCNT_POS1, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + sum = data & 0xffff;<br>
> + ret = read_ccnt(info, CCNT_POS2, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + sum |= (data & 0xffff) << 16;<br>
> + ccnt->pos += sum;<br>
> +<br>
> + ret = read_ccnt(info, CCNT_NEG1, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + sum = data & 0xffff;<br>
> + ret = read_ccnt(info, CCNT_NEG2, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + sum |= (data & 0xffff) << 16;<br>
> + sum = ~sum + 1; /* since it's negative */<br>
> + ccnt->neg += sum;<br>
> +<br>
> + ret = read_ccnt(info, CCNT_SPOS, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + ccnt->spos += data;<br>
> + ret = read_ccnt(info, CCNT_SNEG, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> +<br>
> + /*<br>
> + * charge(mAh) = count * 1.6984 * 1e(-8)<br>
> + * = count * 16984 * 1.024 * 1.024 * 1.024 / (2 ^ 40)<br>
> + * = count * 18236 / (2 ^ 40)<br>
> + */<br>
> + ccnt->total_chg = (int) ((ccnt->pos * 18236) >> 40);<br>
> + ccnt->total_dischg = (int) ((ccnt->neg * 18236) >> 40);<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int clear_ccnt(struct pm860x_battery_info *info, struct ccnt *ccnt)<br>
> +{<br>
> + int data;<br>
> +<br>
> + memset(ccnt, 0, sizeof(struct ccnt));<br>
<br>
</div></div>sizeof(*ccnt), this is what we prefer.<br>
<div class="im"><br>
> + /* read to clear ccnt */<br>
> + read_ccnt(info, CCNT_POS1, &data);<br>
> + read_ccnt(info, CCNT_POS2, &data);<br>
> + read_ccnt(info, CCNT_NEG1, &data);<br>
> + read_ccnt(info, CCNT_NEG2, &data);<br>
> + read_ccnt(info, CCNT_SPOS, &data);<br>
> + read_ccnt(info, CCNT_SNEG, &data);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/* Calculate Open Circuit Voltage */<br>
> +static int calc_ocv(struct pm860x_battery_info *info, int *ocv)<br>
> +{<br>
> + int ret, i, data;<br>
> + int vbatt_avg, vbatt_sum, ibatt_avg, ibatt_sum;<br>
<br>
</div>here too.<br>
<div><div class="h5"><br>
> + if (!ocv)<br>
> + return -EINVAL;<br>
> +<br>
> + for (i = 0, ibatt_sum = 0, vbatt_sum = 0; i < 10; i++) {<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + vbatt_sum += data;<br>
> + ret = measure_current(info, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + ibatt_sum += data;<br>
> + }<br>
> + vbatt_avg = vbatt_sum / 10;<br>
> + ibatt_avg = ibatt_sum / 10;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + if (info->present)<br>
> + *ocv = vbatt_avg - ibatt_avg * info->resistor / 1000;<br>
> + else<br>
> + *ocv = vbatt_avg;<br>
> + mutex_unlock(&info->lock);<br>
> + dev_dbg(info->dev, "VBAT average:%d, OCV:%d\n", vbatt_avg, *ocv);<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +/* Calculate State of Charge (percent points) */<br>
> +static int calc_soc(struct pm860x_battery_info *info, int state, int *soc)<br>
> +{<br>
> + int i, ocv, count, ret = -EINVAL;<br>
<br>
</div></div>and here.<br>
<div class="im"><br>
> +<br>
> + if (!soc)<br>
> + return -EINVAL;<br>
> +<br>
> + switch (state) {<br>
> + case OCV_MODE_ACTIVE:<br>
> + ret = calc_ocv(info, &ocv);<br>
> + break;<br>
> + case OCV_MODE_SLEEP:<br>
> + ret = measure_vbatt(info, OCV_MODE_SLEEP, &ocv);<br>
> + break;<br>
> + }<br>
> + if (ret)<br>
> + goto out;<br>
<br>
</div>I think there is no need for the out label. You can just<br>
write if (ret) return ret;<br>
<div class="im"><br>
> +<br>
> + count = ARRAY_SIZE(array_soc);<br>
> + if (ocv < array_soc[count - 1][0]) {<br>
> + *soc = 0;<br>
> + return 0;<br>
> + }<br>
> +<br>
> + for (i = 0; i < count; i++) {<br>
> + if (ocv >= array_soc[i][0]) {<br>
> + *soc = array_soc[i][1];<br>
> + break;<br>
> + }<br>
> + }<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int calc_capacity(struct pm860x_battery_info *info, int *cap)<br>
> +{<br>
> + int ret, data, ibat;<br>
> + int cap_ocv = 0, cap_cc = 0;<br>
<br>
</div>One variable declaration per line please.<br>
<div><div class="h5"><br>
> + ret = calc_ccnt(info, &ccnt_data);<br>
> + if (ret)<br>
> + goto out;<br>
> +soc:<br>
> + data = info->max_capacity * info->start_soc / 100;<br>
> + if (ccnt_data.total_dischg - ccnt_data.total_chg <= data) {<br>
> + cap_cc =<br>
> + data + ccnt_data.total_chg - ccnt_data.total_dischg;<br>
> + } else {<br>
> + clear_ccnt(info, &ccnt_data);<br>
> + calc_soc(info, OCV_MODE_ACTIVE, &info->start_soc);<br>
> + dev_dbg(info->dev, "restart soc = %d !\n",<br>
> + info->start_soc);<br>
> + goto soc;<br>
> + }<br>
> +<br>
> + cap_cc = cap_cc * 100 / info->max_capacity;<br>
> + if (cap_cc < 0)<br>
> + cap_cc = 0;<br>
> + else if (cap_cc > 100)<br>
> + cap_cc = 100;<br>
> +<br>
> + dev_dbg(info->dev, "%s, last cap : %d", __func__,<br>
> + info->last_capacity);<br>
> +<br>
> + ret = measure_current(info, &ibat);<br>
> + if (ret)<br>
> + goto out;<br>
> + /* Calculate the capacity when discharging(ibat < 0) */<br>
> + if (ibat < 0) {<br>
> + ret = calc_soc(info, OCV_MODE_ACTIVE, &cap_ocv);<br>
> + if (ret)<br>
> + cap_ocv = info->last_capacity;<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + if (data <= LOW_BAT_THRESHOLD) {<br>
> + /* choose the lower capacity value to report<br>
> + * between vbat and CC when vbat < 3.6v;<br>
> + * than 3.6v;<br>
> + */<br>
> + *cap = min(cap_ocv, cap_cc);<br>
> + } else {<br>
> + /* when detect vbat > 3.6v, but cap_cc < 15,and<br>
> + * cap_ocv is 10% larger than cap_cc, we can think<br>
> + * CC have some accumulation error, switch to OCV<br>
> + * to estimate capacity;<br>
> + * */<br>
> + if (cap_cc < 15 && cap_ocv - cap_cc > 10)<br>
> + *cap = cap_ocv;<br>
> + else<br>
> + *cap = cap_cc;<br>
> + }<br>
> + /* when discharging, make sure current capacity<br>
> + * is lower than last*/<br>
> + if (*cap > info->last_capacity)<br>
> + *cap = info->last_capacity;<br>
> + } else {<br>
> + *cap = cap_cc;<br>
> + }<br>
> + info->last_capacity = *cap;<br>
> +<br>
> + dev_dbg(info->dev, "%s, cap_ocv:%d cap_cc:%d, cap:%d\n",<br>
> + (ibat < 0) ? "discharging" : "charging",<br>
> + cap_ocv, cap_cc, *cap);<br>
> + /*<br>
> + * store the current capacity to RTC domain register,<br>
> + * after next power up , it will be restored.<br>
> + */<br>
> + pm860x_set_bits(info->i2c, PM8607_RTC_MISC2, RTC_SOC_5LSB,<br>
> + (*cap & 0x1F) << 3);<br>
> + pm860x_set_bits(info->i2c, PM8607_RTC1, RTC_SOC_3MSB,<br>
> + ((*cap >> 5) & 0x3));<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static void pm860x_external_power_changed(struct power_supply *psy)<br>
> +{<br>
> + struct pm860x_battery_info *info;<br>
> +<br>
> + info = container_of(psy, struct pm860x_battery_info, battery);<br>
> + calc_resistor(info);<br>
> + return;<br>
<br>
</div></div>No need for return statement here.<br>
<div class="im"><br>
> +}<br>
> +<br>
> +static int pm860x_batt_get_prop(struct power_supply *psy,<br>
> + enum power_supply_property psp,<br>
> + union power_supply_propval *val)<br>
> +{<br>
> + struct pm860x_battery_info *info =<br>
> + dev_get_drvdata(psy->dev->parent);<br>
<br>
</div>No need for line wrap here.<br>
<br>
> + int data, ret;<br>
<br>
One variable per line.<br>
<div><div class="h5"><br>
> +<br>
> + switch (psp) {<br>
> + case POWER_SUPPLY_PROP_PRESENT:<br>
> + val->intval = info->present;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_CAPACITY:<br>
> + ret = calc_capacity(info, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + if (data < 0)<br>
> + data = 0;<br>
> + else if (data > 100)<br>
> + data = 100;<br>
> + /* return 100 if battery is not attached */<br>
> + if (!info->present)<br>
> + data = 100;<br>
> + val->intval = data;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_TECHNOLOGY:<br>
> + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:<br>
> + /* return real vbatt Voltage */<br>
> + ret = measure_vbatt(info, OCV_MODE_ACTIVE, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + val->intval = data * 1000;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_VOLTAGE_AVG:<br>
> + /* return Open Circuit Voltage (not measured voltage) */<br>
> + ret = calc_ocv(info, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + val->intval = data * 1000;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_CURRENT_NOW:<br>
> + ret = measure_current(info, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + val->intval = data;<br>
> + break;<br>
> + case POWER_SUPPLY_PROP_TEMP:<br>
> + if (info->present) {<br>
> + ret = measure_temp(info, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + data *= 10;<br>
> + } else {<br>
> + /* Fake Temp 25C Without Battery */<br>
> + data = 250;<br>
> + }<br>
> + val->intval = data;<br>
> + break;<br>
> + default:<br>
> + return -ENODEV;<br>
> + }<br>
> + return 0;<br>
> +out:<br>
<br>
</div></div>No need for the label here, you can just replace all 'goto out;'<br>
with 'return ret';<br>
<div class="im"><br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int pm860x_batt_set_prop(struct power_supply *psy,<br>
> + enum power_supply_property psp,<br>
> + const union power_supply_propval *val)<br>
> +{<br>
> + struct pm860x_battery_info *info =<br>
> + dev_get_drvdata(psy->dev->parent);<br>
<br>
</div>No need to wrap this line, it can fit into 80 chars limit.<br>
<div><div class="h5"><br>
> + switch (psp) {<br>
> + case POWER_SUPPLY_PROP_CHARGE_FULL:<br>
> + clear_ccnt(info, &ccnt_data);<br>
> + info->start_soc = 100;<br>
> + dev_dbg(info->dev, "chg done, update soc = %d\n",<br>
> + info->start_soc);<br>
> + break;<br>
> + default:<br>
> + return -EPERM;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +<br>
> +static enum power_supply_property pm860x_batt_props[] = {<br>
> + POWER_SUPPLY_PROP_PRESENT,<br>
> + POWER_SUPPLY_PROP_CAPACITY,<br>
> + POWER_SUPPLY_PROP_TECHNOLOGY,<br>
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,<br>
> + POWER_SUPPLY_PROP_VOLTAGE_AVG,<br>
> + POWER_SUPPLY_PROP_CURRENT_NOW,<br>
> + POWER_SUPPLY_PROP_TEMP,<br>
> +};<br>
> +<br>
> +static __devinit int pm860x_battery_probe(struct platform_device *pdev)<br>
> +{<br>
> + struct pm860x_chip *chip = dev_get_drvdata(pdev->dev.parent);<br>
> + struct pm860x_battery_info *info;<br>
> + struct pm860x_power_pdata *pdata;<br>
> + int ret;<br>
> +<br>
> + info = kzalloc(sizeof(struct pm860x_battery_info), GFP_KERNEL);<br>
<br>
</div></div>Please use sizeof(*info), and devm_kzalloc();<br>
<div class="im"><br>
> + if (!info)<br>
> + return -ENOMEM;<br>
<br>
</div>An empty line here would look nice. :-)<br>
<div class="im"><br>
> + info->irq_cc = platform_get_irq(pdev, 0);<br>
> + if (info->irq_cc < 0) {<br>
> + dev_err(&pdev->dev, "No IRQ resource!\n");<br>
> + ret = -EINVAL;<br>
> + goto out;<br>
> + }<br>
<br>
</div>Here too.<br>
<div class="im"><br>
> + info->irq_batt = platform_get_irq(pdev, 1);<br>
> + if (info->irq_batt < 0) {<br>
<br>
</div>irq == 0 is also invalid, so the check should be <= 0<br>
<div class="im"><br>
> + dev_err(&pdev->dev, "No IRQ resource!\n");<br>
> + ret = -EINVAL;<br>
> + goto out;<br>
> + }<br>
> +<br>
> + info->chip = chip;<br>
> + info->i2c =<br>
> + (chip->id == CHIP_PM8607) ? chip->client : chip->companion;<br>
> + info->dev = &pdev->dev;<br>
> + info->status = POWER_SUPPLY_STATUS_UNKNOWN;<br>
> + pdata = pdev->dev.platform_data;<br>
> +<br>
> + ret = request_threaded_irq(info->irq_cc, NULL,<br>
> + pm860x_coulomb_handler, IRQF_ONESHOT,<br>
> + "coulomb", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq_cc, ret);<br>
> + goto out;<br>
> + }<br>
<br>
</div>empty line here?<br>
<div class="im"><br>
> + ret = request_threaded_irq(info->irq_batt, NULL, pm860x_batt_handler,<br>
> + IRQF_ONESHOT, "battery", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq_batt, ret);<br>
> + goto out_coulomb;<br>
> + }<br>
<br>
</div>You actually want to request irqs only after you sucessfuly<br>
initialized and registered the power supply (so, request_irq()<br>
usually is the last step in the probe routine), otherwise if<br>
interrupt arrives before power_supply object usable, bad things<br>
may happen.<br>
<div><div class="h5"><br>
> +<br>
> + mutex_init(&info->lock);<br>
> + platform_set_drvdata(pdev, info);<br>
> +<br>
> + pm860x_init_battery(info);<br>
> +<br>
> + info-><a href="http://battery.name" target="_blank">battery.name</a> = "battery-monitor";<br>
> + info->battery.type = POWER_SUPPLY_TYPE_BATTERY;<br>
> + info->battery.properties = pm860x_batt_props;<br>
> + info->battery.num_properties = ARRAY_SIZE(pm860x_batt_props);<br>
> + info->battery.get_property = pm860x_batt_get_prop;<br>
> + info->battery.set_property = pm860x_batt_set_prop;<br>
> + info->battery.external_power_changed = pm860x_external_power_changed;<br>
> +<br>
> + if (pdata && pdata->max_capacity)<br>
> + info->max_capacity = pdata->max_capacity;<br>
> + else<br>
> + info->max_capacity = 1500; /* set default capacity */<br>
> + if (pdata && pdata->resistor)<br>
> + info->resistor = pdata->resistor;<br>
> + else<br>
> + info->resistor = 300; /* set default internal resistor */<br>
> +<br>
> + ret = power_supply_register(&pdev->dev, &info->battery);<br>
> + if (ret)<br>
> + goto out_reg;<br>
> + info->battery.dev->parent = &pdev->dev;<br>
> +<br>
> + return 0;<br>
> +<br>
> +out_reg:<br>
> + free_irq(info->irq_batt, info);<br>
> +out_coulomb:<br>
> + free_irq(info->irq_cc, info);<br>
> +out:<br>
> + kfree(info);<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int __devexit pm860x_battery_remove(struct platform_device *pdev)<br>
> +{<br>
> + struct pm860x_battery_info *info = platform_get_drvdata(pdev);<br>
> +<br>
> + power_supply_unregister(&info->battery);<br>
> + free_irq(info->irq_batt, info);<br>
> + free_irq(info->irq_cc, info);<br>
> + kfree(info);<br>
> + platform_set_drvdata(pdev, NULL);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +#ifdef CONFIG_PM_SLEEP<br>
> +static int pm860x_battery_suspend(struct device *dev)<br>
> +{<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int pm860x_battery_resume(struct device *dev)<br>
> +{<br>
> + return 0;<br>
> +}<br>
> +#endif<br>
> +static SIMPLE_DEV_PM_OPS(pm860x_battery_pm_ops, pm860x_battery_suspend,<br>
> + pm860x_battery_resume);<br>
<br>
</div></div>No need for these empty callbacks.<br>
<div class="im"><br>
> +static struct platform_driver pm860x_battery_driver = {<br>
> + .driver = {<br>
> + .name = "88pm860x-battery",<br>
> + .owner = THIS_MODULE,<br>
> + .pm = &pm860x_battery_pm_ops,<br>
> + },<br>
<br>
</div>Wrong indentation for the closing bracket.<br>
<div class="im"><br>
> + .probe = pm860x_battery_probe,<br>
> + .remove = __devexit_p(pm860x_battery_remove),<br>
> +};<br>
> +<br>
<br>
</div>please no empty line here<br>
<div><div class="h5"><br>
> +module_platform_driver(pm860x_battery_driver);<br>
> +<br>
> +MODULE_DESCRIPTION("Marvell 88PM860x Battery driver");<br>
> +MODULE_LICENSE("GPL");<br>
> diff --git a/drivers/power/88pm860x_charger.c b/drivers/power/88pm860x_charger.c<br>
> new file mode 100644<br>
> index 0000000..0089ad4<br>
> --- /dev/null<br>
> +++ b/drivers/power/88pm860x_charger.c<br>
> @@ -0,0 +1,833 @@<br>
> +/*<br>
> + * Battery driver for Marvell 88PM860x PMIC<br>
> + *<br>
> + * Copyright (c) 2012 Marvell International Ltd.<br>
> + * Author: Jett Zhou <<a href="mailto:jtzhou@marvell.com">jtzhou@marvell.com</a>><br>
> + * Haojian Zhuang <<a href="mailto:haojian.zhuang@marvell.com">haojian.zhuang@marvell.com</a>><br>
> + *<br>
> + * This program is free software; you can redistribute it and/or modify<br>
> + * it under the terms of the GNU General Public License version 2 as<br>
> + * published by the Free Software Foundation.<br>
> + */<br>
> +<br>
> +#include <linux/kernel.h><br>
> +#include <linux/module.h><br>
> +#include <linux/platform_device.h><br>
> +#include <linux/slab.h><br>
> +#include <linux/power_supply.h><br>
> +#include <linux/mfd/88pm860x.h><br>
> +#include <linux/delay.h><br>
> +#include <linux/uaccess.h><br>
> +#include <asm/div64.h><br>
> +<br>
> +<br>
<br>
</div></div>No need for two empty lines.<br>
<div><div class="h5"><br>
> +/* bit definitions of Status Query Interface 2 */<br>
> +#define STATUS2_CHG (1 << 2)<br>
> +<br>
> +/* bit definitions of Reset Out Register */<br>
> +#define RESET_SW_PD (1 << 7)<br>
> +<br>
> +/* bit definitions of PreReg 1 */<br>
> +#define PREREG1_90MA (0x0)<br>
> +#define PREREG1_180MA (0x1)<br>
> +#define PREREG1_450MA (0x4)<br>
> +#define PREREG1_540MA (0x5)<br>
> +#define PREREG1_1350MA (0xE)<br>
> +#define PREREG1_VSYS_4_5V (3 << 4)<br>
> +<br>
> +/* bit definitions of Charger Control 1 Register */<br>
> +#define CC1_MODE_OFF (0)<br>
> +#define CC1_MODE_PRECHARGE (1)<br>
> +#define CC1_MODE_FASTCHARGE (2)<br>
> +#define CC1_MODE_PULSECHARGE (3)<br>
> +#define CC1_ITERM_20MA (0 << 2)<br>
> +#define CC1_ITERM_60MA (2 << 2)<br>
> +#define CC1_VFCHG_4_2V (9 << 4)<br>
> +<br>
> +/* bit definitions of Charger Control 2 Register */<br>
> +#define CC2_ICHG_100MA (0x1)<br>
> +#define CC2_ICHG_500MA (0x9)<br>
> +#define CC2_ICHG_1000MA (0x13)<br>
> +<br>
> +/* bit definitions of Charger Control 3 Register */<br>
> +#define CC3_180MIN_TIMEOUT (0x6 << 4)<br>
> +#define CC3_270MIN_TIMEOUT (0x7 << 4)<br>
> +#define CC3_360MIN_TIMEOUT (0xA << 4)<br>
> +#define CC3_DISABLE_TIMEOUT (0xF << 4)<br>
> +<br>
> +/* bit definitions of Charger Control 4 Register */<br>
> +#define CC4_IPRE_40MA (7)<br>
> +#define CC4_VPCHG_3_2V (3 << 4)<br>
> +#define CC4_IFCHG_MON_EN (1 << 6)<br>
> +#define CC4_BTEMP_MON_EN (1 << 7)<br>
> +<br>
> +/* bit definitions of Charger Control 6 Register */<br>
> +#define CC6_BAT_OV_EN (1 << 2)<br>
> +#define CC6_BAT_UV_EN (1 << 3)<br>
> +#define CC6_UV_VBAT_SET (0x3 << 6) /* 2.8v */<br>
> +<br>
> +/* bit definitions of Charger Control 7 Register */<br>
> +#define CC7_BAT_REM_EN (1 << 3)<br>
> +#define CC7_IFSM_EN (1 << 7)<br>
> +<br>
> +/* bit definitions of Measurement Enable 1 Register */<br>
> +#define MEAS1_VBAT (1 << 0)<br>
> +<br>
> +/* bit definitions of Measurement Enable 3 Register */<br>
> +#define MEAS3_IBAT_EN (1 << 0)<br>
> +#define MEAS3_CC_EN (1 << 2)<br>
> +<br>
> +#define FSM_INIT 0<br>
> +#define FSM_DISCHARGE 1<br>
> +#define FSM_PRECHARGE 2<br>
> +#define FSM_FASTCHARGE 3<br>
> +<br>
> +#define PRECHARGE_THRESHOLD 3100<br>
> +#define POWEROFF_THRESHOLD 3400<br>
> +#define CHARGE_THRESHOLD 4000<br>
> +#define DISCHARGE_THRESHOLD 4180<br>
> +<br>
> +/* over-temperature on PM8606 setting */<br>
> +#define OVER_TEMP_FLAG (1 << 6)<br>
> +#define OVTEMP_AUTORECOVER (1 << 3)<br>
> +<br>
> +/* over-voltage protect on vchg setting mv */<br>
> +#define VCHG_NORMAL_LOW 4200<br>
> +#define VCHG_NORMAL_CHECK 5800<br>
> +#define VCHG_NORMAL_HIGH 6000<br>
> +#define VCHG_OVP_LOW 5500<br>
> +<br>
> +struct pm860x_charger_info {<br>
> + struct pm860x_chip *chip;<br>
> + struct i2c_client *i2c;<br>
> + struct i2c_client *i2c_8606;<br>
> + struct device *dev;<br>
> +<br>
> + struct power_supply usb;<br>
> + struct mutex lock;<br>
> + int irq_nums;<br>
> + int irq[7];<br>
> + unsigned state:3; /* fsm state */<br>
> + unsigned online:1; /* usb charger */<br>
> + unsigned present:1; /* battery present */<br>
> + unsigned allowed:1;<br>
> +};<br>
> +<br>
> +static char *pm860x_supplied_to[] = {<br>
> + "battery-monitor",<br>
> +};<br>
> +<br>
> +static int stop_charge(struct pm860x_charger_info *info, int vbatt);<br>
> +static int set_charging_fsm(struct pm860x_charger_info *info);<br>
> +static void set_vbatt_threshold(struct pm860x_charger_info *info,<br>
> + int min, int max);<br>
> +static void set_vchg_threshold(struct pm860x_charger_info *info,<br>
> + int min, int max);<br>
> +static int measure_vchg(struct pm860x_charger_info *info, int *data);<br>
<br>
</div></div>The same suggestion: try to reorder functions to avoid forward<br>
declarations.<br>
<div><div class="h5"><br>
> +static irqreturn_t pm860x_charger_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_charger_info *info = data;<br>
> + int ret;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + ret = pm860x_reg_read(info->i2c, PM8607_STATUS_2);<br>
> + if (ret < 0) {<br>
> + mutex_unlock(&info->lock);<br>
> + goto out;<br>
> + }<br>
> + if (ret & STATUS2_CHG) {<br>
> + info->online = 1;<br>
> + info->allowed = 1;<br>
> + } else {<br>
> + info->online = 0;<br>
> + info->allowed = 0;<br>
> + }<br>
> + mutex_unlock(&info->lock);<br>
> + dev_dbg(info->dev, "%s, Charger:%s, Allowed:%d\n", __func__,<br>
> + (info->online) ? "online" : "N/A", info->allowed);<br>
> +<br>
> + set_charging_fsm(info);<br>
> +<br>
> + power_supply_changed(&info->usb);<br>
> +out:<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_temp_handler(int irq, void *data)<br>
> +{<br>
> + struct power_supply *psy;<br>
> + struct pm860x_charger_info *info = data;<br>
> + union power_supply_propval temp;<br>
> + int value, ret = -EINVAL;<br>
<br>
</div></div>no need for the initializer for ret.<br>
plus again, one variable per line.<br>
<div><div class="h5"><br>
> +<br>
> + psy = power_supply_get_by_name(pm860x_supplied_to[0]);<br>
> + if (!psy)<br>
> + goto out;<br>
> + ret = psy->get_property(psy, POWER_SUPPLY_PROP_TEMP, &temp);<br>
> + if (ret)<br>
> + goto out;<br>
> + value = temp.intval / 10;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + /* Temperature < -10 C or >40 C, Will not allow charge */<br>
> + if (value < -10 || value > 40)<br>
> + info->allowed = 0;<br>
> + else<br>
> + info->allowed = 1;<br>
> + dev_dbg(info->dev, "%s, Allowed: %d\n", __func__, info->allowed);<br>
> + mutex_unlock(&info->lock);<br>
> +<br>
> + set_charging_fsm(info);<br>
> +out:<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_exception_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_charger_info *info = data;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + info->allowed = 0;<br>
> + mutex_unlock(&info->lock);<br>
> + dev_dbg(info->dev, "%s, irq: %d\n", __func__, irq);<br>
> +<br>
> + set_charging_fsm(info);<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_done_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_charger_info *info = data;<br>
> + struct power_supply *psy;<br>
> + union power_supply_propval val;<br>
> + int ret, vbatt;<br>
<br>
</div></div>int ret;<br>
int vbatt;<br>
<div><div class="h5"><br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + /* pre-charge done, will transimit to fast-charge stage */<br>
> + if (info->state == FSM_PRECHARGE) {<br>
> + info->allowed = 1;<br>
> + goto out;<br>
> + }<br>
> + /*<br>
> + * Fast charge done, delay to read<br>
> + * the correct status of CHG_DET.<br>
> + */<br>
> + mdelay(5);<br>
> + info->allowed = 0;<br>
> + psy = power_supply_get_by_name(pm860x_supplied_to[0]);<br>
> + if (!psy)<br>
> + goto out;<br>
> + ret = psy->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &val);<br>
> + if (ret)<br>
> + goto out;<br>
> + vbatt = val.intval / 1000;<br>
> + /*<br>
> + * CHG_DONE interrupt is faster than CHG_DET interrupt when<br>
> + * plug in/out usb, So we can not rely on info->online, we<br>
> + * need check pm8607 status register to check usb is online<br>
> + * or not, then we can decide it is real charge done<br>
> + * automatically or it is triggered by usb plug out;<br>
> + */<br>
> + ret = pm860x_reg_read(info->i2c, PM8607_STATUS_2);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + if (vbatt > CHARGE_THRESHOLD && ret & STATUS2_CHG)<br>
> + psy->set_property(psy, POWER_SUPPLY_PROP_CHARGE_FULL, &val);<br>
> +<br>
> +out:<br>
> + mutex_unlock(&info->lock);<br>
> + dev_dbg(info->dev, "%s, Allowed: %d\n", __func__, info->allowed);<br>
> + set_charging_fsm(info);<br>
> +<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_vbattery_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_charger_info *info = data;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> +<br>
> + set_vbatt_threshold(info, 0, 0);<br>
> +<br>
> + if (info->present && info->online)<br>
> + info->allowed = 1;<br>
> + else<br>
> + info->allowed = 0;<br>
> + mutex_unlock(&info->lock);<br>
> + dev_dbg(info->dev, "%s, Allowed: %d\n", __func__, info->allowed);<br>
> +<br>
> + set_charging_fsm(info);<br>
> +<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static irqreturn_t pm860x_vchg_handler(int irq, void *data)<br>
> +{<br>
> + struct pm860x_charger_info *info = data;<br>
> + int vchg = 0, status = 0;<br>
<br>
</div></div>No need for the initializer for status. Plus you can move satus<br>
variable declaration under the "if (!info->online) {" block.<br>
<div><div class="h5"><br>
> +<br>
> + if (info->present)<br>
> + goto out;<br>
> +<br>
> + measure_vchg(info, &vchg);<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + if (!info->online) {<br>
> + /* check if over-temp on pm8606 or not */<br>
> + status = pm860x_reg_read(info->i2c_8606, PM8606_FLAGS);<br>
> + if (status & OVER_TEMP_FLAG) {<br>
> + /* clear over temp flag and set auto recover */<br>
> + pm860x_set_bits(info->i2c_8606, PM8606_FLAGS,<br>
> + OVER_TEMP_FLAG, OVER_TEMP_FLAG);<br>
> + pm860x_set_bits(info->i2c_8606,<br>
> + PM8606_VSYS,<br>
> + OVTEMP_AUTORECOVER,<br>
> + OVTEMP_AUTORECOVER);<br>
> + dev_dbg(info->dev,<br>
> + "%s, pm8606 over-temp occure\n", __func__);<br>
> + }<br>
> + }<br>
> +<br>
> + if (vchg > VCHG_NORMAL_CHECK) {<br>
> + set_vchg_threshold(info, VCHG_OVP_LOW, 0);<br>
> + info->allowed = 0;<br>
> + dev_dbg(info->dev,<br>
> + "%s,pm8607 over-vchg occure,vchg = %dmv\n",<br>
> + __func__, vchg);<br>
> + } else if (vchg < VCHG_OVP_LOW) {<br>
> + set_vchg_threshold(info, VCHG_NORMAL_LOW,<br>
> + VCHG_NORMAL_HIGH);<br>
> + info->allowed = 1;<br>
> + dev_dbg(info->dev,<br>
> + "%s,pm8607 over-vchg recover,vchg = %dmv\n",<br>
> + __func__, vchg);<br>
> + }<br>
> + mutex_unlock(&info->lock);<br>
> +<br>
> + dev_dbg(info->dev, "%s, Allowed: %d\n", __func__, info->allowed);<br>
> + set_charging_fsm(info);<br>
> +out:<br>
> + return IRQ_HANDLED;<br>
> +}<br>
> +<br>
> +static ssize_t stop_charging(struct device *dev,<br>
> + struct device_attribute *attr,<br>
> + const char *buf, size_t count)<br>
> +{<br>
> + struct pm860x_charger_info *info = dev_get_drvdata(dev);<br>
> + int disable;<br>
> +<br>
> + sscanf(buf, "%d", &disable);<br>
> + if (disable && info) {<br>
> + dev_dbg(info->dev, "stop charging by manual\n");<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + info->allowed = 0;<br>
> + mutex_unlock(&info->lock);<br>
> + set_charging_fsm(info);<br>
> + }<br>
> + return strnlen(buf, PAGE_SIZE);<br>
> +}<br>
> +<br>
> +static DEVICE_ATTR(stop, S_IWUSR, NULL, stop_charging);<br>
<br>
</div></div>Um... can we handle this via set_property() for PROP_STATUS?<br>
E.g. if set to "Not charging" you manually disable it. And if set<br>
for any other value, enable it.<br>
<br>
If so, would be nice to have it documented in<br>
Documentation/power/power_supply_class.txt.<br>
<div class="im"><br>
> +static int pm860x_usb_get_prop(struct power_supply *psy,<br>
> + enum power_supply_property psp,<br>
> + union power_supply_propval *val)<br>
> +{<br>
> + struct pm860x_charger_info *info =<br>
> + dev_get_drvdata(psy->dev->parent);<br>
> +<br>
> + switch (psp) {<br>
> + case POWER_SUPPLY_PROP_STATUS:<br>
> + val->intval = (info->state == FSM_FASTCHARGE ||<br>
> + info->state == FSM_PRECHARGE);<br>
<br>
</div>This assumes that POWER_SUPPLY_STATUS_CHARGING will always be '1',<br>
but it is not true. It's better to assign enum value explicitly.<br>
<br>
Plus, if not in fastcharge or precharge state, it will return<br>
0, which is POWER_SUPPLY_STATUS_UNKNOWN. Is that really what<br>
you want here?<br>
<div><div class="h5"><br>
> + break;<br>
> + case POWER_SUPPLY_PROP_ONLINE:<br>
> + val->intval = info->online;<br>
> + break;<br>
> + default:<br>
> + return -ENODEV;<br>
> + }<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static enum power_supply_property pm860x_usb_props[] = {<br>
> + POWER_SUPPLY_PROP_STATUS,<br>
> + POWER_SUPPLY_PROP_ONLINE,<br>
> +};<br>
> +<br>
> +static int measure_vchg(struct pm860x_charger_info *info, int *data)<br>
> +{<br>
> + unsigned char buf[2];<br>
> + int ret = 0;<br>
> +<br>
> + ret = pm860x_bulk_read(info->i2c, PM8607_VCHG_MEAS1, 2, buf);<br>
> + if (ret < 0)<br>
> + return ret;<br>
> +<br>
> + *data = ((buf[0] & 0xff) << 4) | (buf[1] & 0x0f);<br>
> + /* V_BATT_MEAS(mV) = value * 5 * 1.8 * 1000 / (2^12) */<br>
> + *data = ((*data & 0xfff) * 9 * 125) >> 9;<br>
> +<br>
> + dev_dbg(info->dev, "%s, vchg: %d mv\n", __func__, *data);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static void set_vchg_threshold(struct pm860x_charger_info *info,<br>
> + int min, int max)<br>
> +{<br>
> + int data;<br>
> +<br>
> + /* (tmp << 8) * / 5 / 1800 */<br>
> + if (min <= 0)<br>
> + data = 0;<br>
> + else<br>
> + data = (min << 5) / 1125;<br>
> + pm860x_reg_write(info->i2c, PM8607_VCHG_LOWTH, data);<br>
> + dev_dbg(info->dev, "VCHG_LOWTH:%dmv, 0x%x\n", min, data);<br>
> +<br>
> + if (max <= 0)<br>
> + data = 0xff;<br>
> + else<br>
> + data = (max << 5) / 1125;<br>
> + pm860x_reg_write(info->i2c, PM8607_VCHG_HIGHTH, data);<br>
> + dev_dbg(info->dev, "VCHG_HIGHTH:%dmv, 0x%x\n", max, data);<br>
> +<br>
> +}<br>
> +<br>
> +static void set_vbatt_threshold(struct pm860x_charger_info *info,<br>
> + int min, int max)<br>
> +{<br>
> + int data;<br>
> +<br>
> + /* (tmp << 8) * 3 / 1800 */<br>
> + if (min <= 0)<br>
> + data = 0;<br>
> + else<br>
> + data = (min << 5) / 675;<br>
> + pm860x_reg_write(info->i2c, PM8607_VBAT_LOWTH, data);<br>
> + dev_dbg(info->dev, "VBAT Min:%dmv, LOWTH:0x%x\n", min, data);<br>
> +<br>
> + if (max <= 0)<br>
> + data = 0xff;<br>
> + else<br>
> + data = (max << 5) / 675;<br>
> + pm860x_reg_write(info->i2c, PM8607_VBAT_HIGHTH, data);<br>
> + dev_dbg(info->dev, "VBAT Max:%dmv, HIGHTH:0x%x\n", max, data);<br>
> +<br>
> + return;<br>
> +}<br>
> +<br>
> +static int start_precharge(struct pm860x_charger_info *info)<br>
> +{<br>
> + int ret;<br>
> +<br>
> + dev_dbg(info->dev, "Start Pre-charging!\n");<br>
> + set_vbatt_threshold(info, 0, 0);<br>
> +<br>
> + ret = pm860x_reg_write(info->i2c_8606, PM8606_PREREGULATORA,<br>
> + PREREG1_1350MA | PREREG1_VSYS_4_5V);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* stop charging */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL1, 3,<br>
> + CC1_MODE_OFF);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* set 270 minutes timeout */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL3, (0xf << 4),<br>
> + CC3_270MIN_TIMEOUT);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* set precharge current, termination voltage, IBAT & TBAT monitor */<br>
> + ret = pm860x_reg_write(info->i2c, PM8607_CHG_CTRL4,<br>
> + CC4_IPRE_40MA | CC4_VPCHG_3_2V |<br>
> + CC4_IFCHG_MON_EN | CC4_BTEMP_MON_EN);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL7,<br>
> + CC7_BAT_REM_EN | CC7_IFSM_EN,<br>
> + CC7_BAT_REM_EN | CC7_IFSM_EN);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* trigger precharge */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL1, 3,<br>
> + CC1_MODE_PRECHARGE);<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int start_fastcharge(struct pm860x_charger_info *info)<br>
> +{<br>
> + int ret;<br>
> +<br>
> + dev_dbg(info->dev, "Start Fast-charging!\n");<br>
> +<br>
> + /* set fastcharge termination current & voltage, disable charging */<br>
> + ret = pm860x_reg_write(info->i2c, PM8607_CHG_CTRL1,<br>
> + CC1_MODE_OFF | CC1_ITERM_60MA |<br>
> + CC1_VFCHG_4_2V);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_reg_write(info->i2c_8606, PM8606_PREREGULATORA,<br>
> + PREREG1_540MA | PREREG1_VSYS_4_5V);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL2, 0x1f,<br>
> + CC2_ICHG_500MA);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* set 270 minutes timeout */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL3, (0xf << 4),<br>
> + CC3_270MIN_TIMEOUT);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* set IBAT & TBAT monitor */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL4,<br>
> + CC4_IFCHG_MON_EN | CC4_BTEMP_MON_EN,<br>
> + CC4_IFCHG_MON_EN | CC4_BTEMP_MON_EN);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL6,<br>
> + CC6_BAT_OV_EN | CC6_BAT_UV_EN |<br>
> + CC6_UV_VBAT_SET,<br>
> + CC6_BAT_OV_EN | CC6_BAT_UV_EN |<br>
> + CC6_UV_VBAT_SET);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL7,<br>
> + CC7_BAT_REM_EN | CC7_IFSM_EN,<br>
> + CC7_BAT_REM_EN | CC7_IFSM_EN);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> + /* launch fast-charge */<br>
> + ret = pm860x_set_bits(info->i2c, PM8607_CHG_CTRL1, 3,<br>
> + CC1_MODE_FASTCHARGE);<br>
> + /* vchg threshold setting */<br>
> + set_vchg_threshold(info, VCHG_NORMAL_LOW, VCHG_NORMAL_HIGH);<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int stop_charge(struct pm860x_charger_info *info, int vbatt)<br>
> +{<br>
> + dev_dbg(info->dev, "Stop charging!\n");<br>
> + pm860x_set_bits(info->i2c, PM8607_CHG_CTRL1, 3, CC1_MODE_OFF);<br>
> + if (vbatt > CHARGE_THRESHOLD && info->online)<br>
> + set_vbatt_threshold(info, CHARGE_THRESHOLD, 0);<br>
> + return 0;<br>
<br>
</div></div>The return value of the function can be changed to void.<br>
<div class="im"><br>
> +}<br>
> +<br>
> +static int power_off_notification(struct pm860x_charger_info *info)<br>
> +{<br>
> + dev_dbg(info->dev, "Power-off notification!\n");<br>
> + return 0;<br>
<br>
</div>Return value can be void, no need for int.<br>
<div><div class="h5"><br>
> +}<br>
> +<br>
> +static int set_charging_fsm(struct pm860x_charger_info *info)<br>
> +{<br>
> + struct power_supply *psy;<br>
> + union power_supply_propval data;<br>
> + unsigned char fsm_state[][16] = { "init", "discharge", "precharge",<br>
> + "fastcharge",<br>
> + };<br>
> + int ret = -EINVAL;<br>
> + int vbatt;<br>
> +<br>
> + psy = power_supply_get_by_name(pm860x_supplied_to[0]);<br>
> + if (!psy)<br>
> + goto out;<br>
> + ret = psy->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> + vbatt = data.intval / 1000;<br>
> +<br>
> + ret = psy->get_property(psy, POWER_SUPPLY_PROP_PRESENT, &data);<br>
> + if (ret)<br>
> + goto out;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + info->present = data.intval;<br>
> +<br>
> + dev_dbg(info->dev, "Entering FSM:%s, Charger:%s, Battery:%s, "<br>
> + "Allowed:%d\n",<br>
> + &fsm_state[info->state][0],<br>
> + (info->online) ? "online" : "N/A",<br>
> + (info->present) ? "present" : "N/A", info->allowed);<br>
> + dev_dbg(info->dev, "set_charging_fsm:vbatt:%d(mV)\n", vbatt);<br>
> +<br>
> + switch (info->state) {<br>
> + case FSM_INIT:<br>
> + if (info->online && info->present && info->allowed) {<br>
> + if (vbatt < PRECHARGE_THRESHOLD) {<br>
> + info->state = FSM_PRECHARGE;<br>
> + start_precharge(info);<br>
> + } else if (vbatt > DISCHARGE_THRESHOLD) {<br>
> + info->state = FSM_DISCHARGE;<br>
> + stop_charge(info, vbatt);<br>
> + } else if (vbatt < DISCHARGE_THRESHOLD) {<br>
> + info->state = FSM_FASTCHARGE;<br>
> + start_fastcharge(info);<br>
> + }<br>
> + } else {<br>
> + if (vbatt < POWEROFF_THRESHOLD) {<br>
> + power_off_notification(info);<br>
> + } else {<br>
> + info->state = FSM_DISCHARGE;<br>
> + stop_charge(info, vbatt);<br>
> + }<br>
> + }<br>
> + break;<br>
> + case FSM_PRECHARGE:<br>
> + if (info->online && info->present && info->allowed) {<br>
> + if (vbatt > PRECHARGE_THRESHOLD) {<br>
> + info->state = FSM_FASTCHARGE;<br>
> + start_fastcharge(info);<br>
> + }<br>
> + } else {<br>
> + info->state = FSM_DISCHARGE;<br>
> + stop_charge(info, vbatt);<br>
> + }<br>
> + break;<br>
> + case FSM_FASTCHARGE:<br>
> + if (info->online && info->present && info->allowed) {<br>
> + if (vbatt < PRECHARGE_THRESHOLD) {<br>
> + info->state = FSM_PRECHARGE;<br>
> + start_precharge(info);<br>
> + }<br>
> + } else {<br>
> + info->state = FSM_DISCHARGE;<br>
> + stop_charge(info, vbatt);<br>
> + }<br>
> + break;<br>
> + case FSM_DISCHARGE:<br>
> + if (info->online && info->present && info->allowed) {<br>
> + if (vbatt < PRECHARGE_THRESHOLD) {<br>
> + info->state = FSM_PRECHARGE;<br>
> + start_precharge(info);<br>
> + } else if (vbatt < DISCHARGE_THRESHOLD) {<br>
> + info->state = FSM_FASTCHARGE;<br>
> + start_fastcharge(info);<br>
> + }<br>
> + } else {<br>
> + if (vbatt < POWEROFF_THRESHOLD) {<br>
> + power_off_notification(info);<br>
> + } else if (vbatt > CHARGE_THRESHOLD<br>
> + && info->online)<br>
> + set_vbatt_threshold(info, CHARGE_THRESHOLD,<br>
> + 0);<br>
> + }<br>
> + break;<br>
> + default:<br>
> + dev_warn(info->dev, "FSM meets wrong state:%d\n",<br>
> + info->state);<br>
> + break;<br>
> + }<br>
> + dev_dbg(info->dev,<br>
> + "Out FSM:%s, Charger:%s, Battery:%s, Allowed:%d\n",<br>
> + &fsm_state[info->state][0],<br>
> + (info->online) ? "online" : "N/A",<br>
> + (info->present) ? "present" : "N/A", info->allowed);<br>
> + mutex_unlock(&info->lock);<br>
> +<br>
> + return 0;<br>
> +out:<br>
<br>
</div></div>No need for out label here. Just write 'return ret', instead of 'goto out'.<br>
That way you also won't need the initializer value for ret.<br>
<div class="im"><br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int pm860x_init_charger(struct pm860x_charger_info *info)<br>
> +{<br>
> + int ret;<br>
> +<br>
> + mutex_lock(&info->lock);<br>
> + info->state = FSM_INIT;<br>
> + ret = pm860x_reg_read(info->i2c, PM8607_STATUS_2);<br>
> + if (ret < 0)<br>
> + goto out;<br>
<br>
</div>The 'out:' branch is missing mutex_unlock().<br>
<br>
But I believe it is safe to read i2c devices w/o locking, so you can<br>
move pm860x_reg_read() out of the lock, into the beginning of<br>
the function; that way you won't need the 'out:' label anymore.<br>
<div class="im"><br>
> + if (ret & STATUS2_CHG) {<br>
> + info->online = 1;<br>
> + info->allowed = 1;<br>
> + } else {<br>
> + info->online = 0;<br>
> + info->allowed = 0;<br>
> + }<br>
> + mutex_unlock(&info->lock);<br>
> +<br>
> + set_charging_fsm(info);<br>
> + return 0;<br>
> +out:<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static __devinit int pm860x_charger_probe(struct platform_device *pdev)<br>
> +{<br>
> + struct pm860x_chip *chip = dev_get_drvdata(pdev->dev.parent);<br>
> + struct pm860x_charger_info *info;<br>
> + int ret, i, j, count;<br>
<br>
</div>One variable per line;<br>
<div class="im"><br>
> +<br>
> + info = kzalloc(sizeof(struct pm860x_charger_info), GFP_KERNEL);<br>
<br>
</div>Please switch to sizeof(*info) and devm_kzalloc();<br>
<div><div class="h5"><br>
> + if (!info)<br>
> + return -ENOMEM;<br>
> +<br>
> + ret = device_create_file(&pdev->dev, &dev_attr_stop);<br>
> + if (ret < 0)<br>
> + goto out;<br>
> +<br>
> + count = pdev->num_resources;<br>
> + for (i = 0, j = 0; i < count; i++) {<br>
> + info->irq[j] = platform_get_irq(pdev, i);<br>
> + if (info->irq[j] < 0)<br>
> + continue;<br>
> + j++;<br>
> + }<br>
> + info->irq_nums = j;<br>
> +<br>
> + info->chip = chip;<br>
> + info->i2c =<br>
> + (chip->id == CHIP_PM8607) ? chip->client : chip->companion;<br>
> + info->i2c_8606 =<br>
> + (chip->id == CHIP_PM8607) ? chip->companion : chip->client;<br>
> + if (!info->i2c_8606) {<br>
> + dev_err(&pdev->dev, "Missed I2C address of 88PM8606!\n");<br>
> + ret = -EINVAL;<br>
> + goto out_dev;<br>
> + }<br>
> + info->dev = &pdev->dev;<br>
> +<br>
> + /* set init value for the case we are not using battery */<br>
> + set_vchg_threshold(info, VCHG_NORMAL_LOW, VCHG_OVP_LOW);<br>
> +<br>
> + ret = request_threaded_irq(info->irq[0], NULL,<br>
> + pm860x_charger_handler,<br>
> + IRQF_ONESHOT, "usb supply detect",<br>
> + info);<br>
<br>
</div></div>The same problem w/ requesting interrupt before ensuring that<br>
everything initialized, registered and thus usable. Imagine that<br>
irq fires before mutex_init(), which is down below.<br>
<div><div class="h5"><br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[0], ret);<br>
> + goto out_dev;<br>
> + }<br>
> +<br>
> + ret = request_threaded_irq(info->irq[1], NULL,<br>
> + pm860x_done_handler,<br>
> + IRQF_ONESHOT, "charge done", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[1], ret);<br>
> + goto out_irq1;<br>
> + }<br>
> + ret = request_threaded_irq(info->irq[2], NULL,<br>
> + pm860x_exception_handler,<br>
> + IRQF_ONESHOT, "charge timeout", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[2], ret);<br>
> + goto out_irq2;<br>
> + }<br>
> + ret = request_threaded_irq(info->irq[3], NULL,<br>
> + pm860x_exception_handler,<br>
> + IRQF_ONESHOT, "charge fault", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[3], ret);<br>
> + goto out_irq3;<br>
> + }<br>
> + ret = request_threaded_irq(info->irq[4], NULL,<br>
> + pm860x_temp_handler,<br>
> + IRQF_ONESHOT, "temperature", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[4], ret);<br>
> + goto out_irq4;<br>
> + }<br>
> + ret = request_threaded_irq(info->irq[5], NULL,<br>
> + pm860x_vbattery_handler,<br>
> + IRQF_ONESHOT, "vbatt", info);<br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[5], ret);<br>
> + goto out_irq5;<br>
> + }<br>
> + ret = request_threaded_irq(info->irq[6], NULL,<br>
> + pm860x_vchg_handler,<br>
> + IRQF_ONESHOT, "vchg", info);<br>
<br>
</div></div>Quite a lot of interrupts. You probably could make an array that<br>
maps idx, name and handler, e.g.<br>
<br>
struct pm860x_irq_desc {<br>
const char *name;<br>
irqreturn_t (*handler)(int irq, void *data);<br>
} pm860x_irq_descs[] = {<br>
{ "usb supply detect", pm860x_charger_handler },<br>
{ "charge done", pm860x_done_handler },<br>
};<br>
<br>
and turn the code into a loop that registers all the interrupts, e.g.<br>
<br>
for (i = 0; i < ARRAY_SIZE(info->irq); i++) {<br>
ret = request_threaded_irq(info->irq[i], NULL,<br>
pm860x_irq_descs[i].handler,<br>
IRQF_ONESHOT, pm860x_irq_descs[i].name, info);<br>
...<br>
}<br>
<br>
That would be much nicer.<br>
<div class="im"><br>
> + if (ret < 0) {<br>
> + dev_err(chip->dev, "Failed to request IRQ: #%d: %d\n",<br>
> + info->irq[6], ret);<br>
> + goto out_irq6;<br>
> + }<br>
> + if (info->irq_nums <= 6) {<br>
<br>
</div>Instead of hardcoding 6, you can write ARRAY_SIZE(info->irq);<br>
<div><div class="h5"><br>
> + dev_err(chip->dev, "IRQ numbers aren't matched\n");<br>
> + goto out_nums;<br>
> + }<br>
> +<br>
> + mutex_init(&info->lock);<br>
> + platform_set_drvdata(pdev, info);<br>
> +<br>
> + info-><a href="http://usb.name" target="_blank">usb.name</a> = "usb";<br>
> + info->usb.type = POWER_SUPPLY_TYPE_USB;<br>
> + info->usb.supplied_to = pm860x_supplied_to;<br>
> + info->usb.num_supplicants = ARRAY_SIZE(pm860x_supplied_to);<br>
> + info->usb.properties = pm860x_usb_props;<br>
> + info->usb.num_properties = ARRAY_SIZE(pm860x_usb_props);<br>
> + info->usb.get_property = pm860x_usb_get_prop;<br>
> + ret = power_supply_register(&pdev->dev, &info->usb);<br>
> + if (ret)<br>
> + goto out_nums;<br>
> +<br>
> + pm860x_init_charger(info);<br>
> +<br>
> + return 0;<br>
> +<br>
> +out_nums:<br>
> + free_irq(info->irq[6], info);<br>
> +out_irq6:<br>
> + free_irq(info->irq[5], info);<br>
> +out_irq5:<br>
> + free_irq(info->irq[4], info);<br>
> +out_irq4:<br>
> + free_irq(info->irq[3], info);<br>
> +out_irq3:<br>
> + free_irq(info->irq[2], info);<br>
> +out_irq2:<br>
> + free_irq(info->irq[1], info);<br>
> +out_irq1:<br>
> + free_irq(info->irq[0], info);<br>
> +out_dev:<br>
> + device_remove_file(&pdev->dev, &dev_attr_stop);<br>
> +out:<br>
> + kfree(info);<br>
> + return ret;<br>
> +}<br>
> +<br>
> +static int __devexit pm860x_charger_remove(struct platform_device *pdev)<br>
> +{<br>
> + struct pm860x_charger_info *info = platform_get_drvdata(pdev);<br>
> + int i;<br>
> +<br>
> + platform_set_drvdata(pdev, NULL);<br>
> + power_supply_unregister(&info->usb);<br>
> + free_irq(info->irq[0], info);<br>
<br>
</div></div>Why irq[0] is special here, i.e. why is it not in the loop down below?<br>
<div class="im"><br>
> + for (i = 1; i < info->irq_nums; i++)<br>
> + free_irq(info->irq[i], info);<br>
> + device_remove_file(info->dev, &dev_attr_stop);<br>
> + kfree(info);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static struct platform_driver pm860x_charger_driver = {<br>
> + .driver = {<br>
> + .name = "88pm860x-charger",<br>
> + .owner = THIS_MODULE,<br>
> + },<br>
<br>
</div>Improper indentation for the bracket.<br>
<div class="im"><br>
> + .probe = pm860x_charger_probe,<br>
> + .remove = __devexit_p(pm860x_charger_remove),<br>
> +};<br>
> +<br>
<br>
</div>No need for this empty line.<br>
<div class="im"><br>
> +module_platform_driver(pm860x_charger_driver);<br>
> +<br>
> +MODULE_DESCRIPTION("Marvell 88PM860x Charger driver");<br>
> +MODULE_LICENSE("GPL");<br>
<br>
</div>Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Anton Vorontsov<br>
Email: <a href="mailto:cbouatmailru@gmail.com">cbouatmailru@gmail.com</a><br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div> </div>
<div>----------------------------------</div>
<div>Best Regards</div>
<div>Jett Zhou</div>
<div> </div><br>
</div>