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

[David Shah]

Hi Nikolaus,

I also saw the high temperature issue on 5.6 when charging sometimes - I guess
you know already from your previous bq work but "60°" just means too hot to
charge as there is no exact readout. This also resulted in a "HOT" print from
the driver.

Maybe we should be lowering the charge current a bit for safer and more reliable
charging, I don't know how much control over this the bq gives. The battery
didn't feel that hot externally, but the thermistor will be sensing the
temperature inside the pack which will be higher.

Best

David

On Sun, 2020-08-16 at 22:09 +0200, H. Nikolaus Schaller wrote:
> Hi Nick,
> I have started a little to test your patches on top of v5.8-rc7 (including
> also David's patches so that the Panel is working again).
> There was a missing comma - easy to fix for compilation.
> 
> Basically the driver seems to work, but OTG doesn't. It does not even try to
> switch power on, although DWC3 detects it.
> 
> Unplugging the OTG cable once did lead to:
> 
> root at letux:~# [  397.149733] ------------[ cut here ]------------
> [  397.154707] WARNING: CPU: 0 PID: 37 at drivers/regulator/core.c:2603
> _regulator_disable+0x3c/0x168
> [  397.170413] unbalanced disables for otg_path
> 
> And once the temperature report was 60°C.
> 
> So I think there is something wrong with interrupts / status change detection.
> Just a note: I have tested on an older Pyra prototype with tca6424 and there,
> IRQs are definitively broken. So we must poll the status bits.
> 
> Really nice is reading the bit fields from
> 
> 	cat /sys/class/power_supply/bq24297/device/registers
> 
> Much better information than the pure register values.
> 
> Still, using regmap still allows to read them the "old" way for comparison
> (except the volatile ones):
> 
> 	cat /sys/kernel/debug/regmap/1-006b/registers
> 
> There are differences in registers 0 and 1 compared to the old driver. The
> other registers are the same (except 8 and 9).
> 
> But this is just a first and quick "hello" test, nothing fundamental. I'll try
> to find out more in the next days as I find time.
> And after upgrading to v5.9-rc1.
> 
> BR and thanks,
> Nikolaus
> 
> 
> 
> > 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.
> >  
> > > * 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.  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.
> >  
> > > * 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.
> >  
> > 
> > > 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.  From my tests, the behavior seems
> > the same with and without the patches.
> > 
> > Thanks,
> > Nick Elsmore
> > _______________________________________________
> > Kernel mailing list
> > Kernel at pyra-handheld.com
> > http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel
> 
> _______________________________________________
> Kernel mailing list
> Kernel at pyra-handheld.com
> http://pyra-handheld.com/cgi-bin/mailman/listinfo/kernel