[PATCH 0/8] ir-rx51: Fixes in response to review comments

[PATCH 0/8] ir-rx51: Fixes in response to review comments

[Timo Kokkonen]

These patches fix most of the issues pointed out in the patch review
by Sean Young and Sakari Ailus. The most noticeable change after these
patch set is that the IR transmission no longer times out even if the
timers are not waking up the MPU as it should be. However, the
transmission itself is still as badly mangled as before, unless there
is some background load preventing the MPU from going into
sleep. Otherwise the patches are mostly clean ups and rather trivial
stuff.

All comments are welcome. Thanks!

Timo Kokkonen (8):
  ir-rx51: Adjust dependencies
  ir-rx51: Handle signals properly
  ir-rx51: Trivial fixes
  ir-rx51: Clean up timer initialization code
  ir-rx51: Move platform data checking into probe function
  ir-rx51: Replace module_{init,exit} macros with
    module_platform_driver
  ir-rx51: Remove MPU wakeup latency adjustments
  ir-rx51: Remove useless variable from struct lirc_rx51

 arch/arm/mach-omap2/board-rx51-peripherals.c |  2 -
 drivers/media/rc/Kconfig                     |  4 +-
 drivers/media/rc/ir-rx51.c                   | 92 +++++++++++++---------------
 include/media/ir-rx51.h                      |  2 -
 4 files changed, 43 insertions(+), 57 deletions(-)

-- 
1.7.12

[PATCH 1/8] ir-rx51: Adjust dependencies

[Timo Kokkonen]

Although this kind of IR diode circuitry is known to exist only in
N900 hardware, nothing prevents making similar circuitry on any OMAP
based board. The MACH_NOKIA_RX51 dependency is thus not something we
want to be there.

Also, this should depend on LIRC as it is a LIRC driver.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index ffef8b4..093982b 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -273,7 +273,7 @@ config IR_IGUANA
 
 config IR_RX51
 	tristate "Nokia N900 IR transmitter diode
-	depends on MACH_NOKIA_RX51 && OMAP_DM_TIMER
+	depends on OMAP_DM_TIMER && LIRC
 	---help---
 	   Say Y or M here if you want to enable support for the IR
 	   transmitter diode built in the Nokia N900 (RX51) device.
-- 
1.7.12

[PATCH 2/8] ir-rx51: Handle signals properly

[Timo Kokkonen]

The lirc-dev expects the ir-code to be transmitted when the write call
returns back to the user space. We should not leave TX ongoing no
matter what is the reason we return to the user space. Easiest
solution for that is to simply remove interruptible sleeps.

The first wait_event_interruptible is thus replaced with return -EBUSY
in case there is still ongoing transfer. This should suffice as the
concept of sending multiple codes in parallel does not make sense.

The second wait_event_interruptible call is replaced with
wait_even_timeout with a fixed and safe timeout that should prevent
the process from getting stuck in kernel for too long.

Also, from now on we will force the TX to stop before we return from
write call. If the TX happened to time out for some reason, we should
not leave the HW transmitting anything.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 9487dd3..a7b787a 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -74,6 +74,19 @@ static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51)
 			      OMAP_TIMER_TRIGGER_NONE);
 }
 
+static void lirc_rx51_stop_tx(struct lirc_rx51 *lirc_rx51)
+{
+	if (lirc_rx51->wbuf_index < 0)
+		return;
+
+	lirc_rx51_off(lirc_rx51);
+	lirc_rx51->wbuf_index = -1;
+	omap_dm_timer_stop(lirc_rx51->pwm_timer);
+	omap_dm_timer_stop(lirc_rx51->pulse_timer);
+	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
+	wake_up_interruptible(&lirc_rx51->wqueue);
+}
+
 static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 {
 	u32 load, match;
@@ -160,13 +173,7 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
 
 	return IRQ_HANDLED;
 end:
-	/* Stop TX here */
-	lirc_rx51_off(lirc_rx51);
-	lirc_rx51->wbuf_index = -1;
-	omap_dm_timer_stop(lirc_rx51->pwm_timer);
-	omap_dm_timer_stop(lirc_rx51->pulse_timer);
-	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
-	wake_up_interruptible(&lirc_rx51->wqueue);
+	lirc_rx51_stop_tx(lirc_rx51);
 
 	return IRQ_HANDLED;
 }
@@ -246,8 +253,9 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 	if ((count > WBUF_LEN) || (count % 2 == 0))
 		return -EINVAL;
 
-	/* Wait any pending transfers to finish */
-	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
+	/* We can have only one transmit at a time */
+	if (lirc_rx51->wbuf_index >= 0)
+		return -EBUSY;
 
 	if (copy_from_user(lirc_rx51->wbuf, buf, n))
 		return -EFAULT;
@@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 
 	/*
 	 * Don't return back to the userspace until the transfer has
-	 * finished
+	 * finished. However, we wish to not spend any more than 500ms
+	 * in kernel. No IR code TX should ever take that long.
+	 */
+	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
+			HZ / 2);
+
+	/*
+	 * Ensure transmitting has really stopped, even if the timers
+	 * went mad or something else happened that caused it still
+	 * sending out something.
 	 */
-	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
+	lirc_rx51_stop_tx(lirc_rx51);
 
 	/* We can sleep again */
 	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
-- 
1.7.12

Re: [PATCH 2/8] ir-rx51: Handle signals properly

[Sean Young]

On Wed, Aug 22, 2012 at 10:50:35PM +0300, Timo Kokkonen wrote:
> The lirc-dev expects the ir-code to be transmitted when the write call
> returns back to the user space. We should not leave TX ongoing no
> matter what is the reason we return to the user space. Easiest
> solution for that is to simply remove interruptible sleeps.
> 
> The first wait_event_interruptible is thus replaced with return -EBUSY
> in case there is still ongoing transfer. This should suffice as the
> concept of sending multiple codes in parallel does not make sense.
> 
> The second wait_event_interruptible call is replaced with
> wait_even_timeout with a fixed and safe timeout that should prevent
> the process from getting stuck in kernel for too long.
> 
> Also, from now on we will force the TX to stop before we return from
> write call. If the TX happened to time out for some reason, we should
> not leave the HW transmitting anything.
> 
> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
> ---
>  drivers/media/rc/ir-rx51.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> index 9487dd3..a7b787a 100644
> --- a/drivers/media/rc/ir-rx51.c
> +++ b/drivers/media/rc/ir-rx51.c
> @@ -74,6 +74,19 @@ static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51)
>  			      OMAP_TIMER_TRIGGER_NONE);
>  }
>  
> +static void lirc_rx51_stop_tx(struct lirc_rx51 *lirc_rx51)
> +{
> +	if (lirc_rx51->wbuf_index < 0)
> +		return;
> +
> +	lirc_rx51_off(lirc_rx51);
> +	lirc_rx51->wbuf_index = -1;
> +	omap_dm_timer_stop(lirc_rx51->pwm_timer);
> +	omap_dm_timer_stop(lirc_rx51->pulse_timer);
> +	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
> +	wake_up_interruptible(&lirc_rx51->wqueue);

Unless I'm mistaken, wait_up_interruptable() won't wake up any 
non-interruptable sleepers. Should this not be wake_up()?

> +}
> +
>  static int init_timing_params(struct lirc_rx51 *lirc_rx51)
>  {
>  	u32 load, match;
> @@ -160,13 +173,7 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
>  
>  	return IRQ_HANDLED;
>  end:
> -	/* Stop TX here */
> -	lirc_rx51_off(lirc_rx51);
> -	lirc_rx51->wbuf_index = -1;
> -	omap_dm_timer_stop(lirc_rx51->pwm_timer);
> -	omap_dm_timer_stop(lirc_rx51->pulse_timer);
> -	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
> -	wake_up_interruptible(&lirc_rx51->wqueue);
> +	lirc_rx51_stop_tx(lirc_rx51);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -246,8 +253,9 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>  	if ((count > WBUF_LEN) || (count % 2 == 0))
>  		return -EINVAL;
>  
> -	/* Wait any pending transfers to finish */
> -	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
> +	/* We can have only one transmit at a time */
> +	if (lirc_rx51->wbuf_index >= 0)
> +		return -EBUSY;
>  
>  	if (copy_from_user(lirc_rx51->wbuf, buf, n))
>  		return -EFAULT;
> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>  
>  	/*
>  	 * Don't return back to the userspace until the transfer has
> -	 * finished
> +	 * finished. However, we wish to not spend any more than 500ms
> +	 * in kernel. No IR code TX should ever take that long.
> +	 */
> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,

Note non-interruptable sleeper.

> +			HZ / 2);
> +
> +	/*
> +	 * Ensure transmitting has really stopped, even if the timers
> +	 * went mad or something else happened that caused it still
> +	 * sending out something.
>  	 */
> -	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
> +	lirc_rx51_stop_tx(lirc_rx51);
>  
>  	/* We can sleep again */
>  	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
> -- 
> 1.7.12
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] ir-rx51: Handle signals properly

[Timo Kokkonen]

On 08/24/12 13:03, Sean Young wrote:
> On Wed, Aug 22, 2012 at 10:50:35PM +0300, Timo Kokkonen wrote:
>> The lirc-dev expects the ir-code to be transmitted when the write call
>> returns back to the user space. We should not leave TX ongoing no
>> matter what is the reason we return to the user space. Easiest
>> solution for that is to simply remove interruptible sleeps.
>>
>> The first wait_event_interruptible is thus replaced with return -EBUSY
>> in case there is still ongoing transfer. This should suffice as the
>> concept of sending multiple codes in parallel does not make sense.
>>
>> The second wait_event_interruptible call is replaced with
>> wait_even_timeout with a fixed and safe timeout that should prevent
>> the process from getting stuck in kernel for too long.
>>
>> Also, from now on we will force the TX to stop before we return from
>> write call. If the TX happened to time out for some reason, we should
>> not leave the HW transmitting anything.
>>
>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
>> ---
>>  drivers/media/rc/ir-rx51.c | 39 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
>> index 9487dd3..a7b787a 100644
>> --- a/drivers/media/rc/ir-rx51.c
>> +++ b/drivers/media/rc/ir-rx51.c
>> @@ -74,6 +74,19 @@ static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51)
>>  			      OMAP_TIMER_TRIGGER_NONE);
>>  }
>>  
>> +static void lirc_rx51_stop_tx(struct lirc_rx51 *lirc_rx51)
>> +{
>> +	if (lirc_rx51->wbuf_index < 0)
>> +		return;
>> +
>> +	lirc_rx51_off(lirc_rx51);
>> +	lirc_rx51->wbuf_index = -1;
>> +	omap_dm_timer_stop(lirc_rx51->pwm_timer);
>> +	omap_dm_timer_stop(lirc_rx51->pulse_timer);
>> +	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
>> +	wake_up_interruptible(&lirc_rx51->wqueue);
> 
> Unless I'm mistaken, wait_up_interruptable() won't wake up any 
> non-interruptable sleepers. Should this not be wake_up()?
> 

Thanks for pointing out that. I guess I never noticed that because I'm
using a timeout with the wait, so it will wake up anyway. I'll fix it.

-Timo

>> +}
>> +
>>  static int init_timing_params(struct lirc_rx51 *lirc_rx51)
>>  {
>>  	u32 load, match;
>> @@ -160,13 +173,7 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
>>  
>>  	return IRQ_HANDLED;
>>  end:
>> -	/* Stop TX here */
>> -	lirc_rx51_off(lirc_rx51);
>> -	lirc_rx51->wbuf_index = -1;
>> -	omap_dm_timer_stop(lirc_rx51->pwm_timer);
>> -	omap_dm_timer_stop(lirc_rx51->pulse_timer);
>> -	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
>> -	wake_up_interruptible(&lirc_rx51->wqueue);
>> +	lirc_rx51_stop_tx(lirc_rx51);
>>  
>>  	return IRQ_HANDLED;
>>  }
>> @@ -246,8 +253,9 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>  	if ((count > WBUF_LEN) || (count % 2 == 0))
>>  		return -EINVAL;
>>  
>> -	/* Wait any pending transfers to finish */
>> -	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
>> +	/* We can have only one transmit at a time */
>> +	if (lirc_rx51->wbuf_index >= 0)
>> +		return -EBUSY;
>>  
>>  	if (copy_from_user(lirc_rx51->wbuf, buf, n))
>>  		return -EFAULT;
>> @@ -273,9 +281,18 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>  
>>  	/*
>>  	 * Don't return back to the userspace until the transfer has
>> -	 * finished
>> +	 * finished. However, we wish to not spend any more than 500ms
>> +	 * in kernel. No IR code TX should ever take that long.
>> +	 */
>> +	i = wait_event_timeout(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0,
> 
> Note non-interruptable sleeper.
> 
>> +			HZ / 2);
>> +
>> +	/*
>> +	 * Ensure transmitting has really stopped, even if the timers
>> +	 * went mad or something else happened that caused it still
>> +	 * sending out something.
>>  	 */
>> -	wait_event_interruptible(lirc_rx51->wqueue, lirc_rx51->wbuf_index < 0);
>> +	lirc_rx51_stop_tx(lirc_rx51);
>>  
>>  	/* We can sleep again */
>>  	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
>> -- 
>> 1.7.12
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/8] ir-rx51: Trivial fixes

[Timo Kokkonen]

-Fix typo

-Change pwm_timer_num type to match type in platform data

-Remove extra parenthesis

-Replace magic constant with proper bit defintions

-Remove duplicate exit pointer

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>

trivial

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/Kconfig   |  2 +-
 drivers/media/rc/ir-rx51.c | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index 093982b..4a68014 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -278,7 +278,7 @@ config IR_RX51
 	   Say Y or M here if you want to enable support for the IR
 	   transmitter diode built in the Nokia N900 (RX51) device.
 
-	   The driver uses omap DM timers for gereating the carrier
+	   The driver uses omap DM timers for generating the carrier
 	   wave and pulses.
 
 config RC_LOOPBACK
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index a7b787a..468c8a1 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -59,7 +59,7 @@ struct lirc_rx51 {
 	int		wbuf[WBUF_LEN];
 	int		wbuf_index;
 	unsigned long	device_is_open;
-	unsigned int	pwm_timer_num;
+	int		pwm_timer_num;
 };
 
 static void lirc_rx51_on(struct lirc_rx51 *lirc_rx51)
@@ -138,11 +138,14 @@ static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
 	if (!retval)
 		return IRQ_NONE;
 
-	if ((retval & ~OMAP_TIMER_INT_MATCH))
+	if (retval & ~OMAP_TIMER_INT_MATCH)
 		dev_err_ratelimited(lirc_rx51->dev,
 				": Unexpected interrupt source: %x\n", retval);
 
-	omap_dm_timer_write_status(lirc_rx51->pulse_timer, 7);
+	omap_dm_timer_write_status(lirc_rx51->pulse_timer,
+				OMAP_TIMER_INT_MATCH	|
+				OMAP_TIMER_INT_OVERFLOW	|
+				OMAP_TIMER_INT_CAPTURE);
 	if (lirc_rx51->wbuf_index < 0) {
 		dev_err_ratelimited(lirc_rx51->dev,
 				": BUG wbuf_index has value of %i\n",
@@ -489,7 +492,6 @@ struct platform_driver lirc_rx51_platform_driver = {
 	.remove		= __exit_p(lirc_rx51_remove),
 	.suspend	= lirc_rx51_suspend,
 	.resume		= lirc_rx51_resume,
-	.remove		= __exit_p(lirc_rx51_remove),
 	.driver		= {
 		.name	= DRIVER_NAME,
 		.owner	= THIS_MODULE,
-- 
1.7.12

[PATCH 4/8] ir-rx51: Clean up timer initialization code

[Timo Kokkonen]

Remove a redundant macro definition. This is unneeded and becomes more
readable once the actual timer code is refactored a little.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 468c8a1..3d2911b 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -105,11 +105,9 @@ static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 	return 0;
 }
 
-#define tics_after(a, b) ((long)(b) - (long)(a) < 0)
-
 static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 {
-	int counter;
+	int counter, counter_now;
 
 	BUG_ON(usec < 0);
 
@@ -122,11 +120,8 @@ static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 	omap_dm_timer_set_match(lirc_rx51->pulse_timer, 1, counter);
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer,
 				     OMAP_TIMER_INT_MATCH);
-	if (tics_after(omap_dm_timer_read_counter(lirc_rx51->pulse_timer),
-		       counter)) {
-		return 1;
-	}
-	return 0;
+	counter_now = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
+	return (counter - counter_now) < 0;
 }
 
 static irqreturn_t lirc_rx51_interrupt_handler(int irq, void *ptr)
-- 
1.7.12

[PATCH 5/8] ir-rx51: Move platform data checking into probe function

[Timo Kokkonen]

This driver is useless without proper platform data. If data is not
available, we should not register the driver at all. Once this check
is done, the BUG_ON check during device open is no longer needed.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 3d2911b..46628c0 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -378,7 +378,6 @@ static long lirc_rx51_ioctl(struct file *filep,
 static int lirc_rx51_open(struct inode *inode, struct file *file)
 {
 	struct lirc_rx51 *lirc_rx51 = lirc_get_pdata(file);
-	BUG_ON(!lirc_rx51);
 
 	file->private_data = lirc_rx51;
 
@@ -458,6 +457,9 @@ static int lirc_rx51_resume(struct platform_device *dev)
 
 static int __devinit lirc_rx51_probe(struct platform_device *dev)
 {
+	if (!dev->dev.platform_data)
+		return -ENODEV;
+
 	lirc_rx51_driver.features = LIRC_RX51_DRIVER_FEATURES;
 	lirc_rx51.pdata = dev->dev.platform_data;
 	lirc_rx51.pwm_timer_num = lirc_rx51.pdata->pwm_timer;
-- 
1.7.12

[PATCH 6/8] ir-rx51: Replace module_{init,exit} macros with module_platform_driver

[Timo Kokkonen]

No reason to avoid using the existing helpers.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 46628c0..7eed541 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -495,17 +495,7 @@ struct platform_driver lirc_rx51_platform_driver = {
 	},
 };
 
-static int __init lirc_rx51_init(void)
-{
-	return platform_driver_register(&lirc_rx51_platform_driver);
-}
-module_init(lirc_rx51_init);
-
-static void __exit lirc_rx51_exit(void)
-{
-	platform_driver_unregister(&lirc_rx51_platform_driver);
-}
-module_exit(lirc_rx51_exit);
+module_platform_driver(lirc_rx51_platform_driver);
 
 MODULE_DESCRIPTION("LIRC TX driver for Nokia RX51");
 MODULE_AUTHOR("Nokia Corporation");
-- 
1.7.12

[PATCH 7/8] ir-rx51: Remove MPU wakeup latency adjustments

[Timo Kokkonen]

The ir-rx51 driver calls omap_pm_set_max_mpu_wakeup_lat() in order to
avoid problems that occur when MPU goes to sleep in the middle of
sending an IR code. Without such calls it takes ridiculously long for
the MPU to wake up from a sleep, which distorts the IR signal
completely.

However, the actual problem is that probably the GP timers are not
able to wake up the MPU at all. That is, adjusting the latency
requirements is not the correct way to solve the issue either. The
reason why this used to work with the original 2.6.28 based N900
kernel that is shipped with the product is that placing strict latency
requirements prevents the MPU from going to sleep at all. Furthermore,
the only PM layer imlementation available at the moment for OMAP3
doesn't do anything with the latency requirement placed with
omap_pm_set_max_mpu_wakeup_lat() calls.

A more appropriate fix for the problem would be to modify the idle
layer so that it does not allow MPU going to too deep sleep modes when
it is expected that the timers need to wake up MPU.

Therefore, it makes sense to actually remove this call entirely from
the ir-rx51 driver as it is both wrong and does nothing useful at the
moment.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c | 2 --
 drivers/media/rc/ir-rx51.c                   | 9 ---------
 include/media/ir-rx51.h                      | 2 --
 3 files changed, 13 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index ca07264..e0750cb 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -34,7 +34,6 @@
 #include <plat/gpmc.h>
 #include <plat/onenand.h>
 #include <plat/gpmc-smc91x.h>
-#include <plat/omap-pm.h>
 
 #include <mach/board-rx51.h>
 
@@ -1227,7 +1226,6 @@ static void __init rx51_init_tsc2005(void)
 
 #if defined(CONFIG_IR_RX51) || defined(CONFIG_IR_RX51_MODULE)
 static struct lirc_rx51_platform_data rx51_lirc_data = {
-	.set_max_mpu_wakeup_lat = omap_pm_set_max_mpu_wakeup_lat,
 	.pwm_timer = 9, /* Use GPT 9 for CIR */
 };
 
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 7eed541..ac7d3f0 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -267,12 +267,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 	if (count < WBUF_LEN)
 		lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
 
-	/*
-	 * Adjust latency requirements so the device doesn't go in too
-	 * deep sleep states
-	 */
-	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
-
 	lirc_rx51_on(lirc_rx51);
 	lirc_rx51->wbuf_index = 1;
 	pulse_timer_set_timeout(lirc_rx51, lirc_rx51->wbuf[0]);
@@ -292,9 +286,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 	 */
 	lirc_rx51_stop_tx(lirc_rx51);
 
-	/* We can sleep again */
-	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
-
 	return n;
 }
 
diff --git a/include/media/ir-rx51.h b/include/media/ir-rx51.h
index 104aa89..57523f2 100644
--- a/include/media/ir-rx51.h
+++ b/include/media/ir-rx51.h
@@ -3,8 +3,6 @@
 
 struct lirc_rx51_platform_data {
 	int pwm_timer;
-
-	int(*set_max_mpu_wakeup_lat)(struct device *dev, long t);
 };
 
 #endif
-- 
1.7.12

Re: [PATCH 7/8] ir-rx51: Remove MPU wakeup latency adjustments

[Jean Pihet]

Hi Timo,

On Wed, Aug 22, 2012 at 9:50 PM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
> The ir-rx51 driver calls omap_pm_set_max_mpu_wakeup_lat() in order to
> avoid problems that occur when MPU goes to sleep in the middle of
> sending an IR code. Without such calls it takes ridiculously long for
> the MPU to wake up from a sleep, which distorts the IR signal
> completely.
>
> However, the actual problem is that probably the GP timers are not
> able to wake up the MPU at all. That is, adjusting the latency
> requirements is not the correct way to solve the issue either. The
> reason why this used to work with the original 2.6.28 based N900
> kernel that is shipped with the product is that placing strict latency
> requirements prevents the MPU from going to sleep at all. Furthermore,
> the only PM layer imlementation available at the moment for OMAP3
> doesn't do anything with the latency requirement placed with
> omap_pm_set_max_mpu_wakeup_lat() calls.
That is correct. The API to use is the PM QoS API which cpuidle uses
to determine the next MPU state based on the allowed latency.

> A more appropriate fix for the problem would be to modify the idle
> layer so that it does not allow MPU going to too deep sleep modes when
> it is expected that the timers need to wake up MPU.
The idle layer already uses the PM QoS framework to decide the next
MPU state. I think the right solution is to convert from
omap_pm_set_max_mpu_wakeup_lat to the PM QoS API.

Cf. http://marc.info/?l=linux-omap&m=133968658305580&w=2 for an
example of the conversion.

> Therefore, it makes sense to actually remove this call entirely from
> the ir-rx51 driver as it is both wrong and does nothing useful at the
> moment.
>
> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>

Regards,
Jean

> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c | 2 --
>  drivers/media/rc/ir-rx51.c                   | 9 ---------
>  include/media/ir-rx51.h                      | 2 --
>  3 files changed, 13 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index ca07264..e0750cb 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> @@ -34,7 +34,6 @@
>  #include <plat/gpmc.h>
>  #include <plat/onenand.h>
>  #include <plat/gpmc-smc91x.h>
> -#include <plat/omap-pm.h>
>
>  #include <mach/board-rx51.h>
>
> @@ -1227,7 +1226,6 @@ static void __init rx51_init_tsc2005(void)
>
>  #if defined(CONFIG_IR_RX51) || defined(CONFIG_IR_RX51_MODULE)
>  static struct lirc_rx51_platform_data rx51_lirc_data = {
> -       .set_max_mpu_wakeup_lat = omap_pm_set_max_mpu_wakeup_lat,
>         .pwm_timer = 9, /* Use GPT 9 for CIR */
>  };
>
> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
> index 7eed541..ac7d3f0 100644
> --- a/drivers/media/rc/ir-rx51.c
> +++ b/drivers/media/rc/ir-rx51.c
> @@ -267,12 +267,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>         if (count < WBUF_LEN)
>                 lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
>
> -       /*
> -        * Adjust latency requirements so the device doesn't go in too
> -        * deep sleep states
> -        */
> -       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
> -
>         lirc_rx51_on(lirc_rx51);
>         lirc_rx51->wbuf_index = 1;
>         pulse_timer_set_timeout(lirc_rx51, lirc_rx51->wbuf[0]);
> @@ -292,9 +286,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>          */
>         lirc_rx51_stop_tx(lirc_rx51);
>
> -       /* We can sleep again */
> -       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
> -
>         return n;
>  }
>
> diff --git a/include/media/ir-rx51.h b/include/media/ir-rx51.h
> index 104aa89..57523f2 100644
> --- a/include/media/ir-rx51.h
> +++ b/include/media/ir-rx51.h
> @@ -3,8 +3,6 @@
>
>  struct lirc_rx51_platform_data {
>         int pwm_timer;
> -
> -       int(*set_max_mpu_wakeup_lat)(struct device *dev, long t);
>  };
>
>  #endif
> --
> 1.7.12
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] ir-rx51: Remove MPU wakeup latency adjustments

[Timo Kokkonen]

Hi Jean,

On 08/23/12 14:58, Jean Pihet wrote:
> Hi Timo,
> 
> On Wed, Aug 22, 2012 at 9:50 PM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
> That is correct. The API to use is the PM QoS API which cpuidle uses
> to determine the next MPU state based on the allowed latency.
> 
>> A more appropriate fix for the problem would be to modify the idle
>> layer so that it does not allow MPU going to too deep sleep modes when
>> it is expected that the timers need to wake up MPU.
> The idle layer already uses the PM QoS framework to decide the next
> MPU state. I think the right solution is to convert from
> omap_pm_set_max_mpu_wakeup_lat to the PM QoS API.
> 
> Cf. http://marc.info/?l=linux-omap&m=133968658305580&w=2 for an
> example of the conversion.
> 

Thanks. It looks like really easy and straightforward conversion.
However, I couldn't find the patch you were referring to from any trees
I could find. So, I take that this API does not really have omap2
support in it yet? I tried git grepping through the source and to me it
appears there is nothing in place yet that actually restricts the MPU
sleep states on omap2 when requested.

Which puzzles me.. The patch you are referring to transfers the omap I2C
from the old omap PM API to the new QOS API is not applied yet in
mainline. The I2C is definitely working with the old API too, I'm just
wondering why I can't make it working with either of the APIs.. Am I
missing something here?

>> Therefore, it makes sense to actually remove this call entirely from
>> the ir-rx51 driver as it is both wrong and does nothing useful at the
>> moment.
>>
>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
> 
> Regards,
> Jean
> 
>> ---
>>  arch/arm/mach-omap2/board-rx51-peripherals.c | 2 --
>>  drivers/media/rc/ir-rx51.c                   | 9 ---------
>>  include/media/ir-rx51.h                      | 2 --
>>  3 files changed, 13 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
>> index ca07264..e0750cb 100644
>> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
>> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
>> @@ -34,7 +34,6 @@
>>  #include <plat/gpmc.h>
>>  #include <plat/onenand.h>
>>  #include <plat/gpmc-smc91x.h>
>> -#include <plat/omap-pm.h>
>>
>>  #include <mach/board-rx51.h>
>>
>> @@ -1227,7 +1226,6 @@ static void __init rx51_init_tsc2005(void)
>>
>>  #if defined(CONFIG_IR_RX51) || defined(CONFIG_IR_RX51_MODULE)
>>  static struct lirc_rx51_platform_data rx51_lirc_data = {
>> -       .set_max_mpu_wakeup_lat = omap_pm_set_max_mpu_wakeup_lat,
>>         .pwm_timer = 9, /* Use GPT 9 for CIR */
>>  };
>>
>> diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
>> index 7eed541..ac7d3f0 100644
>> --- a/drivers/media/rc/ir-rx51.c
>> +++ b/drivers/media/rc/ir-rx51.c
>> @@ -267,12 +267,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>         if (count < WBUF_LEN)
>>                 lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
>>
>> -       /*
>> -        * Adjust latency requirements so the device doesn't go in too
>> -        * deep sleep states
>> -        */
>> -       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
>> -
>>         lirc_rx51_on(lirc_rx51);
>>         lirc_rx51->wbuf_index = 1;
>>         pulse_timer_set_timeout(lirc_rx51, lirc_rx51->wbuf[0]);
>> @@ -292,9 +286,6 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>>          */
>>         lirc_rx51_stop_tx(lirc_rx51);
>>
>> -       /* We can sleep again */
>> -       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, -1);
>> -
>>         return n;
>>  }
>>
>> diff --git a/include/media/ir-rx51.h b/include/media/ir-rx51.h
>> index 104aa89..57523f2 100644
>> --- a/include/media/ir-rx51.h
>> +++ b/include/media/ir-rx51.h
>> @@ -3,8 +3,6 @@
>>
>>  struct lirc_rx51_platform_data {
>>         int pwm_timer;
>> -
>> -       int(*set_max_mpu_wakeup_lat)(struct device *dev, long t);
>>  };
>>
>>  #endif
>> --
>> 1.7.12
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 7/8] ir-rx51: Remove MPU wakeup latency adjustments

[Jean Pihet]

Hi Timo,

On Fri, Aug 24, 2012 at 10:14 AM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
> Hi Jean,
>
> On 08/23/12 14:58, Jean Pihet wrote:
>> Hi Timo,
>>
>> On Wed, Aug 22, 2012 at 9:50 PM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
>> That is correct. The API to use is the PM QoS API which cpuidle uses
>> to determine the next MPU state based on the allowed latency.
>>
>>> A more appropriate fix for the problem would be to modify the idle
>>> layer so that it does not allow MPU going to too deep sleep modes when
>>> it is expected that the timers need to wake up MPU.
>> The idle layer already uses the PM QoS framework to decide the next
>> MPU state. I think the right solution is to convert from
>> omap_pm_set_max_mpu_wakeup_lat to the PM QoS API.
>>
>> Cf. http://marc.info/?l=linux-omap&m=133968658305580&w=2 for an
>> example of the conversion.
>>
>
> Thanks. It looks like really easy and straightforward conversion.
> However, I couldn't find the patch you were referring to from any trees
Correct, this patch is not applied to the mainline code yet, it is
provided as an example of the conversion.

> I could find. So, I take that this API does not really have omap2
> support in it yet? I tried git grepping through the source and to me it
> appears there is nothing in place yet that actually restricts the MPU
> sleep states on omap2 when requested.
The MPU state is controlled from the cpuidle framework, which
retrieves the MPU allowed latency from the PM QoS framework. This is
supported on OMAP2.
Cf. the table of states and the associated latency in
arch/arm/mach-omap2/cpuidle34xx.c.

> Which puzzles me.. The patch you are referring to transfers the omap I2C
> from the old omap PM API to the new QOS API is not applied yet in
> mainline. The I2C is definitely working with the old API too, I'm just
> wondering why I can't make it working with either of the APIs.. Am I
> missing something here?
AFAIK the old API is a noop in mainline. Using the PM QoS API
defnitely is supported, that is why I think the conversion needs to be
performed.

>>> Therefore, it makes sense to actually remove this call entirely from
>>> the ir-rx51 driver as it is both wrong and does nothing useful at the
>>> moment.
>>>
>>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
>>
>> Regards,
>> Jean
>>

Thanks,
Jean

Re: [PATCH 7/8] ir-rx51: Remove MPU wakeup latency adjustments

[Timo Kokkonen]

On 08/24/12 12:04, Jean Pihet wrote:
> Hi Timo,
> 
> On Fri, Aug 24, 2012 at 10:14 AM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
>> Hi Jean,
>>
>> On 08/23/12 14:58, Jean Pihet wrote:
>>> Hi Timo,
>>>
>>> On Wed, Aug 22, 2012 at 9:50 PM, Timo Kokkonen <timo.t.kokkonen@iki.fi> wrote:
>>> That is correct. The API to use is the PM QoS API which cpuidle uses
>>> to determine the next MPU state based on the allowed latency.
>>>
>>>> A more appropriate fix for the problem would be to modify the idle
>>>> layer so that it does not allow MPU going to too deep sleep modes when
>>>> it is expected that the timers need to wake up MPU.
>>> The idle layer already uses the PM QoS framework to decide the next
>>> MPU state. I think the right solution is to convert from
>>> omap_pm_set_max_mpu_wakeup_lat to the PM QoS API.
>>>
>>> Cf. http://marc.info/?l=linux-omap&m=133968658305580&w=2 for an
>>> example of the conversion.
>>>
>>
>> Thanks. It looks like really easy and straightforward conversion.
>> However, I couldn't find the patch you were referring to from any trees
> Correct, this patch is not applied to the mainline code yet, it is
> provided as an example of the conversion.
> 
>> I could find. So, I take that this API does not really have omap2
>> support in it yet? I tried git grepping through the source and to me it
>> appears there is nothing in place yet that actually restricts the MPU
>> sleep states on omap2 when requested.
> The MPU state is controlled from the cpuidle framework, which
> retrieves the MPU allowed latency from the PM QoS framework. This is
> supported on OMAP2.
> Cf. the table of states and the associated latency in
> arch/arm/mach-omap2/cpuidle34xx.c.
> 

Thanks for the pointer. I took a look at the state table and adjusted
the latency requirements in my code. If I lower the latency from 50us to
10us, the timers are then waking up as they should be.

I'll replace this patch with one where I convert it using the new API.

Thanks!

-Timo

[PATCH 8/8] ir-rx51: Remove useless variable from struct lirc_rx51

[Timo Kokkonen]

As clearly visible from the patch, this variable has no useful purpose
what so ever. Thus, it can be removed altogether without any side
effects.

Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index ac7d3f0..23bc8c0 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -55,7 +55,6 @@ struct lirc_rx51 {
 	unsigned int	freq;		/* carrier frequency */
 	unsigned int	duty_cycle;	/* carrier duty cycle */
 	unsigned int	irq_num;
-	unsigned int	match;
 	int		wbuf[WBUF_LEN];
 	int		wbuf_index;
 	unsigned long	device_is_open;
@@ -100,8 +99,6 @@ static int init_timing_params(struct lirc_rx51 *lirc_rx51)
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer, 0);
 	omap_dm_timer_start(lirc_rx51->pulse_timer);
 
-	lirc_rx51->match = 0;
-
 	return 0;
 }
 
@@ -111,11 +108,7 @@ static int pulse_timer_set_timeout(struct lirc_rx51 *lirc_rx51, int usec)
 
 	BUG_ON(usec < 0);
 
-	if (lirc_rx51->match == 0)
-		counter = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
-	else
-		counter = lirc_rx51->match;
-
+	counter = omap_dm_timer_read_counter(lirc_rx51->pulse_timer);
 	counter += (u32)(lirc_rx51->fclk_khz * usec / (1000));
 	omap_dm_timer_set_match(lirc_rx51->pulse_timer, 1, counter);
 	omap_dm_timer_set_int_enable(lirc_rx51->pulse_timer,
-- 
1.7.12