Hi,

On 7/2/21 6:54 PM, Andy Shevchenko wrote:

On Fri, Jul 2, 2021 at 7:50 PM Hans de Goede [off-list ref] wrote:

quoted

The I2C-bus to the XPower AXP288 is shared between the Linux kernel and
the SoCs PUNIT. The PUNIT 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. This is all done by
iosf_mbi_block_punit_i2c_access(). To ensure that no unguarded I2C-bus
accesses happen, iosf_mbi_block_punit_i2c_access() gets called by the
I2C-bus-driver for every I2C transfer. Because this is so expensive it
is allowed to call iosf_mbi_block_punit_i2c_access() in a nested
fashion, so that higher-level code which does multiple I2C-transfers can
call it once for a group of transfers, turning the calls done by the
I2C-bus-driver into no-ops.

Move / add iosf_mbi_block_punit_i2c_access() calls in / to the XPower
OpRegion code so that the PUNIT semaphore only needs to be taken once
for each OpRegion access.

We usually spell "P-Unit" instead of "PUNIT".
Otherwise it looks good to me, thanks!
Reviewed-by: Andy Shevchenko <redacted>

Thank you for the review.

Is your Reviewed-by for just this patch, or for the series ?

Regards,

Hans

quoted

Signed-off-by: Hans de Goede <redacted>
---
 drivers/acpi/pmic/intel_pmic_xpower.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index a091d5a8392c..644a495a4f13 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c

@@ -178,15 +178,17 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
 {
        int data, ret;

-       /* GPIO1 LDO regulator needs special handling */
-       if (reg == XPOWER_GPI1_CTRL)
-               return regmap_update_bits(regmap, reg, GPI1_LDO_MASK,
-                                         on ? GPI1_LDO_ON : GPI1_LDO_OFF);
-
        ret = iosf_mbi_block_punit_i2c_access();
        if (ret)
                return ret;

+       /* GPIO1 LDO regulator needs special handling */
+       if (reg == XPOWER_GPI1_CTRL) {
+               ret = regmap_update_bits(regmap, reg, GPI1_LDO_MASK,
+                                        on ? GPI1_LDO_ON : GPI1_LDO_OFF);
+               goto out;
+       }
+
        if (regmap_read(regmap, reg, &data)) {
                ret = -EIO;
                goto out;

@@ -218,6 +220,10 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
        int ret, adc_ts_pin_ctrl;
        u8 buf[2];

+       ret = iosf_mbi_block_punit_i2c_access();
+       if (ret)
+               return ret;
+
        /*
         * The current-source used for the battery temp-sensor (TS) is shared
         * with the GPADC. For proper fuel-gauge and charger operation the TS

@@ -231,14 +237,14 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
         */
        ret = regmap_read(regmap, AXP288_ADC_TS_PIN_CTRL, &adc_ts_pin_ctrl);
        if (ret)
-               return ret;
+               goto out;

        if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) {
                ret = regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL,
                                         AXP288_ADC_TS_CURRENT_ON_OFF_MASK,
                                         AXP288_ADC_TS_CURRENT_ON_ONDEMAND);
                if (ret)
-                       return ret;
+                       goto out;

                /* Wait a bit after switching the current-source */
                usleep_range(6000, 10000);

@@ -254,6 +260,9 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
                                   AXP288_ADC_TS_CURRENT_ON);
        }

+out:
+       iosf_mbi_unblock_punit_i2c_access();
+
        return ret;
 }

--

2.31.1