Received-SPF: pass (google.com: domain of linux-kernel+bounces-211767-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 12 Jun 2024 08:12:05 -0700 (PDT)
From: matthew.gerlach@linux.intel.com
To: Bjorn Helgaas <helgaas@kernel.org>
cc: lpieralisi@kernel.org, kw@linux.com, robh@kernel.org, bhelgaas@google.com, 
    krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
    joyce.ooi@intel.com, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, 
    linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/2] PCI: altera: support dt binding update
In-Reply-To: <20240611170410.GA989554@bhelgaas>
Message-ID: <alpine.DEB.2.22.394.2406120744350.662691@sj-4150-psse-sw-opae-dev2>
References: <20240611170410.GA989554@bhelgaas>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed


On Tue, 11 Jun 2024, Bjorn Helgaas wrote:

> On Tue, Jun 11, 2024 at 11:35:25AM -0500, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add support for the device tree binding update. As part of
>> converting the binding document from text to yaml, with schema
>> validation, a device tree subnode was added to properly map
>> legacy interrupts. Maintain backward compatibility with previous binding.
>
> If something was *added* to the binding, I think it would be helpful
> to split that into two patches: (1) convert to YAML with zero
> functional changes, (2) add the new stuff.  Adding something at the
> same time as changing the format makes it hard to review.

Thanks for feedback. It was during the conversion to YAML that a problem 
with the original binding was discovered. As Rob Herring pointed out in
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240513205913.313592-1-matthew.gerlach@linux.intel.com/

"Making the PCI host the interrupt parent didn't even work in the kernel
  until somewhat recently (maybe a few years now). That's why a bunch of PCI
  hosts have an interrupt-controller child node."

This was an attempt to fix the problem. I can resubmit a conversion to YAML 
with zero functional changes.

Matthew Gerlach


>
> Then we could have a more specific subject and commit log for *this*
> patch.
>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/pci/controller/pcie-altera.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
>> index a9536dc4bf96..88511fa2f078 100644
>> --- a/drivers/pci/controller/pcie-altera.c
>> +++ b/drivers/pci/controller/pcie-altera.c
>> @@ -667,11 +667,20 @@ static void altera_pcie_isr(struct irq_desc *desc)
>>  static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>>  {
>>  	struct device *dev = &pcie->pdev->dev;
>> -	struct device_node *node = dev->of_node;
>> +	struct device_node *node, *child;
>>
>>  	/* Setup INTx */
>> +	child = of_get_next_child(dev->of_node, NULL);
>> +	if (child)
>> +		node = child;
>> +	else
>> +		node = dev->of_node;
>> +
>>  	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
>> -					&intx_domain_ops, pcie);
>> +						 &intx_domain_ops, pcie);
>> +	if (child)
>> +		of_node_put(child);
>> +
>>  	if (!pcie->irq_domain) {
>>  		dev_err(dev, "Failed to get a INTx IRQ domain\n");
>>  		return -ENOMEM;
>> --
>> 2.34.1
>>
>