[Qemu-devel] Re: [PATCH 01/14] lm32: add Milkymist AC97 support

[Michael Walle <michael@walle.cc>]

Am Mittwoch 16 März 2011, 19:12:44 schrieb malc:
> > > diff --git a/hw/milkymist-ac97.c b/hw/milkymist-ac97.c
> > > new file mode 100644
> > > index 0000000..6c9e318
> > > --- /dev/null
> > > +++ b/hw/milkymist-ac97.c
> > > @@ -0,0 +1,335 @@
> > > +/*
> > > + *  QEMU model of the Milkymist System Controller.
> > > + *
> > > + *  Copyright (c) 2010 Michael Walle<michael@walle.cc>
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2 of the License, or (at your option) any later version.
> > > + *
> > > + * This library is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library; if not,
> > > see<http://www.gnu.org/licenses/>.
> > > + *
> > > + *
> > > + * Specification available at:
> > > + *   http://www.milkymist.org/socdoc/ac97.pdf
> > > + */
> > > +
> > > +#include "hw.h"
> > > +#include "sysbus.h"
> > > +#include "trace.h"
> > > +#include "audio/audio.h"
> > > +#include "qemu-error.h"
> > > +
> > > +enum {
> > > +    R_AC97_CTRL = 0,
> 
> Unneeded..
I wanted to point out, that the registers begin at offset 0. If i change this, 
i'll need to change all other models, too. Including the ones that have been 
committed.

> > > +    R_AC97_ADDR,
> > > +    R_AC97_DATAOUT,
> > > +    R_AC97_DATAIN,
> > > +    R_D_CTRL,
> > > +    R_D_ADDR,
> > > +    R_D_REMAINING,
> > > +    R_RESERVED,
> > > +    R_U_CTRL,
> > > +    R_U_ADDR,
> > > +    R_U_REMAINING,
> > > +    R_MAX
> > > +};
> > > +
> > > +enum {
> > > +    AC97_CTRL_RQEN  = (1<<0),
> 
> Oookay..
> 
> > > +    AC97_CTRL_WRITE = (1<<1),
> 
> Incinsistent(with previous enum formatting) comma (and C99 only at that)
> 
> > > +};
> > > +
> > > +enum {
> > > +    CTRL_EN = (1<<0),
> 
> Ditto x2
Ok, but it has a hidden agenda. R_MAX must always be the last entry, therefore 
i omitted the comma. Whereas the bit flags could still be extended. Again if 
this is a concern for you, i'll change that and all other models too.

> > > +};
> > > +
> > > +struct MilkymistAC97State {
> > > +    SysBusDevice busdev;
> > > +
> > > +    QEMUSoundCard card;
> > > +    SWVoiceIn *voice_in;
> > > +    SWVoiceOut *voice_out;
> > > +
> > > +    uint32_t regs[R_MAX];
> > > +
> > > +    qemu_irq crrequest_irq;
> > > +    qemu_irq crreply_irq;
> > > +    qemu_irq dmar_irq;
> > > +    qemu_irq dmaw_irq;
> > > +};
> > > +typedef struct MilkymistAC97State MilkymistAC97State;
> > > +
> > > +static void update_voices(MilkymistAC97State *s)
> > > +{
> > > +    if (s->regs[R_D_CTRL]&  CTRL_EN) {
> 
> Space before ampersand would be nice.
mh i think Alex email client messed this up. At least for me, there is one 
space before and one space after the ampersand in my original email.

-- 
Michael