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!