On Thu, Apr 28, 2022 at 03:36:34PM +0900, Jung Daehwan wrote:
> 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?
Please submit a full, working driver so these changes can be able to be
properly reviewed. Otherwise it is just a waste of time for us to even
read them, right?
We do not add changes to the kernel that do not work or do anything,
that would be pointless, and cause us extra work and maintenance.
thanks,
greg k-h
