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

[Niklas Cassel]

On Tue, Feb 04, 2025 at 06:12:56PM +0100, Thierry Reding wrote:
> 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).

Ok, fine by me.


> 
> 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.

I think it is quite a fundamental mistake that the commit that added
support for Tegra234, actually requires you to enabled support for a
completely different SoC to actually make use of that driver, so IMO
the Fixes tag is absolutely warrented.


Kind regards,
Niklas