[7/7] P2020ds: add event button handler

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

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

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

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