> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Tuesday, October 14, 2014 7:19 PM > To: Thomas Shao > Cc: tglx@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; 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 devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
