[PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 13577 bytes --]

TWL4030/TPS65950 is a multi-function device with integrated charger,
which allows charging from AC or USB. This driver enables the
charger and provides several monitoring functions.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
For this driver to work, TWL4030-core needs to be patched to use
correct macros so that it registers twl4030_bci platform_device.
I'll send patches for this later.

 drivers/power/Kconfig           |    7 +
 drivers/power/Makefile          |    1 +
 drivers/power/twl4030_charger.c |  499 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 507 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/twl4030_charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index cea6cef..95d7e60 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -110,4 +110,11 @@ config CHARGER_PCF50633
 	help
 	 Say Y to include support for NXP PCF50633 Main Battery Charger.
 
+config CHARGER_TWL4030
+	tristate "OMAP TWL4030 BCI charger driver"
+	depends on TWL4030_CORE
+	default y
+	help
+	  Say Y here to enable support for TWL4030 Battery Charge Interface.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index b96f29d..9cea9b5 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
 obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
 obj-$(CONFIG_BATTERY_MAX17040)	+= max17040_battery.o
 obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
+obj-$(CONFIG_CHARGER_TWL4030)	+= twl4030_charger.o
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
new file mode 100644
index 0000000..604dd56
--- /dev/null
+++ b/drivers/power/twl4030_charger.c
@@ -0,0 +1,499 @@
+/*
+ * TWL4030/TPS65950 BCI (Battery Charger Interface) driver
+ *
+ * Copyright (C) 2009 Gražvydas Ignotas <notasas@gmail.com>
+ *
+ * based on twl4030_bci_battery.c by TI
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ *
+ * 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.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/i2c/twl4030.h>
+#include <linux/power_supply.h>
+
+#define REG_BCIMSTATEC		0x02
+#define REG_BCIICHG		0x08
+#define REG_BCIVAC		0x0a
+#define REG_BCIVBUS		0x0c
+#define REG_BCIMFSTS4		0x10
+#define REG_BCICTL1		0x23
+
+#define REG_BOOT_BCI		0x07
+#define REG_STS_HW_CONDITIONS	0x0f
+
+#define BCIAUTOWEN		0x20
+#define CONFIG_DONE		0x10
+#define CVENAC			0x04
+#define BCIAUTOUSB		0x02
+#define BCIAUTOAC		0x01
+#define BCIMSTAT_MASK		0x3F
+#define STS_VBUS		0x80
+#define STS_CHG			0x02
+#define STS_USB_ID		0x04
+#define CGAIN			0x20
+#define USBFASTMCHG		0x04
+
+#define STATEC_USB		0x10
+#define STATEC_AC		0x20
+#define STATEC_STATUS_MASK	0x0f
+#define STATEC_QUICK1		0x02
+#define STATEC_QUICK7		0x07
+#define STATEC_COMPLETE1	0x0b
+#define STATEC_COMPLETE4	0x0e
+
+#define BCI_DELAY		100
+
+struct twl4030_bci_device_info {
+	struct power_supply	ac;
+	struct power_supply	usb;
+	struct delayed_work	bat_work;
+	bool			started;
+};
+
+/*
+ * clear and set bits on an given register on a given module
+ */
+static int twl4030_clear_set(u8 mod_no, u8 clear, u8 set, u8 reg)
+{
+	u8 val = 0;
+	int ret;
+
+	ret = twl4030_i2c_read_u8(mod_no, &val, reg);
+	if (ret)
+		return ret;
+
+	val &= ~clear;
+	val |= set;
+
+	return twl4030_i2c_write_u8(mod_no, val, reg);
+}
+
+static int twl4030bci_read_adc_val(u8 reg)
+{
+	int ret, temp;
+	u8 val;
+
+	/* read MSB */
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg + 1);
+	if (ret)
+		return ret;
+
+	temp = (int)(val & 0x03) << 8;
+
+	/* read LSB */
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg);
+	if (ret)
+		return ret;
+
+	return temp | val;
+}
+
+static void twl4030bci_power_work(struct work_struct *work)
+{
+	struct twl4030_bci_device_info *di = container_of(work,
+		struct twl4030_bci_device_info, bat_work.work);
+
+	power_supply_changed(&di->ac);
+	power_supply_changed(&di->usb);
+}
+
+/*
+ * Attend to TWL4030 CHG_PRES (AC charger presence) events
+ */
+static irqreturn_t twl4030_charger_interrupt(int irq, void *_di)
+{
+	struct twl4030_bci_device_info *di = _di;
+
+#ifdef CONFIG_LOCKDEP
+	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
+	 * we don't want and can't tolerate.  Although it might be
+	 * friendlier not to borrow this thread context...
+	 */
+	local_irq_enable();
+#endif
+
+	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Enable/Disable AC Charge funtionality.
+ */
+static int twl4030_charger_enable_ac(bool enable)
+{
+	int ret;
+
+	if (enable) {
+		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
+			CONFIG_DONE | BCIAUTOWEN | BCIAUTOAC,
+			REG_BOOT_BCI);
+	} else {
+		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 0*/
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOAC,
+			CONFIG_DONE | BCIAUTOWEN,
+			REG_BOOT_BCI);
+	}
+
+	return ret;
+}
+
+/*
+ * Check if VBUS power is present
+ */
+static int twl4030_charger_check_vbus(void)
+{
+	int ret;
+	u8 hwsts;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &hwsts,
+		REG_STS_HW_CONDITIONS);
+	if (ret) {
+		pr_err("twl4030_bci: error reading STS_HW_CONDITIONS\n");
+		return ret;
+	}
+
+	pr_debug("check_vbus: HW_CONDITIONS %02x\n", hwsts);
+
+	/* in case we also have STS_USB_ID, VBUS is driven by TWL itself */
+	if ((hwsts & STS_VBUS) && !(hwsts & STS_USB_ID))
+		return 1;
+
+	return 0;
+}
+
+/*
+ * Enable/Disable USB Charge funtionality.
+ */
+static int twl4030_charger_enable_usb(bool enable)
+{
+	int ret;
+
+	if (enable) {
+		/* Check for USB charger conneted */
+		ret = twl4030_charger_check_vbus();
+		if (ret < 0)
+			return ret;
+
+		if (!ret)
+			return -ENXIO;
+
+		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
+			CONFIG_DONE | BCIAUTOWEN | BCIAUTOUSB,
+			REG_BOOT_BCI);
+		if (ret)
+			return ret;
+
+		/* forcing USBFASTMCHG(BCIMFSTS4[2]) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_MAIN_CHARGE, 0,
+			USBFASTMCHG, REG_BCIMFSTS4);
+	} else {
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOUSB,
+			CONFIG_DONE | BCIAUTOWEN, REG_BOOT_BCI);
+	}
+
+	return ret;
+}
+
+/*
+ * Return voltage (valid while charging only)
+ * 10 bit ADC (0...0x3ff) scales to 0...6V
+ */
+static int twl4030_get_voltage(int reg)
+{
+	int ret = twl4030bci_read_adc_val(reg);
+	if (ret < 0)
+		return ret;
+
+	return 6000 * ret / 1023;
+}
+
+/*
+ * TI provided formulas:
+ * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 – 1) - 0.85
+ * CGAIN == 1: ICHG = (BCIICHG * 3.4) / (2^10 – 1) - 1.7
+ * Here we use integer approximation of:
+ * CGAIN == 0: val * 1.6618 - 0.85
+ * CGAIN == 1: (val * 1.6618 - 0.85) * 2
+ */
+static int twl4030_charger_get_current(void)
+{
+	int curr;
+	int ret;
+	u8 bcictl1;
+
+	curr = twl4030bci_read_adc_val(REG_BCIICHG);
+	if (curr < 0)
+		return curr;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &bcictl1,
+		REG_BCICTL1);
+	if (ret)
+		return ret;
+
+	ret = (curr * 16618 - 850 * 10000) / 10000;
+	if (bcictl1 & CGAIN)
+		ret *= 2;
+
+	return ret;
+}
+
+/*
+ * Returns the main charge FSM state
+ * Or < 0 on failure.
+ */
+static int twl4030bci_state(void)
+{
+	int ret;
+	u8 state;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+		&state, REG_BCIMSTATEC);
+	if (ret) {
+		pr_err("twl4030_bci: error reading BCIMSTATEC\n");
+		return ret;
+	}
+
+	pr_debug("state: %02x\n", state);
+
+	return state & BCIMSTAT_MASK;
+}
+
+static int twl4030_bci_state_to_status(int state)
+{
+	state &= STATEC_STATUS_MASK;
+	if (STATEC_QUICK1 <= state && state <= STATEC_QUICK7)
+		return POWER_SUPPLY_STATUS_CHARGING;
+	else if (STATEC_COMPLETE1 <= state && state <= STATEC_COMPLETE4)
+		return POWER_SUPPLY_STATUS_FULL;
+	else
+		return POWER_SUPPLY_STATUS_NOT_CHARGING;
+}
+
+static int twl4030_charger_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	int is_charging;
+	int voltage_reg;
+	int state;
+	int ret;
+
+	state = twl4030bci_state();
+	if (state < 0)
+		return state;
+
+	if (psy->type == POWER_SUPPLY_TYPE_USB) {
+		is_charging = state & STATEC_USB;
+		voltage_reg = REG_BCIVBUS;
+	} else {
+		is_charging = state & STATEC_AC;
+		voltage_reg = REG_BCIVAC;
+	}
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (is_charging)
+			val->intval = twl4030_bci_state_to_status(state);
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		/* charging must be active for meaningful result */
+		if (!is_charging) {
+			val->intval = 0;
+			break;
+		}
+		ret = twl4030_get_voltage(voltage_reg);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		if (!is_charging) {
+			val->intval = 0;
+			break;
+		}
+		/* current measurement is shared between AC and USB */
+		ret = twl4030_charger_get_current();
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = is_charging &&
+			twl4030_bci_state_to_status(state) !=
+				POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static enum power_supply_property twl4030_charger_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static struct twl4030_bci_device_info twl4030_bci = {
+	.ac = {
+		.name		= "twl4030_ac",
+		.type		= POWER_SUPPLY_TYPE_MAINS,
+		.properties	= twl4030_charger_props,
+		.num_properties = ARRAY_SIZE(twl4030_charger_props),
+		.get_property	= twl4030_charger_get_property,
+	},
+	.usb = {
+		.name		= "twl4030_usb",
+		.type		= POWER_SUPPLY_TYPE_USB,
+		.properties	= twl4030_charger_props,
+		.num_properties = ARRAY_SIZE(twl4030_charger_props),
+		.get_property	= twl4030_charger_get_property,
+	},
+};
+
+/*
+ * called by TWL4030 USB transceiver driver on USB_PRES interrupt.
+ */
+int twl4030charger_usb_en(int enable)
+{
+	if (twl4030_bci.started)
+		schedule_delayed_work(&twl4030_bci.bat_work,
+			msecs_to_jiffies(BCI_DELAY));
+
+	return twl4030_charger_enable_usb(enable);
+}
+
+static int __devinit twl4030_bci_probe(struct platform_device *pdev)
+{
+	int irq;
+	int ret;
+
+	twl4030_charger_enable_ac(true);
+	twl4030_charger_enable_usb(true);
+
+	irq = platform_get_irq(pdev, 0);
+
+	/* CHG_PRES irq */
+	ret = request_irq(irq, twl4030_charger_interrupt,
+		0, pdev->name, &twl4030_bci);
+	if (ret) {
+		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
+			irq, ret);
+		goto fail_chg_irq;
+	}
+
+	ret = power_supply_register(&pdev->dev, &twl4030_bci.ac);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register ac: %d\n", ret);
+		goto fail_register_ac;
+	}
+
+	ret = power_supply_register(&pdev->dev, &twl4030_bci.usb);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register usb: %d\n", ret);
+		goto fail_register_usb;
+	}
+
+	platform_set_drvdata(pdev, &twl4030_bci);
+
+	INIT_DELAYED_WORK_DEFERRABLE(&twl4030_bci.bat_work,
+			twl4030bci_power_work);
+	schedule_delayed_work(&twl4030_bci.bat_work,
+			msecs_to_jiffies(BCI_DELAY));
+	twl4030_bci.started = true;
+
+	return 0;
+
+fail_register_usb:
+	power_supply_unregister(&twl4030_bci.ac);
+fail_register_ac:
+	free_irq(irq, &twl4030_bci);
+fail_chg_irq:
+	twl4030_charger_enable_ac(false);
+	twl4030_charger_enable_usb(false);
+
+	return ret;
+}
+
+static int __devexit twl4030_bci_remove(struct platform_device *pdev)
+{
+	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	di->started = false;
+	twl4030_charger_enable_ac(false);
+	twl4030_charger_enable_usb(false);
+
+	free_irq(irq, di);
+
+	flush_scheduled_work();
+	power_supply_unregister(&di->ac);
+	power_supply_unregister(&di->usb);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int twl4030_bci_suspend(struct platform_device *pdev,
+	pm_message_t state)
+{
+	/* flush all pending status updates */
+	flush_scheduled_work();
+	return 0;
+}
+
+static int twl4030_bci_resume(struct platform_device *pdev)
+{
+	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
+
+	/* things may have changed while we were away */
+	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
+	return 0;
+}
+#else
+#define twl4030_bci_suspend	NULL
+#define twl4030_bci_resume	NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver twl4030_bci_driver = {
+	.probe		= twl4030_bci_probe,
+	.remove		= __devexit_p(twl4030_bci_remove),
+	.suspend	= twl4030_bci_suspend,
+	.resume		= twl4030_bci_resume,
+	.driver		= {
+		.name	= "twl4030_bci",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_bci_init(void)
+{
+	return platform_driver_register(&twl4030_bci_driver);
+}
+module_init(twl4030_bci_init);
+
+static void __exit twl4030_bci_exit(void)
+{
+	platform_driver_unregister(&twl4030_bci_driver);
+}
+module_exit(twl4030_bci_exit);
+
+MODULE_AUTHOR("Gražvydas Ignotas");
+MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:twl4030_bci");
-- 
1.6.3.3

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Fri, Nov 27, 2009 at 04:44:20PM +0200, Grazvydas Ignotas wrote:
> TWL4030/TPS65950 is a multi-function device with integrated charger,
> which allows charging from AC or USB. This driver enables the
> charger and provides several monitoring functions.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---

Thanks for the patch.

[...]
> +/*
> + * Attend to TWL4030 CHG_PRES (AC charger presence) events
> + */
> +static irqreturn_t twl4030_charger_interrupt(int irq, void *_di)
> +{
> +	struct twl4030_bci_device_info *di = _di;
> +
> +#ifdef CONFIG_LOCKDEP
> +	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> +	 * we don't want and can't tolerate.  Although it might be
> +	 * friendlier not to borrow this thread context...
> +	 */
> +	local_irq_enable();
> +#endif

Can you explain why the driver can't tolerate disabled irqs?
Calling schedule_delayed_work() from an irq context should be OK.

> +	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
> +
> +	return IRQ_HANDLED;
> +}

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On Fri, Nov 27, 2009 at 4:54 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Fri, Nov 27, 2009 at 04:44:20PM +0200, Grazvydas Ignotas wrote:
>> TWL4030/TPS65950 is a multi-function device with integrated charger,
>> which allows charging from AC or USB. This driver enables the
>> charger and provides several monitoring functions.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> ---
>
> Thanks for the patch.
>
> [...]
>> +/*
>> + * Attend to TWL4030 CHG_PRES (AC charger presence) events
>> + */
>> +static irqreturn_t twl4030_charger_interrupt(int irq, void *_di)
>> +{
>> +     struct twl4030_bci_device_info *di = _di;
>> +
>> +#ifdef CONFIG_LOCKDEP
>> +     /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
>> +      * we don't want and can't tolerate.  Although it might be
>> +      * friendlier not to borrow this thread context...
>> +      */
>> +     local_irq_enable();
>> +#endif
>
> Can you explain why the driver can't tolerate disabled irqs?
> Calling schedule_delayed_work() from an irq context should be OK.

Ah, this is leftover from TI code this driver is based on, which used
to do more things directly in interrupt handler. So I guess it can be
removed, updated patch attached.

>
>> +     schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
>> +
>> +     return IRQ_HANDLED;
>> +}
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>

[-- Attachment #2: 0001-power_supply-Add-driver-for-TWL4030-TPS65950-BCI-cha.patch --]
[-- Type: text/x-diff, Size: 13403 bytes --]

From 3255345be7a657bcdef024d329b923dc2b64b0a5 Mon Sep 17 00:00:00 2001
From: Grazvydas Ignotas <notasas@gmail.com>
Date: Fri, 27 Nov 2009 17:38:33 +0200
Subject: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

TWL4030/TPS65950 is a multi-function device with integrated charger,
which allows charging from AC or USB. This driver enables the
charger and provides several monitoring functions.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 drivers/power/Kconfig           |    7 +
 drivers/power/Makefile          |    1 +
 drivers/power/twl4030_charger.c |  491 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 499 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/twl4030_charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index cea6cef..95d7e60 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -110,4 +110,11 @@ config CHARGER_PCF50633
 	help
 	 Say Y to include support for NXP PCF50633 Main Battery Charger.
 
+config CHARGER_TWL4030
+	tristate "OMAP TWL4030 BCI charger driver"
+	depends on TWL4030_CORE
+	default y
+	help
+	  Say Y here to enable support for TWL4030 Battery Charge Interface.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index b96f29d..9cea9b5 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
 obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
 obj-$(CONFIG_BATTERY_MAX17040)	+= max17040_battery.o
 obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
+obj-$(CONFIG_CHARGER_TWL4030)	+= twl4030_charger.o
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
new file mode 100644
index 0000000..a0b2691
--- /dev/null
+++ b/drivers/power/twl4030_charger.c
@@ -0,0 +1,491 @@
+/*
+ * TWL4030/TPS65950 BCI (Battery Charger Interface) driver
+ *
+ * Copyright (C) 2009 Gražvydas Ignotas <notasas@gmail.com>
+ *
+ * based on twl4030_bci_battery.c by TI
+ * Copyright (C) 2008 Texas Instruments, Inc.
+ *
+ * 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.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/i2c/twl4030.h>
+#include <linux/power_supply.h>
+
+#define REG_BCIMSTATEC		0x02
+#define REG_BCIICHG		0x08
+#define REG_BCIVAC		0x0a
+#define REG_BCIVBUS		0x0c
+#define REG_BCIMFSTS4		0x10
+#define REG_BCICTL1		0x23
+
+#define REG_BOOT_BCI		0x07
+#define REG_STS_HW_CONDITIONS	0x0f
+
+#define BCIAUTOWEN		0x20
+#define CONFIG_DONE		0x10
+#define CVENAC			0x04
+#define BCIAUTOUSB		0x02
+#define BCIAUTOAC		0x01
+#define BCIMSTAT_MASK		0x3F
+#define STS_VBUS		0x80
+#define STS_CHG			0x02
+#define STS_USB_ID		0x04
+#define CGAIN			0x20
+#define USBFASTMCHG		0x04
+
+#define STATEC_USB		0x10
+#define STATEC_AC		0x20
+#define STATEC_STATUS_MASK	0x0f
+#define STATEC_QUICK1		0x02
+#define STATEC_QUICK7		0x07
+#define STATEC_COMPLETE1	0x0b
+#define STATEC_COMPLETE4	0x0e
+
+#define BCI_DELAY		100
+
+struct twl4030_bci_device_info {
+	struct power_supply	ac;
+	struct power_supply	usb;
+	struct delayed_work	bat_work;
+	bool			started;
+};
+
+/*
+ * clear and set bits on an given register on a given module
+ */
+static int twl4030_clear_set(u8 mod_no, u8 clear, u8 set, u8 reg)
+{
+	u8 val = 0;
+	int ret;
+
+	ret = twl4030_i2c_read_u8(mod_no, &val, reg);
+	if (ret)
+		return ret;
+
+	val &= ~clear;
+	val |= set;
+
+	return twl4030_i2c_write_u8(mod_no, val, reg);
+}
+
+static int twl4030bci_read_adc_val(u8 reg)
+{
+	int ret, temp;
+	u8 val;
+
+	/* read MSB */
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg + 1);
+	if (ret)
+		return ret;
+
+	temp = (int)(val & 0x03) << 8;
+
+	/* read LSB */
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg);
+	if (ret)
+		return ret;
+
+	return temp | val;
+}
+
+static void twl4030bci_power_work(struct work_struct *work)
+{
+	struct twl4030_bci_device_info *di = container_of(work,
+		struct twl4030_bci_device_info, bat_work.work);
+
+	power_supply_changed(&di->ac);
+	power_supply_changed(&di->usb);
+}
+
+/*
+ * Attend to TWL4030 CHG_PRES (AC charger presence) events
+ */
+static irqreturn_t twl4030_charger_interrupt(int irq, void *_di)
+{
+	struct twl4030_bci_device_info *di = _di;
+
+	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Enable/Disable AC Charge funtionality.
+ */
+static int twl4030_charger_enable_ac(bool enable)
+{
+	int ret;
+
+	if (enable) {
+		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
+			CONFIG_DONE | BCIAUTOWEN | BCIAUTOAC,
+			REG_BOOT_BCI);
+	} else {
+		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 0*/
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOAC,
+			CONFIG_DONE | BCIAUTOWEN,
+			REG_BOOT_BCI);
+	}
+
+	return ret;
+}
+
+/*
+ * Check if VBUS power is present
+ */
+static int twl4030_charger_check_vbus(void)
+{
+	int ret;
+	u8 hwsts;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &hwsts,
+		REG_STS_HW_CONDITIONS);
+	if (ret) {
+		pr_err("twl4030_bci: error reading STS_HW_CONDITIONS\n");
+		return ret;
+	}
+
+	pr_debug("check_vbus: HW_CONDITIONS %02x\n", hwsts);
+
+	/* in case we also have STS_USB_ID, VBUS is driven by TWL itself */
+	if ((hwsts & STS_VBUS) && !(hwsts & STS_USB_ID))
+		return 1;
+
+	return 0;
+}
+
+/*
+ * Enable/Disable USB Charge funtionality.
+ */
+static int twl4030_charger_enable_usb(bool enable)
+{
+	int ret;
+
+	if (enable) {
+		/* Check for USB charger conneted */
+		ret = twl4030_charger_check_vbus();
+		if (ret < 0)
+			return ret;
+
+		if (!ret)
+			return -ENXIO;
+
+		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
+			CONFIG_DONE | BCIAUTOWEN | BCIAUTOUSB,
+			REG_BOOT_BCI);
+		if (ret)
+			return ret;
+
+		/* forcing USBFASTMCHG(BCIMFSTS4[2]) to 1 */
+		ret = twl4030_clear_set(TWL4030_MODULE_MAIN_CHARGE, 0,
+			USBFASTMCHG, REG_BCIMFSTS4);
+	} else {
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOUSB,
+			CONFIG_DONE | BCIAUTOWEN, REG_BOOT_BCI);
+	}
+
+	return ret;
+}
+
+/*
+ * Return voltage (valid while charging only)
+ * 10 bit ADC (0...0x3ff) scales to 0...6V
+ */
+static int twl4030_get_voltage(int reg)
+{
+	int ret = twl4030bci_read_adc_val(reg);
+	if (ret < 0)
+		return ret;
+
+	return 6000 * ret / 1023;
+}
+
+/*
+ * TI provided formulas:
+ * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 – 1) - 0.85
+ * CGAIN == 1: ICHG = (BCIICHG * 3.4) / (2^10 – 1) - 1.7
+ * Here we use integer approximation of:
+ * CGAIN == 0: val * 1.6618 - 0.85
+ * CGAIN == 1: (val * 1.6618 - 0.85) * 2
+ */
+static int twl4030_charger_get_current(void)
+{
+	int curr;
+	int ret;
+	u8 bcictl1;
+
+	curr = twl4030bci_read_adc_val(REG_BCIICHG);
+	if (curr < 0)
+		return curr;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &bcictl1,
+		REG_BCICTL1);
+	if (ret)
+		return ret;
+
+	ret = (curr * 16618 - 850 * 10000) / 10000;
+	if (bcictl1 & CGAIN)
+		ret *= 2;
+
+	return ret;
+}
+
+/*
+ * Returns the main charge FSM state
+ * Or < 0 on failure.
+ */
+static int twl4030bci_state(void)
+{
+	int ret;
+	u8 state;
+
+	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+		&state, REG_BCIMSTATEC);
+	if (ret) {
+		pr_err("twl4030_bci: error reading BCIMSTATEC\n");
+		return ret;
+	}
+
+	pr_debug("state: %02x\n", state);
+
+	return state & BCIMSTAT_MASK;
+}
+
+static int twl4030_bci_state_to_status(int state)
+{
+	state &= STATEC_STATUS_MASK;
+	if (STATEC_QUICK1 <= state && state <= STATEC_QUICK7)
+		return POWER_SUPPLY_STATUS_CHARGING;
+	else if (STATEC_COMPLETE1 <= state && state <= STATEC_COMPLETE4)
+		return POWER_SUPPLY_STATUS_FULL;
+	else
+		return POWER_SUPPLY_STATUS_NOT_CHARGING;
+}
+
+static int twl4030_charger_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	int is_charging;
+	int voltage_reg;
+	int state;
+	int ret;
+
+	state = twl4030bci_state();
+	if (state < 0)
+		return state;
+
+	if (psy->type == POWER_SUPPLY_TYPE_USB) {
+		is_charging = state & STATEC_USB;
+		voltage_reg = REG_BCIVBUS;
+	} else {
+		is_charging = state & STATEC_AC;
+		voltage_reg = REG_BCIVAC;
+	}
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (is_charging)
+			val->intval = twl4030_bci_state_to_status(state);
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		/* charging must be active for meaningful result */
+		if (!is_charging) {
+			val->intval = 0;
+			break;
+		}
+		ret = twl4030_get_voltage(voltage_reg);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		if (!is_charging) {
+			val->intval = 0;
+			break;
+		}
+		/* current measurement is shared between AC and USB */
+		ret = twl4030_charger_get_current();
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = is_charging &&
+			twl4030_bci_state_to_status(state) !=
+				POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static enum power_supply_property twl4030_charger_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static struct twl4030_bci_device_info twl4030_bci = {
+	.ac = {
+		.name		= "twl4030_ac",
+		.type		= POWER_SUPPLY_TYPE_MAINS,
+		.properties	= twl4030_charger_props,
+		.num_properties = ARRAY_SIZE(twl4030_charger_props),
+		.get_property	= twl4030_charger_get_property,
+	},
+	.usb = {
+		.name		= "twl4030_usb",
+		.type		= POWER_SUPPLY_TYPE_USB,
+		.properties	= twl4030_charger_props,
+		.num_properties = ARRAY_SIZE(twl4030_charger_props),
+		.get_property	= twl4030_charger_get_property,
+	},
+};
+
+/*
+ * called by TWL4030 USB transceiver driver on USB_PRES interrupt.
+ */
+int twl4030charger_usb_en(int enable)
+{
+	if (twl4030_bci.started)
+		schedule_delayed_work(&twl4030_bci.bat_work,
+			msecs_to_jiffies(BCI_DELAY));
+
+	return twl4030_charger_enable_usb(enable);
+}
+
+static int __devinit twl4030_bci_probe(struct platform_device *pdev)
+{
+	int irq;
+	int ret;
+
+	twl4030_charger_enable_ac(true);
+	twl4030_charger_enable_usb(true);
+
+	irq = platform_get_irq(pdev, 0);
+
+	/* CHG_PRES irq */
+	ret = request_irq(irq, twl4030_charger_interrupt,
+		0, pdev->name, &twl4030_bci);
+	if (ret) {
+		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
+			irq, ret);
+		goto fail_chg_irq;
+	}
+
+	ret = power_supply_register(&pdev->dev, &twl4030_bci.ac);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register ac: %d\n", ret);
+		goto fail_register_ac;
+	}
+
+	ret = power_supply_register(&pdev->dev, &twl4030_bci.usb);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register usb: %d\n", ret);
+		goto fail_register_usb;
+	}
+
+	platform_set_drvdata(pdev, &twl4030_bci);
+
+	INIT_DELAYED_WORK_DEFERRABLE(&twl4030_bci.bat_work,
+			twl4030bci_power_work);
+	schedule_delayed_work(&twl4030_bci.bat_work,
+			msecs_to_jiffies(BCI_DELAY));
+	twl4030_bci.started = true;
+
+	return 0;
+
+fail_register_usb:
+	power_supply_unregister(&twl4030_bci.ac);
+fail_register_ac:
+	free_irq(irq, &twl4030_bci);
+fail_chg_irq:
+	twl4030_charger_enable_ac(false);
+	twl4030_charger_enable_usb(false);
+
+	return ret;
+}
+
+static int __devexit twl4030_bci_remove(struct platform_device *pdev)
+{
+	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	di->started = false;
+	twl4030_charger_enable_ac(false);
+	twl4030_charger_enable_usb(false);
+
+	free_irq(irq, di);
+
+	flush_scheduled_work();
+	power_supply_unregister(&di->ac);
+	power_supply_unregister(&di->usb);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int twl4030_bci_suspend(struct platform_device *pdev,
+	pm_message_t state)
+{
+	/* flush all pending status updates */
+	flush_scheduled_work();
+	return 0;
+}
+
+static int twl4030_bci_resume(struct platform_device *pdev)
+{
+	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
+
+	/* things may have changed while we were away */
+	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
+	return 0;
+}
+#else
+#define twl4030_bci_suspend	NULL
+#define twl4030_bci_resume	NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver twl4030_bci_driver = {
+	.probe		= twl4030_bci_probe,
+	.remove		= __devexit_p(twl4030_bci_remove),
+	.suspend	= twl4030_bci_suspend,
+	.resume		= twl4030_bci_resume,
+	.driver		= {
+		.name	= "twl4030_bci",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_bci_init(void)
+{
+	return platform_driver_register(&twl4030_bci_driver);
+}
+module_init(twl4030_bci_init);
+
+static void __exit twl4030_bci_exit(void)
+{
+	platform_driver_unregister(&twl4030_bci_driver);
+}
+module_exit(twl4030_bci_exit);
+
+MODULE_AUTHOR("Gražvydas Ignotas");
+MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:twl4030_bci");
-- 
1.6.3.3

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Mark Brown]

On Fri, Nov 27, 2009 at 05:47:40PM +0200, Grazvydas Ignotas wrote:
> On Fri, Nov 27, 2009 at 4:54 PM, Anton Vorontsov

> >> + ? ? /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> >> + ? ? ?* we don't want and can't tolerate. ?Although it might be
> >> + ? ? ?* friendlier not to borrow this thread context...
> >> + ? ? ?*/
> >> + ? ? local_irq_enable();
> >> +#endif

> > Can you explain why the driver can't tolerate disabled irqs?
> > Calling schedule_delayed_work() from an irq context should be OK.

> Ah, this is leftover from TI code this driver is based on, which used
> to do more things directly in interrupt handler. So I guess it can be
> removed, updated patch attached.

Actually, the genirq infrastructure in 2.6.32 has been improved to allow
I2C based interrupt controllers properly so even those twl4030 drivers
that do I/O in interrupt callbacks should now be able to run without
these workarounds once the core has been updated.

RE: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Madhusudhan]

> -----Original Message-----
> From: Grazvydas Ignotas [mailto:notasas@gmail.com]
> Sent: Friday, November 27, 2009 8:44 AM
> To: linux-kernel@vger.kernel.org
> Cc: Anton Vorontsov; Madhusudhan Chikkature; linux-omap@vger.kernel.org;
> Grazvydas Ignotas
> Subject: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger
> 
> TWL4030/TPS65950 is a multi-function device with integrated charger,
> which allows charging from AC or USB. This driver enables the
> charger and provides several monitoring functions.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
> For this driver to work, TWL4030-core needs to be patched to use
> correct macros so that it registers twl4030_bci platform_device.
> I'll send patches for this later.
> 
>  drivers/power/Kconfig           |    7 +
>  drivers/power/Makefile          |    1 +
>  drivers/power/twl4030_charger.c |  499

Is the file name changed from twl4030_bci_battery.c to twl4030_charger.c because it mainly supports voltage monitoring only while charging? If yes, potentially we can add support for monitoring also in discharge state. Do we intend to change the file name then?

Also adding the tested-on info could be helpful here.

> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 507 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/power/twl4030_charger.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index cea6cef..95d7e60 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -110,4 +110,11 @@ config CHARGER_PCF50633
>  	help
>  	 Say Y to include support for NXP PCF50633 Main Battery Charger.
> 
> +config CHARGER_TWL4030
> +	tristate "OMAP TWL4030 BCI charger driver"
> +	depends on TWL4030_CORE
> +	default y
> +	help
> +	  Say Y here to enable support for TWL4030 Battery Charge Interface.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b96f29d..9cea9b5 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_BATTERY_BQ27x00)	+= bq27x00_battery.o
>  obj-$(CONFIG_BATTERY_DA9030)	+= da9030_battery.o
>  obj-$(CONFIG_BATTERY_MAX17040)	+= max17040_battery.o
>  obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
> +obj-$(CONFIG_CHARGER_TWL4030)	+= twl4030_charger.o
> diff --git a/drivers/power/twl4030_charger.c
> b/drivers/power/twl4030_charger.c
> new file mode 100644
> index 0000000..604dd56
> --- /dev/null
> +++ b/drivers/power/twl4030_charger.c
> @@ -0,0 +1,499 @@
> +/*
> + * TWL4030/TPS65950 BCI (Battery Charger Interface) driver
> + *
> + * Copyright (C) 2009 Gražvydas Ignotas <notasas@gmail.com>
> + *
> + * based on twl4030_bci_battery.c by TI
> + * Copyright (C) 2008 Texas Instruments, Inc.
> + *
> + * 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.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c/twl4030.h>
> +#include <linux/power_supply.h>
> +
> +#define REG_BCIMSTATEC		0x02
> +#define REG_BCIICHG		0x08
> +#define REG_BCIVAC		0x0a
> +#define REG_BCIVBUS		0x0c
> +#define REG_BCIMFSTS4		0x10
> +#define REG_BCICTL1		0x23
> +
> +#define REG_BOOT_BCI		0x07
> +#define REG_STS_HW_CONDITIONS	0x0f
> +
> +#define BCIAUTOWEN		0x20
> +#define CONFIG_DONE		0x10
> +#define CVENAC			0x04
> +#define BCIAUTOUSB		0x02
> +#define BCIAUTOAC		0x01
> +#define BCIMSTAT_MASK		0x3F
> +#define STS_VBUS		0x80
> +#define STS_CHG			0x02
> +#define STS_USB_ID		0x04
> +#define CGAIN			0x20
> +#define USBFASTMCHG		0x04
> +
> +#define STATEC_USB		0x10
> +#define STATEC_AC		0x20
> +#define STATEC_STATUS_MASK	0x0f
> +#define STATEC_QUICK1		0x02
> +#define STATEC_QUICK7		0x07
> +#define STATEC_COMPLETE1	0x0b
> +#define STATEC_COMPLETE4	0x0e
> +
> +#define BCI_DELAY		100
> +
> +struct twl4030_bci_device_info {
> +	struct power_supply	ac;
> +	struct power_supply	usb;
> +	struct delayed_work	bat_work;
> +	bool			started;
> +};
> +
> +/*
> + * clear and set bits on an given register on a given module
> + */
> +static int twl4030_clear_set(u8 mod_no, u8 clear, u8 set, u8 reg)
> +{
> +	u8 val = 0;
> +	int ret;
> +
> +	ret = twl4030_i2c_read_u8(mod_no, &val, reg);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~clear;
> +	val |= set;
> +
> +	return twl4030_i2c_write_u8(mod_no, val, reg);
> +}
> +
> +static int twl4030bci_read_adc_val(u8 reg)
> +{
> +	int ret, temp;
> +	u8 val;
> +
> +	/* read MSB */
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg +
> 1);
> +	if (ret)
> +		return ret;
> +
> +	temp = (int)(val & 0x03) << 8;
> +
> +	/* read LSB */
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &val, reg);
> +	if (ret)
> +		return ret;
> +
> +	return temp | val;
> +}
> +
> +static void twl4030bci_power_work(struct work_struct *work)
> +{
> +	struct twl4030_bci_device_info *di = container_of(work,
> +		struct twl4030_bci_device_info, bat_work.work);
> +
> +	power_supply_changed(&di->ac);
> +	power_supply_changed(&di->usb);
> +}
> +
> +/*
> + * Attend to TWL4030 CHG_PRES (AC charger presence) events
> + */
> +static irqreturn_t twl4030_charger_interrupt(int irq, void *_di)
> +{
> +	struct twl4030_bci_device_info *di = _di;
> +
> +#ifdef CONFIG_LOCKDEP
> +	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> +	 * we don't want and can't tolerate.  Although it might be
> +	 * friendlier not to borrow this thread context...
> +	 */
> +	local_irq_enable();
> +#endif
> +
> +	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Enable/Disable AC Charge funtionality.
> + */
> +static int twl4030_charger_enable_ac(bool enable)
> +{
> +	int ret;
> +
> +	if (enable) {
> +		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 1 */
> +		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
> +			CONFIG_DONE | BCIAUTOWEN | BCIAUTOAC,
> +			REG_BOOT_BCI);
> +	} else {
> +		/* forcing the field BCIAUTOAC (BOOT_BCI[0) to 0*/
> +		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOAC,
> +			CONFIG_DONE | BCIAUTOWEN,
> +			REG_BOOT_BCI);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Check if VBUS power is present
> + */
> +static int twl4030_charger_check_vbus(void)
> +{
> +	int ret;
> +	u8 hwsts;
> +
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &hwsts,
> +		REG_STS_HW_CONDITIONS);
> +	if (ret) {
> +		pr_err("twl4030_bci: error reading STS_HW_CONDITIONS\n");
> +		return ret;
> +	}
> +
> +	pr_debug("check_vbus: HW_CONDITIONS %02x\n", hwsts);
> +
> +	/* in case we also have STS_USB_ID, VBUS is driven by TWL itself */
> +	if ((hwsts & STS_VBUS) && !(hwsts & STS_USB_ID))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +/*
> + * Enable/Disable USB Charge funtionality.
> + */
> +static int twl4030_charger_enable_usb(bool enable)
> +{
> +	int ret;
> +
> +	if (enable) {
> +		/* Check for USB charger conneted */
> +		ret = twl4030_charger_check_vbus();
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!ret)
> +			return -ENXIO;
> +
> +		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
> +		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
> +			CONFIG_DONE | BCIAUTOWEN | BCIAUTOUSB,
> +			REG_BOOT_BCI);
> +		if (ret)
> +			return ret;
> +
> +		/* forcing USBFASTMCHG(BCIMFSTS4[2]) to 1 */
> +		ret = twl4030_clear_set(TWL4030_MODULE_MAIN_CHARGE, 0,
> +			USBFASTMCHG, REG_BCIMFSTS4);
> +	} else {
> +		ret = twl4030_clear_set(TWL4030_MODULE_PM_MASTER, BCIAUTOUSB,
> +			CONFIG_DONE | BCIAUTOWEN, REG_BOOT_BCI);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Return voltage (valid while charging only)
> + * 10 bit ADC (0...0x3ff) scales to 0...6V
> + */
> +static int twl4030_get_voltage(int reg)
> +{
> +	int ret = twl4030bci_read_adc_val(reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 6000 * ret / 1023;
> +}
> +
> +/*
> + * TI provided formulas:
> + * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 – 1) - 0.85
> + * CGAIN == 1: ICHG = (BCIICHG * 3.4) / (2^10 – 1) - 1.7
> + * Here we use integer approximation of:
> + * CGAIN == 0: val * 1.6618 - 0.85
> + * CGAIN == 1: (val * 1.6618 - 0.85) * 2
> + */
> +static int twl4030_charger_get_current(void)
> +{
> +	int curr;
> +	int ret;
> +	u8 bcictl1;
> +
> +	curr = twl4030bci_read_adc_val(REG_BCIICHG);
> +	if (curr < 0)
> +		return curr;
> +
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE, &bcictl1,
> +		REG_BCICTL1);
> +	if (ret)
> +		return ret;
> +
> +	ret = (curr * 16618 - 850 * 10000) / 10000;
> +	if (bcictl1 & CGAIN)
> +		ret *= 2;
> +
> +	return ret;
> +}
> +
> +/*
> + * Returns the main charge FSM state
> + * Or < 0 on failure.
> + */
> +static int twl4030bci_state(void)
> +{
> +	int ret;
> +	u8 state;
> +
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> +		&state, REG_BCIMSTATEC);
> +	if (ret) {
> +		pr_err("twl4030_bci: error reading BCIMSTATEC\n");
> +		return ret;
> +	}
> +
> +	pr_debug("state: %02x\n", state);
> +
> +	return state & BCIMSTAT_MASK;
> +}
> +
> +static int twl4030_bci_state_to_status(int state)
> +{
> +	state &= STATEC_STATUS_MASK;
> +	if (STATEC_QUICK1 <= state && state <= STATEC_QUICK7)
> +		return POWER_SUPPLY_STATUS_CHARGING;
> +	else if (STATEC_COMPLETE1 <= state && state <= STATEC_COMPLETE4)
> +		return POWER_SUPPLY_STATUS_FULL;
> +	else
> +		return POWER_SUPPLY_STATUS_NOT_CHARGING;
> +}
> +
> +static int twl4030_charger_get_property(struct power_supply *psy,
> +					enum power_supply_property psp,
> +					union power_supply_propval *val)
> +{
> +	int is_charging;
> +	int voltage_reg;
> +	int state;
> +	int ret;
> +
> +	state = twl4030bci_state();
> +	if (state < 0)
> +		return state;
> +
> +	if (psy->type == POWER_SUPPLY_TYPE_USB) {
> +		is_charging = state & STATEC_USB;
> +		voltage_reg = REG_BCIVBUS;
> +	} else {
> +		is_charging = state & STATEC_AC;
> +		voltage_reg = REG_BCIVAC;
> +	}
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (is_charging)
> +			val->intval = twl4030_bci_state_to_status(state);
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		/* charging must be active for meaningful result */
> +		if (!is_charging) {

How about putting a kern_info here?

> +			val->intval = 0;
> +			break;
> +		}
> +		ret = twl4030_get_voltage(voltage_reg);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		if (!is_charging) {
> +			val->intval = 0;
Ditto
> +			break;
> +		}
> +		/* current measurement is shared between AC and USB */
> +		ret = twl4030_charger_get_current();
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
Does this indicate the source of charging like USB or AC??
> +		val->intval = is_charging &&
> +			twl4030_bci_state_to_status(state) !=
> +				POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static enum power_supply_property twl4030_charger_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static struct twl4030_bci_device_info twl4030_bci = {
> +	.ac = {
> +		.name		= "twl4030_ac",
> +		.type		= POWER_SUPPLY_TYPE_MAINS,
> +		.properties	= twl4030_charger_props,
> +		.num_properties = ARRAY_SIZE(twl4030_charger_props),
> +		.get_property	= twl4030_charger_get_property,
> +	},
> +	.usb = {
> +		.name		= "twl4030_usb",
> +		.type		= POWER_SUPPLY_TYPE_USB,
> +		.properties	= twl4030_charger_props,
> +		.num_properties = ARRAY_SIZE(twl4030_charger_props),
> +		.get_property	= twl4030_charger_get_property,
> +	},
> +};
> +
> +/*
> + * called by TWL4030 USB transceiver driver on USB_PRES interrupt.
> + */
> +int twl4030charger_usb_en(int enable)
> +{
> +	if (twl4030_bci.started)
> +		schedule_delayed_work(&twl4030_bci.bat_work,
> +			msecs_to_jiffies(BCI_DELAY));
> +
> +	return twl4030_charger_enable_usb(enable);
> +}
> +
> +static int __devinit twl4030_bci_probe(struct platform_device *pdev)
> +{
> +	int irq;
> +	int ret;
> +
> +	twl4030_charger_enable_ac(true);
> +	twl4030_charger_enable_usb(true);
> +
> +	irq = platform_get_irq(pdev, 0);
> +
> +	/* CHG_PRES irq */
> +	ret = request_irq(irq, twl4030_charger_interrupt,
> +		0, pdev->name, &twl4030_bci);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
> +			irq, ret);
> +		goto fail_chg_irq;
> +	}
> +
> +	ret = power_supply_register(&pdev->dev, &twl4030_bci.ac);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register ac: %d\n", ret);
> +		goto fail_register_ac;
> +	}
> +
> +	ret = power_supply_register(&pdev->dev, &twl4030_bci.usb);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register usb: %d\n", ret);
> +		goto fail_register_usb;
> +	}
> +
> +	platform_set_drvdata(pdev, &twl4030_bci);
> +
> +	INIT_DELAYED_WORK_DEFERRABLE(&twl4030_bci.bat_work,
> +			twl4030bci_power_work);
> +	schedule_delayed_work(&twl4030_bci.bat_work,
> +			msecs_to_jiffies(BCI_DELAY));
> +	twl4030_bci.started = true;
> +
> +	return 0;
> +
> +fail_register_usb:
> +	power_supply_unregister(&twl4030_bci.ac);
> +fail_register_ac:
> +	free_irq(irq, &twl4030_bci);
> +fail_chg_irq:
> +	twl4030_charger_enable_ac(false);
> +	twl4030_charger_enable_usb(false);
> +
> +	return ret;
> +}
> +
> +static int __devexit twl4030_bci_remove(struct platform_device *pdev)
> +{
> +	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	di->started = false;
> +	twl4030_charger_enable_ac(false);
> +	twl4030_charger_enable_usb(false);
> +
> +	free_irq(irq, di);
> +
> +	flush_scheduled_work();
> +	power_supply_unregister(&di->ac);
> +	power_supply_unregister(&di->usb);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int twl4030_bci_suspend(struct platform_device *pdev,
> +	pm_message_t state)
> +{
> +	/* flush all pending status updates */
> +	flush_scheduled_work();
> +	return 0;
> +}
> +
> +static int twl4030_bci_resume(struct platform_device *pdev)
> +{
> +	struct twl4030_bci_device_info *di = platform_get_drvdata(pdev);
> +
> +	/* things may have changed while we were away */
> +	schedule_delayed_work(&di->bat_work, msecs_to_jiffies(BCI_DELAY));
> +	return 0;
> +}
> +#else
> +#define twl4030_bci_suspend	NULL
> +#define twl4030_bci_resume	NULL
> +#endif /* CONFIG_PM */
> +
> +static struct platform_driver twl4030_bci_driver = {
> +	.probe		= twl4030_bci_probe,
> +	.remove		= __devexit_p(twl4030_bci_remove),
> +	.suspend	= twl4030_bci_suspend,
> +	.resume		= twl4030_bci_resume,
> +	.driver		= {
> +		.name	= "twl4030_bci",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init twl4030_bci_init(void)
> +{
> +	return platform_driver_register(&twl4030_bci_driver);
> +}
> +module_init(twl4030_bci_init);
> +
> +static void __exit twl4030_bci_exit(void)
> +{
> +	platform_driver_unregister(&twl4030_bci_driver);
> +}
> +module_exit(twl4030_bci_exit);
> +
> +MODULE_AUTHOR("Gražvydas Ignotas");
> +MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:twl4030_bci");
> --
> 1.6.3.3

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Mon, Nov 30, 2009 at 12:45:20PM -0600, Madhusudhan wrote:
[...]
> > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +		/* charging must be active for meaningful result */
> > +		if (!is_charging) {
> 
> How about putting a kern_info here?

It might be better to return -EINVAL.

Thanks!

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Mon, Nov 30, 2009 at 8:58 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Mon, Nov 30, 2009 at 12:45:20PM -0600, Madhusudhan wrote:
> [...]
>> > +   case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> > +           /* charging must be active for meaningful result */
>> > +           if (!is_charging) {
>>
>> How about putting a kern_info here?
>
> It might be better to return -EINVAL.

That causes lots of warnings from power_supply core (driver failed to
report XXX property), Not sure what to do here, I'd prefer to keep
returning 0.

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Wed, Dec 02, 2009 at 10:38:31PM +0200, Grazvydas Ignotas wrote:
> On Mon, Nov 30, 2009 at 8:58 PM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Mon, Nov 30, 2009 at 12:45:20PM -0600, Madhusudhan wrote:
> > [...]
> >> > +   case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> >> > +           /* charging must be active for meaningful result */
> >> > +           if (!is_charging) {
> >>
> >> How about putting a kern_info here?
> >
> > It might be better to return -EINVAL.
> 
> That causes lots of warnings from power_supply core (driver failed to
> report XXX property), Not sure what to do here, I'd prefer to keep
> returning 0.

Lying to userspace is a bad idea.

How about this patch + changing the driver to return -ENODATA?


>From 0fe4c834b551c4d4454d57acaf75645675d199ee Mon Sep 17 00:00:00 2001
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Thu, 3 Dec 2009 00:24:51 +0300
Subject: [PATCH] power_supply_sysfs: Handle -ENODATA in a special way

There are cases when some device can not report any meaningful value,
e.g. TWL4030 charger can report voltage only when charging is
active.

In these cases drivers will return -ENODATA, and we shouldn't flood
kernel log with error messages.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/power/power_supply_sysfs.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 0814439..c790e0c 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -65,7 +65,10 @@ static ssize_t power_supply_show_property(struct device *dev,
 	ret = psy->get_property(psy, off, &value);
 
 	if (ret < 0) {
-		if (ret != -ENODEV)
+		if (ret == -ENODATA)
+			dev_dbg(dev, "driver has no data for `%s' property\n",
+				attr->attr.name);
+		else if (ret != -ENODEV)
 			dev_err(dev, "driver failed to report `%s' property\n",
 				attr->attr.name);
 		return ret;
-- 
1.6.3.3

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Wed, Dec 2, 2009 at 11:27 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Dec 02, 2009 at 10:38:31PM +0200, Grazvydas Ignotas wrote:
>> On Mon, Nov 30, 2009 at 8:58 PM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > On Mon, Nov 30, 2009 at 12:45:20PM -0600, Madhusudhan wrote:
>> > [...]
>> >> > +   case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> >> > +           /* charging must be active for meaningful result */
>> >> > +           if (!is_charging) {
>> >>
>> >> How about putting a kern_info here?
>> >
>> > It might be better to return -EINVAL.
>>
>> That causes lots of warnings from power_supply core (driver failed to
>> report XXX property), Not sure what to do here, I'd prefer to keep
>> returning 0.
>
> Lying to userspace is a bad idea.
>
> How about this patch + changing the driver to return -ENODATA?

This is fine for me, thanks.

>
> From 0fe4c834b551c4d4454d57acaf75645675d199ee Mon Sep 17 00:00:00 2001
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Date: Thu, 3 Dec 2009 00:24:51 +0300
> Subject: [PATCH] power_supply_sysfs: Handle -ENODATA in a special way
>
> There are cases when some device can not report any meaningful value,
> e.g. TWL4030 charger can report voltage only when charging is
> active.
>
> In these cases drivers will return -ENODATA, and we shouldn't flood
> kernel log with error messages.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  drivers/power/power_supply_sysfs.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 0814439..c790e0c 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -65,7 +65,10 @@ static ssize_t power_supply_show_property(struct device *dev,
>        ret = psy->get_property(psy, off, &value);
>
>        if (ret < 0) {
> -               if (ret != -ENODEV)
> +               if (ret == -ENODATA)
> +                       dev_dbg(dev, "driver has no data for `%s' property\n",
> +                               attr->attr.name);
> +               else if (ret != -ENODEV)
>                        dev_err(dev, "driver failed to report `%s' property\n",
>                                attr->attr.name);
>                return ret;
> --
> 1.6.3.3
>
>

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Mon, Nov 30, 2009 at 8:45 PM, Madhusudhan <madhu.cr@ti.com> wrote:
>
>
>> -----Original Message-----
>> From: Grazvydas Ignotas [mailto:notasas@gmail.com]
>> Sent: Friday, November 27, 2009 8:44 AM
>> To: linux-kernel@vger.kernel.org
>> Cc: Anton Vorontsov; Madhusudhan Chikkature; linux-omap@vger.kernel.org;
>> Grazvydas Ignotas
>> Subject: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger
>>
>> TWL4030/TPS65950 is a multi-function device with integrated charger,
>> which allows charging from AC or USB. This driver enables the
>> charger and provides several monitoring functions.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> ---
>> For this driver to work, TWL4030-core needs to be patched to use
>> correct macros so that it registers twl4030_bci platform_device.
>> I'll send patches for this later.
>>
>>  drivers/power/Kconfig           |    7 +
>>  drivers/power/Makefile          |    1 +
>>  drivers/power/twl4030_charger.c |  499
>
> Is the file name changed from twl4030_bci_battery.c to twl4030_charger.c because it mainly supports voltage monitoring only while charging? If yes, potentially we can add support for monitoring also in discharge state. Do we intend to change the file name then?

Does the hardware support any monitoring in discharge state? I'm
unable to get any readings, only frozen values (that never update)
from what it had when it was charging. Here is TI confirmation that at
least temperature monitoring won't work while discharging:
http://e2e.ti.com/forums/p/8202/31818.aspx#31818

For this reason I consider BCI a charger.

> Also adding the tested-on info could be helpful here.

ok

<snip>

>> +     case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +             /* charging must be active for meaningful result */
>> +             if (!is_charging) {
>
> How about putting a kern_info here?

That would potentially flood dmesg, will just return -EINVAL like
Anton suggests.

>> +                     val->intval = 0;
>> +                     break;
>> +             }
>> +             ret = twl4030_get_voltage(voltage_reg);
>> +             if (ret < 0)
>> +                     return ret;
>> +             val->intval = ret;
>> +             break;
>> +     case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +             if (!is_charging) {
>> +                     val->intval = 0;
> Ditto
>> +                     break;
>> +             }
>> +             /* current measurement is shared between AC and USB */
>> +             ret = twl4030_charger_get_current();
>> +             if (ret < 0)
>> +                     return ret;
>> +             val->intval = ret;
>> +             break;
>> +     case POWER_SUPPLY_PROP_ONLINE:
> Does this indicate the source of charging like USB or AC??

There are 2 charging devices registered now, AC and USB, each returns
it's state. This is what most other drivers do.

I'll send v2 later, it will also have more accurate voltage formulas I
got from TI.

RE: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Madhusudhan]

> -----Original Message-----
> From: Grazvydas Ignotas [mailto:notasas@gmail.com]
> Sent: Monday, November 30, 2009 3:33 PM
> To: Madhusudhan
> Cc: linux-kernel@vger.kernel.org; Anton Vorontsov; linux-
> omap@vger.kernel.org
> Subject: Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI
> charger
> 
> On Mon, Nov 30, 2009 at 8:45 PM, Madhusudhan <madhu.cr@ti.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Grazvydas Ignotas [mailto:notasas@gmail.com]
> >> Sent: Friday, November 27, 2009 8:44 AM
> >> To: linux-kernel@vger.kernel.org
> >> Cc: Anton Vorontsov; Madhusudhan Chikkature; linux-
> omap@vger.kernel.org;
> >> Grazvydas Ignotas
> >> Subject: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI
> charger
> >>
> >> TWL4030/TPS65950 is a multi-function device with integrated charger,
> >> which allows charging from AC or USB. This driver enables the
> >> charger and provides several monitoring functions.
> >>
> >> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> >> ---
> >> For this driver to work, TWL4030-core needs to be patched to use
> >> correct macros so that it registers twl4030_bci platform_device.
> >> I'll send patches for this later.
> >>
> >>  drivers/power/Kconfig           |    7 +
> >>  drivers/power/Makefile          |    1 +
> >>  drivers/power/twl4030_charger.c |  499
> >
> > Is the file name changed from twl4030_bci_battery.c to twl4030_charger.c
> because it mainly supports voltage monitoring only while charging? If yes,
> potentially we can add support for monitoring also in discharge state. Do
> we intend to change the file name then?
> 
> Does the hardware support any monitoring in discharge state? I'm
> unable to get any readings, only frozen values (that never update)
> from what it had when it was charging. Here is TI confirmation that at
> least temperature monitoring won't work while discharging:
> http://e2e.ti.com/forums/p/8202/31818.aspx#31818
> 
> For this reason I consider BCI a charger.
> 
In the discharge path BCI might not update the registers. It is worth
experiment to try and use MADC conversion to get the values. A driver for
madc is being currently discussed. See the patch:

http://patchwork.kernel.org/patch/62746/

We can try this once the madc driver is accepted in mainline and submit an
update patch to the BCI driver. As a first step I agree that the current BCI
patch should go upstream.

Reviewed-by: Madhusudhan Chikkature <madhu.cr@ti.com>

Thanks,
Madhu

> > Also adding the tested-on info could be helpful here.
> 
> ok
> 
> <snip>
> 
> >> +     case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> >> +             /* charging must be active for meaningful result */
> >> +             if (!is_charging) {
> >
> > How about putting a kern_info here?
> 
> That would potentially flood dmesg, will just return -EINVAL like
> Anton suggests.
> 
> >> +                     val->intval = 0;
> >> +                     break;
> >> +             }
> >> +             ret = twl4030_get_voltage(voltage_reg);
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +             val->intval = ret;
> >> +             break;
> >> +     case POWER_SUPPLY_PROP_CURRENT_NOW:
> >> +             if (!is_charging) {
> >> +                     val->intval = 0;
> > Ditto
> >> +                     break;
> >> +             }
> >> +             /* current measurement is shared between AC and USB */
> >> +             ret = twl4030_charger_get_current();
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +             val->intval = ret;
> >> +             break;
> >> +     case POWER_SUPPLY_PROP_ONLINE:
> > Does this indicate the source of charging like USB or AC??
> 
> There are 2 charging devices registered now, AC and USB, each returns
> it's state. This is what most other drivers do.
> 
> I'll send v2 later, it will also have more accurate voltage formulas I
> got from TI.

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Fri, Nov 27, 2009 at 03:44:20PM +0100, ext Grazvydas Ignotas wrote:
>diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>index b96f29d..9cea9b5 100644
>--- a/drivers/power/Makefile
>+++ b/drivers/power/Makefile
>@@ -29,3 +29,4 @@ obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o
> obj-$(CONFIG_BATTERY_DA9030)   += da9030_battery.o
> obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o
> obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
>+obj-$(CONFIG_CHARGER_TWL4030)  += twl4030_charger.o
>diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
>new file mode 100644
>index 0000000..604dd56
>--- /dev/null
>+++ b/drivers/power/twl4030_charger.c
>@@ -0,0 +1,499 @@
>+/*
>+ * TWL4030/TPS65950 BCI (Battery Charger Interface) driver
>+ *
>+ * Copyright (C) 2009 Gražvydas Ignotas <notasas@gmail.com>
>+ *
>+ * based on twl4030_bci_battery.c by TI
>+ * Copyright (C) 2008 Texas Instruments, Inc.
>+ *
>+ * 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.
>+ */
>+
>+#include <linux/init.h>
>+#include <linux/module.h>
>+#include <linux/platform_device.h>
>+#include <linux/interrupt.h>
>+#include <linux/i2c/twl4030.h>
>+#include <linux/power_supply.h>
>+
>+#define REG_BCIMSTATEC         0x02

please prepend these defines with e.g. TWL4030_BCI_

this would look like:

#define TWL4030_BCI_MSTATEC	0x02

>+#define REG_BCIICHG            0x08
>+#define REG_BCIVAC             0x0a
>+#define REG_BCIVBUS            0x0c
>+#define REG_BCIMFSTS4          0x10
>+#define REG_BCICTL1            0x23
>+
>+#define REG_BOOT_BCI           0x07
>+#define REG_STS_HW_CONDITIONS  0x0f
>+
>+#define BCIAUTOWEN             0x20
>+#define CONFIG_DONE            0x10
>+#define CVENAC                 0x04
>+#define BCIAUTOUSB             0x02
>+#define BCIAUTOAC              0x01
>+#define BCIMSTAT_MASK          0x3F
>+#define STS_VBUS               0x80
>+#define STS_CHG                        0x02
>+#define STS_USB_ID             0x04
>+#define CGAIN                  0x20
>+#define USBFASTMCHG            0x04
>+
>+#define STATEC_USB             0x10
>+#define STATEC_AC              0x20
>+#define STATEC_STATUS_MASK     0x0f
>+#define STATEC_QUICK1          0x02
>+#define STATEC_QUICK7          0x07
>+#define STATEC_COMPLETE1       0x0b
>+#define STATEC_COMPLETE4       0x0e
>+
>+#define BCI_DELAY              100

100ms ??? that's too quick for battery monitoring. Imagine that you'll 
have 10 i2c transfers per-second forever with this one. Don't you think 
you're waking up omap for nothing ??

something like 1 second when doing heavy operation should be enough and 
5 to 10 seconds in idle is good enough as well.

>+static struct twl4030_bci_device_info twl4030_bci = {

this should be allocated on probe() time.

>+/*
>+ * called by TWL4030 USB transceiver driver on USB_PRES interrupt.
>+ */
>+int twl4030charger_usb_en(int enable)
>+{
>+       if (twl4030_bci.started)
>+               schedule_delayed_work(&twl4030_bci.bat_work,
>+                       msecs_to_jiffies(BCI_DELAY));

I don't like the way you did this. I would expect twl4030-usb to kick 
the charger detection based on the VBUS irq. You have to consider the 
possibility of boards which won't use BCI module and will have some 
bq24xxx chip dealing with that like RX51. So instead of implementing 
this here and forcing people to have this driver enabled on e.g. RX51, 
you should implement the charger_enable_usb() logic in twl4030-usb 
itself. /me thinks

>+static int __devinit twl4030_bci_probe(struct platform_device *pdev)
>+{
>+       int irq;
>+       int ret;
>+
>+       twl4030_charger_enable_ac(true);
>+       twl4030_charger_enable_usb(true);
>+
>+       irq = platform_get_irq(pdev, 0);
>+
>+       /* CHG_PRES irq */
>+       ret = request_irq(irq, twl4030_charger_interrupt,
>+               0, pdev->name, &twl4030_bci);

how about using request_threaded_irq() ?? then you avoid having that 
workqueue.

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Wed, Dec 2, 2009 at 7:33 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi,
>
> On Fri, Nov 27, 2009 at 03:44:20PM +0100, ext Grazvydas Ignotas wrote:
>>
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index b96f29d..9cea9b5 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -29,3 +29,4 @@ obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o
>> obj-$(CONFIG_BATTERY_DA9030)   += da9030_battery.o
>> obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o
>> obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
>> +obj-$(CONFIG_CHARGER_TWL4030)  += twl4030_charger.o
>> diff --git a/drivers/power/twl4030_charger.c
>> b/drivers/power/twl4030_charger.c
>> new file mode 100644
>> index 0000000..604dd56
>> --- /dev/null
>> +++ b/drivers/power/twl4030_charger.c
>> @@ -0,0 +1,499 @@
>> +/*
>> + * TWL4030/TPS65950 BCI (Battery Charger Interface) driver
>> + *
>> + * Copyright (C) 2009 Gražvydas Ignotas <notasas@gmail.com>
>> + *
>> + * based on twl4030_bci_battery.c by TI
>> + * Copyright (C) 2008 Texas Instruments, Inc.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/i2c/twl4030.h>
>> +#include <linux/power_supply.h>
>> +
>> +#define REG_BCIMSTATEC         0x02
>
> please prepend these defines with e.g. TWL4030_BCI_
>
> this would look like:
>
> #define TWL4030_BCI_MSTATEC     0x02

ok

<snip>

>> +
>> +#define BCI_DELAY              100
>
> 100ms ??? that's too quick for battery monitoring. Imagine that you'll have
> 10 i2c transfers per-second forever with this one. Don't you think you're
> waking up omap for nothing ??

The work item doesn't queue itself, so this is only used once after
every interrupt. The delay itself is needed because charge state
machine needs some time to switch states and is not yet in expected
state at the time VBUS/AC detect interrupt kicks.

> something like 1 second when doing heavy operation should be enough and 5 to
> 10 seconds in idle is good enough as well.
>
>> +static struct twl4030_bci_device_info twl4030_bci = {
>
> this should be allocated on probe() time.

I need to access it from twl4030charger_usb_en().. Could only leave
delayed_work global and allocate everything else in probe() if you
prefer that.

>
>> +/*
>> + * called by TWL4030 USB transceiver driver on USB_PRES interrupt.
>> + */
>> +int twl4030charger_usb_en(int enable)
>> +{
>> +       if (twl4030_bci.started)
>> +               schedule_delayed_work(&twl4030_bci.bat_work,
>> +                       msecs_to_jiffies(BCI_DELAY));
>
> I don't like the way you did this. I would expect twl4030-usb to kick the
> charger detection based on the VBUS irq. You have to consider the
> possibility of boards which won't use BCI module and will have some bq24xxx
> chip dealing with that like RX51. So instead of implementing this here and
> forcing people to have this driver enabled on e.g. RX51, you should
> implement the charger_enable_usb() logic in twl4030-usb itself. /me thinks

I don't think charging is twl4030-usb's business, also notifying
power_supply core about charge state changes that I do here.

What about just returning early from twl4030charger_usb_en() if this
driver is not started? TWL4030-core requires twl4030_bci_platform_data
to be present to even register this driver, so it won't start on RX51.
RX51 can also choose not to compile this driver in, then
twl4030charger_usb_en() call won't even be done fom twl4030-usb.

>
>> +static int __devinit twl4030_bci_probe(struct platform_device *pdev)
>> +{
>> +       int irq;
>> +       int ret;
>> +
>> +       twl4030_charger_enable_ac(true);
>> +       twl4030_charger_enable_usb(true);
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +
>> +       /* CHG_PRES irq */
>> +       ret = request_irq(irq, twl4030_charger_interrupt,
>> +               0, pdev->name, &twl4030_bci);
>
> how about using request_threaded_irq() ?? then you avoid having that
> workqueue.

I need to do some delayed processing after VBUS/AC detect interrupts
kick, delayed_work looked perfect for this. Also note that I can't get
USB_PRES interrupt (taken by twl4030-usb), only a callback from
twl4030-usb.

>
> --
> balbi
>

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Wed, Dec 02, 2009 at 09:34:00PM +0100, ext Grazvydas Ignotas wrote:
>>> +#define BCI_DELAY              100
>>
>> 100ms ??? that's too quick for battery monitoring. Imagine that you'll have
>> 10 i2c transfers per-second forever with this one. Don't you think you're
>> waking up omap for nothing ??
>
>The work item doesn't queue itself, so this is only used once after
>every interrupt. The delay itself is needed because charge state
>machine needs some time to switch states and is not yet in expected
>state at the time VBUS/AC detect interrupt kicks.

ok got it... not so bad then ;-)

>>> +static struct twl4030_bci_device_info twl4030_bci = {
>>
>> this should be allocated on probe() time.
>
>I need to access it from twl4030charger_usb_en().. Could only leave
>delayed_work global and allocate everything else in probe() if you
>prefer that.

well, you could keep only a global static pointer and after allocating 
that in probe, make the global static pointer, point to it... Anyways, 
I think twl4030charger_usb_en() should change its prototype to something 
like

twl4030charger_usb_en(struct twl4030_bci *bci, int enable);

you could leave userland to decide whether to start charging, specially 
in usb charging case where we still need to know if we where enumerated 
with 100mA or 500mA configuration. How are you differing between those 
currently ?

>> I don't like the way you did this. I would expect twl4030-usb to kick the
>> charger detection based on the VBUS irq. You have to consider the
>> possibility of boards which won't use BCI module and will have some bq24xxx
>> chip dealing with that like RX51. So instead of implementing this here and
>> forcing people to have this driver enabled on e.g. RX51, you should
>> implement the charger_enable_usb() logic in twl4030-usb itself. /me thinks
>
>I don't think charging is twl4030-usb's business, also notifying
>power_supply core about charge state changes that I do here.

I was talking about the charger detection. The start of charge you could 
leave to userland to handle, no ?

>What about just returning early from twl4030charger_usb_en() if this
>driver is not started? TWL4030-core requires twl4030_bci_platform_data
>to be present to even register this driver, so it won't start on RX51.
>RX51 can also choose not to compile this driver in, then
>twl4030charger_usb_en() call won't even be done fom twl4030-usb.

still we need to detect the charger...

>> how about using request_threaded_irq() ?? then you avoid having that
>> workqueue.
>
>I need to do some delayed processing after VBUS/AC detect interrupts
>kick, delayed_work looked perfect for this. Also note that I can't get
>USB_PRES interrupt (taken by twl4030-usb), only a callback from
>twl4030-usb.

or you let userland to handle a bit more of this logic (??)

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Wed, Dec 2, 2009 at 10:49 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi,
>
> On Wed, Dec 02, 2009 at 09:34:00PM +0100, ext Grazvydas Ignotas wrote:
>>>>
>>>> +#define BCI_DELAY              100
>>>
>>> 100ms ??? that's too quick for battery monitoring. Imagine that you'll
>>> have
>>> 10 i2c transfers per-second forever with this one. Don't you think you're
>>> waking up omap for nothing ??
>>
>> The work item doesn't queue itself, so this is only used once after
>> every interrupt. The delay itself is needed because charge state
>> machine needs some time to switch states and is not yet in expected
>> state at the time VBUS/AC detect interrupt kicks.
>
> ok got it... not so bad then ;-)
>
>>>> +static struct twl4030_bci_device_info twl4030_bci = {
>>>
>>> this should be allocated on probe() time.
>>
>> I need to access it from twl4030charger_usb_en().. Could only leave
>> delayed_work global and allocate everything else in probe() if you
>> prefer that.
>
> well, you could keep only a global static pointer and after allocating that
> in probe, make the global static pointer, point to it... Anyways, I think
> twl4030charger_usb_en() should change its prototype to something like
>
> twl4030charger_usb_en(struct twl4030_bci *bci, int enable);

This function is called by twl4030-usb, where will it get this bci pointer from?

> you could leave userland to decide whether to start charging, specially in
> usb charging case where we still need to know if we where enumerated with
> 100mA or 500mA configuration.

>From what I saw in other drivers they are setup to start charging
automatically as soon as they see VBUS. Maybe Anton can give more
details here.

> How are you differing between those currently?

Not handled at all at the moment (uses BCI defaults).

>>> I don't like the way you did this. I would expect twl4030-usb to kick the
>>> charger detection based on the VBUS irq. You have to consider the
>>> possibility of boards which won't use BCI module and will have some
>>> bq24xxx
>>> chip dealing with that like RX51. So instead of implementing this here
>>> and
>>> forcing people to have this driver enabled on e.g. RX51, you should
>>> implement the charger_enable_usb() logic in twl4030-usb itself. /me
>>> thinks
>>
>> I don't think charging is twl4030-usb's business, also notifying
>> power_supply core about charge state changes that I do here.
>
> I was talking about the charger detection.

The USB_PRES interrupt belongs to twl4030-usb, so it gets it's
detection as before and does not need this driver, where is the
problem? This driver only registers CHG_PRES, which is AC charger
detect interrupt.

> The start of charge you could leave to userland to handle, no ?

Maybe, I wonder if there is standard way to do that.
Anton?

>
>> What about just returning early from twl4030charger_usb_en() if this
>> driver is not started? TWL4030-core requires twl4030_bci_platform_data
>> to be present to even register this driver, so it won't start on RX51.
>> RX51 can also choose not to compile this driver in, then
>> twl4030charger_usb_en() call won't even be done fom twl4030-usb.
>
> still we need to detect the charger...
>
>>> how about using request_threaded_irq() ?? then you avoid having that
>>> workqueue.
>>
>> I need to do some delayed processing after VBUS/AC detect interrupts
>> kick, delayed_work looked perfect for this. Also note that I can't get
>> USB_PRES interrupt (taken by twl4030-usb), only a callback from
>> twl4030-usb.
>
> or you let userland to handle a bit more of this logic (??)

Still need to autostart AC charging, who would plug AC adapter and
want to fiddle with the GUI to start charging instead it having it go
automatically? Agree about USB userspace switch though..

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Wed, Dec 02, 2009 at 11:29:10PM +0200, Grazvydas Ignotas wrote:
[...]
> From what I saw in other drivers they are setup to start charging
> automatically as soon as they see VBUS.

Yes. I see nothing wrong here.

> > How are you differing between those currently?
> 
> Not handled at all at the moment (uses BCI defaults).
[...]
> Agree about USB userspace switch though..

Yes, so far we don't have any writable properties in power supply
class, though the feature is in demand by various drivers.
Somebody should step up and implement it. ;-)

But for the start, I like this driver the way it is.

As for the default USB VBUS current value, it could be Kconfig
option (something alike to USB_GADGET_VBUS_DRAW) and/or module
parameter, or hw default, or hardcoded for now. Either will
work.

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Wed, Dec 02, 2009 at 10:54:42PM +0100, ext Anton Vorontsov wrote:
>As for the default USB VBUS current value, it could be Kconfig
>option (something alike to USB_GADGET_VBUS_DRAW) and/or module
>parameter, or hw default, or hardcoded for now. Either will
>work.

cannot be Kconfig, it's mandated by usb battery charging spec 1.x to be 
100mA for 100ms, then if you don't enumerate, you have to cut charging.

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

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Thu, Dec 03, 2009 at 12:31:56AM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 02, 2009 at 10:54:42PM +0100, ext Anton Vorontsov wrote:
> >As for the default USB VBUS current value, it could be Kconfig
> >option (something alike to USB_GADGET_VBUS_DRAW) and/or module
> >parameter, or hw default, or hardcoded for now. Either will
> >work.
> 
> cannot be Kconfig, it's mandated by usb battery charging spec 1.x to
> be 100mA for 100ms, then if you don't enumerate, you have to cut
> charging.

Oh, I thought TWL4030 does the USB stuff somewhat transparently
so the checks in twl4030_charger_check_vbus() would be enough.
Is there any TWL4030 reference manual available?

If TWL4030 just draws the VBUS directly, then it might be a good
idea to integrate the driver with OTG framework, as an example
see

commit 5bf2b994bfe11bfe86231050897b2d881ca544d9
Author: Philipp Zabel <philipp.zabel@gmail.com>
Date:   Sun Jan 18 17:40:27 2009 +0100

    pda_power: Add optional OTG transceiver and voltage regulator support

Though, instead of just a boolean is_usb_online() stuff, you'll
have to get the allowed current draw value and configure the
charger appropriately.

Will this work?

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Wed, Dec 02, 2009 at 11:59:22PM +0100, ext Anton Vorontsov wrote:
>On Thu, Dec 03, 2009 at 12:31:56AM +0200, Felipe Balbi wrote:
>> Hi,
>>
>> On Wed, Dec 02, 2009 at 10:54:42PM +0100, ext Anton Vorontsov wrote:
>> >As for the default USB VBUS current value, it could be Kconfig
>> >option (something alike to USB_GADGET_VBUS_DRAW) and/or module
>> >parameter, or hw default, or hardcoded for now. Either will
>> >work.
>>
>> cannot be Kconfig, it's mandated by usb battery charging spec 1.x to
>> be 100mA for 100ms, then if you don't enumerate, you have to cut
>> charging.
>
>Oh, I thought TWL4030 does the USB stuff somewhat transparently
>so the checks in twl4030_charger_check_vbus() would be enough.
>Is there any TWL4030 reference manual available?

there should be some tpsxxxxx manual available, someone from TI should 
be able to tell us ??

>If TWL4030 just draws the VBUS directly, then it might be a good
>idea to integrate the driver with OTG framework, as an example
>see

thing is that we already have the transceiver driver done... so we have 
to think how to do this properly.

>commit 5bf2b994bfe11bfe86231050897b2d881ca544d9
>Author: Philipp Zabel <philipp.zabel@gmail.com>
>Date:   Sun Jan 18 17:40:27 2009 +0100
>
>    pda_power: Add optional OTG transceiver and voltage regulator support
>
>Though, instead of just a boolean is_usb_online() stuff, you'll
>have to get the allowed current draw value and configure the
>charger appropriately.
>
>Will this work?

yes, that'll work. But how about start charging always with 100mA and if 
userland sees that we were enumerated it reconfigures charging as 
necessary. But this would mean that we have the EM daemon started up 
just after the driver itself is loaded to avoid the problem with 100ms 
no enumeration. How does that sound ? Do we start making some writeable 
power_supply sysfs ???

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Thu, Dec 3, 2009 at 10:39 AM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi,
>
> On Wed, Dec 02, 2009 at 11:59:22PM +0100, ext Anton Vorontsov wrote:
>>
>> On Thu, Dec 03, 2009 at 12:31:56AM +0200, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> On Wed, Dec 02, 2009 at 10:54:42PM +0100, ext Anton Vorontsov wrote:
>>> >As for the default USB VBUS current value, it could be Kconfig
>>> >option (something alike to USB_GADGET_VBUS_DRAW) and/or module
>>> >parameter, or hw default, or hardcoded for now. Either will
>>> >work.
>>>
>>> cannot be Kconfig, it's mandated by usb battery charging spec 1.x to
>>> be 100mA for 100ms, then if you don't enumerate, you have to cut
>>> charging.
>>
>> Oh, I thought TWL4030 does the USB stuff somewhat transparently
>> so the checks in twl4030_charger_check_vbus() would be enough.
>> Is there any TWL4030 reference manual available?
>
> there should be some tpsxxxxx manual available, someone from TI should be
> able to tell us ??

TPS65950 is catalog part of TWL4030 and has documentation here:
http://focus.ti.com/docs/prod/folders/print/tps65950.html#technicaldocuments

It says that it is software's responsibility to detect the device and
set the right charge mode/current..

>
>> If TWL4030 just draws the VBUS directly, then it might be a good
>> idea to integrate the driver with OTG framework, as an example
>> see
>
> thing is that we already have the transceiver driver done... so we have to
> think how to do this properly.
>
>> commit 5bf2b994bfe11bfe86231050897b2d881ca544d9
>> Author: Philipp Zabel <philipp.zabel@gmail.com>
>> Date:   Sun Jan 18 17:40:27 2009 +0100
>>
>>   pda_power: Add optional OTG transceiver and voltage regulator support
>>
>> Though, instead of just a boolean is_usb_online() stuff, you'll
>> have to get the allowed current draw value and configure the
>> charger appropriately.
>>
>> Will this work?
>
> yes, that'll work. But how about start charging always with 100mA and if
> userland sees that we were enumerated it reconfigures charging as necessary.
> But this would mean that we have the EM daemon started up just after the
> driver itself is loaded to avoid the problem with 100ms no enumeration. How
> does that sound ? Do we start making some writeable power_supply sysfs ???

There are also USB chargers that don't enumerate and have D+ and D-
shorted with a resistor (see "dedicated charger port" in the charging
spec), how do we support those?

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Thu, Dec 03, 2009 at 11:55:12AM +0100, ext Grazvydas Ignotas wrote:
>TPS65950 is catalog part of TWL4030 and has documentation here:
>http://focus.ti.com/docs/prod/folders/print/tps65950.html#technicaldocuments
>
>It says that it is software's responsibility to detect the device and
>set the right charge mode/current..

yes, the BCI (or bq24xxx) will never be able to know which configuration 
we were enumerated with...

>> yes, that'll work. But how about start charging always with 100mA and if
>> userland sees that we were enumerated it reconfigures charging as necessary.
>> But this would mean that we have the EM daemon started up just after the
>> driver itself is loaded to avoid the problem with 100ms no enumeration. How
>> does that sound ? Do we start making some writeable power_supply sysfs ???
>
>There are also USB chargers that don't enumerate and have D+ and D-
>shorted with a resistor (see "dedicated charger port" in the charging
>spec), how do we support those?

dedicated chargers are simple. You kick the charger detection according 
to USB BC 1.x and if it returns true, you configure high current 
charging. Host/Hub chargers are also simple, after kicking charger 
detection, you enable Data pullups (e.g. SOFTCONN bit in musb's power 
register) and see if the host sends a setup packet...

the complicated part is passing the information of which configuration 
you were enumerated with to the charger chip.

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Thu, Dec 3, 2009 at 1:03 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi,
>
> On Thu, Dec 03, 2009 at 11:55:12AM +0100, ext Grazvydas Ignotas wrote:
>>
>> TPS65950 is catalog part of TWL4030 and has documentation here:
>>
>> http://focus.ti.com/docs/prod/folders/print/tps65950.html#technicaldocuments
>>
>> It says that it is software's responsibility to detect the device and
>> set the right charge mode/current..
>
> yes, the BCI (or bq24xxx) will never be able to know which configuration we
> were enumerated with...
>
>>> yes, that'll work. But how about start charging always with 100mA and if
>>> userland sees that we were enumerated it reconfigures charging as
>>> necessary.
>>> But this would mean that we have the EM daemon started up just after the
>>> driver itself is loaded to avoid the problem with 100ms no enumeration.
>>> How
>>> does that sound ? Do we start making some writeable power_supply sysfs
>>> ???
>>
>> There are also USB chargers that don't enumerate and have D+ and D-
>> shorted with a resistor (see "dedicated charger port" in the charging
>> spec), how do we support those?
>
> dedicated chargers are simple. You kick the charger detection according to
> USB BC 1.x and if it returns true, you configure high current charging.
> Host/Hub chargers are also simple, after kicking charger detection, you
> enable Data pullups (e.g. SOFTCONN bit in musb's power register) and see if
> the host sends a setup packet...
>
> the complicated part is passing the information of which configuration you
> were enumerated with to the charger chip.

Ok since it doesn't look like this will resolve soon, what about
adding some DEVICE_ATTR for the time being and requiring userspace to
write charge current here to start actual charging?

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Thu, Dec 10, 2009 at 04:09:08PM +0200, Grazvydas Ignotas wrote:
[...]
> > dedicated chargers are simple. You kick the charger detection according to
> > USB BC 1.x and if it returns true, you configure high current charging.
> > Host/Hub chargers are also simple, after kicking charger detection, you
> > enable Data pullups (e.g. SOFTCONN bit in musb's power register) and see if
> > the host sends a setup packet...
> >
> > the complicated part is passing the information of which configuration you
> > were enumerated with to the charger chip.
> 
> Ok since it doesn't look like this will resolve soon, what about
> adding some DEVICE_ATTR for the time being and requiring userspace to
> write charge current here to start actual charging?

Works for me. Let's think of the kernel charging support as an
yet unimplemented feature.

Thanks,

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Thu, Dec 10, 2009 at 03:18:30PM +0100, ext Anton Vorontsov wrote:
>> Ok since it doesn't look like this will resolve soon, what about
>> adding some DEVICE_ATTR for the time being and requiring userspace to
>> write charge current here to start actual charging?
>
>Works for me. Let's think of the kernel charging support as an
>yet unimplemented feature.

but if you guys are ok with this one for now. It can always be changed 
later :-)

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Anton Vorontsov]

On Thu, Dec 10, 2009 at 04:21:27PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Dec 10, 2009 at 03:18:30PM +0100, ext Anton Vorontsov wrote:
> >>Ok since it doesn't look like this will resolve soon, what about
> >>adding some DEVICE_ATTR for the time being and requiring userspace to
> >>write charge current here to start actual charging?
> >
> >Works for me. Let's think of the kernel charging support as an
> >yet unimplemented feature.
> 
> but if you guys are ok with this one for now. It can always be
> changed later :-)

Yep. The only thing I'm afraid of is that once the driver is in,
then nobody will bother with improving it to do the right thing.
But an imperfect driver is better than none.

Thanks,

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Thu, Dec 10, 2009 at 03:44:11PM +0100, ext Anton Vorontsov wrote:
>Yep. The only thing I'm afraid of is that once the driver is in,
>then nobody will bother with improving it to do the right thing.
>But an imperfect driver is better than none.

I'm currently implementing the notifier part in a generic manner that 
would handle all transceivers (well, they would have to provide a some 
implementations for function pointers in struct otg_transceiver).

Then drivers like twl4030-bci and musb_hdrc could register for 
notifications from the transceiver. I was thinking of notifying the 
following events:

enum usb_xceiv_events {
	USB_EVENT_NONE,		/* no events or cable disconnected */
	USB_EVENT_VBUS,		/* vbus valid event */
	USB_EVENT_ID,		/* id was grounded */
	USB_EVENT_CHARGER,	/* usb dedicated charger */
	USB_EVENT_ENUMERATED,	/* gadget driver enumerated */
};

for the USB_EVENT_ENUMERATED we could also pass the bMaxPower (in mA) 
field of the struct usb_configuration, then BCI (or bq24xxx for that 
matter) can configure that as input current for charging.

USB_EVENT_ID is necessary because in most boards (at least the ones I've 
seen) the charging chip (bq24xxx) also plays the role as charge pump, 
sourcing vbus on OTG sessions.

USB_EVENT_VBUS and USB_EVENT_CHARGER aren't necessary on all cases, but 
boards like RX-51 where it has a different transceiver for charger 
detection, it's necessary to have those two separated.

If you guys have any comments already, please comment :-)

I'll try to finish a prototype by tomorrow and try to send it as RFC.
Will be sure to Cc you Anton.

-- 
balbi

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Grazvydas Ignotas]

On Thu, Dec 10, 2009 at 6:51 PM, Felipe Balbi <felipe.balbi@nokia.com> wrote:
> Hi,
>
> On Thu, Dec 10, 2009 at 03:44:11PM +0100, ext Anton Vorontsov wrote:
>>
>> Yep. The only thing I'm afraid of is that once the driver is in,
>> then nobody will bother with improving it to do the right thing.
>> But an imperfect driver is better than none.
>
> I'm currently implementing the notifier part in a generic manner that would
> handle all transceivers (well, they would have to provide a some
> implementations for function pointers in struct otg_transceiver).
>
> Then drivers like twl4030-bci and musb_hdrc could register for notifications
> from the transceiver. I was thinking of notifying the following events:
>
> enum usb_xceiv_events {
>        USB_EVENT_NONE,         /* no events or cable disconnected */
>        USB_EVENT_VBUS,         /* vbus valid event */
>        USB_EVENT_ID,           /* id was grounded */
>        USB_EVENT_CHARGER,      /* usb dedicated charger */
>        USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
> };
>
> for the USB_EVENT_ENUMERATED we could also pass the bMaxPower (in mA) field
> of the struct usb_configuration, then BCI (or bq24xxx for that matter) can
> configure that as input current for charging.
>
> USB_EVENT_ID is necessary because in most boards (at least the ones I've
> seen) the charging chip (bq24xxx) also plays the role as charge pump,
> sourcing vbus on OTG sessions.
>
> USB_EVENT_VBUS and USB_EVENT_CHARGER aren't necessary on all cases, but
> boards like RX-51 where it has a different transceiver for charger
> detection, it's necessary to have those two separated.
>
> If you guys have any comments already, please comment :-)
>
> I'll try to finish a prototype by tomorrow and try to send it as RFC.

Nice! Looking good, it seems this has everything BCI needs, will wait
for your code before updating BCI.

[RFC/PATCH 0/5] usb transceiver notifier

[Felipe Balbi]

The following patches are a prototype for supporting
notification of some usb events to whoever is interested
in them.

This way we can communicate common usb events to several
interested drivers so they can e.g. kick usb charger detection
and configure charging with proper input current.

THEY ARE NOT READY AND ARE NOT SUPPOSED TO BE APPLIED TO ANY
BRANCH.

please comment.

ps: patches were compile tested only.

Felipe Balbi (5):
  usb: otg: add notifier support
  usb: otg: twl4030: add support for notifier
  usb: musb: add support for ulpi block
  usb: musb: isp1704: add registers from isp1704
  usb: musb: musb supports otg notifier

 drivers/usb/musb/isp1704.h    |   81 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_core.c  |   72 ++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_core.h  |    5 +++
 drivers/usb/musb/musb_regs.h  |   62 +++++++++++++++++++++++++++++++
 drivers/usb/otg/twl4030-usb.c |    7 +++-
 include/linux/usb/otg.h       |   25 +++++++++++++
 6 files changed, 250 insertions(+), 2 deletions(-)
 create mode 100644 drivers/usb/musb/isp1704.h

[RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

The notifier will be used to communicate usb events
to other drivers like the charger chip.

This can be used as source of information to kick
usb charger detection as described by the USB
Battery Charging Specification 1.1 and/or to
pass bMaxPower field of selected usb_configuration
to charger chip in order to use that information
as input current on the charging profile
setup.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 include/linux/usb/otg.h |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index 52bb917..6c0b676 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -9,6 +9,8 @@
 #ifndef __LINUX_USB_OTG_H
 #define __LINUX_USB_OTG_H
 
+#include <linux/notifier.h>
+
 /* OTG defines lots of enumeration states before device reset */
 enum usb_otg_state {
 	OTG_STATE_UNDEFINED = 0,
@@ -33,6 +35,14 @@ enum usb_otg_state {
 	OTG_STATE_A_VBUS_ERR,
 };
 
+enum usb_xceiv_events {
+	USB_EVENT_NONE,         /* no events or cable disconnected */
+	USB_EVENT_VBUS,         /* vbus valid event */
+	USB_EVENT_ID,           /* id was grounded */
+	USB_EVENT_CHARGER,      /* usb dedicated charger */
+	USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
+};
+
 #define USB_OTG_PULLUP_ID		(1 << 0)
 #define USB_OTG_PULLDOWN_DP		(1 << 1)
 #define USB_OTG_PULLDOWN_DM		(1 << 2)
@@ -70,6 +80,9 @@ struct otg_transceiver {
 	struct otg_io_access_ops	*io_ops;
 	void __iomem			*io_priv;
 
+	/* for notification of usb_xceiv_events */
+	struct blocking_notifier_head	notifier;
+
 	/* to pass extra port status to the root hub */
 	u16			port_status;
 	u16			port_change;
@@ -203,6 +216,18 @@ otg_start_srp(struct otg_transceiver *otg)
 	return otg->start_srp(otg);
 }
 
+/* notifiers */
+static inline int
+otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&otg->notifier, nb);
+}
+
+static inline void
+otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&otg->notifier, nb);
+}
 
 /* for OTG controller drivers (and maybe other stuff) */
 extern int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num);
-- 
1.6.6.rc0

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Mark Brown]

On Fri, Dec 11, 2009 at 01:31:22PM +0200, Felipe Balbi wrote:
> The notifier will be used to communicate usb events
> to other drivers like the charger chip.

> This can be used as source of information to kick
> usb charger detection as described by the USB
> Battery Charging Specification 1.1 and/or to
> pass bMaxPower field of selected usb_configuration
> to charger chip in order to use that information
> as input current on the charging profile
> setup.

This one looks reasonable to me, though I've not thought it through
properly yet.  Could you please add me to the CC list for future
revisions of this - I've got some charger drivers that could use this?

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

On Fri, Dec 11, 2009 at 12:55:38PM +0100, ext Mark Brown wrote:
>On Fri, Dec 11, 2009 at 01:31:22PM +0200, Felipe Balbi wrote:
>> The notifier will be used to communicate usb events
>> to other drivers like the charger chip.
>
>> This can be used as source of information to kick
>> usb charger detection as described by the USB
>> Battery Charging Specification 1.1 and/or to
>> pass bMaxPower field of selected usb_configuration
>> to charger chip in order to use that information
>> as input current on the charging profile
>> setup.
>
>This one looks reasonable to me, though I've not thought it through
>properly yet.  Could you please add me to the CC list for future
>revisions of this - I've got some charger drivers that could use this?

Sure I'll do it. I want to start the discussion on this, so if you have 
any comments, please, do comment.

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Friday 11 December 2009, Felipe Balbi wrote:
> The notifier will be used to communicate usb events
> to other drivers like the charger chip.

Good idea ... but not OTG-specific.  It doesn't seem to me
that charging hookups belong in that header at all.

In fact, usb_gadget_vbus_draw() might better be implemented
as such a notifier call, removing that configuration mess
from the usb gadget drivers ("how can I know what charger
to use??").  That would be the most common situation:  a
peripheral-only device.

And in fact, OTG can be treated as a trivial superset of
peripheral-only USB.  (In terms of charger support, only!!)

I'd vote to convert all the USB-to-charger interfaces so
they use notifiers.  After fixing the events ... see
comments below.  :)


> This can be used as source of information to kick
> usb charger detection as described by the USB
> Battery Charging Specification 1.1 and/or to
> pass bMaxPower field of selected usb_configuration
> to charger chip in order to use that information
> as input current on the charging profile
> setup.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  include/linux/usb/otg.h |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> index 52bb917..6c0b676 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -9,6 +9,8 @@
>  #ifndef __LINUX_USB_OTG_H
>  #define __LINUX_USB_OTG_H
>  
> +#include <linux/notifier.h>
> +
>  /* OTG defines lots of enumeration states before device reset */
>  enum usb_otg_state {
>  	OTG_STATE_UNDEFINED = 0,
> @@ -33,6 +35,14 @@ enum usb_otg_state {
>  	OTG_STATE_A_VBUS_ERR,
>  };
>  
> +enum usb_xceiv_events {

Let's keep charger events separate from anything else,
like "enter host mode" or "enter peripheral mode" (or
even "disconnect").  The audiences for any other types
of event would be entirely different.

Right now there's a mess in terms of charger hookup,
so getting that straight is IMO a priority over any
other type of event.  Using events will decouple a
bunch of drivers, and simplify driver configuration.


> +	USB_EVENT_NONE,         /* no events or cable disconnected */
> +	USB_EVENT_VBUS,         /* vbus valid event */
> +	USB_EVENT_ID,           /* id was grounded */
> +	USB_EVENT_CHARGER,      /* usb dedicated charger */
> +	USB_EVENT_ENUMERATED,   /* gadget driver enumerated */

Those seem like the wrong events.  The right events for a charger
would be more along the lines of:

 - For peripheral:  "you may use N milliAmperes now".
 - General:  "Don't Charge" (a.k.a. "use 0 mA").

I don't see how "N" would be passed with those events ...

Haven't looked at the details of the charger spec, but
those two events are the *basics* from the USB 2.0 spec,
so "official" charger hardware wouldn't be less capable.

Thing like different levels of VBUS validity, ID grounding,
and so forth ... wouldn't be very relevant.  An OTG driver
will do various things, internally, when ID grounds; but
anything else is a function of what role eventually gets
negotiated.  And for the charger, they all add up to "Don't
Charge" (since ID grounded means A-role, sourcing power).

A host *might* want to be able to say things like "supply
up to N milliAmperes now", e.g. to let a regulator choose
the most efficient mode.


> +};
> +
>  #define USB_OTG_PULLUP_ID		(1 << 0)
>  #define USB_OTG_PULLDOWN_DP		(1 << 1)
>  #define USB_OTG_PULLDOWN_DM		(1 << 2)
> @@ -70,6 +80,9 @@ struct otg_transceiver {
>  	struct otg_io_access_ops	*io_ops;
>  	void __iomem			*io_priv;
>  
> +	/* for notification of usb_xceiv_events */
> +	struct blocking_notifier_head	notifier;

Why "blocking"?  That seems kind of unnatural; for example,
the main users -- like usb_gadget_vbus_draw() -- would be
called in IRQ context (blocking not allowed).

> +
>  	/* to pass extra port status to the root hub */
>  	u16			port_status;
>  	u16			port_change;
> @@ -203,6 +216,18 @@ otg_start_srp(struct otg_transceiver *otg)
>  	return otg->start_srp(otg);
>  }
>  
> +/* notifiers */
> +static inline int
> +otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&otg->notifier, nb);
> +}
> +
> +static inline void
> +otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&otg->notifier, nb);
> +}
>  
>  /* for OTG controller drivers (and maybe other stuff) */
>  extern int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num);
> 

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Mark Brown]

On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:

> I'd vote to convert all the USB-to-charger interfaces so
> they use notifiers.  After fixing the events ... see
> comments below.  :)

Yes please - it's not just chargers either, this can also be used by
PMICs which do power path management that includes USB.

> Those seem like the wrong events.  The right events for a charger
> would be more along the lines of:

>  - For peripheral:  "you may use N milliAmperes now".
>  - General:  "Don't Charge" (a.k.a. "use 0 mA").

> I don't see how "N" would be passed with those events ...

These are good for the peripheral side.  You do get to pass a void *
along with the notifier value, that could be used to pass data like the
current limit.

> A host *might* want to be able to say things like "supply
> up to N milliAmperes now", e.g. to let a regulator choose
> the most efficient mode.

Yes, they definitely want this - not just for efficiency but also to
allow current limiting to protect the system from excessive current
drain.

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Mark Brown wrote:
> On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:
> 
> > I'd vote to convert all the USB-to-charger interfaces so
> > they use notifiers.  After fixing the events ... see
> > comments below.  :)
> 
> Yes please - it's not just chargers either, this can also be used by
> PMICs which do power path management that includes USB.

Color me confused ... what do you mean by "power path"?

Do you mean something like "the board as a whole can take N mA of
current from USB", rather than specifically addressing a charger?

It's not uncommon to do things like use VBUS current to power the
USB circuitry, too.  That can leave less for other purposes.  All
of that being rather board-specific.


> > Those seem like the wrong events.  The right events for a charger
> > would be more along the lines of:
> 
> >  - For peripheral:  "you may use N milliAmperes now".
> >  - General:  "Don't Charge" (a.k.a. "use 0 mA").
> 
> > I don't see how "N" would be passed with those events ...
> 
> These are good for the peripheral side.  You do get to pass a void *
> along with the notifier value, that could be used to pass data like the
> current limit.

I don't think I saw that being done ... either in code, comments,
or documentation.  Passing N is fundamental.


> > A host *might* want to be able to say things like "supply
> > up to N milliAmperes now", e.g. to let a regulator choose
> > the most efficient mode.
> 
> Yes, they definitely want this - not just for efficiency but also to
> allow current limiting to protect the system from excessive current
> drain.

There are load bursting issues too.  All part of the USB spec;
a load that's OK for 1 millisecond might not be OK for 1 second.
ISTR the "supply N mA" refers to an average.  And there are some
limits to the capacitance that can practically be stuck on VBUS
output lines (which includes the cable).  Solvable problems, but
not always pretty if software has to think it through.

Thing is, supplying current is a bit more involved.  If the
board can't supply 300 mA, the USB configuration selection
mechanism has to know that, so it never selects peripheral
configurations which require that much current.

Or maybe these two ports can serve 500 mA, but not those two;
or their total can't exceed N (function of componentry and power
budgeting).

Ergo my desire to start with a straightforward problem whose
solution has real value (how much VBUS current may be consumed?),
and leave some of those other messes for later!

- Dave

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

On Tue, Jan 26, 2010 at 02:35:21PM +0100, ext David Brownell wrote:
>On Tuesday 26 January 2010, Mark Brown wrote:
>> On Tue, Jan 26, 2010 at 03:16:20AM -0800, David Brownell wrote:
>>
>> > I'd vote to convert all the USB-to-charger interfaces so
>> > they use notifiers.  After fixing the events ... see
>> > comments below.  :)
>>
>> Yes please - it's not just chargers either, this can also be used by
>> PMICs which do power path management that includes USB.
>
>Color me confused ... what do you mean by "power path"?
>
>Do you mean something like "the board as a whole can take N mA of
>current from USB", rather than specifically addressing a charger?
>
>It's not uncommon to do things like use VBUS current to power the
>USB circuitry, too.  That can leave less for other purposes.  All
>of that being rather board-specific.
>
>
>> > Those seem like the wrong events.  The right events for a charger
>> > would be more along the lines of:
>>
>> >  - For peripheral:  "you may use N milliAmperes now".
>> >  - General:  "Don't Charge" (a.k.a. "use 0 mA").
>>
>> > I don't see how "N" would be passed with those events ...
>>
>> These are good for the peripheral side.  You do get to pass a void *
>> along with the notifier value, that could be used to pass data like the
>> current limit.
>
>I don't think I saw that being done ... either in code, comments,
>or documentation.  Passing N is fundamental.

yeah, my bad. I should have said that, but it's more related to the 
implementation of the notifier_block.

>> > A host *might* want to be able to say things like "supply
>> > up to N milliAmperes now", e.g. to let a regulator choose
>> > the most efficient mode.
>>
>> Yes, they definitely want this - not just for efficiency but also to
>> allow current limiting to protect the system from excessive current
>> drain.
>
>There are load bursting issues too.  All part of the USB spec;
>a load that's OK for 1 millisecond might not be OK for 1 second.

if you get a SetConfiguration(config), then you can use that load for as 
long as needed, the limitation is when not enumerated, afaict.

>ISTR the "supply N mA" refers to an average.  And there are some
>limits to the capacitance that can practically be stuck on VBUS
>output lines (which includes the cable).  Solvable problems, but
>not always pretty if software has to think it through.
>
>Thing is, supplying current is a bit more involved.  If the
>board can't supply 300 mA, the USB configuration selection
>mechanism has to know that, so it never selects peripheral
>configurations which require that much current.

but that's done already by the usb core, no ? It rules out configuration 
based on the hub->power_budget (can't remember if the field is that 
exact name).

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Oliver Neukum]

Am Dienstag, 26. Januar 2010 15:14:43 schrieb Felipe Balbi:
> >There are load bursting issues too.  All part of the USB spec;
> >a load that's OK for 1 millisecond might not be OK for 1 second.
> 
> if you get a SetConfiguration(config), then you can use that load for as 
> long as needed, the limitation is when not enumerated, afaict.

And when suspended.

	HTH
		Oliver

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

On Tue, Jan 26, 2010 at 03:24:49PM +0100, ext Oliver Neukum wrote:
>Am Dienstag, 26. Januar 2010 15:14:43 schrieb Felipe Balbi:
>> >There are load bursting issues too.  All part of the USB spec;
>> >a load that's OK for 1 millisecond might not be OK for 1 second.
>>
>> if you get a SetConfiguration(config), then you can use that load for as
>> long as needed, the limitation is when not enumerated, afaict.
>
>And when suspended.

but when suspended, we have to cut power ASAP. If not enumerated we can 
still draw power for a few miliseconds due to dead battery provision.

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Felipe Balbi wrote:
> but when suspended, we have to cut power ASAP. If not enumerated we can 
> still draw power for a few miliseconds due to dead battery provision.

When suspended, it's OK to draw a small amount of power.
On the order of one milliamp, based on the config that's
active ... instead of, often, hundreds.

That limit is why for example a PXA 255 could never get
certified as a bus-powered peripheral.  It required much
more than that when in suspend mode.

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Felipe Balbi wrote:
> >
> >Thing is, supplying current is a bit more involved.  If the
> >board can't supply 300 mA, the USB configuration selection
> >mechanism has to know that, so it never selects peripheral
> >configurations which require that much current.
> 
> but that's done already by the usb core, no ? It rules out configuration 
> based on the hub->power_budget (can't remember if the field is that 
> exact name).

Yes, it handles that ... but where does the power budget
come from?  That's what I meant by "more involved".

As in, the host/supplying side of the power equation can't
be event driven like the peripheral/consuming side can.

And that's another reason I think it's best to fully solve
the common (peripheral, recharge-that-batter) case first.

- Dave

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

On Tue, Jan 26, 2010 at 04:21:28PM +0100, ext David Brownell wrote:
>On Tuesday 26 January 2010, Felipe Balbi wrote:
>> >
>> >Thing is, supplying current is a bit more involved.  If the
>> >board can't supply 300 mA, the USB configuration selection
>> >mechanism has to know that, so it never selects peripheral
>> >configurations which require that much current.
>>
>> but that's done already by the usb core, no ? It rules out configuration
>> based on the hub->power_budget (can't remember if the field is that
>> exact name).
>
>Yes, it handles that ... but where does the power budget
>come from?  That's what I meant by "more involved".

we set it from board-files (on ARM, at least). It's a board 
characteristic, no ?

>As in, the host/supplying side of the power equation can't
>be event driven like the peripheral/consuming side can.
>
>And that's another reason I think it's best to fully solve
>the common (peripheral, recharge-that-batter) case first.

fair enough.

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Mark Brown]

On Tue, Jan 26, 2010 at 05:35:21AM -0800, David Brownell wrote:
> On Tuesday 26 January 2010, Mark Brown wrote:

> > Yes please - it's not just chargers either, this can also be used by
> > PMICs which do power path management that includes USB.

> Color me confused ... what do you mean by "power path"?

In the sort of design I'm talking about there is generally a system
power rail which is generated from the various power sources available
to the system, which might include a combination of batteries, USB and
wall adaptors.  The power path management logic is the hardware which
controls which of these are actually being used as supplies, and may
also include battery charger management.

> Do you mean something like "the board as a whole can take N mA of
> current from USB", rather than specifically addressing a charger?

Pretty much, from this point of view.

> It's not uncommon to do things like use VBUS current to power the
> USB circuitry, too.  That can leave less for other purposes.  All
> of that being rather board-specific.

In this sort of design either VBUS goes through the power path
management logic before anything else gets to use it or the hardware
will know about the headroom it needs to leave.  The power path
management will usually do things like try to suppliment VBUS with any
battery that's available to generate the main system supply rail.

This all needs to function without software since it tends to get used
to decide things like if the system is able to begin power up at all, .

> > > Those seem like the wrong events.  The right events for a charger
> > > would be more along the lines of:

> > >  - For peripheral:  "you may use N milliAmperes now".
> > >  - General:  "Don't Charge" (a.k.a. "use 0 mA").

> > > I don't see how "N" would be passed with those events ...

> > These are good for the peripheral side.  You do get to pass a void *
> > along with the notifier value, that could be used to pass data like the
> > current limit.

> I don't think I saw that being done ... either in code, comments,
> or documentation.  Passing N is fundamental.

I think we're talking at cross purposes - I was reading "these events"
as being the new events quoted above, not the events in the existing
code.  I certainly agree that N is fundamental.

> Thing is, supplying current is a bit more involved.  If the
> board can't supply 300 mA, the USB configuration selection
> mechanism has to know that, so it never selects peripheral
> configurations which require that much current.

Indeed, the specific limits are more used for protection against things
like the connected devices drawing more current than they claimed than
anything else.

> Ergo my desire to start with a straightforward problem whose
> solution has real value (how much VBUS current may be consumed?),
> and leave some of those other messes for later!

Understandable.  It would be good to have an idea what sort of general
direction to go in there, though I do agree that the gadget case is much
more important here.

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Mark Brown wrote:
> > > Yes please - it's not just chargers either, this can also be used by
> > > PMICs which do power path management that includes USB.
> 
> > Color me confused ... what do you mean by "power path"?
> 
> In the sort of design I'm talking about there is generally a system
> power rail which is generated from the various power sources available
> to the system, which might include a combination of batteries, USB and
> wall adaptors.

Just as an example:  drivers/mfd/tps6510.c supports exactly
that trio of power sources.  More than one system rail though,
which (as you know) is pretty common -- core != I/O.

It's *way* simpler than e.g. the TWL6030.  :)


> The power path management logic is the hardware which 
> controls which of these are actually being used as supplies, and may
> also include battery charger management.
> 
> > Do you mean something like "the board as a whole can take N mA of
> > current from USB", rather than specifically addressing a charger?
> 
> Pretty much, from this point of view.

OK -- clear now.


> > It's not uncommon to do things like use VBUS current to power the
> > USB circuitry, too.  That can leave less for other purposes.  All
> > of that being rather board-specific.
> 
> In this sort of design either VBUS goes through the power path
> management logic before anything else gets to use it or the hardware
> will know about the headroom it needs to leave.  The power path
> management will usually do things like try to suppliment VBUS with any
> battery that's available to generate the main system supply rail.
> 
> This all needs to function without software since it tends to get used
> to decide things like if the system is able to begin power up at all, .

Yep.  That's part of the reason the USB specs have hard
rules about having 100 mA available (for some period)
even before software can come up.

Bus powered devices can come up on that 100mA, running
enough to enumerate ... and request more power, if they
need it.

Not all Linux systems can boot with that little power!

- Dave

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Mark Brown]

On Tue, Jan 26, 2010 at 07:44:46AM -0800, David Brownell wrote:
> On Tuesday 26 January 2010, Mark Brown wrote:

> > In the sort of design I'm talking about there is generally a system
> > power rail which is generated from the various power sources available
> > to the system, which might include a combination of batteries, USB and
> > wall adaptors.

> Just as an example:  drivers/mfd/tps6510.c supports exactly
> that trio of power sources.

Yup, it's a fairly standard feature set for all in one PMICs, WM835x and
WM831x are also examples of this.

>                              More than one system rail though,
> which (as you know) is pretty common -- core != I/O.

Yes, in this context the system rail is the supply input to the
regulators rather than the regulated voltages that are (mostly)
used directly by the chips.

> Bus powered devices can come up on that 100mA, running
> enough to enumerate ... and request more power, if they
> need it.

> Not all Linux systems can boot with that little power!

Some can even brown themselves out going full pelt with the full 500mA
supply if there's no battery to supplement it :/

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

Hi,

On Tue, Jan 26, 2010 at 12:16:20PM +0100, ext David Brownell wrote:
>On Friday 11 December 2009, Felipe Balbi wrote:
>> The notifier will be used to communicate usb events
>> to other drivers like the charger chip.
>
>Good idea ... but not OTG-specific.  It doesn't seem to me

thanks

>that charging hookups belong in that header at all.

I noticed that when started actually using it :-)

>In fact, usb_gadget_vbus_draw() might better be implemented
>as such a notifier call, removing that configuration mess
>from the usb gadget drivers ("how can I know what charger
>to use??").  That would be the most common situation:  a
>peripheral-only device.
>
>And in fact, OTG can be treated as a trivial superset of
>peripheral-only USB.  (In terms of charger support, only!!)
>
>I'd vote to convert all the USB-to-charger interfaces so
>they use notifiers.  After fixing the events ... see
>comments below.  :)

makes sense

>> @@ -9,6 +9,8 @@
>>  #ifndef __LINUX_USB_OTG_H
>>  #define __LINUX_USB_OTG_H
>>
>> +#include <linux/notifier.h>
>> +
>>  /* OTG defines lots of enumeration states before device reset */
>>  enum usb_otg_state {
>>  	OTG_STATE_UNDEFINED = 0,
>> @@ -33,6 +35,14 @@ enum usb_otg_state {
>>  	OTG_STATE_A_VBUS_ERR,
>>  };
>>
>> +enum usb_xceiv_events {
>
>Let's keep charger events separate from anything else,
>like "enter host mode" or "enter peripheral mode" (or
>even "disconnect").  The audiences for any other types
>of event would be entirely different.

the idea was to notify USB events to interested drivers, not only "usb 
charger events".

>Right now there's a mess in terms of charger hookup,
>so getting that straight is IMO a priority over any
>other type of event.  Using events will decouple a
>bunch of drivers, and simplify driver configuration.

well, if you consider that this transceiver isn't really otg specific, 
then this is already wrong.

>> +	USB_EVENT_NONE,         /* no events or cable disconnected */
>> +	USB_EVENT_VBUS,         /* vbus valid event */
>> +	USB_EVENT_ID,           /* id was grounded */
>> +	USB_EVENT_CHARGER,      /* usb dedicated charger */
>> +	USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
>
>Those seem like the wrong events.  The right events for a charger
>would be more along the lines of:
>
> - For peripheral:  "you may use N milliAmperes now".
> - General:  "Don't Charge" (a.k.a. "use 0 mA").

I have to disagree, which information would you used to kick the usb 
dedicated charger detection other than VBUS irq from transceiver ?

So we need at least that, and also need to notify when the charger 
detection is finished, so we can enable data pullups on the link. 
Remember we might be connected to a charging downstream port.

>I don't see how "N" would be passed with those events ...

there's a void * we can use to pass bMaxPower field of the selected 
configuration.

>Haven't looked at the details of the charger spec, but
>those two events are the *basics* from the USB 2.0 spec,
>so "official" charger hardware wouldn't be less capable.

I believe the dedicated charger is also "basic".

>Thing like different levels of VBUS validity, ID grounding,
>and so forth ... wouldn't be very relevant.  An OTG driver
>will do various things, internally, when ID grounds; but
>anything else is a function of what role eventually gets
>negotiated.  And for the charger, they all add up to "Don't
>Charge" (since ID grounded means A-role, sourcing power).

ID grounding event is necessary if you have an external charge pump. 
At least the boards I've been working use an external chip as the USB 
Charger and Charge pump, iow, the transceiver doesn't source VBUS on ID 
ground, but the charger chip is put into "boost" mode for that role.

>>  #define USB_OTG_PULLUP_ID		(1 << 0)
>>  #define USB_OTG_PULLDOWN_DP		(1 << 1)
>>  #define USB_OTG_PULLDOWN_DM		(1 << 2)
>> @@ -70,6 +80,9 @@ struct otg_transceiver {
>>  	struct otg_io_access_ops	*io_ops;
>>  	void __iomem			*io_priv;
>>
>> +	/* for notification of usb_xceiv_events */
>> +	struct blocking_notifier_head	notifier;
>
>Why "blocking"?  That seems kind of unnatural; for example,
>the main users -- like usb_gadget_vbus_draw() -- would be
>called in IRQ context (blocking not allowed).

what about irqs running in thread, wouldn't we "BUG sleeping in irq 
context" ?

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

Hi again,

On Tue, Jan 26, 2010 at 03:10:16PM +0100, Balbi Felipe (Nokia-D/Helsinki) wrote:
>On Tue, Jan 26, 2010 at 12:16:20PM +0100, ext David Brownell wrote:
>>On Friday 11 December 2009, Felipe Balbi wrote:
>>> The notifier will be used to communicate usb events
>>> to other drivers like the charger chip.
>>
>>Good idea ... but not OTG-specific.  It doesn't seem to me
>
>thanks

just remember of another problem which I couldn't solve yet:

if you boot the board with the usb cable already attached, then we miss 
the first notification because when the notifier is called, usb 
controller driver isn't probed yet.

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Felipe Balbi wrote:
> just remember of another problem which I couldn't solve yet:
> 
> if you boot the board with the usb cable already attached, then we miss 
> the first notification because when the notifier is called, usb 
> controller driver isn't probed yet.

That's part of why the OTG transceiver driver has methods
used by host and peripheral drivers to register themselves.

Standard init sequence there is to do nothing until both
drivers are fully initialized ... last step being to
register the drivers with the transceiver.  That way the
transceiver can know when its peer drivers are ready.

Example:  VBUS present from a host.  If the board runs
in OTG mode, as soon as both drivers are registered then
the B-Default state machine would start running ... and
that involves (see the OTG state machine!) issuing a VBBUS
event.

Same thing can be done with the power events.  As soon
as an event listener is registered, it could be fed any
events it missed.  (Just one approach; one must sort
out any other interdependencies too.  In this case, it
can make sense to consume 100mA current right away, and
then adjust the draw later if needed.)

- Dave

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[David Brownell]

On Tuesday 26 January 2010, Felipe Balbi wrote:
> >> +enum usb_xceiv_events {
> >
> >Let's keep charger events separate from anything else,
> >like "enter host mode" or "enter peripheral mode" (or
> >even "disconnect").  The audiences for any other types
> >of event would be entirely different.
> 
> the idea was to notify USB events to interested drivers, not only "usb 
> charger events".

There are thousands of events that could be issued.
I'd rather start with one specific problem, which
can really benefit from being solved.

If necessary, other events can be added later.


> >Right now there's a mess in terms of charger hookup,
> >so getting that straight is IMO a priority over any
> >other type of event.  Using events will decouple a
> >bunch of drivers, and simplify driver configuration.
> 
> well, if you consider that this transceiver isn't really otg specific, 
> then this is already wrong.

It's the only transceiver interface we have; and it
works for OTG transceivers in peripheral-only mode,
as well as host-only and dual-role modes.  So it's
not especially wrong.


However, "you can consume N milliAmperes now" doesn't
need to be coupled to a transceiver at all.  In fact,
it works just fine with any pure peripheral interface.
The gadget stack uses such calls ... and doesn't need
to be coupled to any transceiver.  (But obviously it
can hook up to an OTG transceiver.)


 
> >> +    USB_EVENT_NONE,         /* no events or cable disconnected */
> >> +    USB_EVENT_VBUS,         /* vbus valid event */
> >> +    USB_EVENT_ID,           /* id was grounded */
> >> +    USB_EVENT_CHARGER,      /* usb dedicated charger */
> >> +    USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
> >
> >Those seem like the wrong events.  The right events for a charger
> >would be more along the lines of:
> >
> > - For peripheral:  "you may use N milliAmperes now".
> > - General:  "Don't Charge" (a.k.a. "use 0 mA").
> 
> I have to disagree, which information would you used to kick the usb 
> dedicated charger detection other than VBUS irq from transceiver ?

That's why I said what I did about the separate charger spec (and 
you quoted it below):  it's not going to be less than those two
ops, which your events don't really capture.

That's "bonus" functionality though ... among other reasons, it's
not all that common yet.  The basic "charge battery over USB"
scenario needs to work without that stuff.

 
> So we need at least that, and also need to notify when the charger 
> detection is finished, so we can enable data pullups on the link. 
> Remember we might be connected to a charging downstream port.

So you're presuming some separate component will do charger
detection by listening for events?  If it's mucking with the
pullups, that seems very much like what an OTG transciever
needs to be managing.  And thus, perhaps, transceiver code.

If there's such a separate component, I'd like to see some
detail about how it'd work.  But ... at first glance, it'd
have thought it'd be simplest inside a transceiver driver.


> >I don't see how "N" would be passed with those events ...
> 
> there's a void * we can use to pass bMaxPower field of the selected 
> configuration.

Needs to be part of the event spec...

 
> >Haven't looked at the details of the charger spec, but
> >those two events are the *basics* from the USB 2.0 spec,
> >so "official" charger hardware wouldn't be less capable.
> 
> I believe the dedicated charger is also "basic".

We could take a vote to see how many folk have even seen
one, much less own one.  They're not very common, and not
part of the USB 2.0 spec.  That's why I say "not basic".

 
> >Thing like different levels of VBUS validity, ID grounding,
> >and so forth ... wouldn't be very relevant.  An OTG driver
> >will do various things, internally, when ID grounds; but
> >anything else is a function of what role eventually gets
> >negotiated.  And for the charger, they all add up to "Don't
> >Charge" (since ID grounded means A-role, sourcing power).
> 
> ID grounding event is necessary if you have an external charge pump. 
> At least the boards I've been working use an external chip as the USB 
> Charger and Charge pump, iow, the transceiver doesn't source VBUS on ID 
> ground, but the charger chip is put into "boost" mode for that role.

As you say:  transceiver stuff.  What I'm used to seeing is
what the OTG spec says:  ID grounding is an event, which
triggers state machine transitions.  One such transition
involves sourcing VBUS power and making sure it ramps up
properly.  Activating that, and monitoring it, depend on
hardware details which are tightly coupled to transceiver
logic and implementation.


> >>  #define USB_OTG_PULLUP_ID           (1 << 0)
> >>  #define USB_OTG_PULLDOWN_DP         (1 << 1)
> >>  #define USB_OTG_PULLDOWN_DM         (1 << 2)
> >> @@ -70,6 +80,9 @@ struct otg_transceiver {
> >>      struct otg_io_access_ops        *io_ops;
> >>      void __iomem                    *io_priv;
> >>
> >> +    /* for notification of usb_xceiv_events */
> >> +    struct blocking_notifier_head   notifier;
> >
> >Why "blocking"?  That seems kind of unnatural; for example,
> >the main users -- like usb_gadget_vbus_draw() -- would be
> >called in IRQ context (blocking not allowed).
> 
> what about irqs running in thread, wouldn't we "BUG sleeping in irq 
> context" ?

Iff the IRQ has a thread context, it can block.

But a SET_CONFIGURATION request is mostly going to
be delivered to a hard IRQ context, and that is what
will pass the host's vbus_draw configuration to the
hardware.  (Same for most of the other events you
sketched.)

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

On Tue, Jan 26, 2010 at 04:07:22PM +0100, ext David Brownell wrote:
>On Tuesday 26 January 2010, Felipe Balbi wrote:
>> >> +enum usb_xceiv_events {
>> >
>> >Let's keep charger events separate from anything else,
>> >like "enter host mode" or "enter peripheral mode" (or
>> >even "disconnect").  The audiences for any other types
>> >of event would be entirely different.
>>
>> the idea was to notify USB events to interested drivers, not only "usb
>> charger events".
>
>There are thousands of events that could be issued.
>I'd rather start with one specific problem, which
>can really benefit from being solved.
>
>If necessary, other events can be added later.
>
>
>> >Right now there's a mess in terms of charger hookup,
>> >so getting that straight is IMO a priority over any
>> >other type of event.  Using events will decouple a
>> >bunch of drivers, and simplify driver configuration.
>>
>> well, if you consider that this transceiver isn't really otg specific,
>> then this is already wrong.
>
>It's the only transceiver interface we have; and it
>works for OTG transceivers in peripheral-only mode,
>as well as host-only and dual-role modes.  So it's
>not especially wrong.
>
>
>However, "you can consume N milliAmperes now" doesn't
>need to be coupled to a transceiver at all.  In fact,
>it works just fine with any pure peripheral interface.
>The gadget stack uses such calls ... and doesn't need
>to be coupled to any transceiver.  (But obviously it
>can hook up to an OTG transceiver.)
>
>
>
>> >> +    USB_EVENT_NONE,         /* no events or cable disconnected */
>> >> +    USB_EVENT_VBUS,         /* vbus valid event */
>> >> +    USB_EVENT_ID,           /* id was grounded */
>> >> +    USB_EVENT_CHARGER,      /* usb dedicated charger */
>> >> +    USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
>> >
>> >Those seem like the wrong events.  The right events for a charger
>> >would be more along the lines of:
>> >
>> > - For peripheral:  "you may use N milliAmperes now".
>> > - General:  "Don't Charge" (a.k.a. "use 0 mA").
>>
>> I have to disagree, which information would you used to kick the usb
>> dedicated charger detection other than VBUS irq from transceiver ?
>
>That's why I said what I did about the separate charger spec (and
>you quoted it below):  it's not going to be less than those two
>ops, which your events don't really capture.
>
>That's "bonus" functionality though ... among other reasons, it's
>not all that common yet.  The basic "charge battery over USB"
>scenario needs to work without that stuff.
>
>
>> So we need at least that, and also need to notify when the charger
>> detection is finished, so we can enable data pullups on the link.
>> Remember we might be connected to a charging downstream port.
>
>So you're presuming some separate component will do charger
>detection by listening for events?  If it's mucking with the
>pullups, that seems very much like what an OTG transciever
>needs to be managing.  And thus, perhaps, transceiver code.

well, if you have access to twl5031 docs you'd understand what I'm 
talking about, the charger detection involves at least 3 blocks on 
twl5031 plus musb to enable/disable pullups. The sequence is pretty much 
as below:

1. vbus irq
2. usb_gadget_disconnect()
3. disable usb ldos
4. switch usb3v1 supply from vbat to vbus (to let charger detection work 
on low bat)
5. enable usb3v1 *only*
6. call the notifier chain
7. BCC module kicks charger detection
8. disable usb3v1
9. switch usb3v1 supply back to vbat
10. enable usb ldos
11. usb_gadget_connect() (necessary since we might be connected to 
charging port)

vbus irq comes from transceiver (drivers/usb/otg/twl4030-usb.c), 
notifier (currently) is also issued from there. 
usb_gadget_connect/disconnect() is implemented in 
drivers/usb/musb/musb_gadget.c, BCC module is a power_supply driver (not 
in mainline yet, I guess).

And after all that, we still have bq2415x as the charger chip itself. On 
that we configure input current and all the filters imposed by pse law. 
There's also the battery monitoring part which will involve the MADC 
part of twl4030/5030/5031/tpsxxxxx and some temperature sensor (maybe).

So the whole thing is quite complicated and should probably be moved to 
some "core" code.

>If there's such a separate component, I'd like to see some
>detail about how it'd work.  But ... at first glance, it'd
>have thought it'd be simplest inside a transceiver driver.

well, we could export some symbols to the transceiver to access the BCC 
address space in twl, but why if we can let bcc do that by itself if we 
just tell it "hey dude, vbus is alive".

>We could take a vote to see how many folk have even seen
>one, much less own one.  They're not very common, and not
>part of the USB 2.0 spec.  That's why I say "not basic".

ok, got it. But we already have plenty of devices on the market which 
support them. Look at n900, for example, the only way to charge its 
battery if via usb port ;-)

>> what about irqs running in thread, wouldn't we "BUG sleeping in irq
>> context" ?
>
>Iff the IRQ has a thread context, it can block.

ok, so what do you suggest in this case ?

we know that on omaps vbus will come from an i2c-connected transceiver 
so its irq handler will be running in a thread and VBUS is the first 
valuable information we have on usb point of view.

-- 
balbi

Re: [RFC/PATCH 1/5] usb: otg: add notifier support

[Felipe Balbi]

Hi,

On Tue, Jan 26, 2010 at 08:09:34PM +0100, Balbi Felipe (Nokia-D/Helsinki) wrote:
>well, if you have access to twl5031 docs you'd understand what I'm
>talking about, the charger detection involves at least 3 blocks on
>twl5031 plus musb to enable/disable pullups. The sequence is pretty much
>as below:

there's more which I forgot:

>1. vbus irq
>2. usb_gadget_disconnect()
>3. disable usb ldos

3.1 put transceiver in non-drivig mode

>4. switch usb3v1 supply from vbat to vbus (to let charger detection work
>on low bat)
>5. enable usb3v1 *only*
>6. call the notifier chain
>7. BCC module kicks charger detection
>8. disable usb3v1
>9. switch usb3v1 supply back to vbat

9.1 put transceiver back to normal mode

>10. enable usb ldos
>11. usb_gadget_connect() (necessary since we might be connected to
>charging port)

now it should be all fine.

-- 
balbi

[RFC/PATCH 2/5] usb: otg: twl4030: add support for notifier

[Felipe Balbi]

it's expected that the transceiver driver will
initialize and call the notifier chain when
necessary. Implement that for twl4030-usb driver.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/otg/twl4030-usb.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index 2ffc8eb..c3e498a 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -36,7 +36,7 @@
 #include <linux/i2c/twl4030.h>
 #include <linux/regulator/consumer.h>
 #include <linux/err.h>
-
+#include <linux/notifier.h>
 
 /* Register defines */
 
@@ -628,7 +628,8 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 		else
 			twl4030_phy_resume(twl);
 
-		twl4030charger_usb_en(status == USB_LINK_VBUS);
+		blocking_notifier_call_chain(&twl->otg.notifier, status,
+				twl->otg.gadget);
 	}
 	sysfs_notify(&twl->dev->kobj, NULL, "vbus");
 
@@ -718,6 +719,8 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
+	BLOCKING_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
+
 	/* Our job is to use irqs and status from the power module
 	 * to keep the transceiver disabled when nothing's connected.
 	 *
-- 
1.6.6.rc0

Re: [RFC/PATCH 2/5] usb: otg: twl4030: add support for notifier

[sai pavan]

>
> @@ -628,7 +628,8 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>                else
>                        twl4030_phy_resume(twl);
>
> -               twl4030charger_usb_en(status == USB_LINK_VBUS);
> +               blocking_notifier_call_chain(&twl->otg.notifier, status,
> +                               twl->otg.gadget);
>        }

You might not want to invoke blocking notifier chain(may sleep) from
interrupt context. May be atomic notifier chain is appropriate here.

Re: [RFC/PATCH 2/5] usb: otg: twl4030: add support for notifier

[Felipe Balbi]

Hi,

On Fri, Dec 11, 2009 at 06:22:14PM +0100, ext sai pavan wrote:
>> @@ -628,7 +628,8 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
>>                else
>>                        twl4030_phy_resume(twl);
>>
>> -               twl4030charger_usb_en(status == USB_LINK_VBUS);
>> +               blocking_notifier_call_chain(&twl->otg.notifier, status,
>> +                               twl->otg.gadget);
>>        }
>
>You might not want to invoke blocking notifier chain(may sleep) from
>interrupt context. May be atomic notifier chain is appropriate here.

that's a threaded irq. Maybe we should patch all twl children to use 
request_threaded_irq() already. I'll test that tomorrow.

-- 
balbi

Re: [RFC/PATCH 2/5] usb: otg: twl4030: add support for notifier

[Mark Brown]

On Fri, Dec 11, 2009 at 10:40:14PM +0200, Felipe Balbi wrote:

> that's a threaded irq. Maybe we should patch all twl children to use
> request_threaded_irq() already. I'll test that tomorrow.

You should also now be able to do all the interrupt handling with genirq
without the lockdep dodges that used to be be required.

[RFC/PATCH 0/4] twl4030 threaded_irq support

[Felipe Balbi]

move twl4030 children to threaded irq.

Felipe Balbi (4):
  input: keyboard: twl4030: move to request_threaded_irq
  input: misc: twl4030: move to request_threaded_irq
  rtc: twl4030: move to request_threaded_irq
  usb: otg: twl4030: move to request_threaded_irq

 drivers/input/keyboard/twl4030_keypad.c |   11 ++---------
 drivers/input/misc/twl4030-pwrbutton.c  |   12 +-----------
 drivers/rtc/rtc-twl4030.c               |   10 +---------
 drivers/usb/otg/twl4030-usb.c           |   10 +---------
 4 files changed, 5 insertions(+), 38 deletions(-)

Re: [RFC/PATCH 0/4] twl4030 threaded_irq support

[David Brownell]

On Monday 14 December 2009, Felipe Balbi wrote:
> move twl4030 children to threaded irq.
> 
> Felipe Balbi (4):
>   input: keyboard: twl4030: move to request_threaded_irq
>   input: misc: twl4030: move to request_threaded_irq
>   rtc: twl4030: move to request_threaded_irq
>   usb: otg: twl4030: move to request_threaded_irq

But nothing in drivers/mfd ... the entry to the whole stack?

Did the threaded IRQ stuff ever get set up so that the top
level IRQ thread didn't have to hand off to another thread
each time?  (That's how the current stuff works.  One thread
calling out to each handler.)

If so, I'd like to see that be used here, rather than needlessly
spend all those pages on mostly-unused stacks.  


> 
>  drivers/input/keyboard/twl4030_keypad.c |   11 ++---------
>  drivers/input/misc/twl4030-pwrbutton.c  |   12 +-----------
>  drivers/rtc/rtc-twl4030.c               |   10 +---------
>  drivers/usb/otg/twl4030-usb.c           |   10 +---------
>  4 files changed, 5 insertions(+), 38 deletions(-)
> 
> 

Re: [RFC/PATCH 0/4] twl4030 threaded_irq support

[David Brownell]

On Monday 25 January 2010, David Brownell wrote:
> Did the threaded IRQ stuff ever get set up so that the top
> level IRQ thread didn't have to hand off to another thread
> each time?  (That's how the current stuff works.  One thread
> calling out to each handler.)

Yes:  set_irq_nested_thread().  Looks like the toplevel IRQ
demux (in drivers/mfd/twl*irq*c) should use that, along with
the ONESHOT flag and (eventually) bus_lock stuff.

All the key parts that were missing a few years ago now seem
to be present.  But, not yet in use here.  :)

- Dave

Re: [RFC/PATCH 0/4] twl4030 threaded_irq support

[Mark Brown]

On Mon, Jan 25, 2010 at 11:06:55PM -0800, David Brownell wrote:
> On Monday 14 December 2009, Felipe Balbi wrote:

> > move twl4030 children to threaded irq.

> But nothing in drivers/mfd ... the entry to the whole stack?

> Did the threaded IRQ stuff ever get set up so that the top
> level IRQ thread didn't have to hand off to another thread
> each time?  (That's how the current stuff works.  One thread
> calling out to each handler.)

Yes, that's in mainline now.  There's two bits required to use it which
are (from the driver point of view) fairly orthogonal - one is to
convert the interrupt controller to use the new stuff, the other is to
make sure that all the drivers that are requesting the interrupts over
to threaded IRQ handlers.  genirq will require the threaded handlers
once it's being used.

Re: [RFC/PATCH 0/4] twl4030 threaded_irq support

[Felipe Balbi]

On Tue, Jan 26, 2010 at 08:06:55AM +0100, ext David Brownell wrote:
>On Monday 14 December 2009, Felipe Balbi wrote:
>> move twl4030 children to threaded irq.
>>
>> Felipe Balbi (4):
>>   input: keyboard: twl4030: move to request_threaded_irq
>>   input: misc: twl4030: move to request_threaded_irq
>>   rtc: twl4030: move to request_threaded_irq
>>   usb: otg: twl4030: move to request_threaded_irq
>
>But nothing in drivers/mfd ... the entry to the whole stack?
>
>Did the threaded IRQ stuff ever get set up so that the top
>level IRQ thread didn't have to hand off to another thread
>each time?  (That's how the current stuff works.  One thread
>calling out to each handler.)
>
>If so, I'd like to see that be used here, rather than needlessly
>spend all those pages on mostly-unused stacks.

correct, that's still missing. I tried to play with it for a while, but 
had to give up due to other musb tasks. So if someone wants to step up 
and code that part, I'd be glad

-- 
balbi

Re: [RFC/PATCH 0/4] twl4030 threaded_irq support

[David Brownell]

On Tuesday 26 January 2010, Felipe Balbi wrote:
> >But nothing in drivers/mfd ... the entry to the whole stack?
> >...
> 
> correct, that's still missing. I tried to play with it for a while, but 
> had to give up due to other musb tasks. So if someone wants to step up 
> and code that part, I'd be glad

I'm quite sure I sent a patch doing that ... sometime early last
summer, when the threaded IRQ stuff was being thrashed out!  (To
show what it'd look like, and see what problems remained.)  That
interface hasn't changed much since then, except for addition of
a stuff like ONESHOT support, set_irq_nested_thread(), and the
bus_lock stuff.  It's surely in LKML archives.  :)

My point here was more like:  these patches shouldn't merge without
that top-level change.  Just switch the whole current structure
(one thread, nesting) over to the now-standard interfaces at the
same time, not incrementally.  Later, the bus_lock stuff can switch
over too (for cleaner mask/unmask).

That's clearly material for 2.6.34 though ...

- Dave

[RFC/PATCH 1/4] input: keyboard: twl4030: move to request_threaded_irq

[Felipe Balbi]

move to request_threaded_irq() on twl4030 children.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/input/keyboard/twl4030_keypad.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/twl4030_keypad.c b/drivers/input/keyboard/twl4030_keypad.c
index 9a2977c..753a693 100644
--- a/drivers/input/keyboard/twl4030_keypad.c
+++ b/drivers/input/keyboard/twl4030_keypad.c
@@ -253,14 +253,6 @@ static irqreturn_t do_kp_irq(int irq, void *_kp)
 	u8 reg;
 	int ret;
 
-#ifdef CONFIG_LOCKDEP
-	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
-	 * we don't want and can't tolerate.  Although it might be
-	 * friendlier not to borrow this thread context...
-	 */
-	local_irq_enable();
-#endif
-
 	/* Read & Clear TWL4030 pending interrupt */
 	ret = twl4030_kpread(kp, &reg, KEYP_ISR1, 1);
 
@@ -403,7 +395,8 @@ static int __devinit twl4030_kp_probe(struct platform_device *pdev)
 	 *
 	 * NOTE:  we assume this host is wired to TWL4040 INT1, not INT2 ...
 	 */
-	error = request_irq(kp->irq, do_kp_irq, 0, pdev->name, kp);
+	error = request_threaded_irq(kp->irq, NULL, do_kp_irq,
+			0, pdev->name, kp);
 	if (error) {
 		dev_info(kp->dbg_dev, "request_irq failed for irq no=%d\n",
 			kp->irq);
-- 
1.6.6.rc0

[RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq

[Felipe Balbi]

move to request_threaded_irq() on twl4030 children.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/input/misc/twl4030-pwrbutton.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index f5fc997..cd47d9e 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -39,16 +39,6 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
 	int err;
 	u8 value;
 
-#ifdef CONFIG_LOCKDEP
-	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
-	 * we don't want and can't tolerate since this is a threaded
-	 * IRQ and can sleep due to the i2c reads it has to issue.
-	 * Although it might be friendlier not to borrow this thread
-	 * context...
-	 */
-	local_irq_enable();
-#endif
-
 	err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
 				  STS_HW_CONDITIONS);
 	if (!err)  {
@@ -80,7 +70,7 @@ static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
 	pwr->phys = "twl4030_pwrbutton/input0";
 	pwr->dev.parent = &pdev->dev;
 
-	err = request_irq(irq, powerbutton_irq,
+	err = request_threaded_irq(irq, NULL, powerbutton_irq,
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 			"twl4030_pwrbutton", pwr);
 	if (err < 0) {
-- 
1.6.6.rc0

RE: [RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq

[Shilimkar, Santosh]

Felipe,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Felipe
> Balbi
> Sent: Monday, December 14, 2009 4:01 PM
> To: linux-kernel@vger.kernel.org
> Cc: Linux OMAP Mailing List; Tony Lindgren; Aaro Koskinen; David Brownell; Linux USB Mailing List;
> Anton Vorontsov; Grazvydas Ignotas; Chikkature Rajashekar, Madhusudhan; Greg Kroah-Hartman; Mark
> Brown; Samuel Ortiz; Felipe Balbi
> Subject: [RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq
> 
> move to request_threaded_irq() on twl4030 children.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  drivers/input/misc/twl4030-pwrbutton.c |   12 +-----------
>  1 files changed, 1 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
> index f5fc997..cd47d9e 100644
> --- a/drivers/input/misc/twl4030-pwrbutton.c
> +++ b/drivers/input/misc/twl4030-pwrbutton.c
> @@ -39,16 +39,6 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
>  	int err;
>  	u8 value;
> 
> -#ifdef CONFIG_LOCKDEP
> -	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> -	 * we don't want and can't tolerate since this is a threaded
> -	 * IRQ and can sleep due to the i2c reads it has to issue.
> -	 * Although it might be friendlier not to borrow this thread
> -	 * context...
> -	 */
> -	local_irq_enable();
> -#endif
> -
>  	err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
>  				  STS_HW_CONDITIONS);
>  	if (!err)  {
> @@ -80,7 +70,7 @@ static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
>  	pwr->phys = "twl4030_pwrbutton/input0";
>  	pwr->dev.parent = &pdev->dev;
> 
> -	err = request_irq(irq, powerbutton_irq,
> +	err = request_threaded_irq(irq, NULL, powerbutton_irq,
>  			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>  			"twl4030_pwrbutton", pwr);
>  	if (err < 0) {
In whole of the series the ISR you have converted to threads using threaded_irq are very
small in size. They are like quick_change_handlers. So only advantage is the particular 
interrupt is masked for bit longer than with you change.

I might be wrong here but it might introduce the spurious IRQ's because of bit of delay in the processing.What is the motive for this change ? Are we using this API as it suppose to be ?

Re: [RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq

[Felipe Balbi]

Hi,

On Mon, Dec 14, 2009 at 12:31:11PM +0100, ext Shilimkar, Santosh wrote:
>In whole of the series the ISR you have converted to threads using threaded_irq are very
>small in size. They are like quick_change_handlers. So only advantage is the particular
>interrupt is masked for bit longer than with you change.
>
>I might be wrong here but it might introduce the spurious IRQ's because 
>of bit of delay in the processing.What is the motive for this change ? 
>Are we using this API as it suppose to be ?

the irq chip is connected through i2c and that mandate us to handle irqs 
in thread context. Now that we have kernel-wise api for that, we're just 
moving towards it instead of definint our own stuff and the lockdep 
shortcuts we had to put before.

I boot tested these patches on a 3430-based board and it seems to be 
working fine. Did a few tests with usb only. I can see I get the usb 
presence irq correctly.

-- 
balbi

RE: [RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq

[Shilimkar, Santosh]

> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@nokia.com]
> Sent: Monday, December 14, 2009 5:10 PM
> To: Shilimkar, Santosh
> Cc: Balbi Felipe (Nokia-D/Helsinki); linux-kernel@vger.kernel.org; Linux OMAP Mailing List; Tony
> Lindgren; Koskinen Aaro (Nokia-D/Helsinki); David Brownell; Linux USB Mailing List; Anton Vorontsov;
> Grazvydas Ignotas; Chikkature Rajashekar, Madhusudhan; Greg Kroah-Hartman; Mark Brown; Samuel Ortiz
> Subject: Re: [RFC/PATCH 2/4] input: misc: twl4030: move to request_threaded_irq
> 
> Hi,
> 
> On Mon, Dec 14, 2009 at 12:31:11PM +0100, ext Shilimkar, Santosh wrote:
> >In whole of the series the ISR you have converted to threads using threaded_irq are very
> >small in size. They are like quick_change_handlers. So only advantage is the particular
> >interrupt is masked for bit longer than with you change.
> >
> >I might be wrong here but it might introduce the spurious IRQ's because
> >of bit of delay in the processing.What is the motive for this change ?
> >Are we using this API as it suppose to be ?
> 
> the irq chip is connected through i2c and that mandate us to handle irqs
> in thread context. Now that we have kernel-wise api for that, we're just
> moving towards it instead of definint our own stuff and the lockdep
> shortcuts we had to put before.
OK. I see your point now.

Regards,
Santosh 

[RFC/PATCH 3/4] rtc: twl4030: move to request_threaded_irq

[Felipe Balbi]

move to request_threaded_irq() on twl4030 children.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/rtc/rtc-twl4030.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
index 9c8c70c..46bc9c1 100644
--- a/drivers/rtc/rtc-twl4030.c
+++ b/drivers/rtc/rtc-twl4030.c
@@ -325,14 +325,6 @@ static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
 	int res;
 	u8 rd_reg;
 
-#ifdef CONFIG_LOCKDEP
-	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
-	 * we don't want and can't tolerate.  Although it might be
-	 * friendlier not to borrow this thread context...
-	 */
-	local_irq_enable();
-#endif
-
 	res = twl4030_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG);
 	if (res)
 		goto out;
@@ -424,7 +416,7 @@ static int __devinit twl4030_rtc_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto out1;
 
-	ret = request_irq(irq, twl4030_rtc_interrupt,
+	ret = request_threaded_irq(irq, NULL, twl4030_rtc_interrupt,
 				IRQF_TRIGGER_RISING,
 				dev_name(&rtc->dev), rtc);
 	if (ret < 0) {
-- 
1.6.6.rc0

[RFC/PATCH 4/4] usb: otg: twl4030: move to request_threaded_irq

[Felipe Balbi]

move to request_threaded_irq() on twl4030 children.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/otg/twl4030-usb.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index bd9883f..36bcd5f 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -576,14 +576,6 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl)
 	struct twl4030_usb *twl = _twl;
 	int status;
 
-#ifdef CONFIG_LOCKDEP
-	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
-	 * we don't want and can't tolerate.  Although it might be
-	 * friendlier not to borrow this thread context...
-	 */
-	local_irq_enable();
-#endif
-
 	status = twl4030_usb_linkstat(twl);
 	if (status != USB_LINK_UNKNOWN) {
 
@@ -702,7 +694,7 @@ static int __devinit twl4030_usb_probe(struct platform_device *pdev)
 	 * need both handles, otherwise just one suffices.
 	 */
 	twl->irq_enabled = true;
-	status = request_irq(twl->irq, twl4030_usb_irq,
+	status = request_threaded_irq(twl->irq, NULL, twl4030_usb_irq,
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 			"twl4030_usb", twl);
 	if (status < 0) {
-- 
1.6.6.rc0

[RFC/PATCH 3/5] usb: musb: add support for ulpi block

[Felipe Balbi]

add register definitions and musb_ulpi_readb/writeb
support.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/musb_regs.h |   62 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
index 473a94e..7e6e68e 100644
--- a/drivers/usb/musb/musb_regs.h
+++ b/drivers/usb/musb/musb_regs.h
@@ -72,6 +72,11 @@
 #define MUSB_DEVCTL_HR		0x02
 #define MUSB_DEVCTL_SESSION	0x01
 
+/* ULPI_REG_CONTROL */
+#define ULPI_REG_REQ		(1 << 0)
+#define ULPI_REG_CMPLT		(1 << 1)
+#define ULPI_RDN_WR		(1 << 2)
+
 /* TESTMODE */
 #define MUSB_TEST_FORCE_HOST	0x80
 #define MUSB_TEST_FIFO_ACCESS	0x40
@@ -247,6 +252,16 @@
 /* REVISIT: vctrl/vstatus: optional vendor utmi+phy register at 0x68 */
 #define MUSB_HWVERS		0x6C	/* 8 bit */
 
+/* ULPI Registers */
+#define ULPI_VBUS_CONTROL	0x70	/* 8 bit */
+#define ULPI_CARKIT_CONTROL	0x71	/* 8 bit */
+#define ULPI_INT_MASK		0x72	/* 8 bit */
+#define ULPI_INT_SRC		0x73	/* 8 bit */
+#define ULPI_REG_DATA		0x74	/* 8 bit */
+#define ULPI_REG_ADDR		0x75	/* 8 bit */
+#define ULPI_REG_CONTROL	0x76	/* 8 bit */
+#define ULPI_RAW_DATA		0x77	/* 8 bit */
+
 #define MUSB_EPINFO		0x78	/* 8 bit */
 #define MUSB_RAMINFO		0x79	/* 8 bit */
 #define MUSB_LINKINFO		0x7a	/* 8 bit */
@@ -502,4 +517,51 @@ static inline void  musb_write_txhubport(void __iomem *mbase, u8 epnum,
 
 #endif /* CONFIG_BLACKFIN */
 
+/* ULPI read/write support */
+static inline u8 musb_ulpi_readb(void __iomem *addr, u8 offset)
+{
+	int	i = 0;
+	u8	r;
+
+	musb_writeb(addr, ULPI_REG_ADDR, offset);
+	musb_writeb(addr, ULPI_REG_CONTROL, ULPI_REG_REQ | ULPI_RDN_WR);
+
+	while (!(musb_readb(addr, ULPI_REG_CONTROL) & ULPI_REG_CMPLT)) {
+		i++;
+		if (i == 10000) {
+			DBG(3, "ULPI read timed out\n");
+			return 0;
+		}
+
+	}
+	r = musb_readb(addr, ULPI_REG_CONTROL);
+	r &= ~ULPI_REG_CMPLT;
+	musb_writeb(addr, ULPI_REG_CONTROL, r);
+
+	return musb_readb(addr, ULPI_REG_DATA);
+}
+
+static inline void musb_ulpi_writeb(void __iomem *addr,
+	u8 offset, u8 data)
+{
+	int	i = 0;
+	u8	r = 0;
+
+	musb_writeb(addr, ULPI_REG_ADDR, offset);
+	musb_writeb(addr, ULPI_REG_DATA, data);
+	musb_writeb(addr, ULPI_REG_CONTROL, ULPI_REG_REQ);
+
+	while(!(musb_readb(addr, ULPI_REG_CONTROL) & ULPI_REG_CMPLT)) {
+		i++;
+		if (i == 10000) {
+			DBG(3, "ULPI write timed out\n");
+			return;
+		}
+	}
+
+	r = musb_readb(addr, ULPI_REG_CONTROL);
+	r &= ~ULPI_REG_CMPLT;
+	musb_writeb(addr, ULPI_REG_CONTROL, r);
+}
+
 #endif	/* __MUSB_REGS_H__ */
-- 
1.6.6.rc0

[RFC/PATCH 4/5] usb: musb: isp1704: add registers from isp1704

[Felipe Balbi]

transceiver used on RX-51 board.

NYET-Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/isp1704.h |   81 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/musb/isp1704.h

diff --git a/drivers/usb/musb/isp1704.h b/drivers/usb/musb/isp1704.h
new file mode 100644
index 0000000..c52406e
--- /dev/null
+++ b/drivers/usb/musb/isp1704.h
@@ -0,0 +1,81 @@
+/*
+ * isp1704.h - ISP 1704 Register
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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 St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ * THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN
+ * NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#ifndef __ISP1704_H__
+#define __ISP1704_H__
+
+#define ISP1704_VENDOR_ID_LOW		0x00
+#define ISP1704_VENDOR_ID_HIGH		0x01
+#define ISP1704_PRODUCT_ID_LOW		0x02
+#define ISP1704_PRODUCT_ID_HIGH		0x03
+#define ISP1704_FUNC_CTRL		0x04
+#define ISP1704_OTG_CTRL		0x0a
+#define ISP1704_USB_INTRISE		0x0d
+#define ISP1704_USB_INTFALL		0x10
+#define ISP1704_DEBUG			0x15
+#define ISP1704_SCRATCH			0x16
+#define ISP1704_PWR_CTRL		0x3d
+
+/* Function control */
+#define ISP1704_FUNC_CTRL_FULL_SPEED	(1 << 0)
+#define ISP1704_FUNC_CTRL_XCVRSELECT	0x3
+#define ISP1704_FUNC_CTRL_XCVRSELECT_SHIFT	(1 << 0)
+#define ISP1704_FUNC_CTRL_TERMSELECT	(1 << 2)
+#define ISP1704_FUNC_CTRL_OPMODE	(1 << 3)
+#define ISP1704_FUNC_CTRL_OPMODE_SHIFT	3
+#define ISP1704_FUNC_CTRL_RESET		(1 << 5)
+#define ISP1704_FUNC_CTRL_SUSPENDM	(1 << 6)
+
+/* OTG Control */
+#define ISP1704_OTG_CTRL_IDPULLUP	(1 << 0)
+#define ISP1704_OTG_CTRL_DP_PULLDOWN	(1 << 1)
+#define ISP1704_OTG_CTRL_DM_PULLDOWN	(1 << 2)
+#define ISP1704_OTG_CTRL_DISCHRG_VBUS	(1 << 3)
+#define ISP1704_OTG_CTRL_CHRG_VBUS	(1 << 4)
+#define ISP1704_OTG_CTRL_DRV_VBUS_EXT	(1 << 6)
+#define ISP1704_OTG_CTRL_USB_EXT_VBUS	(1 << 7)
+
+/* Debug */
+#define ISP1704_DEBUG_LINESTATE0	(1 << 0)
+#define ISP1704_DEBUG_LINESTATE1	(1 << 1)
+
+/* Power control */
+#define ISP1704_PWR_CTRL_SWCTRL		(1 << 0)
+#define ISP1704_PWR_CTRL_DET_COMP	(1 << 1)
+#define ISP1704_PWR_CTRL_BVALID_RISE	(1 << 2)
+#define ISP1704_PWR_CTRL_BVALID_FALL	(1 << 3)
+#define ISP1704_PWR_CTRL_DP_WKPU_EN	(1 << 4)
+#define ISP1704_PWR_CTRL_VDAT_DET	(1 << 5)
+#define ISP1704_PWR_CTRL_DPVSRC_EN	(1 << 6)
+#define ISP1704_PWR_CTRL_HWDETECT	(1 << 7)
+
+#endif	/* __ISP1704_H__ */
-- 
1.6.6.rc0

Re: [RFC/PATCH 4/5] usb: musb: isp1704: add registers from isp1704

[Krogerus Heikki (EXT-Teleca/Helsinki)]

Balbi Felipe (Nokia-D/Helsinki) wrote:
> transceiver used on RX-51 board.
>
> NYET-Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>   

There are now many ULPI transceivers in use. Let's add the global ULPI 
register definitions. isp170x transceivers have only one vendor-specific 
register. The Power Control.

-- 
heikki

Re: [RFC/PATCH 4/5] usb: musb: isp1704: add registers from isp1704

[Felipe Balbi]

On Fri, Dec 11, 2009 at 01:35:04PM +0100, Krogerus Heikki (EXT-Teleca/Helsinki) wrote:
>Balbi Felipe (Nokia-D/Helsinki) wrote:
>> transceiver used on RX-51 board.
>>
>> NYET-Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
>> ---
>>
>
>There are now many ULPI transceivers in use. Let's add the global ULPI
>register definitions. isp170x transceivers have only one vendor-specific
>register. The Power Control.

proof-of-concept, that's why it's NYET-Signed-off-by. It was easier to 
just copy the code from our working version for now, but sure, we have 
to do it in some way that works for everybody. I was thinking that 
RX-51's board files could provide us a charger_detect() implementation
which we can use here.

then:

if (!musb->charger_detect)
	return NOTIFY_DONE;

-- 
balbi

[RFC/PATCH 5/5] usb: musb: musb supports otg notifier

[Felipe Balbi]

we use the notifier to kick the charger detection.
Needed on RX-51 board. Later on we will notify
also the bMaxPower field from usb_configuration.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/musb_core.c |   72 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_core.h |    5 +++
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 55e185a..c7c60df 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -98,6 +98,7 @@
 #include <linux/kobject.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/notifier.h>
 
 #ifdef	CONFIG_ARM
 #include <mach/hardware.h>
@@ -144,6 +145,74 @@ MODULE_ALIAS("platform:" MUSB_DRIVER_NAME);
 
 /*-------------------------------------------------------------------------*/
 
+#include "isp1704.h"
+
+static int musb_detect_charger(struct musb *musb)
+{
+	unsigned long		timeout;
+	u8			vdat = 0;
+	u8			power;
+	u8			r;
+
+	/* first we disable data pullups */
+	power = musb_readb(musb->mregs, MUSB_POWER);
+	power &= ~MUSB_POWER_SOFTCONN;
+	musb_writeb(musb->mregs, MUSB_POWER, power);
+
+	/* now we set SW control bit in PWR_CTRL register */
+	r = musb_ulpi_readb(musb->mregs, ISP1704_PWR_CTRL);
+	r |= ISP1704_PWR_CTRL_SWCTRL;
+	musb_ulpi_writeb(musb->mregs, ISP1704_PWR_CTRL, r);
+
+	/* and finally enable manual charger detection */
+	r |= ISP1704_PWR_CTRL_DPVSRC_EN;
+	musb_ulpi_writeb(musb->mregs, ISP1704_PWR_CTRL, r);
+	msleep(10);
+
+	timeout = jiffies + msecs_to_jiffies(300);
+	while (!time_after(jiffies, timeout)) {
+		/* Check if there is a charger */
+		vdat = !!(musb_ulpi_readb(musb->mregs, ISP1704_PWR_CTRL)
+				& ISP1704_PWR_CTRL_VDAT_DET);
+		if (vdat)
+			break;
+	}
+
+	/* clear DPVSRC_EN, otherwise usb communication doesn't work */
+	r &= ~ISP1704_PWR_CTRL_DPVSRC_EN;
+	musb_ulpi_writeb(musb->mregs, ISP1704_PWR_CTRL, r);
+
+	if (vdat)
+		blocking_notifier_call_chain(&musb->xceiv->notifier,
+				USB_EVENT_CHARGER, &musb->g);
+
+	/*
+	 * re-enable data pullups, we might have been connected
+	 * to a host/hub charger
+	 */
+	power |= MUSB_POWER_SOFTCONN;
+	musb_writeb(musb->mregs, MUSB_POWER, power);
+
+	return vdat;
+}
+
+static int musb_notifier_call(struct notifier_block *nb, unsigned long event,
+		void *_gadget)
+{
+	struct usb_gadget	*g = _gadget;
+	struct musb		*musb = container_of(g, struct musb, g);
+
+	switch (event) {
+	case USB_EVENT_VBUS:
+		musb->is_charger = musb_detect_charger(musb);
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 static inline struct musb *dev_to_musb(struct device *dev)
 {
 #ifdef CONFIG_USB_MUSB_HDRC_HCD
@@ -1961,6 +2030,9 @@ bad_config:
 		goto fail2;
 	}
 
+	musb->nb.notifier_call = musb_notifier_call;
+	otg_register_notifier(musb->xceiv, &musb->nb);
+
 #ifndef CONFIG_MUSB_PIO_ONLY
 	if (use_dma && dev->dma_mask) {
 		struct dma_controller	*c;
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 644fcd8..3de6eb8 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -322,6 +322,9 @@ struct musb {
 	struct clk		*clock;
 	irqreturn_t		(*isr)(int, void *);
 	struct work_struct	irq_work;
+
+	struct notifier_block	nb;
+
 #define MUSB_HWVERS_MAJOR(x)	((x >> 10) & 0x1f)
 #define MUSB_HWVERS_MINOR(x)	(x & 0x3ff)
 #define MUSB_HWVERS_RC		0x8000
@@ -432,6 +435,8 @@ struct musb {
 	unsigned		is_self_powered:1;
 	unsigned		is_bus_powered:1;
 
+	unsigned		is_charger:1;
+
 	unsigned		set_address:1;
 	unsigned		test_mode:1;
 	unsigned		softconnect:1;
-- 
1.6.6.rc0

Re: [RFC/PATCH 5/5] usb: musb: musb supports otg notifier

[Felipe Balbi]

On Fri, Dec 11, 2009 at 12:31:26PM +0100, Balbi Felipe (Nokia-D/Helsinki) wrote:
>we use the notifier to kick the charger detection.
>Needed on RX-51 board. Later on we will notify
>also the bMaxPower field from usb_configuration.
>
>Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>

this is supposed to be:
NYET-Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>

-- 
balbi

RE: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Madhusudhan]

> -----Original Message-----
> From: Anton Vorontsov [mailto:avorontsov@ru.mvista.com]
> Sent: Thursday, December 10, 2009 8:44 AM
> To: Felipe Balbi
> Cc: Grazvydas Ignotas; linux-kernel@vger.kernel.org; Madhusudhan
> Chikkature; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI
> charger
> 
> On Thu, Dec 10, 2009 at 04:21:27PM +0200, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Dec 10, 2009 at 03:18:30PM +0100, ext Anton Vorontsov wrote:
> > >>Ok since it doesn't look like this will resolve soon, what about
> > >>adding some DEVICE_ATTR for the time being and requiring userspace to
> > >>write charge current here to start actual charging?
> > >
> > >Works for me. Let's think of the kernel charging support as an
> > >yet unimplemented feature.
> >
> > but if you guys are ok with this one for now. It can always be
> > changed later :-)
> 
> Yep. The only thing I'm afraid of is that once the driver is in,
> then nobody will bother with improving it to do the right thing.
> But an imperfect driver is better than none.
> 

So, what is the conclusion then? Are you planning to push the BCI charger
patch currently and handle updates later?

Regards,
Madhu

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

Re: [PATCH] power_supply: Add driver for TWL4030/TPS65950 BCI charger

[Felipe Balbi]

Hi,

On Thu, Dec 10, 2009 at 03:09:08PM +0100, ext Grazvydas Ignotas wrote:
>Ok since it doesn't look like this will resolve soon, what about
>adding some DEVICE_ATTR for the time being and requiring userspace to
>write charge current here to start actual charging?

We can get the charging current from configuration.

When gadget driver call usb_gadget_vbus_draw() we can use that to pass 
the information somewhere else.

But what I'm doing now is implementing an atomic_notifier which will 
call whatever function another driver want whenever we have twl4030-usb 
irqs.

We can use that to kick the charger detection itself (on twl4030-usb) 
and you can register your notifier_block callback to enable charging as 
you wish.

Then we will have the charger detection done on the right place 
(twl4030-usb) and the charging start done on the right place 
(twl4030-bci or bq24xxx).

Thanks to Anton Vorontsov <avorontsov@ru.mvista.com> for suggesting 
that.

-- 
balbi