* [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
@ 2011-05-03 6:10 lan,Tianyu
2011-05-04 1:48 ` Lin Ming
0 siblings, 1 reply; 6+ messages in thread
From: lan,Tianyu @ 2011-05-03 6:10 UTC (permalink / raw)
To: lenb; +Cc: linux-acpi, robert.moore, ming.m.lin, rui.zhang
Some places use the acpi_gbl_FADT.sci_interrupt directly to get the ACPI SCI.ACPI SCI will not only be stored in the FADT.
So this patch replaces acpi_gbl_FADT.sci_interrupt with two functions for getting and setting.
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
arch/x86/kernel/acpi/boot.c | 8 ++---
drivers/acpi/acpica/evgpeinit.c | 5 +--
drivers/acpi/acpica/evgpeutil.c | 4 +-
drivers/acpi/acpica/evsci.c | 4 +-
drivers/acpi/acpica/evxface.c | 38 +++++++++++++++++++++++++++
drivers/acpi/bus.c | 4 +-
drivers/acpi/osl.c | 6 ++--
drivers/acpi/pci_link.c | 2 -
drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
include/acpi/acpixf.h | 4 ++
10 files changed, 59 insertions(+), 18 deletions(-)
Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03 10:19:00.163029998 +0800
+++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03 10:20:50.833030004 +0800
@@ -408,7 +408,7 @@
acpi_table_print_madt_entry(header);
- if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
+ if (intsrc->source_irq == acpi_get_sci_interrupt()) {
acpi_sci_ioapic_setup(intsrc->source_irq,
intsrc->inti_flags & ACPI_MADT_POLARITY_MASK,
(intsrc->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2,
@@ -1100,7 +1100,7 @@
return gsi;
/* Don't set up the ACPI SCI because it's already set up */
- if (acpi_gbl_FADT.sci_interrupt == gsi)
+ if (acpi_get_sci_interrupt() == gsi)
return gsi;
ioapic = mp_find_ioapic(gsi);
@@ -1184,8 +1184,8 @@
* pretend we got one so we can set the SCI flags.
*/
if (!acpi_sci_override_gsi)
- acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0,
- acpi_gbl_FADT.sci_interrupt);
+ acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
+ acpi_get_sci_interrupt());
/* Fill in identity legacy mappings where no override */
mp_config_acpi_legacy_irqs();
Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:19:00.243029997 +0800
+++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:20:50.833030004 +0800
@@ -131,7 +131,7 @@
status = acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
&acpi_gbl_FADT.xgpe0_block,
register_count0, 0,
- acpi_gbl_FADT.sci_interrupt,
+ acpi_get_sci_interrupt(),
&acpi_gbl_gpe_fadt_blocks[0]);
if (ACPI_FAILURE(status)) {
@@ -170,8 +170,7 @@
&acpi_gbl_FADT.xgpe1_block,
register_count1,
acpi_gbl_FADT.gpe1_base,
- acpi_gbl_FADT.
- sci_interrupt,
+ acpi_get_sci_interrupt(),
&acpi_gbl_gpe_fadt_blocks
[1]);
Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:19:00.213030003 +0800
+++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:20:50.833030004 +0800
@@ -253,7 +253,7 @@
/* Install new interrupt handler if not SCI_INT */
- if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
+ if (interrupt_number != acpi_get_sci_interrupt()) {
status = acpi_os_install_interrupt_handler(interrupt_number,
acpi_ev_gpe_xrupt_handler,
gpe_xrupt);
@@ -290,7 +290,7 @@
/* We never want to remove the SCI interrupt handler */
- if (gpe_xrupt->interrupt_number == acpi_gbl_FADT.sci_interrupt) {
+ if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
gpe_xrupt->gpe_block_list_head = NULL;
return_ACPI_STATUS(AE_OK);
}
Index: linux-2.6/drivers/acpi/acpica/evsci.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03 10:19:00.223030004 +0800
+++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03 10:20:50.833030004 +0800
@@ -142,7 +142,7 @@
ACPI_FUNCTION_TRACE(ev_install_sci_handler);
status =
- acpi_os_install_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
+ acpi_os_install_interrupt_handler((u32) acpi_get_sci_interrupt(),
acpi_ev_sci_xrupt_handler,
acpi_gbl_gpe_xrupt_list_head);
return_ACPI_STATUS(status);
@@ -176,7 +176,7 @@
/* Just let the OS remove the handler and disable the level */
status =
- acpi_os_remove_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
+ acpi_os_remove_interrupt_handler((u32) acpi_get_sci_interrupt(),
acpi_ev_sci_xrupt_handler);
return_ACPI_STATUS(status);
Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03 10:19:00.213030003 +0800
+++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03 10:20:50.833030004 +0800
@@ -983,3 +983,41 @@
}
ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_get_sci_interrupt
+ *
+ * PARAMETERS: none
+ *
+ * RETURN: SCI Interrupt
+ *
+ * DESCRIPTION: Return the ACPI SCI interrupt
+ *
+ ******************************************************************************/
+u16 acpi_get_sci_interrupt(void)
+{
+ return acpi_gbl_FADT.sci_interrupt;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_set_sci_interrupt
+ *
+ * PARAMETERS: override sci irq
+ *
+ * RETURN: status
+ *
+ * DESCRIPTION: Set the ACPI SCI interrupt
+ *
+ ******************************************************************************/
+acpi_status acpi_set_sci_interrupt(u16 irq)
+{
+ acpi_gbl_FADT.sci_interrupt = irq;
+ return AE_OK;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
+
Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03 10:19:00.203030002 +0800
+++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004 +0800
@@ -890,14 +890,14 @@
acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
}
/* Set PIC-mode SCI trigger type */
- acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
+ acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
} else {
/*
* now that acpi_gbl_FADT is initialized,
* update it with result from INT_SRC_OVR parsing
*/
- acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
+ acpi_set_sci_interrupt(acpi_sci_override_gsi);
}
#endif
Index: linux-2.6/drivers/acpi/osl.c
===================================================================
--- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03 10:19:48.993030009 +0800
+++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004 +0800
@@ -534,7 +534,7 @@
* ACPI interrupts different from the SCI in our copy of the FADT are
* not supported.
*/
- if (gsi != acpi_gbl_FADT.sci_interrupt)
+ if (gsi != acpi_get_sci_interrupt())
return AE_BAD_PARAMETER;
if (acpi_irq_handler)
@@ -559,7 +559,7 @@
acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
{
- if (irq != acpi_gbl_FADT.sci_interrupt)
+ if (irq != acpi_get_sci_interrupt())
return AE_BAD_PARAMETER;
free_irq(irq, acpi_irq);
@@ -1622,7 +1622,7 @@
acpi_status acpi_os_terminate(void)
{
if (acpi_irq_handler) {
- acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
+ acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
acpi_irq_handler);
}
Index: linux-2.6/drivers/acpi/pci_link.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03 10:19:00.203030002 +0800
+++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03 10:20:50.833030004 +0800
@@ -508,7 +508,7 @@
}
}
/* Add a penalty for the SCI */
- acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
+ acpi_irq_penalty[acpi_get_sci_interrupt()] += PIRQ_PENALTY_PCI_USING;
return 0;
}
Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
===================================================================
--- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:19:00.193030001 +0800
+++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:20:50.833030004 +0800
@@ -104,7 +104,7 @@
pci_dev_put(pdev);
/* we're sharing the IRQ with ACPI */
- irq = acpi_gbl_FADT.sci_interrupt;
+ irq = acpi_get_sci_interrupt();
if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON", dcon)) {
printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation failed\n", irq);
return 1;
Index: linux-2.6/include/acpi/acpixf.h
===================================================================
--- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03 10:19:00.253029998 +0800
+++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004 +0800
@@ -411,6 +411,10 @@
acpi_info(const char *module_name,
u32 line_number, const char *format, ...) ACPI_PRINTF_LIKE(3);
+u16 acpi_get_sci_interrupt(void);
+
+acpi_status acpi_set_sci_interrupt(u16 irq);
+
/*
* Debug output
*/
---
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
2011-05-03 6:10 [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places lan,Tianyu
@ 2011-05-04 1:48 ` Lin Ming
2011-05-05 0:24 ` lan,Tianyu
0 siblings, 1 reply; 6+ messages in thread
From: Lin Ming @ 2011-05-04 1:48 UTC (permalink / raw)
To: Lan, Tianyu
Cc: lenb@kernel.org, linux-acpi@vger.kernel.org, Moore, Robert,
Zhang, Rui
On Tue, 2011-05-03 at 14:10 +0800, Lan, Tianyu wrote:
> Some places use the acpi_gbl_FADT.sci_interrupt directly to get the ACPI SCI.ACPI SCI will not only be stored in the FADT.
> So this patch replaces acpi_gbl_FADT.sci_interrupt with two functions for getting and setting.
Some trivial patch style comments,
We usually add "acpi:" prefix to the subject, like
[PATCH] acpi: xxxxx
And if acpica files are also touched, we add "acpi/acpica:" prefix, like
[PATCH] acpi/acpica: xxxx
Another thing, the change logs seems too long for each line, you'd
better split them short for each line. So that makes logs easy to read.
Thanks,
Lin Ming
>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
> arch/x86/kernel/acpi/boot.c | 8 ++---
> drivers/acpi/acpica/evgpeinit.c | 5 +--
> drivers/acpi/acpica/evgpeutil.c | 4 +-
> drivers/acpi/acpica/evsci.c | 4 +-
> drivers/acpi/acpica/evxface.c | 38 +++++++++++++++++++++++++++
> drivers/acpi/bus.c | 4 +-
> drivers/acpi/osl.c | 6 ++--
> drivers/acpi/pci_link.c | 2 -
> drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
> include/acpi/acpixf.h | 4 ++
> 10 files changed, 59 insertions(+), 18 deletions(-)
>
> Index: linux-2.6/arch/x86/kernel/acpi/boot.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03 10:19:00.163029998 +0800
> +++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03 10:20:50.833030004 +0800
> @@ -408,7 +408,7 @@
>
> acpi_table_print_madt_entry(header);
>
> - if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
> + if (intsrc->source_irq == acpi_get_sci_interrupt()) {
> acpi_sci_ioapic_setup(intsrc->source_irq,
> intsrc->inti_flags & ACPI_MADT_POLARITY_MASK,
> (intsrc->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2,
> @@ -1100,7 +1100,7 @@
> return gsi;
>
> /* Don't set up the ACPI SCI because it's already set up */
> - if (acpi_gbl_FADT.sci_interrupt == gsi)
> + if (acpi_get_sci_interrupt() == gsi)
> return gsi;
>
> ioapic = mp_find_ioapic(gsi);
> @@ -1184,8 +1184,8 @@
> * pretend we got one so we can set the SCI flags.
> */
> if (!acpi_sci_override_gsi)
> - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0,
> - acpi_gbl_FADT.sci_interrupt);
> + acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
> + acpi_get_sci_interrupt());
>
> /* Fill in identity legacy mappings where no override */
> mp_config_acpi_legacy_irqs();
> Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:19:00.243029997 +0800
> +++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:20:50.833030004 +0800
> @@ -131,7 +131,7 @@
> status = acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
> &acpi_gbl_FADT.xgpe0_block,
> register_count0, 0,
> - acpi_gbl_FADT.sci_interrupt,
> + acpi_get_sci_interrupt(),
> &acpi_gbl_gpe_fadt_blocks[0]);
>
> if (ACPI_FAILURE(status)) {
> @@ -170,8 +170,7 @@
> &acpi_gbl_FADT.xgpe1_block,
> register_count1,
> acpi_gbl_FADT.gpe1_base,
> - acpi_gbl_FADT.
> - sci_interrupt,
> + acpi_get_sci_interrupt(),
> &acpi_gbl_gpe_fadt_blocks
> [1]);
>
> Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:19:00.213030003 +0800
> +++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:20:50.833030004 +0800
> @@ -253,7 +253,7 @@
>
> /* Install new interrupt handler if not SCI_INT */
>
> - if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
> + if (interrupt_number != acpi_get_sci_interrupt()) {
> status = acpi_os_install_interrupt_handler(interrupt_number,
> acpi_ev_gpe_xrupt_handler,
> gpe_xrupt);
> @@ -290,7 +290,7 @@
>
> /* We never want to remove the SCI interrupt handler */
>
> - if (gpe_xrupt->interrupt_number == acpi_gbl_FADT.sci_interrupt) {
> + if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
> gpe_xrupt->gpe_block_list_head = NULL;
> return_ACPI_STATUS(AE_OK);
> }
> Index: linux-2.6/drivers/acpi/acpica/evsci.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03 10:19:00.223030004 +0800
> +++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03 10:20:50.833030004 +0800
> @@ -142,7 +142,7 @@
> ACPI_FUNCTION_TRACE(ev_install_sci_handler);
>
> status =
> - acpi_os_install_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> + acpi_os_install_interrupt_handler((u32) acpi_get_sci_interrupt(),
> acpi_ev_sci_xrupt_handler,
> acpi_gbl_gpe_xrupt_list_head);
> return_ACPI_STATUS(status);
> @@ -176,7 +176,7 @@
> /* Just let the OS remove the handler and disable the level */
>
> status =
> - acpi_os_remove_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> + acpi_os_remove_interrupt_handler((u32) acpi_get_sci_interrupt(),
> acpi_ev_sci_xrupt_handler);
>
> return_ACPI_STATUS(status);
> Index: linux-2.6/drivers/acpi/acpica/evxface.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03 10:19:00.213030003 +0800
> +++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03 10:20:50.833030004 +0800
> @@ -983,3 +983,41 @@
> }
>
> ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_get_sci_interrupt
> + *
> + * PARAMETERS: none
> + *
> + * RETURN: SCI Interrupt
> + *
> + * DESCRIPTION: Return the ACPI SCI interrupt
> + *
> + ******************************************************************************/
> +u16 acpi_get_sci_interrupt(void)
> +{
> + return acpi_gbl_FADT.sci_interrupt;
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
> +
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_set_sci_interrupt
> + *
> + * PARAMETERS: override sci irq
> + *
> + * RETURN: status
> + *
> + * DESCRIPTION: Set the ACPI SCI interrupt
> + *
> + ******************************************************************************/
> +acpi_status acpi_set_sci_interrupt(u16 irq)
> +{
> + acpi_gbl_FADT.sci_interrupt = irq;
> + return AE_OK;
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
> +
> Index: linux-2.6/drivers/acpi/bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03 10:19:00.203030002 +0800
> +++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004 +0800
> @@ -890,14 +890,14 @@
> acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> }
> /* Set PIC-mode SCI trigger type */
> - acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> + acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
> (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> } else {
> /*
> * now that acpi_gbl_FADT is initialized,
> * update it with result from INT_SRC_OVR parsing
> */
> - acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> + acpi_set_sci_interrupt(acpi_sci_override_gsi);
> }
> #endif
>
> Index: linux-2.6/drivers/acpi/osl.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03 10:19:48.993030009 +0800
> +++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004 +0800
> @@ -534,7 +534,7 @@
> * ACPI interrupts different from the SCI in our copy of the FADT are
> * not supported.
> */
> - if (gsi != acpi_gbl_FADT.sci_interrupt)
> + if (gsi != acpi_get_sci_interrupt())
> return AE_BAD_PARAMETER;
>
> if (acpi_irq_handler)
> @@ -559,7 +559,7 @@
>
> acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> {
> - if (irq != acpi_gbl_FADT.sci_interrupt)
> + if (irq != acpi_get_sci_interrupt())
> return AE_BAD_PARAMETER;
>
> free_irq(irq, acpi_irq);
> @@ -1622,7 +1622,7 @@
> acpi_status acpi_os_terminate(void)
> {
> if (acpi_irq_handler) {
> - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> + acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
> acpi_irq_handler);
> }
>
> Index: linux-2.6/drivers/acpi/pci_link.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03 10:19:00.203030002 +0800
> +++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03 10:20:50.833030004 +0800
> @@ -508,7 +508,7 @@
> }
> }
> /* Add a penalty for the SCI */
> - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> + acpi_irq_penalty[acpi_get_sci_interrupt()] += PIRQ_PENALTY_PCI_USING;
> return 0;
> }
>
> Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
> ===================================================================
> --- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:19:00.193030001 +0800
> +++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:20:50.833030004 +0800
> @@ -104,7 +104,7 @@
> pci_dev_put(pdev);
>
> /* we're sharing the IRQ with ACPI */
> - irq = acpi_gbl_FADT.sci_interrupt;
> + irq = acpi_get_sci_interrupt();
> if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON", dcon)) {
> printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation failed\n", irq);
> return 1;
> Index: linux-2.6/include/acpi/acpixf.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03 10:19:00.253029998 +0800
> +++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004 +0800
> @@ -411,6 +411,10 @@
> acpi_info(const char *module_name,
> u32 line_number, const char *format, ...) ACPI_PRINTF_LIKE(3);
>
> +u16 acpi_get_sci_interrupt(void);
> +
> +acpi_status acpi_set_sci_interrupt(u16 irq);
> +
> /*
> * Debug output
> */
>
> ---
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
2011-05-04 1:48 ` Lin Ming
@ 2011-05-05 0:24 ` lan,Tianyu
2011-05-05 17:30 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: lan,Tianyu @ 2011-05-05 0:24 UTC (permalink / raw)
To: Lin Ming
Cc: lenb@kernel.org, linux-acpi@vger.kernel.org, Moore, Robert,
Zhang, Rui
OK, Thanks for your advices. I will take them.
Best Regards
Lan Tianyu
在 2011-05-04三的 09:48 +0800，Lin Ming写道：
> On Tue, 2011-05-03 at 14:10 +0800, Lan, Tianyu wrote:
> > Some places use the acpi_gbl_FADT.sci_interrupt directly to get the ACPI SCI.ACPI SCI will not only be stored in the FADT.
> > So this patch replaces acpi_gbl_FADT.sci_interrupt with two functions for getting and setting.
>
> Some trivial patch style comments,
>
> We usually add "acpi:" prefix to the subject, like
>
> [PATCH] acpi: xxxxx
>
> And if acpica files are also touched, we add "acpi/acpica:" prefix, like
>
> [PATCH] acpi/acpica: xxxx
>
> Another thing, the change logs seems too long for each line, you'd
> better split them short for each line. So that makes logs easy to read.
>
> Thanks,
> Lin Ming
>
> >
> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> > ---
> > arch/x86/kernel/acpi/boot.c | 8 ++---
> > drivers/acpi/acpica/evgpeinit.c | 5 +--
> > drivers/acpi/acpica/evgpeutil.c | 4 +-
> > drivers/acpi/acpica/evsci.c | 4 +-
> > drivers/acpi/acpica/evxface.c | 38 +++++++++++++++++++++++++++
> > drivers/acpi/bus.c | 4 +-
> > drivers/acpi/osl.c | 6 ++--
> > drivers/acpi/pci_link.c | 2 -
> > drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
> > include/acpi/acpixf.h | 4 ++
> > 10 files changed, 59 insertions(+), 18 deletions(-)
> >
> > Index: linux-2.6/arch/x86/kernel/acpi/boot.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03 10:19:00.163029998 +0800
> > +++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -408,7 +408,7 @@
> >
> > acpi_table_print_madt_entry(header);
> >
> > - if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
> > + if (intsrc->source_irq == acpi_get_sci_interrupt()) {
> > acpi_sci_ioapic_setup(intsrc->source_irq,
> > intsrc->inti_flags & ACPI_MADT_POLARITY_MASK,
> > (intsrc->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2,
> > @@ -1100,7 +1100,7 @@
> > return gsi;
> >
> > /* Don't set up the ACPI SCI because it's already set up */
> > - if (acpi_gbl_FADT.sci_interrupt == gsi)
> > + if (acpi_get_sci_interrupt() == gsi)
> > return gsi;
> >
> > ioapic = mp_find_ioapic(gsi);
> > @@ -1184,8 +1184,8 @@
> > * pretend we got one so we can set the SCI flags.
> > */
> > if (!acpi_sci_override_gsi)
> > - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0,
> > - acpi_gbl_FADT.sci_interrupt);
> > + acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
> > + acpi_get_sci_interrupt());
> >
> > /* Fill in identity legacy mappings where no override */
> > mp_config_acpi_legacy_irqs();
> > Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:19:00.243029997 +0800
> > +++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -131,7 +131,7 @@
> > status = acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
> > &acpi_gbl_FADT.xgpe0_block,
> > register_count0, 0,
> > - acpi_gbl_FADT.sci_interrupt,
> > + acpi_get_sci_interrupt(),
> > &acpi_gbl_gpe_fadt_blocks[0]);
> >
> > if (ACPI_FAILURE(status)) {
> > @@ -170,8 +170,7 @@
> > &acpi_gbl_FADT.xgpe1_block,
> > register_count1,
> > acpi_gbl_FADT.gpe1_base,
> > - acpi_gbl_FADT.
> > - sci_interrupt,
> > + acpi_get_sci_interrupt(),
> > &acpi_gbl_gpe_fadt_blocks
> > [1]);
> >
> > Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:19:00.213030003 +0800
> > +++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -253,7 +253,7 @@
> >
> > /* Install new interrupt handler if not SCI_INT */
> >
> > - if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
> > + if (interrupt_number != acpi_get_sci_interrupt()) {
> > status = acpi_os_install_interrupt_handler(interrupt_number,
> > acpi_ev_gpe_xrupt_handler,
> > gpe_xrupt);
> > @@ -290,7 +290,7 @@
> >
> > /* We never want to remove the SCI interrupt handler */
> >
> > - if (gpe_xrupt->interrupt_number == acpi_gbl_FADT.sci_interrupt) {
> > + if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
> > gpe_xrupt->gpe_block_list_head = NULL;
> > return_ACPI_STATUS(AE_OK);
> > }
> > Index: linux-2.6/drivers/acpi/acpica/evsci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03 10:19:00.223030004 +0800
> > +++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -142,7 +142,7 @@
> > ACPI_FUNCTION_TRACE(ev_install_sci_handler);
> >
> > status =
> > - acpi_os_install_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> > + acpi_os_install_interrupt_handler((u32) acpi_get_sci_interrupt(),
> > acpi_ev_sci_xrupt_handler,
> > acpi_gbl_gpe_xrupt_list_head);
> > return_ACPI_STATUS(status);
> > @@ -176,7 +176,7 @@
> > /* Just let the OS remove the handler and disable the level */
> >
> > status =
> > - acpi_os_remove_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> > + acpi_os_remove_interrupt_handler((u32) acpi_get_sci_interrupt(),
> > acpi_ev_sci_xrupt_handler);
> >
> > return_ACPI_STATUS(status);
> > Index: linux-2.6/drivers/acpi/acpica/evxface.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03 10:19:00.213030003 +0800
> > +++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -983,3 +983,41 @@
> > }
> >
> > ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
> > +
> > +/*******************************************************************************
> > + *
> > + * FUNCTION: acpi_get_sci_interrupt
> > + *
> > + * PARAMETERS: none
> > + *
> > + * RETURN: SCI Interrupt
> > + *
> > + * DESCRIPTION: Return the ACPI SCI interrupt
> > + *
> > + ******************************************************************************/
> > +u16 acpi_get_sci_interrupt(void)
> > +{
> > + return acpi_gbl_FADT.sci_interrupt;
> > +}
> > +
> > +ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
> > +
> > +/*******************************************************************************
> > + *
> > + * FUNCTION: acpi_set_sci_interrupt
> > + *
> > + * PARAMETERS: override sci irq
> > + *
> > + * RETURN: status
> > + *
> > + * DESCRIPTION: Set the ACPI SCI interrupt
> > + *
> > + ******************************************************************************/
> > +acpi_status acpi_set_sci_interrupt(u16 irq)
> > +{
> > + acpi_gbl_FADT.sci_interrupt = irq;
> > + return AE_OK;
> > +}
> > +
> > +ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
> > +
> > Index: linux-2.6/drivers/acpi/bus.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03 10:19:00.203030002 +0800
> > +++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -890,14 +890,14 @@
> > acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> > }
> > /* Set PIC-mode SCI trigger type */
> > - acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> > + acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
> > (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> > } else {
> > /*
> > * now that acpi_gbl_FADT is initialized,
> > * update it with result from INT_SRC_OVR parsing
> > */
> > - acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> > + acpi_set_sci_interrupt(acpi_sci_override_gsi);
> > }
> > #endif
> >
> > Index: linux-2.6/drivers/acpi/osl.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03 10:19:48.993030009 +0800
> > +++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -534,7 +534,7 @@
> > * ACPI interrupts different from the SCI in our copy of the FADT are
> > * not supported.
> > */
> > - if (gsi != acpi_gbl_FADT.sci_interrupt)
> > + if (gsi != acpi_get_sci_interrupt())
> > return AE_BAD_PARAMETER;
> >
> > if (acpi_irq_handler)
> > @@ -559,7 +559,7 @@
> >
> > acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> > {
> > - if (irq != acpi_gbl_FADT.sci_interrupt)
> > + if (irq != acpi_get_sci_interrupt())
> > return AE_BAD_PARAMETER;
> >
> > free_irq(irq, acpi_irq);
> > @@ -1622,7 +1622,7 @@
> > acpi_status acpi_os_terminate(void)
> > {
> > if (acpi_irq_handler) {
> > - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> > + acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
> > acpi_irq_handler);
> > }
> >
> > Index: linux-2.6/drivers/acpi/pci_link.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03 10:19:00.203030002 +0800
> > +++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -508,7 +508,7 @@
> > }
> > }
> > /* Add a penalty for the SCI */
> > - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> > + acpi_irq_penalty[acpi_get_sci_interrupt()] += PIRQ_PENALTY_PCI_USING;
> > return 0;
> > }
> >
> > Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:19:00.193030001 +0800
> > +++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:20:50.833030004 +0800
> > @@ -104,7 +104,7 @@
> > pci_dev_put(pdev);
> >
> > /* we're sharing the IRQ with ACPI */
> > - irq = acpi_gbl_FADT.sci_interrupt;
> > + irq = acpi_get_sci_interrupt();
> > if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON", dcon)) {
> > printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation failed\n", irq);
> > return 1;
> > Index: linux-2.6/include/acpi/acpixf.h
> > ===================================================================
> > --- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03 10:19:00.253029998 +0800
> > +++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004 +0800
> > @@ -411,6 +411,10 @@
> > acpi_info(const char *module_name,
> > u32 line_number, const char *format, ...) ACPI_PRINTF_LIKE(3);
> >
> > +u16 acpi_get_sci_interrupt(void);
> > +
> > +acpi_status acpi_set_sci_interrupt(u16 irq);
> > +
> > /*
> > * Debug output
> > */
> >
> > ---
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
2011-05-05 0:24 ` lan,Tianyu
@ 2011-05-05 17:30 ` Bjorn Helgaas
2011-05-05 17:48 ` Moore, Robert
2011-05-06 2:07 ` lan,Tianyu
0 siblings, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2011-05-05 17:30 UTC (permalink / raw)
To: lan,Tianyu
Cc: Lin Ming, lenb@kernel.org, linux-acpi@vger.kernel.org,
Moore, Robert, Zhang, Rui
>> On Tue, 2011-05-03 at 14:10 +0800, Lan, Tianyu wrote:
>> > Some places use the acpi_gbl_FADT.sci_interrupt directly to get the ACPI SCI.ACPI SCI will not only be stored in the FADT.
You need at least one space after a sentence-ending period, e.g., "...
get the ACPI SCI. ACPI SCI will not ..."
I can't figure out the intent of the patch as a whole. It seems to be
connected with "ACPI SCI will not only be stored in the FADT," but
that isn't clear enough to tell me what's going on. Is there a
scenario where we'll want to look somewhere else first, then fall back
to looking at the FADT? This patch didn't implement anything like
that, but maybe you intend future changes to acpi_get_sci_interrupt()
and acpi_set_sci_interrupt()?
I think the name "acpi_get_sci_interrupt()" is somewhat redundant, or
at least confusing: "SCI" means "system control interrupt," so you're
really saying "acpi_get_system_control_interrupt_interrupt." I guess
the first "interrupt" refers to the mechanism itself, and the second
to the vector number or GSI. Maybe "acpi_get_sci_gsi()" would be
better?
There's also an fadt->sci_interrupt reference in
arch/ia64/kernel/acpi.c. I don't know the intent of your patch or
whether that intent applies to ia64 as well as x86, but it's always
better if you can converge things rather than make x86 and ia64 more
different.
Unless I missed something, this patch makes no functional change.
That's fine, especially if you're getting ready for some future work,
but it's worth mentioning that this patch is reorganization only.
Bjorn
>> > So this patch replaces acpi_gbl_FADT.sci_interrupt with two functions for getting and setting.
>>
>> Some trivial patch style comments,
>>
>> We usually add "acpi:" prefix to the subject, like
>>
>> [PATCH] acpi: xxxxx
>>
>> And if acpica files are also touched, we add "acpi/acpica:" prefix, like
>>
>> [PATCH] acpi/acpica: xxxx
>>
>> Another thing, the change logs seems too long for each line, you'd
>> better split them short for each line. So that makes logs easy to read.
>>
>> Thanks,
>> Lin Ming
>>
>> >
>> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> > ---
>> > arch/x86/kernel/acpi/boot.c | 8 ++---
>> > drivers/acpi/acpica/evgpeinit.c | 5 +--
>> > drivers/acpi/acpica/evgpeutil.c | 4 +-
>> > drivers/acpi/acpica/evsci.c | 4 +-
>> > drivers/acpi/acpica/evxface.c | 38 +++++++++++++++++++++++++++
>> > drivers/acpi/bus.c | 4 +-
>> > drivers/acpi/osl.c | 6 ++--
>> > drivers/acpi/pci_link.c | 2 -
>> > drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
>> > include/acpi/acpixf.h | 4 ++
>> > 10 files changed, 59 insertions(+), 18 deletions(-)
>> >
>> > Index: linux-2.6/arch/x86/kernel/acpi/boot.c
>> > ===================================================================
>> > --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03 10:19:00.163029998 +0800
>> > +++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -408,7 +408,7 @@
>> >
>> > acpi_table_print_madt_entry(header);
>> >
>> > - if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
>> > + if (intsrc->source_irq == acpi_get_sci_interrupt()) {
>> > acpi_sci_ioapic_setup(intsrc->source_irq,
>> > intsrc->inti_flags & ACPI_MADT_POLARITY_MASK,
>> > (intsrc->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2,
>> > @@ -1100,7 +1100,7 @@
>> > return gsi;
>> >
>> > /* Don't set up the ACPI SCI because it's already set up */
>> > - if (acpi_gbl_FADT.sci_interrupt == gsi)
>> > + if (acpi_get_sci_interrupt() == gsi)
>> > return gsi;
>> >
>> > ioapic = mp_find_ioapic(gsi);
>> > @@ -1184,8 +1184,8 @@
>> > * pretend we got one so we can set the SCI flags.
>> > */
>> > if (!acpi_sci_override_gsi)
>> > - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0,
>> > - acpi_gbl_FADT.sci_interrupt);
>> > + acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
>> > + acpi_get_sci_interrupt());
>> >
>> > /* Fill in identity legacy mappings where no override */
>> > mp_config_acpi_legacy_irqs();
>> > Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:19:00.243029997 +0800
>> > +++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -131,7 +131,7 @@
>> > status = acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
>> > &acpi_gbl_FADT.xgpe0_block,
>> > register_count0, 0,
>> > - acpi_gbl_FADT.sci_interrupt,
>> > + acpi_get_sci_interrupt(),
>> > &acpi_gbl_gpe_fadt_blocks[0]);
>> >
>> > if (ACPI_FAILURE(status)) {
>> > @@ -170,8 +170,7 @@
>> > &acpi_gbl_FADT.xgpe1_block,
>> > register_count1,
>> > acpi_gbl_FADT.gpe1_base,
>> > - acpi_gbl_FADT.
>> > - sci_interrupt,
>> > + acpi_get_sci_interrupt(),
>> > &acpi_gbl_gpe_fadt_blocks
>> > [1]);
>> >
>> > Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:19:00.213030003 +0800
>> > +++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -253,7 +253,7 @@
>> >
>> > /* Install new interrupt handler if not SCI_INT */
>> >
>> > - if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
>> > + if (interrupt_number != acpi_get_sci_interrupt()) {
>> > status = acpi_os_install_interrupt_handler(interrupt_number,
>> > acpi_ev_gpe_xrupt_handler,
>> > gpe_xrupt);
>> > @@ -290,7 +290,7 @@
>> >
>> > /* We never want to remove the SCI interrupt handler */
>> >
>> > - if (gpe_xrupt->interrupt_number == acpi_gbl_FADT.sci_interrupt) {
>> > + if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
>> > gpe_xrupt->gpe_block_list_head = NULL;
>> > return_ACPI_STATUS(AE_OK);
>> > }
>> > Index: linux-2.6/drivers/acpi/acpica/evsci.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03 10:19:00.223030004 +0800
>> > +++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -142,7 +142,7 @@
>> > ACPI_FUNCTION_TRACE(ev_install_sci_handler);
>> >
>> > status =
>> > - acpi_os_install_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
>> > + acpi_os_install_interrupt_handler((u32) acpi_get_sci_interrupt(),
>> > acpi_ev_sci_xrupt_handler,
>> > acpi_gbl_gpe_xrupt_list_head);
>> > return_ACPI_STATUS(status);
>> > @@ -176,7 +176,7 @@
>> > /* Just let the OS remove the handler and disable the level */
>> >
>> > status =
>> > - acpi_os_remove_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
>> > + acpi_os_remove_interrupt_handler((u32) acpi_get_sci_interrupt(),
>> > acpi_ev_sci_xrupt_handler);
>> >
>> > return_ACPI_STATUS(status);
>> > Index: linux-2.6/drivers/acpi/acpica/evxface.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03 10:19:00.213030003 +0800
>> > +++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -983,3 +983,41 @@
>> > }
>> >
>> > ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
>> > +
>> > +/*******************************************************************************
>> > + *
>> > + * FUNCTION: acpi_get_sci_interrupt
>> > + *
>> > + * PARAMETERS: none
>> > + *
>> > + * RETURN: SCI Interrupt
>> > + *
>> > + * DESCRIPTION: Return the ACPI SCI interrupt
>> > + *
>> > + ******************************************************************************/
>> > +u16 acpi_get_sci_interrupt(void)
>> > +{
>> > + return acpi_gbl_FADT.sci_interrupt;
>> > +}
>> > +
>> > +ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
>> > +
>> > +/*******************************************************************************
>> > + *
>> > + * FUNCTION: acpi_set_sci_interrupt
>> > + *
>> > + * PARAMETERS: override sci irq
>> > + *
>> > + * RETURN: status
>> > + *
>> > + * DESCRIPTION: Set the ACPI SCI interrupt
>> > + *
>> > + ******************************************************************************/
>> > +acpi_status acpi_set_sci_interrupt(u16 irq)
>> > +{
>> > + acpi_gbl_FADT.sci_interrupt = irq;
>> > + return AE_OK;
>> > +}
>> > +
>> > +ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
>> > +
>> > Index: linux-2.6/drivers/acpi/bus.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03 10:19:00.203030002 +0800
>> > +++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -890,14 +890,14 @@
>> > acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
>> > }
>> > /* Set PIC-mode SCI trigger type */
>> > - acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
>> > + acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
>> > (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
>> > } else {
>> > /*
>> > * now that acpi_gbl_FADT is initialized,
>> > * update it with result from INT_SRC_OVR parsing
>> > */
>> > - acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>> > + acpi_set_sci_interrupt(acpi_sci_override_gsi);
>> > }
>> > #endif
>> >
>> > Index: linux-2.6/drivers/acpi/osl.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03 10:19:48.993030009 +0800
>> > +++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -534,7 +534,7 @@
>> > * ACPI interrupts different from the SCI in our copy of the FADT are
>> > * not supported.
>> > */
>> > - if (gsi != acpi_gbl_FADT.sci_interrupt)
>> > + if (gsi != acpi_get_sci_interrupt())
>> > return AE_BAD_PARAMETER;
>> >
>> > if (acpi_irq_handler)
>> > @@ -559,7 +559,7 @@
>> >
>> > acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>> > {
>> > - if (irq != acpi_gbl_FADT.sci_interrupt)
>> > + if (irq != acpi_get_sci_interrupt())
>> > return AE_BAD_PARAMETER;
>> >
>> > free_irq(irq, acpi_irq);
>> > @@ -1622,7 +1622,7 @@
>> > acpi_status acpi_os_terminate(void)
>> > {
>> > if (acpi_irq_handler) {
>> > - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
>> > + acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
>> > acpi_irq_handler);
>> > }
>> >
>> > Index: linux-2.6/drivers/acpi/pci_link.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03 10:19:00.203030002 +0800
>> > +++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -508,7 +508,7 @@
>> > }
>> > }
>> > /* Add a penalty for the SCI */
>> > - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>> > + acpi_irq_penalty[acpi_get_sci_interrupt()] += PIRQ_PENALTY_PCI_USING;
>> > return 0;
>> > }
>> >
>> > Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:19:00.193030001 +0800
>> > +++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:20:50.833030004 +0800
>> > @@ -104,7 +104,7 @@
>> > pci_dev_put(pdev);
>> >
>> > /* we're sharing the IRQ with ACPI */
>> > - irq = acpi_gbl_FADT.sci_interrupt;
>> > + irq = acpi_get_sci_interrupt();
>> > if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON", dcon)) {
>> > printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation failed\n", irq);
>> > return 1;
>> > Index: linux-2.6/include/acpi/acpixf.h
>> > ===================================================================
>> > --- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03 10:19:00.253029998 +0800
>> > +++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004 +0800
>> > @@ -411,6 +411,10 @@
>> > acpi_info(const char *module_name,
>> > u32 line_number, const char *format, ...) ACPI_PRINTF_LIKE(3);
>> >
>> > +u16 acpi_get_sci_interrupt(void);
>> > +
>> > +acpi_status acpi_set_sci_interrupt(u16 irq);
>> > +
>> > /*
>> > * Debug output
>> > */
>> >
>> > ---
>> >
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
2011-05-05 17:30 ` Bjorn Helgaas
@ 2011-05-05 17:48 ` Moore, Robert
2011-05-06 2:07 ` lan,Tianyu
1 sibling, 0 replies; 6+ messages in thread
From: Moore, Robert @ 2011-05-05 17:48 UTC (permalink / raw)
To: Bjorn Helgaas, Lan, Tianyu
Cc: Lin, Ming M, lenb@kernel.org, linux-acpi@vger.kernel.org,
Zhang, Rui
Bjorn,
It's an ACPI 5.0 thing.
We are going to hold back this patch until 5.0 stabilizes and we integrate this support into the base ACPICA code first.
Bob
> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Thursday, May 05, 2011 10:31 AM
> To: Lan, Tianyu
> Cc: Lin, Ming M; lenb@kernel.org; linux-acpi@vger.kernel.org; Moore,
> Robert; Zhang, Rui
> Subject: Re: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two
> functions in some places.
>
> >> On Tue, 2011-05-03 at 14:10 +0800, Lan, Tianyu wrote:
> >> > Some places use the acpi_gbl_FADT.sci_interrupt directly to get
> the ACPI SCI.ACPI SCI will not only be stored in the FADT.
>
> You need at least one space after a sentence-ending period, e.g., "...
> get the ACPI SCI. ACPI SCI will not ..."
>
> I can't figure out the intent of the patch as a whole. It seems to be
> connected with "ACPI SCI will not only be stored in the FADT," but
> that isn't clear enough to tell me what's going on. Is there a
> scenario where we'll want to look somewhere else first, then fall back
> to looking at the FADT? This patch didn't implement anything like
> that, but maybe you intend future changes to acpi_get_sci_interrupt()
> and acpi_set_sci_interrupt()?
>
> I think the name "acpi_get_sci_interrupt()" is somewhat redundant, or
> at least confusing: "SCI" means "system control interrupt," so you're
> really saying "acpi_get_system_control_interrupt_interrupt." I guess
> the first "interrupt" refers to the mechanism itself, and the second
> to the vector number or GSI. Maybe "acpi_get_sci_gsi()" would be
> better?
>
> There's also an fadt->sci_interrupt reference in
> arch/ia64/kernel/acpi.c. I don't know the intent of your patch or
> whether that intent applies to ia64 as well as x86, but it's always
> better if you can converge things rather than make x86 and ia64 more
> different.
>
> Unless I missed something, this patch makes no functional change.
> That's fine, especially if you're getting ready for some future work,
> but it's worth mentioning that this patch is reorganization only.
>
> Bjorn
>
> >> > So this patch replaces acpi_gbl_FADT.sci_interrupt with two
> functions for getting and setting.
> >>
> >> Some trivial patch style comments,
> >>
> >> We usually add "acpi:" prefix to the subject, like
> >>
> >> [PATCH] acpi: xxxxx
> >>
> >> And if acpica files are also touched, we add "acpi/acpica:" prefix,
> like
> >>
> >> [PATCH] acpi/acpica: xxxx
> >>
> >> Another thing, the change logs seems too long for each line, you'd
> >> better split them short for each line. So that makes logs easy to
> read.
> >>
> >> Thanks,
> >> Lin Ming
> >>
> >> >
> >> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> > ---
> >> > arch/x86/kernel/acpi/boot.c | 8 ++---
> >> > drivers/acpi/acpica/evgpeinit.c | 5 +--
> >> > drivers/acpi/acpica/evgpeutil.c | 4 +-
> >> > drivers/acpi/acpica/evsci.c | 4 +-
> >> > drivers/acpi/acpica/evxface.c | 38
> +++++++++++++++++++++++++++
> >> > drivers/acpi/bus.c | 4 +-
> >> > drivers/acpi/osl.c | 6 ++--
> >> > drivers/acpi/pci_link.c | 2 -
> >> > drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
> >> > include/acpi/acpixf.h | 4 ++
> >> > 10 files changed, 59 insertions(+), 18 deletions(-)
> >> >
> >> > Index: linux-2.6/arch/x86/kernel/acpi/boot.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03
> 10:19:00.163029998 +0800
> >> > +++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -408,7 +408,7 @@
> >> >
> >> > acpi_table_print_madt_entry(header);
> >> >
> >> > - if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
> >> > + if (intsrc->source_irq == acpi_get_sci_interrupt()) {
> >> > acpi_sci_ioapic_setup(intsrc->source_irq,
> >> > intsrc->inti_flags &
> ACPI_MADT_POLARITY_MASK,
> >> > (intsrc->inti_flags &
> ACPI_MADT_TRIGGER_MASK) >> 2,
> >> > @@ -1100,7 +1100,7 @@
> >> > return gsi;
> >> >
> >> > /* Don't set up the ACPI SCI because it's already set up */
> >> > - if (acpi_gbl_FADT.sci_interrupt == gsi)
> >> > + if (acpi_get_sci_interrupt() == gsi)
> >> > return gsi;
> >> >
> >> > ioapic = mp_find_ioapic(gsi);
> >> > @@ -1184,8 +1184,8 @@
> >> > * pretend we got one so we can set the SCI flags.
> >> > */
> >> > if (!acpi_sci_override_gsi)
> >> > - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0,
> 0,
> >> > - acpi_gbl_FADT.sci_interrupt);
> >> > + acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
> >> > + acpi_get_sci_interrupt());
> >> >
> >> > /* Fill in identity legacy mappings where no override */
> >> > mp_config_acpi_legacy_irqs();
> >> > Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03
> 10:19:00.243029997 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -131,7 +131,7 @@
> >> > status =
> acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
> >> >
> &acpi_gbl_FADT.xgpe0_block,
> >> > register_count0, 0,
> >> > -
> acpi_gbl_FADT.sci_interrupt,
> >> > +
> acpi_get_sci_interrupt(),
> >> >
> &acpi_gbl_gpe_fadt_blocks[0]);
> >> >
> >> > if (ACPI_FAILURE(status)) {
> >> > @@ -170,8 +170,7 @@
> >> >
> &acpi_gbl_FADT.xgpe1_block,
> >> > register_count1,
> >> >
> acpi_gbl_FADT.gpe1_base,
> >> > - acpi_gbl_FADT.
> >> > - sci_interrupt,
> >> > +
> acpi_get_sci_interrupt(),
> >> >
> &acpi_gbl_gpe_fadt_blocks
> >> > [1]);
> >> >
> >> > Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03
> 10:19:00.213030003 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -253,7 +253,7 @@
> >> >
> >> > /* Install new interrupt handler if not SCI_INT */
> >> >
> >> > - if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
> >> > + if (interrupt_number != acpi_get_sci_interrupt()) {
> >> > status =
> acpi_os_install_interrupt_handler(interrupt_number,
> >> >
> acpi_ev_gpe_xrupt_handler,
> >> > gpe_xrupt);
> >> > @@ -290,7 +290,7 @@
> >> >
> >> > /* We never want to remove the SCI interrupt handler */
> >> >
> >> > - if (gpe_xrupt->interrupt_number ==
> acpi_gbl_FADT.sci_interrupt) {
> >> > + if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
> >> > gpe_xrupt->gpe_block_list_head = NULL;
> >> > return_ACPI_STATUS(AE_OK);
> >> > }
> >> > Index: linux-2.6/drivers/acpi/acpica/evsci.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03
> 10:19:00.223030004 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -142,7 +142,7 @@
> >> > ACPI_FUNCTION_TRACE(ev_install_sci_handler);
> >> >
> >> > status =
> >> > - acpi_os_install_interrupt_handler((u32)
> acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_os_install_interrupt_handler((u32)
> acpi_get_sci_interrupt(),
> >> >
> acpi_ev_sci_xrupt_handler,
> >> >
> acpi_gbl_gpe_xrupt_list_head);
> >> > return_ACPI_STATUS(status);
> >> > @@ -176,7 +176,7 @@
> >> > /* Just let the OS remove the handler and disable the level */
> >> >
> >> > status =
> >> > - acpi_os_remove_interrupt_handler((u32)
> acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_os_remove_interrupt_handler((u32)
> acpi_get_sci_interrupt(),
> >> >
> acpi_ev_sci_xrupt_handler);
> >> >
> >> > return_ACPI_STATUS(status);
> >> > Index: linux-2.6/drivers/acpi/acpica/evxface.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03
> 10:19:00.213030003 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -983,3 +983,41 @@
> >> > }
> >> >
> >> > ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
> >> > +
> >> >
> +/*********************************************************************
> **********
> >> > + *
> >> > + * FUNCTION: acpi_get_sci_interrupt
> >> > + *
> >> > + * PARAMETERS: none
> >> > + *
> >> > + * RETURN: SCI Interrupt
> >> > + *
> >> > + * DESCRIPTION: Return the ACPI SCI interrupt
> >> > + *
> >> > +
> ***********************************************************************
> *******/
> >> > +u16 acpi_get_sci_interrupt(void)
> >> > +{
> >> > + return acpi_gbl_FADT.sci_interrupt;
> >> > +}
> >> > +
> >> > +ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
> >> > +
> >> >
> +/*********************************************************************
> **********
> >> > + *
> >> > + * FUNCTION: acpi_set_sci_interrupt
> >> > + *
> >> > + * PARAMETERS: override sci irq
> >> > + *
> >> > + * RETURN: status
> >> > + *
> >> > + * DESCRIPTION: Set the ACPI SCI interrupt
> >> > + *
> >> > +
> ***********************************************************************
> *******/
> >> > +acpi_status acpi_set_sci_interrupt(u16 irq)
> >> > +{
> >> > + acpi_gbl_FADT.sci_interrupt = irq;
> >> > + return AE_OK;
> >> > +}
> >> > +
> >> > +ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
> >> > +
> >> > Index: linux-2.6/drivers/acpi/bus.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03
> 10:19:00.203030002 +0800
> >> > +++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004
> +0800
> >> > @@ -890,14 +890,14 @@
> >> > acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >> > }
> >> > /* Set PIC-mode SCI trigger type */
> >> > - acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
> >> > (acpi_sci_flags &
> ACPI_MADT_TRIGGER_MASK) >> 2);
> >> > } else {
> >> > /*
> >> > * now that acpi_gbl_FADT is initialized,
> >> > * update it with result from INT_SRC_OVR parsing
> >> > */
> >> > - acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >> > + acpi_set_sci_interrupt(acpi_sci_override_gsi);
> >> > }
> >> > #endif
> >> >
> >> > Index: linux-2.6/drivers/acpi/osl.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03
> 10:19:48.993030009 +0800
> >> > +++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004
> +0800
> >> > @@ -534,7 +534,7 @@
> >> > * ACPI interrupts different from the SCI in our copy of the
> FADT are
> >> > * not supported.
> >> > */
> >> > - if (gsi != acpi_gbl_FADT.sci_interrupt)
> >> > + if (gsi != acpi_get_sci_interrupt())
> >> > return AE_BAD_PARAMETER;
> >> >
> >> > if (acpi_irq_handler)
> >> > @@ -559,7 +559,7 @@
> >> >
> >> > acpi_status acpi_os_remove_interrupt_handler(u32 irq,
> acpi_osd_handler handler)
> >> > {
> >> > - if (irq != acpi_gbl_FADT.sci_interrupt)
> >> > + if (irq != acpi_get_sci_interrupt())
> >> > return AE_BAD_PARAMETER;
> >> >
> >> > free_irq(irq, acpi_irq);
> >> > @@ -1622,7 +1622,7 @@
> >> > acpi_status acpi_os_terminate(void)
> >> > {
> >> > if (acpi_irq_handler) {
> >> > -
> acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> >> > +
> acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
> >> > acpi_irq_handler);
> >> > }
> >> >
> >> > Index: linux-2.6/drivers/acpi/pci_link.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03
> 10:19:00.203030002 +0800
> >> > +++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03
> 10:20:50.833030004 +0800
> >> > @@ -508,7 +508,7 @@
> >> > }
> >> > }
> >> > /* Add a penalty for the SCI */
> >> > - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
> PIRQ_PENALTY_PCI_USING;
> >> > + acpi_irq_penalty[acpi_get_sci_interrupt()] +=
> PIRQ_PENALTY_PCI_USING;
> >> > return 0;
> >> > }
> >> >
> >> > Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
> 2011-05-03 10:19:00.193030001 +0800
> >> > +++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-
> 05-03 10:20:50.833030004 +0800
> >> > @@ -104,7 +104,7 @@
> >> > pci_dev_put(pdev);
> >> >
> >> > /* we're sharing the IRQ with ACPI */
> >> > - irq = acpi_gbl_FADT.sci_interrupt;
> >> > + irq = acpi_get_sci_interrupt();
> >> > if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON",
> dcon)) {
> >> > printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation
> failed\n", irq);
> >> > return 1;
> >> > Index: linux-2.6/include/acpi/acpixf.h
> >> >
> ===================================================================
> >> > --- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03
> 10:19:00.253029998 +0800
> >> > +++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004
> +0800
> >> > @@ -411,6 +411,10 @@
> >> > acpi_info(const char *module_name,
> >> > u32 line_number, const char *format, ...)
> ACPI_PRINTF_LIKE(3);
> >> >
> >> > +u16 acpi_get_sci_interrupt(void);
> >> > +
> >> > +acpi_status acpi_set_sci_interrupt(u16 irq);
> >> > +
> >> > /*
> >> > * Debug output
> >> > */
> >> >
> >> > ---
> >> >
> >>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-
> acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places.
2011-05-05 17:30 ` Bjorn Helgaas
2011-05-05 17:48 ` Moore, Robert
@ 2011-05-06 2:07 ` lan,Tianyu
1 sibling, 0 replies; 6+ messages in thread
From: lan,Tianyu @ 2011-05-06 2:07 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Lin, Ming M, lenb@kernel.org, linux-acpi@vger.kernel.org,
Moore, Robert, Zhang, Rui
hi Bjorn:
Thanks for your comments and advices. This patch is for ACPI 5.0.
Just like you said, I intend future changes to acpi_get_sci_interrupt()
and acpi_set_sci_interrupt(). This accord to ACPI 5.0 which is not
stable now.
The name "acpi_get_sci_interrupt" is not reasonable.Your analysis
is very comprehensive. I will notice it.
For fadt->sci_interrupt reference
>static int __init acpi_parse_fadt(struct acpi_table_header *table)
>{
> struct acpi_table_header *fadt_header;
> struct acpi_table_fadt *fadt;
>
> if (!table)
> return -EINVAL;
>
> fadt_header = (struct acpi_table_header *)table;
> if (fadt_header->revision != 3)
> return -ENODEV; /* Only deal with ACPI 2.0 FADT */
>
> fadt = (struct acpi_table_fadt *)fadt_header;
>
> acpi_register_gsi(NULL, fadt->sci_interrupt, ACPI_LEVEL_SENSITIVE,
> ACPI_ACTIVE_LOW);
> return 0;
>}
The code just is be used when it is ACPI 2.0. So I haven't
considered it. This blame my unclear comment.
Thank you again. You advices are very useful and helpful.
Tianyu
在 2011-05-06五的 01:30 +0800，Bjorn Helgaas写道：
> >> On Tue, 2011-05-03 at 14:10 +0800, Lan, Tianyu wrote:
> >> > Some places use the acpi_gbl_FADT.sci_interrupt directly to get the ACPI SCI.ACPI SCI will not only be stored in the FADT.
>
> You need at least one space after a sentence-ending period, e.g., "...
> get the ACPI SCI. ACPI SCI will not ..."
>
> I can't figure out the intent of the patch as a whole. It seems to be
> connected with "ACPI SCI will not only be stored in the FADT," but
> that isn't clear enough to tell me what's going on. Is there a
> scenario where we'll want to look somewhere else first, then fall back
> to looking at the FADT? This patch didn't implement anything like
> that, but maybe you intend future changes to acpi_get_sci_interrupt()
> and acpi_set_sci_interrupt()?
>
> I think the name "acpi_get_sci_interrupt()" is somewhat redundant, or
> at least confusing: "SCI" means "system control interrupt," so you're
> really saying "acpi_get_system_control_interrupt_interrupt." I guess
> the first "interrupt" refers to the mechanism itself, and the second
> to the vector number or GSI. Maybe "acpi_get_sci_gsi()" would be
> better?
>
> There's also an fadt->sci_interrupt reference in
> arch/ia64/kernel/acpi.c. I don't know the intent of your patch or
> whether that intent applies to ia64 as well as x86, but it's always
> better if you can converge things rather than make x86 and ia64 more
> different.
>
> Unless I missed something, this patch makes no functional change.
> That's fine, especially if you're getting ready for some future work,
> but it's worth mentioning that this patch is reorganization only.
>
> Bjorn
>
> >> > So this patch replaces acpi_gbl_FADT.sci_interrupt with two functions for getting and setting.
> >>
> >> Some trivial patch style comments,
> >>
> >> We usually add "acpi:" prefix to the subject, like
> >>
> >> [PATCH] acpi: xxxxx
> >>
> >> And if acpica files are also touched, we add "acpi/acpica:" prefix, like
> >>
> >> [PATCH] acpi/acpica: xxxx
> >>
> >> Another thing, the change logs seems too long for each line, you'd
> >> better split them short for each line. So that makes logs easy to read.
> >>
> >> Thanks,
> >> Lin Ming
> >>
> >> >
> >> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> > ---
> >> > arch/x86/kernel/acpi/boot.c | 8 ++---
> >> > drivers/acpi/acpica/evgpeinit.c | 5 +--
> >> > drivers/acpi/acpica/evgpeutil.c | 4 +-
> >> > drivers/acpi/acpica/evsci.c | 4 +-
> >> > drivers/acpi/acpica/evxface.c | 38 +++++++++++++++++++++++++++
> >> > drivers/acpi/bus.c | 4 +-
> >> > drivers/acpi/osl.c | 6 ++--
> >> > drivers/acpi/pci_link.c | 2 -
> >> > drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c | 2 -
> >> > include/acpi/acpixf.h | 4 ++
> >> > 10 files changed, 59 insertions(+), 18 deletions(-)
> >> >
> >> > Index: linux-2.6/arch/x86/kernel/acpi/boot.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/arch/x86/kernel/acpi/boot.c 2011-05-03 10:19:00.163029998 +0800
> >> > +++ linux-2.6/arch/x86/kernel/acpi/boot.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -408,7 +408,7 @@
> >> >
> >> > acpi_table_print_madt_entry(header);
> >> >
> >> > - if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
> >> > + if (intsrc->source_irq == acpi_get_sci_interrupt()) {
> >> > acpi_sci_ioapic_setup(intsrc->source_irq,
> >> > intsrc->inti_flags & ACPI_MADT_POLARITY_MASK,
> >> > (intsrc->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2,
> >> > @@ -1100,7 +1100,7 @@
> >> > return gsi;
> >> >
> >> > /* Don't set up the ACPI SCI because it's already set up */
> >> > - if (acpi_gbl_FADT.sci_interrupt == gsi)
> >> > + if (acpi_get_sci_interrupt() == gsi)
> >> > return gsi;
> >> >
> >> > ioapic = mp_find_ioapic(gsi);
> >> > @@ -1184,8 +1184,8 @@
> >> > * pretend we got one so we can set the SCI flags.
> >> > */
> >> > if (!acpi_sci_override_gsi)
> >> > - acpi_sci_ioapic_setup(acpi_gbl_FADT.sci_interrupt, 0, 0,
> >> > - acpi_gbl_FADT.sci_interrupt);
> >> > + acpi_sci_ioapic_setup(acpi_get_sci_interrupt(), 0, 0,
> >> > + acpi_get_sci_interrupt());
> >> >
> >> > /* Fill in identity legacy mappings where no override */
> >> > mp_config_acpi_legacy_irqs();
> >> > Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:19:00.243029997 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evgpeinit.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -131,7 +131,7 @@
> >> > status = acpi_ev_create_gpe_block(acpi_gbl_fadt_gpe_device,
> >> > &acpi_gbl_FADT.xgpe0_block,
> >> > register_count0, 0,
> >> > - acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_get_sci_interrupt(),
> >> > &acpi_gbl_gpe_fadt_blocks[0]);
> >> >
> >> > if (ACPI_FAILURE(status)) {
> >> > @@ -170,8 +170,7 @@
> >> > &acpi_gbl_FADT.xgpe1_block,
> >> > register_count1,
> >> > acpi_gbl_FADT.gpe1_base,
> >> > - acpi_gbl_FADT.
> >> > - sci_interrupt,
> >> > + acpi_get_sci_interrupt(),
> >> > &acpi_gbl_gpe_fadt_blocks
> >> > [1]);
> >> >
> >> > Index: linux-2.6/drivers/acpi/acpica/evgpeutil.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:19:00.213030003 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evgpeutil.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -253,7 +253,7 @@
> >> >
> >> > /* Install new interrupt handler if not SCI_INT */
> >> >
> >> > - if (interrupt_number != acpi_gbl_FADT.sci_interrupt) {
> >> > + if (interrupt_number != acpi_get_sci_interrupt()) {
> >> > status = acpi_os_install_interrupt_handler(interrupt_number,
> >> > acpi_ev_gpe_xrupt_handler,
> >> > gpe_xrupt);
> >> > @@ -290,7 +290,7 @@
> >> >
> >> > /* We never want to remove the SCI interrupt handler */
> >> >
> >> > - if (gpe_xrupt->interrupt_number == acpi_gbl_FADT.sci_interrupt) {
> >> > + if (gpe_xrupt->interrupt_number == acpi_get_sci_interrupt()) {
> >> > gpe_xrupt->gpe_block_list_head = NULL;
> >> > return_ACPI_STATUS(AE_OK);
> >> > }
> >> > Index: linux-2.6/drivers/acpi/acpica/evsci.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evsci.c 2011-05-03 10:19:00.223030004 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evsci.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -142,7 +142,7 @@
> >> > ACPI_FUNCTION_TRACE(ev_install_sci_handler);
> >> >
> >> > status =
> >> > - acpi_os_install_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_os_install_interrupt_handler((u32) acpi_get_sci_interrupt(),
> >> > acpi_ev_sci_xrupt_handler,
> >> > acpi_gbl_gpe_xrupt_list_head);
> >> > return_ACPI_STATUS(status);
> >> > @@ -176,7 +176,7 @@
> >> > /* Just let the OS remove the handler and disable the level */
> >> >
> >> > status =
> >> > - acpi_os_remove_interrupt_handler((u32) acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_os_remove_interrupt_handler((u32) acpi_get_sci_interrupt(),
> >> > acpi_ev_sci_xrupt_handler);
> >> >
> >> > return_ACPI_STATUS(status);
> >> > Index: linux-2.6/drivers/acpi/acpica/evxface.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/acpica/evxface.c 2011-05-03 10:19:00.213030003 +0800
> >> > +++ linux-2.6/drivers/acpi/acpica/evxface.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -983,3 +983,41 @@
> >> > }
> >> >
> >> > ACPI_EXPORT_SYMBOL(acpi_release_global_lock)
> >> > +
> >> > +/*******************************************************************************
> >> > + *
> >> > + * FUNCTION: acpi_get_sci_interrupt
> >> > + *
> >> > + * PARAMETERS: none
> >> > + *
> >> > + * RETURN: SCI Interrupt
> >> > + *
> >> > + * DESCRIPTION: Return the ACPI SCI interrupt
> >> > + *
> >> > + ******************************************************************************/
> >> > +u16 acpi_get_sci_interrupt(void)
> >> > +{
> >> > + return acpi_gbl_FADT.sci_interrupt;
> >> > +}
> >> > +
> >> > +ACPI_EXPORT_SYMBOL(acpi_get_sci_interrupt)
> >> > +
> >> > +/*******************************************************************************
> >> > + *
> >> > + * FUNCTION: acpi_set_sci_interrupt
> >> > + *
> >> > + * PARAMETERS: override sci irq
> >> > + *
> >> > + * RETURN: status
> >> > + *
> >> > + * DESCRIPTION: Set the ACPI SCI interrupt
> >> > + *
> >> > + ******************************************************************************/
> >> > +acpi_status acpi_set_sci_interrupt(u16 irq)
> >> > +{
> >> > + acpi_gbl_FADT.sci_interrupt = irq;
> >> > + return AE_OK;
> >> > +}
> >> > +
> >> > +ACPI_EXPORT_SYMBOL(acpi_set_sci_interrupt)
> >> > +
> >> > Index: linux-2.6/drivers/acpi/bus.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/bus.c 2011-05-03 10:19:00.203030002 +0800
> >> > +++ linux-2.6/drivers/acpi/bus.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -890,14 +890,14 @@
> >> > acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >> > }
> >> > /* Set PIC-mode SCI trigger type */
> >> > - acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_pic_sci_set_trigger(acpi_get_sci_interrupt(),
> >> > (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> >> > } else {
> >> > /*
> >> > * now that acpi_gbl_FADT is initialized,
> >> > * update it with result from INT_SRC_OVR parsing
> >> > */
> >> > - acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >> > + acpi_set_sci_interrupt(acpi_sci_override_gsi);
> >> > }
> >> > #endif
> >> >
> >> > Index: linux-2.6/drivers/acpi/osl.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/osl.c 2011-05-03 10:19:48.993030009 +0800
> >> > +++ linux-2.6/drivers/acpi/osl.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -534,7 +534,7 @@
> >> > * ACPI interrupts different from the SCI in our copy of the FADT are
> >> > * not supported.
> >> > */
> >> > - if (gsi != acpi_gbl_FADT.sci_interrupt)
> >> > + if (gsi != acpi_get_sci_interrupt())
> >> > return AE_BAD_PARAMETER;
> >> >
> >> > if (acpi_irq_handler)
> >> > @@ -559,7 +559,7 @@
> >> >
> >> > acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> >> > {
> >> > - if (irq != acpi_gbl_FADT.sci_interrupt)
> >> > + if (irq != acpi_get_sci_interrupt())
> >> > return AE_BAD_PARAMETER;
> >> >
> >> > free_irq(irq, acpi_irq);
> >> > @@ -1622,7 +1622,7 @@
> >> > acpi_status acpi_os_terminate(void)
> >> > {
> >> > if (acpi_irq_handler) {
> >> > - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> >> > + acpi_os_remove_interrupt_handler(acpi_get_sci_interrupt(),
> >> > acpi_irq_handler);
> >> > }
> >> >
> >> > Index: linux-2.6/drivers/acpi/pci_link.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/acpi/pci_link.c 2011-05-03 10:19:00.203030002 +0800
> >> > +++ linux-2.6/drivers/acpi/pci_link.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -508,7 +508,7 @@
> >> > }
> >> > }
> >> > /* Add a penalty for the SCI */
> >> > - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> >> > + acpi_irq_penalty[acpi_get_sci_interrupt()] += PIRQ_PENALTY_PCI_USING;
> >> > return 0;
> >> > }
> >> >
> >> > Index: linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:19:00.193030001 +0800
> >> > +++ linux-2.6/drivers/staging/olpc_dcon/olpc_dcon_xo_1_5.c 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -104,7 +104,7 @@
> >> > pci_dev_put(pdev);
> >> >
> >> > /* we're sharing the IRQ with ACPI */
> >> > - irq = acpi_gbl_FADT.sci_interrupt;
> >> > + irq = acpi_get_sci_interrupt();
> >> > if (request_irq(irq, &dcon_interrupt, IRQF_SHARED, "DCON", dcon)) {
> >> > printk(KERN_ERR PREFIX "DCON (IRQ%d) allocation failed\n", irq);
> >> > return 1;
> >> > Index: linux-2.6/include/acpi/acpixf.h
> >> > ===================================================================
> >> > --- linux-2.6.orig/include/acpi/acpixf.h 2011-05-03 10:19:00.253029998 +0800
> >> > +++ linux-2.6/include/acpi/acpixf.h 2011-05-03 10:20:50.833030004 +0800
> >> > @@ -411,6 +411,10 @@
> >> > acpi_info(const char *module_name,
> >> > u32 line_number, const char *format, ...) ACPI_PRINTF_LIKE(3);
> >> >
> >> > +u16 acpi_get_sci_interrupt(void);
> >> > +
> >> > +acpi_status acpi_set_sci_interrupt(u16 irq);
> >> > +
> >> > /*
> >> > * Debug output
> >> > */
> >> >
> >> > ---
> >> >
> >>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-06 2:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-03 6:10 [PATCH] Replace acpi_gbl_FADT.sci_interrupt with two functions in some places lan,Tianyu
2011-05-04 1:48 ` Lin Ming
2011-05-05 0:24 ` lan,Tianyu
2011-05-05 17:30 ` Bjorn Helgaas
2011-05-05 17:48 ` Moore, Robert
2011-05-06 2:07 ` lan,Tianyu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox