[Letux-kernel] [PATCH 0/6] bq2429x: Clean up driver for future fixes

[H. Nikolaus Schaller]

Hi,

> Am 13.08.2020 um 17:51 schrieb Nick Elsmore <nicholaselsmore at gmail.com>:
> 
> Hi Nikolaus,
> 
> > * Interrupts/poll events are often missed, possibly due to hangups in the work queue.
> 
> I haven't observed this in my tests. Do you have a specific example? Or a theory when and why it can happen?
> 
> 
> I instrumented the driver to print before var_lock was acquired (I left loglevel below debug, because currently there are lots of debug prints which flood the output and make it difficult to filter out whats important).  I noticed that I would often not see bq2429x_usb_detect executing until a number of seconds or minutes after I plugged or unplugged the USB OTG.  If I plugged or unplugged multiple times, I would eventually get all of the calls occurring at once after the delay.  This leads me to believe that the workqueue is hanging somewhere.
> 
> Interestingly, I would sometimes see no new interrupts for this device triggered in /proc/interrupts.  This contradicts the theory that the issue is a hangup in the workqueue, but it could be a combination of both issues in the workqueue and IRQ handler.

Well, the interrupt is going through the pcal6524 and I am not convinced if it handles all IRQs properly.
Last time I did look there was no level interrrupt and just edge triggered.
This imposes a possibility to loose interrupts if they come too quickly or multiple interrupts are not processed in one batch.
So the trouble may come from there.

We should have connected it directly to some omap5 gpio but the idea with replaceable processor board made us think it is better to go through the pcal6524 to be more universal.

>  
> > * Plug/unplug events are not properly detected, due to either work queue hangups as described above or faulty logic determining plug status
> 
> Yes, plug/unplug detection is quite some guesswork. A better strategy would be to connect to the USB-PHY...
> 
> 
> I do not see why we would need to connect to the USB-PHY.

The idea is that the USB-PHY should already reliably detect VBUS changes.

>   Interrupts from the bq2429x should be sufficient to determine whether or not the bq2429x is connected to VBUS and whether or not the bq2429x has determined this to be a good power source and is using it.  I have patches which implement the correct logic for handling this in my queue.

Yes, if we can make it reliable without PHY interfacing, it would be much better of course.

>  
> > * Watchdog is left on (if not disabled by bootloader) and not serviced. 
> 
> It should be disabled by bootloader and kept in disabled state by the driver.
> 
> Contrary to what we all would suspect it is not a safety watchdog which would e.g. cut processor power or charging after a kernel panic or stall.
> It just a mechanism to auto-reset the bq2429x registers to default values which could also be done through writing all registers.
> So I do not see how to use it for practical purposes.
> 
> 
> Yes.  We should not assume, however, that the bootloader has disabled the watchdog.  We should make sure its disabled.

Indeed, we should explicitly disable it.

Maybe I had code to disable it and then did some experiments with enabled watchdog to make use of it. And after finding that the watchdog has no good use-case I may have removed the disabling code :)

>  
> 
> > This is not properly handled.
> > * The following properties are missing:
> >    * POWER_SUPPLY_PROP_HEALTH
> 
> I am not sure if the charger can report a reasonable value for health.
> 
> 
> We'll see.  I have patches in my queue that determine reasonable values for health, but we will see how they fare in review.
>  
> 
> Generally I find the patches good from a cursory glance, but I have to do a life check since it is not easy to see from the git diffs, what is changed (any why) and if all functions are transformed correctly. This takes at least until next week.
> 
> 
> Yep, while these patches don't (shouldn't) modify any of the logic, they are rather large and introduce a good amount of restructuring.  I have submitted these patches first before integrating any logic modifications as its possible they have introduced regression.

That is fine and I want to take a look at it before we continue. This should hopefully avoid regressions before we submit it...

What we certainly have to fix/adjust is the bq2429x.yaml bindings document. I had started to write one

https://git.goldelico.com/?p=letux-kernel.git;a=commit;h=1c094ca615be1a5ff7e268d1973c70aa894167e5

but I am not sure if we can enforce some values in the monitored battery node or formally define the defaults for it...
This must also pass make dt_binding_check dtbs_check

BR and thanks,
Nikolaus


>   From my tests, the behavior seems the same with and without the patches.
> 
> Thanks,
> Nick Elsmore
> _______________________________________________
> https://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel at openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.goldelico.com/pipermail/letux-kernel/attachments/20200813/0db28010/attachment-0001.htm>