Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933541AbbGUVhv (ORCPT ); Tue, 21 Jul 2015 17:37:51 -0400 Received: from www.linutronix.de ([62.245.132.108]:35324 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932480AbbGUVhu (ORCPT ); Tue, 21 Jul 2015 17:37:50 -0400 Date: Tue, 21 Jul 2015 23:37:34 +0200 (CEST) From: Thomas Gleixner To: Shenwei Wang cc: "shawn.guo@linaro.org" , "jason@lakedaemon.net" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH V5 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources In-Reply-To: Message-ID: References: <1437150300-8134-1-git-send-email-shenwei.wang@freescale.com> <1437150300-8134-2-git-send-email-shenwei.wang@freescale.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2079 Lines: 49 On Tue, 21 Jul 2015, Shenwei Wang wrote: > > On Tue, 21 Jul 2015, Shenwei Wang wrote: > > > This struct defines the properties and functions that GPCv2 block > > > provides. Since GPCv2 has two key functions: Irq wakeup source > > > management and power management, the intention of the struct is to > > > share data and methods among irqchip, suspend, and cpuidle drivers. > > > > I don't think this is a good idea. The cpuidle driver has nothing to know about the > > internals of the irq driver and vice versa. Neither does the suspend code. > > > > If you failed to split that proper then your design is wrong. > > > The implementation has already been spitted totally. The question is > if we use the same structure among those drivers or not, since they > do share some common data like gpc_base address, enabled_irq, and > mfmix_mask. The suspend and cpuidle driver will use those data to > decide the hardware power modes and the relating power down sequence > of the power domains. The structure is the abstract of the GPCv2 > hardware, and the current struct declaration matches the low level > hardware well. Although it is possible and easy to split it into > two, it may introduce either redundant definition for the common > properties or have to create a global variable to enable them > visible to both the irqchip and the suspend codes. So the proper way to do this is: Have a data structure which contains only the shared information. The pointer to this structure can be global. Have per driver data structures which contain the driver private stuff. Think about whether you need all the function pointers in one of the structs. IOW, you need them only if a pointer can be changed at runtime. If not you can call the function directly as I doubt that any of these drivers can be modular. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/