On Mon, Mar 21, 2022 at 04:45:01PM +0100, Bjørn Mork wrote: > Daehwan Jung <dh10.jung@xxxxxxxxxxx> writes: > > > +++ b/drivers/usb/host/xhci-exynos.c > > @@ -0,0 +1,982 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * xhci-exynos.c - xHCI host controller driver platform Bus Glue for Exynos. > > + * > > + * Copyright (C) 2022 Samsung Electronics Incorporated - http://www.samsung.com > > + * Author: Daehwan Jung <dh10.jung@xxxxxxxxxxx> > > + * > > + * A lot of code borrowed from the Linux xHCI driver. > > + */ > > +#include <linux/clk.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/module.h> > > +#include <linux/pci.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/usb/phy.h> > > +#include <linux/slab.h> > > +#include <linux/acpi.h> > > +#include <linux/usb/of.h> > > + > > +#include "xhci.h" > > +#include "xhci-plat.h" > > +#include "xhci-mvebu.h" > > +#include "xhci-rcar.h" > > The xhci-plat.c file is Copyright (C) 2012 Texas Instruments Incorporated > You can't just steal it. > > Besides, even if you could, this isn't about copying as much code as > posible from A to B. The point is to add as *little* code as possible > to support your hardware. > Hi bjorn, Thanks for your comment. I will write more detail about copyright on next submission. > > +static int xhci_exynos_vendor_init(struct xhci_hcd *xhci) > > +{ > > + /* TODO */ > > + return 0; > > +} > > And you didn't even add that? > > > +static int xhci_exynos_wake_lock(struct xhci_hcd_exynos *xhci_exynos, > > + int is_main_hcd, int is_lock) > > +{ > > + struct usb_hcd *hcd = xhci_exynos->hcd; > > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > + struct wakeup_source *main_wakelock, *shared_wakelock; > > + > > + main_wakelock = xhci_exynos->main_wakelock; > > + shared_wakelock = xhci_exynos->shared_wakelock; > > Are these fields initialized anywhere? > > > > + > > + if (xhci->xhc_state & XHCI_STATE_REMOVING) > > + return -ESHUTDOWN; > > + > > + if (is_lock) { > > bool? > Yes, Currently I use it as if bool. > > + if (is_main_hcd) > > another bool? Same > > > + __pm_stay_awake(main_wakelock); > > + else > > + __pm_stay_awake(shared_wakelock); > > + } else { > > + if (is_main_hcd) > > + __pm_relax(main_wakelock); > > + else > > + __pm_relax(shared_wakelock); > > + } > > Looks interesting. Are you signalling relax/wakeups events to the PM > core on device suspend/resume? Why? I want to enter system sleep if possible at this point. It's related to power scenario on my SOC. > > > +static int xhci_exynos_address_device(struct usb_hcd *hcd, struct usb_device *udev) > > +{ > > + struct xhci_hcd *xhci; > > + int ret; > > + > > + ret = xhci_address_device(hcd, udev); > > + xhci = hcd_to_xhci(hcd); > > + > > + return ret; > > +} > > What's left here if we drop the unused parts? > There's some missing code. Thanks.. > > +#ifdef CONFIG_OF > > +static const struct xhci_plat_priv xhci_plat_marvell_armada = { > > + .init_quirk = xhci_mvebu_mbus_init_quirk, > > +}; > > + > > +static const struct xhci_plat_priv xhci_plat_marvell_armada3700 = { > > + .plat_setup = xhci_mvebu_a3700_plat_setup, > > + .init_quirk = xhci_mvebu_a3700_init_quirk, > > +}; > > > Right... > > > +#ifdef CONFIG_ACPI > > +static const struct acpi_device_id usb_xhci_acpi_match[] = { > > + /* XHCI-compliant USB Controller */ > > + { "PNP0D10", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match); > > +#endif > > Nice one > > There's no need to copy me if you plan to resend any of this. I'm just > a drive-by reader here anyway, and I've seen enough. > > Good luck! > > > > > Bjørn >
