Re: [PATCH v12 4/6] fbmon: add of_videomode helpers

Hi Tomi,

On Wednesday 21 November 2012 14:49:30 Tomi Valkeinen wrote:

On 2012-11-20 17:54, Steffen Trumtrar wrote:

quoted

Add helper to get fb_videomode from devicetree.

Signed-off-by: Steffen Trumtrar <redacted>
Reviewed-by: Thierry Reding <redacted>
Acked-by: Thierry Reding <redacted>
Tested-by: Thierry Reding <redacted>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

 drivers/video/fbmon.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/fb.h    |    7 +++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 920cbe3..41b5e49 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h

@@ -15,6 +15,8 @@

 #include <linux/slab.h>
 #include <asm/io.h>
 #include <linux/videomode.h>

+#include <linux/of.h>
+#include <linux/of_videomode.h>

Guess what? =)

To be honest, I don't know what the general opinion is about including
header files from header files. But I always leave them out if they are
not strictly needed.

I agree, I favor structure declaration as well when possible.

quoted

 struct vm_area_struct;
 struct fb_info;

@@ -715,6 +717,11 @@ 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 IS_ENABLED(CONFIG_OF_VIDEOMODE)
+extern int of_get_fb_videomode(const struct device_node *np,
+			       struct fb_videomode *fb,
+			       unsigned int index);
+#endif

 #if IS_ENABLED(CONFIG_VIDEOMODE)
 extern int fb_videomode_from_videomode(const struct videomode *vm,
 
 				       struct fb_videomode *fbmode);

Do you really need these #ifs in the header files? They do make it look
a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not
enabled, he'll get a linker error anyway.

-- 
Regards,

Laurent Pinchart