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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien & Ian,

On Wed, Apr 9, 2014 at 10:37 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>>>
>>>> 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_...
>

Having 2 patches one for with code changes and
other with pure code movement is fine. but first patch is always confusing
to make this changes.

I prefer to have single patch with changes and code movement as in V1 and this
makes job simple and I think we should bear with this.

> Regards,
>
> --
> Julien Grall

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