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

[malc]

On Thu, 17 Mar 2011, Michael Walle wrote:

> 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<address@hidden>
> > > > + *
> > > > + * 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.

And i just wanted to nitpick that it would be zero even without '= 0' part
(6.7.2.2#3 of ISO/IEC 9899:201x)

> 
> > > > +    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.
> 

Sorry for the noise then. (to Alexander: real men don't use crap MUA
you use)

-- 
mailto:address@hidden