Re: PCI driver

[Manu Abraham]

Jiri Slaby wrote:

> On 10/10/05, Manu Abraham <manu@xxxxxxxxxxx> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/