Re: PCI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jiri Slaby wrote:

On 10/10/05, Manu Abraham <[email protected]> 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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux