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:

quoted

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

quoted

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