Re: [PATCH 12/30] viafb: Move core stuff into via-core.c

[Florian Tobias Schandinat]

Jonathan Corbet schrieb:
> On Sat, 01 May 2010 17:02:30 +0200
> Florian Tobias Schandinat <FlorianSchandinat@xxxxxx> wrote:
>
> > >  struct fb_info *viafbinfo;
> > > +EXPORT_SYMBOL_GPL(viafbinfo);
> > >  struct fb_info *viafbinfo1;
> > >  struct viafb_par *viaparinfo;
> > > +EXPORT_SYMBOL_GPL(viaparinfo);
> > >  struct viafb_par *viaparinfo1;  
> > Ugh, I really hope you introduce those only as temporary exports until 
> > the split is finished. It's ugly enough that viafb uses these internally 
> > as global variables which will vanish in some time but for a 
> > multifunction driver having those sounds even more ridiculous. If we 
> > agree that it's only a temporary solution I'll take this bitter pill.
>
> No we don't agree...  what we're seeing here is some history that I did
> not succeed in getting rid of entirely.  Those exports have no reason
> to exist anymore and shouldn't have slipped through into that patch.  I
> will most certainly make them go away.

That's even better.

> > > @@ -1764,6 +1765,7 @@ static int __devinit via_pci_probe(struct pci_dev *pdev,
> > >  		&viaparinfo->shared->lvds_setting_info2;
> > >  	viaparinfo->crt_setting_info = &viaparinfo->shared->crt_setting_info;
> > >  	viaparinfo->chip_info = &viaparinfo->shared->chip_info;
> > > +	spin_lock_init(&viaparinfo->reg_lock);  
> > I think the initialization of the lock that is made for synchronization 
> > of hardware access should be in the via-core.c you just introduce. (and 
> >   the lock itself in a structure or something outside the framebuffer 
> > flow). Just saw that you did so in your next patch, so there is no 
> > reason to needlessly introduce the spinlock now.
>
> As you note, it's only there for one step in the series, and no
> electrons are harmed in the process.  Is this really worth the trouble
> of changing?

Well I only noticed it just before sending. Not needlessly changing code 
would make the patches simpler but as I know now what is going on I 
don't insist on changing it.


Thanks,

Florian Tobias Schandinat
--
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/