Messages in this thread | | | Date | Thu, 21 Oct 2010 14:20:09 +0200 | From | Lars-Peter Clausen <> | Subject | Re: [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
| |