Re: [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

[Jianjun Wang]

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