On Friday, July 30, 2010, Kenji Kaneshige wrote:
> Make capabilitis buffer in acpi_pci_run_osc() instead of caller of
> this function. This makes the code a little cleaner. This has no
> functional changes.
>
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>
Well, I'm not sure if that really is an improvement. With the patch
acpi_pci_run_osc() simply has more arguments that doesn't really
make it more readable IMHO.
> ---
> drivers/acpi/pci_root.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> Index: linux-2.6.35-rc6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.35-rc6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6.35-rc6/drivers/acpi/pci_root.c
> @@ -206,9 +206,10 @@ static void acpi_pci_bridge_scan(struct
>
> static u8 pci_osc_uuid_str[] = "33DB4D5B-1FF7-401C-9657-7441C03DD766";
>
> -static acpi_status acpi_pci_run_osc(acpi_handle handle,
> - const u32 *capbuf, u32 *retval)
> +static acpi_status acpi_pci_run_osc(acpi_handle handle, u32 support,
> + u32 control, bool query, u32 *retval)
> {
> + u32 capbuf[3];
> struct acpi_osc_context context = {
> .uuid_str = pci_osc_uuid_str,
> .rev = 1,
> @@ -217,6 +218,10 @@ static acpi_status acpi_pci_run_osc(acpi
> };
> acpi_status status;
>
> + capbuf[OSC_QUERY_TYPE] = (query == true) ? OSC_QUERY_ENABLE : 0;
> + capbuf[OSC_SUPPORT_TYPE] = support;
> + capbuf[OSC_CONTROL_TYPE] = control;
> +
> status = acpi_run_osc(handle, &context);
> if (ACPI_SUCCESS(status)) {
> *retval = *((u32 *)(context.ret.pointer + 8));
> @@ -228,17 +233,16 @@ static acpi_status acpi_pci_run_osc(acpi
> static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
> {
> acpi_status status;
> - u32 support_set, result, capbuf[3];
> + u32 result;
>
> /* do _OSC query for all possible controls */
> - support_set = root->osc_support_set | (flags & OSC_PCI_SUPPORT_MASKS);
> - capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
> - capbuf[OSC_SUPPORT_TYPE] = support_set;
> - capbuf[OSC_CONTROL_TYPE] = OSC_PCI_CONTROL_MASKS;
> -
> - status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
> + flags &= OSC_PCI_SUPPORT_MASKS;
> + status = acpi_pci_run_osc(root->device->handle,
> + root->osc_support_set | flags,
> + OSC_PCI_CONTROL_MASKS,
> + true, &result);
> if (ACPI_SUCCESS(status)) {
> - root->osc_support_set = support_set;
> + root->osc_support_set |= flags;
To me, the old code is cleaner. I would do
+ flags &= OSC_PCI_SUPPORT_MASKS;
+ flags |= root->osc_support_set;
+ status = acpi_pci_run_osc(root->device->handle,
+ flags,
+ OSC_PCI_CONTROL_MASKS,
+ true, &result);
if (ACPI_SUCCESS(status)) {
- root->osc_support_set = support_set;
+ root->osc_support_set = flags;
but that's almost the same as the old code.
> root->osc_control_qry = result;
> root->osc_queried = 1;
> }
> @@ -373,7 +377,7 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
> acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
> {
> acpi_status status;
> - u32 control_req, result, capbuf[3];
> + u32 control_req, result;
> acpi_handle tmp;
> struct acpi_pci_root *root;
>
> @@ -407,10 +411,10 @@ acpi_status acpi_pci_osc_control_set(acp
> goto out;
> }
>
> - capbuf[OSC_QUERY_TYPE] = 0;
> - capbuf[OSC_SUPPORT_TYPE] = root->osc_support_set;
> - capbuf[OSC_CONTROL_TYPE] = root->osc_control_set | control_req;
> - status = acpi_pci_run_osc(handle, capbuf, &result);
> + status = acpi_pci_run_osc(handle,
> + root->osc_support_set,
> + root->osc_control_set | control_req,
> + false, &result);
> if (ACPI_SUCCESS(status))
> root->osc_control_set = result;
> out:
Overall, I don't like this change, sorry.
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
