[PATCHv16 5/7] fbmon: add of_videomode helpers

[Mohammed, 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 at 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