Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192
From: Jianjun Wang
Date: Wed Feb 24 2021 - 22:09:11 EST
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