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

[PATCHv2 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. Now that Jean Pihet kindly instructed me how
to use the PM QoS API for setting the latency constraints, the driver
will now work without any background load. Someone might consider such
restriction a blocker bug, that is fixed on this patch set.

Changes since v1:

- Replace wake_up_interruptible with wake_up, as the driver is having
  non-interruptible sleeps

- Instead of just removing the set_max_mpu_wakeup_lat calls, replace
  them with QoS API calls

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: Convert latency constraints to PM QoS API
  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                   | 100 ++++++++++++++-------------
 include/media/ir-rx51.h                      |   2 -
 4 files changed, 54 insertions(+), 54 deletions(-)

-- 
1.7.12

[PATCHv2 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

[PATCHv2 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..e2db94e 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(&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

[PATCHv2 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>
---
 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 e2db94e..125d4c3 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

[PATCHv2 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 125d4c3..f22e5e4 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

[PATCHv2 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 f22e5e4..16b3c1f 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

[PATCHv2 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 16b3c1f..6e1ffa6 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

[PATCHv2 7/8] ir-rx51: Convert latency constraints to PM QoS API

[Timo Kokkonen]

Convert the driver from the obsolete omap_pm_set_max_mpu_wakeup_lat
API to the new PM QoS API. This allows the callback to be removed from
the platform data structure.

The latency requirements are also adjusted to prevent the MPU from
going into sleep mode. This is needed as the GP timers have no means
to wake up the MPU once it has gone into sleep. The "side effect" is
that from now on the driver actually works even if there is no
background load keeping the MPU awake.

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                   | 15 ++++++++++-----
 include/media/ir-rx51.h                      |  2 --
 3 files changed, 10 insertions(+), 9 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 6e1ffa6..008cdab 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
+#include <linux/pm_qos.h>
 
 #include <plat/dmtimer.h>
 #include <plat/clock.h>
@@ -49,6 +50,7 @@ struct lirc_rx51 {
 	struct omap_dm_timer *pulse_timer;
 	struct device	     *dev;
 	struct lirc_rx51_platform_data *pdata;
+	struct pm_qos_request	pm_qos_request;
 	wait_queue_head_t     wqueue;
 
 	unsigned long	fclk_khz;
@@ -268,10 +270,14 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
 		lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
 
 	/*
-	 * Adjust latency requirements so the device doesn't go in too
-	 * deep sleep states
+	 * If the MPU is going into too deep sleep state while we are
+	 * transmitting the IR code, timers will not be able to wake
+	 * up the MPU. Thus, we need to set a strict enough latency
+	 * requirement in order to ensure the interrupts come though
+	 * properly.
 	 */
-	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
+	pm_qos_add_request(&lirc_rx51->pm_qos_request,
+			PM_QOS_CPU_DMA_LATENCY,	10);
 
 	lirc_rx51_on(lirc_rx51);
 	lirc_rx51->wbuf_index = 1;
@@ -292,8 +298,7 @@ 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);
+	pm_qos_remove_request(&lirc_rx51->pm_qos_request);
 
 	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: [PATCHv2 7/8] ir-rx51: Convert latency constraints to PM QoS API

[Tony Lindgren]

* Timo Kokkonen <timo.t.kokkonen@iki.fi> [120824 08:11]:
> Convert the driver from the obsolete omap_pm_set_max_mpu_wakeup_lat
> API to the new PM QoS API. This allows the callback to be removed from
> the platform data structure.
> 
> The latency requirements are also adjusted to prevent the MPU from
> going into sleep mode. This is needed as the GP timers have no means
> to wake up the MPU once it has gone into sleep. The "side effect" is
> that from now on the driver actually works even if there is no
> background load keeping the MPU awake.
> 
> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>

This should get acked by Kevin ideally. Other than that:

Acked-by: Tony Lindgren <tony@atomide.com>

> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c |  2 --
>  drivers/media/rc/ir-rx51.c                   | 15 ++++++++++-----
>  include/media/ir-rx51.h                      |  2 --
>  3 files changed, 10 insertions(+), 9 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 6e1ffa6..008cdab 100644
> --- a/drivers/media/rc/ir-rx51.c
> +++ b/drivers/media/rc/ir-rx51.c
> @@ -25,6 +25,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/wait.h>
> +#include <linux/pm_qos.h>
>  
>  #include <plat/dmtimer.h>
>  #include <plat/clock.h>
> @@ -49,6 +50,7 @@ struct lirc_rx51 {
>  	struct omap_dm_timer *pulse_timer;
>  	struct device	     *dev;
>  	struct lirc_rx51_platform_data *pdata;
> +	struct pm_qos_request	pm_qos_request;
>  	wait_queue_head_t     wqueue;
>  
>  	unsigned long	fclk_khz;
> @@ -268,10 +270,14 @@ static ssize_t lirc_rx51_write(struct file *file, const char *buf,
>  		lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
>  
>  	/*
> -	 * Adjust latency requirements so the device doesn't go in too
> -	 * deep sleep states
> +	 * If the MPU is going into too deep sleep state while we are
> +	 * transmitting the IR code, timers will not be able to wake
> +	 * up the MPU. Thus, we need to set a strict enough latency
> +	 * requirement in order to ensure the interrupts come though
> +	 * properly.
>  	 */
> -	lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
> +	pm_qos_add_request(&lirc_rx51->pm_qos_request,
> +			PM_QOS_CPU_DMA_LATENCY,	10);
>  
>  	lirc_rx51_on(lirc_rx51);
>  	lirc_rx51->wbuf_index = 1;
> @@ -292,8 +298,7 @@ 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);
> +	pm_qos_remove_request(&lirc_rx51->pm_qos_request);
>  
>  	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: [PATCHv2 7/8] ir-rx51: Convert latency constraints to PM QoS API

[Jean Pihet]

Hi Timo,

On Fri, Aug 24, 2012 at 10:39 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Timo Kokkonen <timo.t.kokkonen@iki.fi> [120824 08:11]:
>> Convert the driver from the obsolete omap_pm_set_max_mpu_wakeup_lat
>> API to the new PM QoS API. This allows the callback to be removed from
>> the platform data structure.
>>
>> The latency requirements are also adjusted to prevent the MPU from
>> going into sleep mode. This is needed as the GP timers have no means
>> to wake up the MPU once it has gone into sleep. The "side effect" is
>> that from now on the driver actually works even if there is no
>> background load keeping the MPU awake.
>>
>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
>
> This should get acked by Kevin ideally. Other than that:
>
> Acked-by: Tony Lindgren <tony@atomide.com>

...
@@ -268,10 +270,14 @@ static ssize_t lirc_rx51_write(struct file
*file, const char *buf,
                lirc_rx51->wbuf[count] = -1; /* Insert termination mark */

        /*
-        * Adjust latency requirements so the device doesn't go in too
-        * deep sleep states
+        * If the MPU is going into too deep sleep state while we are
+        * transmitting the IR code, timers will not be able to wake
+        * up the MPU. Thus, we need to set a strict enough latency
+        * requirement in order to ensure the interrupts come though
+        * properly.
         */
-       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
+       pm_qos_add_request(&lirc_rx51->pm_qos_request,
+                       PM_QOS_CPU_DMA_LATENCY, 10);
Minor remark: it would be nice to have more detail on where the
latency number 10 comes from. Is it fixed, is it linked to the baud
rate etc?

Here is my ack for the PM QoS API part:
Acked-by: Jean Pihet <j-pihet@ti.com>

Regards,
Jean

Re: [PATCHv2 7/8] ir-rx51: Convert latency constraints to PM QoS API

[Timo Kokkonen]

On 08/27/12 12:25, Jean Pihet wrote:
> Hi Timo,
> 
> On Fri, Aug 24, 2012 at 10:39 PM, Tony Lindgren <tony@atomide.com> wrote:
>> * Timo Kokkonen <timo.t.kokkonen@iki.fi> [120824 08:11]:
>>> Convert the driver from the obsolete omap_pm_set_max_mpu_wakeup_lat
>>> API to the new PM QoS API. This allows the callback to be removed from
>>> the platform data structure.
>>>
>>> The latency requirements are also adjusted to prevent the MPU from
>>> going into sleep mode. This is needed as the GP timers have no means
>>> to wake up the MPU once it has gone into sleep. The "side effect" is
>>> that from now on the driver actually works even if there is no
>>> background load keeping the MPU awake.
>>>
>>> Signed-off-by: Timo Kokkonen <timo.t.kokkonen@iki.fi>
>>
>> This should get acked by Kevin ideally. Other than that:
>>
>> Acked-by: Tony Lindgren <tony@atomide.com>
> 
> ...
> @@ -268,10 +270,14 @@ static ssize_t lirc_rx51_write(struct file
> *file, const char *buf,
>                 lirc_rx51->wbuf[count] = -1; /* Insert termination mark */
> 
>         /*
> -        * Adjust latency requirements so the device doesn't go in too
> -        * deep sleep states
> +        * If the MPU is going into too deep sleep state while we are
> +        * transmitting the IR code, timers will not be able to wake
> +        * up the MPU. Thus, we need to set a strict enough latency
> +        * requirement in order to ensure the interrupts come though
> +        * properly.
>          */
> -       lirc_rx51->pdata->set_max_mpu_wakeup_lat(lirc_rx51->dev, 50);
> +       pm_qos_add_request(&lirc_rx51->pm_qos_request,
> +                       PM_QOS_CPU_DMA_LATENCY, 10);
> Minor remark: it would be nice to have more detail on where the
> latency number 10 comes from. Is it fixed, is it linked to the baud
> rate etc?
> 

Yeah, it was chosen to be low enough for the MPU to receive the IRQ from
the timers. 50us was good enough back then with the original n900
kernel, but nowadays it is not good enough from preventing the MPU from
going to sleep where the timer interrupts don't come through.

Yes, I should probably have stated that in the comment to make it clear.
Can I re-send just this one patch or should I send the entire set again?
I'm assuming these go in through Mauro's media tree as these depend on
stuff that's already there. So, which ever is easier for him I guess :)

Thanks!

-Timo

> Here is my ack for the PM QoS API part:
> Acked-by: Jean Pihet <j-pihet@ti.com>
> 
> Regards,
> Jean
> --
> 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
> 

[PATCHv2 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 008cdab..179b64d 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -57,7 +57,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;
@@ -102,8 +101,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;
 }
 
@@ -113,11 +110,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