Re: [PATCH v4 4/5] usb: host: add some to xhci overrides for xhci-exynos

[Greg Kroah-Hartman]

On Wed, Apr 27, 2022 at 06:19:01PM +0900, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 12:23:30PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 06:18:47PM +0900, Daehwan Jung wrote:
> > > Co-processor needs some information about connected usb device.
> > > It's proper to pass information after usb device gets address when
> > > getting "Set Address" command. It supports vendors to implement it
> > > using xhci overrides. There're several power scenarios depending
> > > on vendors. It gives vendors flexibilty to meet their power requirement.
> > > They can override suspend and resume of root hub.
> > > 
> > > Signed-off-by: Daehwan Jung <dh10.jung@xxxxxxxxxxx>
> > > ---
> > >  drivers/usb/host/xhci.c | 6 ++++++
> > >  drivers/usb/host/xhci.h | 4 ++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 5ccf1bbe8732..8b3df1302650 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -5555,6 +5555,12 @@ void xhci_init_driver(struct hc_driver *drv,
> > >  			drv->check_bandwidth = over->check_bandwidth;
> > >  		if (over->reset_bandwidth)
> > >  			drv->reset_bandwidth = over->reset_bandwidth;
> > > +		if (over->address_device)
> > > +			drv->address_device = over->address_device;
> > > +		if (over->bus_suspend)
> > > +			drv->bus_suspend = over->bus_suspend;
> > > +		if (over->bus_resume)
> > > +			drv->bus_resume = over->bus_resume;
> > >  	}
> > >  }
> > >  EXPORT_SYMBOL_GPL(xhci_init_driver);
> > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > > index 3a414a2f41f0..5bc621e77762 100644
> > > --- a/drivers/usb/host/xhci.h
> > > +++ b/drivers/usb/host/xhci.h
> > > @@ -1947,6 +1947,9 @@ struct xhci_driver_overrides {
> > >  			     struct usb_host_endpoint *ep);
> > >  	int (*check_bandwidth)(struct usb_hcd *, struct usb_device *);
> > >  	void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
> > > +	int (*address_device)(struct usb_hcd *hcd, struct usb_device *udev);
> > > +	int (*bus_suspend)(struct usb_hcd *hcd);
> > > +	int (*bus_resume)(struct usb_hcd *hcd);
> > >  };
> > >  
> > >  #define	XHCI_CFC_DELAY		10
> > > @@ -2103,6 +2106,7 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
> > >  		       struct usb_host_endpoint *ep);
> > >  int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
> > >  void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
> > > +int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev);
> > 
> > You do not use this function in this change, why include it in here?
> > 
> > Please reorganize your patch series to only include what you need for
> > each step, as-is it's kind of out-of-order and might not build at each
> > step along the way (or it might, it's hard to determine...)
> > 
> 
> This patch is to add function pointers. xhci-exynos or other user could use it.

But this commit has nothing to do with xhci_address_device(), why add it
to the .h file here?

> If I reorganize them as you said, all patches depend on xhci-exynos.

Of course, we need real users of the functions you export, otherwise
they do not need to be exported.

You can export them in one commit, and then use them in the next, but
that's not what you are doing here at all.  You have a mix of .c and .h
changes across different commits that are not coordinated at all.

> That's because hooks and override are only used in xhci-exynos for now.
> I don't want user driver effects common files like xhci platform.
> I've tried removing dependancies.. That's why I split patches like that.

Splitting patches is the correct way, just do it so that it makes sense.
As you can see by the kbuild reports, what you have here is not correct.

thanks,

greg k-h