Re: [PATCH 1/4] USB: Add MSM OTG Controller driver

[Greg KH <greg@kroah.com>]

On Thu, Nov 18, 2010 at 10:30:16AM +0530, Pavan Kondeti wrote:
> Hi Greg,
> 
> On Wed, Nov 17, 2010 at 08:16:37AM -0800, Greg KH wrote:
> > On Wed, Nov 17, 2010 at 03:53:52PM +0530, Pavan Kondeti wrote:
> > > Hi Greg,
> > > 
> > > On Tue, Nov 16, 2010 at 01:43:46PM -0800, Greg KH wrote:
> > > > On Tue, Nov 09, 2010 at 04:49:48PM +0530, Pavankumar Kondeti wrote:
> > > > > This driver implements PHY initialization, clock management, memory mapping
> > > > > register address space, ULPI IO ops and simple OTG state machine to kick
> > > > > host/peripheral based on Id/VBUS line status.  VBUS/Id lines are tied to a
> > > > > reference voltage on some boards.  Hence provide a sysfs interface to
> > > > > select host/peripheral mode.
> > > > 
> > > > As you are creating a new user/kernel abi, it MUST be documented in the
> > > > Documentation/ABI/ directory.  I can't take this patch set until that
> > > > happens.
> > > > 
> > > Thanks for letting me know this. I will add the documentation for the sysfs file.
> > 
> > Also note that if you are adding a new ABI like this one, it needs to
> > work the same for the other existing OTG drivers as well.  So please
> > also work to fix them to do the same thing, or change your code to work
> > like the existing drivers do (hint, do the latter one...)
> > 
> I am not sure if other OTG driver require this sysfs file.

That's the point, why does this driver require something that no other
driver does?

> USB mode i.e Host/Peripheral is changed based on Id/VBUS status. But
> on some of the MSM boards, Id/VBUS is connected to reference voltage
> and need an additional sysfs file for user to change the operation
> mode.

Are you sure this should be user selectable?  Who is going to do that
selection and how is it going to happen "automatically" like OTG is
supposed to handle?

> Where should a driver specific sysfs file should go in Doc/ABI/ ?

Where all others are, there are lots of examples in that directory,
including a README, right?  Did you read that and it not explain things
sufficiently?  If so, please let me know what can be expanded on in that
file to make it easier for others in the future.


My main complaint here is that you are creating a brand new
kernel/userspace ABI, and you bury it in a driver patch without giving
really any warning or description of it at all.  This is something that
we need to make sure we get correct as you (yes you) will be maintaining
it for the next 12+ years.  This is not something to do lightly at all,
as I'm sure you can imagine.


> > > > Care to redo this one, and your device patch set based on the review
> > > > comments there, and resend them?
> > > > 
> > > I have been advised to reuse the ci13xxx_udc.c gadget controller driver. The
> > > driver currently supports PCI but not platform bus. I am working on making it
> > > to support platform bus. I will post RFC patch soon. Meanwhile I am thinking
> > > of resending host driver patches (as of now host/otg uses header files of device
> > > controller driver). Is this fine?
> > 
> > Would it make much sense to have the host code in the tree at this point
> > in time due to the other changes you are going to have to do for the
> > gadget controller?
> > 
> May be, I should wait till gadget controller driver patches are ready to avoid
> any additional changes in otg driver for supporting gadget.

Yes, that sounds like a good idea.

thanks,

greg k-h