[PATCH 0/4] bq27x00 updates

[PATCH 0/4] bq27x00 updates

[Felipe Balbi]

The previous driver for bq27x00 batteries was a real
ifdef mess. The following patches separate common code
and create separate drivers for bq27200 and bq27000.

The code looks much cleaner, but we had to keep a global
static pointer to hold the i2c_client (bq27200) or the
w1 device (bq27000).

If anyone has a better solution, I'd like to hear since
keeping that global pointer doesn't look nice.

I could only test bq27200 final driver, I don't have
hw for testing bq27000 so I'd like to ask anyone who
has such hw, please test it.

I'd say that after the following patches we can already
send both drivers to mainline since they look quite
fine now. Improvements might come later, but for now, they
look fine.

David and Anton, I'm Ccing you guys for you to take a look
at the drivers before I send the final version (against mainline)
to you for integration. It would be nice if you guys could take
a look at the patches and if something looks wrong we could fix
it and get the drivers in better shape before asking bugging lkml
with such drivers.

Felipe Balbi (4):
  power_supply: bq27x00: separate common code
  power_supply: bq27200: separate bq27200-specific driver
  power_supply: bq27200: separate bq27000-specific driver
  power_supply: bq27x00: get rid of old code

 drivers/power/Kconfig           |   25 +--
 drivers/power/Makefile          |    3 +-
 drivers/power/bq27000.c         |  173 ++++++++++++
 drivers/power/bq27200.c         |  202 ++++++++++++++
 drivers/power/bq27x00.c         |  190 +++++++++++++
 drivers/power/bq27x00.h         |   54 ++++
 drivers/power/bq27x00_battery.c |  564 ---------------------------------------
 7 files changed, 630 insertions(+), 581 deletions(-)
 create mode 100644 drivers/power/bq27000.c
 create mode 100644 drivers/power/bq27200.c
 create mode 100644 drivers/power/bq27x00.c
 create mode 100644 drivers/power/bq27x00.h
 delete mode 100644 drivers/power/bq27x00_battery.c

[PATCH 1/4] power_supply: bq27x00: separate common code

[Felipe Balbi]

put common code into a separate file and link it to new
drivers which will come in later patches.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/power/bq27x00.c |  190 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/power/bq27x00.h |   54 +++++++++++++
 2 files changed, 244 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/bq27x00.c
 create mode 100644 drivers/power/bq27x00.h

diff --git a/drivers/power/bq27x00.c b/drivers/power/bq27x00.c
new file mode 100644
index 0000000..5609829
--- /dev/null
+++ b/drivers/power/bq27x00.c
@@ -0,0 +1,190 @@
+/*
+ * bq27x00.c - Shared functions between BQ27200 and BQ27000
+ *
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ *
+ * Author: Texas Instruments
+ *
+ * This package is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+#include <linux/module.h>
+#include <linux/param.h>
+#include <linux/jiffies.h>
+#include <linux/workqueue.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+
+#include "bq27x00.h"
+
+unsigned int cache_time = 60000;
+module_param(cache_time, uint, 0644);
+MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
+
+enum power_supply_property bq27x00_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_TEMP,
+};
+
+static int bq27x00_read(u8 reg, int *rt_value, int b_single,
+	struct bq27x00_device_info *di);
+
+/*
+ * Return the battery temperature in Celcius degrees
+ * Or < 0 if something fails.
+ */
+static int bq27x00_temperature(struct bq27x00_device_info *di)
+{
+	int ret, temp = 0;
+
+	ret = bq27x00_read(BQ27x00_REG_TEMP, &temp, 0, di);
+	if (ret) {
+		pr_err("BQ27x00 battery driver:"
+			"Error reading temperature from the battery\n");
+		return ret;
+	}
+
+	return (temp >> 2) - 273;
+}
+
+/*
+ * Return the battery Voltage in milivolts
+ * Or < 0 if something fails.
+ */
+static int bq27x00_voltage(struct bq27x00_device_info *di)
+{
+	int ret, volt = 0;
+
+	ret = bq27x00_read(BQ27x00_REG_VOLT, &volt, 0, di);
+	if (ret) {
+		pr_err("BQ27x00 battery driver:"
+			"Error reading battery voltage from the battery\n");
+		return ret;
+	}
+
+	return volt;
+}
+
+/*
+ * Return the battery average current
+ * Note that current can be negative signed as well
+ * Or 0 if something fails.
+ */
+static int bq27x00_current(struct bq27x00_device_info *di)
+{
+	int ret, curr = 0, flags = 0;
+
+	ret = bq27x00_read(BQ27x00_REG_AI, &curr, 0, di);
+	if (ret) {
+		dev_dbg(di->dev, "Error reading current from the battery\n");
+		return 0;
+	}
+	ret = bq27x00_read(BQ27x00_REG_FLAGS, &flags, 0, di);
+	if (ret < 0) {
+		dev_dbg(di->dev, "Error reading battery flags\n");
+		return 0;
+	}
+	if ((flags & (1 << 7)) != 0) {
+		pr_debug("Negative current\n");
+		return -curr;
+	} else {
+		return curr;
+	}
+}
+
+/*
+ * Return the battery Relative State-of-Charge
+ * Or < 0 if something fails.
+ */
+static int bq27x00_rsoc(struct bq27x00_device_info *di)
+{
+	int ret, rsoc = 0;
+
+	ret = bq27x00_read(BQ27x00_REG_RSOC, &rsoc, 1, di);
+	if (ret) {
+		pr_err("BQ27x00 battery driver:"
+			"Error reading battery Relative"
+			"State-of-Charge\n");
+		return ret;
+	}
+	return rsoc;
+}
+
+static int bq27x00_read(u8 reg, int *rt_value, int b_single,
+	struct bq27x00_device_info *di)
+{
+	return di->bus->read(reg, rt_value, b_single, di);
+}
+
+/*
+ * Read the battery temp, voltage, current and relative state of charge.
+ */
+void bq27x00_read_status(struct bq27x00_device_info *di)
+{
+	if (di->update_time && time_before(jiffies, di->update_time +
+					msecs_to_jiffies(cache_time)))
+		return;
+
+	di->temp_C = bq27x00_temperature(di);
+	di->voltage_uV = bq27x00_voltage(di);
+	di->current_uA = bq27x00_current(di);
+	di->charge_rsoc = bq27x00_rsoc(di);
+
+	di->update_time = jiffies;
+}
+
+int bq27x00_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct bq27x00_device_info *di = to_bq27x00_device_info(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = di->voltage_uV;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = di->current_uA;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		val->intval = di->charge_rsoc;
+		break;
+	case POWER_SUPPLY_PROP_TEMP:
+		val->intval = di->temp_C;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = di->charge_rsoc;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		if (di->voltage_uV == 0)
+			val->intval = 0;
+		else
+			val->intval = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void bq27x00_powersupply_init(struct bq27x00_device_info *di)
+{
+	di->bat.type = POWER_SUPPLY_TYPE_BATTERY;
+	di->bat.properties = bq27x00_props;
+	di->bat.num_properties = ARRAY_SIZE(bq27x00_props);
+	di->bat.get_property = bq27x00_get_property;
+	di->bat.external_power_changed = NULL;
+}
+
diff --git a/drivers/power/bq27x00.h b/drivers/power/bq27x00.h
new file mode 100644
index 0000000..f465ed8
--- /dev/null
+++ b/drivers/power/bq27x00.h
@@ -0,0 +1,54 @@
+/*
+ * bq27x00.h - exported symbols placeholder
+ *
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ * Copyright (C) 2008 Nokia Corporation
+ *
+ * Author: Texas Instruments
+ *
+ * This package is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+
+#define BQ27x00_REG_TEMP		0x06
+#define BQ27x00_REG_VOLT		0x08
+#define BQ27x00_REG_RSOC		0x0B /* Relative State-of-Charge */
+#define BQ27x00_REG_AI			0x14
+#define BQ27x00_REG_FLAGS		0x0A
+#define HIGH_BYTE(A)			((A) << 8)
+
+struct bq27x00_device_info {
+	struct device		*dev;
+	unsigned long		update_time;
+	int			voltage_uV;
+	int			current_uA;
+	int			temp_C;
+	int			charge_rsoc;
+	struct bq27x00_access_methods	*bus;
+	struct power_supply	bat;
+	struct delayed_work	monitor_work;
+};
+
+struct bq27x00_access_methods {
+	int (*read)(u8 reg, int *rt_value, int b_single,
+		struct bq27x00_device_info *di);
+};
+
+extern unsigned int cache_time;
+extern enum power_supply_property bq27x00_props[];
+
+extern void bq27x00_powersupply_init(struct bq27x00_device_info *di);
+extern void bq27x00_read_status(struct bq27x00_device_info *di);
+extern int bq27x00_get_property(struct power_supply *psy,
+		enum power_supply_property psp,
+		union power_supply_propval *val);
+
+#define to_bq27x00_device_info(x) container_of((x), \
+				struct bq27x00_device_info, bat);
+
-- 
1.6.0.2.307.gc427

[PATCH 2/4] power_supply: bq27200: separate bq27200-specific driver

[Felipe Balbi]

Create a separate driver for bq27200 chip. On a later patch,
the old file and cold will be remove and Makefile/Kconfig
updated.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/power/bq27200.c |  202 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 202 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/bq27200.c

diff --git a/drivers/power/bq27200.c b/drivers/power/bq27200.c
new file mode 100644
index 0000000..ef03743
--- /dev/null
+++ b/drivers/power/bq27200.c
@@ -0,0 +1,202 @@
+/*
+ * bq27200.c - BQ27200 battery driver
+ *
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ * Copyright (C) 2008 Nokia Corporation
+ *
+ * Author: Texas Instruments
+ *
+ * This package is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+#include <linux/module.h>
+#include <linux/param.h>
+#include <linux/jiffies.h>
+#include <linux/workqueue.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/i2c.h>
+
+#include "bq27x00.h"
+
+static struct i2c_client *bq_client;
+
+static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
+	struct bq27x00_device_info *di)
+{
+	struct i2c_client *client = bq_client;
+	struct i2c_msg msg[1];
+	unsigned char data[2];
+	int err;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 1;
+	msg->buf = data;
+
+	data[0] = reg;
+	err = i2c_transfer(client->adapter, msg, 1);
+
+	if (err >= 0) {
+		if (!b_single)
+			msg->len = 2;
+		else
+			msg->len = 1;
+
+		msg->flags = I2C_M_RD;
+		err = i2c_transfer(client->adapter, msg, 1);
+		if (err >= 0) {
+			if (!b_single)
+				*rt_value = data[1] | HIGH_BYTE(data[0]);
+			else
+				*rt_value = data[0];
+
+			return 0;
+		} else {
+			dev_dbg(di->dev, "read failed with status %d\n", err);
+			return err;
+		}
+	} else {
+		dev_dbg(di->dev, "write failed with status %d\n", err);
+		return err;
+	}
+}
+
+static void bq27200_work(struct work_struct *work)
+{
+	struct bq27x00_device_info *di = container_of(work,
+		struct bq27x00_device_info, monitor_work.work);
+
+	bq27x00_read_status(di);
+	schedule_delayed_work(&di->monitor_work, 100);
+}
+
+static int __init bq27200_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct bq27x00_device_info *di;
+	struct bq27x00_access_methods *bus;
+	int retval = 0;
+
+	di = kzalloc(sizeof(*di), GFP_KERNEL);
+	if (!di) {
+		dev_dbg(&client->dev, "could not allocate dev info's memory\n");
+		retval = -ENOMEM;
+		goto err_di;
+	}
+
+	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
+	if (!bus) {
+		dev_dbg(&client->dev, "could not allocate bus' memory\n");
+		retval = -ENOMEM;
+		goto err_bus;
+	}
+
+	i2c_set_clientdata(client, di);
+	di->dev = &client->dev;
+	di->bat.name = "bq27200";
+	bus->read = &bq27200_read;
+	di->bus = bus;
+	bq_client = client;
+
+	bq27x00_powersupply_init(di);
+
+	retval = power_supply_register(&client->dev, &di->bat);
+	if (retval) {
+		dev_dbg(&client->dev, "could not register power_supply, %d\n",
+				retval);
+		goto err_psy;
+	}
+
+	INIT_DELAYED_WORK(&di->monitor_work, bq27200_work);
+	schedule_delayed_work(&di->monitor_work, 100);
+
+	return 0;
+
+err_psy:
+	i2c_set_clientdata(client, NULL);
+	kfree(bus);
+
+err_bus:
+	kfree(di);
+
+err_di:
+	return retval;
+}
+
+static int __exit bq27200_remove(struct i2c_client *client)
+{
+	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
+
+	flush_scheduled_work();
+	power_supply_unregister(&di->bat);
+	kfree(di->bus);
+	kfree(di);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int bq27200_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
+
+	cancel_delayed_work(&di->monitor_work);
+	return 0;
+}
+
+static int bq27200_resume(struct i2c_client *client)
+{
+	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
+
+	schedule_delayed_work(&di->monitor_work, 0);
+	return 0;
+}
+#else
+#define bq27200_suspend	NULL
+#define bq27200_resume	NULL
+#endif /* CONFIG_PM */
+
+static const struct i2c_device_id bq27200_id[] = {
+	{ "bq27200", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, bq27200_id);
+
+static struct i2c_driver bq27200_driver = {
+	.driver		= {
+		.name   = "bq27200",
+	},
+	.probe		= bq27200_probe,
+	.remove		= __exit_p(bq27200_remove),
+	.suspend	= bq27200_suspend,
+	.resume		= bq27200_resume,
+	.id_table	= bq27200_id,
+};
+
+static int __init bq27200_init(void)
+{
+	return i2c_add_driver(&bq27200_driver);
+}
+module_init(bq27200_init);
+
+static void __exit bq27200_exit(void)
+{
+	i2c_del_driver(&bq27200_driver);
+}
+module_exit(bq27200_exit);
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("BQ27200 battery moniter driver");
+MODULE_LICENSE("GPL");
+
-- 
1.6.0.2.307.gc427

[PATCH 3/4] power_supply: bq27200: separate bq27000-specific driver

[Felipe Balbi]

Create a separate driver for bq27000 chip. On a later patch,
the old file and cold will be remove and Makefile/Kconfig
updated

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/power/bq27000.c |  173 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/bq27000.c

diff --git a/drivers/power/bq27000.c b/drivers/power/bq27000.c
new file mode 100644
index 0000000..35a6b67
--- /dev/null
+++ b/drivers/power/bq27000.c
@@ -0,0 +1,173 @@
+/*
+ * bq27000.c - BQ27000 battery driver
+ *
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ * Copyright (C) 2008 Nokia Corporation
+ *
+ * Author: Texas Instruments
+ *
+ * This package is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/param.h>
+#include <linux/jiffies.h>
+#include <linux/workqueue.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+
+#include "../w1/w1.h"
+#include "bq27x00.h"
+
+static struct device *w1_dev;
+
+extern int w1_bq27000_read(struct device *dev, u8 reg);
+
+static inline int bq27000_read(u8 reg, int *rt_value, int b_single,
+	struct bq27x00_device_info *di)
+{
+	u8 val;
+
+	val = w1_bq27000_read(w1_dev, reg);
+	*rt_value = val;
+
+	if (!b_single) {
+		val = 0;
+		val = w1_bq27000_read(w1_dev, reg + 1);
+		*rt_value +=  HIGH_BYTE((int) val);
+	}
+
+	return 0;
+}
+
+static void bq27000_work(struct work_struct *work)
+{
+	struct bq27x00_device_info *di = container_of(work,
+		struct bq27x00_device_info, monitor_work.work);
+
+	bq27x00_read_status(di);
+	schedule_delayed_work(&di->monitor_work, 100);
+}
+
+static int __init bq27000_probe(struct  platform_device *pdev)
+{
+	struct bq27x00_device_info *di;
+	struct bq27x00_access_methods *bus;
+	int retval = 0;
+
+	di = kzalloc(sizeof(*di), GFP_KERNEL);
+	if (!di) {
+		dev_dbg(&pdev->dev, "could not allocate dev info's memory\n");
+		retval = -ENOMEM;
+		goto err_di;
+	}
+
+	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
+	if (!bus) {
+		dev_dbg(&pdev->dev, "could not allocate bus' memory\n");
+		retval = -ENOMEM;
+		goto err_bus;
+	}
+
+	platform_set_drvdata(pdev, di);
+
+	w1_dev = pdev->dev.parent;
+	di->dev = &pdev->dev;
+	di->bat.name = "bq27000";
+	bus->read = &bq27000_read;
+	di->bus = bus;
+
+	bq27x00_powersupply_init(di);
+
+	retval = power_supply_register(&pdev->dev, &di->bat);
+	if (retval) {
+		dev_dbg(&pdev->dev, "could not register power_supply, %d\n",
+				retval);
+		goto err_psy;
+	}
+
+	INIT_DELAYED_WORK(&di->monitor_work, bq27000_work);
+	schedule_delayed_work(&di->monitor_work, 50);
+
+	return 0;
+
+err_psy:
+	platform_set_drvdata(pdev, NULL);
+	kfree(bus);
+
+err_bus:
+	kfree(di);
+
+err_di:
+	return retval;
+}
+
+static int __exit bq27000_remove(struct  platform_device *pdev)
+{
+	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
+
+	flush_scheduled_work();
+	power_supply_unregister(&di->bat);
+	platform_set_drvdata(pdev, NULL);
+	kfree(di->bus);
+	kfree(di);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int bq27000_suspend(struct platform_device *pdev,
+	pm_message_t state)
+{
+	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
+
+	cancel_delayed_work(&di->monitor_work);
+	return 0;
+}
+
+static int bq27000_resume(struct platform_device *pdev)
+{
+	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
+
+	schedule_delayed_work(&di->monitor_work, 0);
+	return 0;
+}
+#else
+#define bq27000_suspend	NULL
+#define bq27000_resume	NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver bq27000_driver = {
+	.driver		= {
+		.name	= "bq27000",
+	},
+	.probe		= bq27000_probe,
+	.remove		= __exit_p(bq27000_remove),
+	.suspend	= bq27000_suspend,
+	.resume		= bq27000_resume,
+};
+
+static int __init bq27x00_init(void)
+{
+	return platform_driver_register(&bq27000_driver);
+}
+module_init(bq27x00_init);
+
+static void __exit bq27x00_exit(void)
+{
+	platform_driver_unregister(&bq27000_driver);
+}
+module_exit(bq27x00_exit);
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("BQ27000 battery moniter driver");
+MODULE_LICENSE("GPL");
+
-- 
1.6.0.2.307.gc427

[PATCH 4/4] power_supply: bq27x00: get rid of old code

[Felipe Balbi]

The old driver was an ifdef mess. Writing separate drivers
using common 'library-like' code seemed to be a better choice.

The old driver is then deleted and Makefile/Kconfig are changed
to make place for new bq27200 and bq27000 drivers.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/power/Kconfig           |   25 +--
 drivers/power/Makefile          |    3 +-
 drivers/power/bq27x00_battery.c |  564 ---------------------------------------
 3 files changed, 11 insertions(+), 581 deletions(-)
 delete mode 100644 drivers/power/bq27x00_battery.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index c0a3036..0ca1aee 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -49,26 +49,19 @@ config BATTERY_OLPC
 	help
 	  Say Y to enable support for the battery on the OLPC laptop.
 
-config BATTERY_BQ27x00
-	tristate "BQ27x00 battery driver"
+config BATTERY_BQ27200
+	tristate "BQ27200 battery driver"
+	depends on I2C
+	select I2C_OMAP
 	help
-	  Say Y here to enable support for batteries with BQ27000 or BQ27200 chip.
+	  Say Y here to enable support for batteries with BQ27200 chip.
 
-config BATTERY_BQ27000
-	bool "BQ27000 battery driver"
-	depends on BATTERY_BQ27x00
-	select W1
+config BATTERY BQ27000
+	tristate "BQ27000 battery driver"
+	depends on W1
 	select W1_SLAVE_BQ27000
 	help
-	  Say Y here to enable support for batteries with BQ27000(HDQ) chip.
-
-config BATTERY_BQ27200
-	bool "BQ27200 battery driver"
-	depends on BATTERY_BQ27x00
-	select I2C
-	select I2C_OMAP
-	help
-	  Say Y here to enable support for batteries with BQ27200(I2C) chip.
+	  Say Y here to enable support for batteries with BQ27000 chip.
 
 config TWL4030_BCI_BATTERY
 	tristate "OMAP TWL4030 BCI Battery driver"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 8da941a..0268c22 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -20,7 +20,8 @@ obj-$(CONFIG_APM_POWER)		+= apm_power.o
 obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
 obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
 obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
-obj-$(CONFIG_BATTERY_BQ27x00)   += bq27x00_battery.o
+obj-$(CONFIG_BATTERY_BQ27200)	+= bq27200.o bq27x00.o
+obj-$(CONFIG_BATTERY_BQ27000)	+= bq27000.o bq27x00.o
 obj-$(CONFIG_TWL4030_BCI_BATTERY)	+= twl4030_bci_battery.o
 obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
 obj-$(CONFIG_BATTERY_PALMTX)	+= palmtx_battery.o
diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
deleted file mode 100644
index 8b439e8..0000000
--- a/drivers/power/bq27x00_battery.c
+++ /dev/null
@@ -1,564 +0,0 @@
-/*
- * linux/drivers/power/bq27x00_battery.c
- *
- * BQ27000/BQ27200 battery driver
- *
- * Copyright (C) 2008 Texas Instruments, Inc.
- *
- * Author: Texas Instruments
- *
- * This package is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
- *
- */
-#include <linux/module.h>
-#include <linux/param.h>
-#include <linux/jiffies.h>
-#include <linux/workqueue.h>
-#include <linux/delay.h>
-#include <linux/platform_device.h>
-#include <linux/power_supply.h>
-
-#ifdef CONFIG_BATTERY_BQ27000
-#include "../w1/w1.h"
-#endif
-#ifdef CONFIG_BATTERY_BQ27200
-#include <linux/i2c.h>
-#endif
-
-#define BQ27x00_REG_TEMP		0x06
-#define BQ27x00_REG_VOLT		0x08
-#define BQ27x00_REG_RSOC		0x0B /* Relative State-of-Charge */
-#define BQ27x00_REG_AI			0x14
-#define BQ27x00_REG_FLAGS		0x0A
-#define HIGH_BYTE(A)			((A) << 8)
-
-#ifdef CONFIG_BATTERY_BQ27000
-extern int w1_bq27000_read(struct device *dev, u8 reg);
-#endif
-
-struct bq27x00_device_info;
-struct bq27x00_access_methods {
-	int (*read)(u8 reg, int *rt_value, int b_single,
-		struct bq27x00_device_info *di);
-};
-
-struct bq27x00_device_info {
-	struct device 		*dev;
-#ifdef CONFIG_BATTERY_BQ27000
-	struct device		*w1_dev;
-#endif
-#ifdef CONFIG_BATTERY_BQ27200
-	struct i2c_client *client;
-#endif
-	unsigned long		update_time;
-	int			voltage_uV;
-	int			current_uA;
-	int			temp_C;
-	int			charge_rsoc;
-	struct bq27x00_access_methods	*bus;
-	struct power_supply	bat;
-	struct delayed_work	monitor_work;
-};
-
-static unsigned int cache_time = 60000;
-module_param(cache_time, uint, 0644);
-MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
-
-static enum power_supply_property bq27x00_battery_props[] = {
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_TEMP,
-};
-
-static int bq27x00_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di);
-
-#ifdef CONFIG_BATTERY_BQ27000
-static int bq27000_battery_probe(struct platform_device *dev);
-static int bq27000_battery_remove(struct platform_device *dev);
-#ifdef CONFIG_PM
-static int bq27000_battery_suspend(struct platform_device *dev,
-	pm_message_t state);
-static int bq27000_battery_resume(struct platform_device *dev);
-#endif /* CONFIG_PM */
-
-static struct platform_driver bq27000_battery_driver = {
-	.probe = bq27000_battery_probe,
-	.remove = bq27000_battery_remove,
-#ifdef CONFIG_PM
-	.suspend = bq27000_battery_suspend,
-	.resume = bq27000_battery_resume,
-#endif /* CONFIG_PM */
-	.driver = {
-		.name = "bq27000-battery",
-	},
-};
-#endif /* CONFIG_BATTERY_BQ27000 */
-
-#ifdef CONFIG_BATTERY_BQ27200
-static int bq27200_battery_probe(struct i2c_client *client);
-static int bq27200_battery_remove(struct i2c_client *client);
-#ifdef CONFIG_PM
-static int bq27200_battery_suspend(struct i2c_client *client,
-		pm_message_t mesg);
-static int bq27200_battery_resume(struct i2c_client *client);
-#endif /* CONFIG_PM */
-static struct i2c_driver bq27200_battery_driver = {
-	.driver = {
-		.name   = "bq27200-bat",
-	},
-	.probe  = bq27200_battery_probe,
-	.remove = bq27200_battery_remove,
-#ifdef CONFIG_PM
-	.suspend = bq27200_battery_suspend,
-	.resume = bq27200_battery_resume,
-#endif /* CONFIG_PM */
-};
-#endif /* CONFIG_BATTERY_BQ27200 */
-
-/*
- * Return the battery temperature in Celcius degrees
- * Or < 0 if something fails.
- */
-static int bq27x00_battery_temperature(struct bq27x00_device_info *di)
-{
-	int ret, temp = 0;
-
-	ret = bq27x00_read(BQ27x00_REG_TEMP, &temp, 0, di);
-	if (ret) {
-		pr_err("BQ27x00 battery driver:"
-			"Error reading temperature from the battery\n");
-		return ret;
-	}
-
-	return (temp >> 2) - 273;
-}
-
-/*
- * Return the battery Voltage in milivolts
- * Or < 0 if something fails.
- */
-static int bq27x00_battery_voltage(struct bq27x00_device_info *di)
-{
-	int ret, volt = 0;
-
-	ret = bq27x00_read(BQ27x00_REG_VOLT, &volt, 0, di);
-	if (ret) {
-		pr_err("BQ27x00 battery driver:"
-			"Error reading battery voltage from the battery\n");
-		return ret;
-	}
-
-	return volt;
-}
-
-/*
- * Return the battery average current
- * Note that current can be negative signed as well
- * Or 0 if something fails.
- */
-static int bq27x00_battery_current(struct bq27x00_device_info *di)
-{
-	int ret, curr = 0, flags = 0;
-
-	ret = bq27x00_read(BQ27x00_REG_AI, &curr, 0, di);
-	if (ret) {
-		pr_err("BQ27x00 battery driver:"
-			"Error reading current from the battery\n");
-		return 0;
-	}
-	ret = bq27x00_read(BQ27x00_REG_FLAGS, &flags, 0, di);
-	if (ret < 0) {
-		pr_err("BQ27x00 battery driver:"
-			"Error reading battery flags\n");
-		return 0;
-	}
-	if ((flags & (1 << 7)) != 0) {
-		pr_debug("Negative current\n");
-		return -curr;
-	} else {
-		return curr;
-	}
-}
-
-/*
- * Return the battery Relative State-of-Charge
- * Or < 0 if something fails.
- */
-static int bq27x00_battery_rsoc(struct bq27x00_device_info *di)
-{
-	int ret, rsoc = 0;
-
-	ret = bq27x00_read(BQ27x00_REG_RSOC, &rsoc, 1, di);
-	if (ret) {
-		pr_err("BQ27x00 battery driver:"
-			"Error reading battery Relative"
-			"State-of-Charge\n");
-		return ret;
-	}
-	return rsoc;
-}
-
-#ifdef CONFIG_BATTERY_BQ27000
-static inline int bq27000_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di)
-{
-	u8 val;
-
-	val = w1_bq27000_read(di->w1_dev, reg);
-	*rt_value = val;
-
-	if (!b_single) {
-		val = 0;
-		val = w1_bq27000_read(di->w1_dev, reg + 1);
-		*rt_value +=  HIGH_BYTE((int) val);
-	}
-
-	return 0;
-}
-#else
-static inline int bq27000_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di)
-{
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_BATTERY_BQ27200
-static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di)
-{
-	struct i2c_client *client = di->client;
-	struct i2c_msg msg[1];
-	unsigned char data[2];
-	int err;
-
-	if (!client->adapter)
-		return -ENODEV;
-
-	msg->addr = client->addr;
-	msg->flags = 0;
-	msg->len = 1;
-	msg->buf = data;
-
-	data[0] = reg;
-	err = i2c_transfer(client->adapter, msg, 1);
-
-	if (err >= 0) {
-		if (!b_single)
-			msg->len = 2;
-		else
-			msg->len = 1;
-
-		msg->flags = I2C_M_RD;
-		err = i2c_transfer(client->adapter, msg, 1);
-		if (err >= 0) {
-			if (!b_single)
-				*rt_value = data[1] | HIGH_BYTE(data[0]);
-			else
-				*rt_value = data[0];
-
-			return 0;
-		} else {
-			pr_err("BQ27200 I2C read failed\n");
-			return err;
-		}
-	} else {
-		pr_err("BQ27200 I2C write failed\n");
-		return err;
-	}
-}
-#else
-static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di)
-{
-	return 0;
-}
-#endif
-
-static int bq27x00_read(u8 reg, int *rt_value, int b_single,
-	struct bq27x00_device_info *di)
-{
-	int ret;
-
-	ret = di->bus->read(reg, rt_value, b_single, di);
-	return ret;
-}
-
-/*
- * Read the battery temp, voltage, current and relative state of charge.
- */
-static void bq27x00_battery_read_status(struct bq27x00_device_info *di)
-{
-	if (di->update_time && time_before(jiffies, di->update_time +
-					msecs_to_jiffies(cache_time)))
-		return;
-
-	di->temp_C = bq27x00_battery_temperature(di);
-	di->voltage_uV = bq27x00_battery_voltage(di);
-	di->current_uA = bq27x00_battery_current(di);
-	di->charge_rsoc = bq27x00_battery_rsoc(di);
-
-	di->update_time = jiffies;
-
-	return;
-}
-
-static void bq27x00_battery_work(struct delayed_work *work)
-{
-	struct bq27x00_device_info *di = container_of(work,
-		struct bq27x00_device_info, monitor_work);
-
-	bq27x00_battery_read_status(di);
-	schedule_delayed_work(&di->monitor_work, 100);
-	return;
-}
-
-#define to_bq27x00_device_info(x) container_of((x), \
-				struct bq27x00_device_info, bat);
-
-static int bq27x00_battery_get_property(struct power_supply *psy,
-					enum power_supply_property psp,
-					union power_supply_propval *val)
-{
-	struct bq27x00_device_info *di = to_bq27x00_device_info(psy);
-
-	switch (psp) {
-	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = di->voltage_uV;
-		break;
-	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		val->intval = di->current_uA;
-		break;
-	case POWER_SUPPLY_PROP_CHARGE_NOW:
-		val->intval = di->charge_rsoc;
-		break;
-	case POWER_SUPPLY_PROP_TEMP:
-		val->intval = di->temp_C;
-		break;
-	case POWER_SUPPLY_PROP_CAPACITY:
-		val->intval = di->charge_rsoc;
-		break;
-	case POWER_SUPPLY_PROP_PRESENT:
-		if (di->voltage_uV == 0)
-			val->intval = 0;
-		else
-			val->intval = 1;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static void bq27x00_powersupply_init(struct bq27x00_device_info *di)
-{
-	di->bat.type = POWER_SUPPLY_TYPE_BATTERY;
-	di->bat.properties = bq27x00_battery_props;
-	di->bat.num_properties = ARRAY_SIZE(bq27x00_battery_props);
-	di->bat.get_property = bq27x00_battery_get_property;
-	di->bat.external_power_changed = NULL;
-	return;
-}
-
-#ifdef CONFIG_BATTERY_BQ27200
-static int bq27200_battery_probe(struct i2c_client *client)
-{
-	struct bq27x00_device_info *di;
-	struct bq27x00_access_methods *bus;
-	int retval = 0;
-
-	di = kzalloc(sizeof(*di), GFP_KERNEL);
-	if (!di) {
-		pr_err("BQ27000 battery driver:"
-			"Failed to allocate device info structure\n");
-		return -ENOMEM;
-	}
-
-	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
-	if (!bus) {
-		pr_err("BQ27000 battery driver:"
-			"Failed to allocate access method structure\n");
-		kfree(di);
-		return -ENOMEM;
-	}
-
-	i2c_set_clientdata(client, di);
-	di->dev = &client->dev;
-	di->bat.name = "bq27200";
-	bus->read = &bq27200_read;
-	di->bus = bus;
-	di->client = client;
-
-	bq27x00_powersupply_init(di);
-
-	retval = power_supply_register(&client->dev, &di->bat);
-	if (retval) {
-		pr_err("BQ27200 battery driver: Failed to register battery\n");
-		goto batt_failed;
-	}
-
-	INIT_DELAYED_WORK(&di->monitor_work, bq27x00_battery_work);
-	schedule_delayed_work(&di->monitor_work, 100);
-
-	return 0;
-
-batt_failed:
-	kfree(bus);
-	kfree(di);
-	return retval;
-}
-
-static int bq27200_battery_remove(struct i2c_client *client)
-{
-	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
-
-	flush_scheduled_work();
-	power_supply_unregister(&di->bat);
-	kfree(di);
-
-	return 0;
-}
-
-#ifdef CONFIG_PM
-static int bq27200_battery_suspend(struct i2c_client *client, pm_message_t mesg)
-{
-	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
-
-	cancel_delayed_work(&di->monitor_work);
-	return 0;
-}
-
-static int bq27200_battery_resume(struct i2c_client *client)
-{
-	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
-
-	schedule_delayed_work(&di->monitor_work, 0);
-	return 0;
-}
-#endif /* CONFIG_PM */
-#endif /* CONFIG_BATTERY_BQ27200 */
-
-#ifdef CONFIG_BATTERY_BQ27000
-static int bq27000_battery_probe(struct  platform_device *pdev)
-{
-	struct bq27x00_device_info *di;
-	struct bq27x00_access_methods *bus;
-	int retval = 0;
-
-	di = kzalloc(sizeof(*di), GFP_KERNEL);
-	if (!di) {
-		pr_err("BQ27000 battery driver:"
-			"Failed to allocate device info structure\n");
-		return -ENOMEM;
-	}
-
-	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
-	if (!bus) {
-		pr_err("BQ27000 battery driver:"
-			"Failed to allocate access method structure\n");
-		kfree(di);
-		return -ENOMEM;
-	}
-
-	platform_set_drvdata(pdev, di);
-
-	di->dev = &pdev->dev;
-	di->w1_dev = pdev->dev.parent;
-	di->bat.name = "bq27000";
-	bus->read = &bq27000_read;
-	di->bus = bus;
-
-	bq27x00_powersupply_init(di);
-
-	retval = power_supply_register(&pdev->dev, &di->bat);
-	if (retval) {
-		pr_err("BQ27000 battery driver: Failed to register battery\n");
-		goto batt_failed;
-	}
-
-	INIT_DELAYED_WORK(&di->monitor_work, bq27x00_battery_work);
-	schedule_delayed_work(&di->monitor_work, 50);
-
-	return 0;
-
-batt_failed:
-	kfree(bus);
-	kfree(di);
-	return retval;
-}
-
-static int bq27000_battery_remove(struct  platform_device *pdev)
-{
-	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
-
-	flush_scheduled_work();
-	power_supply_unregister(&di->bat);
-	platform_set_drvdata(pdev, NULL);
-	kfree(di);
-
-	return 0;
-}
-
-#ifdef CONFIG_PM
-static int bq27000_battery_suspend(struct platform_device *pdev,
-	pm_message_t state)
-{
-	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
-
-	cancel_delayed_work(&di->monitor_work);
-	return 0;
-}
-
-static int bq27000_battery_resume(struct platform_device *pdev)
-{
-	struct bq27x00_device_info *di = platform_get_drvdata(pdev);
-
-	schedule_delayed_work(&di->monitor_work, 0);
-	return 0;
-}
-#endif /* CONFIG_PM */
-#endif /* CONFIG_BATTERY_BQ27000 */
-
-static int __init bq27x00_battery_init(void)
-{
-	int status = 0;
-
-#ifdef CONFIG_BATTERY_BQ27000
-	status = platform_driver_register(&bq27000_battery_driver);
-	if (status)
-		printk(KERN_ERR "Unable to register BQ27000 driver\n");
-#endif
-#ifdef CONFIG_BATTERY_BQ27200
-	status = i2c_add_driver(&bq27200_battery_driver);
-		printk(KERN_ERR "Unable to register BQ27200 driver\n");
-#endif
-	return status;
-}
-
-static void __exit bq27x00_battery_exit(void)
-{
-#ifdef CONFIG_BATTERY_BQ27000
-	platform_driver_unregister(&bq27000_battery_driver);
-#endif
-#ifdef CONFIG_BATTERY_BQ27200
-	i2c_del_driver(&bq27200_battery_driver);
-#endif
-}
-
-module_init(bq27x00_battery_init);
-module_exit(bq27x00_battery_exit);
-
-MODULE_AUTHOR("Texas Instruments");
-MODULE_DESCRIPTION("BQ27x00 battery moniter driver");
-MODULE_LICENSE("GPL");
-- 
1.6.0.2.307.gc427

Re: [PATCH 0/4] bq27x00 updates

[Anton Vorontsov]

Hi Felipe,

On Sat, Oct 18, 2008 at 12:00:44AM +0300, Felipe Balbi wrote:
> The previous driver for bq27x00 batteries was a real
> ifdef mess. The following patches separate common code
> and create separate drivers for bq27200 and bq27000.
> 
> The code looks much cleaner, but we had to keep a global
> static pointer to hold the i2c_client (bq27200) or the
> w1 device (bq27000).
> 
> If anyone has a better solution, I'd like to hear since
> keeping that global pointer doesn't look nice.
> 
> I could only test bq27200 final driver, I don't have
> hw for testing bq27000 so I'd like to ask anyone who
> has such hw, please test it.
> 
> I'd say that after the following patches we can already
> send both drivers to mainline since they look quite
> fine now. Improvements might come later, but for now, they
> look fine.
> 
> David and Anton, I'm Ccing you guys for you to take a look
> at the drivers before I send the final version (against mainline)
> to you for integration. It would be nice if you guys could take
> a look at the patches and if something looks wrong we could fix
> it and get the drivers in better shape before asking bugging lkml
> with such drivers.

At first glance, the patches look ideal. I'll try to review them
more carefully tomorrow.

One thing though: we can't delete the old driver now. I'm about
to send a pull request to Linus. Though I think we can hurry up and
merge the new driver for 2.6.28, but delete the old one in 2.6.29.

This means that you might want to redo the patches this way:

> Felipe Balbi (4):
>   power_supply: bq27x00: separate common code

This one should include its own Kconfig/Makefile entries.

>   power_supply: bq27200: separate bq27200-specific driver

Ditto.

>   power_supply: bq27200: separate bq27000-specific driver

Ditto.

>   power_supply: bq27x00: get rid of old code

We delay this patch for 2.6.29.

>  drivers/power/Kconfig           |   25 +--
>  drivers/power/Makefile          |    3 +-
>  drivers/power/bq27000.c         |  173 ++++++++++++
>  drivers/power/bq27200.c         |  202 ++++++++++++++
>  drivers/power/bq27x00.c         |  190 +++++++++++++
>  drivers/power/bq27x00.h         |   54 ++++
>  drivers/power/bq27x00_battery.c |  564 ---------------------------------------
>  7 files changed, 630 insertions(+), 581 deletions(-)
>  create mode 100644 drivers/power/bq27000.c
>  create mode 100644 drivers/power/bq27200.c
>  create mode 100644 drivers/power/bq27x00.c
>  create mode 100644 drivers/power/bq27x00.h
>  delete mode 100644 drivers/power/bq27x00_battery.c
> 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 0/4] bq27x00 updates

[Felipe Balbi]

On Sat, Oct 18, 2008 at 01:40:06AM +0400, Anton Vorontsov wrote:
> One thing though: we can't delete the old driver now. I'm about
> to send a pull request to Linus. Though I think we can hurry up and
> merge the new driver for 2.6.28, but delete the old one in 2.6.29.
> 
> This means that you might want to redo the patches this way:
> 
> > Felipe Balbi (4):
> >   power_supply: bq27x00: separate common code
> 
> This one should include its own Kconfig/Makefile entries.
> 
> >   power_supply: bq27200: separate bq27200-specific driver
> 
> Ditto.
> 
> >   power_supply: bq27200: separate bq27000-specific driver
> 
> Ditto.
> 
> >   power_supply: bq27x00: get rid of old code
> 
> We delay this patch for 2.6.29.

Ok, I'll wait for you final review and fix everything at once :-)

Thanks

-- 
balbi

Re: [PATCH 0/4] bq27x00 updates

[Anton Vorontsov]

On Sat, Oct 18, 2008 at 12:00:44AM +0300, Felipe Balbi wrote:
> The previous driver for bq27x00 batteries was a real
> ifdef mess. The following patches separate common code
> and create separate drivers for bq27200 and bq27000.

FYI (if you don't know already): Rodolfo Giometti already
cleaned up bq27200 part of that driver and submitted it few
months ago.

http://lkml.org/lkml/2008/6/18/154

It will appear in the 2.6.28 kernel. For now it lives here:

http://git.infradead.org/battery-2.6.git?a=commitdiff;h=b996ad0e9fb15ca4acc60bcd0380912117a45d13

So I'd be happy if you could just rework it to support
bq27000 (w1) interface as well.

> The code looks much cleaner, but we had to keep a global
> static pointer to hold the i2c_client (bq27200) or the
> w1 device (bq27000).

Yeah, this is not great...

> If anyone has a better solution, I'd like to hear since
> keeping that global pointer doesn't look nice.

Can you consider this idea
http://kerneltrap.org/mailarchive/linux-kernel/2008/6/23/2198444
?

It shows only i2c part, but the idea should be clear:
there should be w1 and i2c drivers, both would create bq27x00
platform devices, then the platform driver would not depend on
any hw interface.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/4] power_supply: bq27x00: separate common code

[Anton Vorontsov]

On Sat, Oct 18, 2008 at 12:00:45AM +0300, Felipe Balbi wrote:
> put common code into a separate file and link it to new
> drivers which will come in later patches.

Looks good. Though I would prefer platform_device approach.
Nobody want to implement this though... don't know why, it is
quite easy...
http://kerneltrap.org/mailarchive/linux-kernel/2008/6/23/2198444


Few comments below.

> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  drivers/power/bq27x00.c |  190 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/power/bq27x00.h |   54 +++++++++++++
>  2 files changed, 244 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/power/bq27x00.c
>  create mode 100644 drivers/power/bq27x00.h
> 
> diff --git a/drivers/power/bq27x00.c b/drivers/power/bq27x00.c
> new file mode 100644
> index 0000000..5609829
> --- /dev/null
> +++ b/drivers/power/bq27x00.c
> @@ -0,0 +1,190 @@
> +/*
> + * bq27x00.c - Shared functions between BQ27200 and BQ27000
> + *
> + * Copyright (C) 2008 Texas Instruments, Inc.
> + *
> + * Author: Texas Instruments
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/param.h>
> +#include <linux/jiffies.h>
> +#include <linux/workqueue.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +
> +#include "bq27x00.h"
> +
> +unsigned int cache_time = 60000;

I think it should be static, no?

> +module_param(cache_time, uint, 0644);
> +MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> +
> +enum power_supply_property bq27x00_props[] = {

static?

> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_TEMP,
> +};
> +
> +static int bq27x00_read(u8 reg, int *rt_value, int b_single,
> +	struct bq27x00_device_info *di);
> +
> +/*
> + * Return the battery temperature in Celcius degrees
> + * Or < 0 if something fails.
> + */
> +static int bq27x00_temperature(struct bq27x00_device_info *di)
> +{
> +	int ret, temp = 0;
> +
> +	ret = bq27x00_read(BQ27x00_REG_TEMP, &temp, 0, di);
> +	if (ret) {
> +		pr_err("BQ27x00 battery driver:"
> +			"Error reading temperature from the battery\n");

dev_err()

> +		return ret;
> +	}
> +
> +	return (temp >> 2) - 273;
> +}
> +
> +/*
> + * Return the battery Voltage in milivolts
> + * Or < 0 if something fails.
> + */
> +static int bq27x00_voltage(struct bq27x00_device_info *di)
> +{
> +	int ret, volt = 0;
> +
> +	ret = bq27x00_read(BQ27x00_REG_VOLT, &volt, 0, di);
> +	if (ret) {
> +		pr_err("BQ27x00 battery driver:"
> +			"Error reading battery voltage from the battery\n");

dev_err()

> +		return ret;
> +	}
> +
> +	return volt;
> +}
> +
> +/*
> + * Return the battery average current
> + * Note that current can be negative signed as well
> + * Or 0 if something fails.
> + */
> +static int bq27x00_current(struct bq27x00_device_info *di)
> +{
> +	int ret, curr = 0, flags = 0;

int ret;
int cur = 0;
int flags = 0;

The same applies for other functions.

> +
> +	ret = bq27x00_read(BQ27x00_REG_AI, &curr, 0, di);
> +	if (ret) {
> +		dev_dbg(di->dev, "Error reading current from the battery\n");
> +		return 0;
> +	}
> +	ret = bq27x00_read(BQ27x00_REG_FLAGS, &flags, 0, di);
> +	if (ret < 0) {
> +		dev_dbg(di->dev, "Error reading battery flags\n");
> +		return 0;
> +	}
> +	if ((flags & (1 << 7)) != 0) {
> +		pr_debug("Negative current\n");

Why not dev_dbg() here?

> +		return -curr;
> +	} else {
> +		return curr;
> +	}
> +}
> +
> +/*
> + * Return the battery Relative State-of-Charge
> + * Or < 0 if something fails.
> + */
> +static int bq27x00_rsoc(struct bq27x00_device_info *di)
> +{
> +	int ret, rsoc = 0;
> +
> +	ret = bq27x00_read(BQ27x00_REG_RSOC, &rsoc, 1, di);
> +	if (ret) {
> +		pr_err("BQ27x00 battery driver:"
> +			"Error reading battery Relative"
> +			"State-of-Charge\n");

dev_err()

> +		return ret;
> +	}
> +	return rsoc;
> +}
> +
> +static int bq27x00_read(u8 reg, int *rt_value, int b_single,
> +	struct bq27x00_device_info *di)
> +{
> +	return di->bus->read(reg, rt_value, b_single, di);
> +}
> +
> +/*
> + * Read the battery temp, voltage, current and relative state of charge.
> + */
> +void bq27x00_read_status(struct bq27x00_device_info *di)
> +{
> +	if (di->update_time && time_before(jiffies, di->update_time +
> +					msecs_to_jiffies(cache_time)))
> +		return;
> +
> +	di->temp_C = bq27x00_temperature(di);
> +	di->voltage_uV = bq27x00_voltage(di);
> +	di->current_uA = bq27x00_current(di);
> +	di->charge_rsoc = bq27x00_rsoc(di);
> +
> +	di->update_time = jiffies;
> +}
> +
> +int bq27x00_get_property(struct power_supply *psy,
> +					enum power_supply_property psp,
> +					union power_supply_propval *val)
> +{
> +	struct bq27x00_device_info *di = to_bq27x00_device_info(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = di->voltage_uV;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = di->current_uA;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		val->intval = di->charge_rsoc;
> +		break;
> +	case POWER_SUPPLY_PROP_TEMP:
> +		val->intval = di->temp_C;

Per Documentation/power/power_supply_class.txt we return temperature
in tenths of degree Celsius. I think the driver doesn't convert it to
proper units. (The same applies to the driver that it already
in the battery-2.6.git tree. :-( We should fix that).

> +		break;
> +	case POWER_SUPPLY_PROP_CAPACITY:
> +		val->intval = di->charge_rsoc;

I don't quite get it. In what units rsoc is? PROP_CAPACITY should
be in percents, not uAh.

See Documentation/power/power_supply_class.txt

> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		if (di->voltage_uV == 0)
> +			val->intval = 0;
> +		else
> +			val->intval = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +void bq27x00_powersupply_init(struct bq27x00_device_info *di)
> +{
> +	di->bat.type = POWER_SUPPLY_TYPE_BATTERY;
> +	di->bat.properties = bq27x00_props;
> +	di->bat.num_properties = ARRAY_SIZE(bq27x00_props);
> +	di->bat.get_property = bq27x00_get_property;
> +	di->bat.external_power_changed = NULL;
> +}
> +
> diff --git a/drivers/power/bq27x00.h b/drivers/power/bq27x00.h
> new file mode 100644
> index 0000000..f465ed8
> --- /dev/null
> +++ b/drivers/power/bq27x00.h
> @@ -0,0 +1,54 @@
> +/*
> + * bq27x00.h - exported symbols placeholder
> + *
> + * Copyright (C) 2008 Texas Instruments, Inc.
> + * Copyright (C) 2008 Nokia Corporation
> + *
> + * Author: Texas Instruments
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + */

#ifndef __POWER_BQ27X00_H
#define __POWER_BQ27X00_H

> +#define BQ27x00_REG_TEMP		0x06
> +#define BQ27x00_REG_VOLT		0x08
> +#define BQ27x00_REG_RSOC		0x0B /* Relative State-of-Charge */
> +#define BQ27x00_REG_AI			0x14
> +#define BQ27x00_REG_FLAGS		0x0A
> +#define HIGH_BYTE(A)			((A) << 8)

This macros has a problem that we fixed once already:

http://git.infradead.org/battery-2.6.git?a=commitdiff;h=8aef7e8f8de2d900da892085edbf14ea35fe6881

> +
> +struct bq27x00_device_info {
> +	struct device		*dev;
> +	unsigned long		update_time;
> +	int			voltage_uV;
> +	int			current_uA;
> +	int			temp_C;
> +	int			charge_rsoc;
> +	struct bq27x00_access_methods	*bus;
> +	struct power_supply	bat;
> +	struct delayed_work	monitor_work;
> +};
> +
> +struct bq27x00_access_methods {
> +	int (*read)(u8 reg, int *rt_value, int b_single,
> +		struct bq27x00_device_info *di);
> +};
> +
> +extern unsigned int cache_time;

It seems this extern isn't used anywhere. Plus the name is too generic
for global symbol.

> +extern enum power_supply_property bq27x00_props[];

It seems you don't need this extern.

> +extern void bq27x00_powersupply_init(struct bq27x00_device_info *di);
> +extern void bq27x00_read_status(struct bq27x00_device_info *di);
> +extern int bq27x00_get_property(struct power_supply *psy,
> +		enum power_supply_property psp,
> +		union power_supply_propval *val);

Ditto for these two.

> +
> +#define to_bq27x00_device_info(x) container_of((x), \
> +				struct bq27x00_device_info, bat);
> +

#endif /* __POWER_BQ27X00_H */


Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 2/4] power_supply: bq27200: separate bq27200-specific driver

[Anton Vorontsov]

On Sat, Oct 18, 2008 at 12:00:46AM +0300, Felipe Balbi wrote:
> Create a separate driver for bq27200 chip. On a later patch,
> the old file and cold will be remove and Makefile/Kconfig
> updated.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  drivers/power/bq27200.c |  202 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 202 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/power/bq27200.c
> 
> diff --git a/drivers/power/bq27200.c b/drivers/power/bq27200.c
> new file mode 100644
> index 0000000..ef03743
> --- /dev/null
> +++ b/drivers/power/bq27200.c
> @@ -0,0 +1,202 @@
> +/*
> + * bq27200.c - BQ27200 battery driver
> + *
> + * Copyright (C) 2008 Texas Instruments, Inc.
> + * Copyright (C) 2008 Nokia Corporation
> + *
> + * Author: Texas Instruments
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/param.h>
> +#include <linux/jiffies.h>
> +#include <linux/workqueue.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/i2c.h>
> +
> +#include "bq27x00.h"
> +
> +static struct i2c_client *bq_client;

:-(

Platform device approach would eliminate need for this...

> +static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
> +	struct bq27x00_device_info *di)
> +{
> +	struct i2c_client *client = bq_client;
> +	struct i2c_msg msg[1];
> +	unsigned char data[2];
> +	int err;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	msg->addr = client->addr;
> +	msg->flags = 0;
> +	msg->len = 1;
> +	msg->buf = data;
> +
> +	data[0] = reg;
> +	err = i2c_transfer(client->adapter, msg, 1);
> +
> +	if (err >= 0) {

Would look better if you swap the success/fail cases:

if (err < 0) {
	dev_err();
	return err;
}

success-code-here;

> +		if (!b_single)
> +			msg->len = 2;
> +		else
> +			msg->len = 1;
> +
> +		msg->flags = I2C_M_RD;
> +		err = i2c_transfer(client->adapter, msg, 1);
> +		if (err >= 0) {
> +			if (!b_single)
> +				*rt_value = data[1] | HIGH_BYTE(data[0]);
> +			else
> +				*rt_value = data[0];
> +
> +			return 0;
> +		} else {
> +			dev_dbg(di->dev, "read failed with status %d\n", err);
> +			return err;
> +		}
> +	} else {
> +		dev_dbg(di->dev, "write failed with status %d\n", err);
> +		return err;
> +	}
> +}
> +
> +static void bq27200_work(struct work_struct *work)
> +{
> +	struct bq27x00_device_info *di = container_of(work,
> +		struct bq27x00_device_info, monitor_work.work);
> +
> +	bq27x00_read_status(di);
> +	schedule_delayed_work(&di->monitor_work, 100);
> +}
> +
> +static int __init bq27200_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct bq27x00_device_info *di;
> +	struct bq27x00_access_methods *bus;
> +	int retval = 0;
> +
> +	di = kzalloc(sizeof(*di), GFP_KERNEL);
> +	if (!di) {
> +		dev_dbg(&client->dev, "could not allocate dev info's memory\n");
> +		retval = -ENOMEM;
> +		goto err_di;
> +	}
> +
> +	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
> +	if (!bus) {
> +		dev_dbg(&client->dev, "could not allocate bus' memory\n");
> +		retval = -ENOMEM;
> +		goto err_bus;
> +	}
> +
> +	i2c_set_clientdata(client, di);
> +	di->dev = &client->dev;
> +	di->bat.name = "bq27200";
> +	bus->read = &bq27200_read;
> +	di->bus = bus;
> +	bq_client = client;
> +
> +	bq27x00_powersupply_init(di);
> +
> +	retval = power_supply_register(&client->dev, &di->bat);
> +	if (retval) {
> +		dev_dbg(&client->dev, "could not register power_supply, %d\n",
> +				retval);
> +		goto err_psy;
> +	}
> +
> +	INIT_DELAYED_WORK(&di->monitor_work, bq27200_work);
> +	schedule_delayed_work(&di->monitor_work, 100);

This should be done before registering the power supply. Otherwise
code may use the di->monitor_work before it has initialized.

> +	return 0;
> +
> +err_psy:
> +	i2c_set_clientdata(client, NULL);
> +	kfree(bus);
> +
> +err_bus:
> +	kfree(di);
> +
> +err_di:
> +	return retval;
> +}
> +
> +static int __exit bq27200_remove(struct i2c_client *client)
> +{
> +	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
> +
> +	flush_scheduled_work();
> +	power_supply_unregister(&di->bat);
> +	kfree(di->bus);
> +	kfree(di);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int bq27200_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
> +
> +	cancel_delayed_work(&di->monitor_work);
> +	return 0;
> +}
> +
> +static int bq27200_resume(struct i2c_client *client)
> +{
> +	struct bq27x00_device_info *di  = i2c_get_clientdata(client);
> +
> +	schedule_delayed_work(&di->monitor_work, 0);
> +	return 0;
> +}
> +#else
> +#define bq27200_suspend	NULL
> +#define bq27200_resume	NULL
> +#endif /* CONFIG_PM */
> +
> +static const struct i2c_device_id bq27200_id[] = {
> +	{ "bq27200", 0 },

No need for ", 0".

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, bq27200_id);
> +
> +static struct i2c_driver bq27200_driver = {
> +	.driver		= {
> +		.name   = "bq27200",
> +	},
> +	.probe		= bq27200_probe,
> +	.remove		= __exit_p(bq27200_remove),
> +	.suspend	= bq27200_suspend,
> +	.resume		= bq27200_resume,
> +	.id_table	= bq27200_id,
> +};
> +
> +static int __init bq27200_init(void)
> +{
> +	return i2c_add_driver(&bq27200_driver);
> +}
> +module_init(bq27200_init);
> +
> +static void __exit bq27200_exit(void)
> +{
> +	i2c_del_driver(&bq27200_driver);
> +}
> +module_exit(bq27200_exit);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_DESCRIPTION("BQ27200 battery moniter driver");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 1.6.0.2.307.gc427


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 0/4] bq27x00 updates

[Felipe Balbi]

On Sun, Oct 19, 2008 at 01:16:43AM +0400, Anton Vorontsov wrote:
> On Sat, Oct 18, 2008 at 12:00:44AM +0300, Felipe Balbi wrote:
> > The previous driver for bq27x00 batteries was a real
> > ifdef mess. The following patches separate common code
> > and create separate drivers for bq27200 and bq27000.
> 
> FYI (if you don't know already): Rodolfo Giometti already
> cleaned up bq27200 part of that driver and submitted it few
> months ago.

Didn't know that. Good to know by the way. In any case I think Copyright
Texas Instruments should be kept as is and not like Rodolfo wrote:

"Based on a previous work by Copyright (C) 2008 Texas Instruments, Inc."

since he's not really writing new driver from scratch but only cleaning
up an existing one that still sits in linux-omap tree.

In any case, that's what you get when you don't push your drivers upstream,
right ?? :-p

Hopefully TI will start pushing their code upstream and this kind of
issue will vanish.

> http://lkml.org/lkml/2008/6/18/154
> 
> It will appear in the 2.6.28 kernel. For now it lives here:
> 
> http://git.infradead.org/battery-2.6.git?a=commitdiff;h=b996ad0e9fb15ca4acc60bcd0380912117a45d13
> 
> So I'd be happy if you could just rework it to support
> bq27000 (w1) interface as well.

Sure, but I'll only take a look at that after it falls into mainline.
And btw, I don't have access to bq27000, only bq27200, so don't know if
I'm gonna be the best person to put down that code since I can only
build test.

> > The code looks much cleaner, but we had to keep a global
> > static pointer to hold the i2c_client (bq27200) or the
> > w1 device (bq27000).
> 
> Yeah, this is not great...

What I was trying to do, actually, is to provide a generic structure for
bq27200 and bq27000 and another device specific which would hold the
bus-related hooks (basically a pointer to i2c_client or a pointer to
the w1 dev).

Turned out that would be way too complicated for such a simple driver.

> Can you consider this idea
> http://kerneltrap.org/mailarchive/linux-kernel/2008/6/23/2198444
> ?
> 
> It shows only i2c part, but the idea should be clear:
> there should be w1 and i2c drivers, both would create bq27x00
> platform devices, then the platform driver would not depend on
> any hw interface.

That looks good, but please don't add new stuff to drivers/i2c/chips.
That directory is schedule to vanish asap. Also, I think bq27x00.h
should not site in include/linux since it's not really related to the
kernel as a whole.

Sure you need the access method, but that structure is initialized by
bq27200.c or bq27000.c; better would be to move bq27200.c into
drivers/power and make bq27x00.h sit inside drivers/power as well.

Also, this line is wrong:

+	di->bus = platform_get_drvdata(pdev);

di->bus will get initialized by di again. The correct would be:

di->bus = pdev->dev.platform_data;

another point is that I suppose that goto idr_try_again should have a
max_tries hook to prevent that code from looping unconditionally if we
can't get new IDR (for some reason).

-- 
balbi

Re: [PATCH 1/4] power_supply: bq27x00: separate common code

[Felipe Balbi]

On Sun, Oct 19, 2008 at 01:36:00AM +0400, Anton Vorontsov wrote:
> On Sat, Oct 18, 2008 at 12:00:45AM +0300, Felipe Balbi wrote:
> > put common code into a separate file and link it to new
> > drivers which will come in later patches.
> 
> Looks good. Though I would prefer platform_device approach.
> Nobody want to implement this though... don't know why, it is
> quite easy...
> http://kerneltrap.org/mailarchive/linux-kernel/2008/6/23/2198444
> 
> 
> Few comments below.
> 
> > Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> > ---
> >  drivers/power/bq27x00.c |  190 +++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/power/bq27x00.h |   54 +++++++++++++
> >  2 files changed, 244 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/power/bq27x00.c
> >  create mode 100644 drivers/power/bq27x00.h
> > 
> > diff --git a/drivers/power/bq27x00.c b/drivers/power/bq27x00.c
> > new file mode 100644
> > index 0000000..5609829
> > --- /dev/null
> > +++ b/drivers/power/bq27x00.c
> > @@ -0,0 +1,190 @@
> > +/*
> > + * bq27x00.c - Shared functions between BQ27200 and BQ27000
> > + *
> > + * Copyright (C) 2008 Texas Instruments, Inc.
> > + *
> > + * Author: Texas Instruments
> > + *
> > + * This package is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> > + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> > + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + */
> > +#include <linux/module.h>
> > +#include <linux/param.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/power_supply.h>
> > +
> > +#include "bq27x00.h"
> > +
> > +unsigned int cache_time = 60000;
> 
> I think it should be static, no?
> 
> > +module_param(cache_time, uint, 0644);
> > +MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> > +
> > +enum power_supply_property bq27x00_props[] = {
> 
> static?

no, it's used by bq27200.c and bq27000.c

> 
> > +	POWER_SUPPLY_PROP_PRESENT,
> > +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> > +	POWER_SUPPLY_PROP_CURRENT_NOW,
> > +	POWER_SUPPLY_PROP_CHARGE_NOW,
> > +	POWER_SUPPLY_PROP_CAPACITY,
> > +	POWER_SUPPLY_PROP_TEMP,
> > +};
> > +
> > +static int bq27x00_read(u8 reg, int *rt_value, int b_single,
> > +	struct bq27x00_device_info *di);
> > +
> > +/*
> > + * Return the battery temperature in Celcius degrees
> > + * Or < 0 if something fails.
> > + */
> > +static int bq27x00_temperature(struct bq27x00_device_info *di)
> > +{
> > +	int ret, temp = 0;
> > +
> > +	ret = bq27x00_read(BQ27x00_REG_TEMP, &temp, 0, di);
> > +	if (ret) {
> > +		pr_err("BQ27x00 battery driver:"
> > +			"Error reading temperature from the battery\n");
> 
> dev_err()

will fix, ditto below.

> 
> > +		return ret;
> > +	}
> > +
> > +	return (temp >> 2) - 273;
> > +}
> > +
> > +/*
> > + * Return the battery Voltage in milivolts
> > + * Or < 0 if something fails.
> > + */
> > +static int bq27x00_voltage(struct bq27x00_device_info *di)
> > +{
> > +	int ret, volt = 0;
> > +
> > +	ret = bq27x00_read(BQ27x00_REG_VOLT, &volt, 0, di);
> > +	if (ret) {
> > +		pr_err("BQ27x00 battery driver:"
> > +			"Error reading battery voltage from the battery\n");
> 
> dev_err()
> 
> > +		return ret;
> > +	}
> > +
> > +	return volt;
> > +}
> > +
> > +/*
> > + * Return the battery average current
> > + * Note that current can be negative signed as well
> > + * Or 0 if something fails.
> > + */
> > +static int bq27x00_current(struct bq27x00_device_info *di)
> > +{
> > +	int ret, curr = 0, flags = 0;
> 
> int ret;
> int cur = 0;
> int flags = 0;
> 
> The same applies for other functions.

sure.

> 
> > +
> > +	ret = bq27x00_read(BQ27x00_REG_AI, &curr, 0, di);
> > +	if (ret) {
> > +		dev_dbg(di->dev, "Error reading current from the battery\n");
> > +		return 0;
> > +	}
> > +	ret = bq27x00_read(BQ27x00_REG_FLAGS, &flags, 0, di);
> > +	if (ret < 0) {
> > +		dev_dbg(di->dev, "Error reading battery flags\n");
> > +		return 0;
> > +	}
> > +	if ((flags & (1 << 7)) != 0) {
> > +		pr_debug("Negative current\n");
> 
> Why not dev_dbg() here?
> 
> > +		return -curr;
> > +	} else {
> > +		return curr;
> > +	}
> > +}
> > +
> > +/*
> > + * Return the battery Relative State-of-Charge
> > + * Or < 0 if something fails.
> > + */
> > +static int bq27x00_rsoc(struct bq27x00_device_info *di)
> > +{
> > +	int ret, rsoc = 0;
> > +
> > +	ret = bq27x00_read(BQ27x00_REG_RSOC, &rsoc, 1, di);
> > +	if (ret) {
> > +		pr_err("BQ27x00 battery driver:"
> > +			"Error reading battery Relative"
> > +			"State-of-Charge\n");
> 
> dev_err()
> 
> > +		return ret;
> > +	}
> > +	return rsoc;
> > +}
> > +
> > +static int bq27x00_read(u8 reg, int *rt_value, int b_single,
> > +	struct bq27x00_device_info *di)
> > +{
> > +	return di->bus->read(reg, rt_value, b_single, di);
> > +}
> > +
> > +/*
> > + * Read the battery temp, voltage, current and relative state of charge.
> > + */
> > +void bq27x00_read_status(struct bq27x00_device_info *di)
> > +{
> > +	if (di->update_time && time_before(jiffies, di->update_time +
> > +					msecs_to_jiffies(cache_time)))
> > +		return;
> > +
> > +	di->temp_C = bq27x00_temperature(di);
> > +	di->voltage_uV = bq27x00_voltage(di);
> > +	di->current_uA = bq27x00_current(di);
> > +	di->charge_rsoc = bq27x00_rsoc(di);
> > +
> > +	di->update_time = jiffies;
> > +}
> > +
> > +int bq27x00_get_property(struct power_supply *psy,
> > +					enum power_supply_property psp,
> > +					union power_supply_propval *val)
> > +{
> > +	struct bq27x00_device_info *di = to_bq27x00_device_info(psy);
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +		val->intval = di->voltage_uV;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> > +		val->intval = di->current_uA;
> > +		break;
> > +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> > +		val->intval = di->charge_rsoc;
> > +		break;
> > +	case POWER_SUPPLY_PROP_TEMP:
> > +		val->intval = di->temp_C;
> 
> Per Documentation/power/power_supply_class.txt we return temperature
> in tenths of degree Celsius. I think the driver doesn't convert it to
> proper units. (The same applies to the driver that it already
> in the battery-2.6.git tree. :-( We should fix that).

sure, we have to start paying attention to that.

> 
> > +		break;
> > +	case POWER_SUPPLY_PROP_CAPACITY:
> > +		val->intval = di->charge_rsoc;
> 
> I don't quite get it. In what units rsoc is? PROP_CAPACITY should
> be in percents, not uAh.
> 
> See Documentation/power/power_supply_class.txt
> 
> > +		break;
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +		if (di->voltage_uV == 0)
> > +			val->intval = 0;
> > +		else
> > +			val->intval = 1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void bq27x00_powersupply_init(struct bq27x00_device_info *di)
> > +{
> > +	di->bat.type = POWER_SUPPLY_TYPE_BATTERY;
> > +	di->bat.properties = bq27x00_props;
> > +	di->bat.num_properties = ARRAY_SIZE(bq27x00_props);
> > +	di->bat.get_property = bq27x00_get_property;
> > +	di->bat.external_power_changed = NULL;
> > +}
> > +
> > diff --git a/drivers/power/bq27x00.h b/drivers/power/bq27x00.h
> > new file mode 100644
> > index 0000000..f465ed8
> > --- /dev/null
> > +++ b/drivers/power/bq27x00.h
> > @@ -0,0 +1,54 @@
> > +/*
> > + * bq27x00.h - exported symbols placeholder
> > + *
> > + * Copyright (C) 2008 Texas Instruments, Inc.
> > + * Copyright (C) 2008 Nokia Corporation
> > + *
> > + * Author: Texas Instruments
> > + *
> > + * This package is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> > + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> > + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + */
> 
> #ifndef __POWER_BQ27X00_H
> #define __POWER_BQ27X00_H
> 
> > +#define BQ27x00_REG_TEMP		0x06
> > +#define BQ27x00_REG_VOLT		0x08
> > +#define BQ27x00_REG_RSOC		0x0B /* Relative State-of-Charge */
> > +#define BQ27x00_REG_AI			0x14
> > +#define BQ27x00_REG_FLAGS		0x0A
> > +#define HIGH_BYTE(A)			((A) << 8)
> 
> This macros has a problem that we fixed once already:
> 
> http://git.infradead.org/battery-2.6.git?a=commitdiff;h=8aef7e8f8de2d900da892085edbf14ea35fe6881

cool, better will be to start working on the code that today sits on
battery-2.6.git

-- 
balbi

Re: [PATCH 2/4] power_supply: bq27200: separate bq27200-specific driver

[Felipe Balbi]

On Sun, Oct 19, 2008 at 01:46:35AM +0400, Anton Vorontsov wrote:
> > diff --git a/drivers/power/bq27200.c b/drivers/power/bq27200.c
> > new file mode 100644
> > index 0000000..ef03743
> > --- /dev/null
> > +++ b/drivers/power/bq27200.c
> > @@ -0,0 +1,202 @@
> > +/*
> > + * bq27200.c - BQ27200 battery driver
> > + *
> > + * Copyright (C) 2008 Texas Instruments, Inc.
> > + * Copyright (C) 2008 Nokia Corporation
> > + *
> > + * Author: Texas Instruments
> > + *
> > + * This package is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> > + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> > + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + */
> > +#include <linux/module.h>
> > +#include <linux/param.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/i2c.h>
> > +
> > +#include "bq27x00.h"
> > +
> > +static struct i2c_client *bq_client;
> 
> :-(
> 
> Platform device approach would eliminate need for this...

sure... that looks nice.

> > +static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
> > +	struct bq27x00_device_info *di)
> > +{
> > +	struct i2c_client *client = bq_client;
> > +	struct i2c_msg msg[1];
> > +	unsigned char data[2];
> > +	int err;
> > +
> > +	if (!client->adapter)
> > +		return -ENODEV;
> > +
> > +	msg->addr = client->addr;
> > +	msg->flags = 0;
> > +	msg->len = 1;
> > +	msg->buf = data;
> > +
> > +	data[0] = reg;
> > +	err = i2c_transfer(client->adapter, msg, 1);
> > +
> > +	if (err >= 0) {
> 
> Would look better if you swap the success/fail cases:
> 
> if (err < 0) {
> 	dev_err();
> 	return err;
> }
> 
> success-code-here;

yeah, brain fart.

> > +static int __init bq27200_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct bq27x00_device_info *di;
> > +	struct bq27x00_access_methods *bus;
> > +	int retval = 0;
> > +
> > +	di = kzalloc(sizeof(*di), GFP_KERNEL);
> > +	if (!di) {
> > +		dev_dbg(&client->dev, "could not allocate dev info's memory\n");
> > +		retval = -ENOMEM;
> > +		goto err_di;
> > +	}
> > +
> > +	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
> > +	if (!bus) {
> > +		dev_dbg(&client->dev, "could not allocate bus' memory\n");
> > +		retval = -ENOMEM;
> > +		goto err_bus;
> > +	}
> > +
> > +	i2c_set_clientdata(client, di);
> > +	di->dev = &client->dev;
> > +	di->bat.name = "bq27200";
> > +	bus->read = &bq27200_read;
> > +	di->bus = bus;
> > +	bq_client = client;
> > +
> > +	bq27x00_powersupply_init(di);
> > +
> > +	retval = power_supply_register(&client->dev, &di->bat);
> > +	if (retval) {
> > +		dev_dbg(&client->dev, "could not register power_supply, %d\n",
> > +				retval);
> > +		goto err_psy;
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&di->monitor_work, bq27200_work);
> > +	schedule_delayed_work(&di->monitor_work, 100);
> 
> This should be done before registering the power supply. Otherwise
> code may use the di->monitor_work before it has initialized.

that's right, good catch.

> > +static const struct i2c_device_id bq27200_id[] = {
> > +	{ "bq27200", 0 },
> 
> No need for ", 0".

doesn't the static variables automatic initialization to 0 (or NULL)
only work with the gnu style struct declaration ??

I mean:

static const struct i2c_device_id b27200_id[] = {
	{
		.name	= "bq27200",
	},
};

Anyways, I'm only following the standard used by Jean (i2c maintainer).

-- 
balbi

Re: [PATCH 0/4] bq27x00 updates

[Anton Vorontsov]

On Sun, Oct 19, 2008 at 01:23:43AM +0300, Felipe Balbi wrote:
> On Sun, Oct 19, 2008 at 01:16:43AM +0400, Anton Vorontsov wrote:
> > On Sat, Oct 18, 2008 at 12:00:44AM +0300, Felipe Balbi wrote:
> > > The previous driver for bq27x00 batteries was a real
> > > ifdef mess. The following patches separate common code
> > > and create separate drivers for bq27200 and bq27000.
> > 
> > FYI (if you don't know already): Rodolfo Giometti already
> > cleaned up bq27200 part of that driver and submitted it few
> > months ago.
> 
> Didn't know that. Good to know by the way. In any case I think Copyright
> Texas Instruments should be kept as is and not like Rodolfo wrote:
> 
> "Based on a previous work by Copyright (C) 2008 Texas Instruments, Inc."
> 
> since he's not really writing new driver from scratch but only cleaning
> up an existing one that still sits in linux-omap tree.

Sure, I'll happily apply a patch that you think would correct
copyright/authorship entries. But please preserve Rodolfo's credits
too, since he spent some time cleaning up your (or TI's) code.

> In any case, that's what you get when you don't push your drivers upstream,
> right ?? :-p

Right. ;-)

> Hopefully TI will start pushing their code upstream and this kind of
> issue will vanish.
> 
> > http://lkml.org/lkml/2008/6/18/154
> > 
> > It will appear in the 2.6.28 kernel. For now it lives here:
> > 
> > http://git.infradead.org/battery-2.6.git?a=commitdiff;h=b996ad0e9fb15ca4acc60bcd0380912117a45d13
> > 
> > So I'd be happy if you could just rework it to support
> > bq27000 (w1) interface as well.
> 
> Sure, but I'll only take a look at that after it falls into mainline.

I hope this will be soon.

> And btw, I don't have access to bq27000, only bq27200, so don't know if
> I'm gonna be the best person to put down that code since I can only
> build test.

IMHO build-tested driver is better than nothing. Others could
fix up any issues later, when they'll have the hardware. But it's
just my opinion... (Other's opinion could be: "untested code
is broken code" ;-)

> > > The code looks much cleaner, but we had to keep a global
> > > static pointer to hold the i2c_client (bq27200) or the
> > > w1 device (bq27000).
> > 
> > Yeah, this is not great...
> 
> What I was trying to do, actually, is to provide a generic structure for
> bq27200 and bq27000 and another device specific which would hold the
> bus-related hooks (basically a pointer to i2c_client or a pointer to
> the w1 dev).
> 
> Turned out that would be way too complicated for such a simple driver.
> 
> > Can you consider this idea
> > http://kerneltrap.org/mailarchive/linux-kernel/2008/6/23/2198444
> > ?
> > 
> > It shows only i2c part, but the idea should be clear:
> > there should be w1 and i2c drivers, both would create bq27x00
> > platform devices, then the platform driver would not depend on
> > any hw interface.
> 
> That looks good, but please don't add new stuff to drivers/i2c/chips.

I won't. I won't work on this driver at all. :-) it was just a
patch to show the idea. I didn't even sign off on it. Feel free
to use the idea in your patches. ;-)

> That directory is schedule to vanish asap.

Ok, in the end I think driver/power/ placement would be fine for
i2c part of this driver...

> Also, I think bq27x00.h
> should not site in include/linux since it's not really related to the
> kernel as a whole.

Good point.

> Sure you need the access method, but that structure is initialized by
> bq27200.c or bq27000.c; better would be to move bq27200.c into
> drivers/power and make bq27x00.h sit inside drivers/power as well.
> 
> Also, this line is wrong:
> 
> +	di->bus = platform_get_drvdata(pdev);
> 
> di->bus will get initialized by di again. The correct would be:
> 
> di->bus = pdev->dev.platform_data;
> 
> another point is that I suppose that goto idr_try_again should have a
> max_tries hook to prevent that code from looping unconditionally if we
> can't get new IDR (for some reason).

Good catches.


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/4] power_supply: bq27x00: separate common code

[Anton Vorontsov]

On Sun, Oct 19, 2008 at 01:29:50AM +0300, Felipe Balbi wrote:
[...]
> > > +#include "bq27x00.h"
> > > +
> > > +unsigned int cache_time = 60000;
> > 
> > I think it should be static, no?
> > 
> > > +module_param(cache_time, uint, 0644);
> > > +MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> > > +
> > > +enum power_supply_property bq27x00_props[] = {
> > 
> > static?
> 
> no, it's used by bq27200.c and bq27000.c

I don't see where. The only user is bq27x00_powersupply_init(),
which is in the same file (bq27x00.c).

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2