Re: [PATCH 1/1] drivers/gpio: Altera soft IP GPIO driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux