From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiang Liu Subject: Re: [Patch v5 4/6] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core Date: Thu, 11 Jun 2015 01:19:13 +0800 Message-ID: <55787191.6060400@linux.intel.com> References: <1433780448-18636-1-git-send-email-jiang.liu@linux.intel.com> <1433780448-18636-5-git-send-email-jiang.liu@linux.intel.com> <20150609161230.GC8591@red-moon> <55771B27.1060509@linux.intel.com> <20150610164821.GA18832@red-moon> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150610164821.GA18832@red-moon> Sender: linux-pci-owner@vger.kernel.org To: Lorenzo Pieralisi Cc: "Rafael J . Wysocki" , Bjorn Helgaas , Marc Zyngier , "hanjun.guo@linaro.org" , Liviu Dudau , Yijing Wang , Len Brown , Lv Zheng , LKML , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "x86 @ kernel . org" , "linux-arm-kernel@lists.infradead.org" List-Id: linux-acpi@vger.kernel.org On 2015/6/11 0:48, Lorenzo Pieralisi wrote: > On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote: >> On 2015/6/10 0:12, Lorenzo Pieralisi wrote: > > [...] > >>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >>>> + struct acpi_pci_root_ops *ops, >>>> + struct acpi_pci_root_info *info, >>>> + void *sysdata, int segment, int node) >>> >>> I do not think you need to pass segment and node, they clutter the >>> function signature when you can retrieve them from root, I would >>> make them local variables and use root->segment and acpi_get_node >>> in the function body to retrieve them. >> On x86, node and segment may be overridden under certain conditions. >> For example, segment will always be 0 if 'pci_ignore_seg' is set. > > Ok, so the question would be then why do you not override the value > in root->segment then (actually, is it *correct* to leave the segment > value in root-> unchanged even if it is overriden) ? > > Anyway, node is just used for a printk, why do not you add segment and > node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you > simplify the code, it is not easy to parse. Hi Lorenzo, I used the complex prototype to explicitly reminder callers that you need to prepare all these information. If simpler interface is preferred, we could pack all ops, sysdata, segment, node, root into info, that there's will be only one parameter "info" left. The drawback is that it will add several lines of code to each arch using this interface. So prefer simpler interface or less lines of code?:) Thanks! Gerry > > Thanks, > Lorenzo > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754440AbbFJRTZ (ORCPT ); Wed, 10 Jun 2015 13:19:25 -0400 Received: from mga02.intel.com ([134.134.136.20]:64492 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752417AbbFJRTS (ORCPT ); Wed, 10 Jun 2015 13:19:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,588,1427785200"; d="scan'208";a="585507595" Message-ID: <55787191.6060400@linux.intel.com> Date: Thu, 11 Jun 2015 01:19:13 +0800 From: Jiang Liu Organization: Intel User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Lorenzo Pieralisi CC: "Rafael J . Wysocki" , Bjorn Helgaas , Marc Zyngier , "hanjun.guo@linaro.org" , Liviu Dudau , Yijing Wang , Len Brown , Lv Zheng , LKML , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "x86 @ kernel . org" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [Patch v5 4/6] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core References: <1433780448-18636-1-git-send-email-jiang.liu@linux.intel.com> <1433780448-18636-5-git-send-email-jiang.liu@linux.intel.com> <20150609161230.GC8591@red-moon> <55771B27.1060509@linux.intel.com> <20150610164821.GA18832@red-moon> In-Reply-To: <20150610164821.GA18832@red-moon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/6/11 0:48, Lorenzo Pieralisi wrote: > On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote: >> On 2015/6/10 0:12, Lorenzo Pieralisi wrote: > > [...] > >>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >>>> + struct acpi_pci_root_ops *ops, >>>> + struct acpi_pci_root_info *info, >>>> + void *sysdata, int segment, int node) >>> >>> I do not think you need to pass segment and node, they clutter the >>> function signature when you can retrieve them from root, I would >>> make them local variables and use root->segment and acpi_get_node >>> in the function body to retrieve them. >> On x86, node and segment may be overridden under certain conditions. >> For example, segment will always be 0 if 'pci_ignore_seg' is set. > > Ok, so the question would be then why do you not override the value > in root->segment then (actually, is it *correct* to leave the segment > value in root-> unchanged even if it is overriden) ? > > Anyway, node is just used for a printk, why do not you add segment and > node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you > simplify the code, it is not easy to parse. Hi Lorenzo, I used the complex prototype to explicitly reminder callers that you need to prepare all these information. If simpler interface is preferred, we could pack all ops, sysdata, segment, node, root into info, that there's will be only one parameter "info" left. The drawback is that it will add several lines of code to each arch using this interface. So prefer simpler interface or less lines of code?:) Thanks! Gerry > > Thanks, > Lorenzo > From mboxrd@z Thu Jan 1 00:00:00 1970 From: jiang.liu@linux.intel.com (Jiang Liu) Date: Thu, 11 Jun 2015 01:19:13 +0800 Subject: [Patch v5 4/6] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core In-Reply-To: <20150610164821.GA18832@red-moon> References: <1433780448-18636-1-git-send-email-jiang.liu@linux.intel.com> <1433780448-18636-5-git-send-email-jiang.liu@linux.intel.com> <20150609161230.GC8591@red-moon> <55771B27.1060509@linux.intel.com> <20150610164821.GA18832@red-moon> Message-ID: <55787191.6060400@linux.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2015/6/11 0:48, Lorenzo Pieralisi wrote: > On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote: >> On 2015/6/10 0:12, Lorenzo Pieralisi wrote: > > [...] > >>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >>>> + struct acpi_pci_root_ops *ops, >>>> + struct acpi_pci_root_info *info, >>>> + void *sysdata, int segment, int node) >>> >>> I do not think you need to pass segment and node, they clutter the >>> function signature when you can retrieve them from root, I would >>> make them local variables and use root->segment and acpi_get_node >>> in the function body to retrieve them. >> On x86, node and segment may be overridden under certain conditions. >> For example, segment will always be 0 if 'pci_ignore_seg' is set. > > Ok, so the question would be then why do you not override the value > in root->segment then (actually, is it *correct* to leave the segment > value in root-> unchanged even if it is overriden) ? > > Anyway, node is just used for a printk, why do not you add segment and > node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you > simplify the code, it is not easy to parse. Hi Lorenzo, I used the complex prototype to explicitly reminder callers that you need to prepare all these information. If simpler interface is preferred, we could pack all ops, sysdata, segment, node, root into info, that there's will be only one parameter "info" left. The drawback is that it will add several lines of code to each arch using this interface. So prefer simpler interface or less lines of code?:) Thanks! Gerry > > Thanks, > Lorenzo >