Re: [PATCH 03/12 v3] xen/arm: vpl011: Add pl011 uart emulation in Xen

From: Julien Grall <julien.grall@arm.com>
To: Bhupinder Thakur <bhupinder.thakur@linaro.org>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH 03/12 v3] xen/arm: vpl011: Add pl011 uart emulation in Xen
Date: Thu, 1 Jun 2017 14:56:51 +0100	[thread overview]
Message-ID: <35348fd3-cdca-a4f9-bb12-92ba93cc5e7b@arm.com> (raw)
In-Reply-To: <CACtJ1JRQn9oLKpo8PAdxxuM-gJNJL6aZaU-OfkMkeUVxrny4-g@mail.gmail.com>

Hi Bhupinder,

On 01/06/17 14:34, Bhupinder Thakur wrote:
> On 26 May 2017 at 19:12, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
>>>> +
>>>> +    switch ( vpl011_reg )
>>>> +    {
>>>> +    case DR:
>>>
>>>
>>> As mentioned above, you could do:
>>>
>>> {
>>>      uint8_t ch;
>>>      ....
>>> }
>>>
>>>> +        vpl011_read_data(v->domain, &ch);
>>>> +        *r = ch;
>>>
>>>
>>> Please use vreg_reg32_extract(...).
>>>
>> ok.
>>
>>>> +        break;
>>>
>>>
>>> I admit I would prefer the "return 1;" here rather than "break;". This makes
>>> easier to follow the emulation for a given register.
>>>
>>> I would even be in favor of duplicating the "if ( !vpl011... )" in each case
>>> for the same reason.
>
> Do you mean that I should repeat the vpl011_reg32_check_access() and
> return for each switch case?

I think it would make easier to follow the emulation if you repeat the 
if (vpl011_reg32_check_access) and the return 1 in each case.

So each register emulation is standalone.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel