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

Florian Tobias Schandinat
2010-05-01T15:29:47Z

Jonathan Corbet schrieb:
> On Sat, 01 May 2010 17:02:30 +0200
> Florian Tobias Schandinat <FlorianSchandinat@gmx.de> 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