From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v5 07/22] xen/arm: ITS: Add virtual ITS driver Date: Fri, 31 Jul 2015 11:32:12 +0100 Message-ID: <1438338732.30740.54.camel@citrix.com> References: <1437995524-19772-1-git-send-email-vijay.kilari@gmail.com> <1437995524-19772-8-git-send-email-vijay.kilari@gmail.com> <55B7B820.5020708@citrix.com> <55BB4A94.9090204@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55BB4A94.9090204@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , Vijay Kilari Cc: Stefano Stabellini , Prasun Kapoor , manish.jaggi@caviumnetworks.com, Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Vijaya Kumar K List-Id: xen-devel@lists.xenproject.org On Fri, 2015-07-31 at 11:14 +0100, Julien Grall wrote: > 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. It's also acceptable to indent string constants by less than what would be expected to avoid this, e.g. printk(XENLOG_G_ERR d%"PRId32": vITS: Fail to get vdevice for vdevid 0x%"PRIx32"\n", (also, s/Fail/Failed/ or perhaps "Unable"). Also, I don't think PRId32 is what we should use for d->domain_id. The actual type is uint16_t, which would suggest PRIu16 (or maybe PRId16) is formally correct but in actuality we normally use %u or %d in this case (%u is probably more correct, although it appears to be in the minority). > > > > + 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. I've also suggest elsewhere not adding these new files to the build until later in the series, such that the user of this function is already added by the time the file starts to get compiled. That approach is usually acceptable when introducing a large new file which isn't used until towards the end of the series. Of course you need to avoid calling any functions in that file until after you add it to the build, which introduces a different constraint on the series ordering, so it's not always possible to make use of this approach. Ian.