From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:53488)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <linux@roeck-us.net>) id 1ZZfmS-0006wC-8R
	for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:51:20 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <linux@roeck-us.net>) id 1ZZfmO-0003c9-32
	for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:51:20 -0400
Received: from bh-25.webhostbox.net ([208.91.199.152]:59280)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <linux@roeck-us.net>) id 1ZZfmN-0003af-SD
	for qemu-devel@nongnu.org; Wed, 09 Sep 2015 09:51:16 -0400
Message-ID: <55F0394D.8030907@roeck-us.net>
Date: Wed, 09 Sep 2015 06:51:09 -0700
From: Guenter Roeck <linux@roeck-us.net>
MIME-Version: 1.0
References: <1439415227-24938-1-git-send-email-linux@roeck-us.net>
	<20150908183901.GJ12618@toto>
In-Reply-To: <20150908183901.GJ12618@toto>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] hw/misc: Add support for ADC controller in
 Xilinx Zynq 7000
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Peter Crosthwaite <peter.crosthwaite@xilinx.com>, alistair.francis@xilinx.com, qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>

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