diff mbox

[1/2] input: ti_tsc: Enable shared IRQ for TSC

Message ID 1374882674-18403-2-git-send-email-zubair.lutfullah@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zubair Lutfullah July 26, 2013, 11:51 p.m. UTC 
From: "Patil, Rachna" <rachna@ti.com>

Touchscreen and ADC share the same IRQ line from parent MFD core.
Previously only Touchscreen was interrupt based.
With continuous mode support added in ADC driver, driver requires
interrupt to process the ADC samples, so enable shared IRQ flag bit for
touchscreen.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron Aug. 4, 2013, 11:08 a.m. UTC | #1 
On 07/27/13 00:51, Zubair Lutfullah wrote:
> From: "Patil, Rachna" <rachna@ti.com>
> 
> Touchscreen and ADC share the same IRQ line from parent MFD core.
> Previously only Touchscreen was interrupt based.
> With continuous mode support added in ADC driver, driver requires
> interrupt to process the ADC samples, so enable shared IRQ flag bit for
> touchscreen.
> 
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
I'd rather this went via input independent of the other patch
(if not there all that will happen is one or other driver will
fail to probe if both are attempted?)

Can take it through IIO but only with a Dmitry Ack.

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index e1c5300..68d1250 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	unsigned int fsm;
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> -	if (status & IRQENB_FIFO0THRES) {
> -
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> +	 * interrupts are used by ADC,
> +	 * hence return from touchscreen IRQ handler if FIFO1
> +	 * related interrupts occurred.
> +	 */
> +	if ((status & IRQENB_FIFO1THRES) ||
> +			(status & IRQENB_FIFO1OVRRUN) ||
> +			(status & IRQENB_FIFO1UNDRFLW))
> +		return IRQ_NONE;
> +	else if (status & IRQENB_FIFO0THRES) {
>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>  
>  		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
> @@ -315,7 +325,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	}
>  
>  	if (irqclr) {
> -		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
> +		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>  		am335x_tsc_se_update(ts_dev->mfd_tscadc);
>  		return IRQ_HANDLED;
>  	}
> @@ -389,7 +399,7 @@ static int titsc_probe(struct platform_device *pdev)
>  	}
>  
>  	err = request_irq(ts_dev->irq, titsc_irq,
> -			  0, pdev->dev.driver->name, ts_dev);
> +			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to allocate irq.\n");
>  		goto err_free_mem;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Aug. 5, 2013, 4:12 p.m. UTC | #2 
On Sun, Aug 04, 2013 at 12:08:11PM +0100, Jonathan Cameron wrote:
> On 07/27/13 00:51, Zubair Lutfullah wrote:
> > From: "Patil, Rachna" <rachna@ti.com>
> > 
> > Touchscreen and ADC share the same IRQ line from parent MFD core.
> > Previously only Touchscreen was interrupt based.
> > With continuous mode support added in ADC driver, driver requires
> > interrupt to process the ADC samples, so enable shared IRQ flag bit for
> > touchscreen.
> > 
> > Signed-off-by: Patil, Rachna <rachna@ti.com>
> > Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> I'd rather this went via input independent of the other patch
> (if not there all that will happen is one or other driver will
> fail to probe if both are attempted?)
> 
> Can take it through IIO but only with a Dmitry Ack.
> 
> > ---
> >  drivers/input/touchscreen/ti_am335x_tsc.c |   18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> > index e1c5300..68d1250 100644
> > --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> > +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> > @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> >  	unsigned int fsm;
> >  
> >  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> > -	if (status & IRQENB_FIFO0THRES) {
> > -
> > +	/*
> > +	 * ADC and touchscreen share the IRQ line.
> > +	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> > +	 * interrupts are used by ADC,
> > +	 * hence return from touchscreen IRQ handler if FIFO1
> > +	 * related interrupts occurred.
> > +	 */
> > +	if ((status & IRQENB_FIFO1THRES) ||
> > +			(status & IRQENB_FIFO1OVRRUN) ||
> > +			(status & IRQENB_FIFO1UNDRFLW))
> > +		return IRQ_NONE;
> > +	else if (status & IRQENB_FIFO0THRES) {

What happens if both parts have data at the same time? Can both
IRQENB_FIFO1THRES and IRQENB_FIFO0THRES be signalled? What will happen
in this case?

Thanks.
Zubair Lutfullah Aug. 5, 2013, 5:02 p.m. UTC | #3 
On Mon, Aug 05, 2013 at 09:12:56AM -0700, Dmitry Torokhov wrote:
> > > Touchscreen and ADC share the same IRQ line from parent MFD core.
> > > Previously only Touchscreen was interrupt based.
> > > With continuous mode support added in ADC driver, driver requires
> > > interrupt to process the ADC samples, so enable shared IRQ flag bit for
> > > touchscreen.
> > > 
> > > @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> > >  	unsigned int fsm;
> > >  
> > > +	/*
> > > +	 * ADC and touchscreen share the IRQ line.
> > > +	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> > > +	 * interrupts are used by ADC,
> > > +	 * hence return from touchscreen IRQ handler if FIFO1
> > > +	 * related interrupts occurred.
> > > +	 */
> > > +	if ((status & IRQENB_FIFO1THRES) ||
> > > +			(status & IRQENB_FIFO1OVRRUN) ||
> > > +			(status & IRQENB_FIFO1UNDRFLW))
> > > +		return IRQ_NONE;
> > > +	else if (status & IRQENB_FIFO0THRES) {
> 
> What happens if both parts have data at the same time? Can both
> IRQENB_FIFO1THRES and IRQENB_FIFO0THRES be signalled? What will happen
> in this case?

If ADC is sampling and someone is touching the TSC, both interrupts
can signal so closely that for the purpose of the kernel,
they can be seen as signaled together.

FIFO 1 used only by ADC and FIFO1THRES handler is inside the iio/adc driver
FIFO 0 used only by TSC and FIFO0THRES handler is inside the input/touchscreen

Note: These are level interrupts.

I would like some input on how to handle such a situation. 

Thanks
Zubair Lutfullah
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Aug. 5, 2013, 5:40 p.m. UTC | #4 
On Mon, Aug 05, 2013 at 06:02:02PM +0100, Zubair Lutfullah : wrote:
> On Mon, Aug 05, 2013 at 09:12:56AM -0700, Dmitry Torokhov wrote:
> > > > Touchscreen and ADC share the same IRQ line from parent MFD core.
> > > > Previously only Touchscreen was interrupt based.
> > > > With continuous mode support added in ADC driver, driver requires
> > > > interrupt to process the ADC samples, so enable shared IRQ flag bit for
> > > > touchscreen.
> > > > 
> > > > @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> > > >  	unsigned int fsm;
> > > >  
> > > > +	/*
> > > > +	 * ADC and touchscreen share the IRQ line.
> > > > +	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> > > > +	 * interrupts are used by ADC,
> > > > +	 * hence return from touchscreen IRQ handler if FIFO1
> > > > +	 * related interrupts occurred.
> > > > +	 */
> > > > +	if ((status & IRQENB_FIFO1THRES) ||
> > > > +			(status & IRQENB_FIFO1OVRRUN) ||
> > > > +			(status & IRQENB_FIFO1UNDRFLW))
> > > > +		return IRQ_NONE;
> > > > +	else if (status & IRQENB_FIFO0THRES) {
> > 
> > What happens if both parts have data at the same time? Can both
> > IRQENB_FIFO1THRES and IRQENB_FIFO0THRES be signalled? What will happen
> > in this case?
> 
> If ADC is sampling and someone is touching the TSC, both interrupts
> can signal so closely that for the purpose of the kernel,
> they can be seen as signaled together.
> 
> FIFO 1 used only by ADC and FIFO1THRES handler is inside the iio/adc driver
> FIFO 0 used only by TSC and FIFO0THRES handler is inside the input/touchscreen
> 
> Note: These are level interrupts.
> 
> I would like some input on how to handle such a situation. 

It looks like you need to have smart demultiplexing in MFD core of your
driver instead of relying on shared interrupt handler.

Another option would be to check "your" bits, handle the data, clear the
status and then check bits again and return IRQ_NONE instead of
IRQ_HANDLED if other guys bits are set, but it is way too ugly.

Thanks.
Zubair Lutfullah Aug. 5, 2013, 7:21 p.m. UTC | #5 
On Mon, Aug 05, 2013 at 10:40:31AM -0700, Dmitry Torokhov wrote:
> > 
> > FIFO 1 used only by ADC and FIFO1THRES handler is inside the iio/adc driver
> > FIFO 0 used only by TSC and FIFO0THRES handler is inside the input/touchscreen
> > 
> > Note: These are level interrupts.
> > 
> > I would like some input on how to handle such a situation. 
> 
> It looks like you need to have smart demultiplexing in MFD core of your
> driver instead of relying on shared interrupt handler.
> 
> Another option would be to check "your" bits, handle the data, clear the
> status and then check bits again and return IRQ_NONE instead of
> IRQ_HANDLED if other guys bits are set, but it is way too ugly.
> 
> Thanks.
> 
> -- 
> Dmitry

That is going to make a lot of changes in mfd, input and iio
And should require a separate patch series.

Is it possible to accept the current patches to add continuous mode 
to the ADC side as is before the next merge window?

This issue doesn't disturb each side individually..

I'll look into fixing the IRQs after settling continuous mode.

Thanks
Zubair Lutfullah
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russ Dill Aug. 5, 2013, 10:42 p.m. UTC | #6 
On 08/05/2013 10:02 AM, Zubair Lutfullah : wrote:
> On Mon, Aug 05, 2013 at 09:12:56AM -0700, Dmitry Torokhov wrote:
>>>> Touchscreen and ADC share the same IRQ line from parent MFD core.
>>>> Previously only Touchscreen was interrupt based.
>>>> With continuous mode support added in ADC driver, driver requires
>>>> interrupt to process the ADC samples, so enable shared IRQ flag bit for
>>>> touchscreen.
>>>>
>>>> @@ -260,8 +260,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>>>>  	unsigned int fsm;
>>>>  
>>>> +	/*
>>>> +	 * ADC and touchscreen share the IRQ line.
>>>> +	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
>>>> +	 * interrupts are used by ADC,
>>>> +	 * hence return from touchscreen IRQ handler if FIFO1
>>>> +	 * related interrupts occurred.
>>>> +	 */
>>>> +	if ((status & IRQENB_FIFO1THRES) ||
>>>> +			(status & IRQENB_FIFO1OVRRUN) ||
>>>> +			(status & IRQENB_FIFO1UNDRFLW))
>>>> +		return IRQ_NONE;
>>>> +	else if (status & IRQENB_FIFO0THRES) {
>>
>> What happens if both parts have data at the same time? Can both
>> IRQENB_FIFO1THRES and IRQENB_FIFO0THRES be signalled? What will happen
>> in this case?
> 
> If ADC is sampling and someone is touching the TSC, both interrupts
> can signal so closely that for the purpose of the kernel,
> they can be seen as signaled together.
> 
> FIFO 1 used only by ADC and FIFO1THRES handler is inside the iio/adc driver
> FIFO 0 used only by TSC and FIFO0THRES handler is inside the input/touchscreen
> 
> Note: These are level interrupts.
> 
> I would like some input on how to handle such a situation. 
> 
> Thanks
> Zubair Lutfullah
> 

As far as I know, if there are any bits you can handle and ack, do so
and return IRQ_HANDLED. Otherwise, return IRQ_NONE. If there are still
pending bits, the interrupt will fire again, your handler will be called
again, return IRQ_NONE, and the next handler will be called.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..68d1250 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -260,8 +260,18 @@  static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
-	if (status & IRQENB_FIFO0THRES) {
-
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
+	 * interrupts are used by ADC,
+	 * hence return from touchscreen IRQ handler if FIFO1
+	 * related interrupts occurred.
+	 */
+	if ((status & IRQENB_FIFO1THRES) ||
+			(status & IRQENB_FIFO1OVRRUN) ||
+			(status & IRQENB_FIFO1UNDRFLW))
+		return IRQ_NONE;
+	else if (status & IRQENB_FIFO0THRES) {
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
 
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
@@ -315,7 +325,7 @@  static irqreturn_t titsc_irq(int irq, void *dev)
 	}
 
 	if (irqclr) {
-		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
+		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
 		am335x_tsc_se_update(ts_dev->mfd_tscadc);
 		return IRQ_HANDLED;
 	}
@@ -389,7 +399,7 @@  static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;