On Fri, Nov 18, 2011 at 02:54:09PM +0530, Ashish Jangam wrote:
> Driver for DA9052 battery charger. This driver depends on DA9052 MFD core
> dirver
> for definitions and methods.
>
> Tested on Samsung SMDK6410 board with DA9052-BC and DA9053-BA evaluation
> boards.
>
> Signed-off-by: David Dajun Chen <dc...@diasemi.com>
> Signed-off-by: Ashish Jangam <ashish.jan...@kpitcummins.com>
> ---
Ashish,
Much thanks for your work! The code itself looks OK to me. But there are
some issues regarding readability and coding style. No big deal, but
please consider fixing.
[...]
[..]
> +static int da9052_battery_check_status(struct da9052_battery *battery,
> + int *status)
> +{
> + uint8_t v[2] = {0, 0};
> + uint8_t bat_status, chg_end;
In kernel code we try to use 'uXX' types. I.e. u8 here.
> + int ret, chg_current, chg_end_current;
Each declaration should be on its own line.
int ret;
int chr_current;
> +
> + ret = da9052_group_read(battery->da9052, DA9052_STATUS_A_REG, 2, v);
> + if (ret < 0)
> + return ret;
> +
> + bat_status = v[0];
> + chg_end = v[1];
> +
> + /* Preference to WALL(DCIN) charger unit */
> + if (((bat_status & DA9052_STATUSA_DCINSEL) &&
> + (bat_status & DA9052_STATUSA_DCINDET))
> + ||
> + ((bat_status & DA9052_STATUSA_VBUSSEL) &&
> + (bat_status & DA9052_STATUSA_VBUSDET))
> + ) {
I can't parse it.
Maybe
bool dcinsel = bat_status & DA9052_STATUSA_DCINSEL;
bool dcindet = bat_status & DA9052_STATUSA_DCINDET;
bool vbussel = bat_status & DA9052_STATUSA_VBUSSEL;
bool vbusdet = bat_status & DA9052_STATUSA_VBUSDET;
bool dc = dcinsel && dcindet;
bool vbus = vbussel && vbusdet;
if (dc || vbus) {
...
} else if (dcindet || vbusdet) {
...
} else {
...
}
Note that it does not add a single line of code, but things become
much more readable.
> + battery->charger_type = DA9052_CHARGER;
> +
> + /* If charging end flag is set and Charging current is greater
> + * than charging end limit then battery is charging
> + */
> + if ((chg_end & DA9052_STATUSB_CHGEND) != 0) {
> + ret = da9052_battery_read_current(battery,
> + &chg_current);
> + if (ret < 0)
> + return ret;
> + ret = da9052_battery_read_end_current(battery,
> + &chg_end_current);
> + if (ret < 0)
> + return ret;
> +
> + if (chg_current >= chg_end_current)
> + battery->status = POWER_SUPPLY_STATUS_CHARGING;
> + else
> + battery->status =
> + POWER_SUPPLY_STATUS_NOT_CHARGING;
> + }
> + /* If Charging end flag is cleared then battery is charging */
> + else
> + battery->status = POWER_SUPPLY_STATUS_CHARGING;
> + } else if (bat_status & DA9052_STATUSA_DCINDET ||
> + bat_status & DA9052_STATUSA_VBUSDET) {
> + battery->charger_type = DA9052_CHARGER;
> + battery->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + } else {
> + battery->charger_type = DA9052_NOCHARGER;
> + battery->status = POWER_SUPPLY_STATUS_DISCHARGING;
> + }
> +
> + if (status != NULL)
> + *status = battery->status;
> + return 0;
> +}
[...]
> +unsigned char select_temperature(unsigned char temp_index, int
> bat_temperature)
> +{
> + int temp_temperature;
> +
> + temp_temperature = (temperature_lookup_ref[temp_index] +
> + temperature_lookup_ref[temp_index + 1]) / 2;
> +
> + if (bat_temperature >= temp_temperature) {
> + temp_index += 1;
> + return temp_index;
> + } else
should be "} else {", per coding style.
> + return temp_index;
> +}
> +
> +static int da9052_battery_read_capacity(struct da9052_battery *battery,
> + int *capacity)
> +{
> + int bat_temperature, bat_voltage;
> + int vbat_lower, vbat_upper, level_upper, level_lower;
> + int ret, flag, index, access_index = 0;
> +
> + ret = da9052_battery_read_volt(battery, &bat_voltage);
> + if (ret < 0)
> + return ret;
> +
> + bat_temperature = da9052_adc_temperature_read(battery->da9052);
> + if (bat_temperature < 0)
> + return bat_temperature;
> +
> + for (index = 0; index < (DA9052_NO_OF_LOOKUP_TABLE - 1); index++) {
> + if (bat_temperature <= temperature_lookup_ref[0]) {
> + access_index = 0;
> + break;
> + } else if (bat_temperature >
> + temperature_lookup_ref[DA9052_NO_OF_LOOKUP_TABLE]) {
> + access_index = DA9052_NO_OF_LOOKUP_TABLE - 1;
> + break;
> + } else if ((bat_temperature >= temperature_lookup_ref[index]) &&
> + (bat_temperature >= temperature_lookup_ref[index + 1]
> + )) {
Braces placement is weird. The longer identifiers you use, the less
columns you have. Maybe try to use shorter variable names?
> + access_index = select_temperature(index,
> + bat_temperature);
> + break;
> + }
> + }
> + if (bat_voltage >= vbat_vs_capacity_look_up[access_index][0][0]) {
> + *capacity = 100;
> + return 0;
> + }
> + if (bat_voltage <= vbat_vs_capacity_look_up[access_index]
> + [DA9052_LOOK_UP_TABLE_SIZE - 1][0]) {
> + *capacity = 0;
> + return 0;
> + }
> + flag = 0;
> +
> + for (index = 0; index < (DA9052_LOOK_UP_TABLE_SIZE-1); index++) {
> + if ((bat_voltage <=
> + vbat_vs_capacity_look_up[access_index][index][0]) &&
> + (bat_voltage >=
> + vbat_vs_capacity_look_up[access_index][index + 1][0])) {
> + vbat_upper =
> + vbat_vs_capacity_look_up[access_index][index][0];
> + vbat_lower =
> + vbat_vs_capacity_look_up[access_index][index + 1][0];
> + level_upper =
> + vbat_vs_capacity_look_up[access_index][index][1];
> + level_lower =
> + vbat_vs_capacity_look_up[access_index][index + 1][1];
> + flag = 1;
> + break;
I can't parse it.
You don't have to use 24-character variable name to document your
code. For example, you can rename vbat_vs_capacity_look_up to "vc_tbl"
and just add a comment that the table is used to lookup voltage via
capacity.
s/vbat_vs_capacity_look_up/vc_tbl/g
s/access_index/i/g
s/index/j/g
[...]
> +static irqreturn_t da9052_bat_irq(int irq, void *data)
> +{
> + struct da9052_battery *battery = (struct da9052_battery *)data;
Needless cast.
Thanks!
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
