[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers int
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT |
Date: |
Tue, 22 Dec 2015 16:47:18 +0200 |
On Tue, Dec 22, 2015 at 03:38:24PM +0100, Igor Mammedov wrote:
> On Tue, 22 Dec 2015 11:37:40 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
>
> [...]
> > > > > + for (i = 4; i <= 0xF; i++) {
> > > > > + char *name = g_strdup_printf("_L0%X", i);
> > > > > + method = aml_method(name, 0, AML_NOTSERIALIZED);
> > > > > + aml_append(scope, method);
> > > > > + g_free(name);
> > > > > + }
> > > >
> > > > How about we make aml_method accept ... format instead?
> > > actually instead of making aml_method(format,...) it would be
> > > easier to make it accept Aml* so we could reuse aml_name(format,...)
> > > in the end it would look like:
> > >
> > > Aml gpe_name = aml_name("_L0%X", i);
> > > aml_method(gpe_name, AML_NOTSERIALIZED);
> > >
> > > in addition name object could be reused in other places
> > > that reference that method.
> >
> > Except most methods are simple, so maybe do both APIs.
> > Avoiding string duplication is a good idea,
> > but using string constants works just as well.
> I don't like having 2 APIs, one with 'Aml* name' and other
> with 'const char *name' in C. I'd prefer to have just one
> that suits the majority use cases and
>
> I'm not so comfortable with using format string here directly
> as it would look weird (at least to me):
> aml_method("_L0%X", i, argcount, AML_NOTSERIALIZED);
> - static format string check at compile time won't work here
> or
> aml_method(argcount, AML_NOTSERIALIZED, "_L0%X", i);
> - that should be fine except of that argument order doesn't
> match the way it's in spec, which I'd prefer to keep
Just add ... at the end:
aml_method("_L0%X", argcount, AML_NOTSERIALIZED, i);
looks a bit weird if you have to use it, but
it's uncommon, and the common case looks simple.
> so it leaves for me 2 options:
> 1: use aml_method(aml_name("_L0%X", i), argcount, AML_NOTSERIALIZED)
> It's a bit of code churn and not sure if we really need it
> but I can do if asked for it.
>
> 2: just leave it as is for now, because the most of method names
> are just string constants. These "_L0%X" will be gone after
> cleanup, leaving us with only handlers that use string const and
> do some work.
>
> I can replace g_strdup_printf() with static buffer here if you
> dislike alloc/free in this patch.
So how about just open-coding this loop for now?
That's just about 10 lines of code,
not a big deal.
As you say, will be gone after refactoring.
--
MST
- [Qemu-devel] [PATCH 57/74] pc: acpi: piix4: move IQST() into SSDT, (continued)
- [Qemu-devel] [PATCH 57/74] pc: acpi: piix4: move IQST() into SSDT, Igor Mammedov, 2015/12/09
- [Qemu-devel] [PATCH 65/74] pc: acpi: q35: move ISA bridge into SSDT, Igor Mammedov, 2015/12/09
- [Qemu-devel] [PATCH 49/74] pc: acpi: move MOU device from DSDT to SSDT, Igor Mammedov, 2015/12/09
- [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Igor Mammedov, 2015/12/09
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Michael S. Tsirkin, 2015/12/19
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Michael S. Tsirkin, 2015/12/19
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Igor Mammedov, 2015/12/21
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Igor Mammedov, 2015/12/21
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Michael S. Tsirkin, 2015/12/22
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Igor Mammedov, 2015/12/22
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 54/74] pc: acpi: move remaining GPE handlers into SSDT, Igor Mammedov, 2015/12/22
[Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/09