Messages in this thread | | | Date | Wed, 24 Feb 2021 14:36:16 +0100 | From | Krzysztof Wilczyński <> | 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
| |