Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality

[Julien Grall <julien.grall@linaro.org>]

On 04/09/2014 06:00 PM, Ian Campbell wrote:
> On Fri, 2014-04-04 at 14:55 +0100, Julien Grall wrote:
>> Hi Vijaya,
>>
>> Thank you for the patch.
>>
>> On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> GIC low level functionality is segregated into
>>> separate functions and are called using registered
>>> callback wherever required.
>>>
>>> This helps to separate generic and hardware functionality
>>> later
>>
>> Honestly, your patch on V1 was far better to read. As you are nearly
>> modify every functions, you should directly move it to a new file (e.g
>> merge with patch #9).
> 
> This is a tricky judgement call, if there is to be mass code motion it
> should be done strictly separately from any functional changes. If that
> can be done all at once in a way which is reviewable then fine, but if
> not then I would much rather err on the side of refactoring and then
> moving as two steps even if the interim version looks a bit odd.
> 
> v1 of this patch certainly did mix the motion with functional changes
> and so separating things out was the correct thing to do. Perhaps the
> functional changes are now done elsewhere and it would be possible to
> revert to a single patch which moved blocks of code out into callbacks,
> but I wouldn't require it.

IHMO, I think everything that is more than splitting the function in 2
(e.g, prototype change, merging functions...) should not be part of this
patch.

Keeping one patch (i'm not taking into account my previous comment) for
code movement and function splitting will avoid strange renaming that is
very confusing in this patch.

If we keep two patch, perhaps the gicv2 callbacks should be prefix by
gicv2_...

Regards,

-- 
Julien Grall