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
