s3c24xx: iPAQ rx1950 series

s3c-adc-battery is driver for monitoring
and charging battery on iPAQ H1930/H1940/RX1950.
It depends on s3c-adc driver to get battery voltage and current.

Signed-off-by Vasily Khoruzhick <[email protected]>

drivers/power/s3c_adc_battery.c new file +9 -0

+
+#include <plat/adc.h>
+
+#define BAT_POLL_INTERVAL			10000 /* ms */
+#define JITTER_DELAY				500 /* ms */
+
+struct s3c_adc_bat {
+	struct power_supply	psy;
+	struct s3c_adc_client	*client;

drivers/power/s3c_adc_battery.c new file +7 -0

+static char *s3c_adc_supplied_to[] = {
+	"main-battery",
+};
+
+static enum power_supply_property s3c_adc_ac_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};

drivers/power/s3c_adc_battery.c new file +46 -0

+
+static enum power_supply_property s3c_adc_backup_bat_props[] = {
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+};
+
+static int s3c_adc_backup_bat_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val);
+
+static struct s3c_adc_bat backup_bat = {
+	.psy = {
+		.name		= "backup-battery",
+		.type		= POWER_SUPPLY_TYPE_BATTERY,
+		.properties	= s3c_adc_backup_bat_props,
+		.num_properties = ARRAY_SIZE(s3c_adc_backup_bat_props),
+		.get_property	= s3c_adc_backup_bat_get_property,
+		.use_for_apm	= 1,
+	},
+};
+
+static enum power_supply_property s3c_adc_main_bat_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static int s3c_adc_bat_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val);
+
+static struct s3c_adc_bat main_bat = {
+	.psy = {
+		.name		= "main-battery",
+		.type		= POWER_SUPPLY_TYPE_BATTERY,
+		.properties	= s3c_adc_main_bat_props,
+		.num_properties = ARRAY_SIZE(s3c_adc_main_bat_props),
+		.get_property	= s3c_adc_bat_get_property,
+		.use_for_apm	= 1,
+	},
+};
+

drivers/power/s3c_adc_battery.c new file +8 -0

+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+

drivers/power/s3c_adc_battery.c new file +26 -0

+		return -EINVAL;
+	}
+
+	if (volt_value < 0 ||
+		jiffies_to_msecs(jiffies - timestamp) > BAT_POLL_INTERVAL) {
+		volt_value = s3c_adc_read(bat->client,
+			bat->pdata->backup_volt_channel);
+		volt_value *= bat->pdata->backup_volt_mult;
+		timestamp = jiffies;
+	}
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = volt_value;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		val->intval = bat->pdata->backup_volt_min;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = bat->pdata->backup_volt_max;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+

drivers/power/s3c_adc_battery.c new file +53 -0

+	}
+
+	old_status = is_plugged;
+	power_supply_changed(&s3c_adc_ac);
+}
+
+static void charge_finished_handler(unsigned long data)
+{
+	static int old_status;
+	int is_plugged = !gpio_get_value(main_bat.pdata->gpio_cable_plugged);
+	int is_charged = gpio_get_value(main_bat.pdata->gpio_charge_finished);
+
+	if (old_status == is_charged)
+		return;
+
+	if (!is_plugged)
+		return;
+
+	if (is_charged) {
+#ifdef CONFIG_LEDS_CLASS
+		led_trigger_event(charger_led_trigger, LED_FULL);
+#endif
+		main_bat.pdata->disable_charger();
+	} else {
+#ifdef CONFIG_LEDS_CLASS
+		led_trigger_event(charger_led_trigger, LED_HALF);
+#endif
+		main_bat.pdata->enable_charger();
+	}
+
+	old_status = is_charged;
+	power_supply_changed(&main_bat.psy);
+}
+
+static DEFINE_TIMER(cable_plugged_timer, cable_plugged_handler, 0, 0);
+static DEFINE_TIMER(charge_finished_timer, charge_finished_handler, 0, 0);
+
+static irqreturn_t s3c_adc_batt_cable(int irq, void *dev_id)
+{
+	mod_timer(&cable_plugged_timer,
+		jiffies + msecs_to_jiffies(JITTER_DELAY));
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t s3c_adc_batt_charged(int irq, void *dev_id)
+{
+	mod_timer(&charge_finished_timer,
+		jiffies + msecs_to_jiffies(JITTER_DELAY));
+	return IRQ_HANDLED;
+}
+
+static int calc_full_volt(int volt_val, int cur_val)
+{

drivers/power/s3c_adc_battery.c new file +19 -0

+		return -EINVAL;
+	}
+
+	if (volt_value < 0 || cur_value < 0 ||
+		jiffies_to_msecs(jiffies - timestamp) > BAT_POLL_INTERVAL) {
+		volt_value = s3c_adc_read(bat->client,
+			bat->pdata->volt_channel) * bat->pdata->volt_mult;
+		cur_value = s3c_adc_read(bat->client,
+			bat->pdata->current_channel) * bat->pdata->current_mult;
+		timestamp = jiffies;
+	}
+
+	if (!gpio_get_value(bat->pdata->gpio_cable_plugged)) {
+		if (gpio_get_value(bat->pdata->gpio_charge_finished))
+			status = POWER_SUPPLY_STATUS_FULL;
+		else {
+			status = POWER_SUPPLY_STATUS_CHARGING;
+			lut = bat->pdata->lut_acin;
+			lut_size = bat->pdata->lut_acin_cnt;

drivers/power/s3c_adc_battery.c new file +57 -0

+	/* Battery level can't go up during discharging, and
+	 * can't go down during charging
+	 */
+	if ((status == POWER_SUPPLY_STATUS_DISCHARGING && new_level < level) ||
+		(status == POWER_SUPPLY_STATUS_CHARGING && new_level > level) ||
+		status == POWER_SUPPLY_STATUS_FULL)
+		level = new_level;
+#else
+	level = new_level;
+#endif
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = status;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		val->intval = 100000;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
+		val->intval = 0;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		val->intval = level;
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = volt_value;
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = cur_value;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int __init s3c_adc_batt_probe(struct platform_device *pdev)
+{
+	struct s3c_adc_client	*client;
+	struct s3c_adc_batt_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	client = s3c_adc_register(pdev, NULL, NULL, 0);
+	if (IS_ERR(client)) {
+		dev_err(&pdev->dev, "cannot register adc\n");
+		return PTR_ERR(client);
+	}
+
+	platform_set_drvdata(pdev, client);
+
+	main_bat.client = client;
+	main_bat.pdata = pdev->dev.platform_data;
+
+	ret = power_supply_register(&pdev->dev, &main_bat.psy);
+	if (ret)
+		goto err_reg_main;
+	if (pdata->backup_volt_mult) {
+		backup_bat.client = client;

drivers/power/s3c_adc_battery.c new file +20 -0

+	ret = gpio_request(main_bat.pdata->gpio_cable_plugged, "batcable");
+	if (ret)
+		goto err_gpio1;
+
+	ret = request_irq(gpio_to_irq(main_bat.pdata->gpio_cable_plugged),
+			s3c_adc_batt_cable,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			pdev->name, NULL);
+	if (ret)
+		goto err_irq1;
+
+	ret = gpio_request(main_bat.pdata->gpio_charge_finished, "charged");
+	if (ret)
+		goto err_gpio2;
+
+	ret = request_irq(gpio_to_irq(main_bat.pdata->gpio_charge_finished),
+			s3c_adc_batt_charged,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			pdev->name, NULL);
+	if (ret)

Please Cc me on driver/power/* patches, it's pure luck that I noticed this patch.

Quoted 1 line

+ * drivers/power/s3c_adc_battery.c

This line isn't needed. You'd better move the
"iPAQ h1930/h1940/rx1950 battery controler driver" line here.

#include <linux/init.h> for __init
#include <linux/errno.h> for E-codes

drivers/power/s3c_adc_battery.c line 19

Quoted 7 lines

drivers/power/s3c_adc_battery.c new file +7 -0

+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/leds.h>
+#include <linux/gpio.h>
+#include <linux/err.h>
+#include <linux/io.h>

Do you really need io.h?

drivers/power/s3c_adc_battery.c line 24

Quoted 5 lines

drivers/power/s3c_adc_battery.c new file +5 -0

+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/s3c_adc_battery.h>
+
+#include <asm/irq.h>

Is this really needed?

drivers/power/s3c_adc_battery.c line 26

Quoted 2 lines

drivers/power/s3c_adc_battery.c new file +2 -0

+#include <mach/regs-gpio.h>
+#include <mach/regs-gpioj.h>

Not linux/gpio.h?

drivers/power/s3c_adc_battery.c line 45

Quoted 3 lines

drivers/power/s3c_adc_battery.c new file +3 -0

+static int s3c_adc_ac_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val);

Please avoid forward declarations where possible.

Quoted 7 lines

drivers/power/s3c_adc_battery.c new file +7 -0

+static char *s3c_adc_supplied_to[] = {
+	"main-battery",
+};
+
+static enum power_supply_property s3c_adc_ac_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};

drivers/power/s3c_adc_battery.c line 63

Quoted 10 lines

drivers/power/s3c_adc_battery.c new file +10 -0

+
+static struct power_supply s3c_adc_ac = {
+	.name = "ac",
+	.type = POWER_SUPPLY_TYPE_MAINS,
+	.supplied_to = s3c_adc_supplied_to,
+	.num_supplicants = ARRAY_SIZE(s3c_adc_supplied_to),
+	.properties = s3c_adc_ac_props,
+	.num_properties = ARRAY_SIZE(s3c_adc_ac_props),
+	.get_property = s3c_adc_ac_get_property,
+};

You really shouldn't create yet another "ac" power supply instance. You can use drivers/power/pda_power.c for this.

Yep, it's duplication of pda_power.c functionality.

So, you probably want to register S3C ADC battery and pda-power platform devices. And S3C ADC battery driver would only do battery stuff, and pda-power would do the AC stuff.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!bat) {
+		printk(KERN_ERR "%s: no battery infos ?!\n", __func__);

dev_err()

drivers/power/s3c_adc_battery.c line 173

Quoted 2 lines

drivers/power/s3c_adc_battery.c new file +2 -0

+	printk(KERN_INFO "Power cable %s\n",\
+			is_plugged ? "plugged" : "unplugged");

dev_info()

?

We have a generic charge triggers, see drivers/power/power_supply_leds.c, so you probably don't need this.

drivers/power/s3c_adc_battery.c line 246

Quoted 6 lines

drivers/power/s3c_adc_battery.c new file +6 -0

+static int s3c_adc_bat_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct s3c_adc_bat *bat = container_of(psy, struct s3c_adc_bat, psy);
+	static int volt_value = -1, cur_value = -1;

I don't think that using static variables here is safe. Why you need them? There must be a better way.

drivers/power/s3c_adc_battery.c line 249

Quoted 3 lines

drivers/power/s3c_adc_battery.c new file +3 -0

+	static unsigned int timestamp;
+	static int level = 1000000;
+

No need for this empty line.

drivers/power/s3c_adc_battery.c line 250

Quoted 1 line

drivers/power/s3c_adc_battery.c new file +1 -0

+	int status = POWER_SUPPLY_STATUS_DISCHARGING, new_level, full_volt;

Each declaration on it own line please, e.g.

int status = ...;
int new_level;
int full_volt;

Quoted 7 lines

drivers/power/s3c_adc_battery.c new file +7 -0

+			lut_volt1 = calc_full_volt(lut[0].volt, lut[0].cur);
+			lut_volt2 = calc_full_volt(lut[1].volt, lut[1].cur);
+			if (full_volt < lut_volt1 && full_volt >= lut_volt2) {
+				new_level = (lut[1].level +
+					(lut[0].level - lut[1].level) *
+					(full_volt - lut_volt2) /
+					(lut_volt1 - lut_volt2)) * 1000;

drivers/power/s3c_adc_battery.c line 297

Quoted 6 lines

drivers/power/s3c_adc_battery.c new file +6 -0

+				break;
+			}
+			new_level = lut[1].level * 1000;
+			lut++;
+		}
+	}

Cool!

Quoted 1 line

+#if 0

Do you really need this dead code?

You probably don't need a timer for this.
Use schedule_work() instead.

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	.get_property = s3c_adc_ac_get_property,
+};

You really shouldn't create yet another "ac" power supply instance. You can use drivers/power/pda_power.c for this.

Yep, it's duplication of pda_power.c functionality.

Quoted 3 lines

So, you probably want to register S3C ADC battery and pda-power platform devices. And S3C ADC battery driver would only do battery stuff, and pda-power would do the AC stuff.

Nope, I can't use pda_power here, as it can't control whether battery is already charged (it's separate gpio pin, and I need to disable charger in this case)

Quoted 7 lines

Quoted 7 lines

drivers/power/s3c_adc_battery.c new file +7 -0

+			lut_volt1 = calc_full_volt(lut[0].volt, lut[0].cur);
+			lut_volt2 = calc_full_volt(lut[1].volt, lut[1].cur);
+			if (full_volt < lut_volt1 && full_volt >= lut_volt2) {
+				new_level = (lut[1].level +
+					(lut[0].level - lut[1].level) *
+					(full_volt - lut_volt2) /
+					(lut_volt1 - lut_volt2)) * 1000;

Quoted 8 lines

drivers/power/s3c_adc_battery.c line 297

Quoted 6 lines

drivers/power/s3c_adc_battery.c new file +6 -0

+				break;
+			}
+			new_level = lut[1].level * 1000;
+			lut++;
+		}
+	}

Cool!

Is it some kind of sarcasm? :)

Quoted 2 lines

You probably don't need a timer for this.
Use schedule_work() instead.

Timer's used to prevet jitter on gpio pins, and here it's used to check pins state on startup. I don't see any reason to replace it with delayed work.

I guess you can remove all the cable_plugged handling from s3c battery driver, and then use power_supply.external_power_changed callback to get cable_plugged notification (you should fill pda_power.supplied_to properly to make it work).

See drivers/power/ds2760_battery.c and arch/arm/mach-pxa/hx4700.c as an example.

You probably will need to change .external_power_changed() hook to accept 'pst' argument (the supply that caused the change), so that you could write something like this:

s3c_external_power_changed(struct power_supply *psy,
struct power_supply *ext)

{
	...
	ext->get_property(ext, ONLINE, &val);
	s3c->is_plugged = val.intval;

	s3c_kick_cable_plugged_handler(s3c);
}

Or, instead of ext->get_property() you could just use power_supply_is_system_supplied(). Not very elegant, but should work.

Timer is a softirq, atomic context. It adds latency to your embedded, underpowered device. Plus with timer you can't use sleepable GPIOs (we should change it for pda_power too, btw).

Jitter filtering isn't urgent task for the kernel (or user) needs, right? So it's fine to postpone it for non-atomic context.

? ????????? ?? 9 ???? 2010 16:53:45 ????? Anton Vorontsov ???????:

Quoted 26 lines

I guess you can remove all the cable_plugged handling from s3c battery driver, and then use power_supply.external_power_changed callback to get cable_plugged notification (you should fill pda_power.supplied_to properly to make it work).

See drivers/power/ds2760_battery.c and arch/arm/mach-pxa/hx4700.c as an example.

You probably will need to change .external_power_changed() hook to accept 'pst' argument (the supply that caused the change), so that you could write something like this:

s3c_external_power_changed(struct power_supply *psy,
struct power_supply *ext)

{
	...
	ext->get_property(ext, ONLINE, &val);
	s3c->is_plugged = val.intval;

	s3c_kick_cable_plugged_handler(s3c);
}

Or, instead of ext->get_property() you could just use power_supply_is_system_supplied(). Not very elegant, but should work.

pda_power driver does not support 'battery_charged' pin handling, but I need to handle this pin (and IRQ from it) to prevent battery overcharge

You didn't get the point. Here's workflow:
cable plugged -> GPF2 goes to 0, irq is generated

cable plugged irq handler -> cable_plugged ? enable_charger : disable_charger

charging....
battery is charged, GPF3 goes to 0, irq is generated

battery charged irq handler -> disable_charger

cable unplugged -> GPF2 goest to 1, irq is generated

Still think this wont work?

Ok, one more question: pda_power requests irq with cleared TRIGGER flags, but I need TRIGGER_RISING | TRIGGER_FALLING, how can I add my custom irq flags to pda_power driver? :)