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: <YDZWUGcKet/lNWlF@rocinante>
Date:   Wed, 24 Feb 2021 14:36:16 +0100
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Jianjun Wang <jianjun.wang@...iatek.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 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