Dear Greg Kroah-Hartman,
> 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?
There's a discussion under that patch that clears my intention with this
callback, that's what I actually wanted to point out.
> 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...)
Right, let's have a V2 of this patch with much better documentation then. But
not before we come to some conclusion with Alan, ok? Though I'm really glad for
your guidance on this, every such hint is very helpful.
> > > 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?
Er well, probably the link at the bottom of my reply.
>
> > > 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 :)
:-D
But seriously then, we'd have to craft a structure to be passed to the PHY that
can harbor all possible kinds of things the people might want to pass to the
PHY.
> > > > +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.
Correct, pointer to the function is set into struct usb_phy in there.
Now if you just check in here:
http://www.spinics.net/lists/arm-kernel/msg175154.html
This is where it's called from.
> Please rework to make sense, as I'm totally lost.
Please give the above link one more go, I hope it clears it up.
> greg k-h
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
