Re: [PATCH 7/8] powerpc/5200: Refactor mpc5200 interrupt controller driver

[Grant Likely <grant.likely@secretlab.ca>]

On Sun, Jan 25, 2009 at 1:06 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> Rework the mpc5200-pic driver to simplify it and fix up the setting
>> of desc->status when set_type is called for internal IRQs (so they
>> are reported as level, not edge).  The simplification is due to
>> splitting off the handling of external IRQs into a separate block
>> so they don't need to be handled as exceptions in the normal
>> CRIT, MAIN and PERP paths.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> CC: Wolfram Sang <w.sang@pengutronix.de>
>> ---
>>
>>  arch/powerpc/platforms/52xx/mpc52xx_pic.c |  145 ++++++++++++-----------------
>>  1 files changed, 58 insertions(+), 87 deletions(-)
>>
>>
>> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pic.c b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
>> index c0a9559..277c9c5 100644
>> --- a/arch/powerpc/platforms/52xx/mpc52xx_pic.c
>> +++ b/arch/powerpc/platforms/52xx/mpc52xx_pic.c
>> @@ -190,10 +190,10 @@ static void mpc52xx_extirq_ack(unsigned int virq)
>>
>>  static int mpc52xx_extirq_set_type(unsigned int virq, unsigned int flow_type)
>>  {
>> -     struct irq_desc *desc = get_irq_desc(virq);
>>       u32 ctrl_reg, type;
>>       int irq;
>>       int l2irq;
>> +     void *handler = handle_level_irq;
>>
>>       irq = irq_map[virq].hwirq;
>>       l2irq = irq & MPC52xx_IRQ_L2_MASK;
>> @@ -201,32 +201,21 @@ static int mpc52xx_extirq_set_type(unsigned int virq, unsigned int flow_type)
>>       pr_debug("%s: irq=%x. l2=%d flow_type=%d\n", __func__, irq, l2irq, flow_type);
>>
>>       switch (flow_type) {
>> -     case IRQF_TRIGGER_HIGH:
>> -             type = 0;
>> -             break;
>> -     case IRQF_TRIGGER_RISING:
>> -             type = 1;
>> -             break;
>> -     case IRQF_TRIGGER_FALLING:
>> -             type = 2;
>> -             break;
>> -     case IRQF_TRIGGER_LOW:
>> -             type = 3;
>> -             break;
>> +     case IRQF_TRIGGER_HIGH: type = 0; break;
>> +     case IRQF_TRIGGER_RISING: type = 1; handler = handle_edge_irq; break;
>> +     case IRQF_TRIGGER_FALLING: type = 2; handler = handle_edge_irq; break;
>> +     case IRQF_TRIGGER_LOW: type = 3; break;
>
> The Linux coding style tells us not to do that:
>
> http://lxr.linux.no/linux+v2.6.28.2/Documentation/CodingStyle#L60

In principle I agree and follow that rule most of the time, but I have
good reason for not choosing to do so here.

The whole point of coding style is to promote
readability/manageability.  ie. the 80 column rule is a very strong
guideline, but there are places where breaking that rule makes for
more readable code than breaking things up and it is left to the
discretion of the coder and the maintainer.

In this case I looked at the block of code and saw that a large number
of lines (11) were needed to do a set of nearly identical operations.
I chose to line them up because in my opinion it is easier to follow
the pattern with them written in horizontal columns instead of in
vertical blocks.  In other words, in this case it is harder to hide
something in the code written this way because it wouldn't match the
pattern of the other lines.  For the same reason you'll also notice
that I did *not* put all the statements on the same line for the
default case because it doesn't match the pattern of the specific
cases.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.