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

2009-12-30 Thread Madhusudhan


 -Original Message-
 From: Anton Vorontsov [mailto:avoront...@ru.mvista.com]
 Sent: Thursday, December 10, 2009 8:44 AM
 To: Felipe Balbi
 Cc: Grazvydas Ignotas; linux-ker...@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: cbouatmai...@gmail.com
 irc://irc.freenode.net/bd2

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-10 Thread Grazvydas Ignotas
On Thu, Dec 3, 2009 at 1:03 PM, Felipe Balbi felipe.ba...@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?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-10 Thread 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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-10 Thread 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 avoront...@ru.mvista.com for suggesting 
that.


--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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


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

2009-12-10 Thread 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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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


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

2009-12-10 Thread Grazvydas Ignotas
On Thu, Dec 10, 2009 at 6:51 PM, Felipe Balbi felipe.ba...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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


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

2009-12-03 Thread Grazvydas Ignotas
On Thu, Dec 3, 2009 at 10:39 AM, Felipe Balbi felipe.ba...@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 tpsx 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.za...@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?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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


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

2009-12-02 Thread Madhusudhan


 -Original Message-
 From: Grazvydas Ignotas [mailto:nota...@gmail.com]
 Sent: Monday, November 30, 2009 3:33 PM
 To: Madhusudhan
 Cc: linux-ker...@vger.kernel.org; Anton Vorontsov; linux-
 o...@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...@ti.com wrote:
 
 
  -Original Message-
  From: Grazvydas Ignotas [mailto:nota...@gmail.com]
  Sent: Friday, November 27, 2009 8:44 AM
  To: linux-ker...@vger.kernel.org
  Cc: Anton Vorontsov; Madhusudhan Chikkature; linux-
 o...@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 nota...@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...@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.

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread 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 000..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 nota...@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_BCIICHG0x08
+#define REG_BCIVAC 0x0a
+#define REG_BCIVBUS0x0c
+#define REG_BCIMFSTS4  0x10
+#define REG_BCICTL10x23
+
+#define REG_BOOT_BCI   0x07
+#define REG_STS_HW_CONDITIONS  0x0f
+
+#define BCIAUTOWEN 0x20
+#define CONFIG_DONE0x10
+#define CVENAC 0x04
+#define BCIAUTOUSB 0x02
+#define BCIAUTOAC  0x01
+#define BCIMSTAT_MASK  0x3F
+#define STS_VBUS   0x80
+#define STS_CHG0x02
+#define STS_USB_ID 0x04
+#define CGAIN  0x20
+#define USBFASTMCHG0x04
+
+#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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread Grazvydas Ignotas
On Wed, Dec 2, 2009 at 7:33 PM, Felipe Balbi felipe.ba...@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 000..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 nota...@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

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread Grazvydas Ignotas
On Mon, Nov 30, 2009 at 8:58 PM, Anton Vorontsov
avoront...@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: cbouatmai...@gmail.com
 irc://irc.freenode.net/bd2

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

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


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

2009-12-02 Thread 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
 avoront...@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 avoront...@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 avoront...@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

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread Grazvydas Ignotas
On Wed, Dec 2, 2009 at 10:49 PM, Felipe Balbi felipe.ba...@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..
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread Grazvydas Ignotas
On Wed, Dec 2, 2009 at 11:27 PM, Anton Vorontsov
avoront...@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
 avoront...@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 avoront...@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 avoront...@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


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread 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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread 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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2


--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-12-02 Thread 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.za...@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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-11-30 Thread Madhusudhan


 -Original Message-
 From: Grazvydas Ignotas [mailto:nota...@gmail.com]
 Sent: Friday, November 27, 2009 8:44 AM
 To: linux-ker...@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 nota...@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 000..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 nota...@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_BCIMFSTS40x10
 +#define REG_BCICTL1  0x23
 +
 +#define REG_BOOT_BCI 0x07
 +#define REG_STS_HW_CONDITIONS0x0f
 +
 +#define BCIAUTOWEN   0x20
 +#define CONFIG_DONE  0x10
 +#define CVENAC   0x04
 +#define BCIAUTOUSB   0x02
 +#define BCIAUTOAC0x01
 +#define BCIMSTAT_MASK0x3F
 +#define STS_VBUS 0x80
 +#define STS_CHG  0x02
 +#define STS_USB_ID   0x04
 +#define CGAIN0x20
 +#define USBFASTMCHG  0x04
 +
 +#define STATEC_USB   0x10
 +#define STATEC_AC0x20
 +#define STATEC_STATUS_MASK   0x0f
 +#define STATEC_QUICK10x02
 +#define STATEC_QUICK70x07
 +#define STATEC_COMPLETE1 0x0b
 +#define STATEC_COMPLETE4 0x0e
 +
 +#define BCI_DELAY100
 +
 +struct twl4030_bci_device_info {
 + struct power_supply ac;
 + struct power_supply usb;
 + struct delayed_work bat_work;
 + boolstarted;
 +};
 +
 +/*
 + * 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 = 

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

2009-11-30 Thread 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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-11-30 Thread Grazvydas Ignotas
On Mon, Nov 30, 2009 at 8:45 PM, Madhusudhan madhu...@ti.com wrote:


 -Original Message-
 From: Grazvydas Ignotas [mailto:nota...@gmail.com]
 Sent: Friday, November 27, 2009 8:44 AM
 To: linux-ker...@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 nota...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-11-27 Thread 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 nota...@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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2009-11-27 Thread Grazvydas Ignotas
On Fri, Nov 27, 2009 at 4:54 PM, Anton Vorontsov
avoront...@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 nota...@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: cbouatmai...@gmail.com
 irc://irc.freenode.net/bd2

From 3255345be7a657bcdef024d329b923dc2b64b0a5 Mon Sep 17 00:00:00 2001
From: Grazvydas Ignotas nota...@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 nota...@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 000..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 nota...@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;
+
+	

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

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