Re: [PATCH 4/7] PCI: renesas: Add R-Car Gen4 PCIe Endpoint support

[Rob Herring <robh@kernel.org>]

On Tue, Jun 14, 2022 at 11:58:46AM +0000, Yoshihiro Shimoda wrote:
> Hi Rob,
> 
> Thank you for your review!
> 
> > From: Rob Herring, Sent: Tuesday, June 14, 2022 6:51 AM
> > 
> > On Mon, Jun 13, 2022 at 08:57:09PM +0900, Yoshihiro Shimoda wrote:
> > > Add R-Car Gen4 PCIe Endpoint support. This controller is based on
> > > Synopsys Designware PCIe.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---

[...]

> > > +	u32			num_lanes;
> > 
> > What's wrong with dw_pcie.num_lanes?
> 
> The dw_pcie.num_lanes is set after dw_pcie_ep_init() succeeded.
> However, this driver would like to refer the num_lanes before dw_pcie_ep_init()
> to initialize vendor-specific registers. In other words, this value is only
> needed at that timing. So, we can remove this from struct rcar_gen4_pcie_ep,
> and just get the num_lanes as a local variable.

AFAICT, you are just using it to set PCI_EXP_LNKCAP_MLW. That's a common 
register, so it should be able to set in the common code.


[...]

> > > +static struct platform_driver rcar_gen4_pcie_ep_driver = {
> > > +	.driver = {
> > > +		.name = "pcie-rcar-gen4-ep",
> > > +		.of_match_table = rcar_gen4_pcie_of_match,
> > > +	},
> > > +	.probe = rcar_gen4_pcie_ep_probe,
> > > +	.remove = rcar_gen4_pcie_ep_remove,
> > > +};
> > > +builtin_platform_driver(rcar_gen4_pcie_ep_driver);
> > 
> > Not a module or...
> > 
> > > +
> > > +MODULE_DESCRIPTION("Renesas R-Car Gen4 PCIe endpoint controller driver");
> > > +MODULE_LICENSE("GPL v2");
> > 
> > A module? Should be a module if possible.
> 
> Oops. I'll drop these MODULE_*.

No, is there some reason it can't be a module?

Rob