lkml.org 
[lkml]   [2010]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] power: bq24617: Adding initial charger support
    Hi

    rklein@nvidia.com wrote:
    > From: Rhyland Klein <rklein@nvidia.com>
    >
    > Initial checkin adding basic support for the TI BQ24617 battery charger where
    > the PG output is directed to the driver via a gpio. This gpio address needs to
    > be passed in the platform data to the driver.

    There is not really anything specific to the BQ24617 in this driver except for the
    naming. It is a driver for charger which reports its online property through a gpio
    pin. I wrote a similar which I wanted to submit for 2.6.38. I'll send it as a reply
    to this mail. You'll might be able to reuse it.

    >
    > Signed-off-by: Rhyland Klein <rklein@nvidia.com>
    > ---
    > Addressed comments by Mark Brown
    > * Removed dependency on TEGRA, changed to GPIO
    > * now using power_supply_changed()
    > * now using request_threaded_irq()
    >
    > drivers/power/Kconfig | 7 ++
    > drivers/power/Makefile | 1 +
    > drivers/power/bq24617.c | 262 +++++++++++++++++++++++++++++++++++++++++++++++
    > include/linux/bq24617.h | 10 ++
    > 4 files changed, 280 insertions(+), 0 deletions(-)
    > create mode 100644 drivers/power/bq24617.c
    > create mode 100644 include/linux/bq24617.h
    >
    > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
    > index 60d83d9..1901ccf 100644
    > --- a/drivers/power/Kconfig
    > +++ b/drivers/power/Kconfig
    > @@ -185,4 +185,11 @@ config CHARGER_TWL4030
    > help
    > Say Y here to enable support for TWL4030 Battery Charge Interface.
    >
    > +config CHARGER_BQ24617
    > + tristate "BQ24617 battery charger"
    > + depends on I2C && GENERIC_GPIO

    The driver does not use any I2C functions, so it shouldn't depend on I2C

    > + help
    > + Say Y to include support for the TI BQ24617 battery charger where PG
    > + is attached to a gpio.
    > +
    > endif # POWER_SUPPLY
    > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
    > index c75772e..45cd557 100644
    > --- a/drivers/power/Makefile
    > +++ b/drivers/power/Makefile
    > @@ -32,3 +32,4 @@ obj-$(CONFIG_BATTERY_JZ4740) += jz4740-battery.o
    > obj-$(CONFIG_BATTERY_INTEL_MID) += intel_mid_battery.o
    > obj-$(CONFIG_CHARGER_ISP1704) += isp1704_charger.o
    > obj-$(CONFIG_CHARGER_TWL4030) += twl4030_charger.o
    > +obj-$(CONFIG_CHARGER_BQ24617) += bq24617.o
    > diff --git a/drivers/power/bq24617.c b/drivers/power/bq24617.c
    > new file mode 100644
    > index 0000000..64500e6
    > --- /dev/null
    > +++ b/drivers/power/bq24617.c
    > @@ -0,0 +1,262 @@
    > +/*
    > + * Charger driver for TI's BQ24617
    > + *
    > + * Copyright (c) 2010, NVIDIA Corporation.
    > + *
    > + * 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.
    > + *
    > + * This program is distributed in the hope that it will be useful, but WITHOUT
    > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
    > + * more details.
    > + *
    > + * 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.,
    > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
    > + */
    > +
    > +#include <linux/init.h>
    > +#include <linux/module.h>
    > +#include <linux/kernel.h>
    > +#include <linux/err.h>
    > +#include <linux/power_supply.h>
    > +#include <linux/slab.h>
    > +#include <linux/gpio.h>
    > +#include <linux/bq24617.h>
    > +#include <linux/interrupt.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/irq.h>
    > +
    > +struct bq24617_info {
    > + struct power_supply power_supply;
    > + struct bq24617_platform_data *pdata;
    > + struct platform_device *pdev;
    > + struct mutex work_lock;
    > + struct work_struct ac_work;
    > + int status;
    > +};
    > +
    > +static enum power_supply_property bq24617_properties[] = {
    > + POWER_SUPPLY_PROP_ONLINE,
    > +};
    > +
    > +static void bq24617_batt_update(struct bq24617_info *chip)
    > +{
    > + int old_status = chip->status;
    > + int new_status = old_status;
    > + struct bq24617_platform_data *pdata;
    > + int gpio_value = 0;
    > +
    > + pdata = chip->pdata;
    > +
    > + mutex_lock(&chip->work_lock);
    > +
    > + gpio_value = gpio_get_value(pdata->gpio_addr);
    > +
    > + new_status = !gpio_value;
    > + chip->status = new_status;
    > +
    > + mutex_unlock(&chip->work_lock);

    The mutex in its current form is quite useless since it does not protect against
    anything. In my opinion it can be dropped completly. There is then a chance that
    power_supply_changed is called twice although there was only one change, but that
    should not cause any problems.
    If you want to keep the lock you should move "old_status = chip->status" inside the
    the lock (And gpio_get_value out of it).

    > +
    > + if (old_status != -1 &&

    If old_status is -1 it wont be equal to new_status so there is no need for an extra
    check.

    > + old_status != new_status) {
    > + dev_dbg(&chip->pdev->dev,
    > + "%s: %i -> %i\n", __func__, old_status,
    > + new_status);
    > + power_supply_changed(&chip->power_supply);
    > + }
    > +
    > +}
    > +
    > +static irqreturn_t bq24617_irq_switch(int irq, void *devid)
    > +{
    > + struct bq24617_info *chip = devid;
    > +
    > + schedule_work(&chip->ac_work);

    What Mark meant is that you should use a threaded irq so you can get rid of the
    workqueue and call bq24617_batt_update directly from inside the irq handler.

    > +
    > + return IRQ_HANDLED;
    > +}
    > +
    > +static int bq24617_get_property(struct power_supply *psy,
    > + enum power_supply_property psp,
    > + union power_supply_propval *val)
    > +{
    > + struct bq24617_info *chip = container_of(psy, struct bq24617_info,
    > + power_supply);
    > +
    > + switch (psp) {
    > + case POWER_SUPPLY_PROP_ONLINE:
    > + bq24617_batt_update(chip);
    > + val->intval = chip->status;
    > + break;
    > + default:
    > + dev_err(&chip->pdev->dev,
    > + "%s: Unknown property requested.\n", __func__);
    > + return -EINVAL;
    > + break;
    > + }
    > + return 0;
    > +}
    > +
    > +static void bq24617_ac_work(struct work_struct *work)
    > +{
    > + struct bq24617_info *chip;
    > + chip = container_of(work, struct bq24617_info, ac_work);
    > + bq24617_batt_update(chip);
    > +}
    > +
    > +static int bq24617_probe(struct platform_device *pdev)

    __devinit

    > +{
    > + struct bq24617_info *chip;
    > + struct bq24617_platform_data *pdata = pdev->dev.platform_data;
    > + int rc;
    > +
    > + if (!pdata) {
    > + dev_err(&pdev->dev, "No platform data\n");
    > + return -ENXIO;
    > + }
    > +
    > + if (!(gpio_is_valid(pdata->gpio_addr))) {
    > + dev_err(&pdev->dev, "Gpio is not valid\n");
    > + return -EINVAL;
    > + }
    > +
    > + chip = kzalloc(sizeof(struct bq24617_info), GFP_KERNEL);
    > + if (!chip)
    > + return -ENOMEM;
    > +
    > + chip->pdata = pdata;
    > + chip->pdev = pdev;
    > + chip->power_supply.name = "ac";
    > + chip->status = -1; /* set status to reflect unset */
    > + chip->power_supply.type = POWER_SUPPLY_TYPE_MAINS;
    > + chip->power_supply.properties = bq24617_properties;
    > + chip->power_supply.num_properties = ARRAY_SIZE(bq24617_properties);
    > + chip->power_supply.get_property = bq24617_get_property;
    > + chip->power_supply.supplied_to = pdata->batteries;
    > + chip->power_supply.num_supplicants = pdata->num_batteries;
    > +
    > + mutex_init(&chip->work_lock);
    > +
    > + platform_set_drvdata(pdev, chip);
    > +
    > + rc = gpio_request(pdata->gpio_addr, "ac online");
    > + if (rc) {
    > + dev_err(&pdev->dev,
    > + "%s: Failed to get gpio\n", __func__);
    > + goto free_chip;
    > + }
    > +
    > + gpio_direction_input(pdata->gpio_addr);
    > + if (rc) {
    > + dev_err(&pdev->dev,
    > + "%s: Failed to set gpio direction\n",
    > + __func__);
    > + goto release_gpio;
    > + }
    > +
    > + rc = request_threaded_irq (gpio_to_irq(pdata->gpio_addr),
    > + NULL,
    > + bq24617_irq_switch,
    > + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
    > + "ac detect",
    > + chip);
    > +
    > + if (rc) {
    > + dev_err(&pdev->dev,
    > + "%s: Failed to request threaded IRQ\n", __func__);
    > + goto release_gpio;
    > + }
    > +
    > + INIT_WORK(&chip->ac_work, bq24617_ac_work);

    In theory the irq can fire before the work_struct has been initalised.

    > +
    > + rc = power_supply_register(&pdev->dev, &chip->power_supply);
    > + if (rc) {
    > + dev_err(&pdev->dev,
    > + "%s: Failed to register power supply\n", __func__);
    > + goto free_irq;
    > + }
    > +
    > + dev_info(&pdev->dev,
    > + "%s: battery charger ac device registered\n", pdev->name);
    > +
    > + schedule_work(&chip->ac_work);
    > +
    > + return 0;
    > +
    > +free_irq:
    > + free_irq(gpio_to_irq(pdata->gpio_addr), chip);
    > +release_gpio:
    > + gpio_free(pdata->gpio_addr);
    > +free_chip:
    > + kfree(chip);
    > + return rc;
    > +}
    > +
    > +static int bq24617_remove(struct platform_device *pdev)

    __devexit

    > +{
    > + struct bq24617_info *chip = platform_get_drvdata(pdev);
    > + struct bq24617_platform_data *pdata = chip->pdata;
    > +
    > + flush_scheduled_work();
    > +
    > + power_supply_unregister(&chip->power_supply);
    > +
    > + free_irq(gpio_to_irq(pdata->gpio_addr), chip);
    > + gpio_free(pdata->gpio_addr);
    > + kfree(chip);
    > +
    > + return 0;
    > +}
    > +
    > +#if CONFIG_PM
    > +static int bq24617_suspend(struct platform_device *pdev, pm_message_t state)
    > +{
    > + flush_scheduled_work();
    > +
    > + return 0;
    > +}
    > +
    > +static int bq24617_resume(struct platform_device *pdev)
    > +{
    > + struct bq24617_info *chip = platform_get_drvdata(pdev);
    > +
    > + schedule_work(&chip->ac_work);
    > +
    > + return 0;
    > +}
    > +
    > +#else
    > +#define bq24617_suspend NULL
    > +#define bq24617_resume NULL
    > +#endif
    > +
    > +static struct platform_driver bq24617_driver = {
    > + .driver = {
    > + .name = "bq24617",
    > + .owner = THIS_MODULE,
    > + },
    > + .probe = bq24617_probe,
    > + .remove = __devexit_p(bq24617_remove),
    > + .suspend = bq24617_suspend,
    > + .resume = bq24617_resume,
    > +};
    > +
    > +static int __devinit bq24617_init(void)

    __init

    > +{
    > + return platform_driver_register(&bq24617_driver);
    > +}
    > +module_init(bq24617_init);
    > +
    > +static void __devexit bq24617_exit(void)

    __exit

    > +{
    > + platform_driver_unregister(&bq24617_driver);
    > +}
    > +module_exit(bq24617_exit);
    > +
    > +MODULE_AUTHOR("Rhyland Klein <rklein@nvidia.com");
    > +MODULE_DESCRIPTION("BQ24617 battery charger driver");
    > +MODULE_LICENSE("GPL");


    - Lars



    \
     
     \ /
      Last update: 2010-10-21 14:23    [W:0.048 / U:1.024 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site