Re: [PATCH V1] PCI: tegra194: Add support for PCIe RC & EP in Tegra234 Platforms

[Thierry Reding]

On Tue, Jan 28, 2025 at 01:04:32PM +0100, Niklas Cassel wrote:
> Hello Vidya,
> 
> On Tue, Jan 28, 2025 at 10:12:44AM +0530, Vidya Sagar wrote:
> > Add PCIe RC & EP support for Tegra234 Platforms.
> 
> The commit log does leave quite a few questions unanswered.
> 
> Since you are just updating the Kconfig and nothing else:
> Does the DT binding already have support for the Tegra234 SoC?
> Does the driver already have support for the Tegra234 SoC?
> 
> Looking at the DT binding and driver, the answer to both questions
> is yes. (This should have been in the commit message IMO.)
> 
> 
> But that leads me to the question, since there is support for Tegra234
> SoC in the driver, does this means that this fixes a regression, e.g.
> the Kconfig ARCH_TEGRA_234_SOC was added after the driver support in
> this driver was added. In this case, you should have a Fixes: tag that
> points to the commit that added ARCH_TEGRA_234_SOC.
> 
> Or has the the driver support for Tegra234 been "dead-code" since it
> was originally added? (Because without this patch, no one can have
> tested it, at least not without COMPILE_TEST.)
> In this case, you should add:
> Fixes: a54e19073718 ("PCI: tegra194: Add Tegra234 PCIe support")

Typically we build the default configuration with some custom options
(like everyone else, I assume) and usually in those configurations both
Tegra194 and Tegra234 support will be enabled, so the code ends up
enabled in most cases. I guess the commit message doesn't do a very good
job of making this clear. Really what this commit does is enable the PCI
controller driver for Tegra234-only configurations (i.e. no other Tegra
generations are built-in).

Not sure about the Fixes: tag since this is fairly harmless. Worst case
you'll need to enable Tegra194 support along with Tegra234 in order to
be able to enable this driver, but that's almost always the case anyway.

Thierry

Attachment: signature.asc
Description: PGP signature