Re: [PATCH v3 06/13] usb: hub: Pass |struct usb_port*| to usb_port_is_power_on()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jul 06, 2026 at 07:08:07PM +0800, Chen-Yu Tsai wrote:
> On Fri, Jul 3, 2026 at 9:33 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Jul 03, 2026 at 09:17:16PM +0800, Chen-Yu Tsai wrote:
> > > On Fri, Jul 3, 2026 at 9:11 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Jul 03, 2026 at 07:03:07PM +0800, Chen-Yu Tsai wrote:

...

> > > > At a brief look this will be the only function that takes usb_port
> > > > instead of usb_hub in the entire hub.h (I don't count container_of()
> > > > as a function). With that being said I would rather see it to be moved
> > > > to port.c altogether (yes, it's more invasive change, but looks more
> > > > consistent).  I would even dare to move struct usb_port (and container_of()
> > > > accompanied with that) and this function to port.h. This might require
> > > > a separate patch, though.
> > >
> > > I agree with the reasoning, especially given the function name. However
> > > I wonder if it would cause problems given the linking order. I'll give
> > > it a try nevertheless and report back.
> >
> > Thanks!
> >
> > In case it won't fly (but I still think it's better to split), can you at least
> > group usb_port APIs and struct? Means moving the proto closer to that struct
> > usb_port followed by container_of().
> 
> Looks like it works out fine. port.o is linked in after hub.o, so moving
> the function from the latter to the former doesn't cause issues. One might
> even say that it is the correct thing to do.

That's what I say! :-)

> > > > Perhaps something like: 1) "move struct usb_port and associated APIs to port.h";
> > > > 2) "...this patch...".
> 
> There are still some left in hub.c:
> 
> static void usb_lock_port(struct usb_port *port_dev)
> static void usb_unlock_port(struct usb_port *port_dev)
> static void port_over_current_notify(struct usb_port *port_dev)

> static void hub_usb3_port_prepare_disable(struct usb_hub *hub,
>                                           struct usb_port *port_dev)

This one takes hub as well.

> These are used only in hub.c and moving them probably makes things
> messier?

Perhaps better naming to each (kinda moving to usb hub namespace)?

> OOTH, the following bits are in port.c but declared in hub.h:
> 
> int usb_hub_create_port_device(struct usb_hub *hub,
>          int port1);
> void usb_hub_remove_port_device(struct usb_hub *hub,
>          int port1);
> 
> Any preferences for these while we're reorganizing the code?

These ones would be good to be in hub.c if there is no circular dependencies or
alike.


-- 
With Best Regards,
Andy Shevchenko