Re: [PATCH 3/6] PCI: dwc: fsd: Add FSD PCIe Controller driver support
From: Bjorn Helgaas
Date: Mon Nov 21 2022 - 18:18:57 EST
On Mon, Nov 21, 2022 at 04:22:07PM +0530, Shradha Todi wrote:
> Add PCIe controller driver file for PCIe controller
> found in fsd SoC family. This driver adds support for both RC
> and EP mode.
>
> Signed-off-by: Niyas Ahmed S T <niyas.ahmed@xxxxxxxxxxx>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@xxxxxxxxxxx>
> Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
Would be much, much better if drivers can be combined as Krzysztof
suggests. Superficial comments below if not.
> +config PCIE_FSD
> + bool "Samsung FSD PCIe Controller"
> + default n
Unnecessary since "default n" is the default. Please read and learn
from the existing Kconfig for other drivers.
> +config PCIE_FSD_HOST
> + bool "PCIe FSD Host Mode"
> + depends on PCI
Shouldn't be necessary because the entire menu depends on PCI:
menu "DesignWare PCI Core Support"
depends on PCI
> + depends on PCI_MSI_IRQ_DOMAIN || PCI_DOMAIN
> + select PCIE_DW_HOST
> + select PCIE_FSD
> + default n
Unnecessary.
> +config PCIE_FSD_EP
> + bool "PCIe FSD Endpoint Mode"
> + depends on PCI_ENDPOINT
> + select PCIE_DW_EP
> + select PCIE_FSD
> + default n
Unnecessary.
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Tesla fsd SoC
> + *
> + * Copyright (C) 2017-2022 Samsung Electronics Co., Ltd. http://www.samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
Unnecessary copyright text since you have SPDX at the top.
> + * struct fsd_pcie - representation of the pci controller
s/pci/PCI/ in English text (you do it inconsistently)
s/pcie/PCIe/ same
> + * @pci: representation of dwc pcie device structure
> + * @pdata: private data to determine the oprations supported by device
s/oprations/operations/
> + * @appl_base: represent the appl base
> + * @sysreg: represent the system register base
> + * @sysreg_base: represents the offset of the system register required
"representation", "represent", "represents"? Pick one.
> + * @phy: represents the phy device associated for the controller
s/phy/PHY/ in English text
> + /* External Local Bus interface(ELBI) Register */
Space before "(" in English text.
> + dev_err(dev, "failed to map dbi_base\n");
> + dev_err(dev, "Sysreg regmap lookup failed.\n");
> + dev_err(dev, "Couldn't get the register offset for syscon!\n");
> + dev_err(dev, "couldn't get aux clock\n");
Random capitalization of driver messages. Pick one.
> + /* assert LTSSM enable */
> + /* Return success as link might come up later */
Random capitalization of comments. Pick one.
> + case RADM_VENDOR_MSG:
> + pr_info("Interrupt received for\n");
Must include device identification. This one also looks incomplete
("for ..."?)
> + /* To indicate that controller has accepted to send
> + * Latency Tolerance reporting message
Use standard multi-line comment format.
> + dev_err(dev, "failed to get msi irq\n");
s/msi/MSI/ in English text.
s/irq/IRQ/ also
> + .start_link = fsd_pcie_establish_link,
fsd_pcie_start_link() to match function pointer name.
> + pci->dbi_base2 = NULL;
> + pci->dbi_base = NULL;
> + pci->atu_base = NULL;
Unnecessary since you used kzalloc().
> + dev_info(dev, "FSD PCIe probe completed successfully\n");
Questionable utility. Most drivers don't emit this, or if they do,
they include some useful information like address, IRQ, etc.
> + dev_err(dev, "PCIe Failed to set 36 bit dma mask\n");
s/dma/DMA/ in English text.