[PATCHv5,1/5] fbdev: Add the lacking FB_SYNC_* for matching the DISPLAY_FLAGS_*

Without that fix, drivers using the fb_videomode_from_videomode
  function will not be able to get certain information because
  some DISPLAY_FLAGS_* have no corresponding FB_SYNC_*.

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
ChangeLog v4->v5:
- Added Geert Uytterhoeven, Grant Likely and Shawn Guo in the Cc list.
- Moved the definitions to a non-userspace header.
- Corrected the comment of the second define.

ChangeLog v3->v4:
- Fixed the issue with FB_SYNC_PIXDAT_HIGH_ACT value.

ChangeLog v2->v3:
- Added Jean-Christophe PLAGNIOL-VILLARD's ACK.
---
 drivers/video/fbmon.c |    4 ++++
 include/linux/fb.h    |    3 +++
 2 files changed, 7 insertions(+)

On 2013-11-08 15:01, Denis Carikli wrote:
> Without that fix, drivers using the fb_videomode_from_videomode
>   function will not be able to get certain information because
>   some DISPLAY_FLAGS_* have no corresponding FB_SYNC_*.

> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index ffac70a..cf2ad5d 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -45,6 +45,9 @@ struct device_node;
>  #define FB_SIGNAL_SYNC_ON_GREEN	8
>  #define FB_SIGNAL_SERRATION_ON	16
>  
> +#define FB_SYNC_DE_HIGH_ACT     64      /* data enable active high flag */
> +#define FB_SYNC_PIXDAT_HIGH_ACT 128     /* drive data on positive edge */
> +
>  #define FB_MISC_PRIM_COLOR	1
>  #define FB_MISC_1ST_DETAIL	2	/* First Detailed Timing is preferred */
>  struct fb_chroma {

I don't think this is better than the previous version where
FB_SYNC_DE_HIGH_ACT and FB_SYNC_PIXDAT_HIGH_ACT were in
include/uapi/linux/fb.h. Now those flag defines are not visible to the
userspace, but the actual flags are still visible from the var->sync field.

It's true what Russell replied to the previous version, that the
userspace has no idea how to handle those new flags. But then again, for
LCDs, the userspace has no idea how to handle, say, hsync polarity either.

In any case, splitting the FB_SYNC_ defines into uapi and
kernel-internal header files, but still giving the kernel-internal
values to userspace is surely wrong.

 Tomi

On Fri, Jan 10, 2014 at 02:37:40PM +0200, Tomi Valkeinen wrote:
> I don't think this is better than the previous version where
> FB_SYNC_DE_HIGH_ACT and FB_SYNC_PIXDAT_HIGH_ACT were in
> include/uapi/linux/fb.h. Now those flag defines are not visible to the
> userspace, but the actual flags are still visible from the var->sync field.
> 
> It's true what Russell replied to the previous version, that the
> userspace has no idea how to handle those new flags. But then again, for
> LCDs, the userspace has no idea how to handle, say, hsync polarity either.
> 
> In any case, splitting the FB_SYNC_ defines into uapi and
> kernel-internal header files, but still giving the kernel-internal
> values to userspace is surely wrong.

The difference between the sync states and these other flags is that
the sync states are part of the mode definition - as per the CEA-861
documentation.

However, that does not extend to LCD panels, which generally need to
have one set of timings, with the various control signals at certain
polarities - and those control signal polarities are a property of
the panel itself, not of the displayed mode.  So, if you know that
you have LCD panel X, and it needs control signals with a certain
polarity, that is how you configure the hardware _irrespective_ of
what userspace says that the sync states should be.

The sync states specified by userspace should of course be used for
the sync pulses being sent to a VGA display or HDMI sink.