lkml.org 
[lkml]   [2011]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
Date


> -----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:
> > 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.

>
> 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.
>
> > -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.

>
> 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.

>
> 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?

Regards,

K. Y


\
 
 \ /
  Last update: 2011-03-02 02:45    [W:0.064 / U:10.532 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site