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