On Tue, 21 Jul 2015, Shenwei Wang wrote:

quoted

On Tue, 21 Jul 2015, Shenwei Wang wrote:

quoted

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