Re: [PATCH v8 1/6] video: add display_timing and videomode
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
pgpBUyLwaWzk6.pgp
Description: PGP signature
Re: [PATCH v8 1/6] video: add display_timing and videomode
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- |
--
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 v8 1/6] video: add display_timing and videomode
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
pgpwvfZVVkKPO.pgp
Description: PGP signature
Re: [PATCH v8 1/6] video: add display_timing and videomode
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- |
--
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 v8 1/6] video: add display_timing and videomode
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
pgp6Ncscso1x3.pgp
Description: PGP signature
Re: [PATCH v8 1/6] video: add display_timing and videomode
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
pgpRxdmr3W6ZW.pgp
Description: PGP signature
Re: [PATCH v8 1/6] video: add display_timing and videomode
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- |
--
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
