On Sun, Oct 31, 2021 at 4:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> The handling of PMIC register reads through writing 0 to address 4
> of the OpRegion is wrong. Instead of returning the read value
> through the value64, which is a no-op for function == ACPI_WRITE calls,
> store the value and then on a subsequent function == ACPI_READ with
> address == 3 (the address for the value field of the OpRegion)
> return the stored value.
>
> This has been tested on a Xiaomi Mi Pad 2 and makes the ACPI battery dev
> there mostly functional (unfortunately there are still other issues).
>
> Here are the SET() / GET() functions of the PMIC ACPI device,
> which use this OpRegion, which clearly show the new behavior to
> be correct:
>
> OperationRegion (REGS, 0x8F, Zero, 0x50)
> Field (REGS, ByteAcc, NoLock, Preserve)
> {
> CLNT, 8,
> SA, 8,
> OFF, 8,
> VAL, 8,
> RWM, 8
> }
>
> Method (GET, 3, Serialized)
> {
> If ((AVBE == One))
> {
> CLNT = Arg0
> SA = Arg1
> OFF = Arg2
> RWM = Zero
> If ((AVBG == One))
> {
> GPRW = Zero
> }
> }
>
> Return (VAL) /* \_SB_.PCI0.I2C7.PMI5.VAL_ */
> }
>
> Method (SET, 4, Serialized)
> {
> If ((AVBE == One))
> {
> CLNT = Arg0
> SA = Arg1
> OFF = Arg2
> VAL = Arg3
> RWM = One
> If ((AVBG == One))
> {
> GPRW = One
> }
> }
> }
>
> Fixes: 0afa877a5650 ("ACPI / PMIC: intel: add REGS operation region support")
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Changes in v2:
> - Be consistent with capitalization of OpRegion
> ---
> drivers/acpi/pmic/intel_pmic.c | 51 +++++++++++++++++++---------------
> 1 file changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index a371f273f99d..9cde299eba88 100644
> --- a/drivers/acpi/pmic/intel_pmic.c
> +++ b/drivers/acpi/pmic/intel_pmic.c
> @@ -211,31 +211,36 @@ static acpi_status intel_pmic_regs_handler(u32 function,
> void *handler_context, void *region_context)
> {
> struct intel_pmic_opregion *opregion = region_context;
> - int result = 0;
> + int result = -EINVAL;
> +
> + if (function == ACPI_WRITE) {
> + switch (address) {
> + case 0:
> + return AE_OK;
> + case 1:
> + opregion->ctx.addr |= (*value64 & 0xff) << 8;
> + return AE_OK;
> + case 2:
> + opregion->ctx.addr |= *value64 & 0xff;
> + return AE_OK;
> + case 3:
> + opregion->ctx.val = *value64 & 0xff;
> + return AE_OK;
> + case 4:
> + if (*value64) {
> + result = regmap_write(opregion->regmap, opregion->ctx.addr,
> + opregion->ctx.val);
> + } else {
> + result = regmap_read(opregion->regmap, opregion->ctx.addr,
> + &opregion->ctx.val);
> + }
> + opregion->ctx.addr = 0;
> + }
> + }
>
> - switch (address) {
> - case 0:
> - return AE_OK;
> - case 1:
> - opregion->ctx.addr |= (*value64 & 0xff) << 8;
> - return AE_OK;
> - case 2:
> - opregion->ctx.addr |= *value64 & 0xff;
> + if (function == ACPI_READ && address == 3) {
> + *value64 = opregion->ctx.val;
> return AE_OK;
> - case 3:
> - opregion->ctx.val = *value64 & 0xff;
> - return AE_OK;
> - case 4:
> - if (*value64) {
> - result = regmap_write(opregion->regmap, opregion->ctx.addr,
> - opregion->ctx.val);
> - } else {
> - result = regmap_read(opregion->regmap, opregion->ctx.addr,
> - &opregion->ctx.val);
> - if (result == 0)
> - *value64 = opregion->ctx.val;
> - }
> - memset(&opregion->ctx, 0x00, sizeof(opregion->ctx));
> }
>
> if (result < 0) {
> --
Applied as 5.16-rc material, thanks!
