viafb: Move core stuff into via-core.c
in [Kernel]
|
From: Jonathan Corbet on 1 May 2010 11:10 On Sat, 01 May 2010 17:02:30 +0200 Florian Tobias Schandinat <FlorianSchandinat(a)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. > > @@ -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? Thanks, jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Florian Tobias Schandinat on 1 May 2010 11:10 Jonathan Corbet schrieb: > The first step toward turning viafb into a multifunction driver. This > patch creates a new via-core.c file which serves as the main PCI driver; > everything else comes below that. Some work has been done to rationalize > the i2c drivers in this new scheme. > > Signed-off-by: Jonathan Corbet <corbet(a)lwn.net> > --- > -obj-$(CONFIG_FB_VIA) += viafb.o > +obj-$(CONFIG_FB_VIA) += viafb.o Huh? (the space is not really a problem) > 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. > @@ -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. Thanks, Florian Tobias Schandinat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Florian Tobias Schandinat on 1 May 2010 11:40 Jonathan Corbet schrieb: > On Sat, 01 May 2010 17:02:30 +0200 > Florian Tobias Schandinat <FlorianSchandinat(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Pages: 1 Prev: [PATCH v21 012/100] c/r: extend arch_setup_additional_pages() Next: (none) |