[PATCH v2] power: bq24617: Adding initial charger support

[PATCH v2] power: bq24617: Adding initial charger support

[rklein]

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.

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
+	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);
+
+	if (old_status != -1 &&
+		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);
+
+	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)
+{
+	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);
+
+	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)
+{
+	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)
+{
+	return platform_driver_register(&bq24617_driver);
+}
+module_init(bq24617_init);
+
+static void __devexit bq24617_exit(void)
+{
+	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");
diff --git a/include/linux/bq24617.h b/include/linux/bq24617.h
new file mode 100644
index 0000000..4c58406
--- /dev/null
+++ b/include/linux/bq24617.h
@@ -0,0 +1,10 @@
+#ifndef _LINUX_BQ24617_H
+#define _LINUX_BQ24617_H
+
+struct bq24617_platform_data {
+	int	gpio_addr;
+	char	**batteries;
+	int	num_batteries;
+};
+
+#endif
-- 
1.7.0.4

Re: [PATCH v2] power: bq24617: Adding initial charger support

[Lars-Peter Clausen]

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

[PATCH] POWER: Add gpio chager driver

[Lars-Peter Clausen]

This patch adds a simple driver for chargers indicating their online status
through a GPIO pin.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/power/Kconfig              |   10 ++
 drivers/power/Makefile             |    1 +
 drivers/power/gpio-charger.c       |  184 ++++++++++++++++++++++++++++++++++++
 include/linux/power/gpio-charger.h |   39 ++++++++
 4 files changed, 234 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/gpio-charger.c
 create mode 100644 include/linux/power/gpio-charger.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 0734356..90b1940 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -166,4 +166,14 @@ config BATTERY_INTEL_MID
 	  Say Y here to enable the battery driver on Intel MID
 	  platforms.
 
+config CHARGER_GPIO
+	tristate "GPIO charger"
+	depends on GPIOLIB
+	help
+	  Say Y to include support for chargers which report their online status
+	  through a GPIO pin.
+
+	  This driver can be build as a module. If so, the module will be
+	  called gpio-chager.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 10143aa..2ca71ac 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_BATTERY_S3C_ADC)	+= s3c_adc_battery.o
 obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
 obj-$(CONFIG_BATTERY_JZ4740)	+= jz4740-battery.o
 obj-$(CONFIG_BATTERY_INTEL_MID)	+= intel_mid_battery.o
+obj-$(CONFIG_CHARGER_GPIO)	+= gpio-charger.o
diff --git a/drivers/power/gpio-charger.c b/drivers/power/gpio-charger.c
new file mode 100644
index 0000000..0a2ddfc
--- /dev/null
+++ b/drivers/power/gpio-charger.c
@@ -0,0 +1,184 @@
+/*
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *  Driver for chargers which report their online status through a GPIO pin
+ *
+ *  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.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+
+#include <linux/power/gpio-charger.h>
+
+struct gpio_charger {
+	const struct gpio_charger_platform_data *pdata;
+	int irq;
+
+	struct power_supply charger;
+};
+
+static irqreturn_t gpio_charger_irq(int irq, void *devid)
+{
+	struct power_supply *charger = devid;
+	power_supply_changed(charger);
+
+	return IRQ_HANDLED;
+}
+
+static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
+{
+	return container_of(psy, struct gpio_charger, charger);
+}
+
+static int gpio_charger_get_property(struct power_supply *psy,
+	enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
+	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = gpio_get_value(pdata->gpio);
+		val->intval ^= pdata->gpio_active_low;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property gpio_charger_properties[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int __devinit gpio_charger_probe(struct platform_device *pdev)
+{
+	const struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_charger *gpio_charger;
+	struct power_supply *charger;
+	int ret;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data");
+		return -EINVAL;
+	}
+
+	if (!gpio_is_valid(pdata->gpio)) {
+		dev_err(&pdev->dev, "Invalid gpio pin\n");
+		return -EINVAL;
+	}
+
+	gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
+
+	charger = &gpio_charger->charger;
+
+	charger->name = pdata->name;
+	charger->type = pdata->type;
+	charger->properties = gpio_charger_properties;
+	charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
+	charger->get_property  = gpio_charger_get_property;
+	charger->supplied_to = pdata->supplied_to;
+	charger->num_supplicants = pdata->num_supplicants;
+
+	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
+		goto err_free;
+	}
+	ret = gpio_direction_input(pdata->gpio);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
+		goto err_gpio_free;
+	}
+
+	gpio_charger->irq = gpio_to_irq(pdata->gpio);
+	if (gpio_charger->irq >= 0) {
+		ret = request_irq(gpio_charger->irq, gpio_charger_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				dev_name(&pdev->dev), charger);
+		if (ret) {
+			dev_warn(&pdev->dev, "Failed to request gpio irq: %d\n", ret);
+			gpio_charger->irq = -1;
+		}
+	}
+
+	gpio_charger->pdata = pdata;
+
+	ret = power_supply_register(&pdev->dev, charger);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register power supply: %d\n", ret);
+		goto err_irq_free;
+	}
+
+	platform_set_drvdata(pdev, gpio_charger);
+
+	return 0;
+
+err_irq_free:
+	if (gpio_charger->irq >= 0)
+		free_irq(gpio_charger->irq, charger);
+err_gpio_free:
+	gpio_free(pdata->gpio);
+err_free:
+	kfree(gpio_charger);
+	return ret;
+}
+
+static int __devexit gpio_charger_remove(struct platform_device *pdev)
+{
+	struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
+	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
+
+	power_supply_unregister(&gpio_charger->charger);
+
+	if (gpio_charger->irq >= 0)
+		free_irq(gpio_charger->irq, &gpio_charger->charger);
+	gpio_free(pdata->gpio);
+
+	platform_set_drvdata(pdev, NULL);
+	kfree(gpio_charger);
+
+	return 0;
+}
+
+static struct platform_driver  gpio_charger_driver = {
+	.probe = gpio_charger_probe,
+	.remove = __devexit_p(gpio_charger_remove),
+	.driver = {
+		.name = "gpio-charger",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init gpio_charger_init(void)
+{
+	return platform_driver_register(&gpio_charger_driver);
+}
+module_init(gpio_charger_init);
+
+static void __exit gpio_charger_exit(void)
+{
+	platform_driver_unregister(&gpio_charger_driver);
+}
+module_exit(gpio_charger_exit);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
+MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-charger");
diff --git a/include/linux/power/gpio-charger.h b/include/linux/power/gpio-charger.h
new file mode 100644
index 0000000..404d5e6
--- /dev/null
+++ b/include/linux/power/gpio-charger.h
@@ -0,0 +1,39 @@
+/*
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *
+ *  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 __LINUX_POWER_GPIO_CHARGER_H__
+#define __LINUX_POWER_GPIO_CHARGER_H__
+
+/*
+ * struct gpio_charger_platform_data - platform_data for gpio_charger devices
+ * @name:		Name for the chargers power_supply device
+ * @type:		Type of the charger
+ * @gpio:		GPIO which is used to indicate the chargers status
+ * @gpio_active_low:	Should be set to 1 if the GPIO is active low otherwise 0
+ * @supplied_to:	Array of battery names to which this chargers supplies power
+ * @num_supplicants:	Number of entries in the supplied_to array
+ */
+
+struct gpio_charger_platform_data {
+	const char *name;
+	enum power_supply_type type;
+
+	int gpio;
+	int gpio_active_low;
+
+	char **supplied_to;
+	size_t num_supplicants;
+};
+
+#endif
-- 
1.5.6.5

Re: [PATCH] POWER: Add gpio chager driver

[Piotr Hosowicz]

On 21.10.2010 14:25, Lars-Peter Clausen wrote:
> This patch adds a simple driver for chargers indicating their online status
> through a GPIO pin.
>
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
> ---
>   drivers/power/Kconfig              |   10 ++
>   drivers/power/Makefile             |    1 +
>   drivers/power/gpio-charger.c       |  184 ++++++++++++++++++++++++++++++++++++
>   include/linux/power/gpio-charger.h |   39 ++++++++
>   4 files changed, 234 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/power/gpio-charger.c
>   create mode 100644 include/linux/power/gpio-charger.h
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 0734356..90b1940 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -166,4 +166,14 @@ config BATTERY_INTEL_MID
>   	  Say Y here to enable the battery driver on Intel MID
>   	  platforms.
>
> +config CHARGER_GPIO
> +	tristate "GPIO charger"
> +	depends on GPIOLIB
> +	help
> +	  Say Y to include support for chargers which report their online status
> +	  through a GPIO pin.
> +
> +	  This driver can be build as a module. If so, the module will be
> +	  called gpio-chager.

Shouldn't it say gpio-charger not gpio-chager? A typo.

Regards,

Piotr Hosowicz

-- 
Radzieccy neurolodzy odkryli nerw łączący oko bezpośrednio z dupą.
Kiedy ukłuli pacjenta igłą w dupę, w jego oku pojawiła się łza.
Kiedy wbili tę samą igłę w oko, pacjent się zesrał.
NP: Peter Green Splinter Group - Indians
NB: 2.6.36

Re: [PATCH] POWER: Add gpio chager driver

[Anton Vorontsov]

On Thu, Oct 21, 2010 at 02:25:57PM +0200, Lars-Peter Clausen wrote:
> This patch adds a simple driver for chargers indicating their online status
> through a GPIO pin.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

I agree, the GPIO name fits better. Rhyland, are you OK with that
driver, does it fit your needs?

Some comments down below.

[...]
> +struct gpio_charger {
> +	const struct gpio_charger_platform_data *pdata;
> +	int irq;

This can be unsigned.

> +
> +	struct power_supply charger;
> +};
> +
> +static irqreturn_t gpio_charger_irq(int irq, void *devid)
> +{
> +	struct power_supply *charger = devid;

Please add an empty line here.

> +	power_supply_changed(charger);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
> +{
> +	return container_of(psy, struct gpio_charger, charger);
> +}
> +
> +static int gpio_charger_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)

One more tab for here?

> +{
> +	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
> +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = gpio_get_value(pdata->gpio);
> +		val->intval ^= pdata->gpio_active_low;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property gpio_charger_properties[] = {
> +	POWER_SUPPLY_PROP_ONLINE,
> +};
> +
> +static int __devinit gpio_charger_probe(struct platform_device *pdev)
> +{
> +	const struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;
> +	struct gpio_charger *gpio_charger;
> +	struct power_supply *charger;
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "No platform data");
> +		return -EINVAL;
> +	}
> +
> +	if (!gpio_is_valid(pdata->gpio)) {
> +		dev_err(&pdev->dev, "Invalid gpio pin\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
> +
> +	charger = &gpio_charger->charger;
> +
> +	charger->name = pdata->name;
> +	charger->type = pdata->type;
> +	charger->properties = gpio_charger_properties;
> +	charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
> +	charger->get_property  = gpio_charger_get_property;
> +	charger->supplied_to = pdata->supplied_to;
> +	charger->num_supplicants = pdata->num_supplicants;
> +
> +	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
> +		goto err_free;
> +	}
> +	ret = gpio_direction_input(pdata->gpio);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
> +		goto err_gpio_free;
> +	}
> +
> +	gpio_charger->irq = gpio_to_irq(pdata->gpio);
> +	if (gpio_charger->irq >= 0) {

0 isn't valid IRQ number. The check should be just 'if (gpio_charger->irq)'.

> +		ret = request_irq(gpio_charger->irq, gpio_charger_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), charger);
> +		if (ret) {
> +			dev_warn(&pdev->dev, "Failed to request gpio irq: %d\n", ret);
> +			gpio_charger->irq = -1;
> +		}
> +	}
> +
> +	gpio_charger->pdata = pdata;
> +
> +	ret = power_supply_register(&pdev->dev, charger);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register power supply: %d\n", ret);
> +		goto err_irq_free;
> +	}
> +
> +	platform_set_drvdata(pdev, gpio_charger);
> +
> +	return 0;
> +
> +err_irq_free:
> +	if (gpio_charger->irq >= 0)
> +		free_irq(gpio_charger->irq, charger);
> +err_gpio_free:
> +	gpio_free(pdata->gpio);
> +err_free:
> +	kfree(gpio_charger);
> +	return ret;
> +}
> +
> +static int __devexit gpio_charger_remove(struct platform_device *pdev)
> +{
> +	struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
> +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> +
> +	power_supply_unregister(&gpio_charger->charger);
> +
> +	if (gpio_charger->irq >= 0)
> +		free_irq(gpio_charger->irq, &gpio_charger->charger);
> +	gpio_free(pdata->gpio);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(gpio_charger);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver  gpio_charger_driver = {
> +	.probe = gpio_charger_probe,
> +	.remove = __devexit_p(gpio_charger_remove),
> +	.driver = {
> +		.name = "gpio-charger",
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init gpio_charger_init(void)
> +{
> +	return platform_driver_register(&gpio_charger_driver);
> +}
> +module_init(gpio_charger_init);
> +
> +static void __exit gpio_charger_exit(void)
> +{
> +	platform_driver_unregister(&gpio_charger_driver);
> +}
> +module_exit(gpio_charger_exit);
> +
> +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> +MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:gpio-charger");
> diff --git a/include/linux/power/gpio-charger.h b/include/linux/power/gpio-charger.h
> new file mode 100644
> index 0000000..404d5e6
> --- /dev/null
> +++ b/include/linux/power/gpio-charger.h
> @@ -0,0 +1,39 @@
> +/*
> + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> + *
> + *  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 __LINUX_POWER_GPIO_CHARGER_H__
> +#define __LINUX_POWER_GPIO_CHARGER_H__
> +
> +/*

Should be /**

> + * struct gpio_charger_platform_data - platform_data for gpio_charger devices
> + * @name:		Name for the chargers power_supply device
> + * @type:		Type of the charger
> + * @gpio:		GPIO which is used to indicate the chargers status
> + * @gpio_active_low:	Should be set to 1 if the GPIO is active low otherwise 0
> + * @supplied_to:	Array of battery names to which this chargers supplies power
> + * @num_supplicants:	Number of entries in the supplied_to array
> + */
> +

No need for this empty line.

> +struct gpio_charger_platform_data {
> +	const char *name;
> +	enum power_supply_type type;

#include <linux/power_supply.h> is needed for this.

> +
> +	int gpio;
> +	int gpio_active_low;
> +
> +	char **supplied_to;
> +	size_t num_supplicants;

And #include <linux/types.h> for this.

> +};
> +
> +#endif

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] POWER: Add gpio chager driver

[Lars-Peter Clausen]

Anton Vorontsov wrote:
> On Thu, Oct 21, 2010 at 02:25:57PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a simple driver for chargers indicating their online status
>> through a GPIO pin.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> I agree, the GPIO name fits better. Rhyland, are you OK with that
> driver, does it fit your needs?
> 
> Some comments down below.
> 
> [...]
>> +struct gpio_charger {
>> +	const struct gpio_charger_platform_data *pdata;
>> +	int irq;
> 
> This can be unsigned.

If the gpio has no irq or the irq could not be requested the irq fields value is
negative.

> 
>> +
>> +	struct power_supply charger;
>> +};
>> +
>> +static irqreturn_t gpio_charger_irq(int irq, void *devid)
>> +{
>> +	struct power_supply *charger = devid;
> 
> Please add an empty line here.
> 
>> +	power_supply_changed(charger);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
>> +{
>> +	return container_of(psy, struct gpio_charger, charger);
>> +}
>> +
>> +static int gpio_charger_get_property(struct power_supply *psy,
>> +	enum power_supply_property psp, union power_supply_propval *val)
> 
> One more tab for here?
> 
I prefer one tab, but I can change it.

>> +{
>> +	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
>> +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		val->intval = gpio_get_value(pdata->gpio);
>> +		val->intval ^= pdata->gpio_active_low;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static enum power_supply_property gpio_charger_properties[] = {
>> +	POWER_SUPPLY_PROP_ONLINE,
>> +};
>> +
>> +static int __devinit gpio_charger_probe(struct platform_device *pdev)
>> +{
>> +	const struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;
>> +	struct gpio_charger *gpio_charger;
>> +	struct power_supply *charger;
>> +	int ret;
>> +
>> +	if (!pdata) {
>> +		dev_err(&pdev->dev, "No platform data");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!gpio_is_valid(pdata->gpio)) {
>> +		dev_err(&pdev->dev, "Invalid gpio pin\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
>> +
>> +	charger = &gpio_charger->charger;
>> +
>> +	charger->name = pdata->name;
>> +	charger->type = pdata->type;
>> +	charger->properties = gpio_charger_properties;
>> +	charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
>> +	charger->get_property  = gpio_charger_get_property;
>> +	charger->supplied_to = pdata->supplied_to;
>> +	charger->num_supplicants = pdata->num_supplicants;
>> +
>> +	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
>> +		goto err_free;
>> +	}
>> +	ret = gpio_direction_input(pdata->gpio);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
>> +		goto err_gpio_free;
>> +	}
>> +
>> +	gpio_charger->irq = gpio_to_irq(pdata->gpio);
>> +	if (gpio_charger->irq >= 0) {
> 
> 0 isn't valid IRQ number. The check should be just 'if (gpio_charger->irq)'.

While it is unlikely to be used for an gpio IRQ, as far as I know 0 is a valid IRQ
number.

> 
>> +		ret = request_irq(gpio_charger->irq, gpio_charger_irq,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +				dev_name(&pdev->dev), charger);
>> +		if (ret) {
>> +			dev_warn(&pdev->dev, "Failed to request gpio irq: %d\n", ret);
>> +			gpio_charger->irq = -1;
>> +		}
>> +	}
>> +
>> +	gpio_charger->pdata = pdata;
>> +
>> +	ret = power_supply_register(&pdev->dev, charger);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Failed to register power supply: %d\n", ret);
>> +		goto err_irq_free;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, gpio_charger);
>> +
>> +	return 0;
>> +
>> +err_irq_free:
>> +	if (gpio_charger->irq >= 0)
>> +		free_irq(gpio_charger->irq, charger);
>> +err_gpio_free:
>> +	gpio_free(pdata->gpio);
>> +err_free:
>> +	kfree(gpio_charger);
>> +	return ret;
>> +}
>> +
>> +static int __devexit gpio_charger_remove(struct platform_device *pdev)
>> +{
>> +	struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
>> +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
>> +
>> +	power_supply_unregister(&gpio_charger->charger);
>> +
>> +	if (gpio_charger->irq >= 0)
>> +		free_irq(gpio_charger->irq, &gpio_charger->charger);
>> +	gpio_free(pdata->gpio);
>> +
>> +	platform_set_drvdata(pdev, NULL);
>> +	kfree(gpio_charger);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver  gpio_charger_driver = {
>> +	.probe = gpio_charger_probe,
>> +	.remove = __devexit_p(gpio_charger_remove),
>> +	.driver = {
>> +		.name = "gpio-charger",
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +static int __init gpio_charger_init(void)
>> +{
>> +	return platform_driver_register(&gpio_charger_driver);
>> +}
>> +module_init(gpio_charger_init);
>> +
>> +static void __exit gpio_charger_exit(void)
>> +{
>> +	platform_driver_unregister(&gpio_charger_driver);
>> +}
>> +module_exit(gpio_charger_exit);
>> +
>> +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
>> +MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:gpio-charger");
>> diff --git a/include/linux/power/gpio-charger.h b/include/linux/power/gpio-charger.h
>> new file mode 100644
>> index 0000000..404d5e6
>> --- /dev/null
>> +++ b/include/linux/power/gpio-charger.h
>> @@ -0,0 +1,39 @@
>> +/*
>> + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
>> + *
>> + *  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 __LINUX_POWER_GPIO_CHARGER_H__
>> +#define __LINUX_POWER_GPIO_CHARGER_H__
>> +
>> +/*
> 
> Should be /**
> 
>> + * struct gpio_charger_platform_data - platform_data for gpio_charger devices
>> + * @name:		Name for the chargers power_supply device
>> + * @type:		Type of the charger
>> + * @gpio:		GPIO which is used to indicate the chargers status
>> + * @gpio_active_low:	Should be set to 1 if the GPIO is active low otherwise 0
>> + * @supplied_to:	Array of battery names to which this chargers supplies power
>> + * @num_supplicants:	Number of entries in the supplied_to array
>> + */
>> +
> 
> No need for this empty line.
> 
>> +struct gpio_charger_platform_data {
>> +	const char *name;
>> +	enum power_supply_type type;
> 
> #include <linux/power_supply.h> is needed for this.
> 
>> +
>> +	int gpio;
>> +	int gpio_active_low;
>> +
>> +	char **supplied_to;
>> +	size_t num_supplicants;
> 
> And #include <linux/types.h> for this.
> 
>> +};
>> +
>> +#endif
> 
> Thanks!
> 

Thanks,
- Lars

Re: [PATCH] POWER: Add gpio chager driver

[Lars-Peter Clausen]

Piotr Hosowicz wrote:
> On 21.10.2010 14:25, Lars-Peter Clausen wrote:
>> This patch adds a simple driver for chargers indicating their online
>> status
>> through a GPIO pin.
>>
>> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
>> ---
>>   drivers/power/Kconfig              |   10 ++
>>   drivers/power/Makefile             |    1 +
>>   drivers/power/gpio-charger.c       |  184
>> ++++++++++++++++++++++++++++++++++++
>>   include/linux/power/gpio-charger.h |   39 ++++++++
>>   4 files changed, 234 insertions(+), 0 deletions(-)
>>   create mode 100644 drivers/power/gpio-charger.c
>>   create mode 100644 include/linux/power/gpio-charger.h
>>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 0734356..90b1940 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -166,4 +166,14 @@ config BATTERY_INTEL_MID
>>         Say Y here to enable the battery driver on Intel MID
>>         platforms.
>>
>> +config CHARGER_GPIO
>> +    tristate "GPIO charger"
>> +    depends on GPIOLIB
>> +    help
>> +      Say Y to include support for chargers which report their online
>> status
>> +      through a GPIO pin.
>> +
>> +      This driver can be build as a module. If so, the module will be
>> +      called gpio-chager.
> 
> Shouldn't it say gpio-charger not gpio-chager? A typo.
> 
> Regards,
> 
> Piotr Hosowicz
> 

Hi

Yes, thanks.

- Lars

Re: [PATCH] POWER: Add gpio chager driver

[Anton Vorontsov]

On Thu, Oct 21, 2010 at 04:52:37PM +0200, Lars-Peter Clausen wrote:
[...]
> >> +	gpio_charger->irq = gpio_to_irq(pdata->gpio);
> >> +	if (gpio_charger->irq >= 0) {
> > 
> > 0 isn't valid IRQ number. The check should be just 'if (gpio_charger->irq)'.
> 
> While it is unlikely to be used for an gpio IRQ, as far as I know 0 is a valid IRQ
> number.

0 may be a valid HW IRQ, but not VIRQ.

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg22857.html

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH v2] POWER: Add gpio charger driver

[Lars-Peter Clausen]

This patch adds a simple driver for chargers indicating their online status
through a GPIO pin.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
Changes since v1
* Fix typo in Kconfig
* Fix whitespace issues
* Add missing includes in the header file
* Make the irq field of the driver struct unsigned and assume that 0 is not a
  valid irq.
---
 drivers/power/Kconfig              |   10 ++
 drivers/power/Makefile             |    1 +
 drivers/power/gpio-charger.c       |  185 ++++++++++++++++++++++++++++++++++++
 include/linux/power/gpio-charger.h |   41 ++++++++
 4 files changed, 237 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/gpio-charger.c
 create mode 100644 include/linux/power/gpio-charger.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 0734356..6c771f0 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -166,4 +166,14 @@ config BATTERY_INTEL_MID
 	  Say Y here to enable the battery driver on Intel MID
 	  platforms.
 
+config CHARGER_GPIO
+	tristate "GPIO charger"
+	depends on GPIOLIB
+	help
+	  Say Y to include support for chargers which report their online status
+	  through a GPIO pin.
+
+	  This driver can be build as a module. If so, the module will be
+	  called gpio-charger.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 10143aa..2ca71ac 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_BATTERY_S3C_ADC)	+= s3c_adc_battery.o
 obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
 obj-$(CONFIG_BATTERY_JZ4740)	+= jz4740-battery.o
 obj-$(CONFIG_BATTERY_INTEL_MID)	+= intel_mid_battery.o
+obj-$(CONFIG_CHARGER_GPIO)	+= gpio-charger.o
diff --git a/drivers/power/gpio-charger.c b/drivers/power/gpio-charger.c
new file mode 100644
index 0000000..fccbe99
--- /dev/null
+++ b/drivers/power/gpio-charger.c
@@ -0,0 +1,185 @@
+/*
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *  Driver for chargers which report their online status through a GPIO pin
+ *
+ *  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.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+
+#include <linux/power/gpio-charger.h>
+
+struct gpio_charger {
+	const struct gpio_charger_platform_data *pdata;
+	unsigned int irq;
+
+	struct power_supply charger;
+};
+
+static irqreturn_t gpio_charger_irq(int irq, void *devid)
+{
+	struct power_supply *charger = devid;
+
+	power_supply_changed(charger);
+
+	return IRQ_HANDLED;
+}
+
+static inline struct gpio_charger *psy_to_gpio_charger(struct power_supply *psy)
+{
+	return container_of(psy, struct gpio_charger, charger);
+}
+
+static int gpio_charger_get_property(struct power_supply *psy,
+		enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
+	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = gpio_get_value(pdata->gpio);
+		val->intval ^= pdata->gpio_active_low;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property gpio_charger_properties[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int __devinit gpio_charger_probe(struct platform_device *pdev)
+{
+	const struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_charger *gpio_charger;
+	struct power_supply *charger;
+	int ret;
+	int irq;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data\n");
+		return -EINVAL;
+	}
+
+	if (!gpio_is_valid(pdata->gpio)) {
+		dev_err(&pdev->dev, "Invalid gpio pin\n");
+		return -EINVAL;
+	}
+
+	gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
+
+	charger = &gpio_charger->charger;
+
+	charger->name = pdata->name;
+	charger->type = pdata->type;
+	charger->properties = gpio_charger_properties;
+	charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
+	charger->get_property  = gpio_charger_get_property;
+	charger->supplied_to = pdata->supplied_to;
+	charger->num_supplicants = pdata->num_supplicants;
+
+	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
+		goto err_free;
+	}
+	ret = gpio_direction_input(pdata->gpio);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
+		goto err_gpio_free;
+	}
+
+	irq = gpio_to_irq(pdata->gpio);
+	if (irq > 0) {
+		ret = request_irq(irq, gpio_charger_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				dev_name(&pdev->dev), charger);
+		if (ret)
+			dev_warn(&pdev->dev, "Failed to request irq: %d\n", ret);
+		else
+			gpio_charger->irq = irq;
+	}
+
+	gpio_charger->pdata = pdata;
+
+	ret = power_supply_register(&pdev->dev, charger);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register power supply: %d\n", ret);
+		goto err_irq_free;
+	}
+
+	platform_set_drvdata(pdev, gpio_charger);
+
+	return 0;
+
+err_irq_free:
+	if (gpio_charger->irq)
+		free_irq(gpio_charger->irq, charger);
+err_gpio_free:
+	gpio_free(pdata->gpio);
+err_free:
+	kfree(gpio_charger);
+	return ret;
+}
+
+static int __devexit gpio_charger_remove(struct platform_device *pdev)
+{
+	struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
+
+	power_supply_unregister(&gpio_charger->charger);
+
+	if (gpio_charger->irq)
+		free_irq(gpio_charger->irq, &gpio_charger->charger);
+	gpio_free(gpio_charger->pdata->gpio);
+
+	platform_set_drvdata(pdev, NULL);
+	kfree(gpio_charger);
+
+	return 0;
+}
+
+static struct platform_driver gpio_charger_driver = {
+	.probe = gpio_charger_probe,
+	.remove = __devexit_p(gpio_charger_remove),
+	.driver = {
+		.name = "gpio-charger",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init gpio_charger_init(void)
+{
+	return platform_driver_register(&gpio_charger_driver);
+}
+module_init(gpio_charger_init);
+
+static void __exit gpio_charger_exit(void)
+{
+	platform_driver_unregister(&gpio_charger_driver);
+}
+module_exit(gpio_charger_exit);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
+MODULE_DESCRIPTION("Driver for chargers which report their online status through a GPIO");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-charger");
diff --git a/include/linux/power/gpio-charger.h b/include/linux/power/gpio-charger.h
new file mode 100644
index 0000000..de1dfe0
--- /dev/null
+++ b/include/linux/power/gpio-charger.h
@@ -0,0 +1,41 @@
+/*
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *
+ *  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 __LINUX_POWER_GPIO_CHARGER_H__
+#define __LINUX_POWER_GPIO_CHARGER_H__
+
+#include <linux/power_supply.h>
+#include <linux/types.h>
+
+/**
+ * struct gpio_charger_platform_data - platform_data for gpio_charger devices
+ * @name:		Name for the chargers power_supply device
+ * @type:		Type of the charger
+ * @gpio:		GPIO which is used to indicate the chargers status
+ * @gpio_active_low:	Should be set to 1 if the GPIO is active low otherwise 0
+ * @supplied_to:	Array of battery names to which this chargers supplies power
+ * @num_supplicants:	Number of entries in the supplied_to array
+ */
+struct gpio_charger_platform_data {
+	const char *name;
+	enum power_supply_type type;
+
+	int gpio;
+	int gpio_active_low;
+
+	char **supplied_to;
+	size_t num_supplicants;
+};
+
+#endif
-- 
1.5.6.5

Re: [PATCH v2] POWER: Add gpio charger driver

[Mark Brown]

On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:

> +	irq = gpio_to_irq(pdata->gpio);
> +	if (irq > 0) {
> +		ret = request_irq(irq, gpio_charger_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), charger);
> +		if (ret)
> +			dev_warn(&pdev->dev, "Failed to request irq: %d\n", ret);

It would be good to handle IRQs that can sleep (like those on I2C GPIO
expanders) here - either always use a threaded IRQ handler (probably
won't hurt here) or use one if the IRQ can sleep.

Re: [PATCH v2] POWER: Add gpio charger driver

[Lars-Peter Clausen]

Mark Brown wrote:
> On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
> 
>> +	irq = gpio_to_irq(pdata->gpio);
>> +	if (irq > 0) {
>> +		ret = request_irq(irq, gpio_charger_irq,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +				dev_name(&pdev->dev), charger);
>> +		if (ret)
>> +			dev_warn(&pdev->dev, "Failed to request irq: %d\n", ret);
> 
> It would be good to handle IRQs that can sleep (like those on I2C GPIO
> expanders) here - either always use a threaded IRQ handler (probably
> won't hurt here) or use one if the IRQ can sleep.

I guess the best would be to use `request_any_context_irq`, right?

- Lars

Re: [PATCH v2] POWER: Add gpio charger driver

[Mark Brown]

On Thu, Oct 21, 2010 at 06:16:32PM +0200, Lars-Peter Clausen wrote:
> Mark Brown wrote:
> > It would be good to handle IRQs that can sleep (like those on I2C GPIO
> > expanders) here - either always use a threaded IRQ handler (probably
> > won't hurt here) or use one if the IRQ can sleep.

> I guess the best would be to use `request_any_context_irq`, right?

Ah, that got merged now - yes, that'd be best.

Re: [PATCH v2] POWER: Add gpio charger driver

[Anton Vorontsov]

On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
> This patch adds a simple driver for chargers indicating their online status
> through a GPIO pin.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Hm. I'm getting older, it seems. Now can anybody remind me why
pda_power.c doesn't work here? ;-)

This driver looks like a light-weigh version of pda_power, except
that it can do GPIOs directly (instead of is_*_online() callbacks).

Can we instead improve pda_power? Like this:

	if (!pdata->is_ac_online && pdata->ac_monitor_gpio)
		pdata->is_ac_online = pda_ac_monitor_gpio;

Should look quite cool, I think.

(Plus, we might get rid of ac/usb stuff in that driver, and 
pass enum power_supply_type to the callbacks instead).

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

RE: [PATCH] POWER: Add gpio chager driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7836 bytes --]

> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Thursday, October 21, 2010 7:14 AM
> To: Lars-Peter Clausen
> Cc: Rhyland Klein; broonie@opensource.wolfsonmicro.com; Andrew Chew;
> olof@lixom.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] POWER: Add gpio chager driver
> 
> On Thu, Oct 21, 2010 at 02:25:57PM +0200, Lars-Peter Clausen wrote:
> > This patch adds a simple driver for chargers indicating their online
> status
> > through a GPIO pin.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> I agree, the GPIO name fits better. Rhyland, are you OK with that
> driver, does it fit your needs?

This would be great. I will give it a test but from reading over it, it sounds perfect for my needs.

> 
> Some comments down below.
> 
> [...]
> > +struct gpio_charger {
> > +	const struct gpio_charger_platform_data *pdata;
> > +	int irq;
> 
> This can be unsigned.
> 
> > +
> > +	struct power_supply charger;
> > +};
> > +
> > +static irqreturn_t gpio_charger_irq(int irq, void *devid)
> > +{
> > +	struct power_supply *charger = devid;
> 
> Please add an empty line here.
> 
> > +	power_supply_changed(charger);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static inline struct gpio_charger *psy_to_gpio_charger(struct
> power_supply *psy)
> > +{
> > +	return container_of(psy, struct gpio_charger, charger);
> > +}
> > +
> > +static int gpio_charger_get_property(struct power_supply *psy,
> > +	enum power_supply_property psp, union power_supply_propval *val)
> 
> One more tab for here?
> 
> > +{
> > +	struct gpio_charger *gpio_charger = psy_to_gpio_charger(psy);
> > +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		val->intval = gpio_get_value(pdata->gpio);
> > +		val->intval ^= pdata->gpio_active_low;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static enum power_supply_property gpio_charger_properties[] = {
> > +	POWER_SUPPLY_PROP_ONLINE,
> > +};
> > +
> > +static int __devinit gpio_charger_probe(struct platform_device *pdev)
> > +{
> > +	const struct gpio_charger_platform_data *pdata = pdev-
> >dev.platform_data;
> > +	struct gpio_charger *gpio_charger;
> > +	struct power_supply *charger;
> > +	int ret;
> > +
> > +	if (!pdata) {
> > +		dev_err(&pdev->dev, "No platform data");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!gpio_is_valid(pdata->gpio)) {
> > +		dev_err(&pdev->dev, "Invalid gpio pin\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	gpio_charger = kzalloc(sizeof(*gpio_charger), GFP_KERNEL);
> > +
> > +	charger = &gpio_charger->charger;
> > +
> > +	charger->name = pdata->name;
> > +	charger->type = pdata->type;
> > +	charger->properties = gpio_charger_properties;
> > +	charger->num_properties = ARRAY_SIZE(gpio_charger_properties);
> > +	charger->get_property  = gpio_charger_get_property;
> > +	charger->supplied_to = pdata->supplied_to;
> > +	charger->num_supplicants = pdata->num_supplicants;
> > +
> > +	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
> > +		goto err_free;
> > +	}
> > +	ret = gpio_direction_input(pdata->gpio);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
> > +		goto err_gpio_free;
> > +	}
> > +
> > +	gpio_charger->irq = gpio_to_irq(pdata->gpio);
> > +	if (gpio_charger->irq >= 0) {
> 
> 0 isn't valid IRQ number. The check should be just 'if (gpio_charger-
> >irq)'.
> 
> > +		ret = request_irq(gpio_charger->irq, gpio_charger_irq,
> > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +				dev_name(&pdev->dev), charger);
> > +		if (ret) {
> > +			dev_warn(&pdev->dev, "Failed to request gpio irq: %d\n",
> ret);
> > +			gpio_charger->irq = -1;
> > +		}
> > +	}
> > +
> > +	gpio_charger->pdata = pdata;
> > +
> > +	ret = power_supply_register(&pdev->dev, charger);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register power supply: %d\n",
> ret);
> > +		goto err_irq_free;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, gpio_charger);
> > +
> > +	return 0;
> > +
> > +err_irq_free:
> > +	if (gpio_charger->irq >= 0)
> > +		free_irq(gpio_charger->irq, charger);
> > +err_gpio_free:
> > +	gpio_free(pdata->gpio);
> > +err_free:
> > +	kfree(gpio_charger);
> > +	return ret;
> > +}
> > +
> > +static int __devexit gpio_charger_remove(struct platform_device *pdev)
> > +{
> > +	struct gpio_charger *gpio_charger = platform_get_drvdata(pdev);
> > +	const struct gpio_charger_platform_data *pdata = gpio_charger->pdata;
> > +
> > +	power_supply_unregister(&gpio_charger->charger);
> > +
> > +	if (gpio_charger->irq >= 0)
> > +		free_irq(gpio_charger->irq, &gpio_charger->charger);
> > +	gpio_free(pdata->gpio);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +	kfree(gpio_charger);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver  gpio_charger_driver = {
> > +	.probe = gpio_charger_probe,
> > +	.remove = __devexit_p(gpio_charger_remove),
> > +	.driver = {
> > +		.name = "gpio-charger",
> > +		.owner = THIS_MODULE,
> > +	},
> > +};
> > +
> > +static int __init gpio_charger_init(void)
> > +{
> > +	return platform_driver_register(&gpio_charger_driver);
> > +}
> > +module_init(gpio_charger_init);
> > +
> > +static void __exit gpio_charger_exit(void)
> > +{
> > +	platform_driver_unregister(&gpio_charger_driver);
> > +}
> > +module_exit(gpio_charger_exit);
> > +
> > +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> > +MODULE_DESCRIPTION("Driver for chargers which report their online status
> through a GPIO");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:gpio-charger");
> > diff --git a/include/linux/power/gpio-charger.h
> b/include/linux/power/gpio-charger.h
> > new file mode 100644
> > index 0000000..404d5e6
> > --- /dev/null
> > +++ b/include/linux/power/gpio-charger.h
> > @@ -0,0 +1,39 @@
> > +/*
> > + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> > + *
> > + *  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 __LINUX_POWER_GPIO_CHARGER_H__
> > +#define __LINUX_POWER_GPIO_CHARGER_H__
> > +
> > +/*
> 
> Should be /**
> 
> > + * struct gpio_charger_platform_data - platform_data for gpio_charger
> devices
> > + * @name:		Name for the chargers power_supply device
> > + * @type:		Type of the charger
> > + * @gpio:		GPIO which is used to indicate the chargers status
> > + * @gpio_active_low:	Should be set to 1 if the GPIO is active low
> otherwise 0
> > + * @supplied_to:	Array of battery names to which this chargers
> supplies power
> > + * @num_supplicants:	Number of entries in the supplied_to array
> > + */
> > +
> 
> No need for this empty line.
> 
> > +struct gpio_charger_platform_data {
> > +	const char *name;
> > +	enum power_supply_type type;
> 
> #include <linux/power_supply.h> is needed for this.
> 
> > +
> > +	int gpio;
> > +	int gpio_active_low;
> > +
> > +	char **supplied_to;
> > +	size_t num_supplicants;
> 
> And #include <linux/types.h> for this.
> 
> > +};
> > +
> > +#endif
> 
> Thanks!
> 
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] POWER: Add gpio charger driver

[Lars-Peter Clausen]

Anton Vorontsov wrote:
> On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a simple driver for chargers indicating their online status
>> through a GPIO pin.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> Hm. I'm getting older, it seems. Now can anybody remind me why
> pda_power.c doesn't work here? ;-)
> 
> This driver looks like a light-weigh version of pda_power, except
> that it can do GPIOs directly (instead of is_*_online() callbacks).
> 
> Can we instead improve pda_power? Like this:
> 
> 	if (!pdata->is_ac_online && pdata->ac_monitor_gpio)
> 		pdata->is_ac_online = pda_ac_monitor_gpio;
> 
> Should look quite cool, I think.
> 
> (Plus, we might get rid of ac/usb stuff in that driver, and 
> pass enum power_supply_type to the callbacks instead).
> 

Hi

Hm... I guess it can be, but on the other hand most platform bus chargers type
devices can be, because the pda_power driver is keep pretty generic with custom init,
status and exit callbacks.

- Lars

RE: [PATCH v2] POWER: Add gpio charger driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1776 bytes --]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Thursday, October 21, 2010 10:48 AM
> To: Anton Vorontsov
> Cc: Rhyland Klein; broonie@opensource.wolfsonmicro.com; Andrew Chew;
> olof@lixom.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> 
> Anton Vorontsov wrote:
> > On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
> >> This patch adds a simple driver for chargers indicating their online
> status
> >> through a GPIO pin.
> >>
> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Hm. I'm getting older, it seems. Now can anybody remind me why
> > pda_power.c doesn't work here? ;-)
> >
> > This driver looks like a light-weigh version of pda_power, except
> > that it can do GPIOs directly (instead of is_*_online() callbacks).
> >
> > Can we instead improve pda_power? Like this:
> >
> > 	if (!pdata->is_ac_online && pdata->ac_monitor_gpio)
> > 		pdata->is_ac_online = pda_ac_monitor_gpio;
> >
> > Should look quite cool, I think.
> >
> > (Plus, we might get rid of ac/usb stuff in that driver, and
> > pass enum power_supply_type to the callbacks instead).
> >
> 
> Hi
> 
> Hm... I guess it can be, but on the other hand most platform bus chargers
> type
> devices can be, because the pda_power driver is keep pretty generic with
> custom init,
> status and exit callbacks.
> 
> - Lars


Not sure what the state of this driver is, but I figured I would let you know that I tested
The driver on my platform and it does indeed work in place of my driver. So I am happy with this one :)

-rhyland
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] POWER: Add gpio charger driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1740 bytes --]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Thursday, October 21, 2010 10:48 AM
> To: Anton Vorontsov
> Cc: Rhyland Klein; broonie@opensource.wolfsonmicro.com; Andrew Chew;
> olof@lixom.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> 
> Anton Vorontsov wrote:
> > On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
> >> This patch adds a simple driver for chargers indicating their online
> status
> >> through a GPIO pin.
> >>
> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> >
> > Hm. I'm getting older, it seems. Now can anybody remind me why
> > pda_power.c doesn't work here? ;-)
> >
> > This driver looks like a light-weigh version of pda_power, except
> > that it can do GPIOs directly (instead of is_*_online() callbacks).
> >
> > Can we instead improve pda_power? Like this:
> >
> > 	if (!pdata->is_ac_online && pdata->ac_monitor_gpio)
> > 		pdata->is_ac_online = pda_ac_monitor_gpio;
> >
> > Should look quite cool, I think.
> >
> > (Plus, we might get rid of ac/usb stuff in that driver, and
> > pass enum power_supply_type to the callbacks instead).
> >
> 
> Hi
> 
> Hm... I guess it can be, but on the other hand most platform bus chargers
> type
> devices can be, because the pda_power driver is keep pretty generic with
> custom init,
> status and exit callbacks.
> 
> - Lars

I didn't see any more discussion on this. Is the plan to integrate the gpio-charger driver as is or to instead try to integrate support for this into pda_power?

-rhyland
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] POWER: Add gpio charger driver

[Anton Vorontsov]

On Thu, Oct 28, 2010 at 02:17:41PM -0700, Rhyland Klein wrote:
[...]
> > Hm... I guess it can be, but on the other hand most platform bus chargers
> > type
> > devices can be, because the pda_power driver is keep pretty generic with
> > custom init,
> > status and exit callbacks.
> > 
> > - Lars
> 
> I didn't see any more discussion on this. Is the plan to integrate
> the gpio-charger driver as is or to instead try to integrate support
> for this into pda_power?

Sorry for the delayed response, and thanks for the pings! ;-)

The main thing I'm afraid of is duplication. I.e. someday you
will want debouncing (include/linux/pda_power.h's wait_for_status,
wait_for_charger parameters) support, regulators support etc.

And your gpio driver will look very similar to pda_power.

So I'd vote for adding the GPIO functionality to pda_power, and
refactoring it if needed.

Though, if there are strong objections against this idea, I
can merge the GPIO driver, and let's see how things will evolve.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

RE: [PATCH v2] POWER: Add gpio charger driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1742 bytes --]

> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Thursday, November 04, 2010 10:48 AM
> To: Rhyland Klein
> Cc: Lars-Peter Clausen; broonie@opensource.wolfsonmicro.com; Andrew Chew;
> olof@lixom.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> 
> On Thu, Oct 28, 2010 at 02:17:41PM -0700, Rhyland Klein wrote:
> [...]
> > > Hm... I guess it can be, but on the other hand most platform bus
> chargers
> > > type
> > > devices can be, because the pda_power driver is keep pretty generic
> with
> > > custom init,
> > > status and exit callbacks.
> > >
> > > - Lars
> >
> > I didn't see any more discussion on this. Is the plan to integrate
> > the gpio-charger driver as is or to instead try to integrate support
> > for this into pda_power?
> 
> Sorry for the delayed response, and thanks for the pings! ;-)
> 
> The main thing I'm afraid of is duplication. I.e. someday you
> will want debouncing (include/linux/pda_power.h's wait_for_status,
> wait_for_charger parameters) support, regulators support etc.
> 
> And your gpio driver will look very similar to pda_power.
> 
> So I'd vote for adding the GPIO functionality to pda_power, and
> refactoring it if needed.
> 
> Though, if there are strong objections against this idea, I
> can merge the GPIO driver, and let's see how things will evolve.
> 
> Thanks!
> 
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2

My guess, is that since no one has responded, no one is working on integrating the functionality into pda-power?

-rhyland
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] POWER: Add gpio charger driver

[Lars-Peter Clausen]

Rhyland Klein wrote:
>> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
>> Sent: Thursday, November 04, 2010 10:48 AM
>> To: Rhyland Klein
>> Cc: Lars-Peter Clausen; broonie@opensource.wolfsonmicro.com; Andrew Chew;
>> olof@lixom.net; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
>>
>> On Thu, Oct 28, 2010 at 02:17:41PM -0700, Rhyland Klein wrote:
>> [...]
>>>> Hm... I guess it can be, but on the other hand most platform bus
>> chargers
>>>> type
>>>> devices can be, because the pda_power driver is keep pretty generic
>> with
>>>> custom init,
>>>> status and exit callbacks.
>>>>
>>>> - Lars
>>> I didn't see any more discussion on this. Is the plan to integrate
>>> the gpio-charger driver as is or to instead try to integrate support
>>> for this into pda_power?
>> Sorry for the delayed response, and thanks for the pings! ;-)
>>
>> The main thing I'm afraid of is duplication. I.e. someday you
>> will want debouncing (include/linux/pda_power.h's wait_for_status,
>> wait_for_charger parameters) support, regulators support etc.
>>
>> And your gpio driver will look very similar to pda_power.
>>
>> So I'd vote for adding the GPIO functionality to pda_power, and
>> refactoring it if needed.
>>
>> Though, if there are strong objections against this idea, I
>> can merge the GPIO driver, and let's see how things will evolve.
>>
>> Thanks!
>>
>> --
>> Anton Vorontsov
>> email: cbouatmailru@gmail.com
>> irc://irc.freenode.net/bd2
> 
> My guess, is that since no one has responded, no one is working on integrating the functionality into pda-power?
> 
> -rhyland

hi

Well I'm still not convinced that both drivers should be merged, yet I'm not totally
opposed to it. In my opinion we are in some kind of dilemma here.
I can see Antons point regarding to introducing code duplication, but on the other
hand you'll always have code duplication amongst similar drivers. This will
especially stand out if the setup functions take up a relatively large part of the
whole code size.
One way to avoid this is by putting everything into one driver which handles all
different cases. But the next time yet another sort of similar driver comes along
another if-else-branch is added to the pda-power driver. And slowly over time things
will get messy.
Another option to solve the problem is to add another level of indirection. For
example in form of a simple_charger driver which will take callbacks for add, remove
and status. The gpio-charger and pda-power could then instantiate such a driver and
provide their callbacks. But by adding more and more levels of indirection things
will slowly get messy as well.
One solution that might could work is to provide library functions which aim at
providing aid for implementing (simple) charger drivers.

- Lars

RE: [PATCH v2] POWER: Add gpio charger driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3887 bytes --]

> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Wednesday, November 10, 2010 5:34 PM
> To: Rhyland Klein
> Cc: Anton Vorontsov; broonie@opensource.wolfsonmicro.com; Andrew Chew;
> olof@lixom.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> 
> Rhyland Klein wrote:
> >> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> >> Sent: Thursday, November 04, 2010 10:48 AM
> >> To: Rhyland Klein
> >> Cc: Lars-Peter Clausen; broonie@opensource.wolfsonmicro.com; Andrew
> Chew;
> >> olof@lixom.net; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> >>
> >> On Thu, Oct 28, 2010 at 02:17:41PM -0700, Rhyland Klein wrote:
> >> [...]
> >>>> Hm... I guess it can be, but on the other hand most platform bus
> >> chargers
> >>>> type
> >>>> devices can be, because the pda_power driver is keep pretty generic
> >> with
> >>>> custom init,
> >>>> status and exit callbacks.
> >>>>
> >>>> - Lars
> >>> I didn't see any more discussion on this. Is the plan to integrate
> >>> the gpio-charger driver as is or to instead try to integrate support
> >>> for this into pda_power?
> >> Sorry for the delayed response, and thanks for the pings! ;-)
> >>
> >> The main thing I'm afraid of is duplication. I.e. someday you
> >> will want debouncing (include/linux/pda_power.h's wait_for_status,
> >> wait_for_charger parameters) support, regulators support etc.
> >>
> >> And your gpio driver will look very similar to pda_power.
> >>
> >> So I'd vote for adding the GPIO functionality to pda_power, and
> >> refactoring it if needed.
> >>
> >> Though, if there are strong objections against this idea, I
> >> can merge the GPIO driver, and let's see how things will evolve.
> >>
> >> Thanks!
> >>
> >> --
> >> Anton Vorontsov
> >> email: cbouatmailru@gmail.com
> >> irc://irc.freenode.net/bd2
> >
> > My guess, is that since no one has responded, no one is working on
> integrating the functionality into pda-power?
> >
> > -rhyland
> 
> hi
> 
> Well I'm still not convinced that both drivers should be merged, yet I'm
> not totally
> opposed to it. In my opinion we are in some kind of dilemma here.
> I can see Antons point regarding to introducing code duplication, but on
> the other
> hand you'll always have code duplication amongst similar drivers. This will
> especially stand out if the setup functions take up a relatively large part
> of the
> whole code size.
> One way to avoid this is by putting everything into one driver which
> handles all
> different cases. But the next time yet another sort of similar driver comes
> along
> another if-else-branch is added to the pda-power driver. And slowly over
> time things
> will get messy.
> Another option to solve the problem is to add another level of indirection.
> For
> example in form of a simple_charger driver which will take callbacks for
> add, remove
> and status. The gpio-charger and pda-power could then instantiate such a
> driver and
> provide their callbacks. But by adding more and more levels of indirection
> things
> will slowly get messy as well.
> One solution that might could work is to provide library functions which
> aim at
> providing aid for implementing (simple) charger drivers.
> 
> - Lars

Well, this is how I look at it. For now, we can integrate the generic gpio-charger driver. Then there are 2 concerns. If someone wants a gpio-charger with more functionality that duplicates what pda-power does, then maybe the correct thing to do is to make a new driver instead of trying to extend gpio-charger. Also, if we put in the gpio-charger now, we can still work to integrate that functionality into pda-power, and then deprecate gpio-charger.

-rhyland
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] POWER: Add gpio charger driver

[Rhyland Klein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3251 bytes --]

> Rhyland Klein wrote:
> >> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> >> Sent: Thursday, November 04, 2010 10:48 AM
> >> To: Rhyland Klein
> >> Cc: Lars-Peter Clausen; broonie@opensource.wolfsonmicro.com; Andrew
> Chew;
> >> olof@lixom.net; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v2] POWER: Add gpio charger driver
> >>
> >> On Thu, Oct 28, 2010 at 02:17:41PM -0700, Rhyland Klein wrote:
> >> [...]
> >>>> Hm... I guess it can be, but on the other hand most platform bus
> >> chargers
> >>>> type
> >>>> devices can be, because the pda_power driver is keep pretty generic
> >> with
> >>>> custom init,
> >>>> status and exit callbacks.
> >>>>
> >>>> - Lars
> >>> I didn't see any more discussion on this. Is the plan to integrate
> >>> the gpio-charger driver as is or to instead try to integrate support
> >>> for this into pda_power?
> >> Sorry for the delayed response, and thanks for the pings! ;-)
> >>
> >> The main thing I'm afraid of is duplication. I.e. someday you
> >> will want debouncing (include/linux/pda_power.h's wait_for_status,
> >> wait_for_charger parameters) support, regulators support etc.
> >>
> >> And your gpio driver will look very similar to pda_power.
> >>
> >> So I'd vote for adding the GPIO functionality to pda_power, and
> >> refactoring it if needed.
> >>
> >> Though, if there are strong objections against this idea, I
> >> can merge the GPIO driver, and let's see how things will evolve.
> >>
> >> Thanks!
> >>
> >> --
> >> Anton Vorontsov
> >> email: cbouatmailru@gmail.com
> >> irc://irc.freenode.net/bd2
> >
> > My guess, is that since no one has responded, no one is working on
> integrating the functionality into pda-power?
> >
> > -rhyland
> 
> hi
> 
> Well I'm still not convinced that both drivers should be merged, yet I'm
> not totally
> opposed to it. In my opinion we are in some kind of dilemma here.
> I can see Antons point regarding to introducing code duplication, but on
> the other
> hand you'll always have code duplication amongst similar drivers. This will
> especially stand out if the setup functions take up a relatively large part
> of the
> whole code size.
> One way to avoid this is by putting everything into one driver which
> handles all
> different cases. But the next time yet another sort of similar driver comes
> along
> another if-else-branch is added to the pda-power driver. And slowly over
> time things
> will get messy.
> Another option to solve the problem is to add another level of indirection.
> For
> example in form of a simple_charger driver which will take callbacks for
> add, remove
> and status. The gpio-charger and pda-power could then instantiate such a
> driver and
> provide their callbacks. But by adding more and more levels of indirection
> things
> will slowly get messy as well.
> One solution that might could work is to provide library functions which
> aim at
> providing aid for implementing (simple) charger drivers.
> 
> - Lars
Anton, for the time being, can we integrate this driver? That way the functionality is there and can be used now?

-rhyland
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] POWER: Add gpio charger driver

[Anton Vorontsov]

On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
> This patch adds a simple driver for chargers indicating their online status
> through a GPIO pin.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> ---
> Changes since v1
> * Fix typo in Kconfig
> * Fix whitespace issues
> * Add missing includes in the header file
> * Make the irq field of the driver struct unsigned and assume that 0 is not a
>   valid irq.

Applied, thanks!

But please, look at Mark Brown comments. If they are still
applicable, the follow up patch would be great.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v2] POWER: Add gpio charger driver

[Lars-Peter Clausen]

Anton Vorontsov wrote:
> On Thu, Oct 21, 2010 at 05:55:01PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a simple driver for chargers indicating their online status
>> through a GPIO pin.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>
>> ---
>> Changes since v1
>> * Fix typo in Kconfig
>> * Fix whitespace issues
>> * Add missing includes in the header file
>> * Make the irq field of the driver struct unsigned and assume that 0 is not a
>>   valid irq.
> 
> Applied, thanks!
> 
> But please, look at Mark Brown comments. If they are still
> applicable, the follow up patch would be great.
> 
> Thanks!
> 

Hi

I'll send a follow up patch later today. I think I had one or two other issues on my
list that need to be addressed.
I wanted to wait with v3 until it was clear whether the driver had a chance to be
merged at all.
Do you prefer a v3 of the original patch or an incremental one?

- Lars

Re: [PATCH v2] POWER: Add gpio charger driver

[Anton Vorontsov]

On Thu, Nov 18, 2010 at 03:30:09PM +0100, Lars-Peter Clausen wrote:
[..]
> > But please, look at Mark Brown comments. If they are still
> > applicable, the follow up patch would be great.
> 
> Hi
> 
> I'll send a follow up patch later today. I think I had one or two other issues on my
> list that need to be addressed.
> I wanted to wait with v3 until it was clear whether the driver had a chance to be
> merged at all.
> Do you prefer a v3 of the original patch or an incremental one?

Incremental would be fine, thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2