Dear Greg Kroah-Hartman,
> On Tue, May 15, 2012 at 11:12:27PM +0200, Marek Vasut wrote:
> > 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.
>
> No, you do a callback-per-thing-you-need-to-do, which keeps things sane
> and documentable. And checkable by the complier, none of this void *
> crud, that gets too messy too fast, especially when you are just
> assuming the call receive "knows" what you are referring to (hint, odds
> are it will get wrong very quickly...)
Well, all right. Considering that Richard Zhang actually developed similar
implementation of the MXS USB stuff, I will have to coordiate with him and now I
don't know if this patch will be needed at all.
> > > > > > +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.
>
> Ah, ok, that makes more sense.
>
> You just passed a pointer from the stack, did you really mean to do
> that? (hint, you didn't...)
I actually did, since the data will be available to the function I passed them
to. But I see why you're complaining about this.
>
> Anyway, please rework this to make more sense.
Will do, but let's see what comes out of Richard first.
>
> 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
