Re: [PATCH v2 2/4] usb: cdns2: Add main part of Cadence USBHS driver

["Arnd Bergmann" <arnd@arndb.de>]

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