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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

Hi Tomi,

On Wednesday 21 November 2012 14:49:30 Tomi Valkeinen wrote:
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add helper to get fb_videomode from devicetree.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> > 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.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]