Re: [PATCH v7 1/8] video: add display_timing struct and helpers

2012-11-04 Thread Steffen Trumtrar 
On Thu, Nov 01, 2012 at 09:08:42PM +0100, Thierry Reding wrote:
 On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
 [...]
  +void timings_release(struct display_timings *disp)
  +{
  +   int i;
  +
  +   for (i = 0; i  disp-num_timings; i++)
  +   kfree(disp-timings[i]);
  +}
  +
  +void display_timings_release(struct display_timings *disp)
  +{
  +   timings_release(disp);
  +   kfree(disp-timings);
  +}
 
 I'm not quite sure I understand how these are supposed to be used. The
 only use-case where a struct display_timings is dynamically allocated is
 for the OF helpers. In that case, wouldn't it be more useful to have a
 function that frees the complete structure, including the struct
 display_timings itself? Something like this, which has all of the above
 rolled into one:
 
   void display_timings_free(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);
   }
 

Well, you are right. They can be rolled into one function.
The extra function call is useless and as it seems confusing.

Regards,
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- |
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

2012-11-01 Thread Thierry Reding 
On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
[...]
 +void timings_release(struct display_timings *disp)
 +{
 + int i;
 +
 + for (i = 0; i  disp-num_timings; i++)
 + kfree(disp-timings[i]);
 +}
 +
 +void display_timings_release(struct display_timings *disp)
 +{
 + timings_release(disp);
 + kfree(disp-timings);
 +}

I'm not quite sure I understand how these are supposed to be used. The
only use-case where a struct display_timings is dynamically allocated is
for the OF helpers. In that case, wouldn't it be more useful to have a
function that frees the complete structure, including the struct
display_timings itself? Something like this, which has all of the above
rolled into one:

void display_timings_free(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);
}

Is there a use-case where a struct display_timings is not dynamically
allocated? The only one I can think of is where it is defined as
platform data, but in that case you don't want to be calling
display_timing_release() on it anyway.

Thierry


pgpGO6gYC6vAE.pgp
Description: PGP signature

[PATCH v7 1/8] video: add display_timing struct and helpers

2012-10-31 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.

Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
---
 drivers/video/Kconfig  |5 +++
 drivers/video/Makefile |1 +
 drivers/video/display_timing.c |   24 ++
 include/linux/display_timing.h |   69 
 4 files changed, 99 insertions(+)
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 include/linux/display_timing.h

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index d08d799..1421fc8 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
  This framework adds support for low-level control of the video 
  output switch.
 
+config DISPLAY_TIMING
+   bool Enable display timings helpers
+   help
+ Say Y here, to use the display timing helpers.
+
 menuconfig FB
tristate Support for frame buffer devices
---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 23e948e..552c045 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -167,3 +167,4 @@ 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
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
new file mode 100644
index 000..9ccfdb3
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,24 @@
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include linux/slab.h
+#include linux/display_timing.h
+
+void timings_release(struct display_timings *disp)
+{
+   int i;
+
+   for (i = 0; i  disp-num_timings; i++)
+   kfree(disp-timings[i]);
+}
+
+void display_timings_release(struct display_timings *disp)
+{
+   timings_release(disp);
+   kfree(disp-timings);
+}
diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
new file mode 100644
index 000..aa02a12
--- /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 hback_porch;
+   struct timing_entry hsync_len;
+
+   struct timing_entry vactive;
+   struct timing_entry vfront_porch;
+   struct timing_entry vback_porch;
+   struct timing_entry vsync_len;
+
+   unsigned int vsync_pol_active;
+   unsigned int hsync_pol_active;
+   unsigned int de_pol_active;
+   unsigned int pixelclk_pol;
+   bool interlaced;
+   bool doublescan;
+};
+
+struct display_timings {
+   unsigned int num_timings;
+   unsigned int native_mode;
+
+   struct display_timing **timings;
+};
+
+/* placeholder function until ranges are really needed */
+static inline u32 display_timing_get_value(struct timing_entry *te, int index)
+{
+   return te-typ;
+}
+
+static inline struct display_timing *display_timings_get(struct 
display_timings *disp,
+int index)
+{
+   struct display_timing *dt;
+
+   if (disp-num_timings  index) {
+   dt = disp-timings[index];
+   return dt;
+   } else
+   return NULL;
+}
+
+void timings_release(struct display_timings *disp);
+void display_timings_release(struct display_timings *disp);
+
+#endif
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

2012-10-31 Thread Laurent Pinchart 
Hi Steffen,

Thanks for the patch.

As we'll need a v8 anyway due to the comment on patch 5/8, here are a couple 
of other small comments.

On Wednesday 31 October 2012 10:28:01 Steffen Trumtrar wrote:
 Add display_timing structure and the according helper functions. This allows
 the description of a display via its supported timing parameters.
 
 Every timing parameter can be specified as a single value or a range
 min typ max.
 
 Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
 ---
  drivers/video/Kconfig  |5 +++
  drivers/video/Makefile |1 +
  drivers/video/display_timing.c |   24 ++
  include/linux/display_timing.h |   69 +
  4 files changed, 99 insertions(+)
  create mode 100644 drivers/video/display_timing.c
  create mode 100644 include/linux/display_timing.h
 
 diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
 index d08d799..1421fc8 100644
 --- a/drivers/video/Kconfig
 +++ b/drivers/video/Kconfig
 @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
 This framework adds support for low-level control of the video
 output switch.
 
 +config DISPLAY_TIMING
 +   bool Enable display timings helpers
 +   help
 + Say Y here, to use the display timing helpers.
 +
  menuconfig FB
   tristate Support for frame buffer devices
   ---help---
 diff --git a/drivers/video/Makefile b/drivers/video/Makefile
 index 23e948e..552c045 100644
 --- a/drivers/video/Makefile
 +++ b/drivers/video/Makefile
 @@ -167,3 +167,4 @@ 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
 diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
 new file mode 100644
 index 000..9ccfdb3
 --- /dev/null
 +++ b/drivers/video/display_timing.c
 @@ -0,0 +1,24 @@
 +/*
 + * generic display timing functions
 + *
 + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de,
 Pengutronix + *
 + * This file is released under the GPLv2
 + */
 +
 +#include linux/slab.h
 +#include linux/display_timing.h

I try to keep #include's sorted alphabetically, but I won't push for that.

 +void timings_release(struct display_timings *disp)
 +{
 + int i;
 +
 + for (i = 0; i  disp-num_timings; i++)

disp-num_timings is an unsigned int, i should be an unsigned int as well to 
avoid signed vs. unsigned comparisons.

 + kfree(disp-timings[i]);
 +}
 +
 +void display_timings_release(struct display_timings *disp)
 +{
 + timings_release(disp);
 + kfree(disp-timings);
 +}
 diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
 new file mode 100644
 index 000..aa02a12
 --- /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 hback_porch;
 + struct timing_entry hsync_len;
 +
 + struct timing_entry vactive;
 + struct timing_entry vfront_porch;
 + struct timing_entry vback_porch;
 + struct timing_entry vsync_len;
 +
 + unsigned int vsync_pol_active;
 + unsigned int hsync_pol_active;
 + unsigned int de_pol_active;
 + unsigned int pixelclk_pol;
 + bool interlaced;
 + bool doublescan;
 +};
 +
 +struct display_timings {
 + unsigned int num_timings;
 + unsigned int native_mode;
 +
 + struct display_timing **timings;
 +};
 +
 +/* placeholder function until ranges are really needed */
 +static inline u32 display_timing_get_value(struct timing_entry *te, int
 index)

What is the index parameter for ?

 +{
 + return te-typ;
 +}
 +
 +static inline struct display_timing *display_timings_get(struct
 display_timings *disp,
 +  int index)
 +{
 + struct display_timing *dt;
 +
 + if (disp-num_timings  index) {

index should be an unsigned int for the same reason as above.

 + dt = disp-timings[index];
 + return dt;

Maybe just

return disp-timings[index];

?

 + } else
 + return NULL;
 +}
 +
 +void timings_release(struct display_timings *disp);
 +void display_timings_release(struct display_timings *disp);
 +
 +#endif
-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/8] video: add display_timing struct and helpers

2012-10-31 Thread Laurent Pinchart 
Hi Steffen,

One more comment.

On Wednesday 31 October 2012 10:28:01 Steffen Trumtrar wrote:
 Add display_timing structure and the according helper functions. This allows
 the description of a display via its supported timing parameters.
 
 Every timing parameter can be specified as a single value or a range
 min typ max.
 
 Signed-off-by: Steffen Trumtrar s.trumt...@pengutronix.de
 ---
  drivers/video/Kconfig  |5 +++
  drivers/video/Makefile |1 +
  drivers/video/display_timing.c |   24 ++
  include/linux/display_timing.h |   69 +
  4 files changed, 99 insertions(+)
  create mode 100644 drivers/video/display_timing.c
  create mode 100644 include/linux/display_timing.h
 
 diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
 index d08d799..1421fc8 100644
 --- a/drivers/video/Kconfig
 +++ b/drivers/video/Kconfig
 @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL
 This framework adds support for low-level control of the video
 output switch.
 
 +config DISPLAY_TIMING
 +   bool Enable display timings helpers
 +   help
 + Say Y here, to use the display timing helpers.
 +
  menuconfig FB
   tristate Support for frame buffer devices
   ---help---
 diff --git a/drivers/video/Makefile b/drivers/video/Makefile
 index 23e948e..552c045 100644
 --- a/drivers/video/Makefile
 +++ b/drivers/video/Makefile
 @@ -167,3 +167,4 @@ 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
 diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
 new file mode 100644
 index 000..9ccfdb3
 --- /dev/null
 +++ b/drivers/video/display_timing.c
 @@ -0,0 +1,24 @@
 +/*
 + * generic display timing functions
 + *
 + * Copyright (c) 2012 Steffen Trumtrar s.trumt...@pengutronix.de,
 Pengutronix + *
 + * This file is released under the GPLv2
 + */
 +
 +#include linux/slab.h
 +#include linux/display_timing.h
 +
 +void timings_release(struct display_timings *disp)
 +{
 + int i;
 +
 + for (i = 0; i  disp-num_timings; i++)
 + kfree(disp-timings[i]);
 +}

This function doesn't seem to be called externally, you can make it static.

 +void display_timings_release(struct display_timings *disp)
 +{
 + timings_release(disp);
 + kfree(disp-timings);
 +}
 diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
 new file mode 100644
 index 000..aa02a12
 --- /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 hback_porch;
 + struct timing_entry hsync_len;
 +
 + struct timing_entry vactive;
 + struct timing_entry vfront_porch;
 + struct timing_entry vback_porch;
 + struct timing_entry vsync_len;
 +
 + unsigned int vsync_pol_active;
 + unsigned int hsync_pol_active;
 + unsigned int de_pol_active;
 + unsigned int pixelclk_pol;
 + bool interlaced;
 + bool doublescan;
 +};
 +
 +struct display_timings {
 + unsigned int num_timings;
 + unsigned int native_mode;
 +
 + struct display_timing **timings;
 +};
 +
 +/* placeholder function until ranges are really needed */
 +static inline u32 display_timing_get_value(struct timing_entry *te, int
 index) +{
 + return te-typ;
 +}
 +
 +static inline struct display_timing *display_timings_get(struct
 display_timings *disp, +  
  int index)
 +{
 + struct display_timing *dt;
 +
 + if (disp-num_timings  index) {
 + dt = disp-timings[index];
 + return dt;
 + } else
 + return NULL;
 +}
 +void timings_release(struct display_timings *disp);
 +void display_timings_release(struct display_timings *disp);
 +
 +#endif

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html