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

Anton Vorontsov
2008-10-18T21:36:02Z

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