> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Tuesday, October 14, 2014 7:19 PM
> To: Thomas Shao
> Cc: t...@linutronix.de; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
> a...@canonical.com; jasow...@redhat.com; KY Srinivasan
> Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
> time sample
>
> I had a couple small style nits.
>
> On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index
> > 3b9c9ef..1d8390c 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -51,11 +51,30 @@
> > #define HB_WS2008_MAJOR 1
> > #define HB_WS2008_VERSION (HB_WS2008_MAJOR << 16 |
> HB_MINOR)
> >
> > +#define TIMESAMPLE_INTERVAL 5000000000L /* 5s in nanosecond */
>
> If you wanted you could say:
>
> #define TIMESAMPLE_INTERVAL (5 * NSEC_PER_SEC)
>
> > +
> > +/*host sends time sample for every 5s.So the max polling interval
> > +*is 128*5 = 640s.
> > +*/
>
> The comment still has problems throughout. Read
> Documentation/CodingStyle.
>
I will correct the style of the comment.
> > +#define TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
> > +
> > static int sd_srv_version;
> > static int ts_srv_version;
> > static int hb_srv_version;
> > static int util_fw_version;
> >
> > +/*host sends time sample for every 5s.So the initial polling interval
> > +*is 5s.
> > +*/
> > +static s32 adj_interval = 1;
>
> Prefer mundane types instead there is a reason. This should be int because
> it's not specified in a hardware spec. I know you are being consistent with
> the surrounding code, but the surrounding code is bad so don't emulate it.
>
I agree with you. Maybe it's a good idea to correct the surrounding code in
another patch.
> > +
> > +/*The host_time_sync module parameter is used to control the time
> > + sync between host and guest.
> > +*/
> > +static bool host_time_sync;
> > +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
> > +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with
> host");
>
> Maybe say: "Synchronize time with the host"?
Sounds good.
>
> > +
> > static void shutdown_onchannelcallback(void *context); static struct
> > hv_util_service util_shutdown = {
> > .util_cb = shutdown_onchannelcallback, @@ -163,15 +182,61 @@
> static
> > void shutdown_onchannelcallback(void *context)
> > /*
> > * Set guest time to host UTC time.
> > */
> > -static inline void do_adj_guesttime(u64 hosttime)
> > +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)
>
> I'm surprise checkpatch.pl does't complain about this CamelCase.
I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to
forcesync.
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
