Re: [PATCH v2 14/41] arm : acpi add helper function for setting interrupt type

[Julien Grall <julien.grall@citrix.com>]

Hi,

On 17/05/15 21:03, Parth Dixit wrote:
> set edge/level type information for an interrupt
> 
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>  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