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

Felipe Balbi
2008-10-18T22:30:04Z

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