[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Sat, Oct 20, 2012 at 01:04:12PM +0200, Thierry Reding wrote:
> On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
> [...]
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> [...]
> > +#if defined(CONFIG_DRM)
> 
> This should be:
> 
>   #if IS_ENABLED(CONFIG_DRM)
> 
> or the code below won't be included if DRM is built as a module. But see
> my other replies as to how we can probably handle this better by moving
> this into the DRM subsystem.
> 

I already started with moving...now I only need some time to finish with it.

> > +int videomode_to_display_mode(struct videomode *vm, struct 
> > drm_display_mode *dmode)
> > +{
> > +   memset(dmode, 0, sizeof(*dmode));
> 
> It appears the usual method to obtain a drm_display_mode to allocate it
> using drm_mode_create(), which will allocate it and associate it with
> the struct drm_device.
> 
> Now, if you do a memset() on the structure you'll overwrite a number of
> fields that have previously been initialized and are actually required
> to get everything cleaned up properly later on.
> 
> So I think we should remove the call to memset().
> 

I was not aware of that. The memset has to go than, of course.

> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> > +   int index)
> > +{
> [...]
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
> 
> This should be:
> 
>   EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Oops.

Regrads,

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

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Sat, Oct 20, 2012 at 01:04:12PM +0200, Thierry Reding wrote:
 On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
 [...]
  diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
 [...]
  +#if defined(CONFIG_DRM)
 
 This should be:
 
   #if IS_ENABLED(CONFIG_DRM)
 
 or the code below won't be included if DRM is built as a module. But see
 my other replies as to how we can probably handle this better by moving
 this into the DRM subsystem.
 

I already started with moving...now I only need some time to finish with it.

  +int videomode_to_display_mode(struct videomode *vm, struct 
  drm_display_mode *dmode)
  +{
  +   memset(dmode, 0, sizeof(*dmode));
 
 It appears the usual method to obtain a drm_display_mode to allocate it
 using drm_mode_create(), which will allocate it and associate it with
 the struct drm_device.
 
 Now, if you do a memset() on the structure you'll overwrite a number of
 fields that have previously been initialized and are actually required
 to get everything cleaned up properly later on.
 
 So I think we should remove the call to memset().
 

I was not aware of that. The memset has to go than, of course.

  +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
  +   int index)
  +{
 [...]
  +}
  +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);
 
 This should be:
 
   EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Oops.

Regrads,

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- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
[...]
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
[...]
> +#if defined(CONFIG_DRM)

This should be:

#if IS_ENABLED(CONFIG_DRM)

or the code below won't be included if DRM is built as a module. But see
my other replies as to how we can probably handle this better by moving
this into the DRM subsystem.

> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
> *dmode)
> +{
> + memset(dmode, 0, sizeof(*dmode));

It appears the usual method to obtain a drm_display_mode to allocate it
using drm_mode_create(), which will allocate it and associate it with
the struct drm_device.

Now, if you do a memset() on the structure you'll overwrite a number of
fields that have previously been initialized and are actually required
to get everything cleaned up properly later on.

So I think we should remove the call to memset().

> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
> + int index)
> +{
[...]
> +}
> +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);

This should be:

EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Tue, Oct 09, 2012 at 09:26:08AM +0200, Steffen Trumtrar wrote:
> Hi Laurent,
> 
> On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
> > Hi Steffen,
> > 
> > On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> > > On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
[...]
> > > > > +int of_get_videomode(struct device_node *np, struct videomode *vm, 
> > > > > int
> > > > > index)
> > > > 
> > > > I wonder how to avoid abuse of this functions. It's a useful helper for
> > > > drivers that need to get a video mode once only, but would result in 
> > > > lower
> > > > performances if a driver calls it for every mode. Drivers must call
> > > > of_get_display_timing_list instead in that case and case the display
> > > > timings. I'm wondering whether we should really expose of_get_videomode.
> > > 
> > > The intent was to let the driver decide. That way all the other overhead 
> > > may
> > > be skipped.
> > 
> > My point is that driver writers might just call of_get_videomode() in a 
> > loop, 
> > not knowing that it's expensive. I want to avoid that. We need to at least 
> > add 
> > kerneldoc to the function stating that this shouldn't be done.
> > 
> 
> You're right. That should be made clear in the code.

In that case we should export videomode_from_timing(). For Tegra DRM I
wrote a small utility function that takes a struct display_timings and
converts every timing to a struct videomode which is then converted to
a struct drm_display_mode and added to the DRM connector. The code is
really generic and could be part of the DRM core.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Sun, Oct 07, 2012 at 03:38:33PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
> > On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> > > On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > > Get videomode from devicetree in a format appropriate for the
> > > > backend. drm_display_mode and fb_videomode are supported atm.
> > > > Uses the display signal timings from of_display_timings
> > > > 
> > > > +++ b/drivers/of/of_videomode.c
> > > > 
> > > > +int videomode_from_timing(struct display_timings *disp, struct
> > > > videomode *vm,
> > > > 
> > > > +   st = display_timings_get(disp, index);
> > > > +
> > > > +   if (!st) {
> > > 
> > > It's a little odd to leave a blank line between those two lines.
> > 
> > Hm, well okay. That can be remedied
> > 
> > > Only half of the code in this file seems OF-related; the routines to
> > > convert a timing to a videomode or drm display mode seem like they'd be
> > > useful outside device tree, so I wonder if putting them into
> > > of_videomode.c is the correct thing to do. Still, it's probably not a
> > > big deal.
> > 
> > I am not sure, what the appropriate way to do this is. I can split it up
> > (again).
> 
> I think it would make sense to move them to their respective subsystems.

I agree. While looking at integrating this for Tegra DRM, I came across
the issue that if I build DRM as a module, linking with this code will
fail. The reason for that was that it was that the code, itself builtin,
uses drm_mode_set_name(), which would be exported by the drm module. So
I had to modifiy the Kconfig entries to be "def_tristate DRM". That
obviously isn't very nice since the code can also be used without DRM.

Moving the subsystem specific conversion routines to the respective
subsystems should solve any of these issues.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

Re: [PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Sun, Oct 07, 2012 at 03:38:33PM +0200, Laurent Pinchart wrote:
 Hi Steffen,
 
 On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
  On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
   On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

+++ b/drivers/of/of_videomode.c

+int videomode_from_timing(struct display_timings *disp, struct
videomode *vm,

+   st = display_timings_get(disp, index);
+
+   if (!st) {
   
   It's a little odd to leave a blank line between those two lines.
  
  Hm, well okay. That can be remedied
  
   Only half of the code in this file seems OF-related; the routines to
   convert a timing to a videomode or drm display mode seem like they'd be
   useful outside device tree, so I wonder if putting them into
   of_videomode.c is the correct thing to do. Still, it's probably not a
   big deal.
  
  I am not sure, what the appropriate way to do this is. I can split it up
  (again).
 
 I think it would make sense to move them to their respective subsystems.

I agree. While looking at integrating this for Tegra DRM, I came across
the issue that if I build DRM as a module, linking with this code will
fail. The reason for that was that it was that the code, itself builtin,
uses drm_mode_set_name(), which would be exported by the drm module. So
I had to modifiy the Kconfig entries to be def_tristate DRM. That
obviously isn't very nice since the code can also be used without DRM.

Moving the subsystem specific conversion routines to the respective
subsystems should solve any of these issues.

Thierry


pgptSuZzA052R.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Tue, Oct 09, 2012 at 09:26:08AM +0200, Steffen Trumtrar wrote:
 Hi Laurent,
 
 On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
  Hi Steffen,
  
  On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
   On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
[...]
 +int of_get_videomode(struct device_node *np, struct videomode *vm, 
 int
 index)

I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in 
lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display
timings. I'm wondering whether we should really expose of_get_videomode.
   
   The intent was to let the driver decide. That way all the other overhead 
   may
   be skipped.
  
  My point is that driver writers might just call of_get_videomode() in a 
  loop, 
  not knowing that it's expensive. I want to avoid that. We need to at least 
  add 
  kerneldoc to the function stating that this shouldn't be done.
  
 
 You're right. That should be made clear in the code.

In that case we should export videomode_from_timing(). For Tegra DRM I
wrote a small utility function that takes a struct display_timings and
converts every timing to a struct videomode which is then converted to
a struct drm_display_mode and added to the DRM connector. The code is
really generic and could be part of the DRM core.

Thierry


pgpaJWsrATopQ.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v6] of: add generic videomode description

[Thierry Reding]

On Thu, Oct 04, 2012 at 07:59:20PM +0200, Steffen Trumtrar wrote:
[...]
 diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
[...]
 +#if defined(CONFIG_DRM)

This should be:

#if IS_ENABLED(CONFIG_DRM)

or the code below won't be included if DRM is built as a module. But see
my other replies as to how we can probably handle this better by moving
this into the DRM subsystem.

 +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
 *dmode)
 +{
 + memset(dmode, 0, sizeof(*dmode));

It appears the usual method to obtain a drm_display_mode to allocate it
using drm_mode_create(), which will allocate it and associate it with
the struct drm_device.

Now, if you do a memset() on the structure you'll overwrite a number of
fields that have previously been initialized and are actually required
to get everything cleaned up properly later on.

So I think we should remove the call to memset().

 +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb,
 + int index)
 +{
[...]
 +}
 +EXPORT_SYMBOL_GPL(of_get_drm_display_mode);

This should be:

EXPORT_SYMBOL_GPL(of_get_fb_videomode);

Thierry


pgpbnzWtFv0Wa.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

Hi Laurent,

On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> > On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > > > Get videomode from devicetree in a format appropriate for the
> > > > backend. drm_display_mode and fb_videomode are supported atm.
> > > > Uses the display signal timings from of_display_timings
> > > > 
> > > > Signed-off-by: Steffen Trumtrar 
> > > > ---
> > > > 
> > > >  drivers/of/Kconfig   |5 +
> > > >  drivers/of/Makefile  |1 +
> > > >  drivers/of/of_videomode.c|  212 +++
> > > >  include/linux/of_videomode.h |   41 
> > > >  4 files changed, 259 insertions(+)
> > > >  create mode 100644 drivers/of/of_videomode.c
> > > >  create mode 100644 include/linux/of_videomode.h
> 
> [snip]
> 
> > > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > > new file mode 100644
> > > > index 000..76ac16e
> > > > --- /dev/null
> > > > +++ b/drivers/of/of_videomode.c
> 
> [snip]
> 
> > > > +int videomode_from_timing(struct display_timings *disp, struct
> > > > videomode *vm,
> > > > +   int index)
> > > > +{
> > > > +   struct signal_timing *st = NULL;
> > > > +
> > > > +   if (!vm)
> > > > +   return -EINVAL;
> > > > +
> > > 
> > > What about making vm a mandatory argument ? It looks to me like a caller
> > > bug if vm is NULL.
> > 
> > The caller must provide the struct videomode, yes. Wouldn't the kernel hang
> > itself with a NULL pointer exception, if I just work with it ?
> 
> The kernel would oops, clearly showing the caller that a non-null vm is 
> needed 
> :-)
> 

Okay. No error checking it is then.

> > > > +   st = display_timings_get(disp, index);
> > > > +
> > > 
> > > You can remove the blank line.
> > > 
> > > > +   if (!st) {
> > > > +   pr_err("%s: no signal timings found\n", __func__);
> > > > +   return -EINVAL;
> > > > +   }
> > > > +
> > > > +   vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> > > > +   vm->hactive = signal_timing_get_value(>hactive, 0);
> > > > +   vm->hfront_porch = signal_timing_get_value(>hfront_porch, 
> > > > 0);
> > > > +   vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> > > > +   vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> > > > +
> > > > +   vm->vactive = signal_timing_get_value(>vactive, 0);
> > > > +   vm->vfront_porch = signal_timing_get_value(>vfront_porch, 
> > > > 0);
> > > > +   vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> > > > +   vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> > > > +
> > > > +   vm->vah = st->vsync_pol_active_high;
> > > > +   vm->hah = st->hsync_pol_active_high;
> > > > +   vm->interlaced = st->interlaced;
> > > > +   vm->doublescan = st->doublescan;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > > index)
> > > 
> > > I wonder how to avoid abuse of this functions. It's a useful helper for
> > > drivers that need to get a video mode once only, but would result in lower
> > > performances if a driver calls it for every mode. Drivers must call
> > > of_get_display_timing_list instead in that case and case the display
> > > timings. I'm wondering whether we should really expose of_get_videomode.
> > 
> > The intent was to let the driver decide. That way all the other overhead may
> > be skipped.
> 
> My point is that driver writers might just call of_get_videomode() in a loop, 
> not knowing that it's expensive. I want to avoid that. We need to at least 
> add 
> kerneldoc to the function stating that this shouldn't be done.
> 

You're right. That should be made clear in the code.

> > > > +{
> > > > +   struct display_timings *disp;
> > > > +   int ret = 0;
> > > 
> > > No need to assign ret to 0 here.
> > 
> > Ah, yes. Unneeded in this case.
> > 
> > > > +
> > > > +   disp = of_get_display_timing_list(np);
> > > > +
> > > 
> > > You can remove the blank line.
> > > 
> > > > +   if (!disp) {
> > > > +   pr_err("%s: no timings specified\n", __func__);
> > > > +   return -EINVAL;
> > > > +   }
> > > > +
> > > > +   if (index == OF_DEFAULT_TIMING)
> > > > +   index = disp->default_timing;
> > > > +
> > > > +   ret = videomode_from_timing(disp, vm, index);
> > > > +
> > > 
> > > No need for a blank line.
> > > 
> > > > +   if (ret)
> > > > +   return ret;
> > > > +
> > > > +   display_timings_release(disp);
> > > > +
> > > > +   if (!vm) {
> > > > +   pr_err("%s: could not get videomode %d\n", __func__, 
> > > > index);
> > > > +   return -EINVAL;
> > > > +   

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

Hi Laurent,

On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
 Hi Steffen,
 
 On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
  On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
   On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
---

 drivers/of/Kconfig   |5 +
 drivers/of/Makefile  |1 +
 drivers/of/of_videomode.c|  212 +++
 include/linux/of_videomode.h |   41 
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h
 
 [snip]
 
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 000..76ac16e
--- /dev/null
+++ b/drivers/of/of_videomode.c
 
 [snip]
 
+int videomode_from_timing(struct display_timings *disp, struct
videomode *vm,
+   int index)
+{
+   struct signal_timing *st = NULL;
+
+   if (!vm)
+   return -EINVAL;
+
   
   What about making vm a mandatory argument ? It looks to me like a caller
   bug if vm is NULL.
  
  The caller must provide the struct videomode, yes. Wouldn't the kernel hang
  itself with a NULL pointer exception, if I just work with it ?
 
 The kernel would oops, clearly showing the caller that a non-null vm is 
 needed 
 :-)
 

Okay. No error checking it is then.

+   st = display_timings_get(disp, index);
+
   
   You can remove the blank line.
   
+   if (!st) {
+   pr_err(%s: no signal timings found\n, __func__);
+   return -EINVAL;
+   }
+
+   vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
+   vm-hactive = signal_timing_get_value(st-hactive, 0);
+   vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 
0);
+   vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
+   vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
+
+   vm-vactive = signal_timing_get_value(st-vactive, 0);
+   vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 
0);
+   vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
+   vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
+
+   vm-vah = st-vsync_pol_active_high;
+   vm-hah = st-hsync_pol_active_high;
+   vm-interlaced = st-interlaced;
+   vm-doublescan = st-doublescan;
+
+   return 0;
+}
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int
index)
   
   I wonder how to avoid abuse of this functions. It's a useful helper for
   drivers that need to get a video mode once only, but would result in lower
   performances if a driver calls it for every mode. Drivers must call
   of_get_display_timing_list instead in that case and case the display
   timings. I'm wondering whether we should really expose of_get_videomode.
  
  The intent was to let the driver decide. That way all the other overhead may
  be skipped.
 
 My point is that driver writers might just call of_get_videomode() in a loop, 
 not knowing that it's expensive. I want to avoid that. We need to at least 
 add 
 kerneldoc to the function stating that this shouldn't be done.
 

You're right. That should be made clear in the code.

+{
+   struct display_timings *disp;
+   int ret = 0;
   
   No need to assign ret to 0 here.
  
  Ah, yes. Unneeded in this case.
  
+
+   disp = of_get_display_timing_list(np);
+
   
   You can remove the blank line.
   
+   if (!disp) {
+   pr_err(%s: no timings specified\n, __func__);
+   return -EINVAL;
+   }
+
+   if (index == OF_DEFAULT_TIMING)
+   index = disp-default_timing;
+
+   ret = videomode_from_timing(disp, vm, index);
+
   
   No need for a blank line.
   
+   if (ret)
+   return ret;
+
+   display_timings_release(disp);
+
+   if (!vm) {
+   pr_err(%s: could not get videomode %d\n, __func__, 
index);
+   return -EINVAL;
+   }
   
   This can't happen. If vm is NULL the videomode_from_timing call above will
   return -EINVAL, and this function will then return immediately without
   reaching this code block.
  
  Okay.
  
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
 
 -- 
 Regards,
 
 Laurent Pinchart
 
 

Regards,

Steffen

-- 
Pengutronix e.K.   | |

[PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > > Get videomode from devicetree in a format appropriate for the
> > > backend. drm_display_mode and fb_videomode are supported atm.
> > > Uses the display signal timings from of_display_timings
> > > 
> > > Signed-off-by: Steffen Trumtrar 
> > > ---
> > > 
> > >  drivers/of/Kconfig   |5 +
> > >  drivers/of/Makefile  |1 +
> > >  drivers/of/of_videomode.c|  212 +++
> > >  include/linux/of_videomode.h |   41 
> > >  4 files changed, 259 insertions(+)
> > >  create mode 100644 drivers/of/of_videomode.c
> > >  create mode 100644 include/linux/of_videomode.h

[snip]

> > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > new file mode 100644
> > > index 000..76ac16e
> > > --- /dev/null
> > > +++ b/drivers/of/of_videomode.c

[snip]

> > > +int videomode_from_timing(struct display_timings *disp, struct
> > > videomode *vm,
> > > + int index)
> > > +{
> > > + struct signal_timing *st = NULL;
> > > +
> > > + if (!vm)
> > > + return -EINVAL;
> > > +
> > 
> > What about making vm a mandatory argument ? It looks to me like a caller
> > bug if vm is NULL.
> 
> The caller must provide the struct videomode, yes. Wouldn't the kernel hang
> itself with a NULL pointer exception, if I just work with it ?

The kernel would oops, clearly showing the caller that a non-null vm is needed 
:-)

> > > + st = display_timings_get(disp, index);
> > > +
> > 
> > You can remove the blank line.
> > 
> > > + if (!st) {
> > > + pr_err("%s: no signal timings found\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> > > + vm->hactive = signal_timing_get_value(>hactive, 0);
> > > + vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
> > > + vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> > > + vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> > > +
> > > + vm->vactive = signal_timing_get_value(>vactive, 0);
> > > + vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
> > > + vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> > > + vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> > > +
> > > + vm->vah = st->vsync_pol_active_high;
> > > + vm->hah = st->hsync_pol_active_high;
> > > + vm->interlaced = st->interlaced;
> > > + vm->doublescan = st->doublescan;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > index)
> > 
> > I wonder how to avoid abuse of this functions. It's a useful helper for
> > drivers that need to get a video mode once only, but would result in lower
> > performances if a driver calls it for every mode. Drivers must call
> > of_get_display_timing_list instead in that case and case the display
> > timings. I'm wondering whether we should really expose of_get_videomode.
> 
> The intent was to let the driver decide. That way all the other overhead may
> be skipped.

My point is that driver writers might just call of_get_videomode() in a loop, 
not knowing that it's expensive. I want to avoid that. We need to at least add 
kerneldoc to the function stating that this shouldn't be done.

> > > +{
> > > + struct display_timings *disp;
> > > + int ret = 0;
> > 
> > No need to assign ret to 0 here.
> 
> Ah, yes. Unneeded in this case.
> 
> > > +
> > > + disp = of_get_display_timing_list(np);
> > > +
> > 
> > You can remove the blank line.
> > 
> > > + if (!disp) {
> > > + pr_err("%s: no timings specified\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (index == OF_DEFAULT_TIMING)
> > > + index = disp->default_timing;
> > > +
> > > + ret = videomode_from_timing(disp, vm, index);
> > > +
> > 
> > No need for a blank line.
> > 
> > > + if (ret)
> > > + return ret;
> > > +
> > > + display_timings_release(disp);
> > > +
> > > + if (!vm) {
> > > + pr_err("%s: could not get videomode %d\n", __func__, index);
> > > + return -EINVAL;
> > > + }
> > 
> > This can't happen. If vm is NULL the videomode_from_timing call above will
> > return -EINVAL, and this function will then return immediately without
> > reaching this code block.
> 
> Okay.
> 
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_videomode);

-- 
Regards,

Laurent Pinchart

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> Thanks for the patch.
> 
> On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> > 
> > Signed-off-by: Steffen Trumtrar 
> > ---
> >  drivers/of/Kconfig   |5 +
> >  drivers/of/Makefile  |1 +
> >  drivers/of/of_videomode.c|  212 +++
> >  include/linux/of_videomode.h |   41 
> >  4 files changed, 259 insertions(+)
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 646deb0..74282e2 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
> > help
> >   helper to parse display timings from the devicetree
> > 
> > +config OF_VIDEOMODE
> > +   def_bool y
> > +   help
> > + helper to get videomodes from the devicetree
> > +
> >  endmenu # OF
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index c8e9603..09d556f 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
> >  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
> >  obj-$(CONFIG_OF_MTD)   += of_mtd.o
> >  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 000..76ac16e
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * generic videomode helper
> > + *
> > + * Copyright (c) 2012 Steffen Trumtrar ,
> > Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +void dump_fb_videomode(struct fb_videomode *m)
> > +{
> > +pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> 
> That's going to be pretty difficult to read :-) Would it make sense to group 
> several attributes logically (for instance using %ux%u for m->xres, m->yres) ?
> 

No problem. That can be changed.

> > +m->refresh, m->xres, m->yres, 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);
> > +}
> 
> Shouldn't this (and the other non exported functions below) be static ?
> 

Yes.

> > +void dump_drm_displaymode(struct drm_display_mode *m)
> > +{
> > +pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
> > +m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
> > +m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
> > +m->clock);
> > +}
> > +
> > +int videomode_from_timing(struct display_timings *disp, struct videomode
> > *vm,
> > +   int index)
> > +{
> > +   struct signal_timing *st = NULL;
> > +
> > +   if (!vm)
> > +   return -EINVAL;
> > +
> 
> What about making vm a mandatory argument ? It looks to me like a caller bug 
> if vm is NULL.
> 

The caller must provide the struct videomode, yes. Wouldn't the kernel hang 
itself
with a NULL pointer exception, if I just work with it ?

> > +   st = display_timings_get(disp, index);
> > +
> 
> You can remove the blank line.
> 
> > +   if (!st) {
> > +   pr_err("%s: no signal timings found\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> > +   vm->hactive = signal_timing_get_value(>hactive, 0);
> > +   vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
> > +   vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> > +   vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> > +
> > +   vm->vactive = signal_timing_get_value(>vactive, 0);
> > +   vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
> > +   vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> > +   vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> > +
> > +   vm->vah = st->vsync_pol_active_high;
> > +   vm->hah = st->hsync_pol_active_high;
> > +   vm->interlaced = st->interlaced;
> > +   vm->doublescan = st->doublescan;
> > +
> > +   return 0;
> > +}
> > +
> > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > index)
> 
> I wonder how to avoid abuse of this functions. It's a useful helper for 
> drivers that need to get a video mode once only, but would result in lower 
> performances if a driver calls it for every mode. Drivers must call 
> of_get_display_timing_list instead in that case and case the display timings. 
> I'm 

[PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 09:57:41 Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:

[snip]

> > > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > > new file mode 100644
> > > index 000..96efe01
> > > --- /dev/null
> > > +++ b/include/linux/of_videomode.h
> > > @@ -0,0 +1,41 @@
> > > +/*
> > > + * Copyright 2012 Steffen Trumtrar 
> > > + *
> > > + * generic videomode description
> > > + *
> > > + * This file is released under the GPLv2
> > > + */
> > > +
> > > +#ifndef __LINUX_VIDEOMODE_H
> > > +#define __LINUX_VIDEOMODE_H
> > > +
> > > +#include 
> > 
> > You don't need to include this.
> 
> That is a fix to my liking. Easily done ;-)
> 
> > > +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;
> > > +
> > > + bool hah;
> > > + bool vah;
> > > + bool interlaced;
> > > + bool doublescan;
> > > +
> > > +};
> > 
> > This is not really of related. And actually, neither is the struct
> > signal_timing in the previous patch. It would be nice to have these in a
> > common header that fb, drm, and others could use instead of each having
> > their own timing structs.
> > 
> > But that's probably out of scope for this series =). Did you check the
> > timing structs from the video related frameworks in the kernel to see if
> > your structs contain all the info the others have, so that, at least in
> > theory, everybody could use these common structs?
> > 
> >  Tomi
> 
> Yes. Stephen and Laurent already suggested to split it up.
> No, all info is not contained. That starts with drm, which has width-mm,..
> If time permits, I will go over that.

Just to make sure we won't forget it, the V4L2 version of the timings 
structure is struct v4l2_bt_timings in include/linux/videodev2.h.

-- 
Regards,

Laurent Pinchart

[PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

Thanks for the patch.

On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
> 
> Signed-off-by: Steffen Trumtrar 
> ---
>  drivers/of/Kconfig   |5 +
>  drivers/of/Makefile  |1 +
>  drivers/of/of_videomode.c|  212 +++
>  include/linux/of_videomode.h |   41 
>  4 files changed, 259 insertions(+)
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 646deb0..74282e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
>   help
> helper to parse display timings from the devicetree
> 
> +config OF_VIDEOMODE
> + def_bool y
> + help
> +   helper to get videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c8e9603..09d556f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)+= of_pci.o
>  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD) += of_mtd.o
>  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 000..76ac16e
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,212 @@
> +/*
> + * generic videomode helper
> + *
> + * Copyright (c) 2012 Steffen Trumtrar ,
> Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +void dump_fb_videomode(struct fb_videomode *m)
> +{
> +pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",

That's going to be pretty difficult to read :-) Would it make sense to group 
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?

> +m->refresh, m->xres, m->yres, 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);
> +}

Shouldn't this (and the other non exported functions below) be static ?

> +void dump_drm_displaymode(struct drm_display_mode *m)
> +{
> +pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
> +m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
> +m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
> +m->clock);
> +}
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode
> *vm,
> + int index)
> +{
> + struct signal_timing *st = NULL;
> +
> + if (!vm)
> + return -EINVAL;
> +

What about making vm a mandatory argument ? It looks to me like a caller bug 
if vm is NULL.

> + st = display_timings_get(disp, index);
> +

You can remove the blank line.

> + if (!st) {
> + pr_err("%s: no signal timings found\n", __func__);
> + return -EINVAL;
> + }
> +
> + vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> + vm->hactive = signal_timing_get_value(>hactive, 0);
> + vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
> + vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> + vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> +
> + vm->vactive = signal_timing_get_value(>vactive, 0);
> + vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
> + vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> + vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> +
> + vm->vah = st->vsync_pol_active_high;
> + vm->hah = st->hsync_pol_active_high;
> + vm->interlaced = st->interlaced;
> + vm->doublescan = st->doublescan;
> +
> + return 0;
> +}
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> index)

I wonder how to avoid abuse of this functions. It's a useful helper for 
drivers that need to get a video mode once only, but would result in lower 
performances if a driver calls it for every mode. Drivers must call 
of_get_display_timing_list instead in that case and case the display timings. 
I'm wondering whether we should really expose of_get_videomode.

> +{
> + struct display_timings *disp;
> + int ret = 0;

No need to assign ret to 0 here.

> +
> + disp = of_get_display_timing_list(np);
> +

You can remove the blank line.

> + if (!disp) {
> + pr_err("%s: no timings specified\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (index == OF_DEFAULT_TIMING)
> + index = disp->default_timing;
> +
> +

[PATCH 2/2 v6] of: add generic videomode description

[Tomi Valkeinen]

On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
> 
> Signed-off-by: Steffen Trumtrar 
> ---
>  drivers/of/Kconfig   |5 +
>  drivers/of/Makefile  |1 +
>  drivers/of/of_videomode.c|  212 
> ++
>  include/linux/of_videomode.h |   41 
>  4 files changed, 259 insertions(+)
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 646deb0..74282e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
>   help
> helper to parse display timings from the devicetree
>  
> +config OF_VIDEOMODE
> + def_bool y
> + help
> +   helper to get videomodes from the devicetree
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index c8e9603..09d556f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)+= of_pci.o
>  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>  obj-$(CONFIG_OF_MTD) += of_mtd.o
>  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 000..76ac16e
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,212 @@
> +/*
> + * generic videomode helper
> + *
> + * Copyright (c) 2012 Steffen Trumtrar , 
> Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +void dump_fb_videomode(struct fb_videomode *m)
> +{
> +pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> +m->refresh, m->xres, m->yres, 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);
> +}
> +
> +void dump_drm_displaymode(struct drm_display_mode *m)
> +{
> +pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
> +m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
> +m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
> +m->clock);
> +}
> +
> +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> + int index)
> +{
> + struct signal_timing *st = NULL;
> +
> + if (!vm)
> + return -EINVAL;
> +
> + st = display_timings_get(disp, index);
> +
> + if (!st) {
> + pr_err("%s: no signal timings found\n", __func__);
> + return -EINVAL;
> + }
> +
> + vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> + vm->hactive = signal_timing_get_value(>hactive, 0);
> + vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
> + vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> + vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> +
> + vm->vactive = signal_timing_get_value(>vactive, 0);
> + vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
> + vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> + vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> +
> + vm->vah = st->vsync_pol_active_high;
> + vm->hah = st->hsync_pol_active_high;
> + vm->interlaced = st->interlaced;
> + vm->doublescan = st->doublescan;
> +
> + return 0;
> +}
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
> +{
> + struct display_timings *disp;
> + int ret = 0;
> +
> + disp = of_get_display_timing_list(np);
> +
> + if (!disp) {
> + pr_err("%s: no timings specified\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (index == OF_DEFAULT_TIMING)
> + index = disp->default_timing;
> +
> + ret = videomode_from_timing(disp, vm, index);
> +
> + if (ret)
> + return ret;
> +
> + display_timings_release(disp);
> +
> + if (!vm) {
> + pr_err("%s: could not get videomode %d\n", __func__, index);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_videomode);
> +
> +#if defined(CONFIG_DRM)
> +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
> *dmode)
> +{
> + memset(dmode, 0, sizeof(*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 = 

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> > 
> > Signed-off-by: Steffen Trumtrar 
> > ---
> >  drivers/of/Kconfig   |5 +
> >  drivers/of/Makefile  |1 +
> >  drivers/of/of_videomode.c|  212 
> > ++
> >  include/linux/of_videomode.h |   41 
> >  4 files changed, 259 insertions(+)
> >  create mode 100644 drivers/of/of_videomode.c
> >  create mode 100644 include/linux/of_videomode.h
> > 
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 646deb0..74282e2 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
> > help
> >   helper to parse display timings from the devicetree
> >  
> > +config OF_VIDEOMODE
> > +   def_bool y
> > +   help
> > + helper to get videomodes from the devicetree
> > +
> >  endmenu # OF
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index c8e9603..09d556f 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
> >  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
> >  obj-$(CONFIG_OF_MTD)   += of_mtd.o
> >  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 000..76ac16e
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * generic videomode helper
> > + *
> > + * Copyright (c) 2012 Steffen Trumtrar , 
> > Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +void dump_fb_videomode(struct fb_videomode *m)
> > +{
> > +pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> > +m->refresh, m->xres, m->yres, 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);
> > +}
> > +
> > +void dump_drm_displaymode(struct drm_display_mode *m)
> > +{
> > +pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
> > +m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
> > +m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
> > +m->clock);
> > +}
> > +
> > +int videomode_from_timing(struct display_timings *disp, struct videomode 
> > *vm,
> > +   int index)
> > +{
> > +   struct signal_timing *st = NULL;
> > +
> > +   if (!vm)
> > +   return -EINVAL;
> > +
> > +   st = display_timings_get(disp, index);
> > +
> > +   if (!st) {
> > +   pr_err("%s: no signal timings found\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
> > +   vm->hactive = signal_timing_get_value(>hactive, 0);
> > +   vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
> > +   vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
> > +   vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
> > +
> > +   vm->vactive = signal_timing_get_value(>vactive, 0);
> > +   vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
> > +   vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
> > +   vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
> > +
> > +   vm->vah = st->vsync_pol_active_high;
> > +   vm->hah = st->hsync_pol_active_high;
> > +   vm->interlaced = st->interlaced;
> > +   vm->doublescan = st->doublescan;
> > +
> > +   return 0;
> > +}
> > +
> > +int of_get_videomode(struct device_node *np, struct videomode *vm, int 
> > index)
> > +{
> > +   struct display_timings *disp;
> > +   int ret = 0;
> > +
> > +   disp = of_get_display_timing_list(np);
> > +
> > +   if (!disp) {
> > +   pr_err("%s: no timings specified\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   if (index == OF_DEFAULT_TIMING)
> > +   index = disp->default_timing;
> > +
> > +   ret = videomode_from_timing(disp, vm, index);
> > +
> > +   if (ret)
> > +   return ret;
> > +
> > +   display_timings_release(disp);
> > +
> > +   if (!vm) {
> > +   pr_err("%s: could not get videomode %d\n", __func__, index);
> > +   return -EINVAL;
> > +   }
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_videomode);
> > +
> > +#if defined(CONFIG_DRM)
> > +int videomode_to_display_mode(struct videomode *vm, struct 
> > drm_display_mode *dmode)
> > +{
> > +   memset(dmode, 0, sizeof(*dmode));
> > +
> > +   dmode->hdisplay 

Re: [PATCH 2/2 v6] of: add generic videomode description

[Tomi Valkeinen]

On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
 Get videomode from devicetree in a format appropriate for the
 backend. drm_display_mode and fb_videomode are supported atm.
 Uses the display signal timings from of_display_timings
 
 Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
 ---
  drivers/of/Kconfig   |5 +
  drivers/of/Makefile  |1 +
  drivers/of/of_videomode.c|  212 
 ++
  include/linux/of_videomode.h |   41 
  4 files changed, 259 insertions(+)
  create mode 100644 drivers/of/of_videomode.c
  create mode 100644 include/linux/of_videomode.h
 
 diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
 index 646deb0..74282e2 100644
 --- a/drivers/of/Kconfig
 +++ b/drivers/of/Kconfig
 @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
   help
 helper to parse display timings from the devicetree
  
 +config OF_VIDEOMODE
 + def_bool y
 + help
 +   helper to get videomodes from the devicetree
 +
  endmenu # OF
 diff --git a/drivers/of/Makefile b/drivers/of/Makefile
 index c8e9603..09d556f 100644
 --- a/drivers/of/Makefile
 +++ b/drivers/of/Makefile
 @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)+= of_pci.o
  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
  obj-$(CONFIG_OF_MTD) += of_mtd.o
  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
 +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
 diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
 new file mode 100644
 index 000..76ac16e
 --- /dev/null
 +++ b/drivers/of/of_videomode.c
 @@ -0,0 +1,212 @@
 +/*
 + * generic videomode helper
 + *
 + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de, 
 Pengutronix
 + *
 + * This file is released under the GPLv2
 + */
 +#include linux/of.h
 +#include linux/fb.h
 +#include linux/slab.h
 +#include drm/drm_mode.h
 +#include linux/of_display_timings.h
 +#include linux/of_videomode.h
 +
 +void dump_fb_videomode(struct fb_videomode *m)
 +{
 +pr_debug(fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n,
 +m-refresh, m-xres, m-yres, 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);
 +}
 +
 +void dump_drm_displaymode(struct drm_display_mode *m)
 +{
 +pr_debug(drm_displaymode = %d %d %d %d %d %d %d %d %d\n,
 +m-hdisplay, m-hsync_start, m-hsync_end, m-htotal,
 +m-vdisplay, m-vsync_start, m-vsync_end, m-vtotal,
 +m-clock);
 +}
 +
 +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
 + int index)
 +{
 + struct signal_timing *st = NULL;
 +
 + if (!vm)
 + return -EINVAL;
 +
 + st = display_timings_get(disp, index);
 +
 + if (!st) {
 + pr_err(%s: no signal timings found\n, __func__);
 + return -EINVAL;
 + }
 +
 + vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
 + vm-hactive = signal_timing_get_value(st-hactive, 0);
 + vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
 + vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
 + vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
 +
 + vm-vactive = signal_timing_get_value(st-vactive, 0);
 + vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
 + vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
 + vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
 +
 + vm-vah = st-vsync_pol_active_high;
 + vm-hah = st-hsync_pol_active_high;
 + vm-interlaced = st-interlaced;
 + vm-doublescan = st-doublescan;
 +
 + return 0;
 +}
 +
 +int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
 +{
 + struct display_timings *disp;
 + int ret = 0;
 +
 + disp = of_get_display_timing_list(np);
 +
 + if (!disp) {
 + pr_err(%s: no timings specified\n, __func__);
 + return -EINVAL;
 + }
 +
 + if (index == OF_DEFAULT_TIMING)
 + index = disp-default_timing;
 +
 + ret = videomode_from_timing(disp, vm, index);
 +
 + if (ret)
 + return ret;
 +
 + display_timings_release(disp);
 +
 + if (!vm) {
 + pr_err(%s: could not get videomode %d\n, __func__, index);
 + return -EINVAL;
 + }
 +
 + return 0;
 +}
 +EXPORT_SYMBOL_GPL(of_get_videomode);
 +
 +#if defined(CONFIG_DRM)
 +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
 *dmode)
 +{
 + memset(dmode, 0, sizeof(*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 = 

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
 On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
  Get videomode from devicetree in a format appropriate for the
  backend. drm_display_mode and fb_videomode are supported atm.
  Uses the display signal timings from of_display_timings
  
  Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
  ---
   drivers/of/Kconfig   |5 +
   drivers/of/Makefile  |1 +
   drivers/of/of_videomode.c|  212 
  ++
   include/linux/of_videomode.h |   41 
   4 files changed, 259 insertions(+)
   create mode 100644 drivers/of/of_videomode.c
   create mode 100644 include/linux/of_videomode.h
  
  diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
  index 646deb0..74282e2 100644
  --- a/drivers/of/Kconfig
  +++ b/drivers/of/Kconfig
  @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
  help
helper to parse display timings from the devicetree
   
  +config OF_VIDEOMODE
  +   def_bool y
  +   help
  + helper to get videomodes from the devicetree
  +
   endmenu # OF
  diff --git a/drivers/of/Makefile b/drivers/of/Makefile
  index c8e9603..09d556f 100644
  --- a/drivers/of/Makefile
  +++ b/drivers/of/Makefile
  @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
   obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
   obj-$(CONFIG_OF_MTD)   += of_mtd.o
   obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
  +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
  diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
  new file mode 100644
  index 000..76ac16e
  --- /dev/null
  +++ b/drivers/of/of_videomode.c
  @@ -0,0 +1,212 @@
  +/*
  + * generic videomode helper
  + *
  + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de, 
  Pengutronix
  + *
  + * This file is released under the GPLv2
  + */
  +#include linux/of.h
  +#include linux/fb.h
  +#include linux/slab.h
  +#include drm/drm_mode.h
  +#include linux/of_display_timings.h
  +#include linux/of_videomode.h
  +
  +void dump_fb_videomode(struct fb_videomode *m)
  +{
  +pr_debug(fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n,
  +m-refresh, m-xres, m-yres, 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);
  +}
  +
  +void dump_drm_displaymode(struct drm_display_mode *m)
  +{
  +pr_debug(drm_displaymode = %d %d %d %d %d %d %d %d %d\n,
  +m-hdisplay, m-hsync_start, m-hsync_end, m-htotal,
  +m-vdisplay, m-vsync_start, m-vsync_end, m-vtotal,
  +m-clock);
  +}
  +
  +int videomode_from_timing(struct display_timings *disp, struct videomode 
  *vm,
  +   int index)
  +{
  +   struct signal_timing *st = NULL;
  +
  +   if (!vm)
  +   return -EINVAL;
  +
  +   st = display_timings_get(disp, index);
  +
  +   if (!st) {
  +   pr_err(%s: no signal timings found\n, __func__);
  +   return -EINVAL;
  +   }
  +
  +   vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
  +   vm-hactive = signal_timing_get_value(st-hactive, 0);
  +   vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
  +   vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
  +   vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
  +
  +   vm-vactive = signal_timing_get_value(st-vactive, 0);
  +   vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
  +   vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
  +   vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
  +
  +   vm-vah = st-vsync_pol_active_high;
  +   vm-hah = st-hsync_pol_active_high;
  +   vm-interlaced = st-interlaced;
  +   vm-doublescan = st-doublescan;
  +
  +   return 0;
  +}
  +
  +int of_get_videomode(struct device_node *np, struct videomode *vm, int 
  index)
  +{
  +   struct display_timings *disp;
  +   int ret = 0;
  +
  +   disp = of_get_display_timing_list(np);
  +
  +   if (!disp) {
  +   pr_err(%s: no timings specified\n, __func__);
  +   return -EINVAL;
  +   }
  +
  +   if (index == OF_DEFAULT_TIMING)
  +   index = disp-default_timing;
  +
  +   ret = videomode_from_timing(disp, vm, index);
  +
  +   if (ret)
  +   return ret;
  +
  +   display_timings_release(disp);
  +
  +   if (!vm) {
  +   pr_err(%s: could not get videomode %d\n, __func__, index);
  +   return -EINVAL;
  +   }
  +
  +   return 0;
  +}
  +EXPORT_SYMBOL_GPL(of_get_videomode);
  +
  +#if defined(CONFIG_DRM)
  +int videomode_to_display_mode(struct videomode *vm, struct 
  drm_display_mode *dmode)
  +{
  +   memset(dmode, 0, sizeof(*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;
  

Re: [PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

Thanks for the patch.

On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
 Get videomode from devicetree in a format appropriate for the
 backend. drm_display_mode and fb_videomode are supported atm.
 Uses the display signal timings from of_display_timings
 
 Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
 ---
  drivers/of/Kconfig   |5 +
  drivers/of/Makefile  |1 +
  drivers/of/of_videomode.c|  212 +++
  include/linux/of_videomode.h |   41 
  4 files changed, 259 insertions(+)
  create mode 100644 drivers/of/of_videomode.c
  create mode 100644 include/linux/of_videomode.h
 
 diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
 index 646deb0..74282e2 100644
 --- a/drivers/of/Kconfig
 +++ b/drivers/of/Kconfig
 @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
   help
 helper to parse display timings from the devicetree
 
 +config OF_VIDEOMODE
 + def_bool y
 + help
 +   helper to get videomodes from the devicetree
 +
  endmenu # OF
 diff --git a/drivers/of/Makefile b/drivers/of/Makefile
 index c8e9603..09d556f 100644
 --- a/drivers/of/Makefile
 +++ b/drivers/of/Makefile
 @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)+= of_pci.o
  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
  obj-$(CONFIG_OF_MTD) += of_mtd.o
  obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
 +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
 diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
 new file mode 100644
 index 000..76ac16e
 --- /dev/null
 +++ b/drivers/of/of_videomode.c
 @@ -0,0 +1,212 @@
 +/*
 + * generic videomode helper
 + *
 + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de,
 Pengutronix
 + *
 + * This file is released under the GPLv2
 + */
 +#include linux/of.h
 +#include linux/fb.h
 +#include linux/slab.h
 +#include drm/drm_mode.h
 +#include linux/of_display_timings.h
 +#include linux/of_videomode.h
 +
 +void dump_fb_videomode(struct fb_videomode *m)
 +{
 +pr_debug(fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n,

That's going to be pretty difficult to read :-) Would it make sense to group 
several attributes logically (for instance using %ux%u for m-xres, m-yres) ?

 +m-refresh, m-xres, m-yres, 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);
 +}

Shouldn't this (and the other non exported functions below) be static ?

 +void dump_drm_displaymode(struct drm_display_mode *m)
 +{
 +pr_debug(drm_displaymode = %d %d %d %d %d %d %d %d %d\n,
 +m-hdisplay, m-hsync_start, m-hsync_end, m-htotal,
 +m-vdisplay, m-vsync_start, m-vsync_end, m-vtotal,
 +m-clock);
 +}
 +
 +int videomode_from_timing(struct display_timings *disp, struct videomode
 *vm,
 + int index)
 +{
 + struct signal_timing *st = NULL;
 +
 + if (!vm)
 + return -EINVAL;
 +

What about making vm a mandatory argument ? It looks to me like a caller bug 
if vm is NULL.

 + st = display_timings_get(disp, index);
 +

You can remove the blank line.

 + if (!st) {
 + pr_err(%s: no signal timings found\n, __func__);
 + return -EINVAL;
 + }
 +
 + vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
 + vm-hactive = signal_timing_get_value(st-hactive, 0);
 + vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
 + vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
 + vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
 +
 + vm-vactive = signal_timing_get_value(st-vactive, 0);
 + vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
 + vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
 + vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
 +
 + vm-vah = st-vsync_pol_active_high;
 + vm-hah = st-hsync_pol_active_high;
 + vm-interlaced = st-interlaced;
 + vm-doublescan = st-doublescan;
 +
 + return 0;
 +}
 +
 +int of_get_videomode(struct device_node *np, struct videomode *vm, int
 index)

I wonder how to avoid abuse of this functions. It's a useful helper for 
drivers that need to get a video mode once only, but would result in lower 
performances if a driver calls it for every mode. Drivers must call 
of_get_display_timing_list instead in that case and case the display timings. 
I'm wondering whether we should really expose of_get_videomode.

 +{
 + struct display_timings *disp;
 + int ret = 0;

No need to assign ret to 0 here.

 +
 + disp = of_get_display_timing_list(np);
 +

You can remove the blank line.

 + if (!disp) {
 + pr_err(%s: no timings specified\n, __func__);
 + return -EINVAL;
 + }
 +
 + if (index == OF_DEFAULT_TIMING)
 + index = disp-default_timing;
 +
 + ret = 

Re: [PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 09:57:41 Steffen Trumtrar wrote:
 On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
  On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:

[snip]

   diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
   new file mode 100644
   index 000..96efe01
   --- /dev/null
   +++ b/include/linux/of_videomode.h
   @@ -0,0 +1,41 @@
   +/*
   + * Copyright 2012 Steffen Trumtrar s.trumt...@pengutronix.de
   + *
   + * generic videomode description
   + *
   + * This file is released under the GPLv2
   + */
   +
   +#ifndef __LINUX_VIDEOMODE_H
   +#define __LINUX_VIDEOMODE_H
   +
   +#include drm/drmP.h
  
  You don't need to include this.
 
 That is a fix to my liking. Easily done ;-)
 
   +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;
   +
   + bool hah;
   + bool vah;
   + bool interlaced;
   + bool doublescan;
   +
   +};
  
  This is not really of related. And actually, neither is the struct
  signal_timing in the previous patch. It would be nice to have these in a
  common header that fb, drm, and others could use instead of each having
  their own timing structs.
  
  But that's probably out of scope for this series =). Did you check the
  timing structs from the video related frameworks in the kernel to see if
  your structs contain all the info the others have, so that, at least in
  theory, everybody could use these common structs?
  
   Tomi
 
 Yes. Stephen and Laurent already suggested to split it up.
 No, all info is not contained. That starts with drm, which has width-mm,..
 If time permits, I will go over that.

Just to make sure we won't forget it, the V4L2 version of the timings 
structure is struct v4l2_bt_timings in include/linux/videodev2.h.

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
 Hi Steffen,
 
 Thanks for the patch.
 
 On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
  Get videomode from devicetree in a format appropriate for the
  backend. drm_display_mode and fb_videomode are supported atm.
  Uses the display signal timings from of_display_timings
  
  Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
  ---
   drivers/of/Kconfig   |5 +
   drivers/of/Makefile  |1 +
   drivers/of/of_videomode.c|  212 +++
   include/linux/of_videomode.h |   41 
   4 files changed, 259 insertions(+)
   create mode 100644 drivers/of/of_videomode.c
   create mode 100644 include/linux/of_videomode.h
  
  diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
  index 646deb0..74282e2 100644
  --- a/drivers/of/Kconfig
  +++ b/drivers/of/Kconfig
  @@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
  help
helper to parse display timings from the devicetree
  
  +config OF_VIDEOMODE
  +   def_bool y
  +   help
  + helper to get videomodes from the devicetree
  +
   endmenu # OF
  diff --git a/drivers/of/Makefile b/drivers/of/Makefile
  index c8e9603..09d556f 100644
  --- a/drivers/of/Makefile
  +++ b/drivers/of/Makefile
  @@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
   obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
   obj-$(CONFIG_OF_MTD)   += of_mtd.o
   obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
  +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
  diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
  new file mode 100644
  index 000..76ac16e
  --- /dev/null
  +++ b/drivers/of/of_videomode.c
  @@ -0,0 +1,212 @@
  +/*
  + * generic videomode helper
  + *
  + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de,
  Pengutronix
  + *
  + * This file is released under the GPLv2
  + */
  +#include linux/of.h
  +#include linux/fb.h
  +#include linux/slab.h
  +#include drm/drm_mode.h
  +#include linux/of_display_timings.h
  +#include linux/of_videomode.h
  +
  +void dump_fb_videomode(struct fb_videomode *m)
  +{
  +pr_debug(fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n,
 
 That's going to be pretty difficult to read :-) Would it make sense to group 
 several attributes logically (for instance using %ux%u for m-xres, m-yres) ?
 

No problem. That can be changed.

  +m-refresh, m-xres, m-yres, 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);
  +}
 
 Shouldn't this (and the other non exported functions below) be static ?
 

Yes.

  +void dump_drm_displaymode(struct drm_display_mode *m)
  +{
  +pr_debug(drm_displaymode = %d %d %d %d %d %d %d %d %d\n,
  +m-hdisplay, m-hsync_start, m-hsync_end, m-htotal,
  +m-vdisplay, m-vsync_start, m-vsync_end, m-vtotal,
  +m-clock);
  +}
  +
  +int videomode_from_timing(struct display_timings *disp, struct videomode
  *vm,
  +   int index)
  +{
  +   struct signal_timing *st = NULL;
  +
  +   if (!vm)
  +   return -EINVAL;
  +
 
 What about making vm a mandatory argument ? It looks to me like a caller bug 
 if vm is NULL.
 

The caller must provide the struct videomode, yes. Wouldn't the kernel hang 
itself
with a NULL pointer exception, if I just work with it ?

  +   st = display_timings_get(disp, index);
  +
 
 You can remove the blank line.
 
  +   if (!st) {
  +   pr_err(%s: no signal timings found\n, __func__);
  +   return -EINVAL;
  +   }
  +
  +   vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
  +   vm-hactive = signal_timing_get_value(st-hactive, 0);
  +   vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
  +   vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
  +   vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
  +
  +   vm-vactive = signal_timing_get_value(st-vactive, 0);
  +   vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
  +   vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
  +   vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
  +
  +   vm-vah = st-vsync_pol_active_high;
  +   vm-hah = st-hsync_pol_active_high;
  +   vm-interlaced = st-interlaced;
  +   vm-doublescan = st-doublescan;
  +
  +   return 0;
  +}
  +
  +int of_get_videomode(struct device_node *np, struct videomode *vm, int
  index)
 
 I wonder how to avoid abuse of this functions. It's a useful helper for 
 drivers that need to get a video mode once only, but would result in lower 
 performances if a driver calls it for every mode. Drivers must call 
 of_get_display_timing_list instead in that case and case the display timings. 
 I'm wondering whether we should really expose of_get_videomode.
 

The intent was to let the driver decide. That way all the other overhead may

Re: [PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
 On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
  On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
   Get videomode from devicetree in a format appropriate for the
   backend. drm_display_mode and fb_videomode are supported atm.
   Uses the display signal timings from of_display_timings
   
   Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
   ---
   
drivers/of/Kconfig   |5 +
drivers/of/Makefile  |1 +
drivers/of/of_videomode.c|  212 +++
include/linux/of_videomode.h |   41 
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h

[snip]

   diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
   new file mode 100644
   index 000..76ac16e
   --- /dev/null
   +++ b/drivers/of/of_videomode.c

[snip]

   +int videomode_from_timing(struct display_timings *disp, struct
   videomode *vm,
   + int index)
   +{
   + struct signal_timing *st = NULL;
   +
   + if (!vm)
   + return -EINVAL;
   +
  
  What about making vm a mandatory argument ? It looks to me like a caller
  bug if vm is NULL.
 
 The caller must provide the struct videomode, yes. Wouldn't the kernel hang
 itself with a NULL pointer exception, if I just work with it ?

The kernel would oops, clearly showing the caller that a non-null vm is needed 
:-)

   + st = display_timings_get(disp, index);
   +
  
  You can remove the blank line.
  
   + if (!st) {
   + pr_err(%s: no signal timings found\n, __func__);
   + return -EINVAL;
   + }
   +
   + vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
   + vm-hactive = signal_timing_get_value(st-hactive, 0);
   + vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
   + vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
   + vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
   +
   + vm-vactive = signal_timing_get_value(st-vactive, 0);
   + vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
   + vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
   + vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
   +
   + vm-vah = st-vsync_pol_active_high;
   + vm-hah = st-hsync_pol_active_high;
   + vm-interlaced = st-interlaced;
   + vm-doublescan = st-doublescan;
   +
   + return 0;
   +}
   +
   +int of_get_videomode(struct device_node *np, struct videomode *vm, int
   index)
  
  I wonder how to avoid abuse of this functions. It's a useful helper for
  drivers that need to get a video mode once only, but would result in lower
  performances if a driver calls it for every mode. Drivers must call
  of_get_display_timing_list instead in that case and case the display
  timings. I'm wondering whether we should really expose of_get_videomode.
 
 The intent was to let the driver decide. That way all the other overhead may
 be skipped.

My point is that driver writers might just call of_get_videomode() in a loop, 
not knowing that it's expensive. I want to avoid that. We need to at least add 
kerneldoc to the function stating that this shouldn't be done.

   +{
   + struct display_timings *disp;
   + int ret = 0;
  
  No need to assign ret to 0 here.
 
 Ah, yes. Unneeded in this case.
 
   +
   + disp = of_get_display_timing_list(np);
   +
  
  You can remove the blank line.
  
   + if (!disp) {
   + pr_err(%s: no timings specified\n, __func__);
   + return -EINVAL;
   + }
   +
   + if (index == OF_DEFAULT_TIMING)
   + index = disp-default_timing;
   +
   + ret = videomode_from_timing(disp, vm, index);
   +
  
  No need for a blank line.
  
   + if (ret)
   + return ret;
   +
   + display_timings_release(disp);
   +
   + if (!vm) {
   + pr_err(%s: could not get videomode %d\n, __func__, index);
   + return -EINVAL;
   + }
  
  This can't happen. If vm is NULL the videomode_from_timing call above will
  return -EINVAL, and this function will then return immediately without
  reaching this code block.
 
 Okay.
 
   +
   + return 0;
   +}
   +EXPORT_SYMBOL_GPL(of_get_videomode);

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
> On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> > On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > Get videomode from devicetree in a format appropriate for the
> > > backend. drm_display_mode and fb_videomode are supported atm.
> > > Uses the display signal timings from of_display_timings
> > > 
> > > +++ b/drivers/of/of_videomode.c
> > > 
> > > +int videomode_from_timing(struct display_timings *disp, struct
> > > videomode *vm,
> > > 
> > > + st = display_timings_get(disp, index);
> > > +
> > > + if (!st) {
> > 
> > It's a little odd to leave a blank line between those two lines.
> 
> Hm, well okay. That can be remedied
> 
> > Only half of the code in this file seems OF-related; the routines to
> > convert a timing to a videomode or drm display mode seem like they'd be
> > useful outside device tree, so I wonder if putting them into
> > of_videomode.c is the correct thing to do. Still, it's probably not a
> > big deal.
> 
> I am not sure, what the appropriate way to do this is. I can split it up
> (again).

I think it would make sense to move them to their respective subsystems.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2 v6] of: add generic videomode description

[Laurent Pinchart]

Hi Steffen,

On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
 On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
  On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
   Get videomode from devicetree in a format appropriate for the
   backend. drm_display_mode and fb_videomode are supported atm.
   Uses the display signal timings from of_display_timings
   
   +++ b/drivers/of/of_videomode.c
   
   +int videomode_from_timing(struct display_timings *disp, struct
   videomode *vm,
   
   + st = display_timings_get(disp, index);
   +
   + if (!st) {
  
  It's a little odd to leave a blank line between those two lines.
 
 Hm, well okay. That can be remedied
 
  Only half of the code in this file seems OF-related; the routines to
  convert a timing to a videomode or drm display mode seem like they'd be
  useful outside device tree, so I wonder if putting them into
  of_videomode.c is the correct thing to do. Still, it's probably not a
  big deal.
 
 I am not sure, what the appropriate way to do this is. I can split it up
 (again).

I think it would make sense to move them to their respective subsystems.

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > Get videomode from devicetree in a format appropriate for the
> > backend. drm_display_mode and fb_videomode are supported atm.
> > Uses the display signal timings from of_display_timings
> 
> > +++ b/drivers/of/of_videomode.c
> 
> > +int videomode_from_timing(struct display_timings *disp, struct videomode 
> > *vm,
> 
> > +   st = display_timings_get(disp, index);
> > +
> > +   if (!st) {
> 
> It's a little odd to leave a blank line between those two lines.

Hm, well okay. That can be remedied

> 
> Only half of the code in this file seems OF-related; the routines to
> convert a timing to a videomode or drm display mode seem like they'd be
> useful outside device tree, so I wonder if putting them into
> of_videomode.c is the correct thing to do. Still, it's probably not a
> big deal.
> 

I am not sure, what the appropriate way to do this is. I can split it up 
(again).


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

Re: [PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
 On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
  Get videomode from devicetree in a format appropriate for the
  backend. drm_display_mode and fb_videomode are supported atm.
  Uses the display signal timings from of_display_timings
 
  +++ b/drivers/of/of_videomode.c
 
  +int videomode_from_timing(struct display_timings *disp, struct videomode 
  *vm,
 
  +   st = display_timings_get(disp, index);
  +
  +   if (!st) {
 
 It's a little odd to leave a blank line between those two lines.

Hm, well okay. That can be remedied

 
 Only half of the code in this file seems OF-related; the routines to
 convert a timing to a videomode or drm display mode seem like they'd be
 useful outside device tree, so I wonder if putting them into
 of_videomode.c is the correct thing to do. Still, it's probably not a
 big deal.
 

I am not sure, what the appropriate way to do this is. I can split it up 
(again).


-- 
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- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

Signed-off-by: Steffen Trumtrar 
---
 drivers/of/Kconfig   |5 +
 drivers/of/Makefile  |1 +
 drivers/of/of_videomode.c|  212 ++
 include/linux/of_videomode.h |   41 
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 646deb0..74282e2 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
help
  helper to parse display timings from the devicetree

+config OF_VIDEOMODE
+   def_bool y
+   help
+ helper to get videomodes from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c8e9603..09d556f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)   += of_mtd.o
 obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 000..76ac16e
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,212 @@
+/*
+ * generic videomode helper
+ *
+ * Copyright (c) 2012 Steffen Trumtrar , 
Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+void dump_fb_videomode(struct fb_videomode *m)
+{
+pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
+m->refresh, m->xres, m->yres, 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);
+}
+
+void dump_drm_displaymode(struct drm_display_mode *m)
+{
+pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n",
+m->hdisplay, m->hsync_start, m->hsync_end, m->htotal,
+m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal,
+m->clock);
+}
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+   int index)
+{
+   struct signal_timing *st = NULL;
+
+   if (!vm)
+   return -EINVAL;
+
+   st = display_timings_get(disp, index);
+
+   if (!st) {
+   pr_err("%s: no signal timings found\n", __func__);
+   return -EINVAL;
+   }
+
+   vm->pixelclock = signal_timing_get_value(>pixelclock, 0);
+   vm->hactive = signal_timing_get_value(>hactive, 0);
+   vm->hfront_porch = signal_timing_get_value(>hfront_porch, 0);
+   vm->hback_porch = signal_timing_get_value(>hback_porch, 0);
+   vm->hsync_len = signal_timing_get_value(>hsync_len, 0);
+
+   vm->vactive = signal_timing_get_value(>vactive, 0);
+   vm->vfront_porch = signal_timing_get_value(>vfront_porch, 0);
+   vm->vback_porch = signal_timing_get_value(>vback_porch, 0);
+   vm->vsync_len = signal_timing_get_value(>vsync_len, 0);
+
+   vm->vah = st->vsync_pol_active_high;
+   vm->hah = st->hsync_pol_active_high;
+   vm->interlaced = st->interlaced;
+   vm->doublescan = st->doublescan;
+
+   return 0;
+}
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+   struct display_timings *disp;
+   int ret = 0;
+
+   disp = of_get_display_timing_list(np);
+
+   if (!disp) {
+   pr_err("%s: no timings specified\n", __func__);
+   return -EINVAL;
+   }
+
+   if (index == OF_DEFAULT_TIMING)
+   index = disp->default_timing;
+
+   ret = videomode_from_timing(disp, vm, index);
+
+   if (ret)
+   return ret;
+
+   display_timings_release(disp);
+
+   if (!vm) {
+   pr_err("%s: could not get videomode %d\n", __func__, index);
+   return -EINVAL;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
+
+#if defined(CONFIG_DRM)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
*dmode)
+{
+   memset(dmode, 0, sizeof(*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;
+
+   if (vm->hah)
+   dmode->flags |= 

[PATCH 2/2 v6] of: add generic videomode description

[Stephen Warren]

On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings

> +++ b/drivers/of/of_videomode.c

> +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,

> + st = display_timings_get(disp, index);
> +
> + if (!st) {

It's a little odd to leave a blank line between those two lines.

Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.

[PATCH 2/2 v6] of: add generic videomode description

[Steffen Trumtrar]

Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings

Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
---
 drivers/of/Kconfig   |5 +
 drivers/of/Makefile  |1 +
 drivers/of/of_videomode.c|  212 ++
 include/linux/of_videomode.h |   41 
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/of/of_videomode.c
 create mode 100644 include/linux/of_videomode.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 646deb0..74282e2 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -88,4 +88,9 @@ config OF_DISPLAY_TIMINGS
help
  helper to parse display timings from the devicetree
 
+config OF_VIDEOMODE
+   def_bool y
+   help
+ helper to get videomodes from the devicetree
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index c8e9603..09d556f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)   += of_mtd.o
 obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 000..76ac16e
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,212 @@
+/*
+ * generic videomode helper
+ *
+ * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include linux/of.h
+#include linux/fb.h
+#include linux/slab.h
+#include drm/drm_mode.h
+#include linux/of_display_timings.h
+#include linux/of_videomode.h
+
+void dump_fb_videomode(struct fb_videomode *m)
+{
+pr_debug(fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n,
+m-refresh, m-xres, m-yres, 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);
+}
+
+void dump_drm_displaymode(struct drm_display_mode *m)
+{
+pr_debug(drm_displaymode = %d %d %d %d %d %d %d %d %d\n,
+m-hdisplay, m-hsync_start, m-hsync_end, m-htotal,
+m-vdisplay, m-vsync_start, m-vsync_end, m-vtotal,
+m-clock);
+}
+
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
+   int index)
+{
+   struct signal_timing *st = NULL;
+
+   if (!vm)
+   return -EINVAL;
+
+   st = display_timings_get(disp, index);
+
+   if (!st) {
+   pr_err(%s: no signal timings found\n, __func__);
+   return -EINVAL;
+   }
+
+   vm-pixelclock = signal_timing_get_value(st-pixelclock, 0);
+   vm-hactive = signal_timing_get_value(st-hactive, 0);
+   vm-hfront_porch = signal_timing_get_value(st-hfront_porch, 0);
+   vm-hback_porch = signal_timing_get_value(st-hback_porch, 0);
+   vm-hsync_len = signal_timing_get_value(st-hsync_len, 0);
+
+   vm-vactive = signal_timing_get_value(st-vactive, 0);
+   vm-vfront_porch = signal_timing_get_value(st-vfront_porch, 0);
+   vm-vback_porch = signal_timing_get_value(st-vback_porch, 0);
+   vm-vsync_len = signal_timing_get_value(st-vsync_len, 0);
+
+   vm-vah = st-vsync_pol_active_high;
+   vm-hah = st-hsync_pol_active_high;
+   vm-interlaced = st-interlaced;
+   vm-doublescan = st-doublescan;
+
+   return 0;
+}
+
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+   struct display_timings *disp;
+   int ret = 0;
+
+   disp = of_get_display_timing_list(np);
+
+   if (!disp) {
+   pr_err(%s: no timings specified\n, __func__);
+   return -EINVAL;
+   }
+
+   if (index == OF_DEFAULT_TIMING)
+   index = disp-default_timing;
+
+   ret = videomode_from_timing(disp, vm, index);
+
+   if (ret)
+   return ret;
+
+   display_timings_release(disp);
+
+   if (!vm) {
+   pr_err(%s: could not get videomode %d\n, __func__, index);
+   return -EINVAL;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_videomode);
+
+#if defined(CONFIG_DRM)
+int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode 
*dmode)
+{
+   memset(dmode, 0, sizeof(*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;
+
+   

Re: [PATCH 2/2 v6] of: add generic videomode description

[Stephen Warren]

On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
 Get videomode from devicetree in a format appropriate for the
 backend. drm_display_mode and fb_videomode are supported atm.
 Uses the display signal timings from of_display_timings

 +++ b/drivers/of/of_videomode.c

 +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,

 + st = display_timings_get(disp, index);
 +
 + if (!st) {

It's a little odd to leave a blank line between those two lines.

Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel