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

2013-01-07 Thread 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  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

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

2013-01-07 Thread Rob Clark 
On Mon, Jan 7, 2013 at 2:46 AM, Mohammed, Afzal  wrote:
> Hi Steffen,
>
> On Mon, Jan 07, 2013 at 13:36:48, Steffen Trumtrar wrote:
>> On Mon, Jan 07, 2013 at 06:10:13AM +, 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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-07 Thread Mohammed, Afzal 
Hi Steffen,

On Mon, Jan 07, 2013 at 13:36:48, Steffen Trumtrar wrote:
> On Mon, Jan 07, 2013 at 06:10:13AM +, 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

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

2013-01-07 Thread Steffen Trumtrar 
Hi Afzal,

On Mon, Jan 07, 2013 at 06:10:13AM +, 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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-06 Thread Mohammed, Afzal 
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<--


> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)

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

#ifdef CONFIG_OF_VIDEOMODE

sufficient ?

Regards
Afzal

[PATCHv16 5/7] fbmon: add of_videomode helpers

2012-12-18 Thread Steffen Trumtrar 
Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar 
Reviewed-by: Thierry Reding 
Acked-by: Thierry Reding 
Tested-by: Thierry Reding 
Tested-by: Philipp Zabel 
Reviewed-by: Laurent Pinchart 
Acked-by: Laurent Pinchart 
---
 drivers/video/fbmon.c |   42 ++
 include/linux/fb.h|4 
 2 files changed, 46 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 17ce135..94ad0f7 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #ifdef CONFIG_PPC_OF
 #include 
@@ -1425,6 +1426,47 @@ int fb_videomode_from_videomode(const struct videomode 
*vm,
 EXPORT_SYMBOL_GPL(fb_videomode_from_videomode);
 #endif
 
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static inline void dump_fb_videomode(const struct fb_videomode *m)
+{
+   pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u 
%u\n",
+m->xres, m->yres, m->refresh, m->pixclock, m->left_margin,
+m->right_margin, m->upper_margin, m->lower_margin,
+m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag);
+}
+
+/**
+ * of_get_fb_videomode - get a fb_videomode from devicetree
+ * @np: device_node with the timing specification
+ * @fb: will be set to the return value
+ * @index: index into the list of display timings in devicetree
+ *
+ * DESCRIPTION:
+ * This function is expensive and should only be used, if only one mode is to 
be
+ * read from DT. To get multiple modes start with of_get_display_timings ond
+ * work with that instead.
+ */
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
+   int index)
+{
+   struct videomode vm;
+   int ret;
+
+   ret = of_get_videomode(np, &vm, index);
+   if (ret)
+   return ret;
+
+   fb_videomode_from_videomode(&vm, fb);
+
+   pr_debug("%s: got %dx%d display mode from %s\n",
+   of_node_full_name(np), vm.hactive, vm.vactive, np->name);
+   dump_fb_videomode(fb);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_fb_videomode);
+#endif
+
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 100a176..58b9860 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -20,6 +20,7 @@ struct fb_info;
 struct device;
 struct file;
 struct videomode;
+struct device_node;
 
 /* Definitions below are used in the parsed monitor specs */
 #define FB_DPMS_ACTIVE_OFF 1
@@ -715,6 +716,9 @@ 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);
 
+extern int of_get_fb_videomode(struct device_node *np,
+  struct fb_videomode *fb,
+  int index);
 extern int fb_videomode_from_videomode(const struct videomode *vm,
   struct fb_videomode *fbmode);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv16 5/7] fbmon: add of_videomode helpers

2012-12-18 Thread Steffen Trumtrar 
Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar 
Reviewed-by: Thierry Reding 
Acked-by: Thierry Reding 
Tested-by: Thierry Reding 
Tested-by: Philipp Zabel 
Reviewed-by: Laurent Pinchart 
Acked-by: Laurent Pinchart 
---
 drivers/video/fbmon.c |   42 ++
 include/linux/fb.h|4 
 2 files changed, 46 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 17ce135..94ad0f7 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #ifdef CONFIG_PPC_OF
 #include 
@@ -1425,6 +1426,47 @@ int fb_videomode_from_videomode(const struct videomode 
*vm,
 EXPORT_SYMBOL_GPL(fb_videomode_from_videomode);
 #endif
 
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+static inline void dump_fb_videomode(const struct fb_videomode *m)
+{
+   pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u 
%u\n",
+m->xres, m->yres, m->refresh, m->pixclock, m->left_margin,
+m->right_margin, m->upper_margin, m->lower_margin,
+m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag);
+}
+
+/**
+ * of_get_fb_videomode - get a fb_videomode from devicetree
+ * @np: device_node with the timing specification
+ * @fb: will be set to the return value
+ * @index: index into the list of display timings in devicetree
+ *
+ * DESCRIPTION:
+ * This function is expensive and should only be used, if only one mode is to 
be
+ * read from DT. To get multiple modes start with of_get_display_timings ond
+ * work with that instead.
+ */
+int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
+   int index)
+{
+   struct videomode vm;
+   int ret;
+
+   ret = of_get_videomode(np, &vm, index);
+   if (ret)
+   return ret;
+
+   fb_videomode_from_videomode(&vm, fb);
+
+   pr_debug("%s: got %dx%d display mode from %s\n",
+   of_node_full_name(np), vm.hactive, vm.vactive, np->name);
+   dump_fb_videomode(fb);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_fb_videomode);
+#endif
+
 #else
 int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 100a176..58b9860 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -20,6 +20,7 @@ struct fb_info;
 struct device;
 struct file;
 struct videomode;
+struct device_node;
 
 /* Definitions below are used in the parsed monitor specs */
 #define FB_DPMS_ACTIVE_OFF 1
@@ -715,6 +716,9 @@ 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);
 
+extern int of_get_fb_videomode(struct device_node *np,
+  struct fb_videomode *fb,
+  int index);
 extern int fb_videomode_from_videomode(const struct videomode *vm,
   struct fb_videomode *fbmode);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html