Skip to contentSkip to conversation

Power: DA9052 Battery driver v4

PATCH In review Ashish Jangam 2 participants 2 messages Nov 18, 2011 – Nov 24, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 4/11 Power: DA9052 Battery driver v4

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.

---
Changes since v3
- Included power_supply.h file
- Corrected the definition of DA9052_BAT_THRESHOLD macro
- Changed definition of charger_type_enum
- Added API da9052_battery_read_end_current
- Added API da9052_battery_check_status
- Changed the way of handling the interrupts in da9052_bat_irq Changes since v2
- Correct code styling for inline functions
- Remove averaging algorithm
- Set use_for_apm thru board specific parameter

---
 drivers/power/Kconfig          |    7 +
 drivers/power/Makefile         |    1 +
 drivers/power/da9052-battery.c |  611 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 619 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/da9052-battery.c
drivers/power/Kconfig +7 -0
index 9f88641..9696502 100644
@@ -150,6 +150,13 @@ config BATTERY_DA9030
 	  Say Y here to enable support for batteries charger integrated into
 	  DA9030 PMIC.
 
+config BATTERY_DA9052
+	tristate "Dialog DA9052 Battery"
+	depends on PMIC_DA9052
+	help
+	  Say Y here to enable support for batteries charger integrated into
+	  DA9052 PMIC.
+
 config BATTERY_MAX17040
 	tristate "Maxim MAX17040 Fuel Gauge"
 	depends on I2C
drivers/power/Makefile +1 -0
index b4af13d..c570e20 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
 obj-$(CONFIG_BATTERY_BQ20Z75)	+= bq20z75.o
 obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
 obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
+obj-$(CONFIG_BATTERY_DA9052)	+= da9052-battery.o
 obj-$(CONFIG_BATTERY_MAX17040)	+= max17040_battery.o
 obj-$(CONFIG_BATTERY_MAX17042)	+= max17042_battery.o
 obj-$(CONFIG_BATTERY_Z2)	+= z2_battery.o
drivers/power/da9052-battery.c new file +611 -0

611 lines not on this page · raw message

Signature · 1 line
1.7.1
#1 lore raw
Quoted 8 lines · Ashish Jangam

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.

---

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.

[...]

[..]

drivers/power/da9052-battery.c line 213

Quoted 5 lines
drivers/power/da9052-battery.c new file +5 -0
+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.

drivers/power/da9052-battery.c line 214

Quoted 1 line
drivers/power/da9052-battery.c new file +1 -0
+	int ret, chg_current, chg_end_current;

Each declaration should be on its own line.

int ret;
int chr_current;

Quoted 5 lines
drivers/power/da9052-battery.c new file +5 -0
+
+	ret = da9052_group_read(battery->da9052, DA9052_STATUS_A_REG, 2, v);
+	if (ret < 0)
+		return ret;
+

drivers/power/da9052-battery.c line 229

Quoted 10 lines
drivers/power/da9052-battery.c new file +10 -0
+	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.

Quoted 37 lines
drivers/power/da9052-battery.c new file +37 -0
+		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;
+}

[...]

drivers/power/da9052-battery.c line 321

Quoted 11 lines
drivers/power/da9052-battery.c new file +11 -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.

Quoted 19 lines
drivers/power/da9052-battery.c new file +19 -0
+		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++) {

drivers/power/da9052-battery.c line 350

Quoted 10 lines
drivers/power/da9052-battery.c new file +10 -0
+		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?

Quoted 21 lines
drivers/power/da9052-battery.c new file +21 -0
+				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])) {

drivers/power/da9052-battery.c line 381

Quoted 10 lines
drivers/power/da9052-battery.c new file +10 -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

[...]

drivers/power/da9052-battery.c line 424

Quoted 3 lines
drivers/power/da9052-battery.c new file +3 -0
+static irqreturn_t da9052_bat_irq(int irq, void *data)
+{
+	struct da9052_battery *battery = (struct da9052_battery *)data;

Needless cast.

Thanks!

Signature · 2 lines
Anton Vorontsov
Email: [email protected]