From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932917AbbJAIek (ORCPT ); Thu, 1 Oct 2015 04:34:40 -0400 Received: from down.free-electrons.com ([37.187.137.238]:60723 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932752AbbJAIeg (ORCPT ); Thu, 1 Oct 2015 04:34:36 -0400 Date: Thu, 1 Oct 2015 10:34:12 +0200 From: Boris Brezillon To: Alexandre Belloni Cc: Nicolas Ferre , Jean-Christophe Plagniol-Villard , Michael Turquette , Stephen Boyd , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org Subject: Re: [PATCH 06/16] clk: at91: clk-main: factorize irq handling Message-ID: <20151001103412.1ac2231d@bbrezillon> In-Reply-To: <1443629469-15086-7-git-send-email-alexandre.belloni@free-electrons.com> References: <1443629469-15086-1-git-send-email-alexandre.belloni@free-electrons.com> <1443629469-15086-7-git-send-email-alexandre.belloni@free-electrons.com> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.27; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alexandre, On Wed, 30 Sep 2015 18:10:59 +0200 Alexandre Belloni wrote: > The three different irq handlers are doing the same thing, factorize their > code in a generic irq handler. > > Signed-off-by: Alexandre Belloni > --- > drivers/clk/at91/clk-main.c | 43 ++++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 27 deletions(-) > > diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c > index c1f119748bdc..79a380cd1f4e 100644 > --- a/drivers/clk/at91/clk-main.c > +++ b/drivers/clk/at91/clk-main.c > @@ -71,12 +71,21 @@ struct clk_sam9x5_main { > > #define to_clk_sam9x5_main(hw) container_of(hw, struct clk_sam9x5_main, hw) > > -static irqreturn_t clk_main_osc_irq_handler(int irq, void *dev_id) > +/* Generic structure */ > +struct clk_main { > + struct clk_hw hw; > + struct at91_pmc *pmc; > + unsigned int irq; > + wait_queue_head_t wait; > +}; > +#define to_clk_main(hw) container_of(hw, struct clk_main, hw) It wasn't clear to me at first glance that this structure was actually a base struct for the clk_main_rc_osc and clk_sam9x5_main struct. Maybe you should rename it into struct clk_main_base and then replace the hw, pmc, irq and fields in the child structures by a base field. Something like: struct clk_sam9x5_main { struct clk_main_base base; u8 parent; }; struct clk_main_rc_osc { struct clk_main_base base; unsigned long frequency; unsigned long accuracy; }; Also, it would avoid any problem if someone decides to change the field oder in one of those structures. Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com