On Mon, 18 May 2015 12:33:25 +0200 "Michael S. Tsirkin" <[email protected]> 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 <[email protected]> wrote: > > > > > >> > On 7 May 2015 at 10:29, Shannon Zhao <[email protected]> wrote: > > >>> > > From: Shannon Zhao <[email protected]> > > >>> > > > > >>> > > 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 >
