Hi Felipe, Thanks for the comments. I will fix them and resend the patch. Please note my view on the below point. > > - pdev = platform_device_alloc("omap-bq2700-battery", id); > > + pdev = platform_device_alloc("bq27000-bat", id); > > do you really need to change the name here? > It seems that this change doesn't belong to this patch. I will make this as a separate patch.As BQ27000 chip is not omap specific, I guess the above name is not correct. Hence I intend to make that change. Regards, Madhu > -----Original Message----- > From: Felipe Balbi [mailto:me@xxxxxxxxxxxxxxx] > Sent: Friday, April 25, 2008 4:20 PM > To: Madhusudhan Chikkature Rajashekar > Cc: Tony Lindgren; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [RFC/PATCH] BQ27000/BQ27200 battery monitoring > driver for OMAP34xx > > Some cosmetic comments below > > > > On Fri, 25 Apr 2008 15:01:34 +0530, "Madhusudhan Chikkature > Rajashekar" > <madhu.cr@xxxxxx> wrote: > > This patch provides the battery monitoring driver to > monitor batteries > with > > BQ27000(HDQ) or BQ27200(i2C) chips. > > On omap 3430 sdp BQ27000 battery is tested through the HDQ interface > which > > uses the earlier posted HDQ and BQ slave drivers. > > The BQ27200 support is an extension to the BQ27000 driver > as the register > > set between BQ27000 and BQ27200 is the same. > > > > Thanks to Klaus.K Pedersen and Mikko Ylinen for the > suggestions to add > > support for BQ27200 chip as part of > > this driver. > > > > > > Signed-off-by: Madhusudhan Chikkature<madhu.cr@xxxxxx> > > > > --- > > drivers/power/Kconfig | 21 + > > drivers/power/Makefile | 1 > > drivers/power/bq27x00_battery.c | 556 > > ++++++++++++++++++++++++++++++++++++++++ > > drivers/w1/slaves/w1_bq27000.c | 2 > > 4 files changed, 579 insertions(+), 1 deletion(-) > > > > Index: linux-omap-2.6/drivers/power/bq27x00_battery.c > > =================================================================== > > --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > > +++ linux-omap-2.6/drivers/power/bq27x00_battery.c 2008-04-24 > > 10:50:53.631358952 +0530 > > @@ -0,0 +1,556 @@ > > +/* > > + * 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> > > + > > +#if defined(CONFIG_BATTERY_BQ27000) > > Here use ifdef > > > +#define BATTERY_BQ27000 > > there's no need to add this one as you can use the Kconfig option > > > +#include "../w1/w1.h" > > +#endif > > +#if defined(CONFIG_BATTERY_BQ27200) > > +#define 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) > > + > > +#if defined(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; > > +#if defined(BATTERY_BQ27000) > > #ifdef CONFIG_BATERRY_BQ27000 > > > + struct device *w1_dev; > > +#endif > > +#if defined(BATTERY_BQ27200) > > #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); > > + > > +#if defined(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 > > + > > +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 > > + .driver = { > > + .name = "bq27000-bat", > > + }, > > +}; > > +#endif > > + > > +#if defined(BATTERY_BQ27200) > > +static int bq27200_battery_probe(struct i2c_client *client); > > +static int bq27200_battery_remove(struct i2c_client *client); > > +static int bq27200_battery_suspend(struct i2c_client *client, > > + pm_message_t mesg); > > +static int bq27200_battery_resume(struct i2c_client *client); > > +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 > > +}; > > +#endif > > + > > +/* > > + * 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; > > +} > > + > > +#if defined(BATTERY_BQ27000) > > +static 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 > > +#define bq27000_read NULL > > this should be defined as 0 or an static inline int [...] { > return 0; } > > > +#endif > > + > > +#if defined(BATTERY_BQ27200) > > +static 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 > > +#define bq27200_read NULL > > same here > > > +#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 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 > > +#endif > > put a comment indicating which ifdef you're closing, like: > > #endif /* CONFIG_PM */ > > > + > > +#ifdef 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 > > +#endif > > + > > +static int __init bq27x00_battery_init(void) > > +{ > > + int status = 0; > > + > > +#if defined(BATTERY_BQ27000) > > + status = platform_driver_register(&bq27000_battery_driver); > > + if (status) > > + printk(KERN_ERR "Unable to register BQ27000 driver\n"); > > +#endif > > +#if defined(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) > > +{ > > +#if defined(BATTERY_BQ27000) > > + platform_driver_unregister(&bq27000_battery_driver); > > +#endif > > +#if defined(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"); > > Index: linux-omap-2.6/drivers/power/Kconfig > > =================================================================== > > --- linux-omap-2.6.orig/drivers/power/Kconfig 2008-04-08 > > 17:15:04.000000000 +0530 > > +++ linux-omap-2.6/drivers/power/Kconfig 2008-04-21 > 08:25:00.000000000 > > +0530 > > @@ -49,4 +49,25 @@ > > help > > Say Y to enable support for the battery on the OLPC laptop. > > > > +config BATTERY_BQ27x00 > > + tristate "BQ27x00 battery driver" > > + help > > + Say Y here to enable support for batteries with > BQ27000 or BQ27200 > > chip. > > + > > +config BATTERY_BQ27000 > > + bool "BQ27000 battery driver" > > + depends on BATTERY_BQ27x00 > > + select 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. > > + > > endif # POWER_SUPPLY > > Index: linux-omap-2.6/drivers/power/Makefile > > =================================================================== > > --- linux-omap-2.6.orig/drivers/power/Makefile 2008-04-08 > > 17:15:04.000000000 +0530 > > +++ linux-omap-2.6/drivers/power/Makefile 2008-04-20 > 17:03:51.000000000 > > +0530 > > @@ -20,3 +20,4 @@ > > 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 > > Index: linux-omap-2.6/drivers/w1/slaves/w1_bq27000.c > > =================================================================== > > --- linux-omap-2.6.orig/drivers/w1/slaves/w1_bq27000.c > 2008-04-08 > > 17:15:05.000000000 +0530 > > +++ linux-omap-2.6/drivers/w1/slaves/w1_bq27000.c 2008-04-20 > > 17:08:55.000000000 +0530 > > @@ -59,7 +59,7 @@ > > int id = 1; > > struct platform_device *pdev; > > > > - pdev = platform_device_alloc("omap-bq2700-battery", id); > > + pdev = platform_device_alloc("bq27000-bat", id); > > do you really need to change the name here? > It seems that this change doesn't belong to this patch. > > > if (!pdev) { > > ret = -ENOMEM; > > return ret; > > > > -- > > To unsubscribe from this list: send the line "unsubscribe > linux-omap" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > Best Regards, > > Felipe Balbi > http://felipebalbi.com > me@xxxxxxxxxxxxxxx > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html