diff mbox

[PATCHv16,5/7] fbmon: add of_videomode helpers

Message ID C8443D0743D26F4388EA172BF4E2A7A93EA7FB02@DBDE01.ent.ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Afzal Mohammed Jan. 7, 2013, 6:10 a.m. UTC 
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<----------

---8<----------


> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)


As _OF_VIDEOMODE is a bool type CONFIG, isn't,

#ifdef CONFIG_OF_VIDEOMODE

sufficient ?

Regards
Afzal

Comments

Steffen Trumtrar Jan. 7, 2013, 8:06 a.m. UTC | #1 
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
Afzal Mohammed Jan. 7, 2013, 8:46 a.m. UTC | #2 
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
Rob Clark Jan. 7, 2013, 8:06 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Afzal Mohammed Jan. 8, 2013, 5:31 a.m. UTC | #4 
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 mbox

Patch

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);