Re: [RFC V2 PATCH 02/11] acpi: Implement control method sleep button

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 27 Sep 2024 14:38:57 -0400
Annie Li <annie.li@oracle.com> wrote:

> The control method sleep button is added, as well as its GPE event
> handler.
> 
> Co-developed-by: Miguel Luis <miguel.luis@oracle.com>
> Signed-off-by: Annie Li <annie.li@oracle.com>
> ---
>  hw/acpi/control_method_device.c         | 54 +++++++++++++++++++++++++
>  hw/acpi/meson.build                     |  1 +
>  include/hw/acpi/control_method_device.h | 25 ++++++++++++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/hw/acpi/control_method_device.c b/hw/acpi/control_method_device.c
> new file mode 100644
> index 0000000000..f52c190352
> --- /dev/null
> +++ b/hw/acpi/control_method_device.c
> @@ -0,0 +1,54 @@
> +/*
> + * Control method devices
> + *
> + * Copyright (c) 2023 Oracle and/or its affiliates.
> + *
> + *
> + * Authors:
> + *     Annie Li <annie.li@oracle.com>
> + *
> + *

> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.

replace it with SPDX-License-Identifier like it's done elsewhere

> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/control_method_device.h"
> +#include "hw/mem/nvdimm.h"
> +
> +void acpi_dsdt_add_sleep_button(Aml *scope)
> +{
> +    Aml *dev = aml_device("\\_SB."ACPI_SLEEP_BUTTON_DEVICE);

drop "\\_SB." here and below as well, 

> +    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C0E")));
> +    Aml *pkg = aml_package(2);
> +    aml_append(pkg, aml_int(0x01));
> +    aml_append(pkg, aml_int(0x04));
> +    aml_append(dev, aml_name_decl("_PRW", pkg));
> +    aml_append(dev, aml_operation_region("\\Boo", AML_SYSTEM_IO,
use some sensible name for opreg

> +                                         aml_int(0x201), 0x1));
> +    Aml *field = aml_field("\\Boo", AML_BYTE_ACC, AML_NOLOCK,
> +                           AML_WRITE_AS_ZEROS);
> +    aml_append(field, aml_named_field("SBP", 1));
> +    aml_append(field, aml_named_field("SBW", 1));
> +    aml_append(dev, field);
> +    aml_append(scope, dev);
> +}

also above and below lacks any documentation,
add comments for relevant spec references, like we do with other ACPI
functions. Also perhaps, it's out of order, reviewer has not clue
where from above registers come and how it is supposed to work.

if you invented those registers, there should be a preceding doc patch
that documents them.

Suggest to reorder after patch that implements above registers in hw,
and also comment here where to look for them.

> +
> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope)
> +{
> +     Aml *method = aml_method("_L07", 0, AML_NOTSERIALIZED);
> +     Aml *condition = aml_if(aml_name("\\_SB.SLPB.SBP"));
> +     aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBP")));
> +     aml_append(condition,
> +                aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE),
> +                                    aml_int(0x80)));
> +     aml_append(method, condition);
> +     condition = aml_if(aml_name("\\_SB.SLPB.SBW"));
> +     aml_append(condition, aml_store(aml_int(1), aml_name("\\_SB.SLPB.SBW")));
> +     aml_append(condition,
> +                aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE),
> +                                    aml_int(0x2)));
> +     aml_append(method, condition);
> +     aml_append(scope, method);
> +}
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index fa5c07db90..0b4f1b432d 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -17,6 +17,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_CXL', if_true: files('cxl.c'), if_false: files('cxl-stub.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('control_method_device.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
> diff --git a/include/hw/acpi/control_method_device.h b/include/hw/acpi/control_method_device.h
> new file mode 100644
> index 0000000000..87f8d6fd59
> --- /dev/null
> +++ b/include/hw/acpi/control_method_device.h
> @@ -0,0 +1,25 @@
> +/*
> + * Control method devices
> + *
> + * Copyright (c) 2023 Oracle and/or its affiliates.
> + *
> + *
> + * Authors:
> + *     Annie Li <annie.li@oracle.com>
> + *
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +
> +#ifndef HW_ACPI_CONTROL_METHOD_DEVICE_H
> +#define HW_ACPI_CONTROL_NETHOD_DEVICE_H
> +
> +#define ACPI_SLEEP_BUTTON_DEVICE "SLPB"
> +
> +void acpi_dsdt_add_sleep_button(Aml *scope);
> +void acpi_dsdt_add_sleep_gpe_event_handler(Aml *scope);
> +
> +#endif