Messages in this thread | | | Date | Fri, 25 Nov 2011 03:28:49 +0400 | From | Anton Vorontsov <> | Subject | Re: [PATCH 04/11] Power: DA9052 Battery driver v4 |
| |
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 <dchen@diasemi.com> > Signed-off-by: Ashish Jangam <ashish.jangam@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: cbouatmailru@gmail.com
| |