On Tue, May 15, 2012 at 10:13:22PM +0200, Marek Vasut wrote:
> Dear Greg Kroah-Hartman,
>
> > 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.
>
> Like the rest of the comments in that structure ... I agree it might use some
> further explanation.
>
> > Notify what? Of what? With what? About what?
>
> I tried to make it as generic as possible, so the user can pass whatever kind of
> message he needs. I went on explaining this stuff starting here onwards:
>
> http://www.spinics.net/lists/linux-usb/msg63791.html
I see that, but that patch does not use this callback, so what is the
connection?
And patches need to be self-explanitory, you shouldn't rely on some
other conversation to explain it, how are you going to remember what
this patch did in 10 years when someone asks you (yes, it happens all
the time, sadly...)
> > Ick ick ick.
> >
> > Please make this specific as to exactly what you are wanting to do, this
> > patch is not specific at all.
>
> All right, can you check the discussion above for the details please? I'll
> eventually rework this patch once we come to some conclusion.
Again, I see no connection between the two, as that patch does not use
this hook. What am I missing?
> > Hint, if you are ever passing in a void *,
> > you are probably doing something wrong...
>
> That's true, but the intention was to allow passing any kind of data.
That's not something we ever like doing if at all possible. This isn't
a microkernel :)
>
> >
> > > +static inline int
> > > +usb_phy_notify(struct usb_phy *x, void *data)
> > > +{
> > > + if (x->notify)
> > > + return x->notify(x, data);
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > No one is calling this function, so why add it? You just created a
> > complier warning here, right?
>
> It's then used in here:
> http://www.spinics.net/lists/arm-kernel/msg175152.html
No, it's set there, the function is never called, especially as it is a
static function as you define it here.
Please rework to make sense, as I'm totally lost.
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
