[PATCH 1/2] mrst_max3110: fix unbalanced IRQ issue during resume

[PATCH 1/2] mrst_max3110: fix unbalanced IRQ issue during resume

[David Cohen]

From: "Chen, Jie" <jie.d.chen@intel.com>

During resume, a startup will request_irq again, meantime resume function's
enable_irq will cause unbalanced IRQ issue.
Fix this issue by moving request_irq to probe function.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
Signed-off-by: Chen, Jie <jie.d.chen@intel.com>
---
 drivers/tty/serial/mrst_max3110.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/mrst_max3110.c b/drivers/tty/serial/mrst_max3110.c
index a67e708..6f618dd 100644
--- a/drivers/tty/serial/mrst_max3110.c
+++ b/drivers/tty/serial/mrst_max3110.c
@@ -43,6 +43,7 @@
 
 #include <linux/kthread.h>
 #include <linux/spi/spi.h>
+#include <linux/pm.h>
 
 #include "mrst_max3110.h"
 
@@ -491,19 +492,9 @@ static int serial_m3110_startup(struct uart_port *port)
 	port->state->port.low_latency = 1;
 
 	if (max->irq) {
-		max->read_thread = NULL;
-		ret = request_irq(max->irq, serial_m3110_irq,
-				IRQ_TYPE_EDGE_FALLING, "max3110", max);
-		if (ret) {
-			max->irq = 0;
-			pr_err(PR_FMT "unable to allocate IRQ, polling\n");
-		}  else {
-			/* Enable RX IRQ only */
-			config |= WC_RXA_IRQ_ENABLE;
-		}
-	}
-
-	if (max->irq == 0) {
+		/* Enable RX IRQ only */
+		config |= WC_RXA_IRQ_ENABLE;
+	} else {
 		/* If IRQ is disabled, start a read thread for input data */
 		max->read_thread =
 			kthread_run(max3110_read_thread, max, "max3110_read");
@@ -517,8 +508,6 @@ static int serial_m3110_startup(struct uart_port *port)
 
 	ret = max3110_out(max, config);
 	if (ret) {
-		if (max->irq)
-			free_irq(max->irq, max);
 		if (max->read_thread)
 			kthread_stop(max->read_thread);
 		max->read_thread = NULL;
@@ -540,9 +529,6 @@ static void serial_m3110_shutdown(struct uart_port *port)
 		max->read_thread = NULL;
 	}
 
-	if (max->irq)
-		free_irq(max->irq, max);
-
 	/* Disable interrupts from this port */
 	config = WC_TAG | WC_SW_SHDI;
 	max3110_out(max, config);
@@ -840,6 +826,16 @@ static int serial_m3110_probe(struct spi_device *spi)
 		goto err_kthread;
 	}
 
+	if (max->irq) {
+		ret = request_irq(max->irq, serial_m3110_irq,
+				IRQ_TYPE_EDGE_FALLING, "max3110", max);
+		if (ret) {
+			max->irq = 0;
+			dev_warn(&spi->dev,
+			"unable to allocate IRQ, will use polling method\n");
+		}
+	}
+
 	spi_set_drvdata(spi, max);
 	pmax = max;
 
@@ -867,6 +863,9 @@ static int serial_m3110_remove(struct spi_device *dev)
 
 	free_page((unsigned long)max->con_xmit.buf);
 
+	if (max->irq)
+		free_irq(max->irq, max);
+
 	if (max->main_thread)
 		kthread_stop(max->main_thread);
 
-- 
1.8.4.rc3

[PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

From: Ivan Gorinov <ivan.gorinov@intel.com>

The change in the max3110 driver makes the IRQ handling threaded, now
the handler is called only once per received character. Without that
change, we had many (more than 100) interrupts per one received
character.

Unfortunately, SFI interface does not support IRQ polarity and
triggering modes, so we have to keep the hacks as hard-coded device
names and IRQ numbers until we switch to ACPI.

Edge-triggered IRQ still supported to keep old platforms working.
Use platform data to pass the irq mode argument.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
Signed-off-by: Li Ning <ning.li@intel.com>
Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/tty/serial/mrst_max3110.c | 35 +++++++++++++++++++++++++++--------
 include/linux/serial_max3110.h    | 16 ++++++++++++++++
 2 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100644 include/linux/serial_max3110.h

diff --git a/drivers/tty/serial/mrst_max3110.c b/drivers/tty/serial/mrst_max3110.c
index 6f618dd..3778dab 100644
--- a/drivers/tty/serial/mrst_max3110.c
+++ b/drivers/tty/serial/mrst_max3110.c
@@ -40,6 +40,7 @@
 #include <linux/tty_flip.h>
 #include <linux/serial_core.h>
 #include <linux/serial_reg.h>
+#include <linux/serial_max3110.h>
 
 #include <linux/kthread.h>
 #include <linux/spi/spi.h>
@@ -68,6 +69,7 @@ struct uart_max3110 {
 	u8 clock;
 	u8 parity, word_7bits;
 	u16 irq;
+	u16 irq_edge_triggered;
 
 	unsigned long uart_flags;
 
@@ -415,8 +417,8 @@ static int max3110_main_thread(void *_max)
 				max->uart_flags || kthread_should_stop());
 
 		mutex_lock(&max->thread_mutex);
-
-		if (test_and_clear_bit(BIT_IRQ_PENDING, &max->uart_flags))
+		if (max->irq_edge_triggered &&
+			test_and_clear_bit(BIT_IRQ_PENDING, &max->uart_flags))
 			max3110_con_receive(max);
 
 		/* first handle console output */
@@ -438,11 +440,15 @@ static irqreturn_t serial_m3110_irq(int irq, void *dev_id)
 {
 	struct uart_max3110 *max = dev_id;
 
-	/* max3110's irq is a falling edge, not level triggered,
-	 * so no need to disable the irq */
+	if (max->irq_edge_triggered) {
+		/* max3110's irq is a falling edge, not level triggered,
+		 * so no need to disable the irq */
 
-	if (!test_and_set_bit(BIT_IRQ_PENDING, &max->uart_flags))
-		wake_up(&max->wq);
+		if (!test_and_set_bit(BIT_IRQ_PENDING, &max->uart_flags))
+			wake_up(&max->wq);
+	} else {
+		max3110_con_receive(max);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -766,6 +772,10 @@ static int serial_m3110_probe(struct spi_device *spi)
 	void *buffer;
 	u16 res;
 	int ret = 0;
+	struct plat_max3110 *pdata = spi->dev.platform_data;
+
+	if (!pdata)
+		return -EINVAL;
 
 	max = kzalloc(sizeof(*max), GFP_KERNEL);
 	if (!max)
@@ -826,9 +836,18 @@ static int serial_m3110_probe(struct spi_device *spi)
 		goto err_kthread;
 	}
 
+	max->irq_edge_triggered = pdata->irq_edge_triggered;
+
 	if (max->irq) {
-		ret = request_irq(max->irq, serial_m3110_irq,
-				IRQ_TYPE_EDGE_FALLING, "max3110", max);
+		if (max->irq_edge_triggered) {
+			ret = request_irq(max->irq, serial_m3110_irq,
+					IRQ_TYPE_EDGE_FALLING, "max3110", max);
+		} else {
+			ret = request_threaded_irq(max->irq, NULL,
+					serial_m3110_irq,
+					IRQF_ONESHOT, "max3110", max);
+		}
+
 		if (ret) {
 			max->irq = 0;
 			dev_warn(&spi->dev,
diff --git a/include/linux/serial_max3110.h b/include/linux/serial_max3110.h
new file mode 100644
index 0000000..5470556
--- /dev/null
+++ b/include/linux/serial_max3110.h
@@ -0,0 +1,16 @@
+#ifndef _LINUX_SERIAL_MAX3110_H
+#define _LINUX_SERIAL_MAX3110_H
+
+/**
+ * struct plat_max3110 - MAX3110 SPI UART platform data
+ * @irq_edge_trigger: if IRQ is edge triggered
+ *
+ * You should use this structure in your machine description to specify
+ * how the MAX3110 is connected.
+ *
+ */
+struct plat_max3110 {
+	int irq_edge_triggered;
+};
+
+#endif
-- 
1.8.4.rc3

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[Alexander Shiyan]

> The change in the max3110 driver makes the IRQ handling threaded, now
> the handler is called only once per received character. Without that
> change, we had many (more than 100) interrupts per one received
> character.
> 
> Unfortunately, SFI interface does not support IRQ polarity and
> triggering modes, so we have to keep the hacks as hard-coded device
> names and IRQ numbers until we switch to ACPI.
> 
> Edge-triggered IRQ still supported to keep old platforms working.
> Use platform data to pass the irq mode argument.
> 
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> Signed-off-by: Li Ning <ning.li@intel.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
...
> +++ b/include/linux/serial_max3110.h
> @@ -0,0 +1,16 @@
> +#ifndef _LINUX_SERIAL_MAX3110_H
> +#define _LINUX_SERIAL_MAX3110_H
> +
> +/**
> + * struct plat_max3110 - MAX3110 SPI UART platform data
> + * @irq_edge_trigger: if IRQ is edge triggered
> + *
> + * You should use this structure in your machine description to specify
> + * how the MAX3110 is connected.
> + *
> + */
> +struct plat_max3110 {
> +	int irq_edge_triggered;
> +};
> +
> +#endif
> -- 

Is just resource->flags for IRQ can be reused for handle such case?

---

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

On 10/22/2013 12:46 PM, Alexander Shiyan wrote:
>> The change in the max3110 driver makes the IRQ handling threaded, now
>> the handler is called only once per received character. Without that
>> change, we had many (more than 100) interrupts per one received
>> character.
>>
>> Unfortunately, SFI interface does not support IRQ polarity and
>> triggering modes, so we have to keep the hacks as hard-coded device
>> names and IRQ numbers until we switch to ACPI.
>>
>> Edge-triggered IRQ still supported to keep old platforms working.
>> Use platform data to pass the irq mode argument.
>>
>> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
>> Signed-off-by: Li Ning <ning.li@intel.com>
>> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ...
>> +++ b/include/linux/serial_max3110.h
>> @@ -0,0 +1,16 @@
>> +#ifndef _LINUX_SERIAL_MAX3110_H
>> +#define _LINUX_SERIAL_MAX3110_H
>> +
>> +/**
>> + * struct plat_max3110 - MAX3110 SPI UART platform data
>> + * @irq_edge_trigger: if IRQ is edge triggered
>> + *
>> + * You should use this structure in your machine description to specify
>> + * how the MAX3110 is connected.
>> + *
>> + */
>> +struct plat_max3110 {
>> +	int irq_edge_triggered;
>> +};
>> +
>> +#endif
>> --
>
> Is just resource->flags for IRQ can be reused for handle such case?

I believe your suggestion makes perfect sense. I'll rework it.

Br, David Cohen

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

On 10/22/2013 01:30 PM, David Cohen wrote:
> On 10/22/2013 12:46 PM, Alexander Shiyan wrote:
>>> The change in the max3110 driver makes the IRQ handling threaded, now
>>> the handler is called only once per received character. Without that
>>> change, we had many (more than 100) interrupts per one received
>>> character.
>>>
>>> Unfortunately, SFI interface does not support IRQ polarity and
>>> triggering modes, so we have to keep the hacks as hard-coded device
>>> names and IRQ numbers until we switch to ACPI.
>>>
>>> Edge-triggered IRQ still supported to keep old platforms working.
>>> Use platform data to pass the irq mode argument.
>>>
>>> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
>>> Signed-off-by: Li Ning <ning.li@intel.com>
>>> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
>> ...
>>> +++ b/include/linux/serial_max3110.h
>>> @@ -0,0 +1,16 @@
>>> +#ifndef _LINUX_SERIAL_MAX3110_H
>>> +#define _LINUX_SERIAL_MAX3110_H
>>> +
>>> +/**
>>> + * struct plat_max3110 - MAX3110 SPI UART platform data
>>> + * @irq_edge_trigger: if IRQ is edge triggered
>>> + *
>>> + * You should use this structure in your machine description to specify
>>> + * how the MAX3110 is connected.
>>> + *
>>> + */
>>> +struct plat_max3110 {
>>> +    int irq_edge_triggered;
>>> +};
>>> +
>>> +#endif
>>> --
>>
>> Is just resource->flags for IRQ can be reused for handle such case?
>
> I believe your suggestion makes perfect sense. I'll rework it.

Looks like isp_device has no place for 'resource'. In this case pdata 
seems to be the way to go here.
Or maybe there's a better way to recommend?

Br, David Cohen

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[Alexander Shiyan]

> On 10/22/2013 01:30 PM, David Cohen wrote:
> > On 10/22/2013 12:46 PM, Alexander Shiyan wrote:
> >>> The change in the max3110 driver makes the IRQ handling threaded, now
> >>> the handler is called only once per received character. Without that
> >>> change, we had many (more than 100) interrupts per one received
> >>> character.
> >>>
> >>> Unfortunately, SFI interface does not support IRQ polarity and
> >>> triggering modes, so we have to keep the hacks as hard-coded device
> >>> names and IRQ numbers until we switch to ACPI.
> >>>
> >>> Edge-triggered IRQ still supported to keep old platforms working.
> >>> Use platform data to pass the irq mode argument.
> >>>
> >>> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> >>> Signed-off-by: Li Ning <ning.li@intel.com>
> >>> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> >> ...
> >>> +++ b/include/linux/serial_max3110.h
> >>> @@ -0,0 +1,16 @@
> >>> +#ifndef _LINUX_SERIAL_MAX3110_H
> >>> +#define _LINUX_SERIAL_MAX3110_H
> >>> +
> >>> +/**
> >>> + * struct plat_max3110 - MAX3110 SPI UART platform data
> >>> + * @irq_edge_trigger: if IRQ is edge triggered
> >>> + *
> >>> + * You should use this structure in your machine description to specify
> >>> + * how the MAX3110 is connected.
> >>> + *
> >>> + */
> >>> +struct plat_max3110 {
> >>> +    int irq_edge_triggered;
> >>> +};
> >>> +
> >>> +#endif
> >>> --
> >>
> >> Is just resource->flags for IRQ can be reused for handle such case?
> >
> > I believe your suggestion makes perfect sense. I'll rework it.
> 
> Looks like isp_device has no place for 'resource'. In this case pdata 
> seems to be the way to go here.
> Or maybe there's a better way to recommend?

What do you mean by the isp_device?

My idea is always use threaded irq and passing flags into request.
Like as:
unsigned long flags = res->flags & IORESOURCE_BITS;
...
request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);

---

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

On 10/22/2013 11:27 PM, Alexander Shiyan wrote:
>> On 10/22/2013 01:30 PM, David Cohen wrote:
>>> On 10/22/2013 12:46 PM, Alexander Shiyan wrote:
>>>>> The change in the max3110 driver makes the IRQ handling threaded, now
>>>>> the handler is called only once per received character. Without that
>>>>> change, we had many (more than 100) interrupts per one received
>>>>> character.
>>>>>
>>>>> Unfortunately, SFI interface does not support IRQ polarity and
>>>>> triggering modes, so we have to keep the hacks as hard-coded device
>>>>> names and IRQ numbers until we switch to ACPI.
>>>>>
>>>>> Edge-triggered IRQ still supported to keep old platforms working.
>>>>> Use platform data to pass the irq mode argument.
>>>>>
>>>>> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
>>>>> Signed-off-by: Li Ning <ning.li@intel.com>
>>>>> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
>>>> ...
>>>>> +++ b/include/linux/serial_max3110.h
>>>>> @@ -0,0 +1,16 @@
>>>>> +#ifndef _LINUX_SERIAL_MAX3110_H
>>>>> +#define _LINUX_SERIAL_MAX3110_H
>>>>> +
>>>>> +/**
>>>>> + * struct plat_max3110 - MAX3110 SPI UART platform data
>>>>> + * @irq_edge_trigger: if IRQ is edge triggered
>>>>> + *
>>>>> + * You should use this structure in your machine description to specify
>>>>> + * how the MAX3110 is connected.
>>>>> + *
>>>>> + */
>>>>> +struct plat_max3110 {
>>>>> +    int irq_edge_triggered;
>>>>> +};
>>>>> +
>>>>> +#endif
>>>>> --
>>>>
>>>> Is just resource->flags for IRQ can be reused for handle such case?
>>>
>>> I believe your suggestion makes perfect sense. I'll rework it.
>>
>> Looks like isp_device has no place for 'resource'. In this case pdata
>> seems to be the way to go here.
>> Or maybe there's a better way to recommend?
>
> What do you mean by the isp_device?

Allow me to correct my typo: I meant spi_device (this is a spi
device/driver).

>
> My idea is always use threaded irq and passing flags into request.
> Like as:
> unsigned long flags = res->flags & IORESOURCE_BITS;
> ...
> request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);


Oh, maybe we were talking about different things afterall :)
The reason this struct plat_max3110 was created is to allow platform
code (located under arch/x86/platform/intel-mid/device_libs/) to define
the irq edge type.
When I saw your comment I though you were referring to struct resource
(which has IORESOURCE_IRQ_* flags). But unlike platform_device,
spi_device has no struct resource * to replace the need of struct
plat_max3110.

OTOH your suggestion can replace this piece of code:

@@ -68,6 +69,7 @@ struct uart_max3110 {
         u8 clock;
         u8 parity, word_7bits;
         u16 irq;
+       u16 irq_edge_triggered;

         unsigned long uart_flags;

I'll check again the patch and possibly make the changes.

Thanks,

David Cohen

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Hi,

On Wed, Oct 23, 2013 at 11:10:48AM -0700, David Cohen wrote:
> >My idea is always use threaded irq and passing flags into request.
> >Like as:
> >unsigned long flags = res->flags & IORESOURCE_BITS;
> >...
> >request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);
> 
> 
> Oh, maybe we were talking about different things afterall :)
> The reason this struct plat_max3110 was created is to allow platform
> code (located under arch/x86/platform/intel-mid/device_libs/) to define
> the irq edge type.
> When I saw your comment I though you were referring to struct resource
> (which has IORESOURCE_IRQ_* flags). But unlike platform_device,
> spi_device has no struct resource * to replace the need of struct
> plat_max3110.
> 
> OTOH your suggestion can replace this piece of code:
> 
> @@ -68,6 +69,7 @@ struct uart_max3110 {
>         u8 clock;
>         u8 parity, word_7bits;
>         u16 irq;
> +       u16 irq_edge_triggered;

max3110 is already edge triggered:

495                 ret = request_irq(max->irq, serial_m3110_irq,
496                                 IRQ_TYPE_EDGE_FALLING, "max3110", max);

it would be nice a threaded IRQ instead of using a singlethread
workqueue, though.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

Hi again,

On Wed, Oct 23, 2013 at 01:21:43PM -0500, Felipe Balbi wrote:
> On Wed, Oct 23, 2013 at 11:10:48AM -0700, David Cohen wrote:
> > >My idea is always use threaded irq and passing flags into request.
> > >Like as:
> > >unsigned long flags = res->flags & IORESOURCE_BITS;
> > >...
> > >request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);
> > 
> > 
> > Oh, maybe we were talking about different things afterall :)
> > The reason this struct plat_max3110 was created is to allow platform
> > code (located under arch/x86/platform/intel-mid/device_libs/) to define
> > the irq edge type.
> > When I saw your comment I though you were referring to struct resource
> > (which has IORESOURCE_IRQ_* flags). But unlike platform_device,
> > spi_device has no struct resource * to replace the need of struct
> > plat_max3110.
> > 
> > OTOH your suggestion can replace this piece of code:
> > 
> > @@ -68,6 +69,7 @@ struct uart_max3110 {
> >         u8 clock;
> >         u8 parity, word_7bits;
> >         u16 irq;
> > +       u16 irq_edge_triggered;
> 
> max3110 is already edge triggered:
> 
> 495                 ret = request_irq(max->irq, serial_m3110_irq,
> 496                                 IRQ_TYPE_EDGE_FALLING, "max3110", max);
> 
> it would be nice a threaded IRQ instead of using a singlethread
> workqueue, though.

making it clearer, you only converted to threaded IRQ when
edge_triggered isn't set, why don't you *always* use threaded IRQs ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

On 10/23/2013 11:23 AM, Felipe Balbi wrote:
> Hi again,
>
> On Wed, Oct 23, 2013 at 01:21:43PM -0500, Felipe Balbi wrote:
>> On Wed, Oct 23, 2013 at 11:10:48AM -0700, David Cohen wrote:
>>>> My idea is always use threaded irq and passing flags into request.
>>>> Like as:
>>>> unsigned long flags = res->flags & IORESOURCE_BITS;
>>>> ...
>>>> request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);
>>>
>>>
>>> Oh, maybe we were talking about different things afterall :)
>>> The reason this struct plat_max3110 was created is to allow platform
>>> code (located under arch/x86/platform/intel-mid/device_libs/) to define
>>> the irq edge type.
>>> When I saw your comment I though you were referring to struct resource
>>> (which has IORESOURCE_IRQ_* flags). But unlike platform_device,
>>> spi_device has no struct resource * to replace the need of struct
>>> plat_max3110.
>>>
>>> OTOH your suggestion can replace this piece of code:
>>>
>>> @@ -68,6 +69,7 @@ struct uart_max3110 {
>>>          u8 clock;
>>>          u8 parity, word_7bits;
>>>          u16 irq;
>>> +       u16 irq_edge_triggered;
>>
>> max3110 is already edge triggered:
>>
>> 495                 ret = request_irq(max->irq, serial_m3110_irq,
>> 496                                 IRQ_TYPE_EDGE_FALLING, "max3110", max);
>>
>> it would be nice a threaded IRQ instead of using a singlethread
>> workqueue, though.
>
> making it clearer, you only converted to threaded IRQ when
> edge_triggered isn't set, why don't you *always* use threaded IRQs ?

This is a very difficult question to answer, considering the
non-threaded irq is used just to wake up a wq :)
I wish this driver was better maintained.

I'll rework this whole IRQ scheme here.

Br, David Cohen

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[David Cohen]

On 10/23/2013 11:21 AM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Oct 23, 2013 at 11:10:48AM -0700, David Cohen wrote:
>>> My idea is always use threaded irq and passing flags into request.
>>> Like as:
>>> unsigned long flags = res->flags & IORESOURCE_BITS;
>>> ...
>>> request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);
>>
>>
>> Oh, maybe we were talking about different things afterall :)
>> The reason this struct plat_max3110 was created is to allow platform
>> code (located under arch/x86/platform/intel-mid/device_libs/) to define
>> the irq edge type.
>> When I saw your comment I though you were referring to struct resource
>> (which has IORESOURCE_IRQ_* flags). But unlike platform_device,
>> spi_device has no struct resource * to replace the need of struct
>> plat_max3110.
>>
>> OTOH your suggestion can replace this piece of code:
>>
>> @@ -68,6 +69,7 @@ struct uart_max3110 {
>>          u8 clock;
>>          u8 parity, word_7bits;
>>          u16 irq;
>> +       u16 irq_edge_triggered;
>
> max3110 is already edge triggered:
>
> 495                 ret = request_irq(max->irq, serial_m3110_irq,
> 496                                 IRQ_TYPE_EDGE_FALLING, "max3110", max);

Yeah. But in Merrifield case (at least the reference board used now) 
it's not edge triggered. I need this driver to support this situation
prior to send mrst_max3110 platform code for it.

>
> it would be nice a threaded IRQ instead of using a singlethread
> workqueue, though.
>

That sounds reasonable. I'll add this to my TODO list too. Thanks.

Br, David Cohen

Re: [PATCH 2/2] mrst_max3110: fix SPI UART interrupt parameters

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

Hi,

On Wed, Oct 23, 2013 at 11:33:45AM -0700, David Cohen wrote:
> On 10/23/2013 11:21 AM, Felipe Balbi wrote:
> >Hi,
> >
> >On Wed, Oct 23, 2013 at 11:10:48AM -0700, David Cohen wrote:
> >>>My idea is always use threaded irq and passing flags into request.
> >>>Like as:
> >>>unsigned long flags = res->flags & IORESOURCE_BITS;
> >>>...
> >>>request_threaded_irq(max->irq, serial_m3110_irq, IRQF_ONESHOT | flags, "max3110", max);
> >>
> >>
> >>Oh, maybe we were talking about different things afterall :)
> >>The reason this struct plat_max3110 was created is to allow platform
> >>code (located under arch/x86/platform/intel-mid/device_libs/) to define
> >>the irq edge type.
> >>When I saw your comment I though you were referring to struct resource
> >>(which has IORESOURCE_IRQ_* flags). But unlike platform_device,
> >>spi_device has no struct resource * to replace the need of struct
> >>plat_max3110.
> >>
> >>OTOH your suggestion can replace this piece of code:
> >>
> >>@@ -68,6 +69,7 @@ struct uart_max3110 {
> >>         u8 clock;
> >>         u8 parity, word_7bits;
> >>         u16 irq;
> >>+       u16 irq_edge_triggered;
> >
> >max3110 is already edge triggered:
> >
> >495                 ret = request_irq(max->irq, serial_m3110_irq,
> >496                                 IRQ_TYPE_EDGE_FALLING, "max3110", max);
> 
> Yeah. But in Merrifield case (at least the reference board used now)
> it's not edge triggered. I need this driver to support this situation
> prior to send mrst_max3110 platform code for it.

got it. Then why don't you use irq_set_irq_type() from platform code and
change that request_irq() into:

ret = request_threaded_irq(max->irq, NULL, serial_m3110_irq,
	IRQF_ONESHOT, "max3110", max);

if you call irq_set_irq_type() correctly, driver wouldn't need to know
the IRQ type.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]