[PATCH 0/7] ir-rx51: Various fixes

[PATCH 0/7] ir-rx51: Various fixes

[Timo Kokkonen]

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(-)

-- 
1.8.0

[PATCH 1/7] 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 546199e..125d4c3 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;
@@ -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);
-- 
1.8.0

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Tony Lindgren]

Hi,

* Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
> --- 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;

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


> @@ -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);
> -- 
> 1.8.0
> 
> --
> 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 1/7] ir-rx51: Handle signals properly

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121120 12:00]:
> Hi,
> 
> * Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
> > --- 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;
> 
> 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.

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.
 
> 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
> 
> 
> > @@ -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);
> > -- 
> > 1.8.0
> > 
> > --
> > 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 1/7] ir-rx51: Handle signals properly

[Felipe Balbi]

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

Hi,

On Fri, Dec 14, 2012 at 09:28:09AM -0800, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [121120 12:00]:
> > Hi,
> > 
> > * Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
> > > --- 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;
> > 
> > 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.
> 
> 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.

-- 
balbi

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

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [121214 09:36]:
> Hi,
> 
> On Fri, Dec 14, 2012 at 09:28:09AM -0800, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [121120 12:00]:
> > > Hi,
> > > 
> > > * Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
> > > > --- 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;
> > > 
> > > 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.
> > 
> > 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.

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

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Felipe Balbi]

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

Hi,

On Fri, Dec 14, 2012 at 09:46:29AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [121214 09:36]:
> > Hi,
> > 
> > On Fri, Dec 14, 2012 at 09:28:09AM -0800, Tony Lindgren wrote:
> > > * Tony Lindgren <tony@atomide.com> [121120 12:00]:
> > > > Hi,
> > > > 
> > > > * Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
> > > > > --- 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;
> > > > 
> > > > 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.
> > > 
> > > 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.
> 
> 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 ;-)

-- 
balbi

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

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [121214 09:59]:
> On Fri, Dec 14, 2012 at 09:46:29AM -0800, Tony Lindgren wrote:
> > * Felipe Balbi <balbi@ti.com> [121214 09:36]:
> > > 
> > > 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:
> 
> 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

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Felipe Balbi]

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

Hi,

On Fri, Dec 14, 2012 at 10:06:45AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [121214 09:59]:
> > On Fri, Dec 14, 2012 at 09:46:29AM -0800, Tony Lindgren wrote:
> > > * Felipe Balbi <balbi@ti.com> [121214 09:36]:
> > > > 
> > > > 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:
> > 
> > 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

-- 
balbi

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

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Timo Kokkonen]

On 12/14/12 19:26, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Dec 14, 2012 at 09:28:09AM -0800, Tony Lindgren wrote:
>> * Tony Lindgren <tony@atomide.com> [121120 12:00]:
>>> Hi,
>>>
>>> * Timo Kokkonen <timo.t.kokkonen@iki.fi> [121118 07:15]:
>>>> --- 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;
>>>
>>> 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.
>>
>> 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

> 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.
> 

Re: [PATCH 1/7] ir-rx51: Handle signals properly

[Tony Lindgren]

* Timo Kokkonen <timo.t.kokkonen@iki.fi> [121214 11:33]:
> On 12/14/12 19:26, Felipe Balbi wrote:
> > 
> > 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.
 
> 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

[PATCH 2/7] 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.8.0

[PATCH 3/7] 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.8.0

[PATCH 4/7] 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.8.0

[PATCH 5/7] 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>
Acked-by: Tony Lindgren <tony@atomide.com>
Acked-by: Jean Pihet <j-pihet@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index 020e03c..6d0f29d 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -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 */
 };
 
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 6e1ffa6..96ed23d 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,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;
 }
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.8.0

[PATCH 6/7] 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 96ed23d..edb1562 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.8.0

[PATCH 7/7] ir-rx51: Fix sparse warnings

[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 <timo.t.kokkonen@iki.fi>
---
 drivers/media/rc/ir-rx51.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index edb1562..7ed0616 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -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,
-- 
1.8.0