Re: [PATCH 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 19, 2022 at 09:11:09AM +0200, Krzysztof Kozlowski wrote:
> 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.

Thanks, Krzysztof, all great advice.

Not having looked at the driver at all, and because I'm not at all
expert in native drivers like this, also reuse as much of the
structure of other drivers as possible, especially the siblings in
drivers/pci/controller/dwc/.  That means similar structure, similar
variable names, similar function names, etc.  This makes it easier for
people to compare with other drivers to see which are affected by
future bug fixes.

Bjorn