public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/7] [RFC] Common power driver for Linux gadgets
@ 2007-04-11 23:24 Anton Vorontsov
  2007-04-13 13:50 ` Anton Vorontsov
  0 siblings, 1 reply; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-11 23:24 UTC (permalink / raw)
  To: linux-kernel; +Cc: kernel-discuss, dwmw2

This driver used to stop code/logic duplication through different
machines we porting at handhelds.org. pda_power register machs' power
supplies, and will take care about notifying batteries about power
changes through external power interface.

This driver should be suitable for almost every Linux gadget today.


Here is brief example how we use it:

static int h5000_is_ac_online(void)
{
        return !!(samcop_get_gpio_a(&h5400_samcop.dev) &
                 SAMCOP_GPIO_GPA_ADP_IN_STATUS);
}

static int h5000_is_usb_online(void)
{
        return !!(samcop_get_gpio_a(&h5400_samcop.dev) &
                 SAMCOP_GPIO_GPA_USB_DETECT);
}

static void h5000_set_charge(int flags)
{
        SET_H5400_GPIO(CHG_EN, !!flags);
        SET_H5400_GPIO(EXT_CHG_RATE, !!(flags & PDA_POWER_CHARGE_AC));
        SET_H5400_GPIO(USB_CHG_RATE, !!(flags & PDA_POWER_CHARGE_USB));
        return;
}

static struct pda_power_pdata h5000_power_pdata = {
        .is_ac_online = h5000_is_ac_online,
        .is_usb_online = h5000_is_usb_online,
        .set_charge = h5000_set_charge,
};

static struct resource h5000_power_resourses[] = {
        [0] = {
                .name = "ac",
                .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
                         IORESOURCE_IRQ_LOWEDGE,
        },
        [1] = {
                .name = "usb",
                .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE |
                         IORESOURCE_IRQ_LOWEDGE,
        },
};

static struct platform_device h5000_power_pdev = {
        .name = "pda-power",
        .id = -1,
        .resource = h5000_power_resourses,
        .num_resources = ARRAY_SIZE(h5000_power_resourses),
        .dev = {
                .platform_data = &h5000_power_pdata,
        },
};

---
 drivers/power/Kconfig     |    8 ++
 drivers/power/Makefile    |    1 +
 drivers/power/pda_power.c |  218 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pda_power.h |   27 ++++++
 4 files changed, 254 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/pda_power.c
 create mode 100644 include/linux/pda_power.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 17349c1..b87779e 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -10,4 +10,12 @@ config EXTERNAL_POWER
 
 	  This interface is mandatory for battery class support.
 
+config PDA_POWER
+	tristate "Generic PDA/phone power driver"
+	depends on EXTERNAL_POWER
+	help
+	  Say Y here to enable generic power driver for PDAs and phones with
+	  one or two external power supplies (AC/USB) connected to main and
+	  backup batteries, and optional builtin charger.
+
 endmenu
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index c303b45..6f084e7 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_EXTERNAL_POWER)  += external_power.o
+obj-$(CONFIG_PDA_POWER)       += pda_power.o
diff --git a/drivers/power/pda_power.c b/drivers/power/pda_power.c
new file mode 100644
index 0000000..0256ee4
--- /dev/null
+++ b/drivers/power/pda_power.c
@@ -0,0 +1,218 @@
+/*
+ * Common power driver for PDAs and phones with one or two external
+ * power supplies (AC/USB) connected to main and backup batteries,
+ * and optional builtin charger.
+ *
+ * Copyright 2007 Anton Vorontsov <cbou@mail.ru>
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/pda_power.h>
+
+/* 
+ * include/linux/ioport.h does not provide flags for generic IRQ trigger
+ * types. So, we're using "ISA PnP IRQ specific bits", and converting them.
+ */
+static unsigned int get_irq_flags(struct resource *res)
+{
+	unsigned int flags = IRQF_DISABLED;
+
+	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
+		flags |= IRQF_TRIGGER_RISING;
+	if (res->flags & IORESOURCE_IRQ_LOWEDGE)
+		flags |= IRQF_TRIGGER_FALLING;
+	if (res->flags & IORESOURCE_IRQ_HIGHLEVEL)
+		flags |= IRQF_TRIGGER_HIGH;
+	if (res->flags & IORESOURCE_IRQ_LOWLEVEL)
+		flags |= IRQF_TRIGGER_LOW;
+	if (res->flags & IORESOURCE_IRQ_SHAREABLE)
+		flags |= IRQF_SHARED;
+
+	return flags;
+}
+
+static struct resource *ac_irq, *usb_irq;
+static struct pda_power_pdata *pdata;
+
+static int pda_power_is_ac_online(struct power_supply *psy)
+{
+	return pdata->is_ac_online ? pdata->is_ac_online() : 0;
+}
+
+static int pda_power_is_usb_online(struct power_supply *psy)
+{
+	return pdata->is_usb_online ? pdata->is_usb_online() : 0;
+}
+
+static char *pda_power_supplied_to[] = {
+	"main-battery",
+	"backup-battery",
+};
+
+static struct power_supply pda_power_supplies[] = {
+	{
+		.name = "ac",
+		.type = "ac",
+		.supplied_to = pda_power_supplied_to,
+		.num_supplicants = ARRAY_SIZE(pda_power_supplied_to),
+		.is_online = pda_power_is_ac_online,
+	},
+	{
+		.name = "usb",
+		.type = "dc",
+		.supplied_to = pda_power_supplied_to,
+		.num_supplicants = ARRAY_SIZE(pda_power_supplied_to),
+		.is_online = pda_power_is_usb_online,
+	},
+};
+
+static void update_charger(void)
+{
+	if (!pdata->set_charge)
+		return;
+
+	if (pdata->is_ac_online && pdata->is_ac_online()) {
+		pr_debug("pda_power: charger on (AC)\n");
+		pdata->set_charge(PDA_POWER_CHARGE_AC);
+	}
+	else if (pdata->is_ac_online && pdata->is_usb_online()) {
+		pr_debug("pda_power: charger on (USB)\n");
+		pdata->set_charge(PDA_POWER_CHARGE_USB);
+	}
+	else {
+		pr_debug("pda_power: charger off\n");
+		pdata->set_charge(0);
+	}
+
+	return;
+}
+
+static irqreturn_t power_changed_isr(int irq, void *unused)
+{
+	if (ac_irq && irq == ac_irq->start) {
+		power_supply_changed(&pda_power_supplies[0]);
+	}
+	else if (usb_irq && irq == usb_irq->start) {
+		power_supply_changed(&pda_power_supplies[1]);
+	}
+	else {
+		printk(KERN_WARNING "pda_power: spurious irq %d", irq);
+		return IRQ_HANDLED;
+	}
+
+	update_charger();
+
+	return IRQ_HANDLED;
+}
+
+static int pda_power_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+
+	if (pdev->id != -1) {
+		printk(KERN_WARNING "pda_power: it's meaningless to "
+		       "register several pda_powers, use id = -1\n");
+		ret = -EINVAL;
+		goto wrongid;
+	}
+
+	ac_irq = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "ac");
+	usb_irq = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "usb");
+	if (!ac_irq && !usb_irq) {
+		printk(KERN_ERR "pda_power: no ac/usb irq specified\n");
+		ret = -ENODEV;
+		goto noirqs;
+	}
+
+	ret = power_supply_register(&pdev->dev, &pda_power_supplies[0]);
+	if (ret) {
+		printk(KERN_ERR "pda_power: failed to register %s power "
+		       "supply\n", pda_power_supplies[0].name);
+		goto supply0_failed;
+	}
+
+	ret = power_supply_register(&pdev->dev, &pda_power_supplies[1]);
+	if (ret) {
+		printk(KERN_ERR "pda_power: failed to register %s power "
+		       "supply\n", pda_power_supplies[1].name);
+		goto supply1_failed;
+	}
+
+	if (ac_irq) {
+		ret = request_irq(ac_irq->start, power_changed_isr,
+		           get_irq_flags(ac_irq), ac_irq->name, NULL);
+		if (ret) {
+			printk(KERN_ERR "pda_power: request ac irq failed\n");
+			goto ac_irq_failed;
+		}
+	}
+
+	if (usb_irq) {
+		ret = request_irq(usb_irq->start, power_changed_isr,
+		           get_irq_flags(ac_irq), usb_irq->name, NULL);
+		if (ret) {
+			printk(KERN_ERR "pda_power: request usb irq failed\n");
+			goto usb_irq_failed;
+		}
+	}
+
+	pdata = pdev->dev.platform_data;
+
+	update_charger();
+
+	goto success;
+
+usb_irq_failed:
+	if (ac_irq)
+		free_irq(ac_irq->start, NULL);
+ac_irq_failed:
+	power_supply_unregister(&pda_power_supplies[1]);
+supply1_failed:
+	power_supply_unregister(&pda_power_supplies[0]);
+supply0_failed:
+noirqs:
+wrongid:
+success:
+	return ret;
+}
+
+static int pda_power_remove(struct platform_device *pdev)
+{
+	if (usb_irq)
+		free_irq(usb_irq->start, NULL);
+	if (ac_irq)
+		free_irq(ac_irq->start, NULL);
+	power_supply_unregister(&pda_power_supplies[1]);
+	power_supply_unregister(&pda_power_supplies[0]);
+	return 0;
+}
+
+static struct platform_driver pda_power_pdrv = {
+	.driver = {
+		.name = "pda-power",
+	},
+	.probe = pda_power_probe,
+	.remove = pda_power_remove,
+};
+
+static int __init pda_power_init(void)
+{
+	return platform_driver_register(&pda_power_pdrv);
+}
+
+static void __exit pda_power_exit(void)
+{
+	platform_driver_unregister(&pda_power_pdrv);
+	return;
+}
+
+module_init(pda_power_init);
+module_exit(pda_power_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Anton Vorontsov <cbou@mail.ru>");
diff --git a/include/linux/pda_power.h b/include/linux/pda_power.h
new file mode 100644
index 0000000..12d46dd
--- /dev/null
+++ b/include/linux/pda_power.h
@@ -0,0 +1,27 @@
+/*
+ * Common power driver for PDAs and phones with one or two external
+ * power supplies (AC/USB) connected to main and backup batteries,
+ * and optional builtin charger.
+ *
+ * Copyright 2007 Anton Vorontsov <cbou@mail.ru>
+ *
+ * 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.
+ */
+
+#ifndef __PDA_POWER_H__
+#define __PDA_POWER_H__
+
+#include <linux/external_power.h>
+
+#define PDA_POWER_CHARGE_AC  (1 << 0)
+#define PDA_POWER_CHARGE_USB (1 << 1)
+
+struct pda_power_pdata {
+	int (*is_ac_online)(void);
+	int (*is_usb_online)(void);
+	void (*set_charge)(int flags);
+};
+
+#endif /* __PDA_POWER_H__ */
-- 
1.5.0.5-dirty

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
@ 2007-04-13  6:06 David Brownell
  2007-04-13  7:36 ` Anton Vorontsov
  2007-05-07  1:39 ` David Brownell
  0 siblings, 2 replies; 12+ messages in thread
From: David Brownell @ 2007-04-13  6:06 UTC (permalink / raw)
  To: Anton Vorontsov; +Cc: Linux Kernel list

> This driver used to stop code/logic duplication through different
> machines we porting at handhelds.org. pda_power register machs' power
> supplies, and will take care about notifying batteries about power
> changes through external power interface.

It gets USB power management wrong though.  Have a look at two I2C drivers
in drivers/i2c/chips:  tps65010.c (which would talk to this API) and then
isp1301_omap.c (which would talk to the tps65010 driver).  The tps65010
chip accepts the same two power sources you're addressing here, as part
of its battery charging responsibilities. [1]

Key points:

 - The API needs to say *how much power* can be drawn.  Common values
   are 8 mA (OTG peripherals before configuration), 100 mA (non-OTG ones
   before configuration), 500 mA (high power configurations) ... but the
   exact value depends on what's listed in the configuration descriptor
   for the chosen configuration, any value 0..500 mA (increments of two)
   could be appropriate.

 - Sensing VBUS power is not the same thing as being allowed to consume
   it.  Again, USB OTG devices are different:  OTG hosts **SUPPLY** the
   current, so you really don't want to be telling the OTG transceiver to
   fire up its charge pump (say, 3.0V VBAT converted to 5V VBUS) while at
   the same time telling the battery manager to use that battery-derived
   VBUS current to recharge its battery!  Not only would that waste power,
   but it also deprives the peripheral of the power it needs to draw.

In general, no component other than the USB peripheral (or host) controller
driver has any business trying to control how VBUS power is used.  It's
likely to get it wrong ... as shown by this patch.  ;)

And that's exactly why the USB gadget API has had calls to manage the USB
power consumption since mid-2004.  And I wonder why H5000 code evidently
doesn't implement those calls.

- Dave

[1] You may find http://www.linux-usb.org/gadget/h2-otg.html useful too.
    It gives a high level map of the complicated OTG case, including
    those drivers, and points out how simpler peripheral-only systems
    could behave with the same drivers.  However it does turn out to
    be useful if USB peripheral drivers have a "transciever" notion
    that can receive "VBUS present" interrupts, letting the peripheral
    controller driver power down almost *everything* to save power,
    and that otg_transceiver interface does that job.
 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13  6:06 David Brownell
@ 2007-04-13  7:36 ` Anton Vorontsov
  2007-04-13  8:42   ` David Brownell
  2007-05-07  1:39 ` David Brownell
  1 sibling, 1 reply; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-13  7:36 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list

Hello David,

On Thu, Apr 12, 2007 at 11:06:46PM -0700, David Brownell wrote:
> > This driver used to stop code/logic duplication through different
> > machines we porting at handhelds.org. pda_power register machs' power
> > supplies, and will take care about notifying batteries about power
> > changes through external power interface.
> 
> It gets USB power management wrong though.  Have a look at two I2C drivers
> in drivers/i2c/chips:  tps65010.c (which would talk to this API) and then
> isp1301_omap.c (which would talk to the tps65010 driver).  The tps65010
> chip accepts the same two power sources you're addressing here, as part
> of its battery charging responsibilities. [1]
> 
> Key points:
> 
>  - The API needs to say *how much power* can be drawn.  Common values
>    are 8 mA (OTG peripherals before configuration), 100 mA (non-OTG ones
>    before configuration), 500 mA (high power configurations) ... but the
>    exact value depends on what's listed in the configuration descriptor
>    for the chosen configuration, any value 0..500 mA (increments of two)
>    could be appropriate.
> 
>  - Sensing VBUS power is not the same thing as being allowed to consume
>    it.  Again, USB OTG devices are different:  OTG hosts **SUPPLY** the
>    current, so you really don't want to be telling the OTG transceiver to
>    fire up its charge pump (say, 3.0V VBAT converted to 5V VBUS) while at
>    the same time telling the battery manager to use that battery-derived
>    VBUS current to recharge its battery!  Not only would that waste power,
>    but it also deprives the peripheral of the power it needs to draw.
> 
> In general, no component other than the USB peripheral (or host) controller
> driver has any business trying to control how VBUS power is used.  It's
> likely to get it wrong ... as shown by this patch.  ;)
> 
> And that's exactly why the USB gadget API has had calls to manage the USB
> power consumption since mid-2004.  And I wonder why H5000 code evidently
> doesn't implement those calls.
> 
> - Dave
> 
> [1] You may find http://www.linux-usb.org/gadget/h2-otg.html useful too.
>     It gives a high level map of the complicated OTG case, including
>     those drivers, and points out how simpler peripheral-only systems
>     could behave with the same drivers.  However it does turn out to
>     be useful if USB peripheral drivers have a "transciever" notion
>     that can receive "VBUS present" interrupts, letting the peripheral
>     controller driver power down almost *everything* to save power,
>     and that otg_transceiver interface does that job.

I see. Current devices I have just consumes power from USB host. I.e.
they able to boot using only USB and discharged battery. :-/ Even
more, HP iPaq hx4700 able to charge battery from USB (using that driver).
We just setting USB charging GPIO, and it starts consume power from
the host.

But I got the point, and yes I can't explain why it works correctly.

(1) Anyway, you're hinting that you'd want to see some
"usb_vbus_start_consume_power(how much)" callback, which will be
handled by gadget driver? Or it should just start using some gadget api?
Or even pda_battery must become an "usb power supplicant" gadget itself
to consume power by law?

-

I'm stuck in null-modem century, so don't wonder my USB-dumbness.
Will read http://www.linux-usb.org/gadget/h2-otg.html, of course.

Though I'd appreciate answers for (1), thus I can think in right
direction from the start.

Much thanks for your comments.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13  7:36 ` Anton Vorontsov
@ 2007-04-13  8:42   ` David Brownell
  2007-04-13  9:52     ` Anton Vorontsov
  0 siblings, 1 reply; 12+ messages in thread
From: David Brownell @ 2007-04-13  8:42 UTC (permalink / raw)
  To: cbou; +Cc: Linux Kernel list

On Friday 13 April 2007 12:36 am, Anton Vorontsov wrote:
> Hello David,
> > 
> >  - The API needs to say *how much power* can be drawn...
> > 
> >  - Sensing VBUS power is not the same thing as being allowed to consume
> >    it.  Again, USB OTG devices are different:  OTG hosts **SUPPLY** the
> >    current, ...
> 
> I see. Current devices I have just consumes power from USB host. I.e.
> they able to boot using only USB and discharged battery. :-/ Even
> more, HP iPaq hx4700 able to charge battery from USB (using that driver).
> We just setting USB charging GPIO, and it starts consume power from
> the host.

Sounds like it could be more power than the host expects it to consume;
or in some cases, not as much as it could ... ISTR only the Ethernet
gadget defaults to 100mA (in non-OTG configs), the others are set for
only 2 mA (expecting self-powered configs).


> But I got the point, and yes I can't explain why it works correctly.

It probably doesn't work correctly.  But it's not broken enough to
fail badly.

 
> (1) Anyway, you're hinting that you'd want to see some
> "usb_vbus_start_consume_power(how much)" callback, which will be
> handled by gadget driver? 

The gadget API includes usb_gadget_vbus_draw(gadget, mA) which the
gadget drivers (e.g. network, storage, tty) issue.  How a given board
implements that is best wrapped in a transceiver abstraction, which
would also issue the usb_gadget_vbus_{connect,disconnect}(gadget)
notifications back to the controller.

That is, in the pure-peripheral case there are at three entities involved
outside the battery/power framework:  the gadget driver (what protocol is
used over USB), the peripheral controller driver (talking to USB hardware),
and the transceiver (sensing VBUS and managing power state transitions of
that controller).

In dumb cases (most reference boards!) the transceiver is stubbed
out, and there's no power management either to put the USB stuff
into lowpower mode when it's inactive, or to make use of VBUS power
to help manage the battery (what battery?).  In less dumb cases,
VBUS is only used to sense whether the host is there.  In vaguely
smart cases VBUS will be used to power the USB hardware (saving
maybe 40 mA of battery power in one case).  Recharging the battery
is for some reason not always supported even in product boards.


> Or it should just start using some gadget api? 
> Or even pda_battery must become an "usb power supplicant" gadget itself
> to consume power by law?

I don't know about this "supplicant" thing.  That's actually below
what the gadget API exposes -- implementation detail, but you're
thinking about how to implement such board-specific details.  I
guess I'd think that's a board-specific detail in the same way
that the transceiver is; you'll observe isp1301_omap is where
such things hook up in that example implementation.

- Dave



> I'm stuck in null-modem century, so don't wonder my USB-dumbness.
> Will read http://www.linux-usb.org/gadget/h2-otg.html, of course.
> 
> Though I'd appreciate answers for (1), thus I can think in right
> direction from the start.
> 
> Much thanks for your comments.
> 
> -- 
> Anton Vorontsov
> email: cbou@mail.ru
> backup email: ya-cbou@yandex.ru
> irc://irc.freenode.org/bd2
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13  8:42   ` David Brownell
@ 2007-04-13  9:52     ` Anton Vorontsov
  2007-04-13 10:25       ` David Brownell
  0 siblings, 1 reply; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-13  9:52 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list

On Fri, Apr 13, 2007 at 01:42:36AM -0700, David Brownell wrote:
> On Friday 13 April 2007 12:36 am, Anton Vorontsov wrote:
> > Hello David,
> > > 
> > >  - The API needs to say *how much power* can be drawn...
> > > 
> > >  - Sensing VBUS power is not the same thing as being allowed to consume
> > >    it.  Again, USB OTG devices are different:  OTG hosts **SUPPLY** the
> > >    current, ...
> > 
> > I see. Current devices I have just consumes power from USB host. I.e.
> > they able to boot using only USB and discharged battery. :-/ Even
> > more, HP iPaq hx4700 able to charge battery from USB (using that driver).
> > We just setting USB charging GPIO, and it starts consume power from
> > the host.
> 
> Sounds like it could be more power than the host expects it to consume;
> or in some cases, not as much as it could ... ISTR only the Ethernet
> gadget defaults to 100mA (in non-OTG configs), the others are set for
> only 2 mA (expecting self-powered configs).
> 
> 
> > But I got the point, and yes I can't explain why it works correctly.
> 
> It probably doesn't work correctly.  But it's not broken enough to
> fail badly.

Can that comment be an explanation?

--- drivers/usb/gadget/pxa2xx_udc.c:
static const struct usb_gadget_ops pxa2xx_udc_ops = {
        .get_frame      = pxa2xx_udc_get_frame,
        .wakeup         = pxa2xx_udc_wakeup,
        .vbus_session   = pxa2xx_udc_vbus_session,
        .pullup         = pxa2xx_udc_pullup,

        // .vbus_draw ... boards may consume current from VBUS, up to
        // 100-500mA based on config.  the 500uA suspend ceiling means
        // that exclusively vbus-powered PXA designs violate USB specs.
};


Comparing to omap_udc.

--- drivers/usb/gadget/omap_udc.c
static int omap_vbus_draw(struct usb_gadget *gadget, unsigned mA)
{
        struct omap_udc *udc;

        udc = container_of(gadget, struct omap_udc, gadget);
        if (udc->transceiver)
                return otg_set_power(udc->transceiver, mA);
        return -EOPNOTSUPP;
}
[...]
static struct usb_gadget_ops omap_gadget_ops = {
        .get_frame              = omap_get_frame,
        .wakeup                 = omap_wakeup,
        .set_selfpowered        = omap_set_selfpowered,
        .vbus_session           = omap_vbus_session,
        .vbus_draw              = omap_vbus_draw,
        .pullup                 = omap_pullup,
};



Regarding API. If you all you want is to know how much power you need to
ask from VBUS, we can extend external power interface... thus suppliers
could ask their power consumption requirements in mA/uA, and these
requests will be forwarded to power supply driver, and power driver will
forward that request to USB transceiver (via platform hook).

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13  9:52     ` Anton Vorontsov
@ 2007-04-13 10:25       ` David Brownell
  2007-04-13 11:30         ` Anton Vorontsov
  0 siblings, 1 reply; 12+ messages in thread
From: David Brownell @ 2007-04-13 10:25 UTC (permalink / raw)
  To: cbou; +Cc: Linux Kernel list

On Friday 13 April 2007 2:52 am, Anton Vorontsov wrote:
> > > But I got the point, and yes I can't explain why it works correctly.
> > 
> > It probably doesn't work correctly.  But it's not broken enough to
> > fail badly.
> 
> Can that comment be an explanation?
> 
> --- drivers/usb/gadget/pxa2xx_udc.c:
> static const struct usb_gadget_ops pxa2xx_udc_ops = {
>         .get_frame      = pxa2xx_udc_get_frame,
>         .wakeup         = pxa2xx_udc_wakeup,
>         .vbus_session   = pxa2xx_udc_vbus_session,
>         .pullup         = pxa2xx_udc_pullup,
> 
>         // .vbus_draw ... boards may consume current from VBUS, up to
>         // 100-500mA based on config.  the 500uA suspend ceiling means
>         // that exclusively vbus-powered PXA designs violate USB specs.

That's basically a "plug in implementation here".  Nobody's yet done
that on a platform that _uses_ the VBUS power.


> };
> 
> 
> Comparing to omap_udc.
> 
> --- drivers/usb/gadget/omap_udc.c
> static int omap_vbus_draw(struct usb_gadget *gadget, unsigned mA)
> {
>         struct omap_udc *udc;
> 
>         udc = container_of(gadget, struct omap_udc, gadget);
>         if (udc->transceiver)
>                 return otg_set_power(udc->transceiver, mA);

Where the transceiver would then delegate to something else,
like the tps65010 driver.


>         return -EOPNOTSUPP;
> }
> [...]
> static struct usb_gadget_ops omap_gadget_ops = {
>         .get_frame              = omap_get_frame,
>         .wakeup                 = omap_wakeup,
>         .set_selfpowered        = omap_set_selfpowered,
>         .vbus_session           = omap_vbus_session,
>         .vbus_draw              = omap_vbus_draw,

... which has most certainly been on platforms which are hooked
up to draw power from VBUS.


>         .pullup                 = omap_pullup,
> };
> 
> 
> 
> Regarding API. If you all you want is to know how much power you need to
> ask from VBUS, we can extend external power interface... thus suppliers
> could ask their power consumption requirements in mA/uA, and these
> requests will be forwarded to power supply driver, and power driver will
> forward that request to USB transceiver (via platform hook).

I don't folow what you're saying.  The control flow *MUST* be that
the USB stack provides the only indication of how much power may
be drawn through the VBUS supply.  Nothing else in the system has
the knowledge of what's legal, and when.

If you want to talk about a "supplier", the way to put it might
then be that the USB stack is saying "here's N mA power for you";
it's supplying the power, not the other way around.

There's no "ask" involved, since the host controls "N".  So the
host supplies, the USB gadget stack interprets that, and some
power component must obeys.  That includes rules like reducing
VBUS draw to ~500 uA when the host suspends the USB link.

- Dave


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13 10:25       ` David Brownell
@ 2007-04-13 11:30         ` Anton Vorontsov
  0 siblings, 0 replies; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-13 11:30 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list

On Fri, Apr 13, 2007 at 03:25:13AM -0700, David Brownell wrote:
> On Friday 13 April 2007 2:52 am, Anton Vorontsov wrote:
> > > > But I got the point, and yes I can't explain why it works correctly.
> > > 
> > > It probably doesn't work correctly.  But it's not broken enough to
> > > fail badly.
> > 
> > Can that comment be an explanation?
> > 
> > --- drivers/usb/gadget/pxa2xx_udc.c:
> > static const struct usb_gadget_ops pxa2xx_udc_ops = {
> >         .get_frame      = pxa2xx_udc_get_frame,
> >         .wakeup         = pxa2xx_udc_wakeup,
> >         .vbus_session   = pxa2xx_udc_vbus_session,
> >         .pullup         = pxa2xx_udc_pullup,
> > 
> >         // .vbus_draw ... boards may consume current from VBUS, up to
> >         // 100-500mA based on config.  the 500uA suspend ceiling means
> >         // that exclusively vbus-powered PXA designs violate USB specs.
> 
> That's basically a "plug in implementation here".  Nobody's yet done
> that on a platform that _uses_ the VBUS power.
> 
> 
> > };
> > 
> > 
> > Comparing to omap_udc.
> > 
> > --- drivers/usb/gadget/omap_udc.c
> > static int omap_vbus_draw(struct usb_gadget *gadget, unsigned mA)
> > {
> >         struct omap_udc *udc;
> > 
> >         udc = container_of(gadget, struct omap_udc, gadget);
> >         if (udc->transceiver)
> >                 return otg_set_power(udc->transceiver, mA);
> 
> Where the transceiver would then delegate to something else,
> like the tps65010 driver.
> 
> 
> >         return -EOPNOTSUPP;
> > }
> > [...]
> > static struct usb_gadget_ops omap_gadget_ops = {
> >         .get_frame              = omap_get_frame,
> >         .wakeup                 = omap_wakeup,
> >         .set_selfpowered        = omap_set_selfpowered,
> >         .vbus_session           = omap_vbus_session,
> >         .vbus_draw              = omap_vbus_draw,
> 
> .... which has most certainly been on platforms which are hooked
> up to draw power from VBUS.
> 
> 
> >         .pullup                 = omap_pullup,
> > };
> > 
> > 
> > 
> > Regarding API. If you all you want is to know how much power you need to
> > ask from VBUS, we can extend external power interface... thus suppliers
> > could ask their power consumption requirements in mA/uA, and these
> > requests will be forwarded to power supply driver, and power driver will
> > forward that request to USB transceiver (via platform hook).
> 
> I don't folow what you're saying.  The control flow *MUST* be that
> the USB stack provides the only indication of how much power may
> be drawn through the VBUS supply.  Nothing else in the system has
> the knowledge of what's legal, and when.
> 
> If you want to talk about a "supplier", the way to put it might
> then be that the USB stack is saying "here's N mA power for you";
> it's supplying the power, not the other way around.
> 
> There's no "ask" involved, since the host controls "N".  So the
> host supplies, the USB gadget stack interprets that, and some
> power component must obeys.  That includes rules like reducing
> VBUS draw to ~500 uA when the host suspends the USB link.

This framework (pda_power) support ability to charge from USB. But
it is only dispatcher, which implements logic. Issues like what
exact mA to draw, and how all that communicate to USB stack, left
to specific devices and/or additional drivers, users of framework.

In this regard, "USB" is just a label. Can be replaced with
"alternative power source".

> - Dave

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-11 23:24 [PATCH 2/7] [RFC] Common power driver for Linux gadgets Anton Vorontsov
@ 2007-04-13 13:50 ` Anton Vorontsov
  2007-04-16 20:16   ` Russell King
  0 siblings, 1 reply; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-13 13:50 UTC (permalink / raw)
  To: linux-kernel; +Cc: kernel-discuss

On Thu, Apr 12, 2007 at 03:24:56AM +0400, Anton Vorontsov wrote:
> This driver used to stop code/logic duplication through different
> machines we porting at handhelds.org. pda_power register machs' power
> supplies, and will take care about notifying batteries about power
> changes through external power interface.
> 
> This driver should be suitable for almost every Linux gadget today.

Changes:

- implement timer, it's used for two purposes:
   1) on some machines you can't read is_{ac,usb}_online() values just
      after interrupt. Should wait a bit to read reliable values.
   2) irq debouncing

- cleanups


Subject: [PATCH] [take2] pda_power driver


Signed-off-by: Anton Vorontsov <cbou@mail.ru>
---
 drivers/power/Kconfig     |    8 ++
 drivers/power/Makefile    |    1 +
 drivers/power/pda_power.c |  231 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pda_power.h |   25 +++++
 4 files changed, 265 insertions(+), 0 deletions(-)
 create mode 100644 drivers/power/pda_power.c
 create mode 100644 include/linux/pda_power.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 17349c1..b87779e 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -10,4 +10,12 @@ config EXTERNAL_POWER
 
 	  This interface is mandatory for battery class support.
 
+config PDA_POWER
+	tristate "Generic PDA/phone power driver"
+	depends on EXTERNAL_POWER
+	help
+	  Say Y here to enable generic power driver for PDAs and phones with
+	  one or two external power supplies (AC/USB) connected to main and
+	  backup batteries, and optional builtin charger.
+
 endmenu
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index c303b45..6f084e7 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_EXTERNAL_POWER)  += external_power.o
+obj-$(CONFIG_PDA_POWER)       += pda_power.o
diff --git a/drivers/power/pda_power.c b/drivers/power/pda_power.c
new file mode 100644
index 0000000..ae90bd7
--- /dev/null
+++ b/drivers/power/pda_power.c
@@ -0,0 +1,231 @@
+/*
+ * Common power driver for PDAs and phones with one or two external
+ * power supplies (AC/USB) connected to main and backup batteries,
+ * and optional builtin charger.
+ *
+ * Copyright 2007 Anton Vorontsov <cbou@mail.ru>
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/external_power.h>
+#include <linux/pda_power.h>
+#include <linux/timer.h>
+
+/*
+ * include/linux/ioport.h does not provide flags for generic IRQ trigger
+ * types. So, we're using "ISA PnP IRQ specific bits", and converting them.
+ */
+static unsigned int get_irq_flags(struct resource *res)
+{
+	unsigned int flags = IRQF_DISABLED;
+
+	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
+		flags |= IRQF_TRIGGER_RISING;
+	if (res->flags & IORESOURCE_IRQ_LOWEDGE)
+		flags |= IRQF_TRIGGER_FALLING;
+	if (res->flags & IORESOURCE_IRQ_HIGHLEVEL)
+		flags |= IRQF_TRIGGER_HIGH;
+	if (res->flags & IORESOURCE_IRQ_LOWLEVEL)
+		flags |= IRQF_TRIGGER_LOW;
+	if (res->flags & IORESOURCE_IRQ_SHAREABLE)
+		flags |= IRQF_SHARED;
+
+	return flags;
+}
+
+static struct resource *ac_irq, *usb_irq;
+static struct pda_power_pdata *pdata;
+static struct timer_list isr_timer;
+
+static int pda_power_is_ac_online(struct power_supply *psy)
+{
+	return pdata->is_ac_online ? pdata->is_ac_online() : 0;
+}
+
+static int pda_power_is_usb_online(struct power_supply *psy)
+{
+	return pdata->is_usb_online ? pdata->is_usb_online() : 0;
+}
+
+static char *pda_power_supplied_to[] = {
+	"main-battery",
+	"backup-battery",
+};
+
+static struct power_supply pda_power_supplies[] = {
+	{
+		.name = "ac",
+		.type = "ac",
+		.supplied_to = pda_power_supplied_to,
+		.num_supplicants = ARRAY_SIZE(pda_power_supplied_to),
+		.is_online = pda_power_is_ac_online,
+	},
+	{
+		.name = "usb",
+		.type = "dc",
+		.supplied_to = pda_power_supplied_to,
+		.num_supplicants = ARRAY_SIZE(pda_power_supplied_to),
+		.is_online = pda_power_is_usb_online,
+	},
+};
+
+static void update_charger(void)
+{
+	if (!pdata->set_charge)
+		return;
+
+	if (pdata->is_ac_online && pdata->is_ac_online()) {
+		pr_debug("pda_power: charger on (AC)\n");
+		pdata->set_charge(PDA_POWER_CHARGE_AC);
+	}
+	else if (pdata->is_usb_online && pdata->is_usb_online()) {
+		pr_debug("pda_power: charger on (USB)\n");
+		pdata->set_charge(PDA_POWER_CHARGE_USB);
+	}
+	else {
+		pr_debug("pda_power: charger off\n");
+		pdata->set_charge(0);
+	}
+
+	return;
+}
+
+static void isr_timer_func(unsigned long irq)
+{
+	if (ac_irq && irq == ac_irq->start) {
+		power_supply_changed(&pda_power_supplies[0]);
+	}
+	else if (usb_irq && irq == usb_irq->start) {
+		power_supply_changed(&pda_power_supplies[1]);
+	}
+	else {
+		printk(KERN_WARNING "pda_power: spurious irq %li", irq);
+		return;
+	}
+
+	update_charger();
+
+	return;
+}
+
+static irqreturn_t power_changed_isr(int irq, void *unused)
+{
+	isr_timer.data = irq;
+	mod_timer(&isr_timer, jiffies + HZ);
+	return IRQ_HANDLED;
+}
+
+static int pda_power_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+
+	if (pdev->id != -1) {
+		printk(KERN_WARNING "pda_power: it's meaningless to "
+		       "register several pda_powers, use id = -1\n");
+		ret = -EINVAL;
+		goto wrongid;
+	}
+
+	pdata = pdev->dev.platform_data;
+
+	setup_timer(&isr_timer, isr_timer_func, 0);
+
+	ac_irq = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "ac");
+	usb_irq = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "usb");
+	if (!ac_irq && !usb_irq) {
+		printk(KERN_ERR "pda_power: no ac/usb irq specified\n");
+		ret = -ENODEV;
+		goto noirqs;
+	}
+
+	ret = power_supply_register(&pdev->dev, &pda_power_supplies[0]);
+	if (ret) {
+		printk(KERN_ERR "pda_power: failed to register %s power "
+		       "supply\n", pda_power_supplies[0].name);
+		goto supply0_failed;
+	}
+
+	ret = power_supply_register(&pdev->dev, &pda_power_supplies[1]);
+	if (ret) {
+		printk(KERN_ERR "pda_power: failed to register %s power "
+		       "supply\n", pda_power_supplies[1].name);
+		goto supply1_failed;
+	}
+
+	if (ac_irq) {
+		ret = request_irq(ac_irq->start, power_changed_isr,
+		           get_irq_flags(ac_irq), ac_irq->name, NULL);
+		if (ret) {
+			printk(KERN_ERR "pda_power: request ac irq failed\n");
+			goto ac_irq_failed;
+		}
+	}
+
+	if (usb_irq) {
+		ret = request_irq(usb_irq->start, power_changed_isr,
+		           get_irq_flags(ac_irq), usb_irq->name, NULL);
+		if (ret) {
+			printk(KERN_ERR "pda_power: request usb irq failed\n");
+			goto usb_irq_failed;
+		}
+	}
+
+	update_charger();
+
+	goto success;
+
+usb_irq_failed:
+	if (ac_irq)
+		free_irq(ac_irq->start, NULL);
+ac_irq_failed:
+	power_supply_unregister(&pda_power_supplies[1]);
+supply1_failed:
+	power_supply_unregister(&pda_power_supplies[0]);
+supply0_failed:
+noirqs:
+wrongid:
+success:
+	return ret;
+}
+
+static int pda_power_remove(struct platform_device *pdev)
+{
+	if (usb_irq)
+		free_irq(usb_irq->start, NULL);
+	if (ac_irq)
+		free_irq(ac_irq->start, NULL);
+	del_timer_sync(&isr_timer);
+	power_supply_unregister(&pda_power_supplies[1]);
+	power_supply_unregister(&pda_power_supplies[0]);
+	return 0;
+}
+
+static struct platform_driver pda_power_pdrv = {
+	.driver = {
+		.name = "pda-power",
+	},
+	.probe = pda_power_probe,
+	.remove = pda_power_remove,
+};
+
+static int __init pda_power_init(void)
+{
+	return platform_driver_register(&pda_power_pdrv);
+}
+
+static void __exit pda_power_exit(void)
+{
+	platform_driver_unregister(&pda_power_pdrv);
+	return;
+}
+
+module_init(pda_power_init);
+module_exit(pda_power_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Anton Vorontsov <cbou@mail.ru>");
diff --git a/include/linux/pda_power.h b/include/linux/pda_power.h
new file mode 100644
index 0000000..0d414a8
--- /dev/null
+++ b/include/linux/pda_power.h
@@ -0,0 +1,25 @@
+/*
+ * Common power driver for PDAs and phones with one or two external
+ * power supplies (AC/USB) connected to main and backup batteries,
+ * and optional builtin charger.
+ *
+ * Copyright 2007 Anton Vorontsov <cbou@mail.ru>
+ *
+ * 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.
+ */
+
+#ifndef __PDA_POWER_H__
+#define __PDA_POWER_H__
+
+#define PDA_POWER_CHARGE_AC  (1 << 0)
+#define PDA_POWER_CHARGE_USB (1 << 1)
+
+struct pda_power_pdata {
+	int (*is_ac_online)(void);
+	int (*is_usb_online)(void);
+	void (*set_charge)(int flags);
+};
+
+#endif /* __PDA_POWER_H__ */
-- 
1.5.0.5-dirty


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13 13:50 ` Anton Vorontsov
@ 2007-04-16 20:16   ` Russell King
  2007-04-16 20:43     ` Anton Vorontsov
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King @ 2007-04-16 20:16 UTC (permalink / raw)
  To: Anton Vorontsov; +Cc: linux-kernel, kernel-discuss

On Fri, Apr 13, 2007 at 05:50:01PM +0400, Anton Vorontsov wrote:
> +/*
> + * include/linux/ioport.h does not provide flags for generic IRQ trigger
> + * types. So, we're using "ISA PnP IRQ specific bits", and converting them.
> + */
> +static unsigned int get_irq_flags(struct resource *res)
> +{
> +	unsigned int flags = IRQF_DISABLED;
> +
> +	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
> +		flags |= IRQF_TRIGGER_RISING;
> +	if (res->flags & IORESOURCE_IRQ_LOWEDGE)
> +		flags |= IRQF_TRIGGER_FALLING;
> +	if (res->flags & IORESOURCE_IRQ_HIGHLEVEL)
> +		flags |= IRQF_TRIGGER_HIGH;
> +	if (res->flags & IORESOURCE_IRQ_LOWLEVEL)
> +		flags |= IRQF_TRIGGER_LOW;
> +	if (res->flags & IORESOURCE_IRQ_SHAREABLE)
> +		flags |= IRQF_SHARED;
> +
> +	return flags;
> +}

Eww.  The IORESOURCE IRQ bits are intentionally chosen to be the same as
the IRQF bits:

include/linux/interrupt.h:
/*
 * These correspond to the IORESOURCE_IRQ_* defines in
 * linux/ioport.h to select the interrupt line behaviour.  When
 * requesting an interrupt without specifying a IRQF_TRIGGER, the
 * setting should be assumed to be "as already configured", which
 * may be as per machine or firmware initialisation.
 */

The exception is IRQF_SHARED which should be a _driver_ choice not a
_platform_ choice, and therefore makes no sense in your "get_irq_flags"
definition.

Plus, if we ever did want to introduce such a function, it should be
a generic thing, not specific to some random power subsystem de jour.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-16 20:16   ` Russell King
@ 2007-04-16 20:43     ` Anton Vorontsov
  0 siblings, 0 replies; 12+ messages in thread
From: Anton Vorontsov @ 2007-04-16 20:43 UTC (permalink / raw)
  To: linux-kernel, kernel-discuss

On Mon, Apr 16, 2007 at 09:16:15PM +0100, Russell King wrote:
> On Fri, Apr 13, 2007 at 05:50:01PM +0400, Anton Vorontsov wrote:
> > +/*
> > + * include/linux/ioport.h does not provide flags for generic IRQ trigger
> > + * types. So, we're using "ISA PnP IRQ specific bits", and converting them.
> > + */
> > +static unsigned int get_irq_flags(struct resource *res)
> > +{
> > +	unsigned int flags = IRQF_DISABLED;
> > +
> > +	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
> > +		flags |= IRQF_TRIGGER_RISING;
> > +	if (res->flags & IORESOURCE_IRQ_LOWEDGE)
> > +		flags |= IRQF_TRIGGER_FALLING;
> > +	if (res->flags & IORESOURCE_IRQ_HIGHLEVEL)
> > +		flags |= IRQF_TRIGGER_HIGH;
> > +	if (res->flags & IORESOURCE_IRQ_LOWLEVEL)
> > +		flags |= IRQF_TRIGGER_LOW;
> > +	if (res->flags & IORESOURCE_IRQ_SHAREABLE)
> > +		flags |= IRQF_SHARED;
> > +
> > +	return flags;
> > +}
> 
> Eww.  The IORESOURCE IRQ bits are intentionally chosen to be the same as
> the IRQF bits:

> include/linux/interrupt.h:
> /*
>  * These correspond to the IORESOURCE_IRQ_* defines in

Yeah, few days ago I noticed it myself, already fixed it in my tree.
Thanks anyway.

>  * linux/ioport.h to select the interrupt line behaviour.  When
>  * requesting an interrupt without specifying a IRQF_TRIGGER, the
>  * setting should be assumed to be "as already configured", which
>  * may be as per machine or firmware initialisation.
>  */
> 
> The exception is IRQF_SHARED which should be a _driver_ choice not a
> _platform_ choice, and therefore makes no sense in your "get_irq_flags"
> definition.

Ugh, I see...

> Plus, if we ever did want to introduce such a function, it should be
> a generic thing, not specific to some random power subsystem de jour.
> 
> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:
> 

Thanks for comments.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-04-13  6:06 David Brownell
  2007-04-13  7:36 ` Anton Vorontsov
@ 2007-05-07  1:39 ` David Brownell
  2007-05-07  2:40   ` Anton Vorontsov
  1 sibling, 1 reply; 12+ messages in thread
From: David Brownell @ 2007-05-07  1:39 UTC (permalink / raw)
  To: Anton Vorontsov; +Cc: Linux Kernel list

On Thursday 12 April 2007, David Brownell wrote:
> > This driver used to stop code/logic duplication through different
> > machines we porting at handhelds.org. pda_power register machs' power
> > supplies, and will take care about notifying batteries about power
> > changes through external power interface.
> 
> It gets USB power management wrong though. ...

And that seems not to have been resolved in the version you
reposted and then put into your GIT tree, either ... what's
your plan for addressing the feedback, then?


> Key points:
> 
>  - The API needs to say *how much power* can be drawn.  ...
> 
>  - Sensing VBUS power is not the same thing as being allowed to consume
>    it.  Again, USB OTG devices are different...
> 
> In general, no component other than the USB peripheral (or host) controller
> driver has any business trying to control how VBUS power is used.  It's
> likely to get it wrong ... as shown by this patch.  ;)

I see on other LKML threads that folk are spreading misinformation
about USB, like "no control signaling over USB for power control".
I sincerly hope you're not believing that's correct!


Plus, I'm wondering why this doesn't try to leverage the voltage
framework that's been posted (to the ARM list, most recently).

At some level there's not a lot different between saying "turn on
the 3V3 supply to this MMC card slot, budgeting 80mA" and saying
instead "let the system draw up to 200 mA from USB VBUS".  The
current goes in different directions in those examples, sure.  Plus
the usage of VBUS as a power _input_ is of course board-specific:
battery charging, or powering specific circuits (like USB), etc.

Then there are devices which _output_ power on VBUS of course ...
so the only difference between that and the MMC example being how
much of the power budget is consumed.  Plus devices which either
use VBUS as an input or an output, based on how they're hooked up.

All of those are differences that seem to fit better into the notion
of a general purpose voltage framework than this "power" thing
you've provided.  (Batter management being a separate issue.)


Also that whole notion that there's only a handful of power "supplies"
to manage ("ac", "main-battery", "usb") just flies in the face of
how most boards actually work -- multiple voltage rails and switches,
more types than battery/UPS/"AC"/USB, etc -- and the mechanisms needed
to manage power when it's critical to eke out every last milliWatt.

- Dave

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/7] [RFC] Common power driver for Linux gadgets
  2007-05-07  1:39 ` David Brownell
@ 2007-05-07  2:40   ` Anton Vorontsov
  0 siblings, 0 replies; 12+ messages in thread
From: Anton Vorontsov @ 2007-05-07  2:40 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list

On Sun, May 06, 2007 at 06:39:27PM -0700, David Brownell wrote:
> On Thursday 12 April 2007, David Brownell wrote:
> > > This driver used to stop code/logic duplication through different
> > > machines we porting at handhelds.org. pda_power register machs' power
> > > supplies, and will take care about notifying batteries about power
> > > changes through external power interface.
> > 
> > It gets USB power management wrong though. ...
> 
> And that seems not to have been resolved in the version you
> reposted and then put into your GIT tree, either ... what's
> your plan for addressing the feedback, then?

pda_power have nothing to do with USB at all, as I've said. It
*monitors* MAINS/USB power supply state through *platform hooks*, and
platform hooks will do things as caring how much current to draw, if
charger used.

There is nothing wrong with pda_power. If you're hinting that for
pda_power we can use not some platform hooks, but some generic
USB/UDC/OTG/whatever api - okay, patches are welcome. I'm not usb
expert.

Please, speak code, not USB words. I don't understand USB language.
All I've understood, that we could not get as much current as we
want/expect from USB. And I believe you.

> > Key points:
> > 
> >  - The API needs to say *how much power* can be drawn.  ...
> > 
> >  - Sensing VBUS power is not the same thing as being allowed to consume
> >    it.  Again, USB OTG devices are different...
> > 
> > In general, no component other than the USB peripheral (or host) controller
> > driver has any business trying to control how VBUS power is used.  It's
> > likely to get it wrong ... as shown by this patch.  ;)
> 
> I see on other LKML threads that folk are spreading misinformation
> about USB, like "no control signaling over USB for power control".
> I sincerly hope you're not believing that's correct!

And again, I believe you. But you're not helping.

> Plus, I'm wondering why this doesn't try to leverage the voltage
> framework that's been posted (to the ARM list, most recently).

(1)

> At some level there's not a lot different between saying "turn on
> the 3V3 supply to this MMC card slot, budgeting 80mA" and saying
> instead "let the system draw up to 200 mA from USB VBUS".  The
> current goes in different directions in those examples, sure.  Plus
> the usage of VBUS as a power _input_ is of course board-specific:
> battery charging, or powering specific circuits (like USB), etc.
> 
> Then there are devices which _output_ power on VBUS of course ...
> so the only difference between that and the MMC example being how
> much of the power budget is consumed.  Plus devices which either
> use VBUS as an input or an output, based on how they're hooked up.
> 
> All of those are differences that seem to fit better into the notion
> of a general purpose voltage framework than this "power" thing
> you've provided.  (Batter management being a separate issue.)

^^^ This

> Also that whole notion that there's only a handful of power "supplies"
> to manage ("ac", "main-battery", "usb") just flies in the face of
> how most boards actually work -- multiple voltage rails and switches,
> more types than battery/UPS/"AC"/USB, etc -- and the mechanisms needed
> to manage power when it's critical to eke out every last milliWatt.

And this ^^^ shows that you've completely misunderstood whole power
supply (was battery) class. This is mostly monitoring class, think of
hwmon. This class *monitors* what power supplies *attached* to the
board, monitors state of the power. Drivers to this class is mostly
*passive*.

pda_power is "active" in sense that it controls charger (usually
through one GPIO pin) via platform code. Not more.

And if some driver will want to be "active" (i.e. battery driver can
can control its voltage/current output), patches are welcome to do
(1), i.e. registering "voltage provider" inside power supply class.

> - Dave

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2007-05-07  2:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-04-11 23:24 [PATCH 2/7] [RFC] Common power driver for Linux gadgets Anton Vorontsov
2007-04-13 13:50 ` Anton Vorontsov
2007-04-16 20:16   ` Russell King
2007-04-16 20:43     ` Anton Vorontsov
  -- strict thread matches above, loose matches on Subject: below --
2007-04-13  6:06 David Brownell
2007-04-13  7:36 ` Anton Vorontsov
2007-04-13  8:42   ` David Brownell
2007-04-13  9:52     ` Anton Vorontsov
2007-04-13 10:25       ` David Brownell
2007-04-13 11:30         ` Anton Vorontsov
2007-05-07  1:39 ` David Brownell
2007-05-07  2:40   ` Anton Vorontsov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox