From: Krzysztof Kozlowski <krzk@kernel.org>
To: wangseok.lee@samsung.com, robh+dt <robh+dt@kernel.org>,
krzk+dt <krzk+dt@kernel.org>, kishon <kishon@ti.com>,
vkoul <vkoul@kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
"jesper.nilsson" <jesper.nilsson@axis.com>,
"lars.persson" <lars.persson@axis.com>
Cc: bhelgaas <bhelgaas@google.com>,
linux-phy <linux-phy@lists.infradead.org>,
linux-pci <linux-pci@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
"lorenzo.pieralisi" <lorenzo.pieralisi@arm.com>,
kw <kw@linux.com>, linux-arm-kernel <linux-arm-kernel@axis.com>,
kernel <kernel@axis.com>, Moon-Ki Jun <moonki.jun@samsung.com>,
Dongjin Yang <dj76.yang@samsung.com>
Subject: Re: [PATCH 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver
Date: Tue, 19 Apr 2022 09:11:09 +0200 [thread overview]
Message-ID: <62bbc2a6-92fb-ff2b-a43f-ecb402e8f90c@kernel.org> (raw)
In-Reply-To: <20220419000730epcms2p77c94d5e55db13ebf2f88b25d16b6ef7a@epcms2p7>
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
next prev parent reply other threads:[~2022-04-19 7:11 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20220419000730epcms2p77c94d5e55db13ebf2f88b25d16b6ef7a@epcms2p7>
2022-04-19 0:07 ` [PATCH 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver Wangseok Lee
2022-04-19 7:11 ` Krzysztof Kozlowski [this message]
2022-04-19 15:20 ` Bjorn Helgaas
[not found] <CGME20220328021453epcms2p15977e72b6c96253ecaefcb71e6d2acfe@epcms2p1>
2022-03-28 2:14 ` 이왕석
2022-03-28 18:52 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=62bbc2a6-92fb-ff2b-a43f-ecb402e8f90c@kernel.org \
--to=krzk@kernel.org \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=dj76.yang@samsung.com \
--cc=jesper.nilsson@axis.com \
--cc=kernel@axis.com \
--cc=kishon@ti.com \
--cc=krzk+dt@kernel.org \
--cc=kw@linux.com \
--cc=lars.persson@axis.com \
--cc=linux-arm-kernel@axis.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=moonki.jun@samsung.com \
--cc=robh+dt@kernel.org \
--cc=vkoul@kernel.org \
--cc=wangseok.lee@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).