Re: [RFC PATCH 7/7] x86: add more platform_setup functions

[Thomas Gleixner]

On Fri, 28 Aug 2009, Pan, Jacob jun wrote:

> >From fbfe65d64e4dbb31940d31a59e080bfa15d121bb Mon Sep 17 00:00:00 2001
> From: Jacob Pan <jacob.jun.pan@xxxxxxxxx>
> Date: Wed, 8 Jul 2009 16:51:42 -0700
> Subject: [PATCH] x86: RFC add more platform_setup functions
> 
> check_timer can be abstracted so that we don't have to do the complicated
> checks for platform timer for all platforms.

Well, if it's necessary then we replace it, but the code you showed in
an earlier patch is just adding debug cruft. So what's the point ?

> pci irq enable/disable can also
> benefit from the platform setup layer, but this part is not complete in this
> patch, should be able to include visws. e.g.

Please provide one patch per feature / change.
 
> pcibios_enable_irq = &pci_visws_enable_irq;
> pcibios_disable_irq = &pci_visws_disable_irq;

No, this is patently wrong. You abstract at the wrong point. If
abstraction of the pci init is necessary then the various #ifdeffed
function calls in legacy.c:pci_subsys_init() have to be platformized.
 
>  extern int __init pci_mmcfg_arch_init(void);
> diff --git a/arch/x86/include/asm/platform.h b/arch/x86/include/asm/platform.h
> index 1cea28c..d698f68 100644
> --- a/arch/x86/include/asm/platform.h
> +++ b/arch/x86/include/asm/platform.h
> @@ -6,6 +6,7 @@
>  struct mpc_bus;
>  struct mpc_cpu;
>  struct mpc_table;
> +struct pci_dev;
>  
>  /**
>   * struct platform_setup_quirks - platform specific quirks
> @@ -62,6 +63,11 @@ struct platform_setup_irqs {
>  	void (*pre_vector_init)(void);
>  	void (*intr_init)(void);
>  	void (*trap_init)(void);
> +
> +#ifdef CONFIG_PCI
> +	int (*pci_enable_irq)(struct pci_dev *dev);
> +	void (*pci_disable_irq)(struct pci_dev *dev);
> +#endif

PCI stuff does not belong to irq. It needs a separate data
structure. Also using the function pointers just to assign them later
to some other function pointer is pointless. The code can be
restructured so you can just tweak the existing function pointer.
Also I avoided #ifdefs in the platform code on purpose. The few bytes
you spare in the init structure are not worth the #ifdef mess which is
needed all over the place.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/