add MAX17040 Fuel Gauge driver

PATCH Superseded Minkyu Kang 5 participants 13 messages Jun 4, 2009 I2C subsystem host drivers

Share & tools

Minkyu Kang

author

2009-06-04 08:55 UTC #0 lore raw

PATCH v2 1/1 add MAX17040 Fuel Gauge driver

The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries This patch adds support the MAX17040 Fuel Gauge

Cc Anton Vorontsov <[email protected]>

Signed-off-by Minkyu Kang <[email protected]>

---
 drivers/power/Kconfig            |    8 +
 drivers/power/Makefile           |    3 +-
 drivers/power/max17040_battery.c |  306 ++++++++++++++++++++++++++++++++++++++
 include/linux/max17040_battery.h |   19 +++
 4 files changed, 335 insertions(+), 1 deletions(-)
 create mode 100644 drivers/power/max17040_battery.c
 create mode 100644 include/linux/max17040_battery.h

drivers/power/Kconfig +8 -0

index 33da112..6af5798 100644
@@ -88,4 +88,12 @@ config CHARGER_PCF50633
 	help
 	 Say Y to include support for NXP PCF50633 Main Battery Charger.
 
+config BATTERY_MAX17040
+	tristate "Maxim MAX17040 Fuel Gauge"
+	depends on I2C
+	help
+	  MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries
+	  in handheld and portable equipment. The MAX17040 is configured
+	  to operate with a single lithium cell
+
 endif # POWER_SUPPLY

drivers/power/Makefile +2 -1

index 2fcf41d..9c48995 100644
@@ -25,4 +25,5 @@ obj-$(CONFIG_BATTERY_TOSA)	+= tosa_battery.o
 obj-$(CONFIG_BATTERY_WM97XX)	+= wm97xx_battery.o
 obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
 obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
-obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
\ No newline at end of file
+obj-$(CONFIG_BATTERY_MAX17040))	+= max17040_battery.o
+obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o

drivers/power/max17040_battery.c new file +306 -0

306 lines not on this page · raw message

include/linux/max17040_battery.h new file +19 -0

new file mode 100644
index 0000000..ad97b06
@@ -0,0 +1,19 @@
+/*
+ *  Copyright (C) 2009 Samsung Electronics
+ *  Minkyu Kang <[email protected]>
+ *
+ * This program 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.
+ */
+
+#ifndef __MAX17040_BATTERY_H_
+#define __MAX17040_BATTERY_H_
+
+struct max17040_platform_data {
+	int (*battery_online)(void);
+	int (*charger_online)(void);
+	int (*charger_enable)(void);
+};
+
+#endif

Signature · 1 line

1.5.4.3

Andrew Morton

2009-06-04 09:06 UTC #1 lore raw

Quoted 9 lines · On Thu, 04 Jun 2009 17:55:36 +0900 Minkyu Kang <[email protected]> wrote:

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

+	INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+	schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+	bat_ps.properties = max17040_battery_props;
+	bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+	ret = power_supply_register(&client->dev, &bat_ps);
+	if (ret) {
+		dev_err(&max17040->client->dev,

drivers/power/max17040_battery.c line 244

Quoted 10 lines

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

+				"failed: power supply register\n");
+		cancel_delayed_work(&chip->work);
+		i2c_set_clientdata(client, NULL);
+		kfree(chip);
+		max17040 = NULL;
+		return -1;
+	}
+
+	return 0;
+}

Wouldn't it be better to start the delayed_work after the power_supply_register() has succeeded?

Andrew Morton

2009-06-04 09:07 UTC #2 lore raw

In reply to Minkyu Kang

Quoted 9 lines · On Thu, 04 Jun 2009 17:55:36 +0900 Minkyu Kang <[email protected]> wrote:

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

+	INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+	schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+	bat_ps.properties = max17040_battery_props;
+	bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+	ret = power_supply_register(&client->dev, &bat_ps);
+	if (ret) {
+		dev_err(&max17040->client->dev,

drivers/power/max17040_battery.c line 244

Quoted 10 lines

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

+				"failed: power supply register\n");
+		cancel_delayed_work(&chip->work);
+		i2c_set_clientdata(client, NULL);
+		kfree(chip);
+		max17040 = NULL;
+		return -1;
+	}
+
+	return 0;
+}

Wouldn't it be better to start the delayed_work after the power_supply_register() has succeeded?

Minkyu Kang

2009-06-04 10:39 UTC #3 lore raw

Quoted 3 lines

Wouldn't it be better to start the delayed_work after the power_supply_register() has succeeded?

Yes, that's better. thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Minkyu Kang

2009-06-04 10:40 UTC #4 lore raw

In reply to Andrew Morton

Quoted 3 lines

Wouldn't it be better to start the delayed_work after the power_supply_register() has succeeded?

Yes, that's better. thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Trilok Soni

2009-06-04 09:16 UTC #5 lore raw

In reply to Minkyu Kang

Hi Minkyu Kang,

Adding linux-i2c mailing list, so not deleting any code.

Quoted 93 lines · Minkyu Kang

The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries This patch adds support the MAX17040 Fuel Gauge

Cc Anton Vorontsov <cbou-JGs/[email protected]>

Signed-off-by Minkyu Kang <[email protected]>

---
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 3 +- drivers/power/max17040_battery.c | 306 ++++++++++++++++++++++++++++++++++++++ include/linux/max17040_battery.h | 19 +++

 4 files changed, 335 insertions(+), 1 deletions(-)

create mode 100644 drivers/power/max17040_battery.c create mode 100644 include/linux/max17040_battery.h

drivers/power/Kconfig +0 -0

index 33da112..6af5798 100644
@@ -88,4 +88,12 @@ config CHARGER_PCF50633

help
Say Y to include support for NXP PCF50633 Main Battery Charger.

drivers/power/Kconfig +8 -0

+config BATTERY_MAX17040
+       tristate "Maxim MAX17040 Fuel Gauge"
+       depends on I2C
+       help
+         MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries
+         in handheld and portable equipment. The MAX17040 is configured
+         to operate with a single lithium cell
+

endif # POWER_SUPPLY

drivers/power/Makefile +0 -0

index 2fcf41d..9c48995 100644
@@ -25,4 +25,5 @@ obj-$(CONFIG_BATTERY_TOSA)    += tosa_battery.o

obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o

drivers/power/Makefile +2 -1

-obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
\ No newline at end of file
+obj-$(CONFIG_BATTERY_MAX17040))        += max17040_battery.o
+obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o

drivers/power/max17040_battery.c new file +44 -0

new file mode 100644
index 0000000..25303ba
@@ -0,0 +1,306 @@
+/*
+ *  max17040_battery.c
+ *  fuel-gauge systems for lithium-ion (Li+) batteries
+ *
+ *  Copyright (C) 2009 Samsung Electronics
+ *  Minkyu Kang <[email protected]>
+ *
+ * This program 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/power_supply.h>
+#include <linux/max17040_battery.h>
+
+#define MAX17040_VCELL_MSB     0x02
+#define MAX17040_VCELL_LSB     0x03
+#define MAX17040_SOC_MSB       0x04
+#define MAX17040_SOC_LSB       0x05
+#define MAX17040_MODE_MSB      0x06
+#define MAX17040_MODE_LSB      0x07
+#define MAX17040_VER_MSB       0x08
+#define MAX17040_VER_LSB       0x09
+#define MAX17040_RCOMP_MSB     0x0C
+#define MAX17040_RCOMP_LSB     0x0D
+#define MAX17040_CMD_MSB       0xFE
+#define MAX17040_CMD_LSB       0xFF
+
+#define MAX17040_DELAY         1000
+#define MAX17040_BATTERY_FULL  95
+
+struct max17040_chip {
+       struct i2c_client       *client;
+       struct delayed_work     work;
+
+       /* State Of Connect */
+       int online;

drivers/power/max17040_battery.c line 54

Quoted 10 lines

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

+       /* battery voltage */
+       int vcell;
+       /* battery capacity */
+       int soc;
+       /* State Of Charge */
+       int status;
+};
+
+static struct max17040_chip *max17040;
+static struct max17040_platform_data *pdata;

May be you want to move this pdata under chip structure.

Quoted 155 lines

drivers/power/max17040_battery.c new file +155 -0

+
+static int max17040_get_property(struct power_supply *bat_ps,
+                           enum power_supply_property psp,
+                           union power_supply_propval *val)
+{
+       switch (psp) {
+       case POWER_SUPPLY_PROP_STATUS:
+               val->intval = max17040->status;
+               break;
+       case POWER_SUPPLY_PROP_ONLINE:
+               val->intval = max17040->online;
+               break;
+       case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+               val->intval = max17040->vcell;
+               break;
+       case POWER_SUPPLY_PROP_CAPACITY:
+               val->intval = max17040->soc;
+               break;
+       default:
+               return -EINVAL;
+       }
+       return 0;
+}
+
+static struct power_supply bat_ps = {
+       .name           = "battery",
+       .type           = POWER_SUPPLY_TYPE_BATTERY,
+       .get_property   = max17040_get_property,
+};
+
+static int max17040_write_reg(int reg, u8 value)
+{
+       int ret;
+
+       ret = i2c_smbus_write_byte_data(max17040->client, reg, value);
+
+       if (ret < 0) {
+               dev_err(&max17040->client->dev,
+                               "%s: reg 0x%x, val 0x%x, err %d\n",
+                               __func__, reg, value, ret);
+       }
+
+       return ret;
+}
+
+static u8 max17040_read_reg(int reg)
+{
+       int ret;
+
+       ret = i2c_smbus_read_byte_data(max17040->client, reg);
+
+       if (ret < 0) {
+               dev_err(&max17040->client->dev,
+                               "%s: reg 0x%x, err %d\n",
+                               __func__, reg, ret);
+       }
+
+       return ret;
+}
+
+static void max17040_reset(void)
+{
+       max17040_write_reg(MAX17040_CMD_MSB, 0x54);
+       max17040_write_reg(MAX17040_CMD_LSB, 0x00);
+}
+
+static void max17040_set_rcomp(u16 val)
+{
+       max17040_write_reg(MAX17040_RCOMP_MSB, val >> 8);
+       max17040_write_reg(MAX17040_RCOMP_LSB, (val << 8) >> 8);
+}
+
+static void max17040_get_vcell(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_VCELL_MSB);
+       lsb = max17040_read_reg(MAX17040_VCELL_LSB);
+
+       max17040->vcell = (msb << 4) + (lsb >> 4);
+}
+
+static void max17040_get_soc(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_SOC_MSB);
+       lsb = max17040_read_reg(MAX17040_SOC_LSB);
+
+       max17040->soc = msb;
+}
+
+static void max17040_get_version(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_VER_MSB);
+       lsb = max17040_read_reg(MAX17040_VER_LSB);
+
+       dev_info(&max17040->client->dev,
+                       "MAX17040 Fuel-Gauge Ver %d%d\n", msb, lsb);
+}
+
+static void max17040_get_online(void)
+{
+       if (pdata->battery_online)
+               max17040->online = pdata->battery_online();
+       else
+               max17040->online = 1;
+}
+
+static void max17040_get_status(void)
+{
+       if (!pdata->charger_online || !pdata->charger_enable) {
+               max17040->status = POWER_SUPPLY_STATUS_UNKNOWN;
+               return;
+       }
+
+       if (pdata->charger_online()) {
+               if (pdata->charger_enable())
+                       max17040->status = POWER_SUPPLY_STATUS_CHARGING;
+               else
+                       max17040->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+       } else {
+               max17040->status = POWER_SUPPLY_STATUS_DISCHARGING;
+       }
+
+       if (max17040->soc > MAX17040_BATTERY_FULL)
+               max17040->status = POWER_SUPPLY_STATUS_FULL;
+}
+
+static void max17040_work(struct work_struct *work)
+{
+       max17040_get_vcell();
+       max17040_get_soc();
+       max17040_get_online();
+       max17040_get_status();
+
+       schedule_delayed_work(&max17040->work, MAX17040_DELAY);
+}
+
+static enum power_supply_property max17040_battery_props[] = {
+       POWER_SUPPLY_PROP_STATUS,
+       POWER_SUPPLY_PROP_ONLINE,
+       POWER_SUPPLY_PROP_VOLTAGE_NOW,
+       POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int __devinit max17040_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct max17040_chip *chip;

drivers/power/max17040_battery.c line 219

Quoted 10 lines

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

+       int ret;
+
+       chip = kzalloc(sizeof(struct max17040_chip), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->client = client;
+       pdata = client->dev.platform_data;
+
+       i2c_set_clientdata(client, chip);

Please add i2c_check_functionality check before doing any smbus read/write operations.

drivers/power/max17040_battery.c line 220

Quoted 1 line

drivers/power/max17040_battery.c new file +1 -0
+       max17040 = chip;

This means that we support only one instance of this chip, right?

Quoted 119 lines

drivers/power/max17040_battery.c new file +86 -0

+
+       max17040_reset();
+
+       max17040_get_version();
+
+       INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+       bat_ps.properties = max17040_battery_props;
+       bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+       ret = power_supply_register(&client->dev, &bat_ps);
+       if (ret) {
+               dev_err(&max17040->client->dev,
+                               "failed: power supply register\n");
+               cancel_delayed_work(&chip->work);
+               i2c_set_clientdata(client, NULL);
+               kfree(chip);
+               max17040 = NULL;
+               return -1;
+       }
+
+       return 0;
+}
+
+static int __devexit max17040_remove(struct i2c_client *client)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       power_supply_unregister(&bat_ps);
+       cancel_delayed_work(&chip->work);
+       i2c_set_clientdata(client, NULL);
+       kfree(chip);
+       max17040 = NULL;
+       return 0;
+}
+
+static int max17040_suspend(struct i2c_client *client,
+               pm_message_t state)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       cancel_delayed_work(&chip->work);
+       return 0;
+}
+
+static int max17040_resume(struct i2c_client *client)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+       return 0;
+}
+
+static const struct i2c_device_id max17040_id[] = {
+       { "max17040", 0 },
+       { }
+};
+MODULE_DEVICE_TABLE(i2c, max17040_id);
+
+static struct i2c_driver max17040_i2c_driver = {
+       .driver = {
+               .name   = "max17040",
+       },
+       .probe          = max17040_probe,
+       .remove         = __devexit_p(max17040_remove),
+       .suspend        = max17040_suspend,
+       .resume         = max17040_resume,
+       .id_table       = max17040_id,
+};
+
+static int __init max17040_init(void)
+{
+       return i2c_add_driver(&max17040_i2c_driver);
+}
+module_init(max17040_init);
+
+static void __exit max17040_exit(void)
+{
+       i2c_del_driver(&max17040_i2c_driver);
+}
+module_exit(max17040_exit);
+
+MODULE_AUTHOR("Minkyu Kang <[email protected]>");
+MODULE_DESCRIPTION("MAX17040 Fuel Gauge");
+MODULE_LICENSE("GPL");

include/linux/max17040_battery.h new file +19 -1

new file mode 100644
index 0000000..ad97b06
@@ -0,0 +1,19 @@
+/*
+ *  Copyright (C) 2009 Samsung Electronics
+ *  Minkyu Kang <[email protected]>
+ *
+ * This program 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.
+ */
+
+#ifndef __MAX17040_BATTERY_H_
+#define __MAX17040_BATTERY_H_
+
+struct max17040_platform_data {
+       int (*battery_online)(void);
+       int (*charger_online)(void);
+       int (*charger_enable)(void);
+};
+
+#endif
--

1.5.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 3 lines

---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Jean Delvare

2009-06-04 09:35 UTC #6 lore raw

Quoted 25 lines · Trilok Soni

Hi Minkyu Kang,
Adding linux-i2c mailing list, so not deleting any code.
Quoted 20 lines · Minkyu Kang
The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries This patch adds support the MAX17040 Fuel Gauge
Cc Anton Vorontsov <cbou-JGs/[email protected]>
Signed-off-by Minkyu Kang <[email protected]>
---
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 3 +- drivers/power/max17040_battery.c | 306 ++++++++++++++++++++++++++++++++++++++ include/linux/max17040_battery.h | 19 +++
 4 files changed, 335 insertions(+), 1 deletions(-)
create mode 100644 drivers/power/max17040_battery.c create mode 100644 include/linux/max17040_battery.h (...)
drivers/power/max17040_battery.c new file +6 -0
+static u8 max17040_read_reg(int reg)
+{
+       int ret;
+
+       ret = i2c_smbus_read_byte_data(max17040->client, reg);
+

Quoted 8 lines

drivers/power/max17040_battery.c line 98
Quoted 8 lines
+ if (ret < 0) {
+ dev_err(&max17040->client->dev,
+ "%s: reg 0x%x, err %d\n",
+ __func__, reg, ret);
+ }
drivers/power/max17040_battery.c new file +3 -0
+
+       return ret;
+}

In case of error, this will wrap the error code into a u8 and the caller won't notice. So you'll return a random value, depending on the actual error which happened. No good.

You should either return an int there, and have the caller check for errors, or if you don't want to care about errors, return an arbitrary value on error (e.g. 0.)

Quoted 35 lines

Quoted 5 lines
(...)
drivers/power/max17040_battery.c new file +4 -0
+static int __devinit max17040_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct max17040_chip *chip;
drivers/power/max17040_battery.c line 219
Quoted 10 lines
drivers/power/max17040_battery.c new file +10 -0
+       int ret;
+
+       chip = kzalloc(sizeof(struct max17040_chip), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->client = client;
+       pdata = client->dev.platform_data;
+
+       i2c_set_clientdata(client, chip);
Please add i2c_check_functionality check before doing any smbus read/write operations.
Quoted 1 line
+ max17040 = chip;
This means that we support only one instance of this chip, right?
Quoted 12 lines
drivers/power/max17040_battery.c new file +12 -0
+
+       max17040_reset();
+
+       max17040_get_version();
+
+       INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+       bat_ps.properties = max17040_battery_props;
+       bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+       ret = power_supply_register(&client->dev, &bat_ps);

Quoted 8 lines

Quoted 8 lines
+ if (ret) {
+ dev_err(&max17040->client->dev,
+ "failed: power supply register\n");
+ cancel_delayed_work(&chip->work);
+ i2c_set_clientdata(client, NULL);
+ kfree(chip);
+ max17040 = NULL;
+ return -1;

Please come up with a better error code.

Quoted 4 lines

Quoted 4 lines
+ }
drivers/power/max17040_battery.c new file +3 -0
+
+       return 0;
+}

Signature · 1 line

Jean Delvare

Minkyu Kang

2009-06-04 10:55 UTC #7 lore raw

Dear, Jean Delvare

Quoted 9 lines

In case of error, this will wrap the error code into a u8 and the caller won't notice. So you'll return a random value, depending on the actual error which happened. No good.
You should either return an int there, and have the caller check for errors, or if you don't want to care about errors, return an arbitrary value on error (e.g. 0.)

Yes, I missed it.

Quoted 10 lines

Quoted 9 lines
Quoted 9 lines
+ ret = power_supply_register(&client->dev, &bat_ps);
+ if (ret) {
+ dev_err(&max17040->client->dev,
+ "failed: power supply register\n");
+ cancel_delayed_work(&chip->work);
+ i2c_set_clientdata(client, NULL);
+ kfree(chip);
+ max17040 = NULL;
+ return -1;

Quoted 2 lines

Please come up with a better error code.

Ok, many thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Minkyu Kang

2009-06-04 10:55 UTC #8 lore raw

In reply to Jean Delvare

Dear, Jean Delvare

Quoted 9 lines

In case of error, this will wrap the error code into a u8 and the caller won't notice. So you'll return a random value, depending on the actual error which happened. No good.
You should either return an int there, and have the caller check for errors, or if you don't want to care about errors, return an arbitrary value on error (e.g. 0.)

Yes, I missed it.

Quoted 10 lines

Quoted 9 lines
Quoted 9 lines
+ ret = power_supply_register(&client->dev, &bat_ps);
+ if (ret) {
+ dev_err(&max17040->client->dev,
+ "failed: power supply register\n");
+ cancel_delayed_work(&chip->work);
+ i2c_set_clientdata(client, NULL);
+ kfree(chip);
+ max17040 = NULL;
+ return -1;

Quoted 2 lines

Please come up with a better error code.

Ok, many thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Jean Delvare

2009-06-04 09:35 UTC #9 lore raw

In reply to Trilok Soni

Quoted 25 lines · Trilok Soni

Hi Minkyu Kang,
Adding linux-i2c mailing list, so not deleting any code.
Quoted 20 lines · Minkyu Kang
The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries This patch adds support the MAX17040 Fuel Gauge
Cc Anton Vorontsov <[email protected]>
Signed-off-by Minkyu Kang <[email protected]>
---
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 3 +- drivers/power/max17040_battery.c | 306 ++++++++++++++++++++++++++++++++++++++ include/linux/max17040_battery.h | 19 +++
 4 files changed, 335 insertions(+), 1 deletions(-)
create mode 100644 drivers/power/max17040_battery.c create mode 100644 include/linux/max17040_battery.h (...)
drivers/power/max17040_battery.c new file +6 -0
+static u8 max17040_read_reg(int reg)
+{
+       int ret;
+
+       ret = i2c_smbus_read_byte_data(max17040->client, reg);
+

Quoted 8 lines

drivers/power/max17040_battery.c line 98
Quoted 8 lines
+ if (ret < 0) {
+ dev_err(&max17040->client->dev,
+ "%s: reg 0x%x, err %d\n",
+ __func__, reg, ret);
+ }
drivers/power/max17040_battery.c new file +3 -0
+
+       return ret;
+}

In case of error, this will wrap the error code into a u8 and the caller won't notice. So you'll return a random value, depending on the actual error which happened. No good.

You should either return an int there, and have the caller check for errors, or if you don't want to care about errors, return an arbitrary value on error (e.g. 0.)

Quoted 35 lines

Quoted 5 lines
(...)
drivers/power/max17040_battery.c new file +4 -0
+static int __devinit max17040_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct max17040_chip *chip;
drivers/power/max17040_battery.c line 219
Quoted 10 lines
drivers/power/max17040_battery.c new file +10 -0
+       int ret;
+
+       chip = kzalloc(sizeof(struct max17040_chip), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->client = client;
+       pdata = client->dev.platform_data;
+
+       i2c_set_clientdata(client, chip);
Please add i2c_check_functionality check before doing any smbus read/write operations.
Quoted 1 line
+ max17040 = chip;
This means that we support only one instance of this chip, right?
Quoted 12 lines
drivers/power/max17040_battery.c new file +12 -0
+
+       max17040_reset();
+
+       max17040_get_version();
+
+       INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+       bat_ps.properties = max17040_battery_props;
+       bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+       ret = power_supply_register(&client->dev, &bat_ps);

Quoted 8 lines

Quoted 8 lines
+ if (ret) {
+ dev_err(&max17040->client->dev,
+ "failed: power supply register\n");
+ cancel_delayed_work(&chip->work);
+ i2c_set_clientdata(client, NULL);
+ kfree(chip);
+ max17040 = NULL;
+ return -1;

Please come up with a better error code.

Quoted 4 lines

Quoted 4 lines
+ }
drivers/power/max17040_battery.c new file +3 -0
+
+       return 0;
+}

Signature · 1 line

Jean Delvare

Minkyu Kang

2009-06-04 10:47 UTC #10 lore raw

In reply to Trilok Soni

Hi, Trilok

Quoted 1 line

Adding linux-i2c mailing list, so not deleting any code.

Ok, I will.

Quoted 10 lines

drivers/power/max17040_battery.c line 54
Quoted 2 lines
drivers/power/max17040_battery.c new file +2 -0
+static struct max17040_chip *max17040;
+static struct max17040_platform_data *pdata;
May be you want to move this pdata under chip structure.
Quoted 1 line
+ i2c_set_clientdata(client, chip);
Please add i2c_check_functionality check before doing any smbus read/write operations.

Ok, that's better.

Quoted 4 lines

Quoted 1 line
+ max17040 = chip;
This means that we support only one instance of this chip, right?

Yes right.. but, I think that is not a good way.
I'll modify it, too. thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Minkyu Kang

2009-06-04 10:47 UTC #11 lore raw

In reply to Trilok Soni

Hi, Trilok

Quoted 1 line

Adding linux-i2c mailing list, so not deleting any code.

Ok, I will.

Quoted 10 lines

drivers/power/max17040_battery.c line 54
Quoted 2 lines
drivers/power/max17040_battery.c new file +2 -0
+static struct max17040_chip *max17040;
+static struct max17040_platform_data *pdata;
May be you want to move this pdata under chip structure.
Quoted 1 line
+ i2c_set_clientdata(client, chip);
Please add i2c_check_functionality check before doing any smbus read/write operations.

Ok, that's better.

Quoted 4 lines

Quoted 1 line
+ max17040 = chip;
This means that we support only one instance of this chip, right?

Yes right.. but, I think that is not a good way.
I'll modify it, too. thanks :)

Signature · 2 lines

from. prom.
promsoft.net

Trilok Soni

2009-06-04 09:16 UTC #12 lore raw

In reply to Minkyu Kang

Hi Minkyu Kang,

Adding linux-i2c mailing list, so not deleting any code.

Quoted 93 lines · Minkyu Kang

The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries This patch adds support the MAX17040 Fuel Gauge

Cc Anton Vorontsov <[email protected]>

Signed-off-by Minkyu Kang <[email protected]>

---
drivers/power/Kconfig | 8 +
drivers/power/Makefile | 3 +- drivers/power/max17040_battery.c | 306 ++++++++++++++++++++++++++++++++++++++ include/linux/max17040_battery.h | 19 +++

 4 files changed, 335 insertions(+), 1 deletions(-)

create mode 100644 drivers/power/max17040_battery.c create mode 100644 include/linux/max17040_battery.h

drivers/power/Kconfig +0 -0

index 33da112..6af5798 100644
@@ -88,4 +88,12 @@ config CHARGER_PCF50633

help
Say Y to include support for NXP PCF50633 Main Battery Charger.

drivers/power/Kconfig +8 -0

+config BATTERY_MAX17040
+       tristate "Maxim MAX17040 Fuel Gauge"
+       depends on I2C
+       help
+         MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries
+         in handheld and portable equipment. The MAX17040 is configured
+         to operate with a single lithium cell
+

endif # POWER_SUPPLY

drivers/power/Makefile +0 -0

index 2fcf41d..9c48995 100644
@@ -25,4 +25,5 @@ obj-$(CONFIG_BATTERY_TOSA)    += tosa_battery.o

obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o

drivers/power/Makefile +2 -1

-obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
\ No newline at end of file
+obj-$(CONFIG_BATTERY_MAX17040))        += max17040_battery.o
+obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o

drivers/power/max17040_battery.c new file +44 -0

new file mode 100644
index 0000000..25303ba
@@ -0,0 +1,306 @@
+/*
+ *  max17040_battery.c
+ *  fuel-gauge systems for lithium-ion (Li+) batteries
+ *
+ *  Copyright (C) 2009 Samsung Electronics
+ *  Minkyu Kang <[email protected]>
+ *
+ * This program 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/power_supply.h>
+#include <linux/max17040_battery.h>
+
+#define MAX17040_VCELL_MSB     0x02
+#define MAX17040_VCELL_LSB     0x03
+#define MAX17040_SOC_MSB       0x04
+#define MAX17040_SOC_LSB       0x05
+#define MAX17040_MODE_MSB      0x06
+#define MAX17040_MODE_LSB      0x07
+#define MAX17040_VER_MSB       0x08
+#define MAX17040_VER_LSB       0x09
+#define MAX17040_RCOMP_MSB     0x0C
+#define MAX17040_RCOMP_LSB     0x0D
+#define MAX17040_CMD_MSB       0xFE
+#define MAX17040_CMD_LSB       0xFF
+
+#define MAX17040_DELAY         1000
+#define MAX17040_BATTERY_FULL  95
+
+struct max17040_chip {
+       struct i2c_client       *client;
+       struct delayed_work     work;
+
+       /* State Of Connect */
+       int online;

drivers/power/max17040_battery.c line 54

Quoted 10 lines

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

+       /* battery voltage */
+       int vcell;
+       /* battery capacity */
+       int soc;
+       /* State Of Charge */
+       int status;
+};
+
+static struct max17040_chip *max17040;
+static struct max17040_platform_data *pdata;

May be you want to move this pdata under chip structure.

Quoted 155 lines

drivers/power/max17040_battery.c new file +155 -0

+
+static int max17040_get_property(struct power_supply *bat_ps,
+                           enum power_supply_property psp,
+                           union power_supply_propval *val)
+{
+       switch (psp) {
+       case POWER_SUPPLY_PROP_STATUS:
+               val->intval = max17040->status;
+               break;
+       case POWER_SUPPLY_PROP_ONLINE:
+               val->intval = max17040->online;
+               break;
+       case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+               val->intval = max17040->vcell;
+               break;
+       case POWER_SUPPLY_PROP_CAPACITY:
+               val->intval = max17040->soc;
+               break;
+       default:
+               return -EINVAL;
+       }
+       return 0;
+}
+
+static struct power_supply bat_ps = {
+       .name           = "battery",
+       .type           = POWER_SUPPLY_TYPE_BATTERY,
+       .get_property   = max17040_get_property,
+};
+
+static int max17040_write_reg(int reg, u8 value)
+{
+       int ret;
+
+       ret = i2c_smbus_write_byte_data(max17040->client, reg, value);
+
+       if (ret < 0) {
+               dev_err(&max17040->client->dev,
+                               "%s: reg 0x%x, val 0x%x, err %d\n",
+                               __func__, reg, value, ret);
+       }
+
+       return ret;
+}
+
+static u8 max17040_read_reg(int reg)
+{
+       int ret;
+
+       ret = i2c_smbus_read_byte_data(max17040->client, reg);
+
+       if (ret < 0) {
+               dev_err(&max17040->client->dev,
+                               "%s: reg 0x%x, err %d\n",
+                               __func__, reg, ret);
+       }
+
+       return ret;
+}
+
+static void max17040_reset(void)
+{
+       max17040_write_reg(MAX17040_CMD_MSB, 0x54);
+       max17040_write_reg(MAX17040_CMD_LSB, 0x00);
+}
+
+static void max17040_set_rcomp(u16 val)
+{
+       max17040_write_reg(MAX17040_RCOMP_MSB, val >> 8);
+       max17040_write_reg(MAX17040_RCOMP_LSB, (val << 8) >> 8);
+}
+
+static void max17040_get_vcell(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_VCELL_MSB);
+       lsb = max17040_read_reg(MAX17040_VCELL_LSB);
+
+       max17040->vcell = (msb << 4) + (lsb >> 4);
+}
+
+static void max17040_get_soc(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_SOC_MSB);
+       lsb = max17040_read_reg(MAX17040_SOC_LSB);
+
+       max17040->soc = msb;
+}
+
+static void max17040_get_version(void)
+{
+       u8 msb;
+       u8 lsb;
+
+       msb = max17040_read_reg(MAX17040_VER_MSB);
+       lsb = max17040_read_reg(MAX17040_VER_LSB);
+
+       dev_info(&max17040->client->dev,
+                       "MAX17040 Fuel-Gauge Ver %d%d\n", msb, lsb);
+}
+
+static void max17040_get_online(void)
+{
+       if (pdata->battery_online)
+               max17040->online = pdata->battery_online();
+       else
+               max17040->online = 1;
+}
+
+static void max17040_get_status(void)
+{
+       if (!pdata->charger_online || !pdata->charger_enable) {
+               max17040->status = POWER_SUPPLY_STATUS_UNKNOWN;
+               return;
+       }
+
+       if (pdata->charger_online()) {
+               if (pdata->charger_enable())
+                       max17040->status = POWER_SUPPLY_STATUS_CHARGING;
+               else
+                       max17040->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+       } else {
+               max17040->status = POWER_SUPPLY_STATUS_DISCHARGING;
+       }
+
+       if (max17040->soc > MAX17040_BATTERY_FULL)
+               max17040->status = POWER_SUPPLY_STATUS_FULL;
+}
+
+static void max17040_work(struct work_struct *work)
+{
+       max17040_get_vcell();
+       max17040_get_soc();
+       max17040_get_online();
+       max17040_get_status();
+
+       schedule_delayed_work(&max17040->work, MAX17040_DELAY);
+}
+
+static enum power_supply_property max17040_battery_props[] = {
+       POWER_SUPPLY_PROP_STATUS,
+       POWER_SUPPLY_PROP_ONLINE,
+       POWER_SUPPLY_PROP_VOLTAGE_NOW,
+       POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int __devinit max17040_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct max17040_chip *chip;

drivers/power/max17040_battery.c line 219

Quoted 10 lines

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

+       int ret;
+
+       chip = kzalloc(sizeof(struct max17040_chip), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->client = client;
+       pdata = client->dev.platform_data;
+
+       i2c_set_clientdata(client, chip);

Please add i2c_check_functionality check before doing any smbus read/write operations.

drivers/power/max17040_battery.c line 220

Quoted 1 line

drivers/power/max17040_battery.c new file +1 -0
+       max17040 = chip;

This means that we support only one instance of this chip, right?

Quoted 119 lines

drivers/power/max17040_battery.c new file +86 -0

+
+       max17040_reset();
+
+       max17040_get_version();
+
+       INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+
+       bat_ps.properties = max17040_battery_props;
+       bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
+
+       ret = power_supply_register(&client->dev, &bat_ps);
+       if (ret) {
+               dev_err(&max17040->client->dev,
+                               "failed: power supply register\n");
+               cancel_delayed_work(&chip->work);
+               i2c_set_clientdata(client, NULL);
+               kfree(chip);
+               max17040 = NULL;
+               return -1;
+       }
+
+       return 0;
+}
+
+static int __devexit max17040_remove(struct i2c_client *client)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       power_supply_unregister(&bat_ps);
+       cancel_delayed_work(&chip->work);
+       i2c_set_clientdata(client, NULL);
+       kfree(chip);
+       max17040 = NULL;
+       return 0;
+}
+
+static int max17040_suspend(struct i2c_client *client,
+               pm_message_t state)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       cancel_delayed_work(&chip->work);
+       return 0;
+}
+
+static int max17040_resume(struct i2c_client *client)
+{
+       struct max17040_chip *chip = i2c_get_clientdata(client);
+
+       schedule_delayed_work(&chip->work, MAX17040_DELAY);
+       return 0;
+}
+
+static const struct i2c_device_id max17040_id[] = {
+       { "max17040", 0 },
+       { }
+};
+MODULE_DEVICE_TABLE(i2c, max17040_id);
+
+static struct i2c_driver max17040_i2c_driver = {
+       .driver = {
+               .name   = "max17040",
+       },
+       .probe          = max17040_probe,
+       .remove         = __devexit_p(max17040_remove),
+       .suspend        = max17040_suspend,
+       .resume         = max17040_resume,
+       .id_table       = max17040_id,
+};
+
+static int __init max17040_init(void)
+{
+       return i2c_add_driver(&max17040_i2c_driver);
+}
+module_init(max17040_init);
+
+static void __exit max17040_exit(void)
+{
+       i2c_del_driver(&max17040_i2c_driver);
+}
+module_exit(max17040_exit);
+
+MODULE_AUTHOR("Minkyu Kang <[email protected]>");
+MODULE_DESCRIPTION("MAX17040 Fuel Gauge");
+MODULE_LICENSE("GPL");

include/linux/max17040_battery.h new file +19 -1

new file mode 100644
index 0000000..ad97b06
@@ -0,0 +1,19 @@
+/*
+ *  Copyright (C) 2009 Samsung Electronics
+ *  Minkyu Kang <[email protected]>
+ *
+ * This program 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.
+ */
+
+#ifndef __MAX17040_BATTERY_H_
+#define __MAX17040_BATTERY_H_
+
+struct max17040_platform_data {
+       int (*battery_online)(void);
+       int (*charger_online)(void);
+       int (*charger_enable)(void);
+};
+
+#endif
--

1.5.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 3 lines

---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni