Re: [PATCH v7 11/14] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

From: Marc Zyngier
Date: Mon Apr 29 2019 - 04:48:03 EST


On 23/04/2019 11:00, Lokesh Vutla wrote:
> Hi Marc,

[...]

>> +/**
>> + * ti_sci_inta_set_type() - Update the trigger type of the irq.
>> + * @data: Pointer to corresponding irq_data
>> + * @type: Trigger type as specified by user
>> + *
>> + * Note: This updates the handle_irq callback for level msi.
>> + *
>> + * Return 0 if all went well else appropriate error.
>> + */
>> +static int ti_sci_inta_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + struct irq_desc *desc = irq_to_desc(data->irq);
>> +
>> + /*
>> + * .alloc default sets handle_edge_irq. But if the user specifies
>> + * that IRQ is level MSI, then update the handle to handle_level_irq
>> + */
>> + if (type & IRQF_TRIGGER_HIGH)
>> + desc->handle_irq = handle_level_irq;
>> +
>> + return 0;
>
>
> Returning error value is causing request_irq to fail, so still returning 0. Do
> you suggest any other method to handle this?

But that is the very point, isn't it? If you pass the wrong triggering
type to request_irq, it *must* fail. What you should have is something like:

switch (type & IRQ_TYPE_SENSE_MASK) {
case IRQF_TRIGGER_HIGH:
desc->handle_irq = handle_level_irq;
return 0;
case IRQ_TYPE_EDGE_RISING:
return 0;
default:
return -EINVAL;
}

(adjust as necessary).

What's wrong with this?

Thanks,

M.
--
Jazz is not dead. It just smells funny...