[PATCH v12 0/6] of: add display helper

[PATCH v12 0/6] of: add display helper

[Steffen Trumtrar]

Hi!

Changes since v11:
	- make pointers const where applicable
	- add reviewed-by Laurent Pinchart

Regards,
Steffen


Steffen Trumtrar (6):
  video: add display_timing and videomode
  video: add of helper for videomode
  fbmon: add videomode helpers
  fbmon: add of_videomode helpers
  drm_modes: add videomode helpers
  drm_modes: add of_videomode helpers

 .../devicetree/bindings/video/display-timings.txt  |  107 ++++++++++
 drivers/gpu/drm/drm_modes.c                        |   70 +++++++
 drivers/video/Kconfig                              |   19 ++
 drivers/video/Makefile                             |    4 +
 drivers/video/display_timing.c                     |   24 +++
 drivers/video/fbmon.c                              |   86 ++++++++
 drivers/video/of_display_timing.c                  |  216 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   48 +++++
 drivers/video/videomode.c                          |   46 +++++
 include/drm/drmP.h                                 |   12 ++
 include/linux/display_timing.h                     |   70 +++++++
 include/linux/fb.h                                 |   13 ++
 include/linux/of_display_timings.h                 |   20 ++
 include/linux/of_videomode.h                       |   18 ++
 include/linux/videomode.h                          |   40 ++++
 15 files changed, 793 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display_timing.h
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h
 create mode 100644 include/linux/videomode.h

-- 
1.7.10.4

[PATCH v12 1/6] video: add display_timing and videomode

[Steffen Trumtrar]

Add display_timing structure and the according helper functions. This allows
the description of a display via its supported timing parameters.

Every timing parameter can be specified as a single value or a range
<min typ max>.

Also, add helper functions to convert from display timings to a generic videomode
structure. This videomode can then be converted to the corresponding subsystem
mode representation (e.g. fb_videomode).

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/Kconfig          |    6 ++++
 drivers/video/Makefile         |    2 ++
 drivers/video/display_timing.c |   24 ++++++++++++++
 drivers/video/videomode.c      |   46 ++++++++++++++++++++++++++
 include/linux/display_timing.h |   70 ++++++++++++++++++++++++++++++++++++++++
 include/linux/videomode.h      |   40 +++++++++++++++++++++++
 6 files changed, 188 insertions(+)
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/linux/display_timing.h
 create mode 100644 include/linux/videomode.h

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d08d799..2a23b18 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,12 @@ config VIDEO_OUTPUT_CONTROL
 	  This framework adds support for low-level control of the video 
 	  output switch.
 
+config DISPLAY_TIMING
+       bool
+
+config VIDEOMODE
+       bool
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 23e948e..fc30439 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -167,3 +167,5 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
+obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_VIDEOMODE) += videomode.o
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
new file mode 100644
index 0000000..ac9bbbc
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,24 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/display_timing.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+
+void display_timings_release(struct display_timings *disp)
+{
+	if (disp->timings) {
+		unsigned int i;
+
+		for (i = 0; i < disp->num_timings; i++)
+			kfree(disp->timings[i]);
+		kfree(disp->timings);
+	}
+	kfree(disp);
+}
+EXPORT_SYMBOL_GPL(display_timings_release);
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 0000000..e24f879
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,46 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/display_timing.h>
+#include <linux/kernel.h>
+#include <linux/videomode.h>
+
+int videomode_from_timing(const struct display_timings *disp,
+			  struct videomode *vm, unsigned int index)
+{
+	struct display_timing *dt;
+
+	dt = display_timings_get(disp, index);
+	if (!dt)
+		return -EINVAL;
+
+	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
+	vm->hactive = display_timing_get_value(&dt->hactive, 0);
+	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
+	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
+	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
+
+	vm->vactive = display_timing_get_value(&dt->vactive, 0);
+	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
+	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
+	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
+
+	vm->vah = dt->vsync_pol_active;
+	vm->hah = dt->hsync_pol_active;
+	vm->de = dt->de_pol_active;
+	vm->pixelclk_pol = dt->pixelclk_pol;
+
+	vm->interlaced = dt->interlaced;
+	vm->doublescan = dt->doublescan;
+
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(videomode_from_timing);
diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
new file mode 100644
index 0000000..d5bf03f
--- /dev/null
+++ b/include/linux/display_timing.h
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_TIMINGS_H
+#define __LINUX_DISPLAY_TIMINGS_H
+
+#include <linux/types.h>
+
+struct timing_entry {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+struct display_timing {
+	struct timing_entry pixelclock;
+
+	struct timing_entry hactive;
+	struct timing_entry hfront_porch;
+	struct timing_entry hback_porch;
+	struct timing_entry hsync_len;
+
+	struct timing_entry vactive;
+	struct timing_entry vfront_porch;
+	struct timing_entry vback_porch;
+	struct timing_entry vsync_len;
+
+	unsigned int vsync_pol_active;
+	unsigned int hsync_pol_active;
+	unsigned int de_pol_active;
+	unsigned int pixelclk_pol;
+	bool interlaced;
+	bool doublescan;
+};
+
+struct display_timings {
+	unsigned int num_timings;
+	unsigned int native_mode;
+
+	struct display_timing **timings;
+};
+
+/*
+ * placeholder function until ranges are really needed
+ * the index parameter should then be used to select one of [min typ max]
+ */
+static inline u32 display_timing_get_value(const struct timing_entry *te,
+					   unsigned int index)
+{
+	return te->typ;
+}
+
+static inline struct display_timing *display_timings_get(const struct
+							 display_timings *disp,
+							 unsigned int index)
+{
+	if (disp->num_timings > index)
+		return disp->timings[index];
+	else
+		return NULL;
+}
+
+void display_timings_release(struct display_timings *disp);
+
+#endif
diff --git a/include/linux/videomode.h b/include/linux/videomode.h
new file mode 100644
index 0000000..5d3e796
--- /dev/null
+++ b/include/linux/videomode.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * generic videomode description
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_VIDEOMODE_H
+#define __LINUX_VIDEOMODE_H
+
+#include <linux/display_timing.h>
+
+struct videomode {
+	u32 pixelclock;
+	u32 refreshrate;
+
+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	u32 hah;
+	u32 vah;
+	u32 de;
+	u32 pixelclk_pol;
+
+	bool interlaced;
+	bool doublescan;
+};
+
+int videomode_from_timing(const struct display_timings *disp,
+			  struct videomode *vm, unsigned int index);
+
+#endif
-- 
1.7.10.4

Re: [PATCH v12 1/6] video: add display_timing and videomode

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 6696 bytes --]

Hi,

On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add display_timing structure and the according helper functions. This allows
> the description of a display via its supported timing parameters.
> 
> Every timing parameter can be specified as a single value or a range
> <min typ max>.
> 
> Also, add helper functions to convert from display timings to a generic videomode
> structure. This videomode can then be converted to the corresponding subsystem
> mode representation (e.g. fb_videomode).

Sorry for reviewing this so late.

One thing I'd like to see is some explanation of the structs involved.
For example, in this patch you present structs videomode, display_timing
and display_timings without giving any hint what they represent.

I'm not asking for you to write a long documentation, but perhaps the
header files could include a few lines of comments above the structs,
explaining the idea.

> +void display_timings_release(struct display_timings *disp)
> +{
> +	if (disp->timings) {
> +		unsigned int i;
> +
> +		for (i = 0; i < disp->num_timings; i++)
> +			kfree(disp->timings[i]);
> +		kfree(disp->timings);
> +	}
> +	kfree(disp);
> +}
> +EXPORT_SYMBOL_GPL(display_timings_release);

Perhaps this will become clearer after reading the following patches,
but it feels a bit odd to add a release function, without anything in
this patch that would actually allocate the timings.

> diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> new file mode 100644
> index 0000000..e24f879
> --- /dev/null
> +++ b/drivers/video/videomode.c
> @@ -0,0 +1,46 @@
> +/*
> + * generic display timing functions
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/export.h>
> +#include <linux/errno.h>
> +#include <linux/display_timing.h>
> +#include <linux/kernel.h>
> +#include <linux/videomode.h>
> +
> +int videomode_from_timing(const struct display_timings *disp,
> +			  struct videomode *vm, unsigned int index)
> +{
> +	struct display_timing *dt;
> +
> +	dt = display_timings_get(disp, index);
> +	if (!dt)
> +		return -EINVAL;
> +
> +	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
> +	vm->hactive = display_timing_get_value(&dt->hactive, 0);
> +	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
> +	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
> +	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
> +
> +	vm->vactive = display_timing_get_value(&dt->vactive, 0);
> +	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
> +	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
> +	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);

Shouldn't all these calls get the typical value, with index 1?

> +
> +	vm->vah = dt->vsync_pol_active;
> +	vm->hah = dt->hsync_pol_active;
> +	vm->de = dt->de_pol_active;
> +	vm->pixelclk_pol = dt->pixelclk_pol;
> +
> +	vm->interlaced = dt->interlaced;
> +	vm->doublescan = dt->doublescan;
> +
> +	return 0;
> +}
> +
> +EXPORT_SYMBOL_GPL(videomode_from_timing);
> diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> new file mode 100644
> index 0000000..d5bf03f
> --- /dev/null
> +++ b/include/linux/display_timing.h
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * description of display timings
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_DISPLAY_TIMINGS_H
> +#define __LINUX_DISPLAY_TIMINGS_H
> +
> +#include <linux/types.h>

What is this needed for? u32? I don't see it defined in types.h

> +
> +struct timing_entry {
> +	u32 min;
> +	u32 typ;
> +	u32 max;
> +};
> +
> +struct display_timing {
> +	struct timing_entry pixelclock;
> +
> +	struct timing_entry hactive;
> +	struct timing_entry hfront_porch;
> +	struct timing_entry hback_porch;
> +	struct timing_entry hsync_len;
> +
> +	struct timing_entry vactive;
> +	struct timing_entry vfront_porch;
> +	struct timing_entry vback_porch;
> +	struct timing_entry vsync_len;
> +
> +	unsigned int vsync_pol_active;
> +	unsigned int hsync_pol_active;
> +	unsigned int de_pol_active;
> +	unsigned int pixelclk_pol;
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +struct display_timings {
> +	unsigned int num_timings;
> +	unsigned int native_mode;
> +
> +	struct display_timing **timings;
> +};
> +
> +/*
> + * placeholder function until ranges are really needed
> + * the index parameter should then be used to select one of [min typ max]
> + */
> +static inline u32 display_timing_get_value(const struct timing_entry *te,
> +					   unsigned int index)
> +{
> +	return te->typ;
> +}

Why did you opt for a placeholder here? It feels trivial to me to have
support to get the min/typ/max value properly.

> +static inline struct display_timing *display_timings_get(const struct
> +							 display_timings *disp,
> +							 unsigned int index)
> +{
> +	if (disp->num_timings > index)
> +		return disp->timings[index];
> +	else
> +		return NULL;
> +}
> +
> +void display_timings_release(struct display_timings *disp);
> +
> +#endif
> diff --git a/include/linux/videomode.h b/include/linux/videomode.h
> new file mode 100644
> index 0000000..5d3e796
> --- /dev/null
> +++ b/include/linux/videomode.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * generic videomode description
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_VIDEOMODE_H
> +#define __LINUX_VIDEOMODE_H
> +
> +#include <linux/display_timing.h>

This is not needed, just add:

struct display_timings;

> +struct videomode {
> +	u32 pixelclock;
> +	u32 refreshrate;
> +
> +	u32 hactive;
> +	u32 hfront_porch;
> +	u32 hback_porch;
> +	u32 hsync_len;
> +
> +	u32 vactive;
> +	u32 vfront_porch;
> +	u32 vback_porch;
> +	u32 vsync_len;
> +
> +	u32 hah;
> +	u32 vah;
> +	u32 de;
> +	u32 pixelclk_pol;
> +
> +	bool interlaced;
> +	bool doublescan;
> +};
> +
> +int videomode_from_timing(const struct display_timings *disp,
> +			  struct videomode *vm, unsigned int index);
> +

Are this and the few other functions above meant to be used from
drivers? If so, some explanation of the parameters here would be nice.
If they are just framework internal, they don't probably need that.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v12 1/6] video: add display_timing and videomode

[Steffen Trumtrar]

Hi,

On Wed, Nov 21, 2012 at 01:37:08PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add display_timing structure and the according helper functions. This allows
> > the description of a display via its supported timing parameters.
> > 
> > Every timing parameter can be specified as a single value or a range
> > <min typ max>.
> > 
> > Also, add helper functions to convert from display timings to a generic videomode
> > structure. This videomode can then be converted to the corresponding subsystem
> > mode representation (e.g. fb_videomode).
> 
> Sorry for reviewing this so late.
> 
> One thing I'd like to see is some explanation of the structs involved.
> For example, in this patch you present structs videomode, display_timing
> and display_timings without giving any hint what they represent.
> 
> I'm not asking for you to write a long documentation, but perhaps the
> header files could include a few lines of comments above the structs,
> explaining the idea.
> 

Okay. Will do.

> > +void display_timings_release(struct display_timings *disp)
> > +{
> > +	if (disp->timings) {
> > +		unsigned int i;
> > +
> > +		for (i = 0; i < disp->num_timings; i++)
> > +			kfree(disp->timings[i]);
> > +		kfree(disp->timings);
> > +	}
> > +	kfree(disp);
> > +}
> > +EXPORT_SYMBOL_GPL(display_timings_release);
> 
> Perhaps this will become clearer after reading the following patches,
> but it feels a bit odd to add a release function, without anything in
> this patch that would actually allocate the timings.
> 

2/6 uses this function. And as this does not belong to the DT part, it
is added in this patch.

> > diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> > new file mode 100644
> > index 0000000..e24f879
> > --- /dev/null
> > +++ b/drivers/video/videomode.c
> > @@ -0,0 +1,46 @@
> > +/*
> > + * generic display timing functions
> > + *
> > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#include <linux/export.h>
> > +#include <linux/errno.h>
> > +#include <linux/display_timing.h>
> > +#include <linux/kernel.h>
> > +#include <linux/videomode.h>
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > +			  struct videomode *vm, unsigned int index)
> > +{
> > +	struct display_timing *dt;
> > +
> > +	dt = display_timings_get(disp, index);
> > +	if (!dt)
> > +		return -EINVAL;
> > +
> > +	vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
> > +	vm->hactive = display_timing_get_value(&dt->hactive, 0);
> > +	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
> > +	vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
> > +	vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
> > +
> > +	vm->vactive = display_timing_get_value(&dt->vactive, 0);
> > +	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
> > +	vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
> > +	vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
> 
> Shouldn't all these calls get the typical value, with index 1?
> 

Yes. I omitted the indexing until now. So it didn't matter what value was used.
But I will integrate it in the next version.

> > +
> > +	vm->vah = dt->vsync_pol_active;
> > +	vm->hah = dt->hsync_pol_active;
> > +	vm->de = dt->de_pol_active;
> > +	vm->pixelclk_pol = dt->pixelclk_pol;
> > +
> > +	vm->interlaced = dt->interlaced;
> > +	vm->doublescan = dt->doublescan;
> > +
> > +	return 0;
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(videomode_from_timing);
> > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> > new file mode 100644
> > index 0000000..d5bf03f
> > --- /dev/null
> > +++ b/include/linux/display_timing.h
> > @@ -0,0 +1,70 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * description of display timings
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_DISPLAY_TIMINGS_H
> > +#define __LINUX_DISPLAY_TIMINGS_H
> > +
> > +#include <linux/types.h>
> 
> What is this needed for? u32? I don't see it defined in types.h
> 

Yes, u32. What would be the right header for that if not types.h?

> > +
> > +struct timing_entry {
> > +	u32 min;
> > +	u32 typ;
> > +	u32 max;
> > +};
> > +
> > +struct display_timing {
> > +	struct timing_entry pixelclock;
> > +
> > +	struct timing_entry hactive;
> > +	struct timing_entry hfront_porch;
> > +	struct timing_entry hback_porch;
> > +	struct timing_entry hsync_len;
> > +
> > +	struct timing_entry vactive;
> > +	struct timing_entry vfront_porch;
> > +	struct timing_entry vback_porch;
> > +	struct timing_entry vsync_len;
> > +
> > +	unsigned int vsync_pol_active;
> > +	unsigned int hsync_pol_active;
> > +	unsigned int de_pol_active;
> > +	unsigned int pixelclk_pol;
> > +	bool interlaced;
> > +	bool doublescan;
> > +};
> > +
> > +struct display_timings {
> > +	unsigned int num_timings;
> > +	unsigned int native_mode;
> > +
> > +	struct display_timing **timings;
> > +};
> > +
> > +/*
> > + * placeholder function until ranges are really needed
> > + * the index parameter should then be used to select one of [min typ max]
> > + */
> > +static inline u32 display_timing_get_value(const struct timing_entry *te,
> > +					   unsigned int index)
> > +{
> > +	return te->typ;
> > +}
> 
> Why did you opt for a placeholder here? It feels trivial to me to have
> support to get the min/typ/max value properly.
> 

I will add that in the next version.

> > +static inline struct display_timing *display_timings_get(const struct
> > +							 display_timings *disp,
> > +							 unsigned int index)
> > +{
> > +	if (disp->num_timings > index)
> > +		return disp->timings[index];
> > +	else
> > +		return NULL;
> > +}
> > +
> > +void display_timings_release(struct display_timings *disp);
> > +
> > +#endif
> > diff --git a/include/linux/videomode.h b/include/linux/videomode.h
> > new file mode 100644
> > index 0000000..5d3e796
> > --- /dev/null
> > +++ b/include/linux/videomode.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * generic videomode description
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_VIDEOMODE_H
> > +#define __LINUX_VIDEOMODE_H
> > +
> > +#include <linux/display_timing.h>
> 
> This is not needed, just add:
> 
> struct display_timings;
> 

Okay.

> > +struct videomode {
> > +	u32 pixelclock;
> > +	u32 refreshrate;
> > +
> > +	u32 hactive;
> > +	u32 hfront_porch;
> > +	u32 hback_porch;
> > +	u32 hsync_len;
> > +
> > +	u32 vactive;
> > +	u32 vfront_porch;
> > +	u32 vback_porch;
> > +	u32 vsync_len;
> > +
> > +	u32 hah;
> > +	u32 vah;
> > +	u32 de;
> > +	u32 pixelclk_pol;
> > +
> > +	bool interlaced;
> > +	bool doublescan;
> > +};
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > +			  struct videomode *vm, unsigned int index);
> > +
> 
> Are this and the few other functions above meant to be used from
> drivers? If so, some explanation of the parameters here would be nice.
> If they are just framework internal, they don't probably need that.
>

Okay. I will add some more documentation.

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

Re: [PATCH v12 1/6] video: add display_timing and videomode

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 460 bytes --]

On 2012-11-21 18:11, Steffen Trumtrar wrote:
> Hi,
> 
> On Wed, Nov 21, 2012 at 01:37:08PM +0200, Tomi Valkeinen wrote:

>>> +#include <linux/types.h>
>>
>> What is this needed for? u32? I don't see it defined in types.h
>>
> 
> Yes, u32. What would be the right header for that if not types.h?

Yes, it looks like u32 comes via linux/types.h. It's defined elsewhere,
but linux/types.h sounds like the most reasonable header for it.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v12 2/6] video: add of helper for videomode

[Steffen Trumtrar]

This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Stephen Warren <swarren@nvidia.com>
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>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 ++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  216 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   48 +++++
 include/linux/of_display_timings.h                 |   20 ++
 include/linux/of_videomode.h                       |   18 ++
 7 files changed, 424 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h

diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..a05cade
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@
+display-timings bindings
+========================
+
+display-timings node
+--------------------
+
+required properties:
+ - none
+
+optional properties:
+ - native-mode: The native mode for the display, in case multiple modes are
+		provided. When omitted, assume the first node is the native.
+
+timings subnode
+---------------
+
+required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock-frequency: display clock in Hz
+
+optional properties:
+ - hsync-active: Hsync pulse is active low/high/ignored
+ - vsync-active: Vsync pulse is active low/high/ignored
+ - de-active: Data-Enable pulse is active low/high/ignored
+ - pixelclk-inverted: pixelclock is inverted/non-inverted/ignored
+ - interlaced (bool)
+ - doublescan (bool)
+
+All the optional properties that are not bool follow the following logic:
+    <1>: high active
+    <0>: low active
+    omitted: not used on hardware
+
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the native-mode can be specified.
+
+The parameters are defined as
+
+struct display_timing
+=====================
+
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vback_porch                 |          |       |
+  |          |                ↓                            |          |       |
+  +----------###############################################----------+-------+
+  |          #                ↑                            #          |       |
+  |          #                |                            #          |       |
+  |  hback   #                |                            #  hfront  | hsync |
+  |   porch  #                |       hactive              #  porch   |  len  |
+  |<-------->#<---------------+--------------------------->#<-------->|<----->|
+  |          #                |                            #          |       |
+  |          #                |vactive                     #          |       |
+  |          #                |                            #          |       |
+  |          #                ↓                            #          |       |
+  +----------###############################################----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vfront_porch                |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+  |          |                ↑                            |          |       |
+  |          |                |vsync_len                   |          |       |
+  |          |                ↓                            |          |       |
+  +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+	display-timings {
+		native-mode = <&timing0>;
+		timing0: 1920p24 {
+			/* 1920x1080p24 */
+			clock-frequency = <52000000>;
+			hactive = <1920>;
+			vactive = <1080>;
+			hfront-porch = <25>;
+			hback-porch = <25>;
+			hsync-len = <25>;
+			vback-porch = <2>;
+			vfront-porch = <2>;
+			vsync-len = <2>;
+			hsync-active = <1>;
+		};
+	};
+
+Every required property also supports the use of ranges, so the commonly used
+datasheet description with <min typ max>-tuples can be used.
+
+Example:
+
+	timing1: timing {
+		/* 1920x1080p24 */
+		clock-frequency = <148500000>;
+		hactive = <1920>;
+		vactive = <1080>;
+		hsync-len = <0 44 60>;
+		hfront-porch = <80 88 95>;
+		hback-porch = <100 148 160>;
+		vfront-porch = <0 4 6>;
+		vback-porch = <0 36 50>;
+		vsync-len = <0 5 6>;
+	};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..807fedd 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@ config DISPLAY_TIMING
 config VIDEOMODE
        bool
 
+config OF_DISPLAY_TIMING
+	bool "Enable OF display timing support"
+	select DISPLAY_TIMING
+	help
+	  helper to parse display timings from the devicetree
+
+config OF_VIDEOMODE
+	bool "Enable OF videomode support"
+	select VIDEOMODE
+	select OF_DISPLAY_TIMING
+	help
+	  helper to get videomodes from the devicetree
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index fc30439..b936b00 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
 obj-$(CONFIG_VIDEOMODE) += videomode.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
new file mode 100644
index 0000000..3eb731f
--- /dev/null
+++ b/drivers/video/of_display_timing.c
@@ -0,0 +1,216 @@
+/*
+ * OF helpers for parsing display timings
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/of_display_timings.h>
+
+/**
+ * parse_property - parse timing_entry from device_node
+ * @np: device_node with the property
+ * @name: name of the property
+ * @result: will be set to the return value
+ *
+ * DESCRIPTION:
+ * Every display_timing can be specified with either just the typical value or
+ * a range consisting of min/typ/max. This function helps handling this
+ **/
+static int parse_property(const struct device_node *np, const char *name,
+			  struct timing_entry *result)
+{
+	struct property *prop;
+	int length, cells, ret;
+
+	prop = of_find_property(np, name, &length);
+	if (!prop) {
+		pr_err("%s: could not find property %s\n", __func__, name);
+		return -EINVAL;
+	}
+
+	cells = length / sizeof(u32);
+	if (cells == 1) {
+		ret = of_property_read_u32(np, name, &result->typ);
+		result->min = result->typ;
+		result->max = result->typ;
+	} else if (cells == 3) {
+		ret = of_property_read_u32_array(np, name, &result->min, cells);
+	} else {
+		pr_err("%s: illegal timing specification in %s\n", __func__, name);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+/**
+ * of_get_display_timing - parse display_timing entry from device_node
+ * @np: device_node with the properties
+ **/
+static struct display_timing *of_get_display_timing(const struct device_node *np)
+{
+	struct display_timing *dt;
+	int ret = 0;
+
+	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
+	if (!dt) {
+		pr_err("%s: could not allocate display_timing struct\n", __func__);
+		return NULL;
+	}
+
+	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
+	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
+	ret |= parse_property(np, "hactive", &dt->hactive);
+	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
+	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
+	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
+	ret |= parse_property(np, "vactive", &dt->vactive);
+	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
+	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
+
+	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
+	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
+	of_property_read_u32(np, "de-active", &dt->de_pol_active);
+	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
+	dt->interlaced = of_property_read_bool(np, "interlaced");
+	dt->doublescan = of_property_read_bool(np, "doublescan");
+
+	if (ret) {
+		pr_err("%s: error reading timing properties\n", __func__);
+		kfree(dt);
+		return NULL;
+	}
+
+	return dt;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(const struct device_node *np)
+{
+	struct device_node *timings_np;
+	struct device_node *entry;
+	struct device_node *native_mode;
+	struct display_timings *disp;
+
+	if (!np) {
+		pr_err("%s: no devicenode given\n", __func__);
+		return NULL;
+	}
+
+	timings_np = of_find_node_by_name(np, "display-timings");
+	if (!timings_np) {
+		pr_err("%s: could not find display-timings node\n", __func__);
+		return NULL;
+	}
+
+	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+	if (!disp) {
+		pr_err("%s: could not allocate struct disp'\n", __func__);
+		goto dispfail;
+	}
+
+	entry = of_parse_phandle(timings_np, "native-mode", 0);
+	/* assume first child as native mode if none provided */
+	if (!entry)
+		entry = of_get_next_child(np, NULL);
+	/* if there is no child, it is useless to go on */
+	if (!entry) {
+		pr_err("%s: no timing specifications given\n", __func__);
+		goto entryfail;
+	}
+
+	pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+	native_mode = entry;
+
+	disp->num_timings = of_get_child_count(timings_np);
+	if (disp->num_timings == 0) {
+		/* should never happen, as entry was already found above */
+		pr_err("%s: no timings specified\n", __func__);
+		goto entryfail;
+	}
+
+	disp->timings = kzalloc(sizeof(struct display_timing *) * disp->num_timings,
+				GFP_KERNEL);
+	if (!disp->timings) {
+		pr_err("%s: could not allocate timings array\n", __func__);
+		goto entryfail;
+	}
+
+	disp->num_timings = 0;
+	disp->native_mode = 0;
+
+	for_each_child_of_node(timings_np, entry) {
+		struct display_timing *dt;
+
+		dt = of_get_display_timing(entry);
+		if (!dt) {
+			/*
+			 * to not encourage wrong devicetrees, fail in case of
+			 * an error
+			 */
+			pr_err("%s: error in timing %d\n", __func__,
+			       disp->num_timings + 1);
+			goto timingfail;
+		}
+
+		if (native_mode == entry)
+			disp->native_mode = disp->num_timings;
+
+		disp->timings[disp->num_timings] = dt;
+		disp->num_timings++;
+	}
+	of_node_put(timings_np);
+	of_node_put(native_mode);
+
+	if (disp->num_timings > 0)
+		pr_info("%s: got %d timings. Using timing #%d as default\n",
+			__func__, disp->num_timings, disp->native_mode + 1);
+	else {
+		pr_err("%s: no valid timings specified\n", __func__);
+		display_timings_release(disp);
+		return NULL;
+	}
+	return disp;
+
+timingfail:
+	if (native_mode)
+		of_node_put(native_mode);
+	display_timings_release(disp);
+entryfail:
+	if (disp)
+		kfree(disp);
+dispfail:
+	of_node_put(timings_np);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timings);
+
+/**
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(const struct device_node *np)
+{
+	struct device_node *timings_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+	if (!timings_np)
+		return -EINVAL;
+
+	of_node_put(timings_np);
+	return 1;
+}
+EXPORT_SYMBOL_GPL(of_display_timings_exists);
diff --git a/drivers/video/of_videomode.c b/drivers/video/of_videomode.c
new file mode 100644
index 0000000..c573f92
--- /dev/null
+++ b/drivers/video/of_videomode.c
@@ -0,0 +1,48 @@
+/*
+ * generic videomode helper
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/of_display_timings.h>
+#include <linux/of_videomode.h>
+#include <linux/export.h>
+
+/**
+ * of_get_videomode - get the videomode #<index> from devicetree
+ * @np - devicenode with the display_timings
+ * @vm - set to return value
+ * @index - index into list of display_timings
+ * DESCRIPTION:
+ * Get a list of all display timings and put the one
+ * specified by index into *vm. This function should only be used, if
+ * only one videomode is to be retrieved. A driver that needs to work
+ * with multiple/all videomodes should work with
+ * of_get_display_timings instead.
+ **/
+int of_get_videomode(const struct device_node *np, struct videomode *vm,
+		     int index)
+{
+	struct display_timings *disp;
+	int ret;
+
+	disp = of_get_display_timings(np);
+	if (!disp) {
+		pr_err("%s: no timings specified\n", __func__);
+		return -EINVAL;
+	}
+
+	if (index == OF_USE_NATIVE_MODE)
+		index = disp->native_mode;
+
+	ret = videomode_from_timing(disp, vm, index);
+	if (ret)
+		return ret;
+
+	display_timings_release(disp);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
new file mode 100644
index 0000000..2b4fa0a
--- /dev/null
+++ b/include/linux/of_display_timings.h
@@ -0,0 +1,20 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * display timings of helpers
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
+#define __LINUX_OF_DISPLAY_TIMINGS_H
+
+#include <linux/display_timing.h>
+#include <linux/of.h>
+
+#define OF_USE_NATIVE_MODE -1
+
+struct display_timings *of_get_display_timings(const struct device_node *np);
+int of_display_timings_exists(const struct device_node *np);
+
+#endif
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..4de5fcc
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * videomode of-helpers
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+#include <linux/videomode.h>
+#include <linux/of.h>
+
+int of_get_videomode(const struct device_node *np, struct videomode *vm,
+		     int index);
+
+#endif /* __LINUX_OF_VIDEOMODE_H */
-- 
1.7.10.4

RE: [PATCH v12 2/6] video: add of helper for videomode

[Manjunathappa, Prakash]

Hi Steffen,

On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> This adds support for reading display timings from DT or/and convert one of those
> timings to a videomode.
> The of_display_timing implementation supports multiple children where each
> property can have up to 3 values. All children are read into an array, that
> can be queried.
> of_get_videomode converts exactly one of that timings to a struct videomode.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 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>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  107 ++++++++++
>  drivers/video/Kconfig                              |   13 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/of_display_timing.c                  |  216 ++++++++++++++++++++
>  drivers/video/of_videomode.c                       |   48 +++++
>  include/linux/of_display_timings.h                 |   20 ++
>  include/linux/of_videomode.h                       |   18 ++
>  7 files changed, 424 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/video/of_display_timing.c
>  create mode 100644 drivers/video/of_videomode.c
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
> index 0000000..a05cade
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,107 @@
> +display-timings bindings
> +========================
> +
> +display-timings node
> +--------------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - native-mode: The native mode for the display, in case multiple modes are
> +		provided. When omitted, assume the first node is the native.
> +
> +timings subnode
> +---------------
> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock-frequency: display clock in Hz
> +
> +optional properties:
> + - hsync-active: Hsync pulse is active low/high/ignored
> + - vsync-active: Vsync pulse is active low/high/ignored
> + - de-active: Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted: pixelclock is inverted/non-inverted/ignored
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> +    <1>: high active
> +    <0>: low active
> +    omitted: not used on hardware
> +
> +There are different ways of describing the capabilities of a display. The devicetree
> +representation corresponds to the one commonly found in datasheets for displays.
> +If a display supports multiple signal timings, the native-mode can be specified.
> +
> +The parameters are defined as
> +
> +struct display_timing
> +=====================
> +
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vback_porch                 |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------###############################################----------+-------+
> +  |          #                ↑                            #          |       |
> +  |          #                |                            #          |       |
> +  |  hback   #                |                            #  hfront  | hsync |
> +  |   porch  #                |       hactive              #  porch   |  len  |
> +  |<-------->#<---------------+--------------------------->#<-------->|<----->|
> +  |          #                |                            #          |       |
> +  |          #                |vactive                     #          |       |
> +  |          #                |                            #          |       |
> +  |          #                ↓                            #          |       |
> +  +----------###############################################----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vfront_porch                |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +  |          |                ↑                            |          |       |
> +  |          |                |vsync_len                   |          |       |
> +  |          |                ↓                            |          |       |
> +  +----------+---------------------------------------------+----------+-------+
> +
> +
> +Example:
> +
> +	display-timings {
> +		native-mode = <&timing0>;
> +		timing0: 1920p24 {
> +			/* 1920x1080p24 */
> +			clock-frequency = <52000000>;
> +			hactive = <1920>;
> +			vactive = <1080>;
> +			hfront-porch = <25>;
> +			hback-porch = <25>;
> +			hsync-len = <25>;
> +			vback-porch = <2>;
> +			vfront-porch = <2>;
> +			vsync-len = <2>;
> +			hsync-active = <1>;
> +		};
> +	};
> +
> +Every required property also supports the use of ranges, so the commonly used
> +datasheet description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> +	timing1: timing {
> +		/* 1920x1080p24 */
> +		clock-frequency = <148500000>;
> +		hactive = <1920>;
> +		vactive = <1080>;
> +		hsync-len = <0 44 60>;
> +		hfront-porch = <80 88 95>;
> +		hback-porch = <100 148 160>;
> +		vfront-porch = <0 4 6>;
> +		vback-porch = <0 36 50>;
> +		vsync-len = <0 5 6>;
> +	};
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 2a23b18..807fedd 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -39,6 +39,19 @@ config DISPLAY_TIMING
>  config VIDEOMODE
>         bool
>  
> +config OF_DISPLAY_TIMING
> +	bool "Enable OF display timing support"
> +	select DISPLAY_TIMING
> +	help
> +	  helper to parse display timings from the devicetree
> +
> +config OF_VIDEOMODE
> +	bool "Enable OF videomode support"
> +	select VIDEOMODE
> +	select OF_DISPLAY_TIMING
> +	help
> +	  helper to get videomodes from the devicetree
> +
>  menuconfig FB
>  	tristate "Support for frame buffer devices"
>  	---help---
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index fc30439..b936b00 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -168,4 +168,6 @@ obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
>  #video output switch sysfs driver
>  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
>  obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
> +obj-$(CONFIG_OF_DISPLAY_TIMING) += of_display_timing.o
>  obj-$(CONFIG_VIDEOMODE) += videomode.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> new file mode 100644
> index 0000000..3eb731f
> --- /dev/null
> +++ b/drivers/video/of_display_timing.c
> @@ -0,0 +1,216 @@
> +/*
> + * OF helpers for parsing display timings
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/of_display_timings.h>
> +
> +/**
> + * parse_property - parse timing_entry from device_node
> + * @np: device_node with the property
> + * @name: name of the property
> + * @result: will be set to the return value
> + *
> + * DESCRIPTION:
> + * Every display_timing can be specified with either just the typical value or
> + * a range consisting of min/typ/max. This function helps handling this
> + **/
> +static int parse_property(const struct device_node *np, const char *name,
> +			  struct timing_entry *result)
> +{
> +	struct property *prop;
> +	int length, cells, ret;
> +
> +	prop = of_find_property(np, name, &length);
> +	if (!prop) {
> +		pr_err("%s: could not find property %s\n", __func__, name);
> +		return -EINVAL;
> +	}
> +
> +	cells = length / sizeof(u32);
> +	if (cells == 1) {
> +		ret = of_property_read_u32(np, name, &result->typ);
> +		result->min = result->typ;
> +		result->max = result->typ;
> +	} else if (cells == 3) {
> +		ret = of_property_read_u32_array(np, name, &result->min, cells);
> +	} else {
> +		pr_err("%s: illegal timing specification in %s\n", __func__, name);
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * of_get_display_timing - parse display_timing entry from device_node
> + * @np: device_node with the properties
> + **/
> +static struct display_timing *of_get_display_timing(const struct device_node *np)
> +{
> +	struct display_timing *dt;
> +	int ret = 0;
> +
> +	dt = kzalloc(sizeof(*dt), GFP_KERNEL);
> +	if (!dt) {
> +		pr_err("%s: could not allocate display_timing struct\n", __func__);
> +		return NULL;
> +	}
> +
> +	ret |= parse_property(np, "hback-porch", &dt->hback_porch);
> +	ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
> +	ret |= parse_property(np, "hactive", &dt->hactive);
> +	ret |= parse_property(np, "hsync-len", &dt->hsync_len);
> +	ret |= parse_property(np, "vback-porch", &dt->vback_porch);
> +	ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
> +	ret |= parse_property(np, "vactive", &dt->vactive);
> +	ret |= parse_property(np, "vsync-len", &dt->vsync_len);
> +	ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
> +
> +	of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
> +	of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
> +	of_property_read_u32(np, "de-active", &dt->de_pol_active);
> +	of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
> +	dt->interlaced = of_property_read_bool(np, "interlaced");
> +	dt->doublescan = of_property_read_bool(np, "doublescan");
> +
> +	if (ret) {
> +		pr_err("%s: error reading timing properties\n", __func__);
> +		kfree(dt);
> +		return NULL;
> +	}
> +
> +	return dt;
> +}
> +
> +/**
> + * of_get_display_timings - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timings(const struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +	struct device_node *entry;
> +	struct device_node *native_mode;
> +	struct display_timings *disp;
> +
> +	if (!np) {
> +		pr_err("%s: no devicenode given\n", __func__);
> +		return NULL;
> +	}
> +
> +	timings_np = of_find_node_by_name(np, "display-timings");

I get below build warnings on this line
drivers/video/of_display_timing.c: In function 'of_get_display_timings':
drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'

> +	if (!timings_np) {
> +		pr_err("%s: could not find display-timings node\n", __func__);
> +		return NULL;
> +	}
> +
> +	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
> +	if (!disp) {
> +		pr_err("%s: could not allocate struct disp'\n", __func__);
> +		goto dispfail;
> +	}
> +
> +	entry = of_parse_phandle(timings_np, "native-mode", 0);
> +	/* assume first child as native mode if none provided */
> +	if (!entry)
> +		entry = of_get_next_child(np, NULL);
> +	/* if there is no child, it is useless to go on */
> +	if (!entry) {
> +		pr_err("%s: no timing specifications given\n", __func__);
> +		goto entryfail;
> +	}
> +
> +	pr_info("%s: using %s as default timing\n", __func__, entry->name);
> +
> +	native_mode = entry;
> +
> +	disp->num_timings = of_get_child_count(timings_np);
> +	if (disp->num_timings == 0) {
> +		/* should never happen, as entry was already found above */
> +		pr_err("%s: no timings specified\n", __func__);
> +		goto entryfail;
> +	}
> +
> +	disp->timings = kzalloc(sizeof(struct display_timing *) * disp->num_timings,
> +				GFP_KERNEL);
> +	if (!disp->timings) {
> +		pr_err("%s: could not allocate timings array\n", __func__);
> +		goto entryfail;
> +	}
> +
> +	disp->num_timings = 0;
> +	disp->native_mode = 0;
> +
> +	for_each_child_of_node(timings_np, entry) {
> +		struct display_timing *dt;
> +
> +		dt = of_get_display_timing(entry);
> +		if (!dt) {
> +			/*
> +			 * to not encourage wrong devicetrees, fail in case of
> +			 * an error
> +			 */
> +			pr_err("%s: error in timing %d\n", __func__,
> +			       disp->num_timings + 1);
> +			goto timingfail;
> +		}
> +
> +		if (native_mode == entry)
> +			disp->native_mode = disp->num_timings;
> +
> +		disp->timings[disp->num_timings] = dt;
> +		disp->num_timings++;
> +	}
> +	of_node_put(timings_np);
> +	of_node_put(native_mode);
> +
> +	if (disp->num_timings > 0)
> +		pr_info("%s: got %d timings. Using timing #%d as default\n",
> +			__func__, disp->num_timings, disp->native_mode + 1);
> +	else {
> +		pr_err("%s: no valid timings specified\n", __func__);
> +		display_timings_release(disp);
> +		return NULL;
> +	}
> +	return disp;
> +
> +timingfail:
> +	if (native_mode)
> +		of_node_put(native_mode);
> +	display_timings_release(disp);
> +entryfail:
> +	if (disp)
> +		kfree(disp);
> +dispfail:
> +	of_node_put(timings_np);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timings);
> +
> +/**
> + * of_display_timings_exists - check if a display-timings node is provided
> + * @np: device_node with the timing
> + **/
> +int of_display_timings_exists(const struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	timings_np = of_parse_phandle(np, "display-timings", 0);

Also here:
drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'

Thanks,
Prakash

> +	if (!timings_np)
> +		return -EINVAL;
> +
> +	of_node_put(timings_np);
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(of_display_timings_exists);
> diff --git a/drivers/video/of_videomode.c b/drivers/video/of_videomode.c
> new file mode 100644
> index 0000000..c573f92
> --- /dev/null
> +++ b/drivers/video/of_videomode.c
> @@ -0,0 +1,48 @@
> +/*
> + * generic videomode helper
> + *
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/of_display_timings.h>
> +#include <linux/of_videomode.h>
> +#include <linux/export.h>
> +
> +/**
> + * of_get_videomode - get the videomode #<index> from devicetree
> + * @np - devicenode with the display_timings
> + * @vm - set to return value
> + * @index - index into list of display_timings
> + * DESCRIPTION:
> + * Get a list of all display timings and put the one
> + * specified by index into *vm. This function should only be used, if
> + * only one videomode is to be retrieved. A driver that needs to work
> + * with multiple/all videomodes should work with
> + * of_get_display_timings instead.
> + **/
> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> +		     int index)
> +{
> +	struct display_timings *disp;
> +	int ret;
> +
> +	disp = of_get_display_timings(np);
> +	if (!disp) {
> +		pr_err("%s: no timings specified\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (index == OF_USE_NATIVE_MODE)
> +		index = disp->native_mode;
> +
> +	ret = videomode_from_timing(disp, vm, index);
> +	if (ret)
> +		return ret;
> +
> +	display_timings_release(disp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_videomode);
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..2b4fa0a
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include <linux/display_timing.h>
> +#include <linux/of.h>
> +
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timings(const struct device_node *np);
> +int of_display_timings_exists(const struct device_node *np);
> +
> +#endif
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..4de5fcc
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * videomode of-helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#include <linux/videomode.h>
> +#include <linux/of.h>
> +
> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> +		     int index);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
> -- 
> 1.7.10.4
> 
> --
> 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
> 

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v12 2/6] video: add of helper for videomode

[Steffen Trumtrar]

Hi!

On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
> Hi Steffen,
> 
> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> > +/**
> > + * of_get_display_timings - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timings(const struct device_node *np)
> > +{
> > +	struct device_node *timings_np;
> > +	struct device_node *entry;
> > +	struct device_node *native_mode;
> > +	struct display_timings *disp;
> > +
> > +	if (!np) {
> > +		pr_err("%s: no devicenode given\n", __func__);
> > +		return NULL;
> > +	}
> > +
> > +	timings_np = of_find_node_by_name(np, "display-timings");
> 
> I get below build warnings on this line
> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> 
> > + * of_display_timings_exists - check if a display-timings node is provided
> > + * @np: device_node with the timing
> > + **/
> > +int of_display_timings_exists(const struct device_node *np)
> > +{
> > +	struct device_node *timings_np;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	timings_np = of_parse_phandle(np, "display-timings", 0);
> 
> Also here:
> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> 

The warnings are because the of-functions do not use const pointers where they
should. I had two options: don't use const pointers even if they should be and
have no warnings or use const pointers and have a correct API. (Third option:
send patches for of-functions). I chose the second option.

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

Re: [PATCH v12 2/6] video: add of helper for videomode

[Thierry Reding]

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

On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> Hi!
> 
> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
> > Hi Steffen,
> > 
> > On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> > > +/**
> > > + * of_get_display_timings - parse all display_timing entries from a device_node
> > > + * @np: device_node with the subnodes
> > > + **/
> > > +struct display_timings *of_get_display_timings(const struct device_node *np)
> > > +{
> > > +	struct device_node *timings_np;
> > > +	struct device_node *entry;
> > > +	struct device_node *native_mode;
> > > +	struct display_timings *disp;
> > > +
> > > +	if (!np) {
> > > +		pr_err("%s: no devicenode given\n", __func__);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	timings_np = of_find_node_by_name(np, "display-timings");
> > 
> > I get below build warnings on this line
> > drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> > drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
> > include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> > 
> > > + * of_display_timings_exists - check if a display-timings node is provided
> > > + * @np: device_node with the timing
> > > + **/
> > > +int of_display_timings_exists(const struct device_node *np)
> > > +{
> > > +	struct device_node *timings_np;
> > > +
> > > +	if (!np)
> > > +		return -EINVAL;
> > > +
> > > +	timings_np = of_parse_phandle(np, "display-timings", 0);
> > 
> > Also here:
> > drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> > drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
> > include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> > 
> 
> The warnings are because the of-functions do not use const pointers where they
> should. I had two options: don't use const pointers even if they should be and
> have no warnings or use const pointers and have a correct API. (Third option:
> send patches for of-functions). I chose the second option.

Maybe a better approach would be a combination of 1 and 3: don't use
const pointers for struct device_node for now and bring the issue up
with the OF maintainers, possibly with patches attached that fix the
problematic functions.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v12 2/6] video: add of helper for videomode

[Rob Herring]

On 11/21/2012 05:52 AM, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
>> Hi!
>>
>> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
>>> Hi Steffen,
>>>
>>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
>>>> +/**
>>>> + * of_get_display_timings - parse all display_timing entries from a device_node
>>>> + * @np: device_node with the subnodes
>>>> + **/
>>>> +struct display_timings *of_get_display_timings(const struct device_node *np)
>>>> +{
>>>> +	struct device_node *timings_np;
>>>> +	struct device_node *entry;
>>>> +	struct device_node *native_mode;
>>>> +	struct display_timings *disp;
>>>> +
>>>> +	if (!np) {
>>>> +		pr_err("%s: no devicenode given\n", __func__);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>> +	timings_np = of_find_node_by_name(np, "display-timings");
>>>
>>> I get below build warnings on this line
>>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
>>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
>>> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
>>>
>>>> + * of_display_timings_exists - check if a display-timings node is provided
>>>> + * @np: device_node with the timing
>>>> + **/
>>>> +int of_display_timings_exists(const struct device_node *np)
>>>> +{
>>>> +	struct device_node *timings_np;
>>>> +
>>>> +	if (!np)
>>>> +		return -EINVAL;
>>>> +
>>>> +	timings_np = of_parse_phandle(np, "display-timings", 0);
>>>
>>> Also here:
>>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
>>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
>>> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
>>>
>>
>> The warnings are because the of-functions do not use const pointers where they
>> should. I had two options: don't use const pointers even if they should be and
>> have no warnings or use const pointers and have a correct API. (Third option:
>> send patches for of-functions). I chose the second option.
> 
> Maybe a better approach would be a combination of 1 and 3: don't use
> const pointers for struct device_node for now and bring the issue up
> with the OF maintainers, possibly with patches attached that fix the
> problematic functions.

Why does this need to be const? Since some DT functions increment
refcount the node, I'm not sure that making struct device_node const in
general is right thing to do. I do think it should be okay for
of_parse_phandle.

Rob

Re: [PATCH v12 2/6] video: add of helper for videomode

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3155 bytes --]

On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
> On 11/21/2012 05:52 AM, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> >> Hi!
> >>
> >> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
> >>> Hi Steffen,
> >>>
> >>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> >>>> +/**
> >>>> + * of_get_display_timings - parse all display_timing entries from a device_node
> >>>> + * @np: device_node with the subnodes
> >>>> + **/
> >>>> +struct display_timings *of_get_display_timings(const struct device_node *np)
> >>>> +{
> >>>> +	struct device_node *timings_np;
> >>>> +	struct device_node *entry;
> >>>> +	struct device_node *native_mode;
> >>>> +	struct display_timings *disp;
> >>>> +
> >>>> +	if (!np) {
> >>>> +		pr_err("%s: no devicenode given\n", __func__);
> >>>> +		return NULL;
> >>>> +	}
> >>>> +
> >>>> +	timings_np = of_find_node_by_name(np, "display-timings");
> >>>
> >>> I get below build warnings on this line
> >>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> >>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
> >>> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>>> + * of_display_timings_exists - check if a display-timings node is provided
> >>>> + * @np: device_node with the timing
> >>>> + **/
> >>>> +int of_display_timings_exists(const struct device_node *np)
> >>>> +{
> >>>> +	struct device_node *timings_np;
> >>>> +
> >>>> +	if (!np)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	timings_np = of_parse_phandle(np, "display-timings", 0);
> >>>
> >>> Also here:
> >>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> >>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
> >>> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>
> >> The warnings are because the of-functions do not use const pointers where they
> >> should. I had two options: don't use const pointers even if they should be and
> >> have no warnings or use const pointers and have a correct API. (Third option:
> >> send patches for of-functions). I chose the second option.
> > 
> > Maybe a better approach would be a combination of 1 and 3: don't use
> > const pointers for struct device_node for now and bring the issue up
> > with the OF maintainers, possibly with patches attached that fix the
> > problematic functions.
> 
> Why does this need to be const? Since some DT functions increment
> refcount the node, I'm not sure that making struct device_node const in
> general is right thing to do. I do think it should be okay for
> of_parse_phandle.

I wasn't proposing to do it everywhere but only where possible. If the
node is modified in some way then obviously it shouldn't be const.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v12 2/6] video: add of helper for videomode

[Steffen Trumtrar]

Hi!

On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
> On 11/21/2012 05:52 AM, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> >> Hi!
> >>
> >> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
> >>> Hi Steffen,
> >>>
> >>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> >>>> +/**
> >>>> + * of_get_display_timings - parse all display_timing entries from a device_node
> >>>> + * @np: device_node with the subnodes
> >>>> + **/
> >>>> +struct display_timings *of_get_display_timings(const struct device_node *np)
> >>>> +{
> >>>> +	struct device_node *timings_np;
> >>>> +	struct device_node *entry;
> >>>> +	struct device_node *native_mode;
> >>>> +	struct display_timings *disp;
> >>>> +
> >>>> +	if (!np) {
> >>>> +		pr_err("%s: no devicenode given\n", __func__);
> >>>> +		return NULL;
> >>>> +	}
> >>>> +
> >>>> +	timings_np = of_find_node_by_name(np, "display-timings");
> >>>
> >>> I get below build warnings on this line
> >>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> >>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
> >>> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>>> + * of_display_timings_exists - check if a display-timings node is provided
> >>>> + * @np: device_node with the timing
> >>>> + **/
> >>>> +int of_display_timings_exists(const struct device_node *np)
> >>>> +{
> >>>> +	struct device_node *timings_np;
> >>>> +
> >>>> +	if (!np)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	timings_np = of_parse_phandle(np, "display-timings", 0);
> >>>
> >>> Also here:
> >>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> >>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
> >>> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>
> >> The warnings are because the of-functions do not use const pointers where they
> >> should. I had two options: don't use const pointers even if they should be and
> >> have no warnings or use const pointers and have a correct API. (Third option:
> >> send patches for of-functions). I chose the second option.
> > 
> > Maybe a better approach would be a combination of 1 and 3: don't use
> > const pointers for struct device_node for now and bring the issue up
> > with the OF maintainers, possibly with patches attached that fix the
> > problematic functions.
> 
> Why does this need to be const? Since some DT functions increment
> refcount the node, I'm not sure that making struct device_node const in
> general is right thing to do. I do think it should be okay for
> of_parse_phandle.
> 

Okay, that seems right. I went a little to far with const'ing.
I will send a patch for of_parse_phandle as this function does
not seem to change the pointer it is given, but returns a new one
on which the refcount gets incremented.

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

Re: [PATCH v12 2/6] video: add of helper for videomode

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 3691 bytes --]

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

> +timings subnode
> +---------------
> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock-frequency: display clock in Hz
> +
> +optional properties:
> + - hsync-active: Hsync pulse is active low/high/ignored
> + - vsync-active: Vsync pulse is active low/high/ignored
> + - de-active: Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted: pixelclock is inverted/non-inverted/ignored

Inverted related to what? And isn't this a bool? Pixel clock is either
normal (whatever that is), or inverted. It can't be "not used".

I guess normal case is "pixel data is driven on the rising edge of pixel
clock"? If that's common knowledge, I guess it doesn't need to be
mentioned. But I always have to verify from the documentation what
"normal" means on this particular panel/soc =).

> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> +    <1>: high active
> +    <0>: low active
> +    omitted: not used on hardware

Perhaps it's obvious, but no harm being explicit: mention that the bool
properties are off is omitted.

And I didn't read the rest of the patches yet, so perhaps this is
already correct, but as I think this framework is usable without DT
also, the meanings of the fields in the structs should be explained in
the header files also in case they are not obvious.

> +Example:
> +
> +	display-timings {
> +		native-mode = <&timing0>;
> +		timing0: 1920p24 {

This should still be 1080p24, not 1920p24 =).

> +			/* 1920x1080p24 */
> +			clock-frequency = <52000000>;
> +			hactive = <1920>;
> +			vactive = <1080>;
> +			hfront-porch = <25>;
> +			hback-porch = <25>;
> +			hsync-len = <25>;
> +			vback-porch = <2>;
> +			vfront-porch = <2>;
> +			vsync-len = <2>;
> +			hsync-active = <1>;
> +		};
> +	};
> +

> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..2b4fa0a
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include <linux/display_timing.h>
> +#include <linux/of.h>

No need to include these, just add "struct ...;".

> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timings(const struct device_node *np);
> +int of_display_timings_exists(const struct device_node *np);
> +
> +#endif
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..4de5fcc
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * videomode of-helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#include <linux/videomode.h>
> +#include <linux/of.h>

Same here.

> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> +		     int index);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
> 

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Steffen Trumtrar]

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/drivers/video/fbmon.c b/drivers/video/fbmon.c
index c1939a6..16c353c 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -31,7 +31,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <video/edid.h>
-#include <linux/videomode.h>
+#include <linux/of_videomode.h>
 #ifdef CONFIG_PPC_OF
 #include <asm/prom.h>
 #include <asm/pci-bridge.h>
@@ -1418,6 +1418,46 @@ 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(const struct device_node *np, struct fb_videomode *fb,
+			unsigned 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_info("%s: got %dx%d display mode from %s\n", __func__, 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 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>
 
 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);
-- 
1.7.10.4

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

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2163 bytes --]

On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add helper to get fb_videomode from devicetree.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Acked-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  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.

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

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Laurent Pinchart]

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

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

[Steffen Trumtrar]

On Wed, Nov 21, 2012 at 02:49:30PM +0200, 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.
> 

Okay. Seems to fit with the rest of the file;

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

Well, I don't remember at the moment who requested this, but it was not my
idea to put them there. So, this is a matter of style I guess.
But maybe I understood that wrong.


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

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

[Tomi Valkeinen]

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

On 2012-11-21 18:24, Steffen Trumtrar wrote:
> On Wed, Nov 21, 2012 at 02:49:30PM +0200, Tomi Valkeinen wrote:

>>> @@ -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.
>>
> 
> Well, I don't remember at the moment who requested this, but it was not my
> idea to put them there. So, this is a matter of style I guess.
> But maybe I understood that wrong.

Right, one reviewer says this way, and another says that way =).

With the header files I've made I only use #ifs with #else, when I want
to give a static inline empty/no-op implementation for the function in
case the feature is not compiled into the kernel.

As you said, matter of taste. Up to you.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[PATCH v12 5/6] drm_modes: add videomode helpers

[Steffen Trumtrar]

Add conversion from videomode to drm_display_mode

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/gpu/drm/drm_modes.c |   37 +++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h          |    6 ++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 59450f3..0073b27 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -35,6 +35,7 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
+#include <linux/videomode.h>
 
 /**
  * drm_mode_debug_printmodeline - debug print a mode
@@ -504,6 +505,42 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh,
 }
 EXPORT_SYMBOL(drm_gtf_mode);
 
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int drm_display_mode_from_videomode(const struct videomode *vm,
+				    struct drm_display_mode *dmode)
+{
+	dmode->hdisplay = vm->hactive;
+	dmode->hsync_start = dmode->hdisplay + vm->hfront_porch;
+	dmode->hsync_end = dmode->hsync_start + vm->hsync_len;
+	dmode->htotal = dmode->hsync_end + vm->hback_porch;
+
+	dmode->vdisplay = vm->vactive;
+	dmode->vsync_start = dmode->vdisplay + vm->vfront_porch;
+	dmode->vsync_end = dmode->vsync_start + vm->vsync_len;
+	dmode->vtotal = dmode->vsync_end + vm->vback_porch;
+
+	dmode->clock = vm->pixelclock / 1000;
+
+	dmode->flags = 0;
+	if (vm->hah)
+		dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+	if (vm->vah)
+		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+	if (vm->interlaced)
+		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+	if (vm->doublescan)
+		dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+	drm_mode_set_name(dmode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
+#endif
+
 /**
  * drm_mode_set_name - set the name on a mode
  * @mode: name will be set in this mode
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3fd8280..de2f6cf 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -56,6 +56,7 @@
 #include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/videomode.h>
 #if defined(__alpha__) || defined(__powerpc__)
 #include <asm/pgtable.h>	/* For pte_wrprotect */
 #endif
@@ -1454,6 +1455,11 @@ extern struct drm_display_mode *
 drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 				  struct drm_cmdline_mode *cmd);
 
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+extern int drm_display_mode_from_videomode(const struct videomode *vm,
+					   struct drm_display_mode *dmode);
+#endif
+
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
 extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
-- 
1.7.10.4

Re: [PATCH v12 5/6] drm_modes: add videomode helpers

[Tomi Valkeinen]

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

On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add conversion from videomode to drm_display_mode
> 
> 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/gpu/drm/drm_modes.c |   37 +++++++++++++++++++++++++++++++++++++
>  include/drm/drmP.h          |    6 ++++++
>  2 files changed, 43 insertions(+)
> 

> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3fd8280..de2f6cf 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -56,6 +56,7 @@
>  #include <linux/cdev.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/videomode.h>
>  #if defined(__alpha__) || defined(__powerpc__)
>  #include <asm/pgtable.h>	/* For pte_wrprotect */
>  #endif
> @@ -1454,6 +1455,11 @@ extern struct drm_display_mode *
>  drm_mode_create_from_cmdline_mode(struct drm_device *dev,
>  				  struct drm_cmdline_mode *cmd);
>  
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +extern int drm_display_mode_from_videomode(const struct videomode *vm,
> +					   struct drm_display_mode *dmode);
> +#endif
> +
>  /* Modesetting support */
>  extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
>  extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
> 

And the same comments apply to this header file.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[PATCH v12 3/6] fbmon: add videomode helpers

[Steffen Trumtrar]

Add a function to convert from the generic videomode to a fb_videomode.

Signed-off-by: Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Acked-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Tested-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Tested-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/video/fbmon.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fb.h    |    6 ++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index cef6557..c1939a6 100644
--- a/drivers/video/fbmon.c
+++ b/drivers/video/fbmon.c
@@ -31,6 +31,7 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <video/edid.h>
+#include <linux/videomode.h>
 #ifdef CONFIG_PPC_OF
 #include <asm/prom.h>
 #include <asm/pci-bridge.h>
@@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
 	kfree(timings);
 	return err;
 }
+
+#if IS_ENABLED(CONFIG_VIDEOMODE)
+int fb_videomode_from_videomode(const struct videomode *vm,
+				struct fb_videomode *fbmode)
+{
+	unsigned int htotal, vtotal;
+
+	fbmode->xres = vm->hactive;
+	fbmode->left_margin = vm->hback_porch;
+	fbmode->right_margin = vm->hfront_porch;
+	fbmode->hsync_len = vm->hsync_len;
+
+	fbmode->yres = vm->vactive;
+	fbmode->upper_margin = vm->vback_porch;
+	fbmode->lower_margin = vm->vfront_porch;
+	fbmode->vsync_len = vm->vsync_len;
+
+	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
+
+	fbmode->sync = 0;
+	fbmode->vmode = 0;
+	if (vm->hah)
+		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+	if (vm->vah)
+		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+	if (vm->interlaced)
+		fbmode->vmode |= FB_VMODE_INTERLACED;
+	if (vm->doublescan)
+		fbmode->vmode |= FB_VMODE_DOUBLE;
+	if (vm->de)
+		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
+	fbmode->flag = 0;
+
+	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
+		 vm->hsync_len;
+	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
+		 vm->vsync_len;
+	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fb_videomode_from_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 c7a9571..920cbe3 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -14,6 +14,7 @@
 #include <linux/backlight.h>
 #include <linux/slab.h>
 #include <asm/io.h>
+#include <linux/videomode.h>
 
 struct vm_area_struct;
 struct fb_info;
@@ -714,6 +715,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_VIDEOMODE)
+extern int fb_videomode_from_videomode(const struct videomode *vm,
+				       struct fb_videomode *fbmode);
+#endif
+
 /* drivers/video/modedb.c */
 #define VESA_MODEDB_SIZE 34
 extern void fb_var_to_videomode(struct fb_videomode *mode,
-- 
1.7.10.4

RE: [PATCH v12 3/6] fbmon: add videomode helpers

[Manjunathappa, Prakash]

Hi Steffen,

I am trying to add DT support for da8xx-fb driver on top of your patches.
Encountered below build error. Sorry for reporting it late.

On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:
> Add a function to convert from the generic videomode to a fb_videomode.
> 
> 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 |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fb.h    |    6 ++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> index cef6557..c1939a6 100644
> --- a/drivers/video/fbmon.c
> +++ b/drivers/video/fbmon.c
> @@ -31,6 +31,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <video/edid.h>
> +#include <linux/videomode.h>
>  #ifdef CONFIG_PPC_OF
>  #include <asm/prom.h>
>  #include <asm/pci-bridge.h>
> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
>  	kfree(timings);
>  	return err;
>  }
> +
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +int fb_videomode_from_videomode(const struct videomode *vm,
> +				struct fb_videomode *fbmode)
> +{
> +	unsigned int htotal, vtotal;
> +
> +	fbmode->xres = vm->hactive;
> +	fbmode->left_margin = vm->hback_porch;
> +	fbmode->right_margin = vm->hfront_porch;
> +	fbmode->hsync_len = vm->hsync_len;
> +
> +	fbmode->yres = vm->vactive;
> +	fbmode->upper_margin = vm->vback_porch;
> +	fbmode->lower_margin = vm->vfront_porch;
> +	fbmode->vsync_len = vm->vsync_len;
> +
> +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> +
> +	fbmode->sync = 0;
> +	fbmode->vmode = 0;
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +	if (vm->de)
> +		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;

"FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting
build error on this. Please let me know if I am missing something.

Thanks,
Prakash

> +	fbmode->flag = 0;
> +
> +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> +		 vm->hsync_len;
> +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> +		 vm->vsync_len;
> +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fb_videomode_from_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 c7a9571..920cbe3 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -14,6 +14,7 @@
>  #include <linux/backlight.h>
>  #include <linux/slab.h>
>  #include <asm/io.h>
> +#include <linux/videomode.h>
>  
>  struct vm_area_struct;
>  struct fb_info;
> @@ -714,6 +715,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_VIDEOMODE)
> +extern int fb_videomode_from_videomode(const struct videomode *vm,
> +				       struct fb_videomode *fbmode);
> +#endif
> +
>  /* drivers/video/modedb.c */
>  #define VESA_MODEDB_SIZE 34
>  extern void fb_var_to_videomode(struct fb_videomode *mode,
> -- 
> 1.7.10.4
> 
> --
> 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
> 

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Leela Krishna Amudala]

Yes,
Even I got the same build error.
later I fixed it by including "#include <linux/mxsfb.h>"

Best Wishes,
Leela Krishna.

On Wed, Nov 21, 2012 at 3:39 PM, Manjunathappa, Prakash
<prakash.pm@ti.com> wrote:
> Hi Steffen,
>
> I am trying to add DT support for da8xx-fb driver on top of your patches.
> Encountered below build error. Sorry for reporting it late.
>
> On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:
>> Add a function to convert from the generic videomode to a fb_videomode.
>>
>> 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 |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/fb.h    |    6 ++++++
>>  2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
>> index cef6557..c1939a6 100644
>> --- a/drivers/video/fbmon.c
>> +++ b/drivers/video/fbmon.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/slab.h>
>>  #include <video/edid.h>
>> +#include <linux/videomode.h>
>>  #ifdef CONFIG_PPC_OF
>>  #include <asm/prom.h>
>>  #include <asm/pci-bridge.h>
>> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
>>       kfree(timings);
>>       return err;
>>  }
>> +
>> +#if IS_ENABLED(CONFIG_VIDEOMODE)
>> +int fb_videomode_from_videomode(const struct videomode *vm,
>> +                             struct fb_videomode *fbmode)
>> +{
>> +     unsigned int htotal, vtotal;
>> +
>> +     fbmode->xres = vm->hactive;
>> +     fbmode->left_margin = vm->hback_porch;
>> +     fbmode->right_margin = vm->hfront_porch;
>> +     fbmode->hsync_len = vm->hsync_len;
>> +
>> +     fbmode->yres = vm->vactive;
>> +     fbmode->upper_margin = vm->vback_porch;
>> +     fbmode->lower_margin = vm->vfront_porch;
>> +     fbmode->vsync_len = vm->vsync_len;
>> +
>> +     fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
>> +
>> +     fbmode->sync = 0;
>> +     fbmode->vmode = 0;
>> +     if (vm->hah)
>> +             fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
>> +     if (vm->vah)
>> +             fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
>> +     if (vm->interlaced)
>> +             fbmode->vmode |= FB_VMODE_INTERLACED;
>> +     if (vm->doublescan)
>> +             fbmode->vmode |= FB_VMODE_DOUBLE;
>> +     if (vm->de)
>> +             fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
>
> "FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting
> build error on this. Please let me know if I am missing something.
>
> Thanks,
> Prakash
>
>> +     fbmode->flag = 0;
>> +
>> +     htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
>> +              vm->hsync_len;
>> +     vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
>> +              vm->vsync_len;
>> +     fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(fb_videomode_from_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 c7a9571..920cbe3 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -14,6 +14,7 @@
>>  #include <linux/backlight.h>
>>  #include <linux/slab.h>
>>  #include <asm/io.h>
>> +#include <linux/videomode.h>
>>
>>  struct vm_area_struct;
>>  struct fb_info;
>> @@ -714,6 +715,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_VIDEOMODE)
>> +extern int fb_videomode_from_videomode(const struct videomode *vm,
>> +                                    struct fb_videomode *fbmode);
>> +#endif
>> +
>>  /* drivers/video/modedb.c */
>>  #define VESA_MODEDB_SIZE 34
>>  extern void fb_var_to_videomode(struct fb_videomode *mode,
>> --
>> 1.7.10.4
>>
>> --
>> 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
>>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Steffen Trumtrar]

Hi!

On Wed, Nov 21, 2012 at 04:39:01PM +0530, Leela Krishna Amudala wrote:
> Yes,
> Even I got the same build error.
> later I fixed it by including "#include <linux/mxsfb.h>"
> 
> Best Wishes,
> Leela Krishna.
> 
> On Wed, Nov 21, 2012 at 3:39 PM, Manjunathappa, Prakash
> <prakash.pm@ti.com> wrote:
> > Hi Steffen,
> >
> > I am trying to add DT support for da8xx-fb driver on top of your patches.
> > Encountered below build error. Sorry for reporting it late.
> >
> > On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:
> >> Add a function to convert from the generic videomode to a fb_videomode.
> >>
> >> 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 |   46 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/fb.h    |    6 ++++++
> >>  2 files changed, 52 insertions(+)
> >>
> >> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> >> index cef6557..c1939a6 100644
> >> --- a/drivers/video/fbmon.c
> >> +++ b/drivers/video/fbmon.c
> >> @@ -31,6 +31,7 @@
> >>  #include <linux/pci.h>
> >>  #include <linux/slab.h>
> >>  #include <video/edid.h>
> >> +#include <linux/videomode.h>
> >>  #ifdef CONFIG_PPC_OF
> >>  #include <asm/prom.h>
> >>  #include <asm/pci-bridge.h>
> >> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
> >>       kfree(timings);
> >>       return err;
> >>  }
> >> +
> >> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> >> +int fb_videomode_from_videomode(const struct videomode *vm,
> >> +                             struct fb_videomode *fbmode)
> >> +{
> >> +     unsigned int htotal, vtotal;
> >> +
> >> +     fbmode->xres = vm->hactive;
> >> +     fbmode->left_margin = vm->hback_porch;
> >> +     fbmode->right_margin = vm->hfront_porch;
> >> +     fbmode->hsync_len = vm->hsync_len;
> >> +
> >> +     fbmode->yres = vm->vactive;
> >> +     fbmode->upper_margin = vm->vback_porch;
> >> +     fbmode->lower_margin = vm->vfront_porch;
> >> +     fbmode->vsync_len = vm->vsync_len;
> >> +
> >> +     fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> >> +
> >> +     fbmode->sync = 0;
> >> +     fbmode->vmode = 0;
> >> +     if (vm->hah)
> >> +             fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> >> +     if (vm->vah)
> >> +             fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> >> +     if (vm->interlaced)
> >> +             fbmode->vmode |= FB_VMODE_INTERLACED;
> >> +     if (vm->doublescan)
> >> +             fbmode->vmode |= FB_VMODE_DOUBLE;
> >> +     if (vm->de)
> >> +             fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
> >
> > "FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting
> > build error on this. Please let me know if I am missing something.
> >
> > Thanks,
> > Prakash
> >

I compile-tested the series and didn't have that error. But obviously I should
have. As this is a mxsfs flag, I will throw it out.

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

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Tomi Valkeinen]

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

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

> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index c7a9571..920cbe3 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -14,6 +14,7 @@
>  #include <linux/backlight.h>
>  #include <linux/slab.h>
>  #include <asm/io.h>
> +#include <linux/videomode.h>

No need for this, just add "struct xxx;".

>  struct vm_area_struct;
>  struct fb_info;
> @@ -714,6 +715,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_VIDEOMODE)
> +extern int fb_videomode_from_videomode(const struct videomode *vm,
> +				       struct fb_videomode *fbmode);
> +#endif
> +
>  /* drivers/video/modedb.c */
>  #define VESA_MODEDB_SIZE 34
>  extern void fb_var_to_videomode(struct fb_videomode *mode,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Laurent Pinchart]

Hi Steffen,

Sorry, I've just found another small bug below.

On Tuesday 20 November 2012 16:54:53 Steffen Trumtrar wrote:
> Add a function to convert from the generic videomode to a fb_videomode.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Acked-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  drivers/video/fbmon.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fb.h    |    6 ++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> index cef6557..c1939a6 100644
> --- a/drivers/video/fbmon.c
> +++ b/drivers/video/fbmon.c
> @@ -31,6 +31,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <video/edid.h>
> +#include <linux/videomode.h>
>  #ifdef CONFIG_PPC_OF
>  #include <asm/prom.h>
>  #include <asm/pci-bridge.h>
> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct
> fb_var_screeninfo *var, struct fb_inf kfree(timings);
>  	return err;
>  }
> +
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +int fb_videomode_from_videomode(const struct videomode *vm,
> +				struct fb_videomode *fbmode)
> +{
> +	unsigned int htotal, vtotal;
> +
> +	fbmode->xres = vm->hactive;
> +	fbmode->left_margin = vm->hback_porch;
> +	fbmode->right_margin = vm->hfront_porch;
> +	fbmode->hsync_len = vm->hsync_len;
> +
> +	fbmode->yres = vm->vactive;
> +	fbmode->upper_margin = vm->vback_porch;
> +	fbmode->lower_margin = vm->vfront_porch;
> +	fbmode->vsync_len = vm->vsync_len;
> +
> +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> +
> +	fbmode->sync = 0;
> +	fbmode->vmode = 0;
> +	if (vm->hah)
> +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> +	if (vm->vah)
> +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> +	if (vm->interlaced)
> +		fbmode->vmode |= FB_VMODE_INTERLACED;
> +	if (vm->doublescan)
> +		fbmode->vmode |= FB_VMODE_DOUBLE;
> +	if (vm->de)
> +		fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
> +	fbmode->flag = 0;
> +
> +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> +		 vm->hsync_len;
> +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> +		 vm->vsync_len;
> +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);

This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest solution 
is probably to use 64-bit computation.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fb_videomode_from_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 c7a9571..920cbe3 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -14,6 +14,7 @@
>  #include <linux/backlight.h>
>  #include <linux/slab.h>
>  #include <asm/io.h>
> +#include <linux/videomode.h>
> 
>  struct vm_area_struct;
>  struct fb_info;
> @@ -714,6 +715,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_VIDEOMODE)
> +extern int fb_videomode_from_videomode(const struct videomode *vm,
> +				       struct fb_videomode *fbmode);
> +#endif
> +
>  /* drivers/video/modedb.c */
>  #define VESA_MODEDB_SIZE 34
>  extern void fb_var_to_videomode(struct fb_videomode *mode,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Sascha Hauer]

Hi Laurent,

On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> Hi Steffen,
> 
> > +
> > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > +		 vm->hsync_len;
> > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > +		 vm->vsync_len;
> > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> 
> This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest solution 
> is probably to use 64-bit computation.

You have displays with a pixelclock > 4GHz?

Sascha

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

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Laurent Pinchart]

Hi Sascha,

On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:
> On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> > Hi Steffen,
> > 
> > > +
> > > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > > +		 vm->hsync_len;
> > > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > +		 vm->vsync_len;
> > > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> > 
> > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest
> > solution is probably to use 64-bit computation.
> 
> You have displays with a pixelclock > 4GHz?

vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus overflow if 
the clock frequency is >= ~4.3 MHz. I have displays with a clock frequency 
higher than that :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Sascha Hauer]

On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote:
> Hi Sascha,
> 
> On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:
> > On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> > > Hi Steffen,
> > > 
> > > > +
> > > > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > > > +		 vm->hsync_len;
> > > > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > > +		 vm->vsync_len;
> > > > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> > > 
> > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest
> > > solution is probably to use 64-bit computation.
> > 
> > You have displays with a pixelclock > 4GHz?
> 
> vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus overflow if 
> the clock frequency is >= ~4.3 MHz. I have displays with a clock frequency 
> higher than that :-)

If vm->pixelclock is in Hz, then the * 1000 above is wrong.

Sascha

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

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Laurent Pinchart]

On Thursday 22 November 2012 09:53:42 Sascha Hauer wrote:
> On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote:
> > On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:
> > > On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> > > > Hi Steffen,
> > > > 
> > > > > +
> > > > > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > > > > +		 vm->hsync_len;
> > > > > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > > > +		 vm->vsync_len;
> > > > > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> > > > 
> > > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest
> > > > solution is probably to use 64-bit computation.
> > > 
> > > You have displays with a pixelclock > 4GHz?
> > 
> > vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus
> > overflow if the clock frequency is >= ~4.3 MHz. I have displays with a
> > clock frequency higher than that :-)
> 
> If vm->pixelclock is in Hz, then the * 1000 above is wrong.

My bad, I though refresh was expressed in mHz. So yes, the above computation 
is wrong.

BTW it seems that the refreshrate field in struct videomode isn't used. It 
should then be removed.

I've just realized that the struct videomode fields are not documented. 
kerneldoc in include/linux/videomode.h would be a good addition.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Steffen Trumtrar]

On Thu, Nov 22, 2012 at 10:07:07AM +0100, Laurent Pinchart wrote:
> On Thursday 22 November 2012 09:53:42 Sascha Hauer wrote:
> > On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote:
> > > On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:
> > > > On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> > > > > Hi Steffen,
> > > > > 
> > > > > > +
> > > > > > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > > > > > +		 vm->hsync_len;
> > > > > > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > > > > +		 vm->vsync_len;
> > > > > > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> > > > > 
> > > > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest
> > > > > solution is probably to use 64-bit computation.
> > > > 
> > > > You have displays with a pixelclock > 4GHz?
> > > 
> > > vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus
> > > overflow if the clock frequency is >= ~4.3 MHz. I have displays with a
> > > clock frequency higher than that :-)
> > 
> > If vm->pixelclock is in Hz, then the * 1000 above is wrong.
> 
> My bad, I though refresh was expressed in mHz. So yes, the above computation 
> is wrong.
>

Okay. I will fix that with the next version...

> BTW it seems that the refreshrate field in struct videomode isn't used. It 
> should then be removed.
> 

...and remove this field.

> I've just realized that the struct videomode fields are not documented. 
> kerneldoc in include/linux/videomode.h would be a good addition.
> 

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

[PATCH v12 6/6] drm_modes: add of_videomode helpers

[Steffen Trumtrar]

Add helper to get drm_display_mode from devicetree.

Signed-off-by: Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Acked-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Tested-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Tested-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/gpu/drm/drm_modes.c |   35 ++++++++++++++++++++++++++++++++++-
 include/drm/drmP.h          |    6 ++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 0073b27..04feef8 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -35,7 +35,8 @@
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
-#include <linux/videomode.h>
+#include <linux/of.h>
+#include <linux/of_videomode.h>
 
 /**
  * drm_mode_debug_printmodeline - debug print a mode
@@ -541,6 +542,38 @@ int drm_display_mode_from_videomode(const struct videomode *vm,
 EXPORT_SYMBOL_GPL(drm_display_mode_from_videomode);
 #endif
 
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+/**
+ * of_get_drm_display_mode - get a drm_display_mode from devicetree
+ * @np: device_node with the timing specification
+ * @dmode: will be set to the return value
+ * @index: index into the list of display timings in devicetree
+ *
+ * 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 and
+ * work with that instead.
+ */
+int of_get_drm_display_mode(const struct device_node *np,
+			    struct drm_display_mode *dmode, unsigned int index)
+{
+	struct videomode vm;
+	int ret;
+
+	ret = of_get_videomode(np, &vm, index);
+	if (ret)
+		return ret;
+
+	drm_display_mode_from_videomode(&vm, dmode);
+
+	pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive,
+		vm.vactive, np->name);
+	drm_mode_debug_printmodeline(dmode);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
+#endif
+
 /**
  * drm_mode_set_name - set the name on a mode
  * @mode: name will be set in this mode
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index de2f6cf..377280f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -56,6 +56,7 @@
 #include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <linux/videomode.h>
 #if defined(__alpha__) || defined(__powerpc__)
 #include <asm/pgtable.h>	/* For pte_wrprotect */
@@ -1459,6 +1460,11 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 extern int drm_display_mode_from_videomode(const struct videomode *vm,
 					   struct drm_display_mode *dmode);
 #endif
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
+extern int of_get_drm_display_mode(const struct device_node *np,
+				   struct drm_display_mode *dmode,
+				   unsigned int index);
+#endif
 
 /* Modesetting support */
 extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
-- 
1.7.10.4

Re: [PATCH v12 6/6] drm_modes: add of_videomode helpers

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1823 bytes --]

On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add helper to get drm_display_mode from devicetree.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Acked-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> Tested-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  drivers/gpu/drm/drm_modes.c |   35 ++++++++++++++++++++++++++++++++++-
>  include/drm/drmP.h          |    6 ++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 

> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index de2f6cf..377280f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -56,6 +56,7 @@
>  #include <linux/cdev.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  #include <linux/videomode.h>
>  #if defined(__alpha__) || defined(__powerpc__)
>  #include <asm/pgtable.h>	/* For pte_wrprotect */
> @@ -1459,6 +1460,11 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
>  extern int drm_display_mode_from_videomode(const struct videomode *vm,
>  					   struct drm_display_mode *dmode);
>  #endif
> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
> +extern int of_get_drm_display_mode(const struct device_node *np,
> +				   struct drm_display_mode *dmode,
> +				   unsigned int index);
> +#endif
>  
>  /* Modesetting support */
>  extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);

And the same comments here also.

 Tomi




[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v12 0/6] of: add display helper

[Laurent Pinchart]

On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> Hi!
> 
> Changes since v11:
> 	- make pointers const where applicable
> 	- add reviewed-by Laurent Pinchart

Looks good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Through which tree do you plan to push this ?

> Regards,
> Steffen
> 
> 
> Steffen Trumtrar (6):
>   video: add display_timing and videomode
>   video: add of helper for videomode
>   fbmon: add videomode helpers
>   fbmon: add of_videomode helpers
>   drm_modes: add videomode helpers
>   drm_modes: add of_videomode helpers
> 
>  .../devicetree/bindings/video/display-timings.txt  |  107 ++++++++++
>  drivers/gpu/drm/drm_modes.c                        |   70 +++++++
>  drivers/video/Kconfig                              |   19 ++
>  drivers/video/Makefile                             |    4 +
>  drivers/video/display_timing.c                     |   24 +++
>  drivers/video/fbmon.c                              |   86 ++++++++
>  drivers/video/of_display_timing.c                  |  216 +++++++++++++++++
>  drivers/video/of_videomode.c                       |   48 +++++
>  drivers/video/videomode.c                          |   46 +++++
>  include/drm/drmP.h                                 |   12 ++
>  include/linux/display_timing.h                     |   70 +++++++
>  include/linux/fb.h                                 |   13 ++
>  include/linux/of_display_timings.h                 |   20 ++
>  include/linux/of_videomode.h                       |   18 ++
>  include/linux/videomode.h                          |   40 ++++
>  15 files changed, 793 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/video/display-timings.txt create mode
> 100644 drivers/video/display_timing.c
>  create mode 100644 drivers/video/of_display_timing.c
>  create mode 100644 drivers/video/of_videomode.c
>  create mode 100644 drivers/video/videomode.c
>  create mode 100644 include/linux/display_timing.h
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
>  create mode 100644 include/linux/videomode.h
-- 
Regards,

Laurent Pinchart

Re: [PATCH v12 0/6] of: add display helper

[Robert Schwebel]

On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > Hi!
> > 
> > Changes since v11:
> > 	- make pointers const where applicable
> > 	- add reviewed-by Laurent Pinchart
> 
> Looks good to me.
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Through which tree do you plan to push this ?

We have no idea yet, and none of the people on Cc: have volunteered
so far... what do you think?

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

Re: [PATCH v12 0/6] of: add display helper

[Thierry Reding]

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

On Tue, Nov 20, 2012 at 07:11:29PM +0100, Robert Schwebel wrote:
> On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> > On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > > Hi!
> > > 
> > > Changes since v11:
> > > 	- make pointers const where applicable
> > > 	- add reviewed-by Laurent Pinchart
> > 
> > Looks good to me.
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Through which tree do you plan to push this ?
> 
> We have no idea yet, and none of the people on Cc: have volunteered
> so far... what do you think?

The customary approach would be to take the patches through separate
trees, but I think this particular series is sufficiently interwoven to
warrant taking them all through one tree. However the least that we
should do is collect Acked-by's from the other tree maintainers.

Given that most of the patches modify files in drivers/video, Florian's
fbdev tree would be the most obvious candidate, right? If Florian agrees
to take the patches, all we would need is David's Acked-by.

How does that sound?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v12 0/6] of: add display helper

[Steffen Trumtrar]

On Tue, Nov 20, 2012 at 08:35:31PM +0100, Thierry Reding wrote:
> On Tue, Nov 20, 2012 at 07:11:29PM +0100, Robert Schwebel wrote:
> > On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> > > On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > > > Hi!
> > > > 
> > > > Changes since v11:
> > > > 	- make pointers const where applicable
> > > > 	- add reviewed-by Laurent Pinchart
> > > 
> > > Looks good to me.
> > > 
> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Through which tree do you plan to push this ?
> > 
> > We have no idea yet, and none of the people on Cc: have volunteered
> > so far... what do you think?
> 
> The customary approach would be to take the patches through separate
> trees, but I think this particular series is sufficiently interwoven to
> warrant taking them all through one tree. However the least that we
> should do is collect Acked-by's from the other tree maintainers.
> 
> Given that most of the patches modify files in drivers/video, Florian's
> fbdev tree would be the most obvious candidate, right? If Florian agrees
> to take the patches, all we would need is David's Acked-by.
> 
> How does that sound?
> 
> Thierry

Hi!

That is exactly the way I thought this could happen.

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

Re: [PATCH v12 0/6] of: add display helper

[Laurent Pinchart]

On Wednesday 21 November 2012 09:28:22 Steffen Trumtrar wrote:
> On Tue, Nov 20, 2012 at 08:35:31PM +0100, Thierry Reding wrote:
> > On Tue, Nov 20, 2012 at 07:11:29PM +0100, Robert Schwebel wrote:
> > > On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> > > > On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > > > > Hi!
> > > > > 
> > > > > Changes since v11:
> > > > > 	- make pointers const where applicable
> > > > > 	- add reviewed-by Laurent Pinchart
> > > > 
> > > > Looks good to me.
> > > > 
> > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > Through which tree do you plan to push this ?
> > > 
> > > We have no idea yet, and none of the people on Cc: have volunteered
> > > so far... what do you think?
> > 
> > The customary approach would be to take the patches through separate
> > trees, but I think this particular series is sufficiently interwoven to
> > warrant taking them all through one tree. However the least that we
> > should do is collect Acked-by's from the other tree maintainers.
> > 
> > Given that most of the patches modify files in drivers/video, Florian's
> > fbdev tree would be the most obvious candidate, right? If Florian agrees
> > to take the patches, all we would need is David's Acked-by.
> > 
> > How does that sound?
> > 
> > Thierry
> 
> Hi!
> 
> That is exactly the way I thought this could happen.

Sounds good to me as well.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v12 0/6] of: add display helper

[Steffen Trumtrar]

On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > Hi!
> > 
> > Changes since v11:
> > 	- make pointers const where applicable
> > 	- add reviewed-by Laurent Pinchart
> 
> Looks good to me.
> 
> Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> 

Thanks.

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