Shouldn't we let DT data tell us what trigger to use? I.e. just leave
IRQF_ONESHOT here?
No need to resubmit, just let me know if you agree/disagree...
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Shouldn't we let DT data tell us what trigger to use? I.e. just leave
IRQF_ONESHOT here?
That is an interesting question, that new data is available is signalled by
the irq pin of the chip going low is a property of the chip, not the board
layout, so I believe it is best to leave this as is.
Also note that if we want to get this from devicetree, that simply leaving out the
flag is not enough, we must specifically get the data from devicetree and pass
it into request_irq AFAICT. So the above would change to:
irqflags = irqd_get_trigger_type(irq_get_irq_data(client->irq)) | IRQF_ONESHOT,
error = devm_request_threaded_irq(dev, client->irq, NULL, icn8318_irq, irqflags,
Regards,
Hans
Shouldn't we let DT data tell us what trigger to use? I.e. just leave
IRQF_ONESHOT here?
That is an interesting question, that new data is available is signalled by
the irq pin of the chip going low is a property of the chip, not the board
layout, so I believe it is best to leave this as is.
My concern is that even if pin behavior is property of chip maybe on
some boards we want to use level-triggered interrupts instead of edge?
And if we indeed want to hard-code the trigger then shouldn't the
binding document use onecell mapping (so that users do not attempt to
configure triggers from DT)?
Also note that if we want to get this from devicetree, that simply leaving out the
flag is not enough, we must specifically get the data from devicetree and pass
it into request_irq AFAICT. So the above would change to:
irqflags = irqd_get_trigger_type(irq_get_irq_data(client->irq)) | IRQF_ONESHOT,
error = devm_request_threaded_irq(dev, client->irq, NULL, icn8318_irq, irqflags,
No, of_irq_get() that i2c core calls before probing driver should
already set the trigger type for us. There is no need for the individual
drivers to do that.
Thanks.
--
Dmitry
Shouldn't we let DT data tell us what trigger to use? I.e. just leave
IRQF_ONESHOT here?
That is an interesting question, that new data is available is signalled by
the irq pin of the chip going low is a property of the chip, not the board
layout, so I believe it is best to leave this as is.
My concern is that even if pin behavior is property of chip maybe on
some boards we want to use level-triggered interrupts instead of edge?
And if we indeed want to hard-code the trigger then shouldn't the
binding document use onecell mapping (so that users do not attempt to
configure triggers from DT)?
The number of irq cells is specified by the interrupt controller driver
rather then by the device binding.
quoted
Also note that if we want to get this from devicetree, that simply leaving out the
flag is not enough, we must specifically get the data from devicetree and pass
it into request_irq AFAICT. So the above would change to:
irqflags = irqd_get_trigger_type(irq_get_irq_data(client->irq)) | IRQF_ONESHOT,
error = devm_request_threaded_irq(dev, client->irq, NULL, icn8318_irq, irqflags,
No, of_irq_get() that i2c core calls before probing driver should
already set the trigger type for us. There is no need for the individual
drivers to do that.
Ah I see, ok I've just tested removing the IRQF_TRIGGER_FALLING flag and indeed
things still work fine, so feel free to merge this patch with that flagged dropped.
Regards,
Hans