Hi,
On 7/2/21 6:54 PM, Andy Shevchenko wrote:
> On Fri, Jul 2, 2021 at 7:50 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> 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 <andy.shevchenko@xxxxxxxxx>
Thank you for the review.
Is your Reviewed-by for just this patch, or for the series ?
Regards,
Hans
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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
>>
>
>
