| buZz | maybe that was what i was hitting with chargers too, prevented with a usbcondom? | 00:06 |
|---|---|---|
| sicelo | Wizzup: just as an update regarding my udevadm question/request from yesterday ... the patch in https://paste.debian.net/1331047/ results in https://paste.debian.net/1331046/ | 10:44 |
| sicelo | :-) | 10:44 |
| sicelo | so we have a kernel event now on the bq2415x charger. problem for now is ... 'online' remains '0', so the event fires a bit too soon, i guess | 10:45 |
| sicelo | any ideas? freemangordon perhaps ... | 10:45 |
| sicelo | i'm tempted to do power_supply_get_property(...ONLINE..) just before calling power_supply_changed | 10:50 |
| sicelo | feels like a dirty hack, but the function is already getting property of CURRENT_MAX a few lines above where i added power_supply_changed, https://github.com/torvalds/linux/blob/master/drivers/power/supply/bq2415x_charger.c#L830-L837 | 10:51 |
| freemangordon | sicelo: I would check how the other drivers use power_supply_changed() | 11:12 |
| sicelo | i did, but might have missed something that' not too obvious, at least to me. | 11:14 |
| freemangordon | sicelo: hmm, https://github.com/torvalds/linux/blob/master/drivers/power/supply/bq2415x_charger.c#L842 | 11:17 |
| freemangordon | I think wq should call power_supply_changed() | 11:18 |
| sicelo | what's wq? or you mean bq? or something else? | 11:19 |
| freemangordon | workqueue | 11:20 |
| freemangordon | but, seems there is a bug here | 11:20 |
| freemangordon | if wq is already scheduled, that call on L842 will not re-schedule it for immediate execution afaik | 11:20 |
| sicelo | yeah i don't understand workqueues at all :p | 11:22 |
| freemangordon | see https://stackoverflow.com/questions/14965513/what-happens-when-kernel-delayed-work-is-rescheduled | 11:22 |
| sicelo | but will try to learn | 11:22 |
| sicelo | i did find that if i insert/remove charger a number of times (or it's when i do it fast?), then ONLINE reporting starts to work | 11:23 |
| freemangordon | sicelo: try instead of calling power_supply_changed, to insert cancel_delayed_work_sync(&bq->work); before line 842 | 11:27 |
| freemangordon | so it should look like: | 11:28 |
| freemangordon | schedule_delayed_work(&bq->work, 0); | 11:28 |
| freemangordon | oops, sorry | 11:28 |
| freemangordon | like: | 11:28 |
| freemangordon | cancel_delayed_work_sync(&bq->work); | 11:28 |
| freemangordon | schedule_delayed_work(&bq->work, 0); | 11:28 |
| freemangordon | or maybe use cancel_delayed_work(), not cancel_delayed_work_sync() | 11:29 |
| freemangordon | we don;t really want to wait in notifier function, IIRC it is in atomic context | 11:30 |
| freemangordon | so yeah cancel_delayed_work it is | 11:30 |
| sicelo | thanks. will try that | 11:34 |
| sicelo | freemangordon: we definitely need power_supply_changed() somewhere. isn't that what generates the uevent? i guess we should perhaps move it somewhere else ... question would be where? | 11:46 |
| sicelo | i.e. we need power_supply_changed() in addition to the cancel_delayed_work()... | 11:47 |
| sicelo | Lesson #1: freemangordon is always right | 13:11 |
| sicelo | mmm, back to square 1 ... so i used the suggested way, without power_supply_changed. it worked perfectly for first charger insertion. now it's back to the old behavior (ONLINE property isn't getting updated) | 13:15 |
| sicelo | maybe i should add a small delay to schedule_delayed_work :-/ | 13:22 |
| Wizzup | btw, delta.chat looks kinda neat | 15:11 |
| freemangordon | sicelo: there might be more than one bug | 17:14 |
| freemangordon | also, do you see a change to OFFLINE on charger disconnect? | 17:15 |
| freemangordon | could be that some state var is not propery updated | 17:15 |
| freemangordon | also, do you know what https://github.com/torvalds/linux/blob/master/drivers/power/supply/bq2415x_charger.c#L863 does? | 17:22 |
| freemangordon | what is this 'autotimer'? | 17:23 |
| buZz | /* enable/disable auto resetting chip timer */ | 17:26 |
| buZz | feels like a watchdog? | 17:26 |
| freemangordon | sicelo: I assume that ONLINE property is actually "mode" sysfs file, https://github.com/torvalds/linux/blob/master/drivers/power/supply/bq2415x_charger.c#L780 | 17:26 |
| freemangordon | buZz: could be | 17:26 |
| sicelo | freemangordon: so the ONLINE actually does work ... if you look in sysfs it shows online | 17:31 |
| freemangordon | which file is that? mode? | 17:31 |
| sicelo | online :-) | 17:32 |
| freemangordon | hmm, and what is this then? https://github.com/torvalds/linux/blob/master/drivers/power/supply/bq2415x_charger.c#L863 | 17:32 |
| sicelo | the problem I'm trying to fix is ... when the event fires, it reports wrong online state. however if you manually check online state after the event, it's fine | 17:33 |
| freemangordon | ok, with properly re-queuing wq, does it generate event when it shoud? | 17:33 |
| freemangordon | *should | 17:33 |
| sicelo | timer is like watchdog, yes. it's for ensuring the chip doesn't cause overcharging, etc. | 17:34 |
| freemangordon | sorry, I have no leste n900 around | 17:34 |
| freemangordon | yep, got the docs | 17:34 |
| sicelo | yes, events are sent at the right time (with your wq change, or my original power supply changed) | 17:34 |
| sicelo | just it doesn't update the properties, so event is effectively useless | 17:35 |
| freemangordon | ok, so more than one bug | 17:35 |
| freemangordon | which proerties are not updated when you say reading sysfs returns correct values? | 17:36 |
| sicelo | the ONLINE property is new (was added by me a couple of months ago). however, it's also not just ONLINE that's wrong in event, but also the STATUS ... reports Not Charging. so yeah, common cause, just not sure what source | 17:36 |
| sicelo | sorry I'm on android phone so my typing is a little erratic ... | 17:37 |
| freemangordon | sure | 17:37 |
| sicelo | but have a look at the udevadm output ... you can see event for isp1704, with ONLINE=1. also event for bq24150a, | 17:38 |
| sicelo | status says Not charging and ONLiNE=0 ... this is not correct | 17:39 |
| freemangordon | are you sure? shall it start auto charging? with no user interaction? | 17:39 |
| sicelo | if you now manually check in syfs, you'll see online=1, and status charging | 17:39 |
| freemangordon | ok, then someone is caching values | 17:40 |
| freemangordon | hmm, sec | 17:40 |
| sicelo | we somehow need a way to refresh all properties just before firing the uevent. t | 17:40 |
| freemangordon | mhm | 17:41 |
| sicelo | user interaction is user connecting charger. isp1704 reacts properly, and I'd say it's notifying bq24150 correctly, hence we get bq's uevent. but we seem to report default data in that uevent | 17:43 |
| buZz | maybe the data was cached from previous queries? | 17:43 |
| freemangordon | mhm | 17:43 |
| freemangordon | and the question is who caches the data | 17:44 |
| buZz | maybe just 'override data with known good' on the trigger? | 17:44 |
| buZz | so it can settle later | 17:44 |
| buZz | ah well, that might bite something later | 17:44 |
| * freemangordon checks what isp does | 17:45 | |
| sicelo | isp is the one doing charger detection. ba24 just charges when it sees power | 17:45 |
| sicelo | *bq24 | 17:45 |
| freemangordon | I meant - what isp code is doing | 17:49 |
| freemangordon | sicelo: I don't see any cached data in bqxxx code | 17:51 |
| freemangordon | reading the properties reads registers | 17:51 |
| sicelo | I don't think bq24's problem is caching, yes (because then it would be correct at some point), the behavior hints at just reporting some (uninitialized?) defaults | 17:53 |
| freemangordon | no, how is that | 17:53 |
| freemangordon | it reports nothing basically | 17:53 |
| freemangordon | like, power supply core shall call bq2415x_power_supply_get_property() when it needs a value | 17:54 |
| freemangordon | maybe put printks all over the place to see what happens | 17:55 |
| sicelo | will do | 17:56 |
| sicelo | the ONLINE true does lag by a few milliseconds BTW... so thay could also be it. the uevent fires before it changes | 17:57 |
| freemangordon | ok, then schedule_delayed_work(&bq->work, msecs_to_jiffies(100)); | 18:01 |
| freemangordon | instead of schedule_delayed_work(&bq->work, 0); | 18:01 |
| freemangordon | change 100 as appropriate | 18:02 |
| sicelo | yes, that was the last idea :-) will build in an hour or so and report back | 18:02 |
| freemangordon | ok | 18:03 |
| freemangordon | I might not be around though | 18:03 |
| sicelo | [13:22] sicelo: maybe i should add a small delay to schedule_delayed_work :-/ | 18:03 |
| freemangordon | yeah | 18:04 |
| freemangordon | but that would not work without cancel anyways ;) | 18:04 |
| sicelo | thanks for looking into my pet problem :-) | 18:05 |
| freemangordon | np :) | 18:05 |
| freemangordon | sicelo: see INPUT POWER SOURCE DETECTION in the doc | 18:10 |
| freemangordon | seems input source detection interval is 2 seconds | 18:14 |
| freemangordon | still try with lower values first | 18:15 |
Generated by irclog2html.py 2.17.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!