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