Re: [PATCH v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
Hi Mauro, Am Mittwoch, den 12.02.2014, 06:53 +0900 schrieb Mauro Carvalho Chehab: [...] diff --git a/include/media/of_graph.h b/include/media/of_graph.h new file mode 100644 index 000..3bbeb60 --- /dev/null +++ b/include/media/of_graph.h @@ -0,0 +1,46 @@ +/* + * OF graph binding parsing helpers + * + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki s.nawro...@samsung.com + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski g.liakhovet...@gmx.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#ifndef __LINUX_OF_GRAPH_H +#define __LINUX_OF_GRAPH_H + +#ifdef CONFIG_OF As a matter of consistency, it would be better to test here for CONFIG_OF_GRAPH instead, to reflect the same symbol that enables such functions as used on Kconfig/Makefile. Maybe I'm trying to be too clever for my own good, but my reasoning was as follows: Suppose I newly use the of_graph_ helpers in a subsystem that does not yet select OF_GRAPH. In that case I'd rather get linking errors earlier rather than stubbed out functions that silently fail to parse the DT later. Since there is config VIDEO_DEV select OF_GRAPH if OF already and the same should be added for other users of device tree graphs, I think stubbing out the functions only if OF is disabled should be enough. regards Philipp -- 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 v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
Em Wed, 12 Feb 2014 10:11:54 +0100 Philipp Zabel p.za...@pengutronix.de escreveu: Hi Mauro, Am Mittwoch, den 12.02.2014, 06:53 +0900 schrieb Mauro Carvalho Chehab: [...] diff --git a/include/media/of_graph.h b/include/media/of_graph.h new file mode 100644 index 000..3bbeb60 --- /dev/null +++ b/include/media/of_graph.h @@ -0,0 +1,46 @@ +/* + * OF graph binding parsing helpers + * + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki s.nawro...@samsung.com + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski g.liakhovet...@gmx.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#ifndef __LINUX_OF_GRAPH_H +#define __LINUX_OF_GRAPH_H + +#ifdef CONFIG_OF As a matter of consistency, it would be better to test here for CONFIG_OF_GRAPH instead, to reflect the same symbol that enables such functions as used on Kconfig/Makefile. Maybe I'm trying to be too clever for my own good, but my reasoning was as follows: Suppose I newly use the of_graph_ helpers in a subsystem that does not yet select OF_GRAPH. In that case I'd rather get linking errors earlier rather than stubbed out functions that silently fail to parse the DT later. I see your point, but, imagining that someone pushed a patch using those symbols upstream, that would break compilation and git bisection, with will hurt everyone, and not only the very few of us that would actually need the OF_GRAPH symbol for an specific driver. Also, such push would mean that someone forgot to do his homework and to test if the committed functionality is actually working. So, it seems more fair that the one that did the mistake will be the one that will suffer the consequences for his errors instead of applying a penalty to everybody's else ;) Since there is config VIDEO_DEV select OF_GRAPH if OF already and the same should be added for other users of device tree graphs, I think stubbing out the functions only if OF is disabled should be enough. Well, if you want to be sure that the graph will always be there if OF, then you could do, instead: config OF_GRAPH bool default OF (that would actually make OF_GRAPH just an alias to OF - so we could just use OF instead). In any case, I think that we should use the same config name at Makefile, Kconfig and of_graph.h (either OF_GRAPH or just OF). Regards, Mauro -- 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 v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
Hi Mauro, Am Mittwoch, den 12.02.2014, 22:35 +0900 schrieb Mauro Carvalho Chehab: Em Wed, 12 Feb 2014 10:11:54 +0100 Philipp Zabel p.za...@pengutronix.de escreveu: Hi Mauro, Am Mittwoch, den 12.02.2014, 06:53 +0900 schrieb Mauro Carvalho Chehab: [...] diff --git a/include/media/of_graph.h b/include/media/of_graph.h new file mode 100644 index 000..3bbeb60 --- /dev/null +++ b/include/media/of_graph.h @@ -0,0 +1,46 @@ +/* + * OF graph binding parsing helpers + * + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki s.nawro...@samsung.com + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski g.liakhovet...@gmx.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#ifndef __LINUX_OF_GRAPH_H +#define __LINUX_OF_GRAPH_H + +#ifdef CONFIG_OF As a matter of consistency, it would be better to test here for CONFIG_OF_GRAPH instead, to reflect the same symbol that enables such functions as used on Kconfig/Makefile. Maybe I'm trying to be too clever for my own good, but my reasoning was as follows: Suppose I newly use the of_graph_ helpers in a subsystem that does not yet select OF_GRAPH. In that case I'd rather get linking errors earlier rather than stubbed out functions that silently fail to parse the DT later. I see your point, but, imagining that someone pushed a patch using those symbols upstream, that would break compilation and git bisection, with will hurt everyone, and not only the very few of us that would actually need the OF_GRAPH symbol for an specific driver. Also, such push would mean that someone forgot to do his homework and to test if the committed functionality is actually working. So, it seems more fair that the one that did the mistake will be the one that will suffer the consequences for his errors instead of applying a penalty to everybody's else ;) point taken. Since there is config VIDEO_DEV select OF_GRAPH if OF already and the same should be added for other users of device tree graphs, I think stubbing out the functions only if OF is disabled should be enough. Well, if you want to be sure that the graph will always be there if OF, then you could do, instead: config OF_GRAPH bool default OF (that would actually make OF_GRAPH just an alias to OF - so we could just use OF instead). In any case, I think that we should use the same config name at Makefile, Kconfig and of_graph.h (either OF_GRAPH or just OF). If nobody states a strong preference, I'll just get rid of the CONFIG_OF_GRAPH option altogether and use CONFIG_OF directly. regards Philipp -- 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
[PATCH v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
From: Philipp Zabel philipp.za...@gmail.com This patch moves the parsing helpers used to parse connected graphs in the device tree, like the video interface bindings documented in Documentation/devicetree/bindings/media/video-interfaces.txt, from drivers/media/v4l2-core to drivers/media. This allows to reuse the same parser code from outside the V4L2 framework, most importantly from display drivers. The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port, and v4l2_of_get_remote_port_parent are moved. They are renamed to of_graph_get_next_endpoint, of_graph_get_remote_port, and of_graph_get_remote_port_parent, respectively. Since there are not that many current users, switch all of them to the new functions right away. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Acked-by: Mauro Carvalho Chehab m.che...@samsung.com Acked-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- Changes since v1: - Moved to drivers/media instead of drives/of - Removed v4l2_of_get_ functions and changed all users in one step - Keep providing stubs for !OF case --- drivers/media/Kconfig | 4 + drivers/media/Makefile| 2 + drivers/media/i2c/adv7343.c | 4 +- drivers/media/i2c/mt9p031.c | 4 +- drivers/media/i2c/s5k5baf.c | 3 +- drivers/media/i2c/tvp514x.c | 3 +- drivers/media/i2c/tvp7002.c | 3 +- drivers/media/of_graph.c | 133 ++ drivers/media/platform/exynos4-is/fimc-is.c | 6 +- drivers/media/platform/exynos4-is/media-dev.c | 3 +- drivers/media/platform/exynos4-is/mipi-csis.c | 3 +- drivers/media/v4l2-core/v4l2-of.c | 117 -- include/media/of_graph.h | 46 + include/media/v4l2-of.h | 24 - 14 files changed, 202 insertions(+), 153 deletions(-) create mode 100644 drivers/media/of_graph.c create mode 100644 include/media/of_graph.h diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 1d0758a..7681026 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -96,6 +96,7 @@ config VIDEO_DEV tristate depends on MEDIA_SUPPORT depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT + select OF_GRAPH if OF default y config VIDEO_V4L2_SUBDEV_API @@ -203,3 +204,6 @@ source drivers/media/tuners/Kconfig source drivers/media/dvb-frontends/Kconfig endif # MEDIA_SUPPORT + +config OF_GRAPH +bool diff --git a/drivers/media/Makefile b/drivers/media/Makefile index 620f275..ff980bd 100644 --- a/drivers/media/Makefile +++ b/drivers/media/Makefile @@ -18,6 +18,8 @@ ifeq ($(CONFIG_MEDIA_CONTROLLER),y) obj-$(CONFIG_MEDIA_SUPPORT) += media.o endif +obj-$(CONFIG_OF_GRAPH) += of_graph.o + obj-$(CONFIG_VIDEO_DEV) += v4l2-core/ obj-$(CONFIG_DVB_CORE) += dvb-core/ diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c index d4e15a6..74a1507 100644 --- a/drivers/media/i2c/adv7343.c +++ b/drivers/media/i2c/adv7343.c @@ -28,10 +28,10 @@ #include linux/of.h #include media/adv7343.h +#include media/of_graph.h #include media/v4l2-async.h #include media/v4l2-device.h #include media/v4l2-ctrls.h -#include media/v4l2-of.h #include adv7343_regs.h @@ -410,7 +410,7 @@ adv7343_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node) return client-dev.platform_data; - np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL); + np = of_graph_get_next_endpoint(client-dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c index e5ddf47..60f36dc 100644 --- a/drivers/media/i2c/mt9p031.c +++ b/drivers/media/i2c/mt9p031.c @@ -27,9 +27,9 @@ #include linux/videodev2.h #include media/mt9p031.h +#include media/of_graph.h #include media/v4l2-ctrls.h #include media/v4l2-device.h -#include media/v4l2-of.h #include media/v4l2-subdev.h #include aptina-pll.h @@ -943,7 +943,7 @@ mt9p031_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node) return client-dev.platform_data; - np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL); + np = of_graph_get_next_endpoint(client-dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c index 77e10e0..06261ee 100644 --- a/drivers/media/i2c/s5k5baf.c +++ b/drivers/media/i2c/s5k5baf.c @@ -25,6 +25,7 @@ #include linux/slab.h #include media/media-entity.h +#include media/of_graph.h #include media/v4l2-ctrls.h #include media/v4l2-device.h #include media/v4l2-subdev.h @@ -1855,7 +1856,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev) if
Re: [PATCH v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
Em Tue, 11 Feb 2014 22:41:45 +0100 Philipp Zabel p.za...@pengutronix.de escreveu: From: Philipp Zabel philipp.za...@gmail.com This patch moves the parsing helpers used to parse connected graphs in the device tree, like the video interface bindings documented in Documentation/devicetree/bindings/media/video-interfaces.txt, from drivers/media/v4l2-core to drivers/media. This allows to reuse the same parser code from outside the V4L2 framework, most importantly from display drivers. The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port, and v4l2_of_get_remote_port_parent are moved. They are renamed to of_graph_get_next_endpoint, of_graph_get_remote_port, and of_graph_get_remote_port_parent, respectively. Since there are not that many current users, switch all of them to the new functions right away. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Acked-by: Mauro Carvalho Chehab m.che...@samsung.com Acked-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- Changes since v1: - Moved to drivers/media instead of drives/of - Removed v4l2_of_get_ functions and changed all users in one step - Keep providing stubs for !OF case --- drivers/media/Kconfig | 4 + drivers/media/Makefile| 2 + drivers/media/i2c/adv7343.c | 4 +- drivers/media/i2c/mt9p031.c | 4 +- drivers/media/i2c/s5k5baf.c | 3 +- drivers/media/i2c/tvp514x.c | 3 +- drivers/media/i2c/tvp7002.c | 3 +- drivers/media/of_graph.c | 133 ++ drivers/media/platform/exynos4-is/fimc-is.c | 6 +- drivers/media/platform/exynos4-is/media-dev.c | 3 +- drivers/media/platform/exynos4-is/mipi-csis.c | 3 +- drivers/media/v4l2-core/v4l2-of.c | 117 -- include/media/of_graph.h | 46 + include/media/v4l2-of.h | 24 - 14 files changed, 202 insertions(+), 153 deletions(-) create mode 100644 drivers/media/of_graph.c create mode 100644 include/media/of_graph.h diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 1d0758a..7681026 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -96,6 +96,7 @@ config VIDEO_DEV tristate depends on MEDIA_SUPPORT depends on MEDIA_CAMERA_SUPPORT || MEDIA_ANALOG_TV_SUPPORT || MEDIA_RADIO_SUPPORT + select OF_GRAPH if OF default y config VIDEO_V4L2_SUBDEV_API @@ -203,3 +204,6 @@ source drivers/media/tuners/Kconfig source drivers/media/dvb-frontends/Kconfig endif # MEDIA_SUPPORT + +config OF_GRAPH +bool diff --git a/drivers/media/Makefile b/drivers/media/Makefile index 620f275..ff980bd 100644 --- a/drivers/media/Makefile +++ b/drivers/media/Makefile @@ -18,6 +18,8 @@ ifeq ($(CONFIG_MEDIA_CONTROLLER),y) obj-$(CONFIG_MEDIA_SUPPORT) += media.o endif +obj-$(CONFIG_OF_GRAPH) += of_graph.o + obj-$(CONFIG_VIDEO_DEV) += v4l2-core/ obj-$(CONFIG_DVB_CORE) += dvb-core/ diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c index d4e15a6..74a1507 100644 --- a/drivers/media/i2c/adv7343.c +++ b/drivers/media/i2c/adv7343.c @@ -28,10 +28,10 @@ #include linux/of.h #include media/adv7343.h +#include media/of_graph.h #include media/v4l2-async.h #include media/v4l2-device.h #include media/v4l2-ctrls.h -#include media/v4l2-of.h #include adv7343_regs.h @@ -410,7 +410,7 @@ adv7343_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node) return client-dev.platform_data; - np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL); + np = of_graph_get_next_endpoint(client-dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c index e5ddf47..60f36dc 100644 --- a/drivers/media/i2c/mt9p031.c +++ b/drivers/media/i2c/mt9p031.c @@ -27,9 +27,9 @@ #include linux/videodev2.h #include media/mt9p031.h +#include media/of_graph.h #include media/v4l2-ctrls.h #include media/v4l2-device.h -#include media/v4l2-of.h #include media/v4l2-subdev.h #include aptina-pll.h @@ -943,7 +943,7 @@ mt9p031_get_pdata(struct i2c_client *client) if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node) return client-dev.platform_data; - np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL); + np = of_graph_get_next_endpoint(client-dev.of_node, NULL); if (!np) return NULL; diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c index 77e10e0..06261ee 100644 --- a/drivers/media/i2c/s5k5baf.c +++ b/drivers/media/i2c/s5k5baf.c @@ -25,6 +25,7 @@ #include linux/slab.h #include media/media-entity.h +#include media/of_graph.h #include
Re: [PATCH v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media
Hi, On 11/02/14 23:41, Philipp Zabel wrote: From: Philipp Zabel philipp.za...@gmail.com This patch moves the parsing helpers used to parse connected graphs in the device tree, like the video interface bindings documented in Documentation/devicetree/bindings/media/video-interfaces.txt, from drivers/media/v4l2-core to drivers/media. This allows to reuse the same parser code from outside the V4L2 framework, most importantly from display drivers. The functions v4l2_of_get_next_endpoint, v4l2_of_get_remote_port, and v4l2_of_get_remote_port_parent are moved. They are renamed to of_graph_get_next_endpoint, of_graph_get_remote_port, and of_graph_get_remote_port_parent, respectively. Since there are not that many current users, switch all of them to the new functions right away. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Acked-by: Mauro Carvalho Chehab m.che...@samsung.com Acked-by: Guennadi Liakhovetski g.liakhovet...@gmx.de I don't think the graphs or the parsing code has anything video specific. It could well be used for anything, whenever there's need to describe connections between devices which are not handled by the normal child-parent relationships. So the code could well reside in some generic place, in my opinion. Also, I have no problem with having it in drivers/media, but drivers/video should work also. We already have other, generic, video related things there like hdmi infoframes and display timings. And last, it's fine to move the funcs as-is in the first place, but I think they should be improved a bit before non-v4l2 users use them. There are a couple of things I tried to accomplish with the omapdss specific versions in https://www.mail-archive.com/linux-omap@vger.kernel.org/msg100761.html: - Iterating ports and endpoints separately. If a node has multiple ports, I would think that the driver needs to know which port and endpoint combination is the current one during iteration. It's not enough to just get the endpoint. - Simplify cases when there's just one port and one endpoint, in which case the port node can be omitted from the DT data. Tomi signature.asc Description: OpenPGP digital signature
