On Tue, Jul 23, 2013 at 4:51 AM, Loh Tien Hock <thloh85@xxxxxxxxx> wrote:
> On Tue, Jul 23, 2013 at 3:07 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>> On Mon, Jul 8, 2013 at 9:04 AM, <thloh.linux@xxxxxxxxx> 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
--
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
