Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Nov 2022 15:05:59 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Lizhi Hou <lizhi.hou@amd.com>
Cc:     linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, robh@kernel.org,
        frowand.list@gmail.com, clement.leger@bootlin.com,
        max.zhen@amd.com, sonal.santan@amd.com, larry.liu@amd.com,
        brian.xu@amd.com, stefano.stabellini@xilinx.com, trix@redhat.com
Subject: Re: [PATCH RFC V3 2/2] PCI: Create device tree node for selected
 devices
Message-ID: <20221107210559.GA419452@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1667575454-18706-3-git-send-email-lizhi.hou@amd.com>
Precedence: bulk

On Fri, Nov 04, 2022 at 08:24:14AM -0700, Lizhi Hou wrote:
> The PCIe endpoint device such as Xilinx Alveo PCIe card maps the register
> spaces from multiple hardware peripherals to its PCIe BAR. Normally,
> the PCI core discovers devices and BARs using the PCI enumeration process.
> There is no infrastructure to discover the hardware peripherals that are
> present in a PCI device, and which can be accessed through the PCI BARs.
> 
> For Alveo PCIe card, the card firmware provides a flattened device tree to
> describe the hardware peripherals on its BARs. The Alveo card driver can
> load this flattened device tree and leverage device tree framework to
> generate platform devices for the hardware peripherals eventually.
> 
> Apparently, the device tree framework requires a device tree node for the
> PCIe device. Thus, it can generate the device tree nodes for hardware
> peripherals underneath. Because PCIe is self discoverable bus, there might
> not be a device tree node created for PCIe devices. This patch is to add
> support to generate device tree node for PCIe devices. It introduces a
> kernel option. When the option is turned on, the kernel will generate

Specify the kernel option here.  These last two sentences should
probably be a separate paragraph because they say specifically what
this patch does.

> device tree nodes for PCI bridges unconditionally.

> It will also generate
> a device tree node for Xilinx Alveo U50 by using PCI quirks.

I think I would split the Xilinx Alveo U50 quirk to a separate patch
from the infrastructure and make this patch more generic.

> +config PCI_DYNAMIC_OF_NODES
> +	bool "Device tree node for PCI devices"
> +	depends on OF
> +	select OF_DYNAMIC
> +	help
> +	  This option enables support for generating device tree nodes for some
> +	  PCI devices. Thus, the driver of this kind can load and overlay
> +	  flattened device tree for its downstream devices.
> +
> +	  Once this option is selected, the device tree nodes will be generated
> +	  for all PCI/PCIE bridges.

PCI/PCIe

Actually, in this context (and in the commit log), you should just say
"PCI" because there's nothing PCIe-specific here.

> +void of_pci_make_dev_node(struct pci_dev *pdev)
> +{
> +	struct device_node *parent, *dt_node = NULL;
> +	const char *pci_type = "dev";
> +	struct of_changeset *cset;
> +	const char *full_name;
> +	int ret;
> +
> +	/*
> +	 * if there is already a device tree node linked to this device,
> +	 * return immediately.

s/if there/If there/

> +	 */
> +	if (pci_device_to_OF_node(pdev))
> +		return;
> +
> +	/* check if there is device tree node for parent device */

s/check/Check/

Follow the style of the file, which is "capitalize English sentences."