On Mon, Jul 29, 2013 at 6:24 PM, Loh Tien Hock <thloh@xxxxxxxxxx> wrote:
> On Tue, Jul 30, 2013 at 12:11 AM, Linus Walleij
>>>>> +static int altera_gpio_irq_set_type(struct irq_data *d,
>>>>> + unsigned int type)
>>>>> +{
>>>>> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
>>>>> +
>>>>> + if (type == IRQ_TYPE_NONE)
>>>>> + return 0;
>>>>> +
>>>>> + if (altera_gc->level_trigger) {
>>>>> + if (type == IRQ_TYPE_LEVEL_HIGH)
>>>>> + return 0;
>>>>> + } else {
>>>>> + if (type == IRQ_TYPE_EDGE_RISING &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_RISING)
>>>>> + return 0;
>>>>> + else if (type == IRQ_TYPE_EDGE_FALLING &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_FALLING)
>>>>> + return 0;
>>>>> + else if (type == IRQ_TYPE_EDGE_BOTH &&
>>>>> + altera_gc->edge_type == ALTERA_IRQ_BOTH)
>>>>> + return 0;
>>>>
>>>> I don't quite realize why you need the local version of
>>>> the IRQflag at all. Just store the standard edge indicator
>>>> in an unsigned long?
>>>
>>> I don't understand the comment. Can you elaborate further?
>>
>> I misunderstood it for the above reason. But get rid of the
>> custom ALTERA_IRQ_* defines and just use the standard
>> definitions for IRQ_TYPE_*, the local defines does not add
>> anything useful.
>
> We still need to compare the altera_gc->edge_type to the expected ones
> (so that we don't allow incorrect configuration, eg. the GPIO is
> configured as rising, but set_type tries to set falling).
> So we should compare it with magic numbers, instead of custom defines
> like the example below?
> if (type == IRQ_TYPE_EDGE_RISING &&
> altera_gc->edge_type == 0)
> return 0;
And why can't altera_gc->edge_type use exactly the same enumerators
so it becomes:
(type == IRQ_TYPE_EDGE_RISING &&
altera_gc->edge_type == IRQ_TYPE_EDGE_RISING)
?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
