Re: [Qemu-devel] [PATCH] hw/misc: Add support for ADC controller in Xilinx Zynq 7000

[Guenter Roeck]

Hi Edgar,

On 09/08/2015 11:39 AM, Edgar E. Iglesias wrote:
> On Wed, Aug 12, 2015 at 02:33:47PM -0700, Guenter Roeck wrote:
> > Add support for the Xilinx XADC core used in Zynq 7000.
> >
> > References:
> > - Zynq-7000 All Programmable SoC Technical Reference Manual
> > - 7 Series FPGAs and Zynq-7000 All Programmable SoC XADC
> >    Dual 12-Bit 1 MSPS Analog-to-Digital Converter
> >
> > Tested with Linux using qemu machine xilinx-zynq-a9 with devicetree
> > files zynq-zc702.dtb and zynq-zc706.dtb, and kernel configuration
> > multi_v7_defconfig.
>
>
> Adding Alistair to CC.
> A few comments inline.
>
>
> > Signed-off-by: Guenter Roeck <address@hidden>
> > ---
> >   hw/arm/xilinx_zynq.c  |   5 +
> >   hw/misc/Makefile.objs |   1 +
> >   hw/misc/zynq_xadc.c   | 305 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 311 insertions(+)
> >   create mode 100644 hw/misc/zynq_xadc.c

[ ... ]

> > +
> > +static void zynq_xadc_reset(DeviceState *d)
> > +{
> > +    _zynq_xadc_reset(ZYNQ_XADC(d));
>
> Historically we've tried to avoid symbolnames starting with _.
> You could rename or maybe stick to one reset func?
Turns out using one function was easy, after figuring out how to use
the available macros.

> > +
> > +    switch (reg) {
> > +    case CFG:
> > +        s->regs[CFG] = val;
> > +        break;
> > +    case INTSTS:
> > +        s->regs[INTSTS] &= ~val;
> > +        zynq_xadc_update_ints(s);
> > +        break;
> > +    case INTMSK:
> > +        s->regs[INTMSK] = val & 0x003ff;
> > +        zynq_xadc_update_ints(s);
> > +        break;
> > +    case CFIFO:
> > +        xadc_reg = (val >> 16) & 0x007f;
> > +        xadc_cmd = (val >> 26) & 0x0f;
> > +        xadc_data = val & 0xffff;
>
> We have extract* functions to make these field extractions a bit more
> readable. For example see extract32 in include/qemu/bitops.h.
Done, thanks for the hint.

Should I wait for more feedback or resubmit ?

Thanks,
Guenter