linux-kernel - Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1614222447.25750.6.camel@mhfsdcap03>
Date:   Thu, 25 Feb 2021 11:07:27 +0800
From:   Jianjun Wang <jianjun.wang@...iatek.com>
To:     Krzysztof Wilczyński <kw@...ux.com>
CC:     Bjorn Helgaas <helgaas@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, <maz@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Ryder Lee <ryder.lee@...iatek.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        <linux-pci@...r.kernel.org>, <linux-mediatek@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        "Sj Huang" <sj.huang@...iatek.com>, <youlin.pei@...iatek.com>,
        <chuanjia.liu@...iatek.com>, <qizhong.cheng@...iatek.com>,
        <sin_jieyang@...iatek.com>, <drinkcat@...omium.org>,
        <Rex-BC.Chen@...iatek.com>, <anson.chuang@...iatek.com>
Subject: Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

Hi Krzysztof,

Thanks for your review, I will fix these at next version.

Thanks.

On Wed, 2021-02-24 at 14:36 +0100, Krzysztof Wilczyński wrote:
> Hi Jianjun,
> 
> Thank you for all the work here!
> 
> [...]
> > + * struct mtk_pcie_port - PCIe port information
> > + * @dev: pointer to PCIe device
> > + * @base: IO mapped register base
> > + * @reg_base: Physical register base
> > + * @mac_reset: mac reset control
> > + * @phy_reset: phy reset control
> > + * @phy: PHY controller block
> > + * @clks: PCIe clocks
> > + * @num_clks: PCIe clocks count for this port
> 
> It would be "MAC" and "PHY" in the above.
> 
> [...]
> > + * mtk_pcie_config_tlp_header
> > + * @bus: PCI bus to query
> > + * @devfn: device/function number
> > + * @where: offset in config space
> > + * @size: data size in TLP header
> > + *
> > + * Set byte enable field and device information in configuration TLP header.
> 
> The kernel-doc above might be missing brief function description.  See
> the following for more concrete example:
> 
>   https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 
> [...]
> > +static int mtk_pcie_set_trans_table(struct mtk_pcie_port *port,
> > +				    resource_size_t cpu_addr,
> > +				    resource_size_t pci_addr,
> > +				    resource_size_t size,
> > +				    unsigned long type, int num)
> > +{
> > +	void __iomem *table;
> > +	u32 val;
> > +
> > +	if (num >= PCIE_MAX_TRANS_TABLES) {
> > +		dev_err(port->dev, "not enough translate table[%d] for addr: %#llx, limited to [%d]\n",
> 
> The wording of this error message is a little confusing.
> 
> > +			num, (unsigned long long) cpu_addr,
> 
> No space between the bracket and the variable name.
> 
> [...]
> > +	err = phy_init(port->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to initialize PCIe phy\n");
> > +		goto err_phy_init;
> > +	}
> > +
> > +	err = phy_power_on(port->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to power on PCIe phy\n");
> > +		goto err_phy_on;
> > +	}
> [...]
> 
> It would be "PHY" in the error messages above.
> 
> [...]
> > +	if (err) {
> > +		dev_err(dev, "clock init failed\n");
> > +		goto err_clk_init;
> > +	}
> [...]
> 
> A nitpick, so feel free to ignore it, of course.  What about "failed to
> initialize clock" to keep the style consistent.
> 
> [...]
> > +	err = mtk_pcie_startup_port(port);
> > +	if (err) {
> > +		dev_err(dev, "PCIe startup failed\n");
> [...]
> 
> Also a nitpick.  What about "failed to bring PCIe link up"?
> 
> Krzysztof