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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

+shannon

On 20 May 2015 at 22:51, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
>> ---
>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel