Re: [PATCH v3 00/10] power: supply: axp288_fuel_gauge: Reduce number of register accesses + cleanups

[Hans de Goede <hdegoede@redhat.com>]

Hi Sebastian,

On 8/5/21 7:19 PM, Sebastian Reichel wrote:
> Hi Hans,
> 
> On Sun, Aug 01, 2021 at 03:30:55PM +0200, Hans de Goede wrote:
>> Hi all,
>>
>> Changes in v3:
>> - Replace "depends on X86" with "depends on IOSF_MBI" as the new code uses
>>   symbols which are only defined when IOSF_MBI support is enabled.
>>   Depending on this is ok since IOSF_MBI support should always be enabled
>>   on devices which use the AXP288 PMIC.
>>
>> Changes in v2:
>> - Add a "depends on X86" to Kconfig since the iosf_mbi functions are X86 only
>>   (the AXP288 PMIC is only used on X86 devices).
>>
>> And here is the v1 cover-letter again:
>>
>> The I2C-bus to the XPower AXP288 is shared between the Linux kernel and
>> the SoCs P-Unit. The P-Unit has a semaphore which the kernel must "lock"
>> before it may use the bus and while the kernel holds the semaphore the CPU
>> and GPU power-states must not be changed otherwise the system will freeze.
>> This is a complex process, which is quite expensive.
>>
>> To ensure that no unguarded I2C-bus accesses happen, the semaphore is
>> taken by the I2C-bus-driver for every I2C transfer. When upower refreshes
>> its battery stats it reads all the power-supply properties at once,
>> leading to the semaphore getting hammered which sometimes causes the
>> system to hang.
>>
>> Andrejus maintains a large "fleet" of affected Cherry Trail tablets
>> and was seeing these hangs semi regularly. After discussing this with
>> me Andrejus wrote the caching patch in this series which greatly reduces
>> the number of semaphore accesses and since then there have been no
>> reports of hangs in the fleet of devices which he maintains.
>>
>> I've cleaned up Andrejus work a bit before submitting it upstream and
>> while working on this I found a slew of other issues in this driver
>> which bugged me enough to write a bunch of cleanup patches. I've also
>> added some extra patches to also reduce the semaphore use during driver
>> probe.
> 
> Thanks, I applied the whole series.

Thank you.

> The double goto from the last
> patch is a bit ugly and can be avoided by switching the driver to
> devm_*(). Do you mind doing so?

No I can take care of this, the problem is finding some time for
it though. I've added this to my to do list, but there is a bunch
of higher priority items above it.

Still I will try to get around to doing some clean-up by switching
to devm_... eventually.

Regards,

Hans




>> Andrejus Basovas (1):
>>   power: supply: axp288_fuel_gauge: Refresh all registers in one go
>>
>> Hans de Goede (9):
>>   power: supply: axp288_fuel_gauge: Fix define alignment
>>   power: supply: axp288_fuel_gauge: Remove debugfs support
>>   power: supply: axp288_fuel_gauge: Silence the chatty IRQ mapping code
>>   power: supply: axp288_fuel_gauge: Report register-address on readb /
>>     writeb errors
>>   power: supply: axp288_fuel_gauge: Drop retry logic from
>>     fuel_gauge_reg_readb()
>>   power: supply: axp288_fuel_gauge: Store struct device pointer in
>>     axp288_fg_info
>>   power: supply: axp288_fuel_gauge: Only read PWR_OP_MODE,
>>     FG_LOW_CAP_REG regs once
>>   power: supply: axp288_fuel_gauge: Move the AXP20X_CC_CTRL check
>>     together with the other checks
>>   power: supply: axp288_fuel_gauge: Take the P-Unit semaphore only once
>>     during probe()
>>
>>  drivers/power/supply/Kconfig             |   2 +-
>>  drivers/power/supply/axp288_fuel_gauge.c | 489 +++++++++--------------
>>  2 files changed, 187 insertions(+), 304 deletions(-)
>>
>> -- 
>> 2.31.1