On 19/04/2022 02:07, Wangseok Lee wrote: >> On 28/03/2022 04:14, 이왕석 wrote: >>> Add support Axis, ARTPEC-8 SoC. >>> ARTPEC-8 is the SoC platform of Axis Communications. >>> This is based on arm64 and support GEN4 & 2lane. >>> This PCIe controller is based on DesignWare Hardware core >>> and uses DesignWare core functions to implement the driver. >>> This is based on driver/pci/controller/dwc/pci-exynos.c >>> >>> Signed-off-by: Wangseok Lee >>> --- >>> drivers/pci/controller/dwc/Kconfig | 31 + >>> drivers/pci/controller/dwc/Makefile | 1 + >>> drivers/pci/controller/dwc/pcie-artpec8.c | 912 ++++++++++++++++++++++++++++++ >>> 3 files changed, 944 insertions(+) >>> create mode 100644 drivers/pci/controller/dwc/pcie-artpec8.c >>> >> >> I took a look at the your driver and at existing PCIe Exynos driver. >> Unfortunately PCIe Exynos driver is in poor shape, really poor. This >> would explain that maybe it's better to have new driver instead of >> merging them, especially that hardware is different. Although I am still >> waiting for some description of these differences... >> >> I said that Exynos PCIe looks poor... but what is worse, it looks like >> you based on it so you copied or some bad patterns it had. >> >> Except this the driver has several coding style issues, so please be >> sure to run checkpatch, sparse and smatch before sending it. >> >> Please work on this driver to make it close to Linux coding style, so >> there will be no need for us, reviewers, focus on basic stuff. >> >> Optionally, send all this to staging. :) >> >> Best regards, >> Krzysztof > Hi, > > Thank you for your kindness review. > I will re-work again close to the linux coding style. > Addiltionaly, If you tell me what "bad patterns" you mentioned, > it will be very helpful for the work. > Could you please tell me that? Except the tools I mentioned before, the patterns are: 1. debug messages for probe or other functions (we have ftrace and other tools for that). 2. Inconsistent coding style (e.g. different indentation in structure members). 3. Inconsistent code (e.g. artpec8_pcie_get_subsystem_resources() gets device from pdev and from pci so you have two same pointers; or artpec8_pcie_get_ep_mem_resources() stores dev as local variable but uses instead pdev->dev). 4. Not using devm_platform_ioremap_resource(). 5. Wrappers over writel() and readl() which do nothing else than wrap one function. 6. Printing messages in interrupt handlers. 7. Several local/static structures or array are not const. Plus they are defined all through the code, instead of beginning of a file. Also - you have four clocks, so use clk bulk API. Best regards, Krzysztof
