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

[Marek Vasut <marex@xxxxxxx>]

Dear Richard Zhao,

> 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);
>  };
> 
> 
> @@ -170,6 +179,15 @@ usb_phy_shutdown(struct usb_phy *x)
>  		x->shutdown(x);
>  }
> 
> +static inline int
> +usb_phy_notify(struct usb_phy *x, enum usb_events ev, void *data)
> +{
> +	if (x->notify)
> +		return x->notify(x, ev, data);
> +
> +	return 0;
> +}
> +
>  /* for usb host and peripheral controller drivers */
>  #ifdef CONFIG_USB_OTG_UTILS
>  extern struct usb_phy *usb_get_transceiver(void);

Looks good to me ...

> 
> 
> Thanks
> Richard

Best regards,
Marek Vasut
--
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