linux-mips
[Top] [All Lists]

Re: [RFC][PATCH 04/10] bcma: add mips driver

To: Rafał Miłecki <zajec5@gmail.com>
Subject: Re: [RFC][PATCH 04/10] bcma: add mips driver
From: Hauke Mehrtens <hauke@hauke-m.de>
Date: Tue, 07 Jun 2011 00:06:36 +0200
Cc: linux-wireless@vger.kernel.org, linux-mips@linux-mips.org, mb@bu3sch.de, george@znau.edu.ua, arend@broadcom.com, b43-dev@lists.infradead.org, bernhardloos@googlemail.com
In-reply-to: <BANLkTim_TtNVmmyH5J3G0pK-vrWNL1+24A@mail.gmail.com>
References: <1307311658-15853-1-git-send-email-hauke@hauke-m.de> <1307311658-15853-5-git-send-email-hauke@hauke-m.de> <BANLkTim_TtNVmmyH5J3G0pK-vrWNL1+24A@mail.gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110424 Lightning/1.0b2 Thunderbird/3.1.10
On 06/06/2011 01:23 PM, Rafał Miłecki wrote:
> 2011/6/6 Hauke Mehrtens <hauke@hauke-m.de>:
>> +/* driver_mips.c */
>> +extern unsigned int bcma_core_mips_irq(struct bcma_device *dev);
> 
> Does it compile without CONFIG_BCMA_DRIVER_MIPS?
No ;-) Thought about it after sending these patches, some other patches
will have the same problem.
> 
> 
>> +/* Get the MIPS IRQ assignment for a specified device.
>> + * If unassigned, 0 is returned.
>> + * If disabled, 5 is returned.
>> + * If not supported, 6 is returned.
>> + */
> 
> Does it ever return 6?
Some old comment, will fix this.
> 
>> +unsigned int bcma_core_mips_irq(struct bcma_device *dev)
>> +{
>> +       struct bcma_device *mdev = dev->bus->drv_mips.core;
>> +       u32 irqflag;
>> +       unsigned int irq;
>> +
>> +       irqflag = bcma_core_mips_irqflag(dev);
>> +
>> +       for (irq = 1; irq <= 4; irq++)
>> +               if (bcma_read32(mdev, BCMA_MIPS_MIPS74K_INTMASK(irq)) & (1 
>> << irqflag))
>> +                       break;
> 
> Use scripts/checkpatch*. Braces around "for" and split line to match
> 80 chars width.
Will check all patches with scripts/checkpatch.sh
> 
> Why don't you just use "return irq;" instead of break?
yes this will be better.
> 
> 
>> +
>> +       if (irq == 5)
>> +               irq = 0;
>> +
>> +       return irq;
> 
> You can just make it "return 0;" after changing break to return.
agree
> 
> 
>> +                       for (i = 0; i < bus->nr_cores; i++)
>> +                               if ((1 << 
>> bcma_core_mips_irqflag(&bus->cores[i])) == oldirqflag) {
>> +                                       
>> bcma_core_mips_set_irq(&bus->cores[i], 0);
>> +                                       break;
>> +                               }
> 
> Braces for "for".
Is this needed after the coding guildlines? Shouldn't they be removed if
they are not needed?
> 
>> +       pr_info("after irq reconfiguration\n");
> 
> Make first letter uppercase. I'm not English expert, but doesn't
> something like "IRQ reconfiguration done" sound better?
> 
Sounds better.

<Prev in Thread] Current Thread [Next in Thread>