| Message ID | C8443D0743D26F4388EA172BF4E2A7A93EA7FB02@DBDE01.ent.ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Afzal, On Mon, Jan 07, 2013 at 06:10:13AM +0000, Mohammed, Afzal wrote: > Hi Steffen, > > On Tue, Dec 18, 2012 at 22:34:14, Steffen Trumtrar wrote: > > Add helper to get fb_videomode from devicetree. > > > drivers/video/fbmon.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/fb.h | 4 ++++ > > This breaks DaVinci (da8xx_omapl_defconfig), following change was > required to get it build if OF_VIDEOMODE or/and FB_MODE_HELPERS > is not defined. There may be better solutions, following was the > one that was used by me to test this series. > > ---8<---------- > > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 58b9860..0ce30d1 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -716,9 +716,19 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); > extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); > extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter); > > +#if defined(CONFIG_OF_VIDEOMODE) && defined(CONFIG_FB_MODE_HELPERS) > extern int of_get_fb_videomode(struct device_node *np, > struct fb_videomode *fb, > int index); > +#else > +static inline int of_get_fb_videomode(struct device_node *np, > + struct fb_videomode *fb, > + int index) > +{ > + return -EINVAL; > +} > +#endif > + > extern int fb_videomode_from_videomode(const struct videomode *vm, > struct fb_videomode *fbmode); > > ---8<---------- > I just did a quick "make da8xx_omapl_defconfig && make" and it builds just fine. On what version did you apply the series? At the moment I have the series sitting on 3.7. Didn't try any 3.8-rcx yet. But fixing this shouldn't be a problem. > > > +#if IS_ENABLED(CONFIG_OF_VIDEOMODE) > > As _OF_VIDEOMODE is a bool type CONFIG, isn't, > > #ifdef CONFIG_OF_VIDEOMODE > > sufficient ? > Yes, that is right. But I think IS_ENABLED is the preferred way to do it, isn't it? Regards, Steffen
Hi Steffen, On Mon, Jan 07, 2013 at 13:36:48, Steffen Trumtrar wrote: > On Mon, Jan 07, 2013 at 06:10:13AM +0000, Mohammed, Afzal wrote: > > This breaks DaVinci (da8xx_omapl_defconfig), following change was > > required to get it build if OF_VIDEOMODE or/and FB_MODE_HELPERS > > is not defined. There may be better solutions, following was the > > one that was used by me to test this series. > I just did a quick "make da8xx_omapl_defconfig && make" and it builds just fine. > On what version did you apply the series? > At the moment I have the series sitting on 3.7. Didn't try any 3.8-rcx yet. > But fixing this shouldn't be a problem. You are right, me idiot, error will happen only upon try to make use of of_get_fb_videomode() (defined in this patch) in the da8xx-fb driver (with da8xx_omapl_defconfig), to be exact upon adding, "video: da8xx-fb: obtain fb_videomode info from dt" of my patch series. The change as I mentioned or something similar would be required as any driver that is going to make use of of_get_fb_videomode() would break if CONFIG_OF_VIDEOMODE or CONFIG_FB_MODE_HELPERS is not defined. And testing was done over v3.8-rc2. > > > +#if IS_ENABLED(CONFIG_OF_VIDEOMODE) > > > > As _OF_VIDEOMODE is a bool type CONFIG, isn't, > > > > #ifdef CONFIG_OF_VIDEOMODE > > > > sufficient ? > > > > Yes, that is right. But I think IS_ENABLED is the preferred way to do it, isn't it? Now I realize it is. Regards Afzal
On Mon, Jan 7, 2013 at 2:46 AM, Mohammed, Afzal <afzal@ti.com> wrote: > Hi Steffen, > > On Mon, Jan 07, 2013 at 13:36:48, Steffen Trumtrar wrote: >> On Mon, Jan 07, 2013 at 06:10:13AM +0000, Mohammed, Afzal wrote: > >> > This breaks DaVinci (da8xx_omapl_defconfig), following change was >> > required to get it build if OF_VIDEOMODE or/and FB_MODE_HELPERS >> > is not defined. There may be better solutions, following was the >> > one that was used by me to test this series. > >> I just did a quick "make da8xx_omapl_defconfig && make" and it builds just fine. >> On what version did you apply the series? >> At the moment I have the series sitting on 3.7. Didn't try any 3.8-rcx yet. >> But fixing this shouldn't be a problem. > > You are right, me idiot, error will happen only upon try to make use of > of_get_fb_videomode() (defined in this patch) in the da8xx-fb driver > (with da8xx_omapl_defconfig), to be exact upon adding, > > "video: da8xx-fb: obtain fb_videomode info from dt" of my patch series. > > The change as I mentioned or something similar would be required as > any driver that is going to make use of of_get_fb_videomode() would > break if CONFIG_OF_VIDEOMODE or CONFIG_FB_MODE_HELPERS is not defined. Shouldn't the driver that depends on CONFIG_OF_VIDEOMODE and CONFIG_FB_MODE_HELPERS, explicitly select them? I don't really see the point of having the static-inline fallbacks. fwiw, using 'select' is what I was doing for lcd panel support for lcdc/da8xx drm driver (which was using the of videomode helpers, albeit a slightly earlier version of the patches): https://github.com/robclark/kernel-omap4/commit/e2aef5f281348afaaaeaa132699efc2831aa8384 BR, -R > > And testing was done over v3.8-rc2. > >> > > +#if IS_ENABLED(CONFIG_OF_VIDEOMODE) >> > >> > As _OF_VIDEOMODE is a bool type CONFIG, isn't, >> > >> > #ifdef CONFIG_OF_VIDEOMODE >> > >> > sufficient ? >> > >> >> Yes, that is right. But I think IS_ENABLED is the preferred way to do it, isn't it? > > Now I realize it is. > > Regards > Afzal
Hi Rob, On Tue, Jan 08, 2013 at 01:36:50, Rob Clark wrote: > On Mon, Jan 7, 2013 at 2:46 AM, Mohammed, Afzal <afzal@ti.com> wrote: > > On Mon, Jan 07, 2013 at 13:36:48, Steffen Trumtrar wrote: > >> I just did a quick "make da8xx_omapl_defconfig && make" and it builds just fine. > >> On what version did you apply the series? > >> At the moment I have the series sitting on 3.7. Didn't try any 3.8-rcx yet. > >> But fixing this shouldn't be a problem. > > The change as I mentioned or something similar would be required as > > any driver that is going to make use of of_get_fb_videomode() would > > break if CONFIG_OF_VIDEOMODE or CONFIG_FB_MODE_HELPERS is not defined. > Shouldn't the driver that depends on CONFIG_OF_VIDEOMODE and > CONFIG_FB_MODE_HELPERS, explicitly select them? I don't really see > the point of having the static-inline fallbacks. But here da8xx-fb driver does not depend on _OF_VIDEOMODE and _FB_MODE_HELPERS, currently it works as a pure platform driver for DaVinci SoC's without those CONFIG's. It is only upon enhancing the driver to make use of of_get_fb_videomode() for DT support those CONFIG's are being made use of. As the driver can work w/o these CONFIG's and so as it is not a dependency for driver on non-DT boot (as in the case of DaVinci), I disagree in selecting those options always, but rather giving user an option to select. And selecting these options always will bring in some amount of code onto Kernel image w/o any purpose in the case of DaVinci builds. Another option would be to sprinkle driver with ifdef's to avoid inline fallbacks, which is not a good thing to do. Moreover having a static inline fallback is more in line with other of_*'s. > fwiw, using 'select' is what I was doing for lcd panel support for > lcdc/da8xx drm driver (which was using the of videomode helpers, > albeit a slightly earlier version of the patches): In your case as it is a new driver & is meant only for DT, that is fine, but here it is an existing driver that works w/o these. Regards Afzal
diff --git a/include/linux/fb.h b/include/linux/fb.h index 58b9860..0ce30d1 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -716,9 +716,19 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter); +#if defined(CONFIG_OF_VIDEOMODE) && defined(CONFIG_FB_MODE_HELPERS) extern int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, int index); +#else +static inline int of_get_fb_videomode(struct device_node *np, + struct fb_videomode *fb, + int index) +{ + return -EINVAL; +} +#endif + extern int fb_videomode_from_videomode(const struct videomode *vm, struct fb_videomode *fbmode);