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

2012-11-14 Thread Thierry Reding 
On Wed, Nov 14, 2012 at 12:10:15PM +0100, Steffen Trumtrar wrote:
> On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote:
> > On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
> > > On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
> > > > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> > > > [...]
> > > > > diff --git a/drivers/video/display_timing.c 
> > > > > b/drivers/video/display_timing.c
> > > > [...]
> > > > > +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);
> > > > > +}
> > > > 
> > > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
> > > > be used from modules.
> > > > 
> > > > Thierry
> > > 
> > > Yes. Just in time. I was just starting to type the send-email command ;-)
> > 
> > Great! In that case don't forget to also look at my other email before
> > sending. =)
> > 
> Sure.

Besides those comments (and those from other people) I think your
patchset is in pretty good shape. Have you thought about how and when
you want to get things merged?

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

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

2012-11-14 Thread Steffen Trumtrar 
On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote:
> On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
> > On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
> > > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> > > [...]
> > > > diff --git a/drivers/video/display_timing.c 
> > > > b/drivers/video/display_timing.c
> > > [...]
> > > > +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);
> > > > +}
> > > 
> > > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
> > > be used from modules.
> > > 
> > > Thierry
> > 
> > Yes. Just in time. I was just starting to type the send-email command ;-)
> 
> Great! In that case don't forget to also look at my other email before
> sending. =)
> 
Sure.


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

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

2012-11-14 Thread Thierry Reding 
On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
> On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
> > On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> > [...]
> > > diff --git a/drivers/video/display_timing.c 
> > > b/drivers/video/display_timing.c
> > [...]
> > > +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);
> > > +}
> > 
> > I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
> > be used from modules.
> > 
> > Thierry
> 
> Yes. Just in time. I was just starting to type the send-email command ;-)

Great! In that case don't forget to also look at my other email before
sending. =)

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

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

2012-11-14 Thread Steffen Trumtrar 
On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
> On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> [...]
> > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
> [...]
> > +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);
> > +}
> 
> I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
> be used from modules.
> 
> Thierry

Yes. Just in time. I was just starting to type the send-email command ;-)

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

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

2012-11-14 Thread Thierry Reding 
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
> diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
[...]
> +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);
> +}

I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
be used from modules.

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 v8 1/6] video: add display_timing and videomode

2012-11-14 Thread Thierry Reding 
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
 diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
[...]
 +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);
 +}

I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
be used from modules.

Thierry


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

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

2012-11-14 Thread Steffen Trumtrar 
On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
 On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
 [...]
  diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
 [...]
  +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);
  +}
 
 I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
 be used from modules.
 
 Thierry

Yes. Just in time. I was just starting to type the send-email command ;-)

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

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

2012-11-14 Thread Thierry Reding 
On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
 On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
  On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
  [...]
   diff --git a/drivers/video/display_timing.c 
   b/drivers/video/display_timing.c
  [...]
   +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);
   +}
  
  I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
  be used from modules.
  
  Thierry
 
 Yes. Just in time. I was just starting to type the send-email command ;-)

Great! In that case don't forget to also look at my other email before
sending. =)

Thierry


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

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

2012-11-14 Thread Steffen Trumtrar 
On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote:
 On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
  On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
   On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
   [...]
diff --git a/drivers/video/display_timing.c 
b/drivers/video/display_timing.c
   [...]
+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);
+}
   
   I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
   be used from modules.
   
   Thierry
  
  Yes. Just in time. I was just starting to type the send-email command ;-)
 
 Great! In that case don't forget to also look at my other email before
 sending. =)
 
Sure.


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

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

2012-11-14 Thread Thierry Reding 
On Wed, Nov 14, 2012 at 12:10:15PM +0100, Steffen Trumtrar wrote:
 On Wed, Nov 14, 2012 at 12:02:15PM +0100, Thierry Reding wrote:
  On Wed, Nov 14, 2012 at 11:59:25AM +0100, Steffen Trumtrar wrote:
   On Wed, Nov 14, 2012 at 11:56:34AM +0100, Thierry Reding wrote:
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
 diff --git a/drivers/video/display_timing.c 
 b/drivers/video/display_timing.c
[...]
 +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);
 +}

I think this is still missing an EXPORT_SYMBOL_GPL. Otherwise it can't
be used from modules.

Thierry
   
   Yes. Just in time. I was just starting to type the send-email command ;-)
  
  Great! In that case don't forget to also look at my other email before
  sending. =)
  
 Sure.

Besides those comments (and those from other people) I think your
patchset is in pretty good shape. Have you thought about how and when
you want to get things merged?

Thierry


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

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

2012-11-13 Thread Steffen Trumtrar 
On Tue, Nov 13, 2012 at 11:41:59AM +0100, Thierry Reding wrote:
> On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> [...]
> > 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
> 
> DISPLAY_TIMINGS?
> 
> >  #video output switch sysfs driver
> >  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
> > +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
> 
> display_timings.o?
> 
> > +obj-$(CONFIG_VIDEOMODE) += videomode.o
> > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
> 
> display_timings.c?
> 

I originally had that and changed it by request to the singular form.
(Can't find the mail atm). And I think this fits better with all the other 
drivers.

> > +int videomode_from_timing(struct display_timings *disp, struct videomode 
> > *vm,
> > + unsigned int index)
> 
> I find the indexing API a bit confusing. But that's perhaps just a
> matter of personal preference.
> 
> Also the ordering of arguments seems a little off. I find it more
> natural to have the destination pointer in the first argument, similar
> to the memcpy() function, so this would be:
> 
> int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
> unsigned int index);
> 
> Actually, when reading videomode_from_timing() I'd expect the argument
> list to be:
> 
> int videomode_from_timing(struct videomode *vm, struct display_timing 
> *timing);
> 
> Am I the only one confused by this?
> 

I went with the of_xxx-functions that have fname(from_node, to_property)
and personally prefer it this way. Therefore I'd like to keep it as is.

> > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> 
> display_timings.h?
> 
> > +/* placeholder function until ranges are really needed 
> 
> The above line has trailing whitespace. Also the block comment should
> have the opening /* on a separate line.
> 

Okay.

> > + * the index parameter should then be used to select one of [min typ max]
> 
> If index is supposed to select min, typ or max, then maybe an enum would
> be a better candidate? Or alternatively provide separate accessors, like
> display_timing_get_{minimum,typical,maximum}().
> 

Hm, I'm not so sure about this one. I'd prefer the enum.

> > + */
> > +static inline u32 display_timing_get_value(struct timing_entry *te,
> > +  unsigned int index)
> > +{
> > +   return te->typ;
> > +}
> > +
> > +static inline struct display_timing *display_timings_get(struct 
> > display_timings *disp,
> > +unsigned int index)
> > +{
> > +   if (disp->num_timings > index)
> > +   return disp->timings[index];
> > +   else
> > +   return NULL;
> > +}
> > +
> > +void timings_release(struct display_timings *disp);
> 
> This function no longer exists.
> 
Right.

Steffen

> ___
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss


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

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

2012-11-13 Thread Thierry Reding 
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
> 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

DISPLAY_TIMINGS?

>  #video output switch sysfs driver
>  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
> +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o

display_timings.o?

> +obj-$(CONFIG_VIDEOMODE) += videomode.o
> diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c

display_timings.c?

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

I find the indexing API a bit confusing. But that's perhaps just a
matter of personal preference.

Also the ordering of arguments seems a little off. I find it more
natural to have the destination pointer in the first argument, similar
to the memcpy() function, so this would be:

int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
  unsigned int index);

Actually, when reading videomode_from_timing() I'd expect the argument
list to be:

int videomode_from_timing(struct videomode *vm, struct display_timing *timing);

Am I the only one confused by this?

> diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h

display_timings.h?

> +/* placeholder function until ranges are really needed 

The above line has trailing whitespace. Also the block comment should
have the opening /* on a separate line.

> + * the index parameter should then be used to select one of [min typ max]

If index is supposed to select min, typ or max, then maybe an enum would
be a better candidate? Or alternatively provide separate accessors, like
display_timing_get_{minimum,typical,maximum}().

> + */
> +static inline u32 display_timing_get_value(struct timing_entry *te,
> +unsigned int index)
> +{
> + return te->typ;
> +}
> +
> +static inline struct display_timing *display_timings_get(struct 
> display_timings *disp,
> +  unsigned int index)
> +{
> + if (disp->num_timings > index)
> + return disp->timings[index];
> + else
> + return NULL;
> +}
> +
> +void timings_release(struct display_timings *disp);

This function no longer exists.

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 v8 1/6] video: add display_timing and videomode

2012-11-13 Thread Thierry Reding 
On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
[...]
 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

DISPLAY_TIMINGS?

  #video output switch sysfs driver
  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
 +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o

display_timings.o?

 +obj-$(CONFIG_VIDEOMODE) += videomode.o
 diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c

display_timings.c?

 +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
 +   unsigned int index)

I find the indexing API a bit confusing. But that's perhaps just a
matter of personal preference.

Also the ordering of arguments seems a little off. I find it more
natural to have the destination pointer in the first argument, similar
to the memcpy() function, so this would be:

int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
  unsigned int index);

Actually, when reading videomode_from_timing() I'd expect the argument
list to be:

int videomode_from_timing(struct videomode *vm, struct display_timing *timing);

Am I the only one confused by this?

 diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h

display_timings.h?

 +/* placeholder function until ranges are really needed 

The above line has trailing whitespace. Also the block comment should
have the opening /* on a separate line.

 + * the index parameter should then be used to select one of [min typ max]

If index is supposed to select min, typ or max, then maybe an enum would
be a better candidate? Or alternatively provide separate accessors, like
display_timing_get_{minimum,typical,maximum}().

 + */
 +static inline u32 display_timing_get_value(struct timing_entry *te,
 +unsigned int index)
 +{
 + return te-typ;
 +}
 +
 +static inline struct display_timing *display_timings_get(struct 
 display_timings *disp,
 +  unsigned int index)
 +{
 + if (disp-num_timings  index)
 + return disp-timings[index];
 + else
 + return NULL;
 +}
 +
 +void timings_release(struct display_timings *disp);

This function no longer exists.

Thierry


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

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

2012-11-13 Thread Steffen Trumtrar 
On Tue, Nov 13, 2012 at 11:41:59AM +0100, Thierry Reding wrote:
 On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
 [...]
  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
 
 DISPLAY_TIMINGS?
 
   #video output switch sysfs driver
   obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
  +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
 
 display_timings.o?
 
  +obj-$(CONFIG_VIDEOMODE) += videomode.o
  diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
 
 display_timings.c?
 

I originally had that and changed it by request to the singular form.
(Can't find the mail atm). And I think this fits better with all the other 
drivers.

  +int videomode_from_timing(struct display_timings *disp, struct videomode 
  *vm,
  + unsigned int index)
 
 I find the indexing API a bit confusing. But that's perhaps just a
 matter of personal preference.
 
 Also the ordering of arguments seems a little off. I find it more
 natural to have the destination pointer in the first argument, similar
 to the memcpy() function, so this would be:
 
 int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
 unsigned int index);
 
 Actually, when reading videomode_from_timing() I'd expect the argument
 list to be:
 
 int videomode_from_timing(struct videomode *vm, struct display_timing 
 *timing);
 
 Am I the only one confused by this?
 

I went with the of_xxx-functions that have fname(from_node, to_property)
and personally prefer it this way. Therefore I'd like to keep it as is.

  diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
 
 display_timings.h?
 
  +/* placeholder function until ranges are really needed 
 
 The above line has trailing whitespace. Also the block comment should
 have the opening /* on a separate line.
 

Okay.

  + * the index parameter should then be used to select one of [min typ max]
 
 If index is supposed to select min, typ or max, then maybe an enum would
 be a better candidate? Or alternatively provide separate accessors, like
 display_timing_get_{minimum,typical,maximum}().
 

Hm, I'm not so sure about this one. I'd prefer the enum.

  + */
  +static inline u32 display_timing_get_value(struct timing_entry *te,
  +  unsigned int index)
  +{
  +   return te-typ;
  +}
  +
  +static inline struct display_timing *display_timings_get(struct 
  display_timings *disp,
  +unsigned int index)
  +{
  +   if (disp-num_timings  index)
  +   return disp-timings[index];
  +   else
  +   return NULL;
  +}
  +
  +void timings_release(struct display_timings *disp);
 
 This function no longer exists.
 
Right.

Steffen

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


-- 
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 v8 1/6] video: add display_timing and videomode

2012-11-12 Thread 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
.

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 
---
 drivers/video/Kconfig  |6 
 drivers/video/Makefile |2 ++
 drivers/video/display_timing.c |   22 +
 drivers/video/videomode.c  |   45 ++
 include/linux/display_timing.h |   69 
 include/linux/videomode.h  |   39 +++
 6 files changed, 183 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 000..04b7b69
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,22 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar , 
Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include 
+#include 
+
+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);
+}
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 000..087374a
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,45 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar , 
Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+int videomode_from_timing(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(>pixelclock, 0);
+   vm->hactive = display_timing_get_value(>hactive, 0);
+   vm->hfront_porch = display_timing_get_value(>hfront_porch, 0);
+   vm->hback_porch = display_timing_get_value(>hback_porch, 0);
+   vm->hsync_len = display_timing_get_value(>hsync_len, 0);
+
+   vm->vactive = display_timing_get_value(>vactive, 0);
+   vm->vfront_porch = display_timing_get_value(>vfront_porch, 0);
+   vm->vback_porch = display_timing_get_value(>vback_porch, 0);
+   vm->vsync_len = display_timing_get_value(>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 000..0ed2a1e
--- /dev/null
+++ b/include/linux/display_timing.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2012 Steffen Trumtrar 
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_TIMINGS_H
+#define __LINUX_DISPLAY_TIMINGS_H
+
+#include 
+
+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

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

2012-11-12 Thread 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.trumt...@pengutronix.de
---
 drivers/video/Kconfig  |6 
 drivers/video/Makefile |2 ++
 drivers/video/display_timing.c |   22 +
 drivers/video/videomode.c  |   45 ++
 include/linux/display_timing.h |   69 
 include/linux/videomode.h  |   39 +++
 6 files changed, 183 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 000..04b7b69
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,22 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include linux/display_timing.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);
+}
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 000..087374a
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,45 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar s.trumt...@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(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 000..0ed2a1e
--- /dev/null
+++ b/include/linux/display_timing.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2012 Steffen Trumtrar s.trumt...@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