From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934264AbcJZQHF (ORCPT ); Wed, 26 Oct 2016 12:07:05 -0400 Received: from mail-oi0-f42.google.com ([209.85.218.42]:33620 "EHLO mail-oi0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933352AbcJZQHC (ORCPT ); Wed, 26 Oct 2016 12:07:02 -0400 MIME-Version: 1.0 In-Reply-To: <20161026154608.GB22713@leverpostej> References: <1475086637-1914-1-git-send-email-fu.wei@linaro.org> <1475086637-1914-8-git-send-email-fu.wei@linaro.org> <20161021113213.GD16630@leverpostej> <20161026154608.GB22713@leverpostej> From: Fu Wei Date: Thu, 27 Oct 2016 00:07:00 +0800 Message-ID: Subject: Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT To: Mark Rutland Cc: "Rafael J. Wysocki" , Len Brown , Daniel Lezcano , Thomas Gleixner , Marc Zyngier , Lorenzo Pieralisi , Sudeep Holla , Hanjun Guo , linux-arm-kernel@lists.infradead.org, Linaro ACPI Mailman List , Linux Kernel Mailing List , ACPI Devel Maling List , rruigrok@codeaurora.org, "Abdulhamid, Harb" , Christopher Covington , Timur Tabi , G Gregory , Al Stone , Jon Masters , Wei Huang , Arnd Bergmann , Catalin Marinas , Will Deacon , Suravee Suthikulpanit , Leo Duran , Wim Van Sebroeck , Guenter Roeck , linux-watchdog@vger.kernel.org, Tomasz Nowicki , Christoffer Dall , Julien Grall Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, On 26 October 2016 at 23:46, Mark Rutland wrote: > On Wed, Oct 26, 2016 at 11:24:32PM +0800, Fu Wei wrote: >> On 21 October 2016 at 19:32, Mark Rutland wrote: >> > On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote: >> >> +static int __init arch_timer_mem_register(struct device_node *np, void *frame) >> >> { >> >> - int ret; >> >> - irq_handler_t func; >> >> + struct device_node *frame_node = NULL; >> >> struct arch_timer *t; >> >> + void __iomem *base; >> >> + irq_handler_t func; >> >> + unsigned int irq; >> >> + int ret; >> >> + >> >> + if (!frame) >> >> + return -EINVAL; >> > >> > Why would we call this without a frame? >> >> Sorry, I just verify it , make sure frame is not NULL, >> Because it is a "static" function, so we do need this check? > > I'd rather we simply don't call the function rather than passing a NULL > frame in. > OK, NP, will do >> >> + >> >> + if (np) { >> > >> > ... or without a node? >> >> For "np", for now, we just just verify it, but it is just paperation >> for GTDT support, >> Because in next patch, if np == NULL, that means we call this function >> from GTDT, but not DT. > > Please don't do that. More on that below. > >> > Please as Marc requested several versions ago: split the FW parsing >> > (ACPI and DT) so that happens first, *then* once we have the data in a >> > common format, use that to drive poking the HW, requesting IRQs, etc, >> > completely independent of the source. >> > >> > In patches, do this by: >> > >> > (1) adding the data structures >> > (2) splitting the existing DT probing to use them >> > (3) Adding ACPI functionality atop >> >> this patch is a preparation for GTDT support, I have splitted some >> functions for reusing them in next patch(GTDT support) >> >> if np == NULL, that means we call this function from GTDT, but >> if np != NULL, that means we call this function from DT > > As above, please structure the patches such that that never happens. > > We currently have: > > +--------------------------+ > | DT Parsing + Common code | > +--------------------------+ > > Per (1 and 2) make this: > > +------------+ +-------------+ > | DT parsing |--(common structure)-->| Common code | > +------------+ +-------------+ > > Then per (3) make this: > > +------------+ > | DT parsing |--(common structure)------+ > +------------+ | > v > +-------------+ > | Common code | > +-------------+ > ^ > +--------------+ | > | ACPI parsing |--(common structure)----+ Thanks for your suggestion , I will do this way in my next patchset Thanks :-) > +--------------+ > > Thanks, > Mark. -- Best regards, Fu Wei Software Engineer Red Hat