Re: PCI driver

[Manu Abraham]

Jiri Slaby wrote:

> On 10/10/05, Manu Abraham <manu@xxxxxxxxxxx> wrote:
>  
>
> > Jiri Slaby wrote:
> >    
> >
> > > 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 ?
> >    
> Divide strings not "text blabalba \
>         continue"
> but with better "text blablabalasjdl"
>         "continue"
>  

The dprintk() macro (in mantis_common.h ) was looking very badly with 
wrap, which Andrew also commented on (about the col's) (the macro being 
the same, eventhough i was using it elsewhere) it being more than 80 
cols, but wrapping the macro made it look like hell.

> wrap line, if it is longer than 80 columns, near last comma, |, & and so on
> You seem to use some editor with tab to be less than 8 columns and
> some headers are indented bad because of it.
>  

My editor uses 8 columns only as a tab, Thunderbird does really handle 
things a bit different though.


> > 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..
> >
> >    
> >
> > > >      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
> >    
> And it is called only from places, where pdev is known (i.e. in
> parameter of function, e.g. mantis_pci_probe). So you don't need it to
> store in mantis, but only call mantis_dma_init(mantis, pdev). Read
> below.
>  
>
> > 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() .. ?
> >    
> But if you really want it, call pci_get_dev() and store it into mantis
> struct. In the _device_ exit routine call the latter. But I think,
> that not to store is better, or the best is to call
> mantis_dma_init(pdev) and do pci_get_drvdata inside.
>  

i think will pass (pdev) it as a function argument. Looks a bit more cleaner
But what i fail to understand is , if you can pass it as an argument, 
why can't you save the pointer in the struct ?

> > > 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.
> >    
> Kernel frees up the sections that won't be needed anymore. You put the
> function in some section by this.
>  
Ok,

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/