ir-rx51: Various fixes

PATCH New Timo Kokkonen 3 participants 17 messages Nov 18, 2012 – Dec 14, 2012

Subsystems (5)

Arm (32-bit) Media (V4L/DVB) Arm sub-architectures OMAP2+ support +1 more subsystems

lore

Share & tools

lore

Timo Kokkonen

author

2012-11-18 15:13 UTC #0 lore raw

cover ir-rx51: Various fixes

Hi,

Here is a set of fixes that did not make in the last merge window. Everything else except the last patch that fixes sparse complaints have been posted before.

Especially the PM QoS conversion patch is important, without that one the driver does not work at all unless there is some background load keeping the CPU from sleeping.

The signal handling fixup patches did raise all sorts of discussion last time, but my conclusion was that the patch itself should be fine for now.

Please provide feedback and consider accepting them in. Thank you.

Timo Kokkonen (7):
ir-rx51: Handle signals properly
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
ir-rx51: Fix sparse warnings

 arch/arm/mach-omap2/board-rx51-peripherals.c |   2 -
 drivers/media/rc/ir-rx51.c                   | 108 ++++++++++++++-------------
 include/media/ir-rx51.h                      |   2 -
 3 files changed, 56 insertions(+), 56 deletions(-)

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #1 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

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 <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

drivers/media/rc/ir-rx51.c +28 ~~-11~~

index 546199e..125d4c3 100644
@@ -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;

@@ -163,13 +176,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;
 }

@@ -249,8 +256,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;

@@ -276,9 +284,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);

Signature · 1 line

1.8.0

Tony Lindgren

maintainer

2012-11-20 19:57 UTC #2 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Hi,

Quoted 12 lines · Timo Kokkonen

drivers/media/rc/ir-rx51.c +6 -0

@@ -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);

drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92

Quoted 10 lines

drivers/media/rc/ir-rx51.c +7 -0

+	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;

Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.

We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.

If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.

The other alternative is to set them up as platform_data function pointers, but that won't work after we make omap2+ device tree only. And that really just postpones the problem.

Cheers,

Tony

Quoted 55 lines

drivers/media/rc/ir-rx51.c +15 ~~-11~~

@@ -163,13 +176,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;
 }

@@ -249,8 +256,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;

@@ -276,9 +284,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);

Signature · 6 lines

1.8.0

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren

maintainer

2012-12-14 17:28 UTC #3 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Quoted 30 lines · Tony Lindgren

Hi,
Quoted 12 lines · Timo Kokkonen
drivers/media/rc/ir-rx51.c +6 -0
@@ -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);
drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92
Quoted 10 lines
drivers/media/rc/ir-rx51.c +7 -0
+	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;
Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.

Quoted 6 lines

We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.
If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.

Just to update the status on this.. It seems that we'll be moving parts of plat/dmtimer into a minimal include/linux/timer-omap.h unless people have better ideas on what to do with custom hardware timers for PWM etc.

Quoted 68 lines

The other alternative is to set them up as platform_data function pointers, but that won't work after we make omap2+ device tree only. And that really just postpones the problem.

Cheers,

Tony

Quoted 55 lines

drivers/media/rc/ir-rx51.c +15 ~~-11~~

@@ -163,13 +176,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;
 }

@@ -249,8 +256,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;

@@ -276,9 +284,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);

Signature · 6 lines

1.8.0

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at 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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Felipe Balbi

2012-12-14 17:26 UTC #4 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Hi,

Quoted 35 lines · Tony Lindgren

Quoted 34 lines · Tony Lindgren
Hi,
Quoted 12 lines · Timo Kokkonen
drivers/media/rc/ir-rx51.c +6 -0
@@ -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);
drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92
Quoted 10 lines
drivers/media/rc/ir-rx51.c +7 -0
+	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;
Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.
We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.

Quoted 7 lines

Quoted 2 lines
If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.
Just to update the status on this.. It seems that we'll be moving parts of plat/dmtimer into a minimal include/linux/timer-omap.h unless people have better ideas on what to do with custom hardware timers for PWM etc.

if it's really for PWM, shouldn't we be using drivers/pwm/ ??

Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121214/5551b90b/attachment.sig>

Tony Lindgren

maintainer

2012-12-14 17:46 UTC #5 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Quoted 46 lines · Felipe Balbi

Hi,
Quoted 35 lines · Tony Lindgren
Quoted 34 lines · Tony Lindgren
Hi,
Quoted 12 lines · Timo Kokkonen
drivers/media/rc/ir-rx51.c +6 -0
@@ -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);
drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92
Quoted 10 lines
drivers/media/rc/ir-rx51.c +7 -0
+	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;
Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.
We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.
Quoted 7 lines
Quoted 2 lines
If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.
Just to update the status on this.. It seems that we'll be moving parts of plat/dmtimer into a minimal include/linux/timer-omap.h unless people have better ideas on what to do with custom hardware timers for PWM etc.

Quoted 5 lines

if it's really for PWM, shouldn't we be using drivers/pwm/ ??
Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.

That would only help with omap_dm_timer_set_pwm() I think.

The other functions are also needed by the clocksource and clockevent drivers. And tidspbridge too:

$ grep -r omap_dm_timer drivers/
...

Regards,

Tony

Felipe Balbi

2012-12-14 17:49 UTC #6 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Hi,

Quoted 49 lines · Tony Lindgren

Quoted 48 lines · Felipe Balbi
Hi,
Quoted 35 lines · Tony Lindgren
Quoted 34 lines · Tony Lindgren
Hi,
Quoted 12 lines · Timo Kokkonen
drivers/media/rc/ir-rx51.c +6 -0
@@ -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);
drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92
Quoted 10 lines
drivers/media/rc/ir-rx51.c +7 -0
+	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;
Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.
We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.
Quoted 7 lines
Quoted 2 lines
If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.
Just to update the status on this.. It seems that we'll be moving parts of plat/dmtimer into a minimal include/linux/timer-omap.h unless people have better ideas on what to do with custom hardware timers for PWM etc.
if it's really for PWM, shouldn't we be using drivers/pwm/ ??

Quoted 8 lines

Quoted 3 lines
Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.
That would only help with omap_dm_timer_set_pwm() I think.
The other functions are also needed by the clocksource and clockevent drivers. And tidspbridge too:

well, we _do_ have drivers/clocksource ;-)

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121214/feacf12c/attachment.sig>

Tony Lindgren

maintainer

2012-12-14 18:06 UTC #7 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Quoted 9 lines · Felipe Balbi

Quoted 8 lines · Tony Lindgren
Quoted 6 lines · Felipe Balbi
if it's really for PWM, shouldn't we be using drivers/pwm/ ??
Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.

Quoted 6 lines

Quoted 4 lines
That would only help with omap_dm_timer_set_pwm() I think.
The other functions are also needed by the clocksource and clockevent drivers. And tidspbridge too:
well, we _do_ have drivers/clocksource ;-)

That's where the dmtimer code should live. But still it does not help with the header.

Thomas, maybe we could use the hrtimer framework for it if there was some way to completely leave out the rb tree for the dedicated hardware timers? There's no queue needed as there's always just one value tied to a specific timer.

Regards,

Tony

Felipe Balbi

2012-12-14 18:08 UTC #8 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Hi,

Quoted 12 lines · Tony Lindgren

Quoted 11 lines · Felipe Balbi
Quoted 10 lines · Tony Lindgren
Quoted 6 lines · Felipe Balbi
if it's really for PWM, shouldn't we be using drivers/pwm/ ??
Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.
That would only help with omap_dm_timer_set_pwm() I think.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
The other functions are also needed by the clocksource and clockevent drivers. And tidspbridge too:
well, we _do_ have drivers/clocksource ;-)
That's where the dmtimer code should live. But still it does not help with the header.

yeah, the header should be where you suggested, no doubts. I was actually criticizing the current timer code.

cheers

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121214/977cf548/attachment.sig>

Timo Kokkonen

author

2012-12-14 19:31 UTC #9 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

In reply to Felipe Balbi

Quoted 41 lines · Felipe Balbi

Hi,
Quoted 38 lines · Tony Lindgren
Quoted 36 lines · Tony Lindgren
Hi,
Quoted 12 lines · Timo Kokkonen
drivers/media/rc/ir-rx51.c +6 -0
@@ -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);
drivers/media/rc/ir-rx51.c static void lirc_rx51_off(struct lirc_rx51 *lirc_rx51) · line 92
Quoted 10 lines
drivers/media/rc/ir-rx51.c +7 -0
+	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;
Good fixes in general.. But you won't be able to access the omap_dm_timer functions after we enable ARM multiplatform support for omap2+. That's for v3.9 probably right after v3.8-rc1.
We need to find some Linux generic API to use hardware timers like this, so I've added Thomas Gleixner and linux-arm-kernel mailing list to cc.
If no such API is available, then maybe we can export some of the omap_dm_timer functions if Thomas is OK with that.

Quoted 7 lines

Quoted 4 lines
Just to update the status on this.. It seems that we'll be moving parts of plat/dmtimer into a minimal include/linux/timer-omap.h unless people have better ideas on what to do with custom hardware timers for PWM etc.
if it's really for PWM, shouldn't we be using drivers/pwm/ ??

Now that Neil Brown posted the PWM driver for omap, I've been thinking about whether converting the ir-rx51 into the PWM API would work. Maybe controlling the PWM itself would be sufficient, but the ir-rx51 uses also another dmtimer for creating accurate (enough) timing source for the IR pulse edges.

I haven't tried whether the default 32kHz clock source is enough for that. Now that I think about it, I don't see why it wouldn't be good enough. I think it would even be possible to just use the PWM api alone (plus hr-timers) in order to generate good enough IR pulses.

-Timo

Quoted 4 lines

Meaning that $SUBJECT would just request a PWM device and use it. That doesn't solve the whole problem, however, as pwm-omap.c would still need access to timer-omap.h.

Tony Lindgren

maintainer

2012-12-14 19:41 UTC #10 lore raw

PATCH 1/7 ir-rx51: Handle signals properly

Quoted 10 lines · Timo Kokkonen

Quoted 3 lines · Felipe Balbi
if it's really for PWM, shouldn't we be using drivers/pwm/ ??
Now that Neil Brown posted the PWM driver for omap, I've been thinking about whether converting the ir-rx51 into the PWM API would work. Maybe controlling the PWM itself would be sufficient, but the ir-rx51 uses also another dmtimer for creating accurate (enough) timing source for the IR pulse edges.

OK.

Quoted 3 lines

I haven't tried whether the default 32kHz clock source is enough for that. Now that I think about it, I don't see why it wouldn't be good enough. I think it would even be possible to just use the PWM api alone

Cool.

Regards,

Tony

Timo Kokkonen

author

2012-11-18 15:13 UTC #11 lore raw

PATCH 2/7 ir-rx51: Clean up timer initialization code

In reply to 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 <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

drivers/media/rc/ir-rx51.c +3 -8

index 125d4c3..f22e5e4 100644
@@ -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)

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #12 lore raw

PATCH 3/7 ir-rx51: Move platform data checking into probe function

In reply to 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 <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/media/rc/ir-rx51.c +3 -1

index f22e5e4..16b3c1f 100644
@@ -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;

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #13 lore raw

PATCH 4/7 ir-rx51: Replace module_{init,exit} macros with module_platform_driver

In reply to Timo Kokkonen

No reason to avoid using the existing helpers.

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

drivers/media/rc/ir-rx51.c +1 ~~-11~~

index 16b3c1f..6e1ffa6 100644
@@ -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");

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #14 lore raw

PATCH 5/7 ir-rx51: Convert latency constraints to PM QoS API

In reply to 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 <[email protected]>

Acked-by Tony Lindgren <[email protected]>

Acked-by Jean Pihet <[email protected]>

---
 arch/arm/mach-omap2/board-rx51-peripherals.c |  2 --
 drivers/media/rc/ir-rx51.c                   | 17 ++++++++++++-----
 include/media/ir-rx51.h                      |  2 --
 3 files changed, 12 insertions(+), 9 deletions(-)

arch/arm/mach-omap2/board-rx51-peripherals.c +0 -2

index 020e03c..6d0f29d 100644
@@ -33,7 +33,6 @@
 #include "common.h"
 #include <plat/dma.h>
 #include <plat/gpmc.h>
-#include <plat/omap-pm.h>
 #include "gpmc-smc91x.h"
 
 #include "board-rx51.h"

@@ -1224,7 +1223,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 */
 };

drivers/media/rc/ir-rx51.c +12 -5

index 6e1ffa6..96ed23d 100644
@@ -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,16 @@ 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. The 10us latency requirement is low enough to
+	 * keep MPU from sleeping and thus ensures the interrupts are
+	 * getting through 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 +300,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;
 }

include/media/ir-rx51.h +0 -2

index 104aa89..57523f2 100644
@@ -3,8 +3,6 @@
 
 struct lirc_rx51_platform_data {
 	int pwm_timer;
-
-	int(*set_max_mpu_wakeup_lat)(struct device *dev, long t);
 };
 
 #endif

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #15 lore raw

PATCH 6/7 ir-rx51: Remove useless variable from struct lirc_rx51

In reply to 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 <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

drivers/media/rc/ir-rx51.c +1 -8

index 96ed23d..edb1562 100644
@@ -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,

Signature · 1 line

1.8.0

Timo Kokkonen

author

2012-11-18 15:13 UTC #16 lore raw

PATCH 7/7 ir-rx51: Fix sparse warnings

In reply to Timo Kokkonen

Add missing __user annotation to all of the user space memory accesses. Otherwise sparse is complainign about address space difference in types.

Also struct lirc_rx51_platform_driver is missing static keyword even though it should have it.

Signed-off-by Timo Kokkonen <[email protected]>

---
 drivers/media/rc/ir-rx51.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/media/rc/ir-rx51.c +8 -8

index edb1562..7ed0616 100644
@@ -233,7 +233,7 @@ static int lirc_rx51_free_port(struct lirc_rx51 *lirc_rx51)
 	return 0;
 }
 
-static ssize_t lirc_rx51_write(struct file *file, const char *buf,
+static ssize_t lirc_rx51_write(struct file *file, const char __user *buf,
 			  size_t n, loff_t *ppos)
 {
 	int count, i;

@@ -308,13 +308,13 @@ static long lirc_rx51_ioctl(struct file *filep,
 
 	switch (cmd) {
 	case LIRC_GET_SEND_MODE:
-		result = put_user(LIRC_MODE_PULSE, (unsigned long *)arg);
+		result = put_user(LIRC_MODE_PULSE, (unsigned long __user *)arg);
 		if (result)
 			return result;
 		break;
 
 	case LIRC_SET_SEND_MODE:
-		result = get_user(value, (unsigned long *)arg);
+		result = get_user(value, (unsigned long __user *)arg);
 		if (result)
 			return result;

@@ -324,7 +324,7 @@ static long lirc_rx51_ioctl(struct file *filep,
 		break;
 
 	case LIRC_GET_REC_MODE:
-		result = put_user(0, (unsigned long *) arg);
+		result = put_user(0, (unsigned long __user *)arg);
 		if (result)
 			return result;
 		break;

@@ -334,7 +334,7 @@ static long lirc_rx51_ioctl(struct file *filep,
 		break;
 
 	case LIRC_SET_SEND_DUTY_CYCLE:
-		result = get_user(ivalue, (unsigned int *) arg);
+		result = get_user(ivalue, (unsigned int __user *)arg);
 		if (result)
 			return result;

@@ -348,7 +348,7 @@ static long lirc_rx51_ioctl(struct file *filep,
 		break;
 
 	case LIRC_SET_SEND_CARRIER:
-		result = get_user(ivalue, (unsigned int *) arg);
+		result = get_user(ivalue, (unsigned int __user *)arg);
 		if (result)
 			return result;

@@ -363,7 +363,7 @@ static long lirc_rx51_ioctl(struct file *filep,
 
 	case LIRC_GET_FEATURES:
 		result = put_user(LIRC_RX51_DRIVER_FEATURES,
-				  (unsigned long *) arg);
+				(unsigned long __user *)arg);
 		if (result)
 			return result;
 		break;

@@ -484,7 +484,7 @@ static int __exit lirc_rx51_remove(struct platform_device *dev)
 	return lirc_unregister_driver(lirc_rx51_driver.minor);
 }
 
-struct platform_driver lirc_rx51_platform_driver = {
+static struct platform_driver lirc_rx51_platform_driver = {
 	.probe		= lirc_rx51_probe,
 	.remove		= __exit_p(lirc_rx51_remove),
 	.suspend	= lirc_rx51_suspend,

Signature · 1 line

1.8.0