From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Murray Subject: Re: [PATCH v5 01/17] of/pci: Provide support for parsing PCI DT ranges property Date: Fri, 22 Mar 2013 11:03:37 +0000 Message-ID: <20130322110337.GA2573@arm.com> References: <1363887025-19931-1-git-send-email-thomas.petazzoni@free-electrons.com> <1363887025-19931-2-git-send-email-thomas.petazzoni@free-electrons.com> <20130322101238.GA22929@avionic-0098.mockup.avionic-design.de> <20130322112059.35675858@skate> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20130322112059.35675858@skate> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Thomas Petazzoni Cc: Lior Amsalem , Andrew Lunn , Russell King , Jason Cooper , "linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" , Liviu Dudau , Maen Suleiman , Bjorn Helgaas , Tawfik Bayouk , Mitch Bradley , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Jason Gunthorpe List-Id: devicetree@vger.kernel.org On Fri, Mar 22, 2013 at 10:20:59AM +0000, Thomas Petazzoni wrote: > > On Fri, 22 Mar 2013 11:12:39 +0100, Thierry Reding wrote: > > > This sounds like you're trying to do too much within the for loop. When > > we discussed this previously I had a vague idea that this functionality > > could be wrapped into something a bit more object-like. > > > > What I had in mind was something like: > > > > struct of_pci_range_parser; > > struct of_pci_range; > > > > struct of_pci_range_parser parser; > > struct of_pci_range range; > > > > err = of_pci_range_parser(&parser, np); > > if (err < 0) > > return err; > > > > for_each_of_pci_range(range, parser) { > > struct resource res; > > > > ... > > usage of range similar to iterator > > ... > > > > of_pci_range_to_resource(&res, &range); > > } > > > > In the above the of_pci_range structure pretty much replaces the > > iterator and the whole is wrapped up within a parser structure to give > > some extra flexibility and provides for easier (or more structured) > > setup compared to doing all of it within the loop statement. > > > > But aside from the (perceived?) increased robustness there's not a lot > > of technical benefit over your implementation, so it isn't a very hard > > objection. I find it to be a little more encapsulated and therefore > > easier to work with, but that's possibly just a matter of taste. > > I don't have a strong opinion on this. Andrew, what do you think? The changes Thierry suggest are subtle but it does look a lot cleaner, it would move the memset elsewhere and probably split the of_pci_process_ranges function into two. I think this is all good. Though I'm not sure when of_pci_range_parser would ever return an error (perhaps if ranges doesn't exist? - should that be treated as an error?). Perhaps Grant can provide some feedback - the patch originally provided a parser for ARM without adding more arch code - this then became a refactoring exercise for ranges parsing across the kernel. I think this patch has gone as far as it can in that vein without introducing common pci_controller structures. All we need here is a parser that [at least] ARM can use to support the pending ARM host-bridge drivers. Is its current form likely to be acceptable? I'm happy to update the patch for Thierry's suggestions. Thanks, Andrew Murray