On Thu, Apr 20, 2023, at 11:00, Pawel Laszczak wrote:
> This patch introduces the main part of Cadence USBHS driver
> to Linux kernel.
Not sure why I was on Cc, but I gave it a quick look anyway, looking
for common issues. I only found a few very minor things that can be
improved, no real problems:
> +++ b/drivers/usb/gadget/udc/cdns2/Kconfig
> @@ -0,0 +1,11 @@
> +config USB_CDNS2_UDC
> + tristate "Cadence USBHS Device Controller"
> + depends on USB_PCI && ACPI && HAS_DMA
Why the ACPI dependency?
> + response_pkt = (__le16 *)pdev->ep0_preq.request.buf;
> + *response_pkt = cpu_to_le16(status);
You can simplify this using put_unaligned_le16()
> +
> + preq->num_of_trb = num_trbs;
> +
> + /*
> + * Memory barrier - cycle bit must be set as the last operation.
> + */
> + wmb();
This can probably be the cheaper dma_wmb() if you only serialize
between accesses to a DMA buffer.
> +static int __maybe_unused cdns2_pci_suspend(struct device *dev)
> +{
> + struct cdns2_device *priv_dev = dev_get_drvdata(dev);
> +
> + return cdns2_gadget_suspend(priv_dev);
> +}
> +
> +static int __maybe_unused cdns2_pci_resume(struct device *dev)
> +{
> + struct cdns2_device *priv_dev = dev_get_drvdata(dev);
> +
> + return cdns2_gadget_resume(priv_dev, 1);
> +}
> +
> +static const struct dev_pm_ops cdns2_pci_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(cdns2_pci_suspend, cdns2_pci_resume)
> +};
You can use SYSTEM_SLEEP_PM_OPS() instead of SET_SYSTEM_SLEEP_PM_OPS()
and then remove the __maybe_unused.
Arnd
