Re: PCI driver

Manu Abraham
2005-10-10T15:13:55Z

Jiri Slaby wrote:

>On 10/10/05, Manu Abraham <manu@linuxtv.org> wrote:
>
>
>>I have fixed most of the stuff, it is partly working, not ready yet as
>>there are some more things to be added to ..
>>I have attached what i was working on.
>>
>>
>I'm not so bad, I help people (but not 10 times with the same point),
>fuck it and forget.
>
>
>
presumption is not always good, and it is very clear. Most of the
flamewars blaze up because of that. Anyway let's not hover on that ..

>Some points to the new driver:
>indentation isn't so good, the code is not readable well (80 chars on
>a line mainly).
>
>
>

I do agree that indentation is not so good, but how would you suggest
the code be wrapped ?

The problem with wrapping is that readability goes down horribly, but
while debugging a driver, this is too painful.
Considering that it has a long way to go still..

>>static irqreturn_t mantis_pci_irq(int irq, void *dev_id, struct pt_regs *regs)
>>{
>> int count = 0, interrupts = 0;
>> u32 stat = 0, mask = 0, temp;
>>
>> struct mantis_pci *mantis;
>>
>> mantis = (struct mantis_pci *) dev_id;
>>
>>
>you don't need to cast from void *
>struct mantis_pci *mantis = dev_id; is enough
>
>
>

Thanks, Ack'd.

>> mantis->pdev = pdev;
>>
>>
>If you work with this out from pci functions, you should call
>pci_get_dev and in exit function pci_dev_put, otherwise you don't need
>it at all.
>
>
>
Well i am using it in mantis_dma.c, pci_alloc/free

You mean rather than saving off the pointer, i do a pci_get_dev()
and later on in the exit routine, i do a pci_dev_put() .. ?

>mantis_dma_init and others could be __devinit too, or not? Try to use
>it as much as possible.
>
>
>

Any thoughts as to why ? I am not saying it is not needed, but i would
like to know what was the idea.

>>static struct pci_driver mantis_pci_driver = {
>> .name = DRIVER_NAME,
>> .id_table = mantis_pci_table,
>> .probe = mantis_pci_probe,
>> .remove = mantis_pci_remove,
>>
>>
>here should be = __devexit_p(...),
>
>

Ack'd, Thanks ..

>>};
>>
>>
>
>
>
>>err0:
>>
>>
>you should change naming of labels such as errfree, where do you do kfree etc.
>
>
>
Ah , okay.

Thanks,
Manu