[U-Boot] [PATCH] dm: usb: Make usb_lowlevel_init set the default ops pointer

[Simon Glass]

Hi Tom.

On 17 April 2015 at 16:02, Tom Rini <trini at konsulko.com> wrote:
> On Fri, Apr 17, 2015 at 03:28:30PM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> On 17 April 2015 at 14:31, Tom Rini <trini at konsulko.com> wrote:
>> > In order to switch to a set of function pointers for certain operations
>> > we need to ensure that the defaults are first set so that they can be
>> > overriden as needed by SoC/board specific code.
>> >
>> > Signed-off-by: Tom Rini <trini at konsulko.com>
>> > ---
>> >  drivers/usb/host/ehci-hcd.c |   10 +++++++---
>> >  1 file changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> > index 79cf3b3..e7c438c 100644
>> > --- a/drivers/usb/host/ehci-hcd.c
>> > +++ b/drivers/usb/host/ehci-hcd.c
>> > @@ -945,9 +945,7 @@ const struct ehci_ops default_ehci_ops = {
>> >
>> >  static void ehci_setup_ops(struct ehci_ctrl *ctrl, const struct ehci_ops *ops)
>> >  {
>> > -       if (!ops) {
>> > -               ctrl->ops = default_ehci_ops;
>> > -       } else {
>>
>> Do we need to remove this? I think this will break when driver model
>> is used, if NULL is passed in to select the default ops.
>
> Nope, just cleaning up the code as-is.

I'm missing something though. For ehci_register() it calls this will
ops possibly equal to NULL, expecting it to set up the ops.

>
>> > +       if (ops) {
>> >                 ctrl->ops = *ops;
>> >                 if (!ctrl->ops.set_usb_mode)
>> >                         ctrl->ops.set_usb_mode = ehci_set_usbmode;
>> > @@ -1095,6 +1093,12 @@ int usb_lowlevel_init(int index, enum usb_init_type init, void **controller)
>> >         uint tweaks = 0;
>> >         int rc;
>> >
>> > +       /**
>> > +        * Set ops to default_ehci_ops, ehci_hcd_init should call
>> > +        * ehci_set_controller_priv to change any of these function pointers.
>> > +        */
>> > +       ctrl->ops = default_ehci_ops;
>> > +
>>
>> OK what I missed is that lots of things call ehci_hcd_init(). I only
>> changed a few to call ehci_set_controller_priv().
>
> Yes, you changed the ehci_hcd_init()'s that need to call
> ehci_set_controller_priv() to either set a priv or now to set the ops
> pointer.  Most need neither so I think this is the right solution.

Sounds good.

>
> --
> Tom

Regards,
Simon