Re: [PATCH v5 07/22] xen/arm: ITS: Add virtual ITS driver

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 31/07/15 07:49, Vijay Kilari wrote:
>>> +static int vits_vitt_entry(struct domain *d, uint32_t devid,
>>> +                           uint32_t event, struct vitt *entry, bool_t set)
>>> +{
>>> +    struct vdevice_table dt_entry;
>>> +    paddr_t vitt_entry;
>>> +    uint64_t offset;
>>> +
>>> +    BUILD_BUG_ON(sizeof(struct vitt) != 8);
>>> +
>>> +    if ( vits_get_vdevice_entry(d, devid, &dt_entry) )
>>> +    {
>>> +        printk(XENLOG_G_ERR
>>> +                "d%"PRId32": vITS: Fail to get vdevice for vdev 0x%"PRIx32"\n",
>>
>> s/vdev/vdevid/
> 
> I think, to manage within 80 char, I used just "vdev"

80 character in the source file I guess? If so, you should avoid this
kind of cutting just for coding style benefits. We are not so strict on it.

> 
>>
>>> +                d->domain_id, devid);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* dt_entry is validated in vits_get_vdevice_entry */
>>
>> s/is validated/has been validated/
>>
>> [..]
>>
>>> +int vits_set_vitt_entry(struct domain *d, uint32_t devid,
>>> +                        uint32_t event, struct vitt *entry)
>>
>> Same remark as vits_set_vdevice_entry.
> 
> I have made non-static for compilation purpose. I will try to introduce
> this in the patch where it is used. But it is more logical to have this
> in this patch. Anyway I forget to make it static in later patches

Having introduce static here would have avoid forgetting the static
later... It's just a matter of how you split your series.

For instance, if you would have merge this patch with #8, making this
function non-static wouldn't have been necessary.

Regards,

-- 
Julien Grall