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@xxxxxxxxx> > > --- > > 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 -- 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
