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

[Guenter Roeck <linux@roeck-us.net>]

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 <linux@roeck-us.net>
>> ---
>>   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