| Message ID | 20211222185154.4560-1-tcrawford@system76.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: system76_acpi: Guard System76 EC specific functionality | expand |
Hi, On Wed, 2021-12-22 at 11:51 -0700, Tim Crawford wrote: > Certain functionality or its implementation in System76 EC firmware may > be different to the proprietary ODM EC firmware. Introduce a new bool, > `has_open_ec`, to guard our specific logic. Detect the use of this by > looking for a custom ACPI method name used in System76 firmware. > > Signed-off-by: Tim Crawford <tcrawford@system76.com> > --- > drivers/platform/x86/system76_acpi.c | 58 ++++++++++++++-------------- > 1 file changed, 30 insertions(+), 28 deletions(-) > > diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c > index 8b292ee95a14..7299ad08c838 100644 > --- a/drivers/platform/x86/system76_acpi.c > +++ b/drivers/platform/x86/system76_acpi.c > @@ -35,6 +35,7 @@ struct system76_data { > union acpi_object *nfan; > union acpi_object *ntmp; > struct input_dev *input; > + bool has_open_ec; > }; > > static const struct acpi_device_id device_ids[] = { > @@ -279,20 +280,12 @@ static struct acpi_battery_hook system76_battery_hook = { > > static void system76_battery_init(void) > { > - acpi_handle handle; > - > - handle = ec_get_handle(); > - if (handle && acpi_has_method(handle, "GBCT")) > - battery_hook_register(&system76_battery_hook); > + battery_hook_register(&system76_battery_hook); > } > > static void system76_battery_exit(void) > { > - acpi_handle handle; > - > - handle = ec_get_handle(); > - if (handle && acpi_has_method(handle, "GBCT")) > - battery_hook_unregister(&system76_battery_hook); > + battery_hook_unregister(&system76_battery_hook); > } > > // Get the airplane mode LED brightness > @@ -673,6 +666,10 @@ static int system76_add(struct acpi_device *acpi_dev) > acpi_dev->driver_data = data; > data->acpi_dev = acpi_dev; > > + // Some models do not run open EC firmware. Check for an ACPI method > + // that only exists on open EC to guard functionality specific to it. > + data->has_open_ec = acpi_has_method(acpi_device_handle(data->acpi_dev), "NFAN"); > + > err = system76_get(data, "INIT"); > if (err) > return err; > @@ -718,27 +715,31 @@ static int system76_add(struct acpi_device *acpi_dev) > if (err) > goto error; > > - err = system76_get_object(data, "NFAN", &data->nfan); > - if (err) > - goto error; > + if (data->has_open_ec) { > + err = system76_get_object(data, "NFAN", &data->nfan); > + if (err) > + goto error; > > - err = system76_get_object(data, "NTMP", &data->ntmp); > - if (err) > - goto error; > + err = system76_get_object(data, "NTMP", &data->ntmp); > + if (err) > + goto error; > > - data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, > - "system76_acpi", data, &thermal_chip_info, NULL); > - err = PTR_ERR_OR_ZERO(data->therm); > - if (err) > - goto error; > + data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, > + "system76_acpi", data, &thermal_chip_info, NULL); > + err = PTR_ERR_OR_ZERO(data->therm); > + if (err) > + goto error; > > - system76_battery_init(); > + system76_battery_init(); > + } > > return 0; > > error: > - kfree(data->ntmp); > - kfree(data->nfan); > + if (data->has_open_ec) { > + kfree(data->ntmp); > + kfree(data->nfan); > + } It appears that calling system76_battery_(init/exit) depends on has_open_ec. If so would it make sense to just move the code to those functions (get_objects in init and kfrees in exit)? But I can't tell if there are other calls. David > return err; > } > > @@ -749,14 +750,15 @@ static int system76_remove(struct acpi_device *acpi_dev) > > data = acpi_driver_data(acpi_dev); > > - system76_battery_exit(); > + if (data->has_open_ec) { > + system76_battery_exit(); > + kfree(data->nfan); > + kfree(data->ntmp); > + } > > devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led); > devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led); > > - kfree(data->nfan); > - kfree(data->ntmp); > - > system76_get(data, "FINI"); > > return 0;
Hi, On 12/22/21 19:51, Tim Crawford wrote: > Certain functionality or its implementation in System76 EC firmware may > be different to the proprietary ODM EC firmware. Introduce a new bool, > `has_open_ec`, to guard our specific logic. Detect the use of this by > looking for a custom ACPI method name used in System76 firmware. > > Signed-off-by: Tim Crawford <tcrawford@system76.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I will also add this to the pdx86/fixes branch and send it out to Linus with the next pdx86 fixes pull-req for Linus (since this fixes changes introduced in 5.16). Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/system76_acpi.c | 58 ++++++++++++++-------------- > 1 file changed, 30 insertions(+), 28 deletions(-) > > diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c > index 8b292ee95a14..7299ad08c838 100644 > --- a/drivers/platform/x86/system76_acpi.c > +++ b/drivers/platform/x86/system76_acpi.c > @@ -35,6 +35,7 @@ struct system76_data { > union acpi_object *nfan; > union acpi_object *ntmp; > struct input_dev *input; > + bool has_open_ec; > }; > > static const struct acpi_device_id device_ids[] = { > @@ -279,20 +280,12 @@ static struct acpi_battery_hook system76_battery_hook = { > > static void system76_battery_init(void) > { > - acpi_handle handle; > - > - handle = ec_get_handle(); > - if (handle && acpi_has_method(handle, "GBCT")) > - battery_hook_register(&system76_battery_hook); > + battery_hook_register(&system76_battery_hook); > } > > static void system76_battery_exit(void) > { > - acpi_handle handle; > - > - handle = ec_get_handle(); > - if (handle && acpi_has_method(handle, "GBCT")) > - battery_hook_unregister(&system76_battery_hook); > + battery_hook_unregister(&system76_battery_hook); > } > > // Get the airplane mode LED brightness > @@ -673,6 +666,10 @@ static int system76_add(struct acpi_device *acpi_dev) > acpi_dev->driver_data = data; > data->acpi_dev = acpi_dev; > > + // Some models do not run open EC firmware. Check for an ACPI method > + // that only exists on open EC to guard functionality specific to it. > + data->has_open_ec = acpi_has_method(acpi_device_handle(data->acpi_dev), "NFAN"); > + > err = system76_get(data, "INIT"); > if (err) > return err; > @@ -718,27 +715,31 @@ static int system76_add(struct acpi_device *acpi_dev) > if (err) > goto error; > > - err = system76_get_object(data, "NFAN", &data->nfan); > - if (err) > - goto error; > + if (data->has_open_ec) { > + err = system76_get_object(data, "NFAN", &data->nfan); > + if (err) > + goto error; > > - err = system76_get_object(data, "NTMP", &data->ntmp); > - if (err) > - goto error; > + err = system76_get_object(data, "NTMP", &data->ntmp); > + if (err) > + goto error; > > - data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, > - "system76_acpi", data, &thermal_chip_info, NULL); > - err = PTR_ERR_OR_ZERO(data->therm); > - if (err) > - goto error; > + data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, > + "system76_acpi", data, &thermal_chip_info, NULL); > + err = PTR_ERR_OR_ZERO(data->therm); > + if (err) > + goto error; > > - system76_battery_init(); > + system76_battery_init(); > + } > > return 0; > > error: > - kfree(data->ntmp); > - kfree(data->nfan); > + if (data->has_open_ec) { > + kfree(data->ntmp); > + kfree(data->nfan); > + } > return err; > } > > @@ -749,14 +750,15 @@ static int system76_remove(struct acpi_device *acpi_dev) > > data = acpi_driver_data(acpi_dev); > > - system76_battery_exit(); > + if (data->has_open_ec) { > + system76_battery_exit(); > + kfree(data->nfan); > + kfree(data->ntmp); > + } > > devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led); > devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led); > > - kfree(data->nfan); > - kfree(data->ntmp); > - > system76_get(data, "FINI"); > > return 0; >
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c index 8b292ee95a14..7299ad08c838 100644 --- a/drivers/platform/x86/system76_acpi.c +++ b/drivers/platform/x86/system76_acpi.c @@ -35,6 +35,7 @@ struct system76_data { union acpi_object *nfan; union acpi_object *ntmp; struct input_dev *input; + bool has_open_ec; }; static const struct acpi_device_id device_ids[] = { @@ -279,20 +280,12 @@ static struct acpi_battery_hook system76_battery_hook = { static void system76_battery_init(void) { - acpi_handle handle; - - handle = ec_get_handle(); - if (handle && acpi_has_method(handle, "GBCT")) - battery_hook_register(&system76_battery_hook); + battery_hook_register(&system76_battery_hook); } static void system76_battery_exit(void) { - acpi_handle handle; - - handle = ec_get_handle(); - if (handle && acpi_has_method(handle, "GBCT")) - battery_hook_unregister(&system76_battery_hook); + battery_hook_unregister(&system76_battery_hook); } // Get the airplane mode LED brightness @@ -673,6 +666,10 @@ static int system76_add(struct acpi_device *acpi_dev) acpi_dev->driver_data = data; data->acpi_dev = acpi_dev; + // Some models do not run open EC firmware. Check for an ACPI method + // that only exists on open EC to guard functionality specific to it. + data->has_open_ec = acpi_has_method(acpi_device_handle(data->acpi_dev), "NFAN"); + err = system76_get(data, "INIT"); if (err) return err; @@ -718,27 +715,31 @@ static int system76_add(struct acpi_device *acpi_dev) if (err) goto error; - err = system76_get_object(data, "NFAN", &data->nfan); - if (err) - goto error; + if (data->has_open_ec) { + err = system76_get_object(data, "NFAN", &data->nfan); + if (err) + goto error; - err = system76_get_object(data, "NTMP", &data->ntmp); - if (err) - goto error; + err = system76_get_object(data, "NTMP", &data->ntmp); + if (err) + goto error; - data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, - "system76_acpi", data, &thermal_chip_info, NULL); - err = PTR_ERR_OR_ZERO(data->therm); - if (err) - goto error; + data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, + "system76_acpi", data, &thermal_chip_info, NULL); + err = PTR_ERR_OR_ZERO(data->therm); + if (err) + goto error; - system76_battery_init(); + system76_battery_init(); + } return 0; error: - kfree(data->ntmp); - kfree(data->nfan); + if (data->has_open_ec) { + kfree(data->ntmp); + kfree(data->nfan); + } return err; } @@ -749,14 +750,15 @@ static int system76_remove(struct acpi_device *acpi_dev) data = acpi_driver_data(acpi_dev); - system76_battery_exit(); + if (data->has_open_ec) { + system76_battery_exit(); + kfree(data->nfan); + kfree(data->ntmp); + } devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led); devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led); - kfree(data->nfan); - kfree(data->ntmp); - system76_get(data, "FINI"); return 0;
Certain functionality or its implementation in System76 EC firmware may be different to the proprietary ODM EC firmware. Introduce a new bool, `has_open_ec`, to guard our specific logic. Detect the use of this by looking for a custom ACPI method name used in System76 firmware. Signed-off-by: Tim Crawford <tcrawford@system76.com> --- drivers/platform/x86/system76_acpi.c | 58 ++++++++++++++-------------- 1 file changed, 30 insertions(+), 28 deletions(-)