Re: [PATCH v3] input: Add support for ChipOne icn8318 based touchscreens

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sun, Mar 22, 2015 at 12:00:55PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 22-03-15 05:03, Dmitry Torokhov wrote:
> >Hi Hans,
> >
> >On Tue, Mar 10, 2015 at 06:05:33PM +0100, Hans de Goede wrote:
> >>+	error = devm_request_threaded_irq(dev, client->irq, NULL, icn8318_irq,
> >>+					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >
> >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