Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752560AbdGFMkm (ORCPT ); Thu, 6 Jul 2017 08:40:42 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:37806 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752010AbdGFMkk (ORCPT ); Thu, 6 Jul 2017 08:40:40 -0400 Subject: Re: [PATCH v9 0/3] Tango PCIe controller support To: Mason , Bjorn Helgaas References: <987fac41-80dc-f1d0-ec0b-91ae57b91bfd@sigmadesigns.com> <20170704202412.GK13824@bhelgaas-glaptop.roam.corp.google.com> <6256cab3-dd77-c0f6-66b8-be261695bbb1@free.fr> <20170705180359.GL13824@bhelgaas-glaptop.roam.corp.google.com> <5a1207cb-31fd-6b85-86af-8c37bd57ad4f@free.fr> <20170705213419.GD25063@bhelgaas-glaptop.roam.corp.google.com> <8dcab663-245b-880a-be3a-2965fd12d663@free.fr> <20170706033946.GO13824@bhelgaas-glaptop.roam.corp.google.com> Cc: Marc Gonzalez , Thomas Gleixner , linux-pci , Linux ARM , LKML , DT , Thibaud Cornic From: Marc Zyngier Organization: ARM Ltd Message-ID: <37d9f990-c037-5ccd-c162-786ca2bd07b5@arm.com> Date: Thu, 6 Jul 2017 13:40:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2544 Lines: 70 On 06/07/17 13:26, Mason wrote: > On 06/07/2017 05:39, Bjorn Helgaas wrote: > >> On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote: >> >>> There were a few nits I wanted to address: >>> >>> - Since we added suppress_bind_attrs = true, probe() >>> can only be called at init, so I wanted to mark __init >>> all the probe functions, to save space. >>> >>> - I left the definition of MSI_MAX in the wrong patch >>> >>> - You put a pointer to the pdev in the struct tango_pcie. >>> I think this is redundant, since the pdev already has a >>> pointer to the struct, as drvdata. >>> So I wanted to change tango_msi_probe() to take a pdev >>> as argument (to make it more like an actual probe function) >>> and derive pcie from pdev, instead of the other way around. >> >> I don't think tango_msi_probe() is really a "probe" function. It's >> all part of the tango driver, and it's not claiming a separate piece >> of hardware. > > I agree that tango_msi_probe() is not a probe function; > it is merely a piece of the actual probe function (static > with single call site). I split the probe function in two, > because it seemed to make sense at the time. > > Perhaps it's better to inline tango_msi_probe? That would > avoid the issues of that function's name and parameters. > > If you think it's better to keep the two pieces separate, > I can rename the MSI part to tango_msi_init() or some such. > But I'd like to avoid adding unnecessary fields to the struct. > >> So I would keep the name and structure similar to these: >> >> advk_pcie_init_msi_irq_domain() >> nwl_pcie_init_msi_irq_domain() >> >> BTW, those functions use irq_domain_add_linear(), while you are one of >> the very few callers of irq_domain_create_linear(). Why the difference? >> If your code does basically the same thing, it's very helpful to me if >> it *looks* basically the same. irq_domain_add_linear() can only take an of_node as the identifier for the domain, while the _create_ variants use a fwnode. Given that an of+node is also a fwnode, the former is now deprecated in favour of the latter. > > It was a suggestion from Marc Z on 2017-03-23. > > > + irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie); > > Use irq_domain_create_linear, pass the same fwnode. > > > It seems odd to pass NULL as the first argument. > (As I had first done, when copying the Altera driver.) Indeed, as it creates a "default" domain, which is almost always wrong. Thanks, M. -- Jazz is not dead. It just smells funny...