On Thu, Apr 28, 2022 at 07:19:04AM +0200, Krzysztof Kozlowski wrote: > On 28/04/2022 03:29, Jung Daehwan wrote: > > On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote: > >> On 26/04/2022 11:18, Daehwan Jung wrote: > >>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat > >>> driver mainly and extends some functions by xhci hooks and overrides. > >>> > >>> It supports USB Audio offload with Co-processor. It only cares DCBAA, > >>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated > >>> on specific address with xhci hooks. Co-processor could use them directly > >>> without xhci driver after then. > >> > >> This does not look like developed in current Linux kernel, but something > >> out-of-tree, with some other unknown modifications. This is not how the > >> code should be developed. Please rebase on linux-next and drop any > >> unrelated modifications (these which are not sent with this patchset). > >> > > > > I've been developing on linux-next and I rebase before submitting. > > Could you tell me one of dropped modifications or patches? > > > >> (...) > >> > >>> + > >>> +static int xhci_exynos_suspend(struct device *dev) > >>> +{ > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > >>> + > >>> + /* TODO: AP sleep scenario*/ > >> > >> Shall the patchset be called RFC? > >> > > OK. I will add RFC for this patch on next submission. > > > >>> + > >>> + return xhci_suspend(xhci, device_may_wakeup(dev)); > >>> +} > >>> + > >>> +static int xhci_exynos_resume(struct device *dev) > >>> +{ > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > >>> + int ret; > >>> + > >>> + /* TODO: AP resume scenario*/ > >>> + > >>> + ret = xhci_resume(xhci, 0); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + pm_runtime_disable(dev); > >>> + pm_runtime_set_active(dev); > >>> + pm_runtime_enable(dev); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static const struct dev_pm_ops xhci_exynos_pm_ops = { > >>> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume) > >>> +}; > >>> + > >>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver"); > >>> +MODULE_LICENSE("GPL"); > >> > >> You don't have list of compatibles (and missing bindings), driver > >> definition, driver registration. Entire solution is not used - nothing > >> calls xhci_exynos_vendor_init(), because nothign uses "ops". > >> > > > > xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init) > > [v4,2/5] usb: host: add xhci hooks for xhci-exynos > > ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in > > all xhci-exynos. > > > Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is > not called anywhere, so the xhci_vendor_init() does not call > xhci_exynos_vendor_init(). > You are right. xhci_exynos_register_vendor_ops should be called by other module. It's only thing not called anywhere in this patchset. I don't uses xhci-exynos alone in my scenario. Other module loads this on runtime. > > > > xhci-exynos is not a standalone driver. It could be enabled when other module > > makes xhci platform driver probed as it uses xhci platform mainly. > > It "could be" or "will be"? We do not talk here about theoretical usage > of the driver, but a real one. > > > I thought I just used existing compltible not adding new one. > > I will add them if needed. > > Since you called everything here as "exynos" it is specific to one > hardware and not-reusable on anything else. How can then you use some > other compatible? It would be a misuse of Devicetree bindings. > I got it. Let me add them. Is it still necessary if it is only used by other module on runtime as I said above? Best Regards, Jung Daehwan > Unless this is not specific to "exynos", but then please remove any > exynos and vendor references and make it integrated in generic XHCI > working for all drivers. > > > >> This does not work and it makes it impossible to test it. Please provide > >> proper XHCI Exynos driver, assuming you need it and it is not part of > >> regular Exynos XHCI drivers (DWC3 and so on). > >> > > > > What makes you think it doesn't work? > > Not possible to probe. The code cannot be loaded. > > > I think it's almost proper. I just removed > > other IPs code like Co-Processor(we call it abox) or Power Management because > > it would make build-error. I've added hooking points in some files(xhci-mem.c, > > xhci.c..) and ops are implemented in xhci-exynos. It's mainly operated with > > xhci platform driver. > > Best regards, > Krzysztof >