On Wed, Mar 02, 2011 at 01:43:13AM +0000, KY Srinivasan wrote:

quoted

-----Original Message-----
From: Greg KH [mailto:greg@kroah.com]
Sent: Monday, February 28, 2011 9:53 PM
To: KY Srinivasan
Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
Janssen
Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions

On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote:

quoted

This patch combines the two driver abstractions into
a single driver abstraction.

Ah, how sweet.  Unfortunatly you don't say "how" you did this.

Nor do you describe _what_ those two driver abstractions were.  Are we
talking i2c and usb abstractions?  gpio and spi?  Driver core and
platform?  We want to know exactly what is going on here.

My mistake; I will have a more descriptive comment.

quoted

Think of writing something that when you look back, in 3 years, while
staring at a Linux hyperv driver originally written for the 2.6.9
kernel, that somehow never got forward ported and you are tasked with
doing this, that you can just do a simple 'git log drivers/staging/hv/'
and instantly know just from the changelog comments exactly what you
need to do to your driver to clean it up and properly get it to work on
the new 8.2.2 kernel release.

This changelog entry, would require you to go and dig through the guts
of the patch itself, trying to figure out what abstractions you are
talking about, and exactly how they were combined, all the while
wondering _why_ they were combined.

Please, think of your future self, you will thank him in the years to
come by doing this properly.  Not to mention making other's lives easier
if you happen to have escaped this dire task by then.

Oh, you have an extra space up there in the subject, please fix it next
time.

quoted

-int blk_vsc_initialize(struct hv_driver *driver)
+int blk_vsc_initialize(struct driver_context *driver)

"struct driver_context"?  Oh please no.

Greg; this is the patch that consolidates the state in  struct hv_driver into 
struct driver_context. In the spirit of doing one thing in a patch;
other relevant changes are made in:
Patch[5/6]: Changes the name driver_context to hyperv_driver
Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver.

Yes, but on its own, this patch is wrong, that is not a valid name, even
if it is a "temporary" name.

quoted

I realize that you are hopefully going to later rename this to something
else, but remember, a few patches back you thought that the "ctx" name
wasn't nice.  And here you go resuscitating it from the graveyard of
pointy bits.

As I noted in a different email, may be the granularity I chose in breaking up
these patches is causing all this confusion.

Yes, as I think you need to go much finer as you were doing more than
one thing in these patches, and not describing them properly at all.

Please try to redo them in a simpler manner, probably breaking it into
more steps, so we can properly review them.

quoted

And what happens if your future patch is rejected?  You are stuck with a
"driver_context" structure in a subsystem?  That's a pretty big abuse of
the global namespace, don't you think?  It sounds like something that
should go into include/linux/device.h

Please be careful about names, they mean things, even when you think
they don't.

Greg, would it be better if I just had one patch for dealing with all
device related issues and a Separate patch for dealing all driver
related issues?

Again, only do one thing per patch.  So yes, of course they should be
separate.

thanks,

greg k-h