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

[Pavan Kondeti <pkondeti@codeaurora.org>]

On Wed, Nov 17, 2010 at 09:20:16PM -0800, Greg KH wrote:
> 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?
> 
The board I am using is a reference board where Id is grounded always.  But we
use it in peripheral mode for ADB (Android debugging Bridge). So user is going
to write into the sysfs file ("host"/"peripheral"/"none"). But this is mainly
for debugging/testing purpose.
> > 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.
> 
I have read the README file and it only talks about different levels of
stability. I have not looked at examples. After looking at examples I
understand that testing/sysfs-driver-usb-msm_otg is the correct file.
> 
> 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.
> 
Okay. I understand now. Thanks for the explanation. I should not have used a
sysfs file, which creates a new kernel/userspace ABI. I will use debugfs as it
is mainly intended for debugging purpose. The only reason I have gone
for sysfs is that to have this option available without CONFIG_DEBUGFS.
> 
-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.