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

[Pawel Laszczak]

>
>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:

You were detected by get_maintainer.pl script :). 
I will remove you from cc in next version.

./scripts/get_maintainer.pl patch/0002-usb-cdns2-Add-main-part-of-Cadence-USBHS-driver.patch
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> (supporter:USB SUBSYSTEM,commit_signer:8/13=62%,commit_signer:4/6=67%)
Pawel Laszczak <pawell@xxxxxxxxxxx> (maintainer:CADENCE USBHS DRIVER,commit_signer:1/6=17%,added_lines:1/4=25%)
Arnd Bergmann <arnd@xxxxxxxx> (commit_signer:5/13=38%,authored:5/13=38%,added_lines:3/47=6%,removed_lines:31/47=66%,authored:1/6=17%,removed_lines:2/3=67%)
Tony Lindgren <tony@xxxxxxxxxxx> (commit_signer:2/13=15%)
Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx> (commit_signer:1/13=8%)
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> (commit_signer:1/13=8%)
Randy Dunlap <rdunlap@xxxxxxxxxxxxx> (authored:1/13=8%)
Jean Delvare <jdelvare@xxxxxxx> (authored:1/13=8%)
Biju Das <biju.das.jz@xxxxxxxxxxxxxx> (authored:1/13=8%,added_lines:13/47=28%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Hans-Christian Noren Egtvedt <egtvedt@xxxxxxxxxxxx> (authored:1/13=8%)
Neal Liu <neal_liu@xxxxxxxxxxxxxx> (added_lines:13/47=28%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Herve Codina <herve.codina@xxxxxxxxxxx> (added_lines:11/47=23%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Linus Walleij <linus.walleij@xxxxxxxxxx> (removed_lines:11/47=23%,authored:1/6=17%,removed_lines:1/3=33%)
linux-kernel@xxxxxxxxxxxxxxx (open list)

Thanks,
Pawel

>
>> +++ 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