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

[Linus Walleij <linus.walleij@linaro.org>]

On Tue, Jul 23, 2013 at 4:51 AM, Loh Tien Hock <thloh85@gmail.com> wrote:
> On Tue, Jul 23, 2013 at 3:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> On Mon, Jul 8, 2013 at 9:04 AM,  <thloh.linux@gmail.com> wrote:
>>> +- level_trigger: Specifies whether the GPIO interrupt is level trigger (high).
>>> +  This field is required if the Altera GPIO controller used has IRQ enabled.
>>> +- edge_type: Specifies edge type if the GPIO interrupt is edge trigger:
>>> +       0 = Rising edge
>>> +       1 = Falling edge
>>> +       2 = Both edge
>>
>> No thanks.
>
> I don't understand the comment here. Could you elaborate further?
> The edge_type is required as the soft IP's interrupt type isn't
> software configurable, and thus is a hardware parameter passed in from
> device tree blob.

Aha I see. Explain this in the binding document, i.e. state that
this is a synthesis parameter in the hardware block and not
software controlled.

>>> +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.

>>> +       chip->irq_mask(&desc->irq_data);
>>> +
>>> +       if (altera_gc->level_trigger) {
>>> +               status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
>>> +               status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
>>> +
>>> +               for (base = 0; base < mm_gc->gc.ngpio; base++) {
>>> +                       if (BIT(base) & status) {
>>> +                               generic_handle_irq(irq_linear_revmap(
>>> +                                       altera_gc->irq, base));
>>> +                       }
>>> +               }
>>> +       } else {
>>> +               while ((status =
>>> +                       (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
>>> +                       readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
>>> +                       writel_relaxed(status,
>>> +                               mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
>>
>> Nice use if relaxed accessors in the irq handler!
>
> I don't understand the comment. Could you elaborate further?

I am just saying that you are doing the right thing, it is a positive
comment :-)

Yours,
Linus Walleij