[PATCH 0/4] Add support for tps65090-charger

[PATCH 0/4] Add support for tps65090-charger

[Rhyland Klein]

This patchset adds support for the TPS65090-charger. This charger is
registered as a subdevice of the TPS65090 PMIC.

This series includes a fix for the tps65090 header file where all
the interrupts were shifted by 1.

Rhyland Klein (4):
  mfd: tps65090: Fix enum in header file
  mfd: tps65090: Add resources for charger
  power_supply: tps65090-charger: Add binding doc
  power: tps65090: Add support for tps65090-charger

 .../devicetree/bindings/power_supply/tps65090.txt  |   23 ++
 drivers/mfd/tps65090.c                             |   10 +
 drivers/power/Kconfig                              |    7 +
 drivers/power/Makefile                             |    1 +
 drivers/power/tps65090-charger.c                   |  310 ++++++++++++++++++++
 include/linux/mfd/tps65090.h                       |    6 +
 6 files changed, 357 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt
 create mode 100644 drivers/power/tps65090-charger.c

-- 
1.7.9.5

[PATCH 1/4] mfd: tps65090: Fix enum in header file

[Rhyland Klein]

The enum is missing the definition for the first bit, which makes all
the rest off by one. Add definition for the TPS65090_IRQ_INTERRUPT bit
which at 0.

Signed-off-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 include/linux/mfd/tps65090.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 6694cf4..9ce231f 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -27,6 +27,7 @@
 
 /* TPS65090 IRQs */
 enum {
+	TPS65090_IRQ_INTERRUPT,
 	TPS65090_IRQ_VAC_STATUS_CHANGE,
 	TPS65090_IRQ_VSYS_STATUS_CHANGE,
 	TPS65090_IRQ_BAT_STATUS_CHANGE,
-- 
1.7.9.5

[PATCH 2/4] mfd: tps65090: Add resources for charger

[Rhyland Klein]

Add irq resources to pass to the charger mfd sub dev so
the charger can listen for interrupts.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/mfd/tps65090.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index 98edb5be..88846ae 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -56,12 +56,22 @@
 #define TPS65090_INT2_MASK_OVERLOAD_FET6		6
 #define TPS65090_INT2_MASK_OVERLOAD_FET7		7
 
+static struct resource charger_resources[] = {
+	{
+		.start  = TPS65090_IRQ_VAC_STATUS_CHANGE,
+		.end    = TPS65090_IRQ_VAC_STATUS_CHANGE,
+		.flags  = IORESOURCE_IRQ,
+	}
+};
+
 static struct mfd_cell tps65090s[] = {
 	{
 		.name = "tps65090-pmic",
 	},
 	{
 		.name = "tps65090-charger",
+		.num_resources = ARRAY_SIZE(charger_resources),
+		.resources = &charger_resources[0],
 	},
 };
 
-- 
1.7.9.5

[PATCH 3/4] power_supply: tps65090-charger: Add binding doc

[Rhyland Klein]

This change adds the binding documentation for the tps65090-charger.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 .../devicetree/bindings/power_supply/tps65090.txt  |   23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/tps65090.txt

diff --git a/Documentation/devicetree/bindings/power_supply/tps65090.txt b/Documentation/devicetree/bindings/power_supply/tps65090.txt
new file mode 100644
index 0000000..56370c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/tps65090.txt
@@ -0,0 +1,23 @@
+TPS65090 Frontend PMU with Switchmode Charger
+
+Required Properties:
+-compatible: "ti,tps65090"
+-reg: I2C slave address
+-interrupts: the interrupt output to which this device connects
+
+Optional Properties:
+-ti,enable-low-current-chrg: Enables charging when a low current is detected
+ while the default logic is to stop charging.
+
+Example:
+
+	tps65090@48 {
+		compatible = "ti,tps65090";
+		reg = <0x48>;
+		interrupts = <0 88 0x4>;
+
+		ti,enable-low-current-chrg;
+
+		regulators {
+			...
+		};
-- 
1.7.9.5

[PATCH 4/4] power: tps65090: Add support for tps65090-charger

[Rhyland Klein]

This patch adds support for the tps65090 charger driver.

Based on work by:
Syed Rafiuddin <srafiuddin@nvidia.com>
Laxman Dewangan <ldewangan@nvidia.com>

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/power/Kconfig            |    7 +
 drivers/power/Makefile           |    1 +
 drivers/power/tps65090-charger.c |  310 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps65090.h     |    5 +
 4 files changed, 323 insertions(+)
 create mode 100644 drivers/power/tps65090-charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 9f45e2f..6dcc3bd 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -340,6 +340,13 @@ config CHARGER_SMB347
 	  Say Y to include support for Summit Microelectronics SMB347
 	  Battery Charger.
 
+config CHARGER_TPS65090
+	tristate "TPS65090 battery charger driver"
+	depends on MFD_TPS65090
+	help
+	 Say Y here to enable support for battery charging with TPS65090
+	 PMIC chips.
+
 config AB8500_BM
 	bool "AB8500 Battery Management Driver"
 	depends on AB8500_CORE && AB8500_GPADC
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 22c8913..9523f81 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -51,4 +51,5 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
 obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
 obj-$(CONFIG_POWER_AVS)		+= avs/
 obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
+obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_POWER_RESET)	+= reset/
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
new file mode 100644
index 0000000..1234b81
--- /dev/null
+++ b/drivers/power/tps65090-charger.c
@@ -0,0 +1,310 @@
+/*
+ * Battery charger driver for TI's tps65090
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/mfd/tps65090.h>
+
+#define TPS65090_REG_INTR_STS	0x00
+#define TPS65090_REG_CG_CTRL0	0x04
+#define TPS65090_REG_CG_CTRL1	0x05
+#define TPS65090_REG_CG_CTRL2	0x06
+#define TPS65090_REG_CG_CTRL3	0x07
+#define TPS65090_REG_CG_CTRL4	0x08
+#define TPS65090_REG_CG_CTRL5	0x09
+#define TPS65090_REG_CG_STATUS1	0x0a
+#define TPS65090_REG_CG_STATUS2	0x0b
+
+#define TPS65090_CHARGER_ENABLE	BIT(0)
+#define TPS65090_VACG		BIT(1)
+#define TPS65090_NOITERM	BIT(5)
+
+struct tps65090_charger {
+	struct	device	*dev;
+	int	ac_online;
+	int	prev_ac_online;
+	struct power_supply	ac;
+	struct tps65090_platform_data *pdata;
+};
+
+static enum power_supply_property tps65090_ac_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static int tps65090_low_chrg_current(struct tps65090_charger *charger)
+{
+	int ret;
+
+	ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
+			TPS65090_NOITERM);
+	if (ret < 0) {
+		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+			__func__, TPS65090_REG_CG_CTRL5);
+		return ret;
+	}
+	return 0;
+}
+
+static int tps65090_enable_charging(struct tps65090_charger *charger,
+	uint8_t enable)
+{
+	int ret;
+	uint8_t retval = 0;
+
+	ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+			    &retval);
+	if (ret < 0) {
+		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+				__func__, TPS65090_REG_CG_CTRL0);
+		return ret;
+	}
+
+	ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL0,
+				(retval | TPS65090_CHARGER_ENABLE));
+	if (ret < 0) {
+		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
+				__func__, TPS65090_REG_CG_CTRL0);
+		return ret;
+	}
+	return 0;
+}
+
+static int tps65090_config_charger(struct tps65090_charger *charger)
+{
+	int ret;
+
+	if (charger->pdata->enable_low_current_chrg) {
+		ret = tps65090_low_chrg_current(charger);
+		if (ret < 0) {
+			dev_err(charger->dev,
+				"error configuring low charge current\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int tps65090_ac_get_property(struct power_supply *psy,
+			enum power_supply_property psp,
+			union power_supply_propval *val)
+{
+	struct tps65090_charger *charger = container_of(psy,
+					struct tps65090_charger, ac);
+
+	if (psp == POWER_SUPPLY_PROP_ONLINE) {
+		val->intval = charger->ac_online;
+		charger->prev_ac_online = charger->ac_online;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
+{
+	struct tps65090_charger *charger = dev_id;
+	int ret;
+	uint8_t retval = 0;
+	uint8_t retval2 = 0;
+
+	ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_STATUS1,
+			    &retval);
+	if (ret < 0) {
+		dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+				__func__, TPS65090_REG_CG_STATUS1);
+		goto error;
+	}
+	msleep(75);
+	ret = tps65090_read(charger->dev->parent, TPS65090_REG_INTR_STS,
+			    &retval2);
+	if (ret < 0) {
+		dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
+				__func__, TPS65090_REG_INTR_STS);
+		goto error;
+	}
+
+
+	if (retval2 & TPS65090_VACG) {
+		ret = tps65090_enable_charging(charger, 1);
+		if (ret < 0)
+			goto error;
+		charger->ac_online = 1;
+	} else {
+		charger->ac_online = 0;
+	}
+
+	if (charger->prev_ac_online != charger->ac_online)
+		power_supply_changed(&charger->ac);
+error:
+	return IRQ_HANDLED;
+}
+
+#if defined(CONFIG_OF)
+
+#include <linux/of_device.h>
+
+static struct tps65090_platform_data *tps65090_parse_dt_charger_data(
+	struct platform_device *pdev)
+{
+	struct tps65090_platform_data *pdata;
+	struct device_node *np = pdev->dev.parent->of_node;
+	unsigned int prop;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "Memory alloc for tps65090_pdata failed\n");
+		return NULL;
+	}
+
+	prop = of_property_read_bool(np, "ti,enable-low-current-chrg");
+	pdata->enable_low_current_chrg = prop;
+
+	pdata->irq_base = -1;
+
+	return pdata;
+}
+#else
+static struct tps65090_platform_data *tps65090_parse_dt_charger_data(
+	struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
+static int tps65090_charger_probe(struct platform_device *pdev)
+{
+	struct tps65090 *tps65090_mfd = dev_get_drvdata(pdev->dev.parent);
+	struct tps65090_charger *charger_data;
+	struct tps65090_platform_data *pdata;
+	uint8_t retval = 0;
+	int ret;
+	int irq;
+
+	pdata = dev_get_platdata(pdev->dev.parent);
+
+	if (!pdata && tps65090_mfd->dev->of_node)
+		pdata = tps65090_parse_dt_charger_data(pdev);
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "%s():no platform data available\n",
+				__func__);
+		return -ENODEV;
+	}
+
+	charger_data = devm_kzalloc(&pdev->dev, sizeof(*charger_data),
+			GFP_KERNEL);
+	if (!charger_data) {
+		dev_err(&pdev->dev, "failed to allocate memory status\n");
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&pdev->dev, charger_data);
+
+	charger_data->dev = &pdev->dev;
+	charger_data->pdata = pdata;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
+		return irq;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+		tps65090_charger_isr, 0, "tps65090-charger", charger_data);
+	if (ret) {
+		dev_err(charger_data->dev, "Unable to register irq %d err %d\n",
+				irq, ret);
+		return ret;
+	}
+
+	charger_data->ac.name		= "tps65090-ac";
+	charger_data->ac.type		= POWER_SUPPLY_TYPE_MAINS;
+	charger_data->ac.get_property	= tps65090_ac_get_property;
+	charger_data->ac.properties	= tps65090_ac_props;
+	charger_data->ac.num_properties	= ARRAY_SIZE(tps65090_ac_props);
+	charger_data->ac.supplied_to	= pdata->supplied_to;
+	charger_data->ac.num_supplicants = pdata->num_supplicants;
+
+	ret = power_supply_register(&pdev->dev, &charger_data->ac);
+	if (ret) {
+		dev_err(&pdev->dev, "failed: power supply register\n");
+		goto fail_suppy_reg;
+	}
+
+	ret = tps65090_config_charger(charger_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
+		goto fail_config;
+	}
+
+	/* Check for charger presence */
+	ret = tps65090_read(charger_data->dev->parent, TPS65090_REG_CG_STATUS1,
+			&retval);
+	if (ret < 0) {
+		dev_err(charger_data->dev, "%s(): Error in reading reg 0x%x",
+			__func__, TPS65090_REG_CG_STATUS1);
+		goto fail_config;
+	}
+
+	if (retval != 0) {
+		ret = tps65090_enable_charging(charger_data, 1);
+		if (ret < 0) {
+			dev_err(charger_data->dev, "error enabling charger\n");
+			return ret;
+		}
+		charger_data->ac_online = 1;
+		power_supply_changed(&charger_data->ac);
+	}
+
+	return 0;
+fail_config:
+	power_supply_unregister(&charger_data->ac);
+
+fail_suppy_reg:
+	return ret;
+}
+
+static int tps65090_charger_remove(struct platform_device *pdev)
+{
+	struct tps65090_charger *charger = dev_get_drvdata(&pdev->dev);
+
+	power_supply_unregister(&charger->ac);
+	return 0;
+}
+
+static struct platform_driver tps65090_charger_driver = {
+	.driver	= {
+		.name	= "tps65090-charger",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= tps65090_charger_probe,
+	.remove = tps65090_charger_remove,
+};
+
+module_platform_driver(tps65090_charger_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Syed Rafiuddin <srafiuddin@nvidia.com>");
+MODULE_DESCRIPTION("tps65090 battery charger driver");
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 9ce231f..3f43069 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -87,6 +87,11 @@ struct tps65090_regulator_plat_data {
 
 struct tps65090_platform_data {
 	int irq_base;
+
+	char **supplied_to;
+	size_t num_supplicants;
+	int enable_low_current_chrg;
+
 	struct tps65090_regulator_plat_data *reg_pdata[TPS65090_REGULATOR_MAX];
 };
 
-- 
1.7.9.5

Re: [PATCH 4/4] power: tps65090: Add support for tps65090-charger

[Anton Vorontsov]

Hello Rhyland,

Thanks for the driver! A few comments down below...

On Wed, Feb 27, 2013 at 06:07:30PM -0500, Rhyland Klein wrote:
> This patch adds support for the tps65090 charger driver.

Would be nice to get a few more words about the hardware and the driver.
Why it is cool, main features, what is missing (if any), what is planned.

> Based on work by:
> Syed Rafiuddin <srafiuddin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Signed-off-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/power/Kconfig            |    7 +
>  drivers/power/Makefile           |    1 +
>  drivers/power/tps65090-charger.c |  310 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tps65090.h     |    5 +
>  4 files changed, 323 insertions(+)
>  create mode 100644 drivers/power/tps65090-charger.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 9f45e2f..6dcc3bd 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -340,6 +340,13 @@ config CHARGER_SMB347
>  	  Say Y to include support for Summit Microelectronics SMB347
>  	  Battery Charger.
>  
> +config CHARGER_TPS65090
> +	tristate "TPS65090 battery charger driver"
> +	depends on MFD_TPS65090
> +	help
> +	 Say Y here to enable support for battery charging with TPS65090
> +	 PMIC chips.
> +
>  config AB8500_BM
>  	bool "AB8500 Battery Management Driver"
>  	depends on AB8500_CORE && AB8500_GPADC
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 22c8913..9523f81 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -51,4 +51,5 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>  obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>  obj-$(CONFIG_POWER_AVS)		+= avs/
>  obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
> +obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
>  obj-$(CONFIG_POWER_RESET)	+= reset/
> diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
> new file mode 100644
> index 0000000..1234b81
> --- /dev/null
> +++ b/drivers/power/tps65090-charger.c
> @@ -0,0 +1,310 @@
> +/*
> + * Battery charger driver for TI's tps65090
> + *
> + * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
> +
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> +
> + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/mfd/tps65090.h>
> +
> +#define TPS65090_REG_INTR_STS	0x00
> +#define TPS65090_REG_CG_CTRL0	0x04
> +#define TPS65090_REG_CG_CTRL1	0x05
> +#define TPS65090_REG_CG_CTRL2	0x06
> +#define TPS65090_REG_CG_CTRL3	0x07
> +#define TPS65090_REG_CG_CTRL4	0x08
> +#define TPS65090_REG_CG_CTRL5	0x09
> +#define TPS65090_REG_CG_STATUS1	0x0a
> +#define TPS65090_REG_CG_STATUS2	0x0b
> +
> +#define TPS65090_CHARGER_ENABLE	BIT(0)
> +#define TPS65090_VACG		BIT(1)
> +#define TPS65090_NOITERM	BIT(5)
> +
> +struct tps65090_charger {
> +	struct	device	*dev;
> +	int	ac_online;
> +	int	prev_ac_online;
> +	struct power_supply	ac;
> +	struct tps65090_platform_data *pdata;
> +};
> +
> +static enum power_supply_property tps65090_ac_props[] = {
> +	POWER_SUPPLY_PROP_ONLINE,
> +};
> +
> +static int tps65090_low_chrg_current(struct tps65090_charger *charger)
> +{
> +	int ret;
> +
> +	ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
> +			TPS65090_NOITERM);
> +	if (ret < 0) {
> +		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
> +			__func__, TPS65090_REG_CG_CTRL5);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int tps65090_enable_charging(struct tps65090_charger *charger,
> +	uint8_t enable)
> +{
> +	int ret;
> +	uint8_t retval = 0;

maybe 'ctrl0' instead of retval?

> +
> +	ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
> +			    &retval);
> +	if (ret < 0) {
> +		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
> +				__func__, TPS65090_REG_CG_CTRL0);
> +		return ret;
> +	}
> +
> +	ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL0,
> +				(retval | TPS65090_CHARGER_ENABLE));
> +	if (ret < 0) {
> +		dev_err(charger->dev, "%s(): error reading in register 0x%x\n",
> +				__func__, TPS65090_REG_CG_CTRL0);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static int tps65090_config_charger(struct tps65090_charger *charger)
> +{
> +	int ret;
> +
> +	if (charger->pdata->enable_low_current_chrg) {
> +		ret = tps65090_low_chrg_current(charger);
> +		if (ret < 0) {
> +			dev_err(charger->dev,
> +				"error configuring low charge current\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps65090_ac_get_property(struct power_supply *psy,
> +			enum power_supply_property psp,
> +			union power_supply_propval *val)
> +{
> +	struct tps65090_charger *charger = container_of(psy,
> +					struct tps65090_charger, ac);
> +
> +	if (psp == POWER_SUPPLY_PROP_ONLINE) {
> +		val->intval = charger->ac_online;
> +		charger->prev_ac_online = charger->ac_online;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
> +{
> +	struct tps65090_charger *charger = dev_id;
> +	int ret;
> +	uint8_t retval = 0;
> +	uint8_t retval2 = 0;

... status1 = 0;
... intrsts = 0;

> +	ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_STATUS1,
> +			    &retval);
> +	if (ret < 0) {
> +		dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
> +				__func__, TPS65090_REG_CG_STATUS1);
> +		goto error;

You don't need the error label, can just 'return ...;'

> +	}
> +	msleep(75);
> +	ret = tps65090_read(charger->dev->parent, TPS65090_REG_INTR_STS,
> +			    &retval2);
> +	if (ret < 0) {
> +		dev_err(charger->dev, "%s(): Error in reading reg 0x%x\n",
> +				__func__, TPS65090_REG_INTR_STS);
> +		goto error;
> +	}
> +
> +
> +	if (retval2 & TPS65090_VACG) {
> +		ret = tps65090_enable_charging(charger, 1);
> +		if (ret < 0)
> +			goto error;
> +		charger->ac_online = 1;
> +	} else {
> +		charger->ac_online = 0;
> +	}
> +
> +	if (charger->prev_ac_online != charger->ac_online)
> +		power_supply_changed(&charger->ac);
> +error:
> +	return IRQ_HANDLED;
> +}
> +
> +#if defined(CONFIG_OF)
> +
> +#include <linux/of_device.h>
> +
> +static struct tps65090_platform_data *tps65090_parse_dt_charger_data(
> +	struct platform_device *pdev)

One more tab is needed. Plus I'd wrap the line after '*', to no leave the
hanging parenthesis.

> +{
> +	struct tps65090_platform_data *pdata;
> +	struct device_node *np = pdev->dev.parent->of_node;
> +	unsigned int prop;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +

No need for this empty space.

> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Memory alloc for tps65090_pdata failed\n");
> +		return NULL;
> +	}
> +
> +	prop = of_property_read_bool(np, "ti,enable-low-current-chrg");
> +	pdata->enable_low_current_chrg = prop;
> +
> +	pdata->irq_base = -1;
> +
> +	return pdata;
> +}

Need an empty space here, for consistency.

> +#else
> +static struct tps65090_platform_data *tps65090_parse_dt_charger_data(
> +	struct platform_device *pdev)

Same comment wrt. wrapping.

> +{
> +	return NULL;
> +}
> +#endif
> +
> +static int tps65090_charger_probe(struct platform_device *pdev)
> +{
> +	struct tps65090 *tps65090_mfd = dev_get_drvdata(pdev->dev.parent);
> +	struct tps65090_charger *charger_data;

Could be shorter 'cdata'?

> +	struct tps65090_platform_data *pdata;
> +	uint8_t retval = 0;

... status1 = 0;

> +	int ret;
> +	int irq;
> +
> +	pdata = dev_get_platdata(pdev->dev.parent);
> +
> +	if (!pdata && tps65090_mfd->dev->of_node)
> +		pdata = tps65090_parse_dt_charger_data(pdev);
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "%s():no platform data available\n",
> +				__func__);
> +		return -ENODEV;
> +	}
> +
> +	charger_data = devm_kzalloc(&pdev->dev, sizeof(*charger_data),
> +			GFP_KERNEL);
> +	if (!charger_data) {
> +		dev_err(&pdev->dev, "failed to allocate memory status\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, charger_data);
> +
> +	charger_data->dev = &pdev->dev;
> +	charger_data->pdata = pdata;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0) {
> +		dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +		tps65090_charger_isr, 0, "tps65090-charger", charger_data);

You should request irq after power_supply object (and charger_data) are
fully usable. Otherwise, if irq suddenly comes midway in the registration
process, we'll get an oops.

> +	if (ret) {
> +		dev_err(charger_data->dev, "Unable to register irq %d err %d\n",
> +				irq, ret);
> +		return ret;
> +	}
> +
> +	charger_data->ac.name		= "tps65090-ac";
> +	charger_data->ac.type		= POWER_SUPPLY_TYPE_MAINS;
> +	charger_data->ac.get_property	= tps65090_ac_get_property;
> +	charger_data->ac.properties	= tps65090_ac_props;
> +	charger_data->ac.num_properties	= ARRAY_SIZE(tps65090_ac_props);
> +	charger_data->ac.supplied_to	= pdata->supplied_to;
> +	charger_data->ac.num_supplicants = pdata->num_supplicants;
> +
> +	ret = power_supply_register(&pdev->dev, &charger_data->ac);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed: power supply register\n");
> +		goto fail_suppy_reg;
> +	}
> +
> +	ret = tps65090_config_charger(charger_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
> +		goto fail_config;
> +	}
> +
> +	/* Check for charger presence */
> +	ret = tps65090_read(charger_data->dev->parent, TPS65090_REG_CG_STATUS1,
> +			&retval);
> +	if (ret < 0) {
> +		dev_err(charger_data->dev, "%s(): Error in reading reg 0x%x",
> +			__func__, TPS65090_REG_CG_STATUS1);
> +		goto fail_config;
> +	}
> +
> +	if (retval != 0) {
> +		ret = tps65090_enable_charging(charger_data, 1);
> +		if (ret < 0) {
> +			dev_err(charger_data->dev, "error enabling charger\n");
> +			return ret;
> +		}
> +		charger_data->ac_online = 1;
> +		power_supply_changed(&charger_data->ac);
> +	}
> +
> +	return 0;
> +fail_config:
> +	power_supply_unregister(&charger_data->ac);
> +
> +fail_suppy_reg:
> +	return ret;
> +}
> +
> +static int tps65090_charger_remove(struct platform_device *pdev)
> +{
> +	struct tps65090_charger *charger = dev_get_drvdata(&pdev->dev);
> +
> +	power_supply_unregister(&charger->ac);

You have to unregister irq before unregistering power_supply object.
Otherwise irq routine might try to use the already unregistered object.

> +	return 0;
> +}
> +
> +static struct platform_driver tps65090_charger_driver = {
> +	.driver	= {
> +		.name	= "tps65090-charger",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe	= tps65090_charger_probe,
> +	.remove = tps65090_charger_remove,
> +};
> +

No need for this empty line.

> +module_platform_driver(tps65090_charger_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Syed Rafiuddin <srafiuddin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("tps65090 battery charger driver");

Re: [PATCH 4/4] power: tps65090: Add support for tps65090-charger

[Rhyland Klein]

On 3/2/2013 7:03 PM, Anton Vorontsov wrote:
> Hello Rhyland,
>
> Thanks for the driver! A few comments down below...
>
> On Wed, Feb 27, 2013 at 06:07:30PM -0500, Rhyland Klein wrote:
>> This patch adds support for the tps65090 charger driver.
> Would be nice to get a few more words about the hardware and the driver.
> Why it is cool, main features, what is missing (if any), what is planned.

Yah, this is really a child driver of the tps65090 mfd chip, so a lot of 
the driver coolness
is already defined there :) I'll look into the hw some and see what 
other coolness might
be possible to add around here :)

> ...
> +
> +     irq = platform_get_irq(pdev, 0);
> +     if (irq <= 0) {
> +             dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
> +             return irq;
> +     }
> +
> +     ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +             tps65090_charger_isr, 0, "tps65090-charger", charger_data);
> You should request irq after power_supply object (and charger_data) are
> fully usable. Otherwise, if irq suddenly comes midway in the registration
> process, we'll get an oops.

Yah the first time I looked at this the order struck me as kinda off, 
but I guess I got
sidetracked and forgot to go back to it. Thanks for catching this!

-rhyland

-- 
nvpublic