| Message ID | 20210913110805.12387-1-pobrn@protonmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v1] platform/x86: wmi: initialize method name directly | expand |
Hi, On 9/13/21 1:08 PM, Barnabás Pőcze wrote: > Instead of calling `snprintf()`, generate the method > name in the initializer. This way the array size > is determined automatically, specifying it explicitly > is no longer needed. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> In "platform/x86: wmi: introduce helper to generate method names" you added a get_acpi_method_name() helper for generating WMI method-names and you are using that everywhere else. IMHO it would be better to also use that here. Is there any specific reason why you are not using this here ? Regards, Hans > --- > drivers/platform/x86/wmi.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index e6997be206f1..1cde9dd417c4 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -184,15 +184,17 @@ static int get_subobj_info(acpi_handle handle, const char *pathname, > > static acpi_status wmi_method_enable(struct wmi_block *wblock, bool enable) > { > - struct guid_block *block; > - char method[5]; > + acpi_handle handle = wblock->acpi_device->handle; > + struct guid_block *block = &wblock->gblock; > acpi_status status; > - acpi_handle handle; > - > - block = &wblock->gblock; > - handle = wblock->acpi_device->handle; > + char method[] = { > + 'W', > + 'E', > + hex_asc_upper_hi(block->notify_id), > + hex_asc_upper_lo(block->notify_id), > + '\0' > + }; > > - snprintf(method, 5, "WE%02X", block->notify_id); > status = acpi_execute_simple_method(handle, method, enable); > if (status == AE_NOT_FOUND) > return AE_OK; > -- > 2.33.0 > >
Hi 2021. szeptember 13., hétfő 13:53 keltezéssel, Hans de Goede írta: > In "platform/x86: wmi: introduce helper to generate method names" > you added a get_acpi_method_name() helper for generating WMI > method-names and you are using that everywhere else. > > IMHO it would be better to also use that here. Is there any > specific reason why you are not using this here ? Yes, indeed, but `get_acpi_method_name()` uses the `object_id` of the GUID block, this one uses the `notify_id`. And it seemed problematic to find a simple way to reconcile the differences, so I haven't really given it much thought. I'm open to suggestions. Regards, Barnabás Pőcze
Hi, On 9/13/21 2:15 PM, Barnabás Pőcze wrote: > Hi > > > 2021. szeptember 13., hétfő 13:53 keltezéssel, Hans de Goede írta: >> In "platform/x86: wmi: introduce helper to generate method names" >> you added a get_acpi_method_name() helper for generating WMI >> method-names and you are using that everywhere else. >> >> IMHO it would be better to also use that here. Is there any >> specific reason why you are not using this here ? > > Yes, indeed, but `get_acpi_method_name()` uses the `object_id` of the GUID block, > this one uses the `notify_id`. And it seemed problematic to find a simple > way to reconcile the differences, so I haven't really given it much thought. > I'm open to suggestions. Ah I see, TBH I liked your original patch just adding sizeof() better then the new one. Or maybe replace the 2 "5"-s used in the snprintf version with WMI_ACPI_METHOD_NAME_SIZE ? Regards, Hans
Hi 2021. szeptember 13., hétfő 14:20 keltezéssel, Hans de Goede írta: > Hi, > > On 9/13/21 2:15 PM, Barnabás Pőcze wrote: > > Hi > > > > > > 2021. szeptember 13., hétfő 13:53 keltezéssel, Hans de Goede írta: > >> In "platform/x86: wmi: introduce helper to generate method names" > >> you added a get_acpi_method_name() helper for generating WMI > >> method-names and you are using that everywhere else. > >> > >> IMHO it would be better to also use that here. Is there any > >> specific reason why you are not using this here ? > > > > Yes, indeed, but `get_acpi_method_name()` uses the `object_id` of the GUID block, > > this one uses the `notify_id`. And it seemed problematic to find a simple > > way to reconcile the differences, so I haven't really given it much thought. > > I'm open to suggestions. > > Ah I see, TBH I liked your original patch just adding sizeof() better > then the new one. Or maybe replace the 2 "5"-s used in the snprintf > version with WMI_ACPI_METHOD_NAME_SIZE ? > [...] I will not resend it, but please feel free to apply the older patch if think it is better. Personally, I don't like it, but oh well... Best regards, Barnabás Pőcze
Hi, On 9/13/21 2:39 PM, Barnabás Pőcze wrote: > Hi > > > 2021. szeptember 13., hétfő 14:20 keltezéssel, Hans de Goede írta: >> Hi, >> >> On 9/13/21 2:15 PM, Barnabás Pőcze wrote: >>> Hi >>> >>> >>> 2021. szeptember 13., hétfő 13:53 keltezéssel, Hans de Goede írta: >>>> In "platform/x86: wmi: introduce helper to generate method names" >>>> you added a get_acpi_method_name() helper for generating WMI >>>> method-names and you are using that everywhere else. >>>> >>>> IMHO it would be better to also use that here. Is there any >>>> specific reason why you are not using this here ? >>> >>> Yes, indeed, but `get_acpi_method_name()` uses the `object_id` of the GUID block, >>> this one uses the `notify_id`. And it seemed problematic to find a simple >>> way to reconcile the differences, so I haven't really given it much thought. >>> I'm open to suggestions. >> >> Ah I see, TBH I liked your original patch just adding sizeof() better >> then the new one. Or maybe replace the 2 "5"-s used in the snprintf >> version with WMI_ACPI_METHOD_NAME_SIZE ? >> [...] > > I will not resend it, but please feel free to apply the older patch if think it is better. > Personally, I don't like it, but oh well... In that case I'm just going to leave this as is, thank you for all the (other) nice cleanups! Regards, Hans
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index e6997be206f1..1cde9dd417c4 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -184,15 +184,17 @@ static int get_subobj_info(acpi_handle handle, const char *pathname, static acpi_status wmi_method_enable(struct wmi_block *wblock, bool enable) { - struct guid_block *block; - char method[5]; + acpi_handle handle = wblock->acpi_device->handle; + struct guid_block *block = &wblock->gblock; acpi_status status; - acpi_handle handle; - - block = &wblock->gblock; - handle = wblock->acpi_device->handle; + char method[] = { + 'W', + 'E', + hex_asc_upper_hi(block->notify_id), + hex_asc_upper_lo(block->notify_id), + '\0' + }; - snprintf(method, 5, "WE%02X", block->notify_id); status = acpi_execute_simple_method(handle, method, enable); if (status == AE_NOT_FOUND) return AE_OK;
Instead of calling `snprintf()`, generate the method name in the initializer. This way the array size is determined automatically, specifying it explicitly is no longer needed. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- drivers/platform/x86/wmi.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) -- 2.33.0