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
