Re: [PATCH 1/4] PHY: Add .notify() into struct usb_phy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Fri, May 18, 2012 at 09:19:25PM -0700, Greg Kroah-Hartman wrote:
> On Sat, May 19, 2012 at 09:35:30AM +0800, Richard Zhao wrote:
> > On Tue, May 15, 2012 at 08:14:13AM -0700, Greg Kroah-Hartman wrote:
> > > On Tue, May 15, 2012 at 06:10:19AM +0200, Marek Vasut wrote:
> > > > Add this function so for example USB Host driver can notify
> > > > the PHY about some event. This is useful for me on i.MX28, where
> > > > I need to tell the PHY, from the EHCI interrupt handler, to disable
> > > > disconnection detector (or reenable it).
> > > > 
> > > > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > > > Cc: Felipe Balbi <balbi@xxxxxx>
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: Linux USB <linux-usb@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  include/linux/usb/otg.h |   12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > > > index 38ab3f4..4a74618 100644
> > > > --- a/include/linux/usb/otg.h
> > > > +++ b/include/linux/usb/otg.h
> > > > @@ -117,6 +117,9 @@ struct usb_phy {
> > > >  	int	(*set_suspend)(struct usb_phy *x,
> > > >  				int suspend);
> > > >  
> > > > +	/* for host driver to pass data to the PHY */
> > > > +	int	(*notify)(struct usb_phy *x,
> > > > +				void *data);
> > > 
> > > Wow that is vague.
> > > 
> > > Notify what?  Of what?  With what?  About what?
> > > 
> > > Ick ick ick.
> > > 
> > > Please make this specific as to exactly what you are wanting to do, this
> > > patch is not specific at all.  Hint, if you are ever passing in a void *,
> > > you are probably doing something wrong...
> > > 
> >  
> > how about this one?
> > 
> > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > index 38ab3f4..705e391 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -43,6 +43,12 @@ enum usb_phy_events {
> >  	USB_EVENT_ENUMERATED,   /* gadget driver enumerated */
> >  };
> >  
> > +/* define events for usb host or device driver to notify the PHY */
> > +enum usb_events {
> > +	USB_EVENT_HCD_CONNECT,  /* device connected to the host */
> > +	USB_EVENT_HCD_DISCONNECT, /* device disconnected to the host */
> > +};
> > +
> >  struct usb_phy;
> >  
> >  /* for transceivers connected thru an ULPI interface, the user must
> > @@ -117,6 +123,9 @@ struct usb_phy {
> >  	int	(*set_suspend)(struct usb_phy *x,
> >  				int suspend);
> >  
> > +	/* for host driver to pass data to the PHY */
> > +	int	(*notify)(struct usb_phy *x,
> > +				enum usb_events ev, void *data);
> 
> Let me guess, you have experience with a different operating system...  :)
yes, the style can be often seen. :)
> 
> Closer, but you've heard the famous quote about History and those who do
> not learn it, right?
> 
> Again, be specific, it looks like you want 2 different callbacks here,
> right?  Then make them:
> 	int	notify_connect(struct usb_phy *phy);
> 	int	notify_disconnect(struct usb_phy *phy);
> that should be all that you need.
It's enough for me. The next problem is where to call it.
> 
> We've learned from our past mistakes in trying to pass in events and
> then extending them, and people getting confused, and all hell breaking
> out in other parts of the kernel.  It took a few years to clean up the
> mess, let's not ever do that again, even in tiny ways like this.  Just
> spell it out, be specific, and look, no void * needed at all.
Understood. 

Thanks
Richard
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux