From mboxrd@z Thu Jan 1 00:00:00 1970 From: Parth Dixit Subject: Re: [PATCH v2 14/41] arm : acpi add helper function for setting interrupt type Date: Sun, 5 Jul 2015 18:39:56 +0530 Message-ID: References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org> <1431893048-5214-15-git-send-email-parth.dixit@linaro.org> <555CC2A5.7030608@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <555CC2A5.7030608@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: keir@xen.org, Ian Campbell , andrew.cooper3@citrix.com, tim@xen.org, xen-devel , Stefano Stabellini , shannon.zhao@linaro.org, Jan Beulich , Christoffer Dall List-Id: xen-devel@lists.xenproject.org +shannon On 20 May 2015 at 22:51, Julien Grall wrote: > Hi, > > On 17/05/15 21:03, Parth Dixit wrote: >> set edge/level type information for an interrupt >> >> Signed-off-by: Parth Dixit >> --- >> xen/arch/arm/irq.c | 17 +++++++++++++++++ >> xen/include/asm-arm/acpi.h | 26 ++++++++++++++++++++++++++ >> xen/include/asm-arm/irq.h | 2 ++ >> 3 files changed, 45 insertions(+) >> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 376c9f2..1713935 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -679,6 +679,23 @@ int platform_get_irq(const struct dt_device_node *device, int index) >> return irq; >> } >> >> +int set_irq_type(int irq,int type) > > > int set_irq_type(unsigned int irq, unsigned int type) > >> +{ >> + int res; >> + >> + /* Setup the IRQ type */ >> + if ( irq < NR_LOCAL_IRQS ) >> + res = irq_local_set_type(irq, type); >> + else >> + res = irq_set_spi_type(irq, type); >> + >> + if ( res ) >> + return -1; > > It would be better to return res which contain a more meaningful error > than -1. > >> + >> + return 0; >> + >> +} >> + > > At the same time, please call this function from platform_get_irq as the > code is the same. > > Furthermore, please move the function code with the other irq_set_* > function and rename it to irq_set_type in order to keep the same naming > convention. > >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h >> index 0845f14..1767143 100644 >> --- a/xen/include/asm-arm/acpi.h >> +++ b/xen/include/asm-arm/acpi.h >> @@ -50,4 +50,30 @@ static inline void disable_acpi(void) >> >> #define ACPI_GTDT_INTR_MASK ( ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY ) >> >> +/** >> + * IRQ line type. >> + * >> + * ACPI_IRQ_TYPE_NONE - default, unspecified type >> + * ACPI_IRQ_TYPE_EDGE_RISING - rising edge triggered >> + * ACPI_IRQ_TYPE_EDGE_FALLING - falling edge triggered >> + * ACPI_IRQ_TYPE_EDGE_BOTH - rising and falling edge triggered >> + * ACPI_IRQ_TYPE_LEVEL_HIGH - high level triggered >> + * ACPI_IRQ_TYPE_LEVEL_LOW - low level triggered >> + * ACPI_IRQ_TYPE_LEVEL_MASK - Mask to filter out the level bits >> + * ACPI_IRQ_TYPE_SENSE_MASK - Mask for all the above bits >> + * ACPI_IRQ_TYPE_INVALID - Use to initialize the type >> + */ >> +#define ACPI_IRQ_TYPE_NONE 0x00000000 >> +#define ACPI_IRQ_TYPE_EDGE_RISING 0x00000001 >> +#define ACPI_IRQ_TYPE_EDGE_FALLING 0x00000002 >> +#define ACPI_IRQ_TYPE_EDGE_BOTH \ >> + (ACPI_IRQ_TYPE_EDGE_FALLING | ACPI_IRQ_TYPE_EDGE_RISING) >> +#define ACPI_IRQ_TYPE_LEVEL_HIGH 0x00000004 >> +#define ACPI_IRQ_TYPE_LEVEL_LOW 0x00000008 >> +#define ACPI_IRQ_TYPE_LEVEL_MASK \ >> + (ACPI_IRQ_TYPE_LEVEL_LOW | ACPI_IRQ_TYPE_LEVEL_HIGH) >> +#define ACPI_IRQ_TYPE_SENSE_MASK 0x0000000f >> + >> +#define ACPI_IRQ_TYPE_INVALID 0x00000010 >> + > > While having a function to set the type is a good idea. > Using a separate set a define and mixing them together is wrong. > > In Xen we only care about edge vs level. > > Although, if you really want to keep all these types. It should be > firmware agnostic. > > >> #endif /*_ASM_ARM_ACPI_H*/ >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h >> index 34b492b..ddad0a9 100644 >> --- a/xen/include/asm-arm/irq.h >> +++ b/xen/include/asm-arm/irq.h >> @@ -51,6 +51,8 @@ void arch_move_irqs(struct vcpu *v); >> /* Set IRQ type for an SPI */ >> int irq_set_spi_type(unsigned int spi, unsigned int type); >> >> +int set_irq_type(int irq,int type); > > int set_irq_type(unsigned int irq, unsigned int type); > >> + >> int platform_get_irq(const struct dt_device_node *device, int index); >> >> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask); >> > > Regards, > > -- > Julien Grall