| Message ID | 20250124023021.9418-1-dhs@frame.work (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | chrome/cros_ec_lpc: Match on Framework ACPI device | expand |
On Fri, Jan 24, 2025 at 10:30:21AM +0800, Daniel Schaefer wrote: > This patch loads the cros_ec_lpc driver based on a Framework FRMWC006 > ACPI device, which mirrors GOOG0004, but also applies npcx quirks for > Framework systems. s/This patch loads/Load/. On a related note, "platform/chrome: cros_ec_lpc:" is the preferred prefix for the patch. > Matching on ACPI will let us avoid having to change the SMBIOS match > rules again and again. Does it mean the following DMI match is not comprehensive enough and would need to change again and again? ``` .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Framework"), DMI_MATCH(DMI_PRODUCT_FAMILY, "Laptop"), }, .driver_data = (void *)&framework_laptop_npcx_lpc_driver_data, ``` > I tested all new possible combinations and they all behave as expected. > If there is any match, /dev/cros_ec will be present, with the below > dmesg. > I did not test any device with GOOG0004. > > | DMI match | FRMWC004 match | Notes > | False | False | 3rdparty > | True | False | Current framework > | False | True | Possible future framework > | True | True | Future framework > > ``` > > sudo dmesg | grep cros_ec_lpcs > [ 17.521972] cros_ec_lpcs FRMWC004:00: loaded with quirks 00000001 > [ 17.534012] cros_ec_lpcs FRMWC004:00: Chrome EC device registered > ``` > > ``` > [ 74.366596] cros_ec_lpcs cros_ec_lpcs.0: loaded with quirks 00000001 > [ 74.377921] cros_ec_lpcs cros_ec_lpcs.0: Chrome EC device registered > ``` Just in case if you don't want to include them in the commit messages, please put them after "---"[1]. [1]: https://docs.kernel.org/process/submitting-patches.html > /* True if ACPI device is present */ > static bool cros_ec_lpc_acpi_device_found; > +static bool cros_ec_lpc_frmw_acpi_device_found; I guess it doesn't need the flag. See below. > @@ -526,6 +532,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev) > ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP; > > driver_data = platform_get_drvdata(pdev); > + if (!driver_data) > + driver_data = platform_get_drvdata(&cros_ec_lpc_device); NACK, it doesn't look like a good idea to me. It uses `cros_ec_lpc_device` merely for passing the pointer of struct lpc_driver_data. It should use acpi_device_get_match_data() instead. > @@ -698,6 +706,7 @@ static void cros_ec_lpc_remove(struct platform_device *pdev) > > static const struct acpi_device_id cros_ec_lpc_acpi_device_ids[] = { > { ACPI_DRV_NAME, 0 }, > + { FRMW_ACPI_DRV_NAME, 0 }, So, if the NCPX is the only valid driver data for FRMW_ACPI_DRV_NAME, how about `{ FRMW_ACPI_DRV_NAME, &framework_laptop_npcx_lpc_driver_data },`? As a result, the cros_ec_lpc_probe() can get it via acpi_device_get_match_data(). > static int __init cros_ec_lpc_init(void) > { > int ret; > const struct dmi_system_id *dmi_match; > > cros_ec_lpc_acpi_device_found = !!cros_ec_lpc_get_device(ACPI_DRV_NAME); > + cros_ec_lpc_frmw_acpi_device_found = !!cros_ec_lpc_get_device(FRMW_ACPI_DRV_NAME); > > dmi_match = dmi_first_match(cros_ec_lpc_dmi_table); > > - if (!cros_ec_lpc_acpi_device_found && !dmi_match) { > + if (cros_ec_lpc_frmw_acpi_device_found) { > + platform_set_drvdata(&cros_ec_lpc_device, > + (void *)&framework_laptop_npcx_lpc_driver_data); NACK. See above comment.
On Tue, Jan 28, 2025 at 12:22 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > On Fri, Jan 24, 2025 at 10:30:21AM +0800, Daniel Schaefer wrote: > > This patch loads the cros_ec_lpc driver based on a Framework FRMWC006 > > ACPI device, which mirrors GOOG0004, but also applies npcx quirks for > > Framework systems. > > s/This patch loads/Load/. On a related note, "platform/chrome: cros_ec_lpc:" > is the preferred prefix for the patch. Thanks > > Matching on ACPI will let us avoid having to change the SMBIOS match > > rules again and again. > > Does it mean the following DMI match is not comprehensive enough and > would need to change again and again? Possibly, there have been 4 revisions to the Framework DMI matches. We won't make breaking changes on purpose, but it might be necessary in the future. Additionally, pre-production systems don't have the correct DMI strings and we'd like to have the kernel module load on those as well, while testing them internally. ACPI is much easier to patch than SMBIOS. > > @@ -526,6 +532,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev) > > ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP; > > > > driver_data = platform_get_drvdata(pdev); > > + if (!driver_data) > > + driver_data = platform_get_drvdata(&cros_ec_lpc_device); > > NACK, it doesn't look like a good idea to me. It uses `cros_ec_lpc_device` > merely for passing the pointer of struct lpc_driver_data. It should use > acpi_device_get_match_data() instead. Thanks, that looks easier and cleaner. I'll send out v2.
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c index 5a2f1d98b350..ac951ab26ddf 100644 --- a/drivers/platform/chrome/cros_ec_lpc.c +++ b/drivers/platform/chrome/cros_ec_lpc.c @@ -30,9 +30,11 @@ #define DRV_NAME "cros_ec_lpcs" #define ACPI_DRV_NAME "GOOG0004" +#define FRMW_ACPI_DRV_NAME "FRMWC004" /* True if ACPI device is present */ static bool cros_ec_lpc_acpi_device_found; +static bool cros_ec_lpc_frmw_acpi_device_found; /* * Indicates that lpc_driver_data.quirk_mmio_memory_base should @@ -507,6 +509,10 @@ static acpi_status cros_ec_lpc_resources(struct acpi_resource *res, void *data) return AE_OK; } +static struct platform_device cros_ec_lpc_device = { + .name = DRV_NAME +}; + static int cros_ec_lpc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -526,6 +532,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev) ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP; driver_data = platform_get_drvdata(pdev); + if (!driver_data) + driver_data = platform_get_drvdata(&cros_ec_lpc_device); if (driver_data) { quirks = driver_data->quirks; @@ -698,6 +706,7 @@ static void cros_ec_lpc_remove(struct platform_device *pdev) static const struct acpi_device_id cros_ec_lpc_acpi_device_ids[] = { { ACPI_DRV_NAME, 0 }, + { FRMW_ACPI_DRV_NAME, 0 }, { } }; MODULE_DEVICE_TABLE(acpi, cros_ec_lpc_acpi_device_ids); @@ -857,20 +866,20 @@ static struct platform_driver cros_ec_lpc_driver = { .remove = cros_ec_lpc_remove, }; -static struct platform_device cros_ec_lpc_device = { - .name = DRV_NAME -}; - static int __init cros_ec_lpc_init(void) { int ret; const struct dmi_system_id *dmi_match; cros_ec_lpc_acpi_device_found = !!cros_ec_lpc_get_device(ACPI_DRV_NAME); + cros_ec_lpc_frmw_acpi_device_found = !!cros_ec_lpc_get_device(FRMW_ACPI_DRV_NAME); dmi_match = dmi_first_match(cros_ec_lpc_dmi_table); - if (!cros_ec_lpc_acpi_device_found && !dmi_match) { + if (cros_ec_lpc_frmw_acpi_device_found) { + platform_set_drvdata(&cros_ec_lpc_device, + (void *)&framework_laptop_npcx_lpc_driver_data); + } else if (!cros_ec_lpc_acpi_device_found && !dmi_match) { pr_err(DRV_NAME ": unsupported system.\n"); return -ENODEV; } @@ -882,7 +891,7 @@ static int __init cros_ec_lpc_init(void) return ret; } - if (!cros_ec_lpc_acpi_device_found) { + if (!cros_ec_lpc_acpi_device_found && !cros_ec_lpc_frmw_acpi_device_found) { /* Pass the DMI match's driver data down to the platform device */ platform_set_drvdata(&cros_ec_lpc_device, dmi_match->driver_data); @@ -899,7 +908,7 @@ static int __init cros_ec_lpc_init(void) static void __exit cros_ec_lpc_exit(void) { - if (!cros_ec_lpc_acpi_device_found) + if (!cros_ec_lpc_acpi_device_found && !cros_ec_lpc_frmw_acpi_device_found) platform_device_unregister(&cros_ec_lpc_device); platform_driver_unregister(&cros_ec_lpc_driver); }