lkml.org 
[lkml]   [2005]   [Oct]   [10]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateMon, 10 Oct 2005 19:00:53 +0400
FromManu Abraham <>
SubjectRe: PCI driver
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

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

\
 
 \ /
  Last update: 2005-10-10 15:16    [from the cache]
©2003-2008