Patchwork [7/7] P2020ds: add event button handler

login
register
mail settings
Submitter Yang Li
Date Dec. 3, 2010, 12:34 p.m.
Message ID <1291379651-8822-7-git-send-email-leoli@freescale.com>
Download mbox | patch
Permalink /patch/74115/
State Superseded
Delegated to: Kumar Gala
Headers show

Comments

Yang Li - Dec. 3, 2010, 12:34 p.m.
This can be used as a wakeup source for power management.

Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/boot/dts/p2020ds.dts        |    9 ++++++++-
 arch/powerpc/platforms/85xx/mpc85xx_ds.c |   26 +++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 2 deletions(-)
Timur Tabi - Dec. 13, 2010, 9:56 p.m.
On Fri, Dec 3, 2010 at 6:34 AM, Li Yang <leoli@freescale.com> wrote:
> This can be used as a wakeup source for power management.

This patch doesn't actually add wake-up support.

This patch should probably be split up, since you're adding generic
functionality for the IRQ that applies to all 85xx boards, but you
only update the device tree for one board.

> +static irqreturn_t event_isr(int irq, void *dev_id)
> +{
> +
> +       printk(KERN_INFO "MPC85xxDS: Event button been pushed.\n");
> +       return IRQ_HANDLED;
> +}

Would it make sense to have this be a weak function, so that it would
be easier to implement board-specific support?

> +
> +static int __init p2020ds_ngpixis_init(void)

You're adding a function called "p2020ds_ngpixis_init" to the file
"mpc85xx_ds.c".  mpc85xx_ds.c supports more than just the P2020DS.

> +{
> +       int event_irq, ret;
> +       struct device_node *np;
> +
> +       np = of_find_compatible_node(NULL, NULL, "fsl,p2020ds-fpga");
> +       if (np) {
> +               event_irq = irq_of_parse_and_map(np, 0);
> +               ret = request_irq(event_irq, event_isr, 0, "event", NULL);

You should probably choose a less generic name than "event".

> +               if (ret)
> +                       printk(KERN_ERR "Can't request board event int\n");

Use pr_err()
Yang Li - Dec. 14, 2010, 4:24 a.m.
On Tue, Dec 14, 2010 at 5:56 AM, Timur Tabi <timur.tabi@gmail.com> wrote:
> On Fri, Dec 3, 2010 at 6:34 AM, Li Yang <leoli@freescale.com> wrote:
>> This can be used as a wakeup source for power management.
>
> This patch doesn't actually add wake-up support.

Any enabled IRQ is a valid wake-up source for standby.  The patch
enables a board specific interrupt for the purpose of wakeup.

>
> This patch should probably be split up, since you're adding generic
> functionality for the IRQ that applies to all 85xx boards, but you
> only update the device tree for one board.

The IRQ is a board specific one from GPIO which not applicable on all
85xx boards.

>
>> +static irqreturn_t event_isr(int irq, void *dev_id)
>> +{
>> +
>> +       printk(KERN_INFO "MPC85xxDS: Event button been pushed.\n");
>> +       return IRQ_HANDLED;
>> +}
>
> Would it make sense to have this be a weak function, so that it would
> be easier to implement board-specific support?

It's already a board-specific one.

>
>> +
>> +static int __init p2020ds_ngpixis_init(void)
>
> You're adding a function called "p2020ds_ngpixis_init" to the file
> "mpc85xx_ds.c".  mpc85xx_ds.c supports more than just the P2020DS.

I'm not sure if other DS boards covered by this file has the same functionality.

>
>> +{
>> +       int event_irq, ret;
>> +       struct device_node *np;
>> +
>> +       np = of_find_compatible_node(NULL, NULL, "fsl,p2020ds-fpga");
>> +       if (np) {
>> +               event_irq = irq_of_parse_and_map(np, 0);
>> +               ret = request_irq(event_irq, event_isr, 0, "event", NULL);
>
> You should probably choose a less generic name than "event".

Well, it's the name suggested by the board manual.  We may change it
to "event_button" if not too long.

- Leo
Timur Tabi - Dec. 14, 2010, 4:49 p.m.
On Mon, Dec 13, 2010 at 10:24 PM, Li Yang <leoli@freescale.com> wrote:

> Any enabled IRQ is a valid wake-up source for standby.  The patch
> enables a board specific interrupt for the purpose of wakeup.

Oh, right.  I should have realized that.

>> This patch should probably be split up, since you're adding generic
>> functionality for the IRQ that applies to all 85xx boards, but you
>> only update the device tree for one board.
>
> The IRQ is a board specific one from GPIO which not applicable on all
> 85xx boards.

Doesn't the device tree take care of that?  If the node exists, the
IRQ number is specified.  Otherwise, the feature is not enabled.

>>> +static irqreturn_t event_isr(int irq, void *dev_id)
>>> +{
>>> +
>>> +       printk(KERN_INFO "MPC85xxDS: Event button been pushed.\n");
>>> +       return IRQ_HANDLED;
>>> +}
>>
>> Would it make sense to have this be a weak function, so that it would
>> be easier to implement board-specific support?
>
> It's already a board-specific one.

Hmmm.... I guess technically it is, but I wonder if it should be.

>>> +
>>> +static int __init p2020ds_ngpixis_init(void)
>>
>> You're adding a function called "p2020ds_ngpixis_init" to the file
>> "mpc85xx_ds.c".  mpc85xx_ds.c supports more than just the P2020DS.
>
> I'm not sure if other DS boards covered by this file has the same functionality.

Well, either it does or it doesn't, but you can't add a p2020-specific
function to mpc85xx_ds.c.  You should just rename the function,
because I see no reason why it can't work on other DS boards.

The problem is the compatible string.  Each pixis implementation is
different, but they share several common traits.  I wonder if we need
to have a generic compatible string for the FPGA node.

>> You should probably choose a less generic name than "event".
>
> Well, it's the name suggested by the board manual.  We may change it
> to "event_button" if not too long.

I was thinking something like "fsl-fpga-event".

Patch

diff --git a/arch/powerpc/boot/dts/p2020ds.dts b/arch/powerpc/boot/dts/p2020ds.dts
index 1101914..f4c6520 100644
--- a/arch/powerpc/boot/dts/p2020ds.dts
+++ b/arch/powerpc/boot/dts/p2020ds.dts
@@ -1,7 +1,7 @@ 
 /*
  * P2020 DS Device Tree Source
  *
- * Copyright 2009 Freescale Semiconductor Inc.
+ * Copyright 2009-2010 Freescale Semiconductor Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -155,6 +155,13 @@ 
 			compatible = "fsl,elbc-fcm-nand";
 			reg = <0x6 0x0 0x40000>;
 		};
+
+		ngpixis@3,0 {
+			compatible = "fsl,p2020ds-fpga";
+			reg = <0x3 0 0x30>;
+			interrupt-parent = <&mpic>;
+			interrupts = <0 0>;
+		};
 	};
 
 	soc@ffe00000 {
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 8190bc2..a8807fe 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -4,7 +4,7 @@ 
  * Author Xianghua Xiao (x.xiao@freescale.com)
  * Roy Zang <tie-fei.zang@freescale.com>
  * 	- Add PCI/PCI Exprees support
- * Copyright 2007 Freescale Semiconductor Inc.
+ * Copyright 2007-2010 Freescale Semiconductor Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -200,6 +200,30 @@  static void __init mpc85xx_ds_setup_arch(void)
 	printk("MPC85xx DS board from Freescale Semiconductor\n");
 }
 
+static irqreturn_t event_isr(int irq, void *dev_id)
+{
+
+	printk(KERN_INFO "MPC85xxDS: Event button been pushed.\n");
+	return IRQ_HANDLED;
+}
+
+static int __init p2020ds_ngpixis_init(void)
+{
+	int event_irq, ret;
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,p2020ds-fpga");
+	if (np) {
+		event_irq = irq_of_parse_and_map(np, 0);
+		ret = request_irq(event_irq, event_isr, 0, "event", NULL);
+		if (ret)
+			printk(KERN_ERR "Can't request board event int\n");
+		of_node_put(np);
+	}
+	return 0;
+}
+machine_device_initcall(p2020_ds, p2020ds_ngpixis_init);
+
 /*
  * Called very early, device-tree isn't unflattened
  */