Re: PCI driver

[Manu Abraham <manu@linuxtv.org>]

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