[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 05/22] hw/acpi/aml-build: Add aml_interrupt()
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v6 05/22] hw/acpi/aml-build: Add aml_interrupt() term |
Date: |
Tue, 19 May 2015 14:02:43 +0200 |
On Mon, 18 May 2015 12:33:25 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
> On Mon, May 18, 2015 at 12:05:49PM +0800, Shannon Zhao wrote:
> > On 2015/5/15 20:14, Igor Mammedov wrote:
> > > On Thu, 7 May 2015 16:51:31 +0100
> > > Peter Maydell <address@hidden> wrote:
> > >
> > >> > On 7 May 2015 at 10:29, Shannon Zhao <address@hidden> wrote:
> > >>> > > From: Shannon Zhao <address@hidden>
> > >>> > >
> > >>> > > Add aml_interrupt() for describing device interrupt in resource
> > >>> > > template.
> > >>> > > These can be used to generating DSDT table for ACPI on ARM.
> > >> >
> > >>> > > + /* Interrupt Number */
> > >>> > > + build_append_byte(var->buf, extract32(irq, 0, 8)); /*
> > >>> > > bits[7:0] */
> > >>> > > + build_append_byte(var->buf, extract32(irq, 8, 8)); /*
> > >>> > > bits[15:8] */
> > >>> > > + build_append_byte(var->buf, extract32(irq, 16, 8)); /*
> > >>> > > bits[23:16] */
> > >>> > > + build_append_byte(var->buf, extract32(irq, 24, 8)); /*
> > >>> > > bits[31:24] */
> > >> >
> > >> > You used this twice in the previous patch and again here. Can
> > >> > we factor it out so we can
> > >> > build_append_uint32(var->buf, irq);
> > > I'd prefer to leave it as it is now, yes it's a bit of code duplication
> > > but when you compare to spec it's 1:1 match and easy to compare
> > >
> >
> > I think we could use build_append_uint32 because this really makes the
> > code cleaner and it also matches the spec. When comparing to the spec,
> > uint32 means 4 bytes, so it implies that it includes this byte and next
> > 3 bytes and it also says the 4 bytes are integral.
>
> To me it doesn't, build_append_uint32 suggests integer encoding
> is used which is not the case here. build_append_4byte?
that's why I suggest to just leave it as is. There isn't much duplication here
and reader isn't forced to jump around code to see what build_append_foo()
means.
>
> > --
> > Shannon
>
- [Qemu-devel] [PATCH v6 07/22] hw/arm/virt-acpi-build: Generate FADT table and update ACPI headers, (continued)
Re: [Qemu-devel] [PATCH v6 05/22] hw/acpi/aml-build: Add aml_interrupt() term, Igor Mammedov, 2015/05/15
[Qemu-devel] [PATCH v6 03/22] hw/arm/virt-acpi-build: Basic framework for building ACPI tables on ARM, Shannon Zhao, 2015/05/07