diff mbox series

[v2,3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

Message ID 20240603063834.5580-4-ben@jubnut.com (mailing list archive)
State Superseded
Headers show
Series platform/chrome: Fix MEC concurrency problems for Framework Laptop | expand

Commit Message

Ben Walsh June 3, 2024, 6:38 a.m. UTC
Framework Laptops' ACPI exposes the EC with id "PNP0C09". But
"PNP0C09" is part of the ACPI standard; there are lots of computers
with EC chips with this id, and most of them don't support the cros_ec
protocol.

The driver could find the ACPI device by having "PNP0C09" in the
acpi_match_table, but this would match devices which don't support the
cros_ec protocol. Instead, add a new quirk "CROS_EC_LPC_QUIRK_ACPI_ID"
which allows the id to be specified. This quirk is applied after the
DMI check shows that the device is supported.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 53 ++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 13 deletions(-)

Comments

Tzung-Bi Shih June 3, 2024, 9:30 a.m. UTC | #1
On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  
>  	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>  
> +	adev = ACPI_COMPANION(dev);
> +

The change is irrelevant to the patch.

> @@ -538,7 +577,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  	 * Connect a notify handler to process MKBP messages if we have a
>  	 * companion ACPI device.
>  	 */
> -	adev = ACPI_COMPANION(dev);
>  	if (adev) {
>  		status = acpi_install_notify_handler(adev->handle,
>  						     ACPI_ALL_NOTIFY,

See above comment.
Mario Limonciello June 3, 2024, 4 p.m. UTC | #2
On 6/3/2024 04:30, Tzung-Bi Shih wrote:
> On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
>> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>   
>>   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>>   
>> +	adev = ACPI_COMPANION(dev);
>> +
> 
> The change is irrelevant to the patch.

It looks relevant to me.  The companion needs to get set before the 
quirk overwrites it.

> 
>> @@ -538,7 +577,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>   	 * Connect a notify handler to process MKBP messages if we have a
>>   	 * companion ACPI device.
>>   	 */
>> -	adev = ACPI_COMPANION(dev);
>>   	if (adev) {
>>   		status = acpi_install_notify_handler(adev->handle,
>>   						     ACPI_ALL_NOTIFY,
> 
> See above comment.
Ben Walsh June 3, 2024, 7:10 p.m. UTC | #3
Mario Limonciello <mario.limonciello@amd.com> writes:

> On 6/3/2024 04:30, Tzung-Bi Shih wrote:
>> On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
>>> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>>   
>>>   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>>>   
>>> +	adev = ACPI_COMPANION(dev);
>>> +
>> 
>> The change is irrelevant to the patch.
>
> It looks relevant to me.  The companion needs to get set before the 
> quirk overwrites it.

That was my thinking. You can either update "adev" and use it to refer
to the ACPI companion device, or (as Tzung-Bi suggests) use
"ACPI_COMPANION" / "ACPI_COMPANION_SET" as needed.

I really don't mind either way. I'm happy to change it if necessary.
Tzung-Bi Shih June 4, 2024, 2:30 a.m. UTC | #4
On Mon, Jun 03, 2024 at 11:00:22AM -0500, Mario Limonciello wrote:
> On 6/3/2024 04:30, Tzung-Bi Shih wrote:
> > On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
> > > @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> > >   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
> > > +	adev = ACPI_COMPANION(dev);
> > > +
> > 
> > The change is irrelevant to the patch.
> 
> It looks relevant to me.  The companion needs to get set before the quirk
> overwrites it.

Please see code for CROS_EC_LPC_QUIRK_ACPI_ID in the patch, `adev` is going to
be overridden soon.  I don't see why the ACPI_COMPANION() needs to move here.
diff mbox series

Patch

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 6663141a07d4..c1c26a7ba6d4 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -39,6 +39,11 @@  static bool cros_ec_lpc_acpi_device_found;
  * be used as the base port for EC mapped memory.
  */
 #define CROS_EC_LPC_QUIRK_REMAP_MEMORY              BIT(0)
+/*
+ * Indicates that lpc_driver_data.quirk_acpi_id should be used to find
+ * the ACPI device.
+ */
+#define CROS_EC_LPC_QUIRK_ACPI_ID                   BIT(1)
 
 /**
  * struct lpc_driver_data - driver data attached to a DMI device ID to indicate
@@ -46,10 +51,12 @@  static bool cros_ec_lpc_acpi_device_found;
  * @quirks: a bitfield composed of quirks from CROS_EC_LPC_QUIRK_*
  * @quirk_mmio_memory_base: The first I/O port addressing EC mapped memory (used
  *                          when quirk ...REMAP_MEMORY is set.)
+ * @quirk_acpi_id: An ACPI HID to be used to find the ACPI device.
  */
 struct lpc_driver_data {
 	u32 quirks;
 	u16 quirk_mmio_memory_base;
+	const char *quirk_acpi_id;
 };
 
 /**
@@ -418,6 +425,26 @@  static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
 		pm_system_wakeup();
 }
 
+static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level,
+					    void *context, void **retval)
+{
+	*(struct acpi_device **)context = acpi_fetch_acpi_dev(handle);
+	return AE_CTRL_TERMINATE;
+}
+
+static struct acpi_device *cros_ec_lpc_get_device(const char *id)
+{
+	struct acpi_device *adev = NULL;
+	acpi_status status = acpi_get_devices(id, cros_ec_lpc_parse_device,
+					      &adev, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_warn(DRV_NAME ": Looking for %s failed\n", id);
+		return NULL;
+	}
+
+	return adev;
+}
+
 static int cros_ec_lpc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -436,6 +463,8 @@  static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
 
+	adev = ACPI_COMPANION(dev);
+
 	driver_data = platform_get_drvdata(pdev);
 	if (driver_data) {
 		quirks = driver_data->quirks;
@@ -445,6 +474,16 @@  static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 		if (quirks & CROS_EC_LPC_QUIRK_REMAP_MEMORY)
 			ec_lpc->mmio_memory_base = driver_data->quirk_mmio_memory_base;
+
+		if (quirks & CROS_EC_LPC_QUIRK_ACPI_ID) {
+			adev = cros_ec_lpc_get_device(driver_data->quirk_acpi_id);
+			if (!adev) {
+				dev_err(dev, "failed to get ACPI device '%s'",
+					driver_data->quirk_acpi_id);
+				return -ENODEV;
+			}
+			ACPI_COMPANION_SET(dev, adev);
+		}
 	}
 
 	/*
@@ -538,7 +577,6 @@  static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 * Connect a notify handler to process MKBP messages if we have a
 	 * companion ACPI device.
 	 */
-	adev = ACPI_COMPANION(dev);
 	if (adev) {
 		status = acpi_install_notify_handler(adev->handle,
 						     ACPI_ALL_NOTIFY,
@@ -705,23 +743,12 @@  static struct platform_device cros_ec_lpc_device = {
 	.name = DRV_NAME
 };
 
-static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level,
-					    void *context, void **retval)
-{
-	*(bool *)context = true;
-	return AE_CTRL_TERMINATE;
-}
-
 static int __init cros_ec_lpc_init(void)
 {
 	int ret;
-	acpi_status status;
 	const struct dmi_system_id *dmi_match;
 
-	status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
-				  &cros_ec_lpc_acpi_device_found, NULL);
-	if (ACPI_FAILURE(status))
-		pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
+	cros_ec_lpc_acpi_device_found = !!cros_ec_lpc_get_device(ACPI_DRV_NAME);
 
 	dmi_match = dmi_first_match(cros_ec_lpc_dmi_table);