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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
