[PATCH] jz battery driver cleanup

Lars-Peter Clausen lars at metafoo.de
Sat Sep 19 16:19:40 EDT 2009


Hi JieJing.Zhang

looks good already :)
I commited the last part of your patch (the include guards for the
jz4740_fb.h)

Some more commtens for the battery driver:

> This patch is cleanup more about gpio and platform data. I don't
> test in board. But I noticed that qi_lb60.c also have set gpio as
> input, and the jz_battey do same by gpiolib, Is that cause problem?
> 
The gpio code in board_qi_lb60.c should go away.

>
>
> Signed-off-by: JieJing.Zhang <kzjeef at gmail.com> ---
> .../files-2.6.31/arch/mips/jz4740/platform.c       |   23 +++
> .../xburst/files-2.6.31/drivers/power/jz_battery.c |  152
> ++++++++++++++------ .../files-2.6.31/include/linux/jz4740_batt.h
> |   27 ++++ .../xburst/files-2.6.31/include/linux/jz4740_fb.h  |
> 5 + 4 files changed, 164 insertions(+), 43 deletions(-) create mode
> 100644 target/linux/xburst/files-2.6.31/include/linux/jz4740_batt.h
>
>
> diff --git
> a/target/linux/xburst/files-2.6.31/arch/mips/jz4740/platform.c
> b/target/linux/xburst/files-2.6.31/arch/mips/jz4740/platform.c
> index db3d045..b75ddde 100644 ---
> a/target/linux/xburst/files-2.6.31/arch/mips/jz4740/platform.c +++
> b/target/linux/xburst/files-2.6.31/arch/mips/jz4740/platform.c @@
> -19,6 +19,8 @@ #include <linux/mtd/jz4740_nand.h> #include
> <linux/spi/spi.h> #include <linux/spi/spi_gpio.h> +#include
> <linux/power_supply.h> +#include <linux/jz4740_batt.h>
>
> #include <asm/jzsoc.h> #include <asm/gpio.h> @@ -408,6 +410,26 @@
> static struct platform_device jz_codec_device = { .resource    =
> codec_resources, };
>
> +#define JZ_BAT_MAX_VOLTAGE 4200000 +#define JZ_BAT_MIN_VOLTAGE
> 3600000 +static struct jz_batt_info jz_batt_gpio_platform_data = {
> +    .dc_dect_gpio    = GPIO_DC_DETE_N, +    .usb_dect_gpio    =
GPIO_USB_DETE,
>  +    .charg_stat_gpio  = GPIO_CHARG_STAT_N, + +    .min_voltag    =
> JZ_BAT_MIN_VOLTAGE, +    .max_voltag    = JZ_BAT_MAX_VOLTAGE, +
> .batt_tech    = POWER_SUPPLY_TECHNOLOGY_LIPO, +}; + +static struct
> platform_device batt_gpio_device = { +    .name = "batt_gpio", +    .id =
> -1, +    .dev = { +        .platform_data = &jz_batt_gpio_platform_data, +
> }, +}; + /* All */ static struct platform_device
> *jz_platform_devices[] __initdata = { &jz_usb_ohci_device, @@
> -420,6 +442,7 @@ static struct platform_device
> *jz_platform_devices[] __initdata = { &qi_lb60_fb, &jz_i2s_device,
> &jz_codec_device, +    &batt_gpio_device, };
>
> static int __init jz_platform_init(void) diff --git
> a/target/linux/xburst/files-2.6.31/drivers/power/jz_battery.c
> b/target/linux/xburst/files-2.6.31/drivers/power/jz_battery.c index
> 80b3747..f9ef04f 100755 ---
> a/target/linux/xburst/files-2.6.31/drivers/power/jz_battery.c +++
> b/target/linux/xburst/files-2.6.31/drivers/power/jz_battery.c @@
> -6,6 +6,7 @@ * based on tosa_battery.c * * Copyright (C) 2008 Marek
> Vasut <marek.vasut at gmail.com> + * Copyright (C) 2009 Jiejing Zhang
> <kzjeef at gmail.com> * * 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 @@ -19,28 +20,19 @@ #include
> <linux/spinlock.h> #include <linux/interrupt.h> #include
> <linux/platform_device.h> +#include <linux/jz4740_batt.h>
>
> #include <asm/jzsoc.h>
>
> -#ifdef CONFIG_POWER_SUPPLY_DEBUG -#define dprintk(x...) printk(x)
> -#else -#define dprintk(x...) while(0){} -#endif - -#define
> JZ_BAT_MAX_VOLTAGE 4200000 // uV -#define JZ_BAT_MIN_VOLTAGE
> 3600000 - -static DEFINE_MUTEX(bat_lock); struct workqueue_struct
> *monitor_wqueue; struct delayed_work bat_work; struct mutex
> work_lock;
>
> -int bat_status = POWER_SUPPLY_STATUS_DISCHARGING; +static int
> bat_status = POWER_SUPPLY_STATUS_DISCHARGING; +static struct
> jz_batt_info *pdata = 0;
>
> extern unsigned int jz_read_battery(void);
>
> -
> /*********************************************************************
>  *        Power
> *********************************************************************/
>  @@ -52,9 +44,9 @@ static int jz_get_power_prop(struct power_supply
> *psy, switch (psp) { case POWER_SUPPLY_PROP_ONLINE: if (psy->type
> == POWER_SUPPLY_TYPE_MAINS) -            val->intval =
> !__gpio_get_pin(GPIO_DC_DETE_N); +            val->intval =
> !gpio_get_value(pdata->dc_dect_gpio); else -            val->intval =
> __gpio_get_pin(GPIO_USB_DETE); +            val->intval =
> !!gpio_get_value(pdata->usb_dect_gpio); break; default: return
> -EINVAL; @@ -92,13 +84,26 @@ static unsigned long
> jz_read_bat(struct power_supply *bat_ps) { unsigned long val; if
> (CFG_PBAT_DIV == 1) -        val = (((unsigned long
> long)jz_read_battery() * 7500000)) >> 12; +        val = (((unsigned long
> long)jz_read_battery() * 7500000L) >> 12) + 33000L; else -        val =
> (((unsigned long long)jz_read_battery() * CFG_PBAT_DIV * 2500000))
> >> 12; -    dprintk("--raw_batter_vol=%d uV\n", val); +        val =
> ((unsigned long long)jz_read_battery() * CFG_PBAT_DIV * 2500000L)
> >> 12; +    dev_dbg(bat_ps->dev, "%s: raw_batter_vol = %d
> uV\n",__func__,val); return val; }
>
> +static int jz_bat_get_capacity(struct power_supply *bat_ps) +{ +
> int ret; +    ret = (jz_read_bat(bat_ps) - pdata->min_voltag) * 100 +
> / (pdata->max_voltag - pdata->min_voltag); +    if (ret > 100) { +
> dev_warn(bat_ps->dev, "%s: capacity=%d which exceeds 100," +
> "set to 100\n", __func__, ret); +        ret = 100; +    } +    return
ret; +}
> + static int jz_bat_get_property(struct power_supply *bat_ps, enum
> power_supply_property psp, union power_supply_propval *val) @@
> -108,40 +113,37 @@ static int jz_bat_get_property(struct
> power_supply *bat_ps, val->intval = bat_status; break; case
> POWER_SUPPLY_PROP_TECHNOLOGY: -        val->intval =
> POWER_SUPPLY_TECHNOLOGY_LIPO; +        val->intval = pdata->batt_tech;
> break; case POWER_SUPPLY_PROP_HEALTH: -        if(jz_read_bat(bat_ps) <
> 3600000) { -            dprintk("--battery dead\n"); +
> if(jz_read_bat(bat_ps) < JZ_BAT_MIN_VOLTAGE) { +
> dev_dbg(bat_ps->dev, "%s: battery is dead," +                "voltage too
> low!\n", __func__); val->intval = POWER_SUPPLY_HEALTH_DEAD; } else
> { -            dprintk("--battery good\n"); +          
 dev_dbg(bat_ps->dev, "%s:
> battery is good," +                "voltage normal.\n", __func__);
val->intval
> = POWER_SUPPLY_HEALTH_GOOD; } break; case
> POWER_SUPPLY_PROP_CAPACITY: -        val->intval = (jz_read_bat(bat_ps) -
> 3600000) * 100 / (4200000 - 3600000); -        if (val->intval > 100) -
> val->intval = 100; -
> dprintk("--battery_capacity=%d\%\n",val->intval); +        val->intval =
> jz_bat_get_capacity(bat_ps); +        dev_dbg(bat_ps->dev, "%s:
> battery_capacity = %d\%\n", +            __func__, val->intval); break;
case
> POWER_SUPPLY_PROP_VOLTAGE_NOW: val->intval = jz_read_bat(bat_ps);
> break; case POWER_SUPPLY_PROP_VOLTAGE_MAX: case
> POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: -        val->intval =
> JZ_BAT_MAX_VOLTAGE; +        val->intval = pdata->max_voltag; break; case
> POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: -        val->intval =
> JZ_BAT_MIN_VOLTAGE; +        val->intval = pdata->min_voltag; break; case
> POWER_SUPPLY_PROP_PRESENT: val->intval = 1; break; -    case
> POWER_SUPPLY_PROP_TEMP: -    case POWER_SUPPLY_PROP_VOL: -
> val->intval = 0; // reading TEMP and VOL aren't supported -        break;
>  default: return -EINVAL; } @@ -168,18 +170,21 @@ static void
> jz_bat_update(struct power_supply *bat_ps) unsigned long batt_vol =
> jz_read_bat(bat_ps); mutex_lock(&work_lock);
>
> -    if(!__gpio_get_pin(GPIO_CHARG_STAT_N)) +
> if(!gpio_get_value(pdata->charg_stat_pgio)) bat_status =
> POWER_SUPPLY_STATUS_CHARGING; -    else { +    else bat_status =
> POWER_SUPPLY_STATUS_NOT_CHARGING; -    }
>
> -    dprintk("--battery status=%s\n", status_text[bat_status]); +
> dev_dbg(bat_ps->dev, "%s: battery status=%s\n", +        __func__,
> status_text[bat_status]); + if ((old_status != bat_status) ||
> (old_batt_vol - batt_vol > 50000)) { -        pr_debug("%s %s -> %s\n",
> bat_ps->name, -                status_text[old_status], -
> status_text[bat_status]); +        dev_dbg(bat_ps->dev, "%s %s -> %s\n",
> +             bat_ps->name, +             status_text[old_status], +
> status_text[bat_status]); + power_supply_changed(bat_ps); }
>
> @@ -196,8 +201,6 @@ static enum power_supply_property
> jz_bat_main_props[] = { POWER_SUPPLY_PROP_VOLTAGE_MAX,
> POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, POWER_SUPPLY_PROP_PRESENT, -
> POWER_SUPPLY_PROP_TEMP, -    POWER_SUPPLY_PROP_VOL, };
>
> struct power_supply bat_ps = { @@ -212,7 +215,8 @@ struct
> power_supply bat_ps = {
>
> static void jz_bat_work(struct work_struct *work) { -    const int
> interval = HZ * 6; +    /* query interval too small will increase
> system workload*/ +    const int interval = HZ * 30;
>
> jz_bat_update(&bat_ps); queue_delayed_work(monitor_wqueue,
> &bat_work, interval); @@ -242,12 +246,47 @@ static int
> jz_bat_resume(struct platform_device *dev) static int __devinit
> jz_bat_probe(struct platform_device *dev) { int ret = 0; +
> printk("JZ battery init.\n"); mutex_init(&work_lock); -
> INIT_DELAYED_WORK(&bat_work, jz_bat_work);
>
> -    __gpio_disable_pull(GPIO_USB_DETE); +    if
> (!dev->dev->platform_data) { +        dev_error(&dev->dev, "Please set
> battery info\n"); +        return -EINVAL; +    } + +    pdata =
> dev->dev->platform_data; + +    if (pdata->dc_dect_gpio >= 0 &&
> gpio_is_valid(pdata->dc_dect_gpio)) {
gpio_is_valid checks if the gpio is non negative. So its enough to
check gpio_is_valid
>
> +        ret = gpio_request(pdata->dc_dect_gpio, "AC/DC DECT"); +        if
> (ret) +            goto err_dc_gpio_request; +        ret =
> gpio_direction_input(pdata->dc_dect_gpio); +        if (ret) +      
     goto
> err_dc_gpio_direction; +    } + +    if (pdata->usb_dect_gpio >= 0 &&
> gpio_is_valid(pdata->usb_dect_gpio)) {
dito
>
> +        ret = gpio_request(pdata->usb_dect_gpio, "USB DECT"); +        if
> (ret) +            goto err_usb_gpio_request; +        ret =
> gpio_direction_input(pdata->usb_dect_gpio); +        if (ret) +      
     goto
> err_usb_gpio_direction; + +
> jz_gpio_disable_pullup(pdata->usb_dect_gpio); +        /* TODO: Use
> generic gpio is better */ +    } + +    if (pdata->charg_stat_gpio >= 0
> && gpio_is_valid(pdata->charg_stat_gpio)) {
dito
>
> +        ret = gpio_request(pdata->charg_stat_gpio, "CHARG STAT"); +   
    if
> (ret) +            goto err_charg_gpio_request; +        ret =
> gpio_direction_input(pdata->charg_stat_pgio); +        if (ret) +      
     goto
> err_charg_gpio_direction; +    }
>
> power_supply_register(&dev->dev, &jz_ac);
> power_supply_register(&dev->dev, &jz_usb);
power_supply_register can fail. Please check its return value and
unregister already registered power supplies if one fails.
>
> @@ -262,10 +301,37 @@ static int __devinit jz_bat_probe(struct
> platform_device *dev) }
>
> return ret; + +err_charg_gpio_direction +    dev_err(dev->dev,
> "charger state gpio set direction failed.\n");
The error message should be not be printed here. If setting direction
for the charge gpio you'll see all following error messages aswell.
The error messages should be printed before you goto the error label.
>
> +    gpio_free(pdata->charg_stat_pgio); +err_charg_gpio_request: +
> dev_err(dev->dev, "charger state gpio request failed.\n");
> +err_usb_gpio_direction: +    dev_err(dev->dev, "usb dect gpio set
> direction failed.\n"); +    gpio_free(pdata->usb_dect_gpio);
> +err_usb_gpio_request: +    dev_err(dev->dev, "usb dect gpio request
> failed.\n"); +err_dc_gpio_direction: +    dev_err(dev->dev, "ac/dc
> dect gpio request failed.\n"); +    gpio_free(pdata->dc_dect_gpio);
> +err_err_dc_gpio_request: +    dev_err(dev->dev, "ac/dc dect gpio
> request failed.\n"); +    return ret; + }
>
> static int __devexit jz_bat_remove(struct platform_device *dev) { +
> if (pdata) { +        if (pdata->dc_dect_gpio >= 0)
Use gpio_is_valid
>
> +            gpio_free(pdata->dc_dect_gpio); +        if
> (pdata->usb_dect_gpio >= 0)
dito
>
> +            gpio_free(pdata->usb_dect_pgio); +        if
> (pdata->charg_stat_gpio >= 0)
dito
>
> +            gpio_free(pdata->charg_stat_gpio); +    } +
> power_supply_unregister(&bat_ps);
The other power supplies should be unregistered aswell
>
> return 0; } diff --git
> a/target/linux/xburst/files-2.6.31/include/linux/jz4740_batt.h
> b/target/linux/xburst/files-2.6.31/include/linux/jz4740_batt.h new
> file mode 100644 index 0000000..bef9412 --- /dev/null +++
> b/target/linux/xburst/files-2.6.31/include/linux/jz4740_batt.h @@
> -0,0 +1,27 @@ +/* + *  Copyright (C) 2009, Jiejing Zhang
> <kzjeef at gmail.com> + * + *  This program is free software; you can
> redistribute     it and/or modify it + *  under  the terms of     the GNU
> General  Public License as published by the + *  Free Software
> Foundation;  either version 2 of the    License, or (at your + *
> option) any later version. + * + *  You should have received a copy
> of the  GNU General Public License along + *  with this program; if
> not, write  to the Free Software Foundation, Inc., + *  675 Mass
> Ave, Cambridge, MA 02139, USA. + * + */ + +#ifndef JZ4740_BATT_H
> +#define JZ4740_BATT_H + +struct jz_batt_info { +    int dc_dect_gpio;
> /* GPIO port of DC charger detection */ +    int usb_dect_gpio;    /*
> GPIO port of USB charger detection */ +    int charg_stat_gpio;    /*
> GPIO port of Charger state */ + +    int min_voltag;        /* Mininal
> battery voltage in uV */ +    int max_voltag;        /* Maximum battery
> voltage in uV */ +    int batt_tech;        /* Battery technoledge */ +};
> +#endif




More information about the discussion mailing list


interactive