Received-SPF: pass (google.com: domain of linux-kernel+bounces-94355-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <bac77347-1308-49a1-b02a-19c6b7a5a31b@gmx.de>
Date: Wed, 6 Mar 2024 18:57:27 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] platform/x86: wmi: Support reading/writing 16 bit EC
 values
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org,
 LKML <linux-kernel@vger.kernel.org>, "Rafael J. Wysocki"
 <rafael@kernel.org>, Len Brown <lenb@kernel.org>
References: <20240304221732.39272-1-W_Armin@gmx.de>
 <ffb87c8f-3d6d-c96c-71e9-2abccfe68405@linux.intel.com>
Content-Language: en-US
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <ffb87c8f-3d6d-c96c-71e9-2abccfe68405@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:PwLHHy9sUMw=;TA1kQu+WXeJuNKAPVKA2+2uoFIG
 VQE3tQsOtJqSL8gdHWA95rWsSM89K6db6K1NOGuS9rya72twymnhsAGlGPcmvDa4OrmnwF6u7
 VsWeQNhOGgeonat/8TqlywIsEiRNjB7h5iL+5ROjNfmXC4+v6VrTyRr0WKVk0DNGqSke3dR/5
 j48gsRD0VBf2zhaOCwVxKcNYpYBmGmBGeG3DyOs/qv6W22zuRoIUEl7WERkLAA48MQbXZ27/+
 ntUzG6JN1OWjK1sTssE/haIUJoWcw4h7nhfSYwRAwKqQqemWOt2hjK7ptImNWOTjwMo47Qf1N
 e3M/Nfrk4ubvTGiuRhaZ4QAY9tD9TFQpH58MOlE/zs8pcftfQPUsFNLVR8nV87X+kpgIbPAKS
 mRzwmm1AMsh2Yd/5Pg3krQvdoLw1MsdvIzsmpIxREiDk0b7fiZd19eHCA1OasfeJoEtzgf1iM
 kwfabf5BIj/UQYRhoS52WPgIWteXfQ2KD37hbHzCxCNL0iiRAvnU/d0k6+sfTBd8JSWe/zVc0
 qCMihghA+IUpi8nUj8Ps45/R1kY6vT+oKNx5EFa8QCVH8d94LgSqZG0z9yP5j4ZCsBtS8BHeA
 WjT4jtkAb705/6G//q0xlIuVjXWTVuj4dQu4wC2sNTCUHG2b7mQnjZ2xoDRKrMm4flrnywzpz
 RlYPm1H3pRSkXji3+wo79ZJCwwF7+fLFqiGHbOO/3wz657ZuNkDvmIOn5WrJPxn7z7e/OTUwk
 tdtRrqqcOxBryM4HeBuTIoHCtXZHe/s5i1dFFD60PVlP0X8n3fRhP2ZlEXU84ehgEkFJSPJ+V
 1nIsEgbcTxi6OuZGF038vNtXn2SVRkhFAIGJCXdKY/XLY=

Am 06.03.24 um 11:19 schrieb Ilpo J=C3=A4rvinen:

> On Mon, 4 Mar 2024, Armin Wolf wrote:
>
>> The ACPI EC address space handler currently only supports
>> reading/writing 8 bit values. Some firmware implementations however
>> want to access for example 16 bit values, which is prefectly legal
>> according to the ACPI spec.
>>
>> Add support for reading/writing such values.
>>
>> Tested on a Dell Inspiron 3505 and a Asus Prime B650-Plus.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/wmi.c | 44 +++++++++++++++++++++++++++++--------=
-
>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 1920e115da89..900e0e52a5fa 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1153,6 +1153,32 @@ static int parse_wdg(struct device *wmi_bus_dev,=
 struct platform_device *pdev)
>>   	return 0;
>>   }
>>
>> +static int ec_read_multiple(u8 address, u8 *buffer, size_t bytes)
>> +{
>> +	int i, ret;
>> +
>> +	for (i =3D 0; i < bytes; i++) {
>> +		ret =3D ec_read(address + i, &buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ec_write_multiple(u8 address, u8 *buffer, size_t bytes)
>> +{
>> +	int i, ret;
>> +
>> +	for (i =3D 0; i < bytes; i++) {
>> +		ret =3D ec_write(address + i, buffer[i]);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /*
>>    * WMI can have EmbeddedControl access regions. In which case, we jus=
t want to
>>    * hand these off to the EC driver.
>> @@ -1162,27 +1188,25 @@ acpi_wmi_ec_space_handler(u32 function, acpi_ph=
ysical_address address,
>>   			  u32 bits, u64 *value,
>>   			  void *handler_context, void *region_context)
>>   {
>> -	int result =3D 0;
>> -	u8 temp =3D 0;
>> +	int bytes =3D bits / 8;
> I'm a quite hesitant about this. IMO, it should do DIV_ROUND_UP(bits,
> BITS_PER_BYTE) or return AE_BAD_PARAMETER when bits is not divisable by =
8.
> And if you choose the round up approach, I'm not sure what the write
> should do with the excess bits.
>
> In any case, 8 -> BITS_PER_BYTE.

After taking a look at acpi_ex_access_region(), which invokes the address =
space handler,
i think the number of bits are always divisible by 8.

I CCed the maintainers of the ACPI EC driver, so that we can clarify if th=
is is indeed
always the case.

Thanks,
Armin Wolf

>> +	int ret;
>>
>> -	if ((address > 0xFF) || !value)
>> +	if (address > 0xFF || !value)
> This should takes bytes into account to not overflow the u8 address?
>