| Message ID | 1348754853-28619-6-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
On 09/27/2012 04:07 PM, Guennadi Liakhovetski wrote: > Add a V4L2 OF parser, implementing bindings, documented in > Documentation/devicetree/bindings/media/v4l2.txt. > > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de> > --- > drivers/media/v4l2-core/Makefile | 3 + > drivers/media/v4l2-core/v4l2-of.c | 190 +++++++++++++++++++++++++++++++++++++ > include/media/v4l2-of.h | 62 ++++++++++++ > 3 files changed, 255 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/v4l2-core/v4l2-of.c > create mode 100644 include/media/v4l2-of.h > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > index c2d61d4..00f64d6 100644 > --- a/drivers/media/v4l2-core/Makefile > +++ b/drivers/media/v4l2-core/Makefile > @@ -9,6 +9,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ > ifeq ($(CONFIG_COMPAT),y) > videodev-objs += v4l2-compat-ioctl32.o > endif > +ifeq ($(CONFIG_OF),y) > + videodev-objs += v4l2-of.o > +endif > > obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o > obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c > new file mode 100644 > index 0000000..f45d64b > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-of.c > @@ -0,0 +1,190 @@ > +/* > + * V4L2 OF binding parsing library > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski<g.liakhovetski@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. > + */ > +#include<linux/kernel.h> > +#include<linux/module.h> > +#include<linux/of.h> > +#include<linux/types.h> > + > +#include<media/v4l2-of.h> > + > +/* > + * All properties are optional. If none are found, we don't set any flags. This > + * means, the port has a static configuration and no properties have to be > + * specified explicitly. > + * If any properties are found, that identify the bus as parallel, and > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise the > + * bus as serial CSI-2 and clock-noncontinuous isn't set, we set the > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. > + * The caller should hold a reference to "node." > + */ > +void v4l2_of_parse_link(const struct device_node *node, > + struct v4l2_of_link *link) > +{ > + const struct device_node *port_node = of_get_parent(node); > + int size; > + unsigned int v; > + u32 data_lanes[ARRAY_SIZE(link->mipi_csi_2.data_lanes)]; > + bool data_lanes_present; > + > + memset(link, 0, sizeof(*link)); > + > + link->local_node = node; > + > + /* Doesn't matter, whether the below two calls succeed */ > + of_property_read_u32(port_node, "reg",&link->port); > + of_property_read_u32(node, "reg",&link->addr); > + > + if (!of_property_read_u32(node, "bus-width",&v)) > + link->parallel.bus_width = v; > + > + if (!of_property_read_u32(node, "data-shift",&v)) > + link->parallel.data_shift = v; > + > + if (!of_property_read_u32(node, "hsync-active",&v)) > + link->mbus_flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH : > + V4L2_MBUS_HSYNC_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "vsync-active",&v)) > + link->mbus_flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH : > + V4L2_MBUS_VSYNC_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "data-active",&v)) > + link->mbus_flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH : > + V4L2_MBUS_DATA_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "pclk-sample",&v)) > + link->mbus_flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING : > + V4L2_MBUS_PCLK_SAMPLE_FALLING; > + > + if (!of_property_read_u32(node, "field-even-active",&v)) > + link->mbus_flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH : > + V4L2_MBUS_FIELD_EVEN_LOW; > + > + if (of_get_property(node, "slave-mode",&size)) > + link->mbus_flags |= V4L2_MBUS_SLAVE; > + > + /* If any parallel-bus properties have been found, skip serial ones */ > + if (link->parallel.bus_width || link->parallel.data_shift || > + link->mbus_flags) { > + /* Default parallel bus-master */ > + if (!(link->mbus_flags& V4L2_MBUS_SLAVE)) > + link->mbus_flags |= V4L2_MBUS_MASTER; > + return; > + } > + > + if (!of_property_read_u32(node, "clock-lanes",&v)) > + link->mipi_csi_2.clock_lane = v; > + > + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > + ARRAY_SIZE(data_lanes))) { > + int i; > + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; It doesn't look like what we aimed for. The data-lanes array is supposed to be of variable length, thus I don't think it can be parsed like that. Or am I missing something ? I think we need more something like below (based on of_property_read_u32_array(), not tested): void v4l2_of_parse_mipi_csi2(const struct device_node *node, struct mipi_csi2 *mipi_csi2) { struct property *prop = of_find_property(node, "data-lanes", NULL); u8 *out_data_lanes = mipi_csi_2->data_lanes; int num_lanes; const __be32 *val; if (!prop) return; mipi_csi2->num_lanes = 0; if (WARN (!prop->value, "Empty data-lanes property\n")) return; num_lanes = prop->length / sizeof(u32); if (WARN_ON(num_lanes > ARRAY_SIZE(mipi_csi_2->data_lanes)) num_lanes = ARRAY_SIZE(mipi_csi_2->data_lanes); val = prop->value; while (num_lanes--) *out_data_lanes++ = be32_to_cpup(val++); mipi_csi2->num_lanes = num_lanes; } v4l2_of_parse_mipi_csi2(node, &link->mipi_csi2); > + data_lanes_present = true; > + } else { > + data_lanes_present = false; > + } > + > + if (of_get_property(node, "clock-noncontinuous",&size)) > + link->mbus_flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > + > + if ((link->mipi_csi_2.clock_lane || data_lanes_present)&& > + !(link->mbus_flags& V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) > + /* Default CSI-2: continuous clock */ > + link->mbus_flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; > +} > +EXPORT_SYMBOL(v4l2_of_parse_link); > + ... > diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h > new file mode 100644 > index 0000000..6fafedb > --- /dev/null > +++ b/include/media/v4l2-of.h > @@ -0,0 +1,62 @@ > +/* > + * V4L2 OF binding parsing library > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski<g.liakhovetski@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 _V4L2_OF_H > +#define _V4L2_OF_H > + > +#include<linux/list.h> > +#include<linux/types.h> > + > +#include<media/v4l2-mediabus.h> > + > +struct device_node; > + > +struct v4l2_of_link { > + unsigned int port; > + unsigned int addr; > + struct list_head head; > + const struct device_node *local_node; > + const __be32 *remote; > + unsigned int mbus_flags; > + union { > + struct { > + unsigned char bus_width; > + unsigned char data_shift; > + } parallel; > + struct { > + unsigned char data_lanes[4]; Some devices are interested only in absolute number of lanes. I can't see how we could specify the number of data lanes here. Shouldn't something like 'unsigned char num_data_lanes;' be added to this structure ? > + unsigned char clock_lane; > + } mipi_csi_2; > + }; > +}; -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester On Mon, 1 Oct 2012, Sylwester Nawrocki wrote: > On 09/27/2012 04:07 PM, Guennadi Liakhovetski wrote: > > Add a V4L2 OF parser, implementing bindings, documented in > > Documentation/devicetree/bindings/media/v4l2.txt. > > > > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de> > > --- > > drivers/media/v4l2-core/Makefile | 3 + > > drivers/media/v4l2-core/v4l2-of.c | 190 +++++++++++++++++++++++++++++++++++++ > > include/media/v4l2-of.h | 62 ++++++++++++ > > 3 files changed, 255 insertions(+), 0 deletions(-) > > create mode 100644 drivers/media/v4l2-core/v4l2-of.c > > create mode 100644 include/media/v4l2-of.h > > > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > > index c2d61d4..00f64d6 100644 > > --- a/drivers/media/v4l2-core/Makefile > > +++ b/drivers/media/v4l2-core/Makefile > > @@ -9,6 +9,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ > > ifeq ($(CONFIG_COMPAT),y) > > videodev-objs += v4l2-compat-ioctl32.o > > endif > > +ifeq ($(CONFIG_OF),y) > > + videodev-objs += v4l2-of.o > > +endif > > > > obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o > > obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o > > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c > > new file mode 100644 > > index 0000000..f45d64b > > --- /dev/null > > +++ b/drivers/media/v4l2-core/v4l2-of.c > > @@ -0,0 +1,190 @@ > > +/* > > + * V4L2 OF binding parsing library > > + * > > + * Copyright (C) 2012 Renesas Electronics Corp. > > + * Author: Guennadi Liakhovetski<g.liakhovetski@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. > > + */ > > +#include<linux/kernel.h> > > +#include<linux/module.h> > > +#include<linux/of.h> > > +#include<linux/types.h> > > + > > +#include<media/v4l2-of.h> > > + > > +/* > > + * All properties are optional. If none are found, we don't set any flags. This > > + * means, the port has a static configuration and no properties have to be > > + * specified explicitly. > > + * If any properties are found, that identify the bus as parallel, and > > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise the > > + * bus as serial CSI-2 and clock-noncontinuous isn't set, we set the > > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. > > + * The caller should hold a reference to "node." > > + */ > > +void v4l2_of_parse_link(const struct device_node *node, > > + struct v4l2_of_link *link) > > +{ > > + const struct device_node *port_node = of_get_parent(node); > > + int size; > > + unsigned int v; > > + u32 data_lanes[ARRAY_SIZE(link->mipi_csi_2.data_lanes)]; > > + bool data_lanes_present; > > + > > + memset(link, 0, sizeof(*link)); > > + > > + link->local_node = node; > > + > > + /* Doesn't matter, whether the below two calls succeed */ > > + of_property_read_u32(port_node, "reg",&link->port); > > + of_property_read_u32(node, "reg",&link->addr); > > + > > + if (!of_property_read_u32(node, "bus-width",&v)) > > + link->parallel.bus_width = v; > > + > > + if (!of_property_read_u32(node, "data-shift",&v)) > > + link->parallel.data_shift = v; > > + > > + if (!of_property_read_u32(node, "hsync-active",&v)) > > + link->mbus_flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH : > > + V4L2_MBUS_HSYNC_ACTIVE_LOW; > > + > > + if (!of_property_read_u32(node, "vsync-active",&v)) > > + link->mbus_flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH : > > + V4L2_MBUS_VSYNC_ACTIVE_LOW; > > + > > + if (!of_property_read_u32(node, "data-active",&v)) > > + link->mbus_flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH : > > + V4L2_MBUS_DATA_ACTIVE_LOW; > > + > > + if (!of_property_read_u32(node, "pclk-sample",&v)) > > + link->mbus_flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING : > > + V4L2_MBUS_PCLK_SAMPLE_FALLING; > > + > > + if (!of_property_read_u32(node, "field-even-active",&v)) > > + link->mbus_flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH : > > + V4L2_MBUS_FIELD_EVEN_LOW; > > + > > + if (of_get_property(node, "slave-mode",&size)) > > + link->mbus_flags |= V4L2_MBUS_SLAVE; > > + > > + /* If any parallel-bus properties have been found, skip serial ones */ > > + if (link->parallel.bus_width || link->parallel.data_shift || > > + link->mbus_flags) { > > + /* Default parallel bus-master */ > > + if (!(link->mbus_flags& V4L2_MBUS_SLAVE)) > > + link->mbus_flags |= V4L2_MBUS_MASTER; > > + return; > > + } > > + > > + if (!of_property_read_u32(node, "clock-lanes",&v)) > > + link->mipi_csi_2.clock_lane = v; > > + > > + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > > + ARRAY_SIZE(data_lanes))) { > > + int i; > > + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > > + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > It doesn't look like what we aimed for. The data-lanes array is supposed > to be of variable length, thus I don't think it can be parsed like that. > Or am I missing something ? I think we need more something like below > (based on of_property_read_u32_array(), not tested): Ok, you're right, that my version only was suitable for fixed-size arrays, which wasn't our goal. But I don't think we should go down to manually parsing property data. How about (tested;-)) data = of_find_property(node, "data-lanes", NULL); if (data) { int i = 0; const __be32 *lane = NULL; do { lane = of_prop_next_u32(data, lane, &data_lanes[i]); } while (lane && i++ < ARRAY_SIZE(data_lanes)); link->mipi_csi_2.num_data_lanes = i; while (i--) link->mipi_csi_2.data_lanes[i] = data_lanes[i]; } > void v4l2_of_parse_mipi_csi2(const struct device_node *node, > struct mipi_csi2 *mipi_csi2) > { > struct property *prop = of_find_property(node, "data-lanes", NULL); > u8 *out_data_lanes = mipi_csi_2->data_lanes; > int num_lanes; > const __be32 *val; > > if (!prop) > return; > > mipi_csi2->num_lanes = 0; > > if (WARN (!prop->value, "Empty data-lanes property\n")) > return; > > num_lanes = prop->length / sizeof(u32); > if (WARN_ON(num_lanes > ARRAY_SIZE(mipi_csi_2->data_lanes)) > num_lanes = ARRAY_SIZE(mipi_csi_2->data_lanes); > > val = prop->value; > while (num_lanes--) > *out_data_lanes++ = be32_to_cpup(val++); > > mipi_csi2->num_lanes = num_lanes; > } > > v4l2_of_parse_mipi_csi2(node, &link->mipi_csi2); [snip] > > +struct v4l2_of_link { > > + unsigned int port; > > + unsigned int addr; > > + struct list_head head; > > + const struct device_node *local_node; > > + const __be32 *remote; > > + unsigned int mbus_flags; > > + union { > > + struct { > > + unsigned char bus_width; > > + unsigned char data_shift; > > + } parallel; > > + struct { > > + unsigned char data_lanes[4]; > > Some devices are interested only in absolute number of lanes. > I can't see how we could specify the number of data lanes here. > Shouldn't something like 'unsigned char num_data_lanes;' be > added to this structure ? Yes, good point. As you see above, I've added it. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, >>> + ARRAY_SIZE(data_lanes))) { >>> + int i; >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; >> >> It doesn't look like what we aimed for. The data-lanes array is supposed >> to be of variable length, thus I don't think it can be parsed like that. >> Or am I missing something ? I think we need more something like below >> (based on of_property_read_u32_array(), not tested): > > Ok, you're right, that my version only was suitable for fixed-size arrays, > which wasn't our goal. But I don't think we should go down to manually > parsing property data. How about (tested;-)) > > data = of_find_property(node, "data-lanes", NULL); > if (data) { > int i = 0; > const __be32 *lane = NULL; > do { > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > link->mipi_csi_2.num_data_lanes = i; > while (i--) > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > } Yes, that looks neat and does what it is supposed to do. :) Thanks! For now, I'll trust you it works ;) With regards to the remaining patches, it looks a bit scary to me how complicated it got, perhaps mostly because of requirement to reference host devices from subdevs. And it seems to rely on the existing SoC camera infrastructure, which might imply lot's of work need to be done for non soc-camera drivers. But I'm going to take a closer look and comment more on the details at the corresponding patches. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2 Oct 2012, Sylwester Nawrocki wrote: > Hi Guennadi, > > On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: > >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > >>> + ARRAY_SIZE(data_lanes))) { > >>> + int i; > >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > >> > >> It doesn't look like what we aimed for. The data-lanes array is supposed > >> to be of variable length, thus I don't think it can be parsed like that. > >> Or am I missing something ? I think we need more something like below > >> (based on of_property_read_u32_array(), not tested): > > > > Ok, you're right, that my version only was suitable for fixed-size arrays, > > which wasn't our goal. But I don't think we should go down to manually > > parsing property data. How about (tested;-)) > > > > data = of_find_property(node, "data-lanes", NULL); > > if (data) { > > int i = 0; > > const __be32 *lane = NULL; > > do { > > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > > link->mipi_csi_2.num_data_lanes = i; > > while (i--) > > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > } > > Yes, that looks neat and does what it is supposed to do. :) Thanks! > For now, I'll trust you it works ;) > > With regards to the remaining patches, it looks a bit scary to me how > complicated it got, perhaps mostly because of requirement to reference > host devices from subdevs. If you mean uses of the v4l2_of_get_remote() function, then it's the other way round: I'm accessing subdevices from bridges, which is also understandable - you need the subdevice. Or do you mean the need to turn the master clock on and off from the subdevice driver? This shall be eliminated, once we switch to using the generic clock framework. > And it seems to rely on the existing SoC > camera infrastructure, which might imply lot's of work need to be done > for non soc-camera drivers. Sorry, what "it" is relying on soc-camera? The patch series consists of several generic patches, which have nothing to do with soc-camera, and patches, porting soc-camera to use OF with cameras. I think, complexity with soc-camera is also higher, than what you'd need with specific single bridge drivers, because it is generic. A big part of the complexity is supporting deferred subdevice (sensor) probing. Beginning with what most bridge drivers currently use - static subdevice lists in platform data, for which they then register I2C devices, it is natural to implement that in 2 steps: (1) support directly registered I2C sensors from platform data, that request deferred probing until the bridge driver has been probed and is ready to handle them; (2) support I2C subdevices in DT. After this your bridge driver will support 3 (!) modes in which subdevices can be initialised. In principle you could drop step (1) - supporting that isn't really required, but then the jump to (2) will be larger. Another part of the complexity is specific to soc-camera, it comes from the way, how subdevices are represented in platform data - as platform devices with a bus ID, used to link bridges and subdevices. With DT those platform devices and bus ID have to be generated dynamically. And you're right - soc-camera bridge drivers have the advantage, that the soc-camera core has taken the most work on supporting DT on itself, so, DT support will come to them at only a fraction of the cost;-) Thanks Guennadi > But I'm going to take a closer look and > comment more on the details at the corresponding patches. > > -- > > Regards, > Sylwester --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue October 2 2012 12:13:20 Sylwester Nawrocki wrote: > Hi Guennadi, > > On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: > >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > >>> + ARRAY_SIZE(data_lanes))) { > >>> + int i; > >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > >> > >> It doesn't look like what we aimed for. The data-lanes array is supposed > >> to be of variable length, thus I don't think it can be parsed like that. > >> Or am I missing something ? I think we need more something like below > >> (based on of_property_read_u32_array(), not tested): > > > > Ok, you're right, that my version only was suitable for fixed-size arrays, > > which wasn't our goal. But I don't think we should go down to manually > > parsing property data. How about (tested;-)) > > > > data = of_find_property(node, "data-lanes", NULL); > > if (data) { > > int i = 0; > > const __be32 *lane = NULL; > > do { > > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > > link->mipi_csi_2.num_data_lanes = i; > > while (i--) > > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > } > > Yes, that looks neat and does what it is supposed to do. :) Thanks! > For now, I'll trust you it works ;) > > With regards to the remaining patches, it looks a bit scary to me how > complicated it got, perhaps mostly because of requirement to reference > host devices from subdevs. And it seems to rely on the existing SoC > camera infrastructure, which might imply lot's of work need to be done > for non soc-camera drivers. But I'm going to take a closer look and > comment more on the details at the corresponding patches. I have to say that I agree with Sylwester here. It seems awfully complicated, but I can't really put my finger on the actual cause. It would be really interesting to see this implemented for a non-SoC device and to compare the two. One area that I do not yet completely understand is the i2c bus notifications (or asynchronous loading or i2c modules). I would have expected that using OF the i2c devices are still initialized before the host/bridge driver is initialized. But I gather that's not the case? If this deferred probing is a general problem, then I think we need a general solution as well that's part of the v4l2 core. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 5 Oct 2012, Hans Verkuil wrote: > On Tue October 2 2012 12:13:20 Sylwester Nawrocki wrote: > > Hi Guennadi, > > > > On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: > > >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > > >>> + ARRAY_SIZE(data_lanes))) { > > >>> + int i; > > >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > > >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > >> > > >> It doesn't look like what we aimed for. The data-lanes array is supposed > > >> to be of variable length, thus I don't think it can be parsed like that. > > >> Or am I missing something ? I think we need more something like below > > >> (based on of_property_read_u32_array(), not tested): > > > > > > Ok, you're right, that my version only was suitable for fixed-size arrays, > > > which wasn't our goal. But I don't think we should go down to manually > > > parsing property data. How about (tested;-)) > > > > > > data = of_find_property(node, "data-lanes", NULL); > > > if (data) { > > > int i = 0; > > > const __be32 *lane = NULL; > > > do { > > > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > > > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > > > link->mipi_csi_2.num_data_lanes = i; > > > while (i--) > > > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > > } > > > > Yes, that looks neat and does what it is supposed to do. :) Thanks! > > For now, I'll trust you it works ;) > > > > With regards to the remaining patches, it looks a bit scary to me how > > complicated it got, perhaps mostly because of requirement to reference > > host devices from subdevs. And it seems to rely on the existing SoC > > camera infrastructure, which might imply lot's of work need to be done > > for non soc-camera drivers. But I'm going to take a closer look and > > comment more on the details at the corresponding patches. > > I have to say that I agree with Sylwester here. It seems awfully complicated, > but I can't really put my finger on the actual cause. Well, which exactly part? The V4L2 OF part is quite simple. > It would be really > interesting to see this implemented for a non-SoC device and to compare the > two. Sure, volunteers? ;-) In principle, if I find time, I could try to convert sh_vou, which is also interesting, because it's an output driver. > One area that I do not yet completely understand is the i2c bus notifications > (or asynchronous loading or i2c modules). > > I would have expected that using OF the i2c devices are still initialized > before the host/bridge driver is initialized. But I gather that's not the > case? No, it's not. I'm not sure, whether it depends on the order of devices in the .dts, but, I think, it's better to not have to mandate a certain order and I also seem to have seen devices being registered in different order with the same DT, but I'm not 100% sure about that. > If this deferred probing is a general problem, then I think we need a general > solution as well that's part of the v4l2 core. That can be done, perhaps. But we can do it as a next step. As soon as we're happy with the OF implementation as such, we can commit that, possibly leaving soc-camera patches out for now, then we can think where to put async I2C handling. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri October 5 2012 12:58:21 Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > On Tue October 2 2012 12:13:20 Sylwester Nawrocki wrote: > > > Hi Guennadi, > > > > > > On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: > > > >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > > > >>> + ARRAY_SIZE(data_lanes))) { > > > >>> + int i; > > > >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > > > >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > > >> > > > >> It doesn't look like what we aimed for. The data-lanes array is supposed > > > >> to be of variable length, thus I don't think it can be parsed like that. > > > >> Or am I missing something ? I think we need more something like below > > > >> (based on of_property_read_u32_array(), not tested): > > > > > > > > Ok, you're right, that my version only was suitable for fixed-size arrays, > > > > which wasn't our goal. But I don't think we should go down to manually > > > > parsing property data. How about (tested;-)) > > > > > > > > data = of_find_property(node, "data-lanes", NULL); > > > > if (data) { > > > > int i = 0; > > > > const __be32 *lane = NULL; > > > > do { > > > > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > > > > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > > > > link->mipi_csi_2.num_data_lanes = i; > > > > while (i--) > > > > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > > > } > > > > > > Yes, that looks neat and does what it is supposed to do. :) Thanks! > > > For now, I'll trust you it works ;) > > > > > > With regards to the remaining patches, it looks a bit scary to me how > > > complicated it got, perhaps mostly because of requirement to reference > > > host devices from subdevs. And it seems to rely on the existing SoC > > > camera infrastructure, which might imply lot's of work need to be done > > > for non soc-camera drivers. But I'm going to take a closer look and > > > comment more on the details at the corresponding patches. > > > > I have to say that I agree with Sylwester here. It seems awfully complicated, > > but I can't really put my finger on the actual cause. > > Well, which exactly part? The V4L2 OF part is quite simple. No, the soc_camera part. The V4L2 OF part looks OK. Sorry, I should have mentioned that! > > It would be really > > interesting to see this implemented for a non-SoC device and to compare the > > two. > > Sure, volunteers? ;-) In principle, if I find time, I could try to convert > sh_vou, which is also interesting, because it's an output driver. > > > One area that I do not yet completely understand is the i2c bus notifications > > (or asynchronous loading or i2c modules). > > > > I would have expected that using OF the i2c devices are still initialized > > before the host/bridge driver is initialized. But I gather that's not the > > case? > > No, it's not. I'm not sure, whether it depends on the order of devices in > the .dts, but, I think, it's better to not have to mandate a certain order > and I also seem to have seen devices being registered in different order > with the same DT, but I'm not 100% sure about that. > > > If this deferred probing is a general problem, then I think we need a general > > solution as well that's part of the v4l2 core. > > That can be done, perhaps. But we can do it as a next step. As soon as > we're happy with the OF implementation as such, we can commit that, > possibly leaving soc-camera patches out for now, then we can think where > to put async I2C handling. It would be good to have a number of 'Reviewed-by's or 'Acked-by's for the DT binding documentation at least before it is merged. I think the soc_camera patches should be left out for now. I suspect that by adding core support for async i2c handling first, the soc_camera patches will become a lot easier to understand. Regards, Hans > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 5 Oct 2012, Hans Verkuil wrote: > On Fri October 5 2012 12:58:21 Guennadi Liakhovetski wrote: > > On Fri, 5 Oct 2012, Hans Verkuil wrote: [snip] > > > One area that I do not yet completely understand is the i2c bus notifications > > > (or asynchronous loading or i2c modules). > > > > > > I would have expected that using OF the i2c devices are still initialized > > > before the host/bridge driver is initialized. But I gather that's not the > > > case? > > > > No, it's not. I'm not sure, whether it depends on the order of devices in > > the .dts, but, I think, it's better to not have to mandate a certain order > > and I also seem to have seen devices being registered in different order > > with the same DT, but I'm not 100% sure about that. > > > > > If this deferred probing is a general problem, then I think we need a general > > > solution as well that's part of the v4l2 core. > > > > That can be done, perhaps. But we can do it as a next step. As soon as > > we're happy with the OF implementation as such, we can commit that, > > possibly leaving soc-camera patches out for now, then we can think where > > to put async I2C handling. > > It would be good to have a number of 'Reviewed-by's or 'Acked-by's for the > DT binding documentation at least before it is merged. Definitely, I'm sure you'll be honoured to be the first one in the list;-) > I think the soc_camera patches should be left out for now. I suspect that > by adding core support for async i2c handling first, the soc_camera patches > will become a lot easier to understand. Ok, we can do this. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/05/2012 12:58 PM, Guennadi Liakhovetski wrote: >> One area that I do not yet completely understand is the i2c bus notifications >> (or asynchronous loading of i2c modules). >> >> I would have expected that using OF the i2c devices are still initialized >> before the host/bridge driver is initialized. But I gather that's not the >> case? > > No, it's not. I'm not sure, whether it depends on the order of devices in > the .dts, but, I think, it's better to not have to mandate a certain order > and I also seem to have seen devices being registered in different order > with the same DT, but I'm not 100% sure about that. The device instantiation (and initialization) order is not something that is supposed to be ensured by a specific device tree source layout, IIUC. The initialization sequence is probably something that is specific to a particular operating system. I checked the device tree compiler but couldn't find if it is free to reorder anything when converting from the human readable device tree to its binary representation. The deferred probing was introduced in Linux to resolve resource inter-dependencies in case of FDT based booting AFAIK. >> If this deferred probing is a general problem, then I think we need a general >> solution as well that's part of the v4l2 core. > > That can be done, perhaps. But we can do it as a next step. As soon as > we're happy with the OF implementation as such, we can commit that, > possibly leaving soc-camera patches out for now, then we can think where > to put async I2C handling. I would really like to see more than one user until we add any core code. No that it couldn't be changed afterwards, but it would be nice to ensure the concepts are right and proven in real life. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 05, 2012 at 08:30:59PM +0200, Sylwester Nawrocki wrote: > The deferred probing was introduced in Linux to resolve resource > inter-dependencies in case of FDT based booting AFAIK. It's completely unrelated to FDT, it's a general issue. There's no sane way to use hacks like link ordering to resolve boot time dependencies - start getting things like regulators connected to I2C or SPI controllers which may also use GPIOs for some signals and may be parents for other regulators and mapping out the dependency graph becomes unreasonably complicated. Deferred probing is designed to solve this problem by working things out dynamically at runtime. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > On 10/05/2012 12:58 PM, Guennadi Liakhovetski wrote: > >> One area that I do not yet completely understand is the i2c bus notifications > >> (or asynchronous loading of i2c modules). > >> > >> I would have expected that using OF the i2c devices are still initialized > >> before the host/bridge driver is initialized. But I gather that's not the > >> case? > > > > No, it's not. I'm not sure, whether it depends on the order of devices in > > the .dts, but, I think, it's better to not have to mandate a certain order > > and I also seem to have seen devices being registered in different order > > with the same DT, but I'm not 100% sure about that. > > The device instantiation (and initialization) order is not something that > is supposed to be ensured by a specific device tree source layout, IIUC. > The initialization sequence is probably something that is specific to a > particular operating system. I checked the device tree compiler but couldn't > find if it is free to reorder anything when converting from the human > readable device tree to its binary representation. > > The deferred probing was introduced in Linux to resolve resource > inter-dependencies in case of FDT based booting AFAIK. > > >> If this deferred probing is a general problem, then I think we need a general > >> solution as well that's part of the v4l2 core. > > > > That can be done, perhaps. But we can do it as a next step. As soon as > > we're happy with the OF implementation as such, we can commit that, > > possibly leaving soc-camera patches out for now, then we can think where > > to put async I2C handling. > > I would really like to see more than one user until we add any core code. > No that it couldn't be changed afterwards, but it would be nice to ensure > the concepts are right and proven in real life. Unfortunately I don't have any more systems on which I could easily enough try this. I've got a beagleboard with a camera, but I don't think I'm a particularly good candidate for implementing DT support for OMAP3 camera drivers;-) Apart from that I've only got soc-camera based systems, of which none are _really_ DT-ready... At best I could try an i.MX31 based board, but that doesn't have a very well developed .dts and that would be soc-camera too anyway. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On 09/27/2012 04:07 PM, Guennadi Liakhovetski wrote: > Add a V4L2 OF parser, implementing bindings, documented in > Documentation/devicetree/bindings/media/v4l2.txt. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > --- > drivers/media/v4l2-core/Makefile | 3 + > drivers/media/v4l2-core/v4l2-of.c | 190 +++++++++++++++++++++++++++++++++++++ > include/media/v4l2-of.h | 62 ++++++++++++ > 3 files changed, 255 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/v4l2-core/v4l2-of.c > create mode 100644 include/media/v4l2-of.h > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > index c2d61d4..00f64d6 100644 > --- a/drivers/media/v4l2-core/Makefile > +++ b/drivers/media/v4l2-core/Makefile > @@ -9,6 +9,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ > ifeq ($(CONFIG_COMPAT),y) > videodev-objs += v4l2-compat-ioctl32.o > endif > +ifeq ($(CONFIG_OF),y) > + videodev-objs += v4l2-of.o > +endif > > obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o > obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c > new file mode 100644 > index 0000000..f45d64b > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-of.c > @@ -0,0 +1,190 @@ > +/* > + * V4L2 OF binding parsing library > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski <g.liakhovetski@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. > + */ > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/types.h> > + > +#include <media/v4l2-of.h> > + > +/* > + * All properties are optional. If none are found, we don't set any flags. This > + * means, the port has a static configuration and no properties have to be > + * specified explicitly. > + * If any properties are found, that identify the bus as parallel, and > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise the > + * bus as serial CSI-2 and clock-noncontinuous isn't set, we set the > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. > + * The caller should hold a reference to "node." > + */ Since this is a library function, how about converting this description to kernel-doc ? > +void v4l2_of_parse_link(const struct device_node *node, > + struct v4l2_of_link *link) > +{ > + const struct device_node *port_node = of_get_parent(node); > + int size; > + unsigned int v; > + u32 data_lanes[ARRAY_SIZE(link->mipi_csi_2.data_lanes)]; > + bool data_lanes_present; > + > + memset(link, 0, sizeof(*link)); > + > + link->local_node = node; > + > + /* Doesn't matter, whether the below two calls succeed */ > + of_property_read_u32(port_node, "reg", &link->port); > + of_property_read_u32(node, "reg", &link->addr); > + > + if (!of_property_read_u32(node, "bus-width", &v)) > + link->parallel.bus_width = v; > + > + if (!of_property_read_u32(node, "data-shift", &v)) > + link->parallel.data_shift = v; > + > + if (!of_property_read_u32(node, "hsync-active", &v)) > + link->mbus_flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH : > + V4L2_MBUS_HSYNC_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "vsync-active", &v)) > + link->mbus_flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH : > + V4L2_MBUS_VSYNC_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "data-active", &v)) > + link->mbus_flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH : > + V4L2_MBUS_DATA_ACTIVE_LOW; > + > + if (!of_property_read_u32(node, "pclk-sample", &v)) > + link->mbus_flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING : > + V4L2_MBUS_PCLK_SAMPLE_FALLING; > + > + if (!of_property_read_u32(node, "field-even-active", &v)) > + link->mbus_flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH : > + V4L2_MBUS_FIELD_EVEN_LOW; > + > + if (of_get_property(node, "slave-mode", &size)) > + link->mbus_flags |= V4L2_MBUS_SLAVE; > + > + /* If any parallel-bus properties have been found, skip serial ones */ > + if (link->parallel.bus_width || link->parallel.data_shift || > + link->mbus_flags) { > + /* Default parallel bus-master */ > + if (!(link->mbus_flags & V4L2_MBUS_SLAVE)) > + link->mbus_flags |= V4L2_MBUS_MASTER; > + return; > + } > + > + if (!of_property_read_u32(node, "clock-lanes", &v)) > + link->mipi_csi_2.clock_lane = v; > + > + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > + ARRAY_SIZE(data_lanes))) { > + int i; > + for (i = 0; i < ARRAY_SIZE(data_lanes); i++) > + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > + data_lanes_present = true; > + } else { > + data_lanes_present = false; > + } > + > + if (of_get_property(node, "clock-noncontinuous", &size)) > + link->mbus_flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > + > + if ((link->mipi_csi_2.clock_lane || data_lanes_present) && > + !(link->mbus_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) > + /* Default CSI-2: continuous clock */ > + link->mbus_flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; > +} > +EXPORT_SYMBOL(v4l2_of_parse_link); > + > +/* > + * Return a refcounted next "link" DT node. Contrary to the common OF practice, > + * we do not drop the reference to previous, users have to do it themselves, > + * when they're done with the node. > + */ > +struct device_node *v4l2_of_get_next_link(const struct device_node *parent, > + struct device_node *previous) Looks good to me. Only a proper kernel-doc description seems to be missing. > +{ > + struct device_node *child, *port; > + > + if (!parent) > + return NULL; > + > + if (!previous) { > + /* > + * If this is the first call, we have to find a port within this > + * node > + */ > + for_each_child_of_node(parent, port) { > + if (!of_node_cmp(port->name, "port")) > + break; > + } > + if (port) { > + /* Found a port, get a link */ > + child = of_get_next_child(port, NULL); > + of_node_put(port); > + } else { > + child = NULL; > + } > + if (!child) > + pr_err("%s(): Invalid DT: %s has no link children!\n", > + __func__, parent->name); > + } else { > + port = of_get_parent(previous); > + if (!port) > + /* Hm, has someone given us the root node?... */ > + return NULL; > + > + /* Avoid dropping previous refcount to 0 */ > + of_node_get(previous); > + child = of_get_next_child(port, previous); > + if (child) { > + of_node_put(port); > + return child; > + } > + > + /* No more links under this port, try the next one */ > + do { > + port = of_get_next_child(parent, port); > + if (!port) > + return NULL; > + } while (of_node_cmp(port->name, "port")); > + > + /* Pick up the first link on this port */ > + child = of_get_next_child(port, NULL); > + of_node_put(port); > + } > + > + return child; > +} > +EXPORT_SYMBOL(v4l2_of_get_next_link); > + > +/* Return a refcounted DT node, owning the link, referenced by "remote" */ Since this returns parent node of of a port on the other end of the link, how about changing the name to v4l2_of_get_remote_link_parent() ? Also kernel-doc description would be useful, so one doesn't have to necessarily dig into the code to see what this function does exactly. > +struct device_node *v4l2_of_get_remote(const struct device_node *node) > +{ > + struct device_node *remote, *tmp; > + > + /* Get remote link DT node */ > + remote = of_parse_phandle(node, "remote", 0); > + if (!remote) > + return NULL; > + > + /* remote port */ > + tmp = of_get_parent(remote); > + of_node_put(remote); > + if (!tmp) > + return NULL; > + > + /* remote DT node */ > + remote = of_get_parent(tmp); > + of_node_put(tmp); > + > + return remote; > +} > +EXPORT_SYMBOL(v4l2_of_get_remote); -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans On Fri, 5 Oct 2012, Hans Verkuil wrote: [snip] > I think the soc_camera patches should be left out for now. I suspect that > by adding core support for async i2c handling first, Ok, let's think, what this meacs - async I2C in media / V4L2 core. The main reason for our probing order problem is the master clock, typically supplied from the camera bridge to I2C subdevices, which we only want to start when necessary, i.e. when accessing the subdevice. And the subdevice driver needs that clock running during its .probe() to be able to access and verify or configure the hardware. Our current solution is to not register I2C subdevices from the platform data, as is usual for all I2C devices, but from the bridge driver and only after it has switched on the master clock. After the subdevice driver has completed its probing we switch the clock back off until the subdevice has to be activated, e.g. for video capture. Also important - when we want to unregister the bridge driver we just also unregister the I2C device. Now, to reverse the whole thing and to allow I2C devices be registered as usual - via platform data or OF, first of all we have to teach I2C subdevice drivers to recognise the "too early" situation and request deferred probing in such a case. Then it will be reprobed after each new successful probe or unregister on the system. After the bridge driver has successfully probed the subdevice driver will be re-probed and at that time it should succeed. Now, there is a problem here too: who should switch on and off the master clock? If we do it from the bridge driver, we could install an I2C bus-notifier, _before_ the subdevice driver is probed, i.e. upon the BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER event to switch the clock back off. BUT - if the subdevice fails probing? How do we find out about that and turn the clock back off? There is no notification event for that... Possible solutions: 1. timer - ugly and unreliable. 2. add a "probing failed" notifier event to the device core - would this be accepted? 3. let the subdevice turn the master clock on and off for the duration of probing. My vote goes for (3). Ideally this should be done using the generic clock framework. But can we really expect all drivers and platforms to switch to it quickly enough? If not, we need a V4L2-specific callback from subdevice drivers to bridge drivers to turn the clock on and off. That's what I've done "temporarily" in this patch series for soc-camera. Suppose we decide to do the same for V4L2 centrally - add call-backs. Then we can think what else we need to add to V4L2 to support asynchronous subdevice driver probing. 1. We'll have to create these V4L2 clock start and stop functions, that, supplied (in case of I2C) with client address and adapter number will find the correct v4l2_device instance and call its callbacks. 2. The I2C notifier. I'm not sure, whether this one should be common. Of common tasks we have to refcount the I2C adapter and register the subdevice. Then we'd have to call the bridge driver's callback. Is it worth it doing this centrally or rather allow individual drivers to do that themselves? Also, ideally OF-compatible (I2C) drivers should run with no platform data, but soc-camera is using I2C device platform data intensively. To avoid modifying the soc-camera core and all drivers, I also trigger on the BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically created platform data to the I2C device. Would we also want to do this for all V4L2 bridge drivers? We could call this a "prepare" callback or something similar... 3. Bridge driver unregistering. Here we have to put the subdevice driver back into the deferred-probe state... Ugliness alert: I'm doing this by unregistering and re-registering the I2C device... For that I also have to create a copy of devices I2C board-info data. Lovely, ain't it? This I'd be happy to move to the V4L2 core;-) Thanks Guennadi > the soc_camera patches > will become a lot easier to understand. > > Regards, > > Hans > > > > > Thanks > > Guennadi > > --- > > Guennadi Liakhovetski, Ph.D. > > Freelance Open-Source Software Developer > > http://www.open-technology.de/ > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > Hi Hans > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > [snip] > > > I think the soc_camera patches should be left out for now. I suspect that > > by adding core support for async i2c handling first, > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > The main reason for our probing order problem is the master clock, > typically supplied from the camera bridge to I2C subdevices, which we only > want to start when necessary, i.e. when accessing the subdevice. And the > subdevice driver needs that clock running during its .probe() to be able > to access and verify or configure the hardware. Our current solution is to > not register I2C subdevices from the platform data, as is usual for all > I2C devices, but from the bridge driver and only after it has switched on > the master clock. After the subdevice driver has completed its probing we > switch the clock back off until the subdevice has to be activated, e.g. > for video capture. > > Also important - when we want to unregister the bridge driver we just also > unregister the I2C device. > > Now, to reverse the whole thing and to allow I2C devices be registered as > usual - via platform data or OF, first of all we have to teach I2C > subdevice drivers to recognise the "too early" situation and request > deferred probing in such a case. Then it will be reprobed after each new > successful probe or unregister on the system. After the bridge driver has > successfully probed the subdevice driver will be re-probed and at that > time it should succeed. Now, there is a problem here too: who should > switch on and off the master clock? > > If we do it from the bridge driver, we could install an I2C bus-notifier, > _before_ the subdevice driver is probed, i.e. upon the > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > event to switch the clock back off. BUT - if the subdevice fails probing? > How do we find out about that and turn the clock back off? There is no > notification event for that... Possible solutions: > > 1. timer - ugly and unreliable. > 2. add a "probing failed" notifier event to the device core - would this > be accepted? > 3. let the subdevice turn the master clock on and off for the duration of > probing. > > My vote goes for (3). Ideally this should be done using the generic clock > framework. But can we really expect all drivers and platforms to switch to > it quickly enough? If not, we need a V4L2-specific callback from subdevice > drivers to bridge drivers to turn the clock on and off. That's what I've > done "temporarily" in this patch series for soc-camera. > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > we can think what else we need to add to V4L2 to support asynchronous > subdevice driver probing. I wonder, don't we have the necessary code already? V4L2 subdev drivers can have internal_ops with register/unregister ops. These are called by v4l2_device_register_subdev. This happens during the bridge driver's probe. Suppose the subdev's probe does not actually access the i2c device, but instead defers that to the register callback? The bridge driver will turn on the clock before calling v4l2_device_register_subdev to ensure that the register callback can access the i2c registers. The register callback will do any initialization and can return an error. In case of an error the i2c client is automatically unregistered as well. In addition, during the register op the subdev driver can call into the bridge driver since it knows the v4l2_device struct. This has also the advantage that subdev drivers can change to this model gradually. Only drivers that need master clocks, etc. need to move any probe code that actually accesses hardware to the register op. Others can remain as. Nor should this change behavior of existing drivers as this happens all in the V4L2 core. The bridge driver may still have to wait until all i2c drivers are loaded, though. But that can definitely be handled centrally (i.e.: 'I need these drivers, wait until all are loaded'). > 1. We'll have to create these V4L2 clock start and stop functions, that, > supplied (in case of I2C) with client address and adapter number will find > the correct v4l2_device instance and call its callbacks. > > 2. The I2C notifier. I'm not sure, whether this one should be common. Of > common tasks we have to refcount the I2C adapter and register the > subdevice. Then we'd have to call the bridge driver's callback. Is it > worth it doing this centrally or rather allow individual drivers to do > that themselves? > > Also, ideally OF-compatible (I2C) drivers should run with no platform > data, but soc-camera is using I2C device platform data intensively. To > avoid modifying the soc-camera core and all drivers, I also trigger on the > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > created platform data to the I2C device. Would we also want to do this for > all V4L2 bridge drivers? We could call this a "prepare" callback or > something similar... Well, subdev drivers should either parse the OF data, or use the platform_data. The way soc_camera uses platform_data is one reason why it is so hard to reuse subdevs for non-soc_camera drivers. All the callbacks in soc_camera_link should be replaced by calls to the v4l2_device notify() callback. After that we can see what is needed to drop struct soc_camera_link altogether as platform_data. > 3. Bridge driver unregistering. Here we have to put the subdevice driver > back into the deferred-probe state... Ugliness alert: I'm doing this by > unregistering and re-registering the I2C device... For that I also have to > create a copy of devices I2C board-info data. Lovely, ain't it? This I'd > be happy to move to the V4L2 core;-) By just using the unregister ops this should be greatly simplified as well. Unless I am missing something, which is perfectly possible :-) Regards, Hans > > Thanks > Guennadi > > > the soc_camera patches > > will become a lot easier to understand. > > > > Regards, > > > > Hans > > > > > > > > Thanks > > > Guennadi > > > --- > > > Guennadi Liakhovetski, Ph.D. > > > Freelance Open-Source Software Developer > > > http://www.open-technology.de/ > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Oct 2012, Hans Verkuil wrote: > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > Hi Hans > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > [snip] > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > by adding core support for async i2c handling first, > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > The main reason for our probing order problem is the master clock, > > typically supplied from the camera bridge to I2C subdevices, which we only > > want to start when necessary, i.e. when accessing the subdevice. And the > > subdevice driver needs that clock running during its .probe() to be able > > to access and verify or configure the hardware. Our current solution is to > > not register I2C subdevices from the platform data, as is usual for all > > I2C devices, but from the bridge driver and only after it has switched on > > the master clock. After the subdevice driver has completed its probing we > > switch the clock back off until the subdevice has to be activated, e.g. > > for video capture. > > > > Also important - when we want to unregister the bridge driver we just also > > unregister the I2C device. > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > usual - via platform data or OF, first of all we have to teach I2C > > subdevice drivers to recognise the "too early" situation and request > > deferred probing in such a case. Then it will be reprobed after each new > > successful probe or unregister on the system. After the bridge driver has > > successfully probed the subdevice driver will be re-probed and at that > > time it should succeed. Now, there is a problem here too: who should > > switch on and off the master clock? > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > _before_ the subdevice driver is probed, i.e. upon the > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > event to switch the clock back off. BUT - if the subdevice fails probing? > > How do we find out about that and turn the clock back off? There is no > > notification event for that... Possible solutions: > > > > 1. timer - ugly and unreliable. > > 2. add a "probing failed" notifier event to the device core - would this > > be accepted? > > 3. let the subdevice turn the master clock on and off for the duration of > > probing. > > > > My vote goes for (3). Ideally this should be done using the generic clock > > framework. But can we really expect all drivers and platforms to switch to > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > drivers to bridge drivers to turn the clock on and off. That's what I've > > done "temporarily" in this patch series for soc-camera. > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > we can think what else we need to add to V4L2 to support asynchronous > > subdevice driver probing. > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > have internal_ops with register/unregister ops. These are called by > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > Suppose the subdev's probe does not actually access the i2c device, but > instead defers that to the register callback? The bridge driver will turn on > the clock before calling v4l2_device_register_subdev to ensure that the > register callback can access the i2c registers. The register callback will > do any initialization and can return an error. In case of an error the i2c > client is automatically unregistered as well. Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed several times before and always what I didn't like in this is, that I2C device probe() in this case succeeds without even trying to access the hardware. And think about DT. In this case we don't instantiate the I2C device, OF code does it for us. What do you do then? If you let probe() succeed, then you will have to somehow remember the subdevice to later match it against bridges... > In addition, during the register op the subdev driver can call into the > bridge driver since it knows the v4l2_device struct. > > This has also the advantage that subdev drivers can change to this model > gradually. Only drivers that need master clocks, etc. need to move any probe > code that actually accesses hardware to the register op. Others can remain > as. Nor should this change behavior of existing drivers as this happens > all in the V4L2 core. > > The bridge driver may still have to wait until all i2c drivers are loaded, > though. But that can definitely be handled centrally (i.e.: 'I need these > drivers, wait until all are loaded'). > > > 1. We'll have to create these V4L2 clock start and stop functions, that, > > supplied (in case of I2C) with client address and adapter number will find > > the correct v4l2_device instance and call its callbacks. > > > > 2. The I2C notifier. I'm not sure, whether this one should be common. Of > > common tasks we have to refcount the I2C adapter and register the > > subdevice. Then we'd have to call the bridge driver's callback. Is it > > worth it doing this centrally or rather allow individual drivers to do > > that themselves? > > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > data, but soc-camera is using I2C device platform data intensively. To > > avoid modifying the soc-camera core and all drivers, I also trigger on the > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > created platform data to the I2C device. Would we also want to do this for > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > something similar... > > Well, subdev drivers should either parse the OF data, or use the platform_data. > The way soc_camera uses platform_data is one reason why it is so hard to > reuse subdevs for non-soc_camera drivers. All the callbacks in soc_camera_link > should be replaced by calls to the v4l2_device notify() callback. After that we > can see what is needed to drop struct soc_camera_link altogether as platform_data. They don't have to be, they are not (or should not be) called by subdevices. > > 3. Bridge driver unregistering. Here we have to put the subdevice driver > > back into the deferred-probe state... Ugliness alert: I'm doing this by > > unregistering and re-registering the I2C device... For that I also have to > > create a copy of devices I2C board-info data. Lovely, ain't it? This I'd > > be happy to move to the V4L2 core;-) > > By just using the unregister ops this should be greatly simplified as well. Sorry, which unregister ops do you mean? internal_ops->unregistered()? Yes, but only if we somehow go your way and use dummy probe() methods... Thanks Guennadi > Unless I am missing something, which is perfectly possible :-) > > Regards, > > Hans > > > Thanks > > Guennadi > > > > > the soc_camera patches > > > will become a lot easier to understand. --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > Hi Hans > > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > [snip] > > > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > > by adding core support for async i2c handling first, > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > The main reason for our probing order problem is the master clock, > > > typically supplied from the camera bridge to I2C subdevices, which we only > > > want to start when necessary, i.e. when accessing the subdevice. And the > > > subdevice driver needs that clock running during its .probe() to be able > > > to access and verify or configure the hardware. Our current solution is to > > > not register I2C subdevices from the platform data, as is usual for all > > > I2C devices, but from the bridge driver and only after it has switched on > > > the master clock. After the subdevice driver has completed its probing we > > > switch the clock back off until the subdevice has to be activated, e.g. > > > for video capture. > > > > > > Also important - when we want to unregister the bridge driver we just also > > > unregister the I2C device. > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > > usual - via platform data or OF, first of all we have to teach I2C > > > subdevice drivers to recognise the "too early" situation and request > > > deferred probing in such a case. Then it will be reprobed after each new > > > successful probe or unregister on the system. After the bridge driver has > > > successfully probed the subdevice driver will be re-probed and at that > > > time it should succeed. Now, there is a problem here too: who should > > > switch on and off the master clock? > > > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > > _before_ the subdevice driver is probed, i.e. upon the > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > > event to switch the clock back off. BUT - if the subdevice fails probing? > > > How do we find out about that and turn the clock back off? There is no > > > notification event for that... Possible solutions: > > > > > > 1. timer - ugly and unreliable. > > > 2. add a "probing failed" notifier event to the device core - would this > > > be accepted? > > > 3. let the subdevice turn the master clock on and off for the duration of > > > probing. > > > > > > My vote goes for (3). Ideally this should be done using the generic clock > > > framework. But can we really expect all drivers and platforms to switch to > > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > > drivers to bridge drivers to turn the clock on and off. That's what I've > > > done "temporarily" in this patch series for soc-camera. > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > > we can think what else we need to add to V4L2 to support asynchronous > > > subdevice driver probing. > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > > have internal_ops with register/unregister ops. These are called by > > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > > > Suppose the subdev's probe does not actually access the i2c device, but > > instead defers that to the register callback? The bridge driver will turn on > > the clock before calling v4l2_device_register_subdev to ensure that the > > register callback can access the i2c registers. The register callback will > > do any initialization and can return an error. In case of an error the i2c > > client is automatically unregistered as well. > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > several times before and always what I didn't like in this is, that I2C > device probe() in this case succeeds without even trying to access the > hardware. And think about DT. In this case we don't instantiate the I2C > device, OF code does it for us. What do you do then? If you let probe() > succeed, then you will have to somehow remember the subdevice to later > match it against bridges... Yes, but you need that information anyway. The bridge still needs to call v4l2_device_register_subdev so it needs to know which subdevs are loaded. And can't it get that from DT as well? In my view you cannot do a proper initialization unless you have both the bridge driver and all subdev drivers loaded and instantiated. They need one another. So I am perfectly fine with letting the probe function do next to nothing and postponing that until register() is called. I2C and actual probing to check if it's the right device is a bad idea in general since you have no idea what a hardware access to an unknown i2c device will do. There are still some corner cases where that is needed, but I do not think that that is an issue here. It would simplify things a lot IMHO. Also note that the register() op will work with any device, not just i2c. That may be a useful property as well. > > In addition, during the register op the subdev driver can call into the > > bridge driver since it knows the v4l2_device struct. > > > > This has also the advantage that subdev drivers can change to this model > > gradually. Only drivers that need master clocks, etc. need to move any probe > > code that actually accesses hardware to the register op. Others can remain > > as. Nor should this change behavior of existing drivers as this happens > > all in the V4L2 core. > > > > The bridge driver may still have to wait until all i2c drivers are loaded, > > though. But that can definitely be handled centrally (i.e.: 'I need these > > drivers, wait until all are loaded'). > > > > > 1. We'll have to create these V4L2 clock start and stop functions, that, > > > supplied (in case of I2C) with client address and adapter number will find > > > the correct v4l2_device instance and call its callbacks. > > > > > > 2. The I2C notifier. I'm not sure, whether this one should be common. Of > > > common tasks we have to refcount the I2C adapter and register the > > > subdevice. Then we'd have to call the bridge driver's callback. Is it > > > worth it doing this centrally or rather allow individual drivers to do > > > that themselves? > > > > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > > data, but soc-camera is using I2C device platform data intensively. To > > > avoid modifying the soc-camera core and all drivers, I also trigger on the > > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > > created platform data to the I2C device. Would we also want to do this for > > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > > something similar... > > > > Well, subdev drivers should either parse the OF data, or use the platform_data. > > The way soc_camera uses platform_data is one reason why it is so hard to > > reuse subdevs for non-soc_camera drivers. All the callbacks in soc_camera_link > > should be replaced by calls to the v4l2_device notify() callback. After that we > > can see what is needed to drop struct soc_camera_link altogether as platform_data. > > They don't have to be, they are not (or should not be) called by > subdevices. Then why are those callbacks in a struct that subdevs can access? I always have a hard time with soc_camera figuring out who is using what when :-( > > > 3. Bridge driver unregistering. Here we have to put the subdevice driver > > > back into the deferred-probe state... Ugliness alert: I'm doing this by > > > unregistering and re-registering the I2C device... For that I also have to > > > create a copy of devices I2C board-info data. Lovely, ain't it? This I'd > > > be happy to move to the V4L2 core;-) > > > > By just using the unregister ops this should be greatly simplified as well. > > Sorry, which unregister ops do you mean? internal_ops->unregistered()? Yes. > Yes, but only if we somehow go your way and use dummy probe() methods... Of course. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Oct 2012, Hans Verkuil wrote: > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > Hi Hans > > > > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > [snip] > > > > > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > > > by adding core support for async i2c handling first, > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > The main reason for our probing order problem is the master clock, > > > > typically supplied from the camera bridge to I2C subdevices, which we only > > > > want to start when necessary, i.e. when accessing the subdevice. And the > > > > subdevice driver needs that clock running during its .probe() to be able > > > > to access and verify or configure the hardware. Our current solution is to > > > > not register I2C subdevices from the platform data, as is usual for all > > > > I2C devices, but from the bridge driver and only after it has switched on > > > > the master clock. After the subdevice driver has completed its probing we > > > > switch the clock back off until the subdevice has to be activated, e.g. > > > > for video capture. > > > > > > > > Also important - when we want to unregister the bridge driver we just also > > > > unregister the I2C device. > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > > > usual - via platform data or OF, first of all we have to teach I2C > > > > subdevice drivers to recognise the "too early" situation and request > > > > deferred probing in such a case. Then it will be reprobed after each new > > > > successful probe or unregister on the system. After the bridge driver has > > > > successfully probed the subdevice driver will be re-probed and at that > > > > time it should succeed. Now, there is a problem here too: who should > > > > switch on and off the master clock? > > > > > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > > > _before_ the subdevice driver is probed, i.e. upon the > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > > > event to switch the clock back off. BUT - if the subdevice fails probing? > > > > How do we find out about that and turn the clock back off? There is no > > > > notification event for that... Possible solutions: > > > > > > > > 1. timer - ugly and unreliable. > > > > 2. add a "probing failed" notifier event to the device core - would this > > > > be accepted? > > > > 3. let the subdevice turn the master clock on and off for the duration of > > > > probing. > > > > > > > > My vote goes for (3). Ideally this should be done using the generic clock > > > > framework. But can we really expect all drivers and platforms to switch to > > > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > > > drivers to bridge drivers to turn the clock on and off. That's what I've > > > > done "temporarily" in this patch series for soc-camera. > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > > > we can think what else we need to add to V4L2 to support asynchronous > > > > subdevice driver probing. > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > > > have internal_ops with register/unregister ops. These are called by > > > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > > > > > Suppose the subdev's probe does not actually access the i2c device, but > > > instead defers that to the register callback? The bridge driver will turn on > > > the clock before calling v4l2_device_register_subdev to ensure that the > > > register callback can access the i2c registers. The register callback will > > > do any initialization and can return an error. In case of an error the i2c > > > client is automatically unregistered as well. > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > several times before and always what I didn't like in this is, that I2C > > device probe() in this case succeeds without even trying to access the > > hardware. And think about DT. In this case we don't instantiate the I2C > > device, OF code does it for us. What do you do then? If you let probe() > > succeed, then you will have to somehow remember the subdevice to later > > match it against bridges... > > Yes, but you need that information anyway. The bridge still needs to call > v4l2_device_register_subdev so it needs to know which subdevs are loaded. But how do you get the subdev pointer? With the notifier I get it from i2c_get_clientdata(client) and what do you do without it? How do you get to the client? > And can't it get that from DT as well? No, I don't think there is a way to get a device pointer from a DT node. > In my view you cannot do a proper initialization unless you have both the > bridge driver and all subdev drivers loaded and instantiated. They need one > another. So I am perfectly fine with letting the probe function do next to > nothing and postponing that until register() is called. I2C and actual probing > to check if it's the right device is a bad idea in general since you have no > idea what a hardware access to an unknown i2c device will do. There are still > some corner cases where that is needed, but I do not think that that is an > issue here. > > It would simplify things a lot IMHO. Also note that the register() op will > work with any device, not just i2c. That may be a useful property as well. And what if the subdevice device is not yet instantiated by OF by the time your bridge driver probes? Thanks Guennadi > > > In addition, during the register op the subdev driver can call into the > > > bridge driver since it knows the v4l2_device struct. > > > > > > This has also the advantage that subdev drivers can change to this model > > > gradually. Only drivers that need master clocks, etc. need to move any probe > > > code that actually accesses hardware to the register op. Others can remain > > > as. Nor should this change behavior of existing drivers as this happens > > > all in the V4L2 core. > > > > > > The bridge driver may still have to wait until all i2c drivers are loaded, > > > though. But that can definitely be handled centrally (i.e.: 'I need these > > > drivers, wait until all are loaded'). > > > > > > > 1. We'll have to create these V4L2 clock start and stop functions, that, > > > > supplied (in case of I2C) with client address and adapter number will find > > > > the correct v4l2_device instance and call its callbacks. > > > > > > > > 2. The I2C notifier. I'm not sure, whether this one should be common. Of > > > > common tasks we have to refcount the I2C adapter and register the > > > > subdevice. Then we'd have to call the bridge driver's callback. Is it > > > > worth it doing this centrally or rather allow individual drivers to do > > > > that themselves? > > > > > > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > > > data, but soc-camera is using I2C device platform data intensively. To > > > > avoid modifying the soc-camera core and all drivers, I also trigger on the > > > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > > > created platform data to the I2C device. Would we also want to do this for > > > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > > > something similar... > > > > > > Well, subdev drivers should either parse the OF data, or use the platform_data. > > > The way soc_camera uses platform_data is one reason why it is so hard to > > > reuse subdevs for non-soc_camera drivers. All the callbacks in soc_camera_link > > > should be replaced by calls to the v4l2_device notify() callback. After that we > > > can see what is needed to drop struct soc_camera_link altogether as platform_data. > > > > They don't have to be, they are not (or should not be) called by > > subdevices. > > Then why are those callbacks in a struct that subdevs can access? I always > have a hard time with soc_camera figuring out who is using what when :-( > > > > > 3. Bridge driver unregistering. Here we have to put the subdevice driver > > > > back into the deferred-probe state... Ugliness alert: I'm doing this by > > > > unregistering and re-registering the I2C device... For that I also have to > > > > create a copy of devices I2C board-info data. Lovely, ain't it? This I'd > > > > be happy to move to the V4L2 core;-) > > > > > > By just using the unregister ops this should be greatly simplified as well. > > > > Sorry, which unregister ops do you mean? internal_ops->unregistered()? > > Yes. > > > Yes, but only if we somehow go your way and use dummy probe() methods... > > Of course. > > Regards, > > Hans > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon October 8 2012 17:15:53 Guennadi Liakhovetski wrote: > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > > Hi Hans > > > > > > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > > > [snip] > > > > > > > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > > > > by adding core support for async i2c handling first, > > > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > > > The main reason for our probing order problem is the master clock, > > > > > typically supplied from the camera bridge to I2C subdevices, which we only > > > > > want to start when necessary, i.e. when accessing the subdevice. And the > > > > > subdevice driver needs that clock running during its .probe() to be able > > > > > to access and verify or configure the hardware. Our current solution is to > > > > > not register I2C subdevices from the platform data, as is usual for all > > > > > I2C devices, but from the bridge driver and only after it has switched on > > > > > the master clock. After the subdevice driver has completed its probing we > > > > > switch the clock back off until the subdevice has to be activated, e.g. > > > > > for video capture. > > > > > > > > > > Also important - when we want to unregister the bridge driver we just also > > > > > unregister the I2C device. > > > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > > > > usual - via platform data or OF, first of all we have to teach I2C > > > > > subdevice drivers to recognise the "too early" situation and request > > > > > deferred probing in such a case. Then it will be reprobed after each new > > > > > successful probe or unregister on the system. After the bridge driver has > > > > > successfully probed the subdevice driver will be re-probed and at that > > > > > time it should succeed. Now, there is a problem here too: who should > > > > > switch on and off the master clock? > > > > > > > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > > > > _before_ the subdevice driver is probed, i.e. upon the > > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > > > > event to switch the clock back off. BUT - if the subdevice fails probing? > > > > > How do we find out about that and turn the clock back off? There is no > > > > > notification event for that... Possible solutions: > > > > > > > > > > 1. timer - ugly and unreliable. > > > > > 2. add a "probing failed" notifier event to the device core - would this > > > > > be accepted? > > > > > 3. let the subdevice turn the master clock on and off for the duration of > > > > > probing. > > > > > > > > > > My vote goes for (3). Ideally this should be done using the generic clock > > > > > framework. But can we really expect all drivers and platforms to switch to > > > > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > > > > drivers to bridge drivers to turn the clock on and off. That's what I've > > > > > done "temporarily" in this patch series for soc-camera. > > > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > > > > we can think what else we need to add to V4L2 to support asynchronous > > > > > subdevice driver probing. > > > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > > > > have internal_ops with register/unregister ops. These are called by > > > > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > > > > > > > Suppose the subdev's probe does not actually access the i2c device, but > > > > instead defers that to the register callback? The bridge driver will turn on > > > > the clock before calling v4l2_device_register_subdev to ensure that the > > > > register callback can access the i2c registers. The register callback will > > > > do any initialization and can return an error. In case of an error the i2c > > > > client is automatically unregistered as well. > > > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > > several times before and always what I didn't like in this is, that I2C > > > device probe() in this case succeeds without even trying to access the > > > hardware. And think about DT. In this case we don't instantiate the I2C > > > device, OF code does it for us. What do you do then? If you let probe() > > > succeed, then you will have to somehow remember the subdevice to later > > > match it against bridges... > > > > Yes, but you need that information anyway. The bridge still needs to call > > v4l2_device_register_subdev so it needs to know which subdevs are loaded. > > But how do you get the subdev pointer? With the notifier I get it from > i2c_get_clientdata(client) and what do you do without it? How do you get > to the client? > > > And can't it get that from DT as well? > > No, I don't think there is a way to get a device pointer from a DT node. Not a device pointer, but the i2c bus and i2c address. With that information you can get the i2c_client, and with that you can get the subdev pointer. If there is no way to get that information from the proposed V4L2 DT, then it needs to be modified since a bridge driver really needs to know which subdevs it has to register with the v4l2_device struct. That information is also board specific so it should be part of the DT. > > > In my view you cannot do a proper initialization unless you have both the > > bridge driver and all subdev drivers loaded and instantiated. They need one > > another. So I am perfectly fine with letting the probe function do next to > > nothing and postponing that until register() is called. I2C and actual probing > > to check if it's the right device is a bad idea in general since you have no > > idea what a hardware access to an unknown i2c device will do. There are still > > some corner cases where that is needed, but I do not think that that is an > > issue here. > > > > It would simplify things a lot IMHO. Also note that the register() op will > > work with any device, not just i2c. That may be a useful property as well. > > And what if the subdevice device is not yet instantiated by OF by the time > your bridge driver probes? That is where you still need to have a bus notifier mechanism. You have to be able to wait until all dependent drivers are loaded/instantiated, or alternatively you have to be able to load them explicitly. But this should be relatively easy to implement in a generic manner. I still think this sucks (excuse my language), but I see no way around it as long as there is no explicit probe order one can rely on. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Oct 2012, Hans Verkuil wrote: > On Mon October 8 2012 17:15:53 Guennadi Liakhovetski wrote: > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > > > > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > > > Hi Hans > > > > > > > > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > > > > > [snip] > > > > > > > > > > > > > I think the soc_camera patches should be left out for now. I suspect that > > > > > > > by adding core support for async i2c handling first, > > > > > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > > > > > The main reason for our probing order problem is the master clock, > > > > > > typically supplied from the camera bridge to I2C subdevices, which we only > > > > > > want to start when necessary, i.e. when accessing the subdevice. And the > > > > > > subdevice driver needs that clock running during its .probe() to be able > > > > > > to access and verify or configure the hardware. Our current solution is to > > > > > > not register I2C subdevices from the platform data, as is usual for all > > > > > > I2C devices, but from the bridge driver and only after it has switched on > > > > > > the master clock. After the subdevice driver has completed its probing we > > > > > > switch the clock back off until the subdevice has to be activated, e.g. > > > > > > for video capture. > > > > > > > > > > > > Also important - when we want to unregister the bridge driver we just also > > > > > > unregister the I2C device. > > > > > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered as > > > > > > usual - via platform data or OF, first of all we have to teach I2C > > > > > > subdevice drivers to recognise the "too early" situation and request > > > > > > deferred probing in such a case. Then it will be reprobed after each new > > > > > > successful probe or unregister on the system. After the bridge driver has > > > > > > successfully probed the subdevice driver will be re-probed and at that > > > > > > time it should succeed. Now, there is a problem here too: who should > > > > > > switch on and off the master clock? > > > > > > > > > > > > If we do it from the bridge driver, we could install an I2C bus-notifier, > > > > > > _before_ the subdevice driver is probed, i.e. upon the > > > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > > > > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > > > > > > event to switch the clock back off. BUT - if the subdevice fails probing? > > > > > > How do we find out about that and turn the clock back off? There is no > > > > > > notification event for that... Possible solutions: > > > > > > > > > > > > 1. timer - ugly and unreliable. > > > > > > 2. add a "probing failed" notifier event to the device core - would this > > > > > > be accepted? > > > > > > 3. let the subdevice turn the master clock on and off for the duration of > > > > > > probing. > > > > > > > > > > > > My vote goes for (3). Ideally this should be done using the generic clock > > > > > > framework. But can we really expect all drivers and platforms to switch to > > > > > > it quickly enough? If not, we need a V4L2-specific callback from subdevice > > > > > > drivers to bridge drivers to turn the clock on and off. That's what I've > > > > > > done "temporarily" in this patch series for soc-camera. > > > > > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > > > > > > we can think what else we need to add to V4L2 to support asynchronous > > > > > > subdevice driver probing. > > > > > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers can > > > > > have internal_ops with register/unregister ops. These are called by > > > > > v4l2_device_register_subdev. This happens during the bridge driver's probe. > > > > > > > > > > Suppose the subdev's probe does not actually access the i2c device, but > > > > > instead defers that to the register callback? The bridge driver will turn on > > > > > the clock before calling v4l2_device_register_subdev to ensure that the > > > > > register callback can access the i2c registers. The register callback will > > > > > do any initialization and can return an error. In case of an error the i2c > > > > > client is automatically unregistered as well. > > > > > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > > > several times before and always what I didn't like in this is, that I2C > > > > device probe() in this case succeeds without even trying to access the > > > > hardware. And think about DT. In this case we don't instantiate the I2C > > > > device, OF code does it for us. What do you do then? If you let probe() > > > > succeed, then you will have to somehow remember the subdevice to later > > > > match it against bridges... > > > > > > Yes, but you need that information anyway. The bridge still needs to call > > > v4l2_device_register_subdev so it needs to know which subdevs are loaded. > > > > But how do you get the subdev pointer? With the notifier I get it from > > i2c_get_clientdata(client) and what do you do without it? How do you get > > to the client? > > > > > And can't it get that from DT as well? > > > > No, I don't think there is a way to get a device pointer from a DT node. > > Not a device pointer, but the i2c bus and i2c address. With that information > you can get the i2c_client, and with that you can get the subdev pointer. How? How can you get a client pointer from adapter and i2c device address? I haven't found anything suitable in i2c.h. > If there is no way to get that information from the proposed V4L2 DT, then > it needs to be modified since a bridge driver really needs to know which > subdevs it has to register with the v4l2_device struct. That information is > also board specific so it should be part of the DT. > > > > > > In my view you cannot do a proper initialization unless you have both the > > > bridge driver and all subdev drivers loaded and instantiated. They need one > > > another. So I am perfectly fine with letting the probe function do next to > > > nothing and postponing that until register() is called. I2C and actual probing > > > to check if it's the right device is a bad idea in general since you have no > > > idea what a hardware access to an unknown i2c device will do. There are still > > > some corner cases where that is needed, but I do not think that that is an > > > issue here. > > > > > > It would simplify things a lot IMHO. Also note that the register() op will > > > work with any device, not just i2c. That may be a useful property as well. > > > > And what if the subdevice device is not yet instantiated by OF by the time > > your bridge driver probes? > > That is where you still need to have a bus notifier mechanism. You have to be > able to wait until all dependent drivers are loaded/instantiated, No, drivers are not the problem, as you say, you can load them. Devices are the problem. You don't know when they will be registered. > or > alternatively you have to be able to load them explicitly. But this should > be relatively easy to implement in a generic manner. So, if you need notifiers anyway, why not use them in all cases instead of mixing multiple methods? Thanks Guennadi > I still think this sucks (excuse my language), but I see no way around it as > long as there is no explicit probe order one can rely on. > > Regards, > > Hans > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Oct 2012, Guennadi Liakhovetski wrote: > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 17:15:53 Guennadi Liakhovetski wrote: [snip] > > > No, I don't think there is a way to get a device pointer from a DT node. > > > > Not a device pointer, but the i2c bus and i2c address. With that information > > you can get the i2c_client, and with that you can get the subdev pointer. > > How? How can you get a client pointer from adapter and i2c device address? > I haven't found anything suitable in i2c.h. Ok, you could use of_find_i2c_device_by_node(), but the second argument remains - if you need notifiers in one case, I don't think it makes sense to implement multiple methods. Thanks Guennadi > > If there is no way to get that information from the proposed V4L2 DT, then > > it needs to be modified since a bridge driver really needs to know which > > subdevs it has to register with the v4l2_device struct. That information is > > also board specific so it should be part of the DT. > > > > > > > > > In my view you cannot do a proper initialization unless you have both the > > > > bridge driver and all subdev drivers loaded and instantiated. They need one > > > > another. So I am perfectly fine with letting the probe function do next to > > > > nothing and postponing that until register() is called. I2C and actual probing > > > > to check if it's the right device is a bad idea in general since you have no > > > > idea what a hardware access to an unknown i2c device will do. There are still > > > > some corner cases where that is needed, but I do not think that that is an > > > > issue here. > > > > > > > > It would simplify things a lot IMHO. Also note that the register() op will > > > > work with any device, not just i2c. That may be a useful property as well. > > > > > > And what if the subdevice device is not yet instantiated by OF by the time > > > your bridge driver probes? > > > > That is where you still need to have a bus notifier mechanism. You have to be > > able to wait until all dependent drivers are loaded/instantiated, > > No, drivers are not the problem, as you say, you can load them. Devices > are the problem. You don't know when they will be registered. > > > or > > alternatively you have to be able to load them explicitly. But this should > > be relatively easy to implement in a generic manner. > > So, if you need notifiers anyway, why not use them in all cases instead of > mixing multiple methods? > > Thanks > Guennadi > > > I still think this sucks (excuse my language), but I see no way around it as > > long as there is no explicit probe order one can rely on. > > > > Regards, > > > > Hans > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 05 October 2012 12:58:21 Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > On Tue October 2 2012 12:13:20 Sylwester Nawrocki wrote: > > > On 10/02/2012 11:49 AM, Guennadi Liakhovetski wrote: > > > >>> + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, > > > >>> + ARRAY_SIZE(data_lanes))) { > > > >>> + int i; > > > >>> + for (i = 0; i< ARRAY_SIZE(data_lanes); i++) > > > >>> + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > > >> > > > >> It doesn't look like what we aimed for. The data-lanes array is > > > >> supposed to be of variable length, thus I don't think it can be > > > >> parsed like that. Or am I missing something ? I think we need more > > > >> something like below > > > > > > > >> (based on of_property_read_u32_array(), not tested): > > > > Ok, you're right, that my version only was suitable for fixed-size > > > > arrays, which wasn't our goal. But I don't think we should go down to > > > > manually parsing property data. How about (tested;-)) > > > > > > > > data = of_find_property(node, "data-lanes", NULL); > > > > if (data) { > > > > > > > > int i = 0; > > > > const __be32 *lane = NULL; > > > > do { > > > > > > > > lane = of_prop_next_u32(data, lane, &data_lanes[i]); > > > > > > > > } while (lane && i++ < ARRAY_SIZE(data_lanes)); > > > > link->mipi_csi_2.num_data_lanes = i; > > > > while (i--) > > > > > > > > link->mipi_csi_2.data_lanes[i] = data_lanes[i]; > > > > > > > > } > > > > > > Yes, that looks neat and does what it is supposed to do. :) Thanks! > > > For now, I'll trust you it works ;) > > > > > > With regards to the remaining patches, it looks a bit scary to me how > > > complicated it got, perhaps mostly because of requirement to reference > > > host devices from subdevs. And it seems to rely on the existing SoC > > > camera infrastructure, which might imply lot's of work need to be done > > > for non soc-camera drivers. But I'm going to take a closer look and > > > comment more on the details at the corresponding patches. > > > > I have to say that I agree with Sylwester here. It seems awfully > > complicated, but I can't really put my finger on the actual cause. > > Well, which exactly part? The V4L2 OF part is quite simple. > > > It would be really interesting to see this implemented for a non-SoC > > device and to compare the two. > > Sure, volunteers? ;-) In principle, if I find time, I could try to convert > sh_vou, which is also interesting, because it's an output driver. The OMAP3 ISP is on my to-do list, but that depends on the generic clock availability on the OMAP3, so I have to wait. > > One area that I do not yet completely understand is the i2c bus > > notifications (or asynchronous loading or i2c modules). > > > > I would have expected that using OF the i2c devices are still initialized > > before the host/bridge driver is initialized. But I gather that's not the > > case? > > No, it's not. I'm not sure, whether it depends on the order of devices in > the .dts, but, I think, it's better to not have to mandate a certain order > and I also seem to have seen devices being registered in different order > with the same DT, but I'm not 100% sure about that. > > > If this deferred probing is a general problem, then I think we need a > > general solution as well that's part of the v4l2 core. > > That can be done, perhaps. But we can do it as a next step. As soon as > we're happy with the OF implementation as such, we can commit that, > possibly leaving soc-camera patches out for now, then we can think where > to put async I2C handling. I agree that async I2C handling should have V4L2 core helpers, otherwise it's going to be pretty complex for ISP drivers.
Hi Guennadi, On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: >> I would really like to see more than one user until we add any core code. >> No that it couldn't be changed afterwards, but it would be nice to ensure >> the concepts are right and proven in real life. > > Unfortunately I don't have any more systems on which I could easily enough > try this. I've got a beagleboard with a camera, but I don't think I'm a > particularly good candidate for implementing DT support for OMAP3 camera > drivers;-) Apart from that I've only got soc-camera based systems, of > which none are _really_ DT-ready... At best I could try an i.MX31 based > board, but that doesn't have a very well developed .dts and that would be > soc-camera too anyway. I certainly wouldn't expect you would do all the job. I mean it would be good to possibly have some other developers adding device tree support based on that new bindings and new infrastructure related to them. There have been recently some progress in device tree support for Exynos SoCs, including common clock framework support and we hope to add FDT support to the Samsung SoC camera devices during this kernel cycle, based on the newly designed media bindings. This is going to be a second attempt, after our initial RFC from May [1]. It would still be SoC specific implementation, but not soc-camera based. I wasn't a big fan of this asynchronous sub-devices probing, but it now seems to be a most complete solution to me. I think it just need to be done right at the v4l2-core so individual drivers don't get complicated too much. -- Regards, Sylwester [1] http://www.spinics.net/lists/linux-media/msg48341.html -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: > Hi Guennadi, > > On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > > On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > >> I would really like to see more than one user until we add any core code. > >> No that it couldn't be changed afterwards, but it would be nice to ensure > >> the concepts are right and proven in real life. > > > > Unfortunately I don't have any more systems on which I could easily enough > > try this. I've got a beagleboard with a camera, but I don't think I'm a > > particularly good candidate for implementing DT support for OMAP3 camera > > drivers;-) Apart from that I've only got soc-camera based systems, of > > which none are _really_ DT-ready... At best I could try an i.MX31 based > > board, but that doesn't have a very well developed .dts and that would be > > soc-camera too anyway. > > I certainly wouldn't expect you would do all the job. I mean it would be good > to possibly have some other developers adding device tree support based on that > new bindings and new infrastructure related to them. > > There have been recently some progress in device tree support for Exynos SoCs, > including common clock framework support and we hope to add FDT support to the > Samsung SoC camera devices during this kernel cycle, based on the newly designed > media bindings. This is going to be a second attempt, after our initial RFC from > May [1]. It would still be SoC specific implementation, but not soc-camera based. > > I wasn't a big fan of this asynchronous sub-devices probing, but it now seems > to be a most complete solution to me. I think it just need to be done right > at the v4l2-core so individual drivers don't get complicated too much. After investigating this some more I think I agree with that. There are some things where we should probably ask for advice from the i2c subsystem devs, I'm thinking of putting the driver back into the deferred-probe state in particular. Creating v4l2-core support for this is crucial as it is quite complex and without core support this is going to be a nightmare for drivers. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Monday 08 October 2012 14:23:25 Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > [snip] > > > I think the soc_camera patches should be left out for now. I suspect that > > by adding core support for async i2c handling first, > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > The main reason for our probing order problem is the master clock, > typically supplied from the camera bridge to I2C subdevices, which we only > want to start when necessary, i.e. when accessing the subdevice. And the > subdevice driver needs that clock running during its .probe() to be able > to access and verify or configure the hardware. Our current solution is to > not register I2C subdevices from the platform data, as is usual for all > I2C devices, but from the bridge driver and only after it has switched on > the master clock. After the subdevice driver has completed its probing we > switch the clock back off until the subdevice has to be activated, e.g. > for video capture. > > Also important - when we want to unregister the bridge driver we just also > unregister the I2C device. > > Now, to reverse the whole thing and to allow I2C devices be registered as > usual - via platform data or OF, first of all we have to teach I2C > subdevice drivers to recognise the "too early" situation and request > deferred probing in such a case. Then it will be reprobed after each new > successful probe or unregister on the system. After the bridge driver has > successfully probed the subdevice driver will be re-probed and at that > time it should succeed. Now, there is a problem here too: who should > switch on and off the master clock? > > If we do it from the bridge driver, we could install an I2C bus-notifier, > _before_ the subdevice driver is probed, i.e. upon the > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > event to switch the clock back off. BUT - if the subdevice fails probing? > How do we find out about that and turn the clock back off? There is no > notification event for that... Possible solutions: > > 1. timer - ugly and unreliable. > 2. add a "probing failed" notifier event to the device core - would this > be accepted? > 3. let the subdevice turn the master clock on and off for the duration of > probing. > > My vote goes for (3). Ideally this should be done using the generic clock > framework. But can we really expect all drivers and platforms to switch to > it quickly enough? If not, we need a V4L2-specific callback from subdevice > drivers to bridge drivers to turn the clock on and off. That's what I've > done "temporarily" in this patch series for soc-camera. I vote for (3) as well, using the generic clock framework. You're right that we need an interim solution, as not all platforms will switch quickly enough. I'm fine with that interim solution being a bit hackish, as long as we push new drivers towards the right solution. > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then > we can think what else we need to add to V4L2 to support asynchronous > subdevice driver probing. > > 1. We'll have to create these V4L2 clock start and stop functions, that, > supplied (in case of I2C) with client address and adapter number will find > the correct v4l2_device instance and call its callbacks. > > 2. The I2C notifier. I'm not sure, whether this one should be common. Of > common tasks we have to refcount the I2C adapter and register the > subdevice. Then we'd have to call the bridge driver's callback. Is it > worth it doing this centrally or rather allow individual drivers to do > that themselves? What about going through board code for platforms that don't support the generic clock framework yet ? That's what the OMAP3 ISP driver does. DT support would then require the generic clock framework. > Also, ideally OF-compatible (I2C) drivers should run with no platform > data, but soc-camera is using I2C device platform data intensively. To > avoid modifying the soc-camera core and all drivers, I also trigger on the > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > created platform data to the I2C device. Would we also want to do this for > all V4L2 bridge drivers? We could call this a "prepare" callback or > something similar... If that's going to be an interim solution only I'm fine with keeping it in soc-camera. > 3. Bridge driver unregistering. Here we have to put the subdevice driver > back into the deferred-probe state... Ugliness alert: I'm doing this by > unregistering and re-registering the I2C device... For that I also have to > create a copy of devices I2C board-info data. Lovely, ain't it? This I'd > be happy to move to the V4L2 core;-) That's the ugly part. As soon as the I2C device starts using resources provided by the bridge, those resources can't disappear behind the scene. Reference counting must ensure that the bridge driver doesn't get removed. And that's where it gets bad: the bridge uses resources provided by the I2C driver, through the subdev operations. This creates circular dependencies that we need to break somehow. I currently have no solution for that problem.
Hi Hans, On Monday 08 October 2012 16:53:55 Hans Verkuil wrote: > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > [snip] > > > > > > > > > I think the soc_camera patches should be left out for now. I suspect > > > > > that by adding core support for async i2c handling first, > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > The main reason for our probing order problem is the master clock, > > > > typically supplied from the camera bridge to I2C subdevices, which we > > > > only want to start when necessary, i.e. when accessing the subdevice. > > > > And the subdevice driver needs that clock running during its .probe() > > > > to be able to access and verify or configure the hardware. Our current > > > > solution is to not register I2C subdevices from the platform data, as > > > > is usual for all I2C devices, but from the bridge driver and only > > > > after it has switched on the master clock. After the subdevice driver > > > > has completed its probing we switch the clock back off until the > > > > subdevice has to be activated, e.g. for video capture. > > > > > > > > Also important - when we want to unregister the bridge driver we just > > > > also> unregister the I2C device. > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be registered > > > > as usual - via platform data or OF, first of all we have to teach I2C > > > > subdevice drivers to recognise the "too early" situation and request > > > > deferred probing in such a case. Then it will be reprobed after each > > > > new successful probe or unregister on the system. After the bridge > > > > driver has successfully probed the subdevice driver will be re-probed > > > > and at that time it should succeed. Now, there is a problem here too: > > > > who should switch on and off the master clock? > > > > > > > > If we do it from the bridge driver, we could install an I2C bus- > > > > notifier, _before_ the subdevice driver is probed, i.e. upon the > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > > > > probing was successful, we can then wait for the > > > > BUS_NOTIFY_BOUND_DRIVER event to switch the clock back off. BUT - if > > > > the subdevice fails probing? > > > > How do we find out about that and turn the clock back off? There is no > > > > notification event for that... Possible solutions: > > > > > > > > 1. timer - ugly and unreliable. > > > > 2. add a "probing failed" notifier event to the device core - would > > > > this be accepted? > > > > 3. let the subdevice turn the master clock on and off for the duration > > > > of probing. > > > > > > > > My vote goes for (3). Ideally this should be done using the generic > > > > clock framework. But can we really expect all drivers and platforms to > > > > switch to it quickly enough? If not, we need a V4L2-specific callback > > > > from subdevice drivers to bridge drivers to turn the clock on and off. > > > > That's what I've done "temporarily" in this patch series for soc- > > > > camera. > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add call-backs. > > > > Then we can think what else we need to add to V4L2 to support > > > > asynchronous subdevice driver probing. > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev drivers > > > can have internal_ops with register/unregister ops. These are called by > > > v4l2_device_register_subdev. This happens during the bridge driver's > > > probe. > > > > > > Suppose the subdev's probe does not actually access the i2c device, but > > > instead defers that to the register callback? The bridge driver will > > > turn on the clock before calling v4l2_device_register_subdev to ensure > > > that the register callback can access the i2c registers. The register > > > callback will do any initialization and can return an error. In case of > > > an error the i2c client is automatically unregistered as well. > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > several times before and always what I didn't like in this is, that I2C > > device probe() in this case succeeds without even trying to access the > > hardware. And think about DT. In this case we don't instantiate the I2C > > device, OF code does it for us. What do you do then? If you let probe() > > succeed, then you will have to somehow remember the subdevice to later > > match it against bridges... > > Yes, but you need that information anyway. The bridge still needs to call > v4l2_device_register_subdev so it needs to know which subdevs are loaded. > And can't it get that from DT as well? That information will definitely come from the DT, but the bridge won't instantiate the I2C devices. They will be instantiated asynchronously by the I2C bus master driver when parsing the DT. The bridge will then need to be notified or I2C devices registration and finish its initialization when all needed I2C (or SPI, ...) devices will be available. That should in my opinion be handled by the V4L2 core : the bridge would pass a list of devices (possibly DT nodes) to a V4L2 core function along with a callback, and the callback would be called when all required devices are available. I've also thought about adding a synchronous function that would wait until all required devices are available, but I'm not sure whether that's a good idea. > In my view you cannot do a proper initialization unless you have both the > bridge driver and all subdev drivers loaded and instantiated. You can do a proper initialization of the bridge device. The OMAP3 ISP could already be used for mem-to-mem operation for instance. > They need one another. So I am perfectly fine with letting the probe > function do next to nothing and postponing that until register() is called. I don't really like that solution, it's an abuse of the probe() function. It would be much better to defer probing of the I2C device until all the needed resources (such as clocks) are available. That would also mean that the I2C device won't be able to access the bridge directly during probing, which could actually be seen as a feature : with proper abstractions in place (such as generic clock objects) accessing the bridge at probe() time should not be needed. > I2C and actual probing to check if it's the right device is a bad idea in > general since you have no idea what a hardware access to an unknown i2c > device will do. There are still some corner cases where that is needed, but > I do not think that that is an issue here. > > It would simplify things a lot IMHO. Also note that the register() op will > work with any device, not just i2c. That may be a useful property as well. > > > > In addition, during the register op the subdev driver can call into the > > > bridge driver since it knows the v4l2_device struct. > > > > > > This has also the advantage that subdev drivers can change to this model > > > gradually. Only drivers that need master clocks, etc. need to move any > > > probe code that actually accesses hardware to the register op. Others > > > can remain as. Nor should this change behavior of existing drivers as > > > this happens all in the V4L2 core. > > > > > > The bridge driver may still have to wait until all i2c drivers are > > > loaded, though. But that can definitely be handled centrally (i.e.: 'I > > > need these drivers, wait until all are loaded'). > > > > > > > 1. We'll have to create these V4L2 clock start and stop functions, > > > > that, supplied (in case of I2C) with client address and adapter number > > > > will find the correct v4l2_device instance and call its callbacks. > > > > > > > > 2. The I2C notifier. I'm not sure, whether this one should be common. > > > > Of common tasks we have to refcount the I2C adapter and register the > > > > subdevice. Then we'd have to call the bridge driver's callback. Is it > > > > worth it doing this centrally or rather allow individual drivers to do > > > > that themselves? > > > > > > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > > > data, but soc-camera is using I2C device platform data intensively. To > > > > avoid modifying the soc-camera core and all drivers, I also trigger on > > > > the BUS_NOTIFY_BIND_DRIVER event and assign a reference to the > > > > dynamically created platform data to the I2C device. Would we also > > > > want to do this for all V4L2 bridge drivers? We could call this a > > > > "prepare" callback or something similar... > > > > > > Well, subdev drivers should either parse the OF data, or use the > > > platform_data. The way soc_camera uses platform_data is one reason why > > > it is so hard to reuse subdevs for non-soc_camera drivers. All the > > > callbacks in soc_camera_link should be replaced by calls to the > > > v4l2_device notify() callback. After that we can see what is needed to > > > drop struct soc_camera_link altogether as platform_data. > > > > They don't have to be, they are not (or should not be) called by > > subdevices. > > Then why are those callbacks in a struct that subdevs can access? I always > have a hard time with soc_camera figuring out who is using what when :-( > > > > > 3. Bridge driver unregistering. Here we have to put the subdevice > > > > driver back into the deferred-probe state... Ugliness alert: I'm doing > > > > this by unregistering and re-registering the I2C device... For that I > > > > also have to create a copy of devices I2C board-info data. Lovely, > > > > isn't it? This I'd be happy to move to the V4L2 core;-) > > > > > > By just using the unregister ops this should be greatly simplified as > > > well. > > > > Sorry, which unregister ops do you mean? internal_ops->unregistered()? > > Yes. > > > Yes, but only if we somehow go your way and use dummy probe() methods... > > Of course.
Hi Guennadi, On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > > On Mon October 8 2012 14:23:25 Guennadi Liakhovetski wrote: > > > > > On Fri, 5 Oct 2012, Hans Verkuil wrote: > > > > > > > > > > [snip] > > > > > > > > > > > I think the soc_camera patches should be left out for now. I > > > > > > suspect that by adding core support for async i2c handling first, > > > > > > > > > > Ok, let's think, what this meacs - async I2C in media / V4L2 core. > > > > > > > > > > The main reason for our probing order problem is the master clock, > > > > > typically supplied from the camera bridge to I2C subdevices, which > > > > > we only want to start when necessary, i.e. when accessing the > > > > > subdevice. And the subdevice driver needs that clock running during > > > > > its .probe() to be able to access and verify or configure the > > > > > hardware. Our current solution is to not register I2C subdevices > > > > > from the platform data, as is usual for all I2C devices, but from > > > > > the bridge driver and only after it has switched on the master > > > > > clock. After the subdevice driver has completed its probing we > > > > > switch the clock back off until the subdevice has to be activated, > > > > > e.g. for video capture. > > > > > > > > > > Also important - when we want to unregister the bridge driver we > > > > > just also unregister the I2C device. > > > > > > > > > > Now, to reverse the whole thing and to allow I2C devices be > > > > > registered as usual - via platform data or OF, first of all we have > > > > > to teach I2C subdevice drivers to recognise the "too early" > > > > > situation and request deferred probing in such a case. Then it will > > > > > be reprobed after each new successful probe or unregister on the > > > > > system. After the bridge driver has successfully probed the > > > > > subdevice driver will be re-probed and at that time it should > > > > > succeed. Now, there is a problem here too: who should switch on and > > > > > off the master clock? > > > > > > > > > > If we do it from the bridge driver, we could install an I2C > > > > > bus-notifier, _before_ the subdevice driver is probed, i.e. upon the > > > > > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If > > > > > subdevice probing was successful, we can then wait for the > > > > > BUS_NOTIFY_BOUND_DRIVER event to switch the clock back off. BUT - if > > > > > the subdevice fails probing? > > > > > How do we find out about that and turn the clock back off? There is > > > > > no notification event for that... Possible solutions: > > > > > > > > > > 1. timer - ugly and unreliable. > > > > > 2. add a "probing failed" notifier event to the device core - would > > > > > this be accepted? > > > > > 3. let the subdevice turn the master clock on and off for the > > > > > duration of probing. > > > > > > > > > > My vote goes for (3). Ideally this should be done using the generic > > > > > clock framework. But can we really expect all drivers and platforms > > > > > to switch to it quickly enough? If not, we need a V4L2-specific > > > > > callback from subdevice drivers to bridge drivers to turn the clock > > > > > on and off. That's what I've done "temporarily" in this patch series > > > > > for soc-camera. > > > > > > > > > > Suppose we decide to do the same for V4L2 centrally - add > > > > > call-backs. Then we can think what else we need to add to V4L2 to > > > > > support asynchronous subdevice driver probing. > > > > > > > > I wonder, don't we have the necessary code already? V4L2 subdev > > > > drivers can have internal_ops with register/unregister ops. These are > > > > called by v4l2_device_register_subdev. This happens during the bridge > > > > driver's probe. > > > > > > > > Suppose the subdev's probe does not actually access the i2c device, > > > > but instead defers that to the register callback? The bridge driver > > > > will turn on the clock before calling v4l2_device_register_subdev to > > > > ensure that the register callback can access the i2c registers. The > > > > register callback will do any initialization and can return an error. > > > > In case of an error the i2c client is automatically unregistered as > > > > well. > > > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > > several times before and always what I didn't like in this is, that I2C > > > device probe() in this case succeeds without even trying to access the > > > hardware. And think about DT. In this case we don't instantiate the I2C > > > device, OF code does it for us. What do you do then? If you let probe() > > > succeed, then you will have to somehow remember the subdevice to later > > > match it against bridges... > > > > Yes, but you need that information anyway. The bridge still needs to call > > v4l2_device_register_subdev so it needs to know which subdevs are loaded. > > But how do you get the subdev pointer? With the notifier I get it from > i2c_get_clientdata(client) and what do you do without it? How do you get > to the client? > > > And can't it get that from DT as well? > > No, I don't think there is a way to get a device pointer from a DT node. But we'll need a way. The bridge driver will get sensor DT nodes from the V4L2 DT bindings, and will need to get the corresponding subdev. This could be limited to V4L2 though, we could keep a map of DT nodes to subdevs without requiring a generic solution in the device base code (although I'm wondering if there's a specific reason not to have a device pointer in the DT node structure). > > In my view you cannot do a proper initialization unless you have both the > > bridge driver and all subdev drivers loaded and instantiated. They need > > one another. So I am perfectly fine with letting the probe function do > > next to nothing and postponing that until register() is called. I2C and > > actualprobing to check if it's the right device is a bad idea in general > > since you have no idea what a hardware access to an unknown i2c device > > will do. There are still some corner cases where that is needed, but I do > > not think that that is an issue here. > > > > It would simplify things a lot IMHO. Also note that the register() op will > > work with any device, not just i2c. That may be a useful property as well. > > And what if the subdevice device is not yet instantiated by OF by the time > your bridge driver probes?
Hi Hans, On Monday 08 October 2012 17:41:43 Hans Verkuil wrote: > On Mon October 8 2012 17:15:53 Guennadi Liakhovetski wrote: > > On Mon, 8 Oct 2012, Hans Verkuil wrote: > > > On Mon October 8 2012 16:30:53 Guennadi Liakhovetski wrote: > > > > On Mon, 8 Oct 2012, Hans Verkuil wrote: [snip] > > > > > I wonder, don't we have the necessary code already? V4L2 subdev > > > > > drivers can have internal_ops with register/unregister ops. These > > > > > are called by v4l2_device_register_subdev. This happens during the > > > > > bridge driver's probe. > > > > > > > > > > Suppose the subdev's probe does not actually access the i2c device, > > > > > but instead defers that to the register callback? The bridge driver > > > > > will turn on the clock before calling v4l2_device_register_subdev to > > > > > ensure that the register callback can access the i2c registers. The > > > > > register callback will do any initialization and can return an > > > > > error. In case of an error the i2c client is automatically > > > > > unregistered as well. > > > > > > > > Yes, if v4l2_i2c_new_subdev_board() is used. This has been discussed > > > > several times before and always what I didn't like in this is, that > > > > I2C device probe() in this case succeeds without even trying to access > > > > the hardware. And think about DT. In this case we don't instantiate > > > > the I2C device, OF code does it for us. What do you do then? If you > > > > let probe() succeed, then you will have to somehow remember the > > > > subdevice to later match it against bridges... > > > > > > Yes, but you need that information anyway. The bridge still needs to > > > call v4l2_device_register_subdev so it needs to know which subdevs are > > > loaded. > > > > But how do you get the subdev pointer? With the notifier I get it from > > i2c_get_clientdata(client) and what do you do without it? How do you get > > to the client? > > > > > And can't it get that from DT as well? > > > > No, I don't think there is a way to get a device pointer from a DT node. > > Not a device pointer, but the i2c bus and i2c address. With that information > you can get the i2c_client, and with that you can get the subdev pointer. That could work as well, but it might be easier to keep a mapping from the DT node to struct device or struct v4l2_subdev instead. I have no strong opinion on this at the moment. > If there is no way to get that information from the proposed V4L2 DT, then > it needs to be modified since a bridge driver really needs to know which > subdevs it has to register with the v4l2_device struct. That information is > also board specific so it should be part of the DT. > > > > In my view you cannot do a proper initialization unless you have both > > > the bridge driver and all subdev drivers loaded and instantiated. They > > > need one another. So I am perfectly fine with letting the probe function > > > do next to nothing and postponing that until register() is called. I2C > > > and actual probing to check if it's the right device is a bad idea in > > > general since you have no idea what a hardware access to an unknown i2c > > > device will do. There are still some corner cases where that is needed, > > > but I do not think that that is an issue here. > > > > > > It would simplify things a lot IMHO. Also note that the register() op > > > will work with any device, not just i2c. That may be a useful property > > > as well. > > > > And what if the subdevice device is not yet instantiated by OF by the time > > your bridge driver probes? > > That is where you still need to have a bus notifier mechanism. You have to > be able to wait until all dependent drivers are loaded/instantiated, or > alternatively you have to be able to load them explicitly. But this should > be relatively easy to implement in a generic manner. > > I still think this sucks (excuse my language), but I see no way around it as > long as there is no explicit probe order one can rely on.
Hi Hans, On Tuesday 09 October 2012 13:00:24 Hans Verkuil wrote: > On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: > > On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > > > On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > > >> I would really like to see more than one user until we add any core > > >> code. No that it couldn't be changed afterwards, but it would be nice > > >> to ensure the concepts are right and proven in real life. > > > > > > Unfortunately I don't have any more systems on which I could easily > > > enough try this. I've got a beagleboard with a camera, but I don't think > > > I'm a particularly good candidate for implementing DT support for OMAP3 > > > camera drivers;-) Apart from that I've only got soc-camera based > > > systems, of which none are _really_ DT-ready... At best I could try an > > > i.MX31 based board, but that doesn't have a very well developed .dts and > > > that would be soc-camera too anyway. > > > > I certainly wouldn't expect you would do all the job. I mean it would be > > good to possibly have some other developers adding device tree support > > based on that new bindings and new infrastructure related to them. As I mentioned in another e-mail, I plan to work on DT support for the OMAP3 ISP, but I first need generic clock framework support for OMAP3. > > There have been recently some progress in device tree support for Exynos > > SoCs, including common clock framework support and we hope to add FDT > > support to the Samsung SoC camera devices during this kernel cycle, based > > on the newly designed media bindings. This is going to be a second > > attempt, after our initial RFC from May [1]. It would still be SoC > > specific implementation, but not soc-camera based. > > > > I wasn't a big fan of this asynchronous sub-devices probing, but it now > > seems to be a most complete solution to me. I think it just need to be > > done right at the v4l2-core so individual drivers don't get complicated > > too much. > > After investigating this some more I think I agree with that. There are some > things where we should probably ask for advice from the i2c subsystem devs, > I'm thinking of putting the driver back into the deferred-probe state in > particular. We might actually not need that, it might be easier to handle the circular dependency problem from the other end. We could add a way (ioctl, sysfs, ...) to force a V4L2 bridge driver to release its subdevs. Once done, the subdev driver could be unloaded and/or the subdev device unregistered, which would release the resources used by the subdev, such as clocks. The bridge driver could then be unregistered. > Creating v4l2-core support for this is crucial as it is quite complex and > without core support this is going to be a nightmare for drivers.
Em Wed, 10 Oct 2012 14:54 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > data, but soc-camera is using I2C device platform data intensively. To > > avoid modifying the soc-camera core and all drivers, I also trigger on the > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > created platform data to the I2C device. Would we also want to do this for > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > something similar... > > If that's going to be an interim solution only I'm fine with keeping it in > soc-camera. I'm far from being an OF expert, but why do we need to export I2C devices to DT/OF? On my understanding, it is the bridge code that should be responsible for detecting, binding and initializing the proper I2C devices. On several cases, it is impossible or it would cause lots of ugly hacks if we ever try to move away from platform data stuff, as only the bridge driver knows what initialization is needed for an specific I2C driver. To make things more complex, it is expected that most I2C drivers to be arch independent, and they should be allowed to be used by a personal computer or by an embedded device. Let me give 2 such examples: - ir-i2c-kbd driver supports lots of IR devices. Platform_data is needed to specify what hardware will actually be used, and what should be the default Remote Controller keymap; - Sensor drivers like ov2940 is needed by soc_camera and by other webcam drivers like em28xx. The setup for em28xx should be completely different than the one for soc_camera: the initial registers init sequence is different for both. As several registers there aren't properly documented, there's no easy way to parametrize the configuration. So, for me, we should not expose the I2C devices directly on OF; it should, instead, see just the bridge, and let the bridge to map the needed I2C devices using the needed platform_data.
Hi Mauro, On Wednesday 10 October 2012 10:45:22 Mauro Carvalho Chehab wrote: > Em Wed, 10 Oct 2012 14:54 +0200 Laurent Pinchart escreveu: > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > > data, but soc-camera is using I2C device platform data intensively. To > > > avoid modifying the soc-camera core and all drivers, I also trigger on > > > the > > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > > created platform data to the I2C device. Would we also want to do this > > > for > > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > > something similar... > > > > If that's going to be an interim solution only I'm fine with keeping it in > > soc-camera. > > I'm far from being an OF expert, but why do we need to export I2C devices to > DT/OF? On my understanding, it is the bridge code that should be > responsible for detecting, binding and initializing the proper I2C devices. > On several cases, it is impossible or it would cause lots of ugly hacks if > we ever try to move away from platform data stuff, as only the bridge > driver knows what initialization is needed for an specific I2C driver. In a nutshell, a DT-based platform doesn't have any board code (except in rare cases, but let's not get into that), and thus doesn't pass any platform data structure to drivers. However, drivers receive a DT node, which contains a hierarchical description of the hardware, and parse those to extract information necessary to configure the device. One very important point to keep in mind here is that the DT is not Linux- specific. DT bindings are designed to be portable, and thus must only contain hardware descriptions, without any OS-specific information or policy information. For instance information such as the maximum video buffers size is not allowed in the DT. The DT is used both to provide platform data to drivers and to instantiate devices. I2C device DT nodes are stored as children of the I2C bus master DT node, and are automatically instantiated by the I2C bus master. This is a significant change compared to our current situation where the V4L2 bridge driver receives an array of I2C board information structures and instatiates the devices itself. Most of the DT support work will go in supporting that new instantiation mechanism. The bridge driver doesn't control instantiation of the I2C devices anymore, but need to bind with them at runtime. > To make things more complex, it is expected that most I2C drivers to be arch > independent, and they should be allowed to be used by a personal computer > or by an embedded device. Agreed. That's why platform data structures won't go away anytime soon, a PCI bridge driver for hardware that contain I2C devices will still instantiate the I2C devices itself. We don't plan to or even want to get rid of that mechanism, as there are perfectly valid use cases. However, for DT-based embedded platforms, we need to support a new instantiation mechanism. > Let me give 2 such examples: > > - ir-i2c-kbd driver supports lots of IR devices. Platform_data is needed > to specify what hardware will actually be used, and what should be the > default Remote Controller keymap; That driver isn't used on embedded platforms so it doesn't need to be changed now. > - Sensor drivers like ov2940 is needed by soc_camera and by other > webcam drivers like em28xx. The setup for em28xx should be completely > different than the one for soc_camera: the initial registers init sequence > is different for both. As several registers there aren't properly > documented, there's no easy way to parametrize the configuration. Such drivers will need to support both DT-based platform data and structure- based platform data. They will likely parse the DT node and fill a platform data structure, to avoid duplicating initialization code. Please note that the new subdevs instantiation mechanism needed for DT will need to handle any instantiation order, as we can't guarantee the I2C device and bridge device instantiation order with DT. It should thus then support the current instantiation order we use for PCI and USB platforms. > So, for me, we should not expose the I2C devices directly on OF; it should, > instead, see just the bridge, and let the bridge to map the needed I2C > devices using the needed platform_data. We can't do that, there will be no platform data anymore with DT-based platforms. As a summary, platform data structures won't go away, I2C drivers that need to work both on DT-based and non-DT-based platforms will need to support both the DT and platform data structures to get platform data. PCI and USB bridges will still instantiate their I2C devices, and binding between the I2C devices and the bridge can either be handled with two different instantiation mechanisms (the new one for DT platforms, with runtime bindings, and the existing one for non-DT platforms, with binding at instantiation time) or move to a single runtime binding mechanism. It's too early to know which solution will be simpler.
Em Wed, 10 Oct 2012 16:48 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi Mauro, > > On Wednesday 10 October 2012 10:45:22 Mauro Carvalho Chehab wrote: > > Em Wed, 10 Oct 2012 14:54 +0200 Laurent Pinchart escreveu: > > > > Also, ideally OF-compatible (I2C) drivers should run with no platform > > > > data, but soc-camera is using I2C device platform data intensively. To > > > > avoid modifying the soc-camera core and all drivers, I also trigger on > > > > the > > > > BUS_NOTIFY_BIND_DRIVER event and assign a reference to the dynamically > > > > created platform data to the I2C device. Would we also want to do this > > > > for > > > > all V4L2 bridge drivers? We could call this a "prepare" callback or > > > > something similar... > > > > > > If that's going to be an interim solution only I'm fine with keeping it in > > > soc-camera. > > > > I'm far from being an OF expert, but why do we need to export I2C devices to > > DT/OF? On my understanding, it is the bridge code that should be > > responsible for detecting, binding and initializing the proper I2C devices. > > On several cases, it is impossible or it would cause lots of ugly hacks if > > we ever try to move away from platform data stuff, as only the bridge > > driver knows what initialization is needed for an specific I2C driver. > > In a nutshell, a DT-based platform doesn't have any board code (except in rare > cases, but let's not get into that), and thus doesn't pass any platform data > structure to drivers. However, drivers receive a DT node, which contains a > hierarchical description of the hardware, and parse those to extract > information necessary to configure the device. > > One very important point to keep in mind here is that the DT is not Linux- > specific. DT bindings are designed to be portable, and thus must only contain > hardware descriptions, without any OS-specific information or policy > information. For instance information such as the maximum video buffers size > is not allowed in the DT. > > The DT is used both to provide platform data to drivers and to instantiate > devices. I2C device DT nodes are stored as children of the I2C bus master DT > node, and are automatically instantiated by the I2C bus master. This is a > significant change compared to our current situation where the V4L2 bridge > driver receives an array of I2C board information structures and instatiates > the devices itself. Most of the DT support work will go in supporting that new > instantiation mechanism. The bridge driver doesn't control instantiation of > the I2C devices anymore, but need to bind with them at runtime. > > > To make things more complex, it is expected that most I2C drivers to be arch > > independent, and they should be allowed to be used by a personal computer > > or by an embedded device. > > Agreed. That's why platform data structures won't go away anytime soon, a PCI > bridge driver for hardware that contain I2C devices will still instantiate the > I2C devices itself. We don't plan to or even want to get rid of that > mechanism, as there are perfectly valid use cases. However, for DT-based > embedded platforms, we need to support a new instantiation mechanism. > > > Let me give 2 such examples: > > > > - ir-i2c-kbd driver supports lots of IR devices. Platform_data is needed > > to specify what hardware will actually be used, and what should be the > > default Remote Controller keymap; > > That driver isn't used on embedded platforms so it doesn't need to be changed > now. > > > - Sensor drivers like ov2940 is needed by soc_camera and by other > > webcam drivers like em28xx. The setup for em28xx should be completely > > different than the one for soc_camera: the initial registers init sequence > > is different for both. As several registers there aren't properly > > documented, there's no easy way to parametrize the configuration. > > Such drivers will need to support both DT-based platform data and structure- > based platform data. They will likely parse the DT node and fill a platform > data structure, to avoid duplicating initialization code. > > Please note that the new subdevs instantiation mechanism needed for DT will > need to handle any instantiation order, as we can't guarantee the I2C device > and bridge device instantiation order with DT. It should thus then support the > current instantiation order we use for PCI and USB platforms. > > > So, for me, we should not expose the I2C devices directly on OF; it should, > > instead, see just the bridge, and let the bridge to map the needed I2C > > devices using the needed platform_data. > > We can't do that, there will be no platform data anymore with DT-based > platforms. > > As a summary, platform data structures won't go away, I2C drivers that need to > work both on DT-based and non-DT-based platforms will need to support both the > DT and platform data structures to get platform data. PCI and USB bridges will > still instantiate their I2C devices, and binding between the I2C devices and > the bridge can either be handled with two different instantiation mechanisms > (the new one for DT platforms, with runtime bindings, and the existing one for > non-DT platforms, with binding at instantiation time) or move to a single > runtime binding mechanism. It's too early to know which solution will be > simpler. > It seems that you're designing a Frankstein monster with DT/OF and I2C. Increasing each I2C driver code to support both platform_data and DT way of doing things seems a huge amount of code that will be added, and I really fail to understand why this is needed, in the first place. Ok, I understand that OF specs are OS-independent, but I suspect that they don't dictate how things should be wired internally at the OS. So, if DT/OF is so restrictive, and require its own way of doing things, instead of changing everything with the risks of adding (more) regressions to platform drivers, why don't just create a shell driver that will encapsulate DT/OF specific stuff into the platform_data? Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mauro, On Wednesday 10 October 2012 11:57:03 Mauro Carvalho Chehab wrote: > Em Wed, 10 Oct 2012 16:48 +0200 Laurent Pinchart escreveu: > > On Wednesday 10 October 2012 10:45:22 Mauro Carvalho Chehab wrote: > > > Em Wed, 10 Oct 2012 14:54 +0200 Laurent Pinchart escreveu: > > > > > Also, ideally OF-compatible (I2C) drivers should run with no > > > > > platform data, but soc-camera is using I2C device platform data > > > > > intensively. To avoid modifying the soc-camera core and all drivers, > > > > > I also trigger on the BUS_NOTIFY_BIND_DRIVER event and assign a > > > > > reference to the dynamically created platform data to the I2C > > > > > device. Would we also want to do this for all V4L2 bridge drivers? > > > > > We could call this a "prepare" callback or something similar... > > > > > > > > If that's going to be an interim solution only I'm fine with keeping > > > > it in soc-camera. > > > > > > I'm far from being an OF expert, but why do we need to export I2C > > > devices to DT/OF? On my understanding, it is the bridge code that > > > should be responsible for detecting, binding and initializing the proper > > > I2C devices. On several cases, it is impossible or it would cause lots > > > of ugly hacks if we ever try to move away from platform data stuff, as > > > only the bridge driver knows what initialization is needed for an > > > specific I2C driver. > > > > In a nutshell, a DT-based platform doesn't have any board code (except in > > rare cases, but let's not get into that), and thus doesn't pass any > > platform data structure to drivers. However, drivers receive a DT node, > > which contains a hierarchical description of the hardware, and parse > > those to extract information necessary to configure the device. > > > > One very important point to keep in mind here is that the DT is not Linux- > > specific. DT bindings are designed to be portable, and thus must only > > contain hardware descriptions, without any OS-specific information or > > policy information. For instance information such as the maximum video > > buffers size is not allowed in the DT. > > > > The DT is used both to provide platform data to drivers and to instantiate > > devices. I2C device DT nodes are stored as children of the I2C bus master > > DT node, and are automatically instantiated by the I2C bus master. This > > is a significant change compared to our current situation where the V4L2 > > bridge driver receives an array of I2C board information structures and > > instatiates the devices itself. Most of the DT support work will go in > > supporting that new instantiation mechanism. The bridge driver doesn't > > control instantiation of the I2C devices anymore, but need to bind with > > them at runtime. > > > > > To make things more complex, it is expected that most I2C drivers to be > > > arch independent, and they should be allowed to be used by a personal > > > computer or by an embedded device. > > > > Agreed. That's why platform data structures won't go away anytime soon, a > > PCI bridge driver for hardware that contain I2C devices will still > > instantiate the I2C devices itself. We don't plan to or even want to get > > rid of that mechanism, as there are perfectly valid use cases. However, > > for DT-based embedded platforms, we need to support a new instantiation > > mechanism. > > > > > Let me give 2 such examples: > > > - ir-i2c-kbd driver supports lots of IR devices. Platform_data is > > > needed > > > > > > to specify what hardware will actually be used, and what should be the > > > default Remote Controller keymap; > > > > That driver isn't used on embedded platforms so it doesn't need to be > > changed now. > > > > > - Sensor drivers like ov2940 is needed by soc_camera and by other > > > > > > webcam drivers like em28xx. The setup for em28xx should be completely > > > different than the one for soc_camera: the initial registers init > > > sequence > > > is different for both. As several registers there aren't properly > > > documented, there's no easy way to parametrize the configuration. > > > > Such drivers will need to support both DT-based platform data and > > structure- based platform data. They will likely parse the DT node and > > fill a platform data structure, to avoid duplicating initialization code. > > > > Please note that the new subdevs instantiation mechanism needed for DT > > will need to handle any instantiation order, as we can't guarantee the I2C > > device and bridge device instantiation order with DT. It should thus then > > support the current instantiation order we use for PCI and USB platforms. > > > > > So, for me, we should not expose the I2C devices directly on OF; it > > > should, > > > instead, see just the bridge, and let the bridge to map the needed I2C > > > devices using the needed platform_data. > > > > We can't do that, there will be no platform data anymore with DT-based > > platforms. > > > > As a summary, platform data structures won't go away, I2C drivers that > > need to work both on DT-based and non-DT-based platforms will need to > > support both the DT and platform data structures to get platform data. > > PCI and USB bridges will still instantiate their I2C devices, and binding > > between the I2C devices and the bridge can either be handled with two > > different instantiation mechanisms (the new one for DT platforms, with > > runtime bindings, and the existing one for non-DT platforms, with binding > > at instantiation time) or move to a single runtime binding mechanism. > > It's too early to know which solution will be simpler. > > It seems that you're designing a Frankstein monster with DT/OF and I2C. > > Increasing each I2C driver code to support both platform_data and DT way > of doing things seems a huge amount of code that will be added, and I really > fail to understand why this is needed, in the first place. > > Ok, I understand that OF specs are OS-independent, but I suspect that > they don't dictate how things should be wired internally at the OS. > > So, if DT/OF is so restrictive, and require its own way of doing things, > instead of changing everything with the risks of adding (more) regressions > to platform drivers, why don't just create a shell driver that will > encapsulate DT/OF specific stuff into the platform_data? DT support requires two independent parts, DT-based probing and device instantiation changes. To support DT probing existing I2C drivers (and only the drivers that need to support DT-enabled platforms, we don't have to modify any I2C driver used on non-DT platforms only) will need to add a function that parses a DT node to fill a platform data structure, and a new OF match table pointer in their struct device. For instance here's what the OMAP I2C bus master does in its probe function: match = of_match_device(of_match_ptr(omap_i2c_of_match), &pdev->dev); if (match) { u32 freq = 100000; /* default to 100000 Hz */ pdata = match->data; dev->dtrev = pdata->rev; dev->flags = pdata->flags; of_property_read_u32(node, "clock-frequency", &freq); /* convert DT freq value in Hz into kHz for speed */ dev->speed = freq / 1000; } else if (pdata != NULL) { dev->speed = pdata->clkrate; dev->flags = pdata->flags; dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; dev->dtrev = pdata->rev; } Before DT support only the second branch of the if was there. The above code could be rewritten as if (DT enabled) fill_pdata_from_dt(pdata, DT node); Rest of normal pdata-based initialization code here with the first branch of the if in the fill_pdata_from_dt() function. It's really not a big deal in my opinion, and creating a new wrapper for that would just be overkill. The device instantiation issue, discusses in this mail thread, is a more complex problem for which we need a solution, but isn't related to platform data.
On 10/10/2012 07:18 AM, Laurent Pinchart wrote: > On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: ... >> But how do you get the subdev pointer? With the notifier I get it from >> i2c_get_clientdata(client) and what do you do without it? How do you get >> to the client? >> >>> And can't it get that from DT as well? >> >> No, I don't think there is a way to get a device pointer from a DT node. I don't believe there's a generic API for this (although perhaps there could be), but it can be implemented quite easily. For example, on Tegra, the SMMU needs to flip a bit in the AHB register space in order to enable itself. The SMMU DT node contains a phandle that points at the AHB DT node. The SMMU driver parses the phandle and passes the DT node pointer to the AHB driver. The AHB driver looks up the struct device that was instantiated for that node. See drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu(). There are a few other cases of similar code in the kernel, although I can't remember the others! -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On 10/10/2012 03:25 PM, Laurent Pinchart wrote: > On Tuesday 09 October 2012 13:00:24 Hans Verkuil wrote: >> On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: >>> On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: >>>> On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: >>>>> I would really like to see more than one user until we add any core >>>>> code. No that it couldn't be changed afterwards, but it would be nice >>>>> to ensure the concepts are right and proven in real life. >>>> >>>> Unfortunately I don't have any more systems on which I could easily >>>> enough try this. I've got a beagleboard with a camera, but I don't think >>>> I'm a particularly good candidate for implementing DT support for OMAP3 >>>> camera drivers;-) Apart from that I've only got soc-camera based >>>> systems, of which none are _really_ DT-ready... At best I could try an >>>> i.MX31 based board, but that doesn't have a very well developed .dts and >>>> that would be soc-camera too anyway. >>> >>> I certainly wouldn't expect you would do all the job. I mean it would be >>> good to possibly have some other developers adding device tree support >>> based on that new bindings and new infrastructure related to them. > > As I mentioned in another e-mail, I plan to work on DT support for the OMAP3 > ISP, but I first need generic clock framework support for OMAP3. OK, let's hope it's available soon. >>> There have been recently some progress in device tree support for Exynos >>> SoCs, including common clock framework support and we hope to add FDT >>> support to the Samsung SoC camera devices during this kernel cycle, based >>> on the newly designed media bindings. This is going to be a second >>> attempt, after our initial RFC from May [1]. It would still be SoC >>> specific implementation, but not soc-camera based. >>> >>> I wasn't a big fan of this asynchronous sub-devices probing, but it now >>> seems to be a most complete solution to me. I think it just need to be >>> done right at the v4l2-core so individual drivers don't get complicated >>> too much. >> >> After investigating this some more I think I agree with that. There are some >> things where we should probably ask for advice from the i2c subsystem devs, >> I'm thinking of putting the driver back into the deferred-probe state in >> particular. > > We might actually not need that, it might be easier to handle the circular > dependency problem from the other end. We could add a way (ioctl, sysfs, ...) > to force a V4L2 bridge driver to release its subdevs. Once done, the subdev > driver could be unloaded and/or the subdev device unregistered, which would > release the resources used by the subdev, such as clocks. The bridge driver > could then be unregistered. That sounds like an option. Perhaps it could be done by v4l2-core, e.g. a sysfs entry could be registered for a media or video device if driver requests it. I'm not sure if we should allow subdevs in "released" state, perhaps it's better to just unregister subdevs entirely right away ? >> Creating v4l2-core support for this is crucial as it is quite complex and >> without core support this is going to be a nightmare for drivers. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 10 Oct 2012, Sylwester Nawrocki wrote: > Hi Laurent, > > On 10/10/2012 03:25 PM, Laurent Pinchart wrote: > > On Tuesday 09 October 2012 13:00:24 Hans Verkuil wrote: > >> On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: > >>> On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > >>>> On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > >>>>> I would really like to see more than one user until we add any core > >>>>> code. No that it couldn't be changed afterwards, but it would be nice > >>>>> to ensure the concepts are right and proven in real life. > >>>> > >>>> Unfortunately I don't have any more systems on which I could easily > >>>> enough try this. I've got a beagleboard with a camera, but I don't think > >>>> I'm a particularly good candidate for implementing DT support for OMAP3 > >>>> camera drivers;-) Apart from that I've only got soc-camera based > >>>> systems, of which none are _really_ DT-ready... At best I could try an > >>>> i.MX31 based board, but that doesn't have a very well developed .dts and > >>>> that would be soc-camera too anyway. > >>> > >>> I certainly wouldn't expect you would do all the job. I mean it would be > >>> good to possibly have some other developers adding device tree support > >>> based on that new bindings and new infrastructure related to them. > > > > As I mentioned in another e-mail, I plan to work on DT support for the OMAP3 > > ISP, but I first need generic clock framework support for OMAP3. > > OK, let's hope it's available soon. > > >>> There have been recently some progress in device tree support for Exynos > >>> SoCs, including common clock framework support and we hope to add FDT > >>> support to the Samsung SoC camera devices during this kernel cycle, based > >>> on the newly designed media bindings. This is going to be a second > >>> attempt, after our initial RFC from May [1]. It would still be SoC > >>> specific implementation, but not soc-camera based. > >>> > >>> I wasn't a big fan of this asynchronous sub-devices probing, but it now > >>> seems to be a most complete solution to me. I think it just need to be > >>> done right at the v4l2-core so individual drivers don't get complicated > >>> too much. > >> > >> After investigating this some more I think I agree with that. There are some > >> things where we should probably ask for advice from the i2c subsystem devs, > >> I'm thinking of putting the driver back into the deferred-probe state in > >> particular. > > > > We might actually not need that, it might be easier to handle the circular > > dependency problem from the other end. We could add a way (ioctl, sysfs, ...) > > to force a V4L2 bridge driver to release its subdevs. Once done, the subdev > > driver could be unloaded and/or the subdev device unregistered, which would > > release the resources used by the subdev, such as clocks. The bridge driver > > could then be unregistered. > > That sounds like an option. Perhaps it could be done by v4l2-core, e.g. a sysfs > entry could be registered for a media or video device if driver requests it. > I'm not sure if we should allow subdevs in "released" state, perhaps it's better > to just unregister subdevs entirely right away ? What speaks against holding a clock reference only during streaming, or at least between open and close? Wouldn't that solve the problem naturally? Yes, after giving up your reference to a clock at close() and re-acquiring it at open() you will have to make sure the frequency hasn't change, resp. adjust it, but I don't see it as a huge problem, I don't think many users on embedded systems will compete for your camera master clock. And if they do, you have a different problem, IMHO;-) Thanks Guennadi > >> Creating v4l2-core support for this is crucial as it is quite complex and > >> without core support this is going to be a nightmare for drivers. > > -- > > Regards, > Sylwester --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/10/2012 10:32 PM, Guennadi Liakhovetski wrote: >>> We might actually not need that, it might be easier to handle the circular >>> dependency problem from the other end. We could add a way (ioctl, sysfs, ...) >>> to force a V4L2 bridge driver to release its subdevs. Once done, the subdev >>> driver could be unloaded and/or the subdev device unregistered, which would >>> release the resources used by the subdev, such as clocks. The bridge driver >>> could then be unregistered. >> >> That sounds like an option. Perhaps it could be done by v4l2-core, e.g. a sysfs >> entry could be registered for a media or video device if driver requests it. >> I'm not sure if we should allow subdevs in "released" state, perhaps it's better >> to just unregister subdevs entirely right away ? > > What speaks against holding a clock reference only during streaming, or at > least between open and close? Wouldn't that solve the problem naturally? > Yes, after giving up your reference to a clock at close() and re-acquiring > it at open() you will have to make sure the frequency hasn't change, resp. > adjust it, but I don't see it as a huge problem, I don't think many users > on embedded systems will compete for your camera master clock. And if they > do, you have a different problem, IMHO;-) I agree, normally nobody should touch these clocks except the subdev (or as of now the host) drivers. It depends on a sensor, video encoder, etc. how much it tolerates switching the clock on/off. I suppose it's best to acquire/release it in .s_power callback, since only then the proper voltage supply, GPIO, clock enable/disable sequences could be ensured. I know those things are currently mostly ignored, but some sensors might be picky WRT their initialization/shutdown sequences and it would be good to ensure these sequences are fully controllable by the sensor driver itsels, where the host's architecture allows that. To summarize, I can't see how holding a clock only when a device is active could cause any problems, in case of camera sensors. I'm not sure about other devices, like e.g. tuners. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 10 October 2012 10:50:20 Stephen Warren wrote: > On 10/10/2012 07:18 AM, Laurent Pinchart wrote: > > On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: > ... > > >> But how do you get the subdev pointer? With the notifier I get it from > >> i2c_get_clientdata(client) and what do you do without it? How do you get > >> to the client? > >> > >>> And can't it get that from DT as well? > >> > >> No, I don't think there is a way to get a device pointer from a DT node. > > I don't believe there's a generic API for this (although perhaps there > could be), but it can be implemented quite easily. > > For example, on Tegra, the SMMU needs to flip a bit in the AHB register > space in order to enable itself. The SMMU DT node contains a phandle > that points at the AHB DT node. The SMMU driver parses the phandle and > passes the DT node pointer to the AHB driver. The AHB driver looks up > the struct device that was instantiated for that node. See > drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu(). There are a few other > cases of similar code in the kernel, although I can't remember the others! That's a very naive approach, but what about storing the struct device in struct device_node when the device is instantiated ? It's so simple that there's probably a good reason why that hasn't been implemented though.
Hi Sylwester, On Wednesday 10 October 2012 22:23:35 Sylwester Nawrocki wrote: > On 10/10/2012 03:25 PM, Laurent Pinchart wrote: > > On Tuesday 09 October 2012 13:00:24 Hans Verkuil wrote: > >> On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: > >>> On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > >>>> On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > >>>>> I would really like to see more than one user until we add any core > >>>>> code. No that it couldn't be changed afterwards, but it would be nice > >>>>> to ensure the concepts are right and proven in real life. > >>>> > >>>> Unfortunately I don't have any more systems on which I could easily > >>>> enough try this. I've got a beagleboard with a camera, but I don't > >>>> think > >>>> I'm a particularly good candidate for implementing DT support for OMAP3 > >>>> camera drivers;-) Apart from that I've only got soc-camera based > >>>> systems, of which none are _really_ DT-ready... At best I could try an > >>>> i.MX31 based board, but that doesn't have a very well developed .dts > >>>> and > >>>> that would be soc-camera too anyway. > >>> > >>> I certainly wouldn't expect you would do all the job. I mean it would be > >>> good to possibly have some other developers adding device tree support > >>> based on that new bindings and new infrastructure related to them. > > > > As I mentioned in another e-mail, I plan to work on DT support for the > > OMAP3 ISP, but I first need generic clock framework support for OMAP3. > > OK, let's hope it's available soon. I've been told a month and a half ago that v3.7 was a plausible target, let's see. > >>> There have been recently some progress in device tree support for Exynos > >>> SoCs, including common clock framework support and we hope to add FDT > >>> support to the Samsung SoC camera devices during this kernel cycle, > >>> based on the newly designed media bindings. This is going to be a second > >>> attempt, after our initial RFC from May [1]. It would still be SoC > >>> specific implementation, but not soc-camera based. > >>> > >>> I wasn't a big fan of this asynchronous sub-devices probing, but it now > >>> seems to be a most complete solution to me. I think it just need to be > >>> done right at the v4l2-core so individual drivers don't get complicated > >>> too much. > >> > >> After investigating this some more I think I agree with that. There are > >> some things where we should probably ask for advice from the i2c > >> subsystem devs, I'm thinking of putting the driver back into the > >> deferred-probe state in particular. > > > > We might actually not need that, it might be easier to handle the circular > > dependency problem from the other end. We could add a way (ioctl, sysfs, > > ...) to force a V4L2 bridge driver to release its subdevs. Once done, the > > subdev driver could be unloaded and/or the subdev device unregistered, > > which would release the resources used by the subdev, such as clocks. The > > bridge driver could then be unregistered. > > That sounds like an option. Perhaps it could be done by v4l2-core, e.g. a > sysfs entry could be registered for a media or video device if driver > requests it. That's what I was thinking about. > I'm not sure if we should allow subdevs in "released" state, perhaps it's > better to just unregister subdevs entirely right away ? I think we need three states: not created, unbound and bound (names are not set in stone). The not created state corresponds to a subdev that hasn't been created yet by its I2C/SPI/whatever driver (agreed, it's not really a state technically). Upon creation the subdev is not bound to any bridge driver. It later gets bound when the bridge driver requests the subdev through the API the V4L2 core will provide (most probably using notifiers). The new sysfs entry would be used to unbind subdevs (either selectively, or all in one go) from the bridge, in which case they will go back to the unbound state, allowing driver removal or device release. > >> Creating v4l2-core support for this is crucial as it is quite complex and > >> without core support this is going to be a nightmare for drivers.
Hi Guennadi, On Wednesday 10 October 2012 22:32:22 Guennadi Liakhovetski wrote: > On Wed, 10 Oct 2012, Sylwester Nawrocki wrote: > > On 10/10/2012 03:25 PM, Laurent Pinchart wrote: > > > On Tuesday 09 October 2012 13:00:24 Hans Verkuil wrote: > > >> On Tue 9 October 2012 12:34:48 Sylwester Nawrocki wrote: > > >>> On 10/08/2012 11:40 AM, Guennadi Liakhovetski wrote: > > >>>> On Fri, 5 Oct 2012, Sylwester Nawrocki wrote: > > >>>>> I would really like to see more than one user until we add any core > > >>>>> code. No that it couldn't be changed afterwards, but it would be > > >>>>> nice to ensure the concepts are right and proven in real life. > > >>>> > > >>>> Unfortunately I don't have any more systems on which I could easily > > >>>> enough try this. I've got a beagleboard with a camera, but I don't > > >>>> think I'm a particularly good candidate for implementing DT support > > >>>> for OMAP3 camera drivers;-) Apart from that I've only got soc-camera > > >>>> based systems, of which none are _really_ DT-ready... At best I could > > >>>> try an i.MX31 based board, but that doesn't have a very well > > >>>> developed .dts and that would be soc-camera too anyway. > > >>> > > >>> I certainly wouldn't expect you would do all the job. I mean it would > > >>> be good to possibly have some other developers adding device tree > > >>> support based on that new bindings and new infrastructure related to > > >>> them. > > > > > > As I mentioned in another e-mail, I plan to work on DT support for the > > > OMAP3 ISP, but I first need generic clock framework support for OMAP3. > > > > OK, let's hope it's available soon. > > > > >>> There have been recently some progress in device tree support for > > >>> Exynos SoCs, including common clock framework support and we hope to > > >>> add FDT support to the Samsung SoC camera devices during this kernel > > >>> cycle, based on the newly designed media bindings. This is going to be > > >>> a second attempt, after our initial RFC from May [1]. It would still > > >>> be SoC specific implementation, but not soc-camera based. > > >>> > > >>> I wasn't a big fan of this asynchronous sub-devices probing, but it > > >>> now seems to be a most complete solution to me. I think it just need > > >>> to be done right at the v4l2-core so individual drivers don't get > > >>> complicated too much. > > >> > > >> After investigating this some more I think I agree with that. There are > > >> some things where we should probably ask for advice from the i2c > > >> subsystem devs, I'm thinking of putting the driver back into the > > >> deferred-probe state in particular. > > > > > > We might actually not need that, it might be easier to handle the > > > circular dependency problem from the other end. We could add a way > > > (ioctl, sysfs, ...) to force a V4L2 bridge driver to release its > > > subdevs. Once done, the subdev driver could be unloaded and/or the > > > subdev device unregistered, which would release the resources used by > > > the subdev, such as clocks. The bridge driver could then be > > > unregistered. > > > > That sounds like an option. Perhaps it could be done by v4l2-core, e.g. a > > sysfs entry could be registered for a media or video device if driver > > requests it. I'm not sure if we should allow subdevs in "released" state, > > perhaps it's better to just unregister subdevs entirely right away ? > > What speaks against holding a clock reference only during streaming, or at > least between open and close? Wouldn't that solve the problem naturally? > Yes, after giving up your reference to a clock at close() and re-acquiring > it at open() you will have to make sure the frequency hasn't change, resp. > adjust it, but I don't see it as a huge problem, I don't think many users > on embedded systems will compete for your camera master clock. And if they > do, you have a different problem, IMHO;-) That's an option as well. I'm a bit worried that it would make subdev drivers more complex, as they would need to acquire/release resources in a more fine- grained fashion at runtime, and handle failures dynamically there instead of doing it all at probe time. It could work though, I think we need to experiment the possible solutions to find out which one is the best. Regardless of how we solve the circular dependencies issue at unregistration time we will need an easy way for bridge drivers to bind to subdevs. I believe that's orthogonal to the unregistration problem, so we can start working on registration before knowing exactly how unregistration will be handled. > > >> Creating v4l2-core support for this is crucial as it is quite complex > > >> and without core support this is going to be a nightmare for drivers.
On 10/10/2012 04:51 PM, Laurent Pinchart wrote: > On Wednesday 10 October 2012 10:50:20 Stephen Warren wrote: >> On 10/10/2012 07:18 AM, Laurent Pinchart wrote: >>> On Monday 08 October 2012 17:15:53 Guennadi Liakhovetski wrote: >> ... >> >>>> But how do you get the subdev pointer? With the notifier I get it from >>>> i2c_get_clientdata(client) and what do you do without it? How do you get >>>> to the client? >>>> >>>>> And can't it get that from DT as well? >>>> >>>> No, I don't think there is a way to get a device pointer from a DT node. >> >> I don't believe there's a generic API for this (although perhaps there >> could be), but it can be implemented quite easily. >> >> For example, on Tegra, the SMMU needs to flip a bit in the AHB register >> space in order to enable itself. The SMMU DT node contains a phandle >> that points at the AHB DT node. The SMMU driver parses the phandle and >> passes the DT node pointer to the AHB driver. The AHB driver looks up >> the struct device that was instantiated for that node. See >> drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu(). There are a few other >> cases of similar code in the kernel, although I can't remember the others! > > That's a very naive approach, but what about storing the struct device in > struct device_node when the device is instantiated ? It's so simple that > there's probably a good reason why that hasn't been implemented though. It sounds like that would work. The advantage of calling a function in the driver for the node, rather than just grabbing something from the node directly in code unrelated to the driver for the node, is that it any knowledge of what kind of device/driver is handling that node is embedded into a function in the driver for the node, not the driver using the node, which makes everything a bit more type-safe. Now, perhaps the implementation of that function could just pull a field out of struct of_node rather than calling driver_find_device(), but it'd then have to validate that the struct device that was found was truly managed by the expected driver, for safety. I assume that's pretty simple, but haven't checked. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Mon, Oct 08, 2012 at 02:23:25PM +0200, Guennadi Liakhovetski wrote: ... > If we do it from the bridge driver, we could install an I2C bus-notifier, > _before_ the subdevice driver is probed, i.e. upon the > BUS_NOTIFY_BIND_DRIVER event we could turn on the clock. If subdevice > probing was successful, we can then wait for the BUS_NOTIFY_BOUND_DRIVER > event to switch the clock back off. BUT - if the subdevice fails probing? > How do we find out about that and turn the clock back off? There is no > notification event for that... Possible solutions: > > 1. timer - ugly and unreliable. > 2. add a "probing failed" notifier event to the device core - would this > be accepted? > 3. let the subdevice turn the master clock on and off for the duration of > probing. > > My vote goes for (3). Ideally this should be done using the generic clock > framework. But can we really expect all drivers and platforms to switch to > it quickly enough? If not, we need a V4L2-specific callback from subdevice > drivers to bridge drivers to turn the clock on and off. That's what I've > done "temporarily" in this patch series for soc-camera. I'd say the clock has to be controlled by the sub-device driver. Sensors also have a power-up (and power-down) sequences that should be followed. Usually they also involve switching the external clock on (and off) at a given point of time. Also the OMAP 3 provides that clock through ISP so it, too, requires the generic clock framework to function with DT. > Suppose we decide to do the same for V4L2 centrally - add call-backs. Then I'd prefer to use the generic clock framework, albeit I admit it may take some time till all the relevant platforms support it. Nevertheless, if there's going to be a temporary solution it should be removed once the clock framework support is there. Kind regards,
Hi Bearing in mind the local time, I hope my brevity will be excused:-) Just wanted to give a sign, that I just finished a first successful run of a fully asynchronous and uniform multi-component non-DT video (soc-camera) pipeline probing, verified by a sample capture. What this means: The pipeline consists of 3 components: a sensor, a CSI-2 interface, and a bridge. The platform code registers 3 devices: 2 platform devices for the bridge and CSI-2 and an I2C device for the sensor. The bridge driver gets a list of device descriptors, which it passes to the (soc-camera) generic code. That is used to initialise internal data and install bus (platform- and i2c-) notifiers. After all components have been collected the normal probing continues. Next week I'll (hopefully) be cleaning all this up and converting from soc-camera to V4L2 core... After that I'll start posting, beginning with v2 of this DT series, taking into account possibly many comments:-) I think, it might make sense to first post and have a look at the purely asynchronous code, first without DT additions, that we planned to implement for bus notifiers and whatever is related to them. If that looks reasonable, we can move on with adding DT. (One of) the ugly part(s) is going to be, that with this we'll be supporting 3 (!) pipeline initialisation modes: current (legacy) mode, generating I2C devices on the fly, non-DT asynchronous with all devices probing independently, and DT... Let's see if all this actually flies. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile index c2d61d4..00f64d6 100644 --- a/drivers/media/v4l2-core/Makefile +++ b/drivers/media/v4l2-core/Makefile @@ -9,6 +9,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ ifeq ($(CONFIG_COMPAT),y) videodev-objs += v4l2-compat-ioctl32.o endif +ifeq ($(CONFIG_OF),y) + videodev-objs += v4l2-of.o +endif obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c new file mode 100644 index 0000000..f45d64b --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-of.c @@ -0,0 +1,190 @@ +/* + * V4L2 OF binding parsing library + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski <g.liakhovetski@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. + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/types.h> + +#include <media/v4l2-of.h> + +/* + * All properties are optional. If none are found, we don't set any flags. This + * means, the port has a static configuration and no properties have to be + * specified explicitly. + * If any properties are found, that identify the bus as parallel, and + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise the + * bus as serial CSI-2 and clock-noncontinuous isn't set, we set the + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag. + * The caller should hold a reference to "node." + */ +void v4l2_of_parse_link(const struct device_node *node, + struct v4l2_of_link *link) +{ + const struct device_node *port_node = of_get_parent(node); + int size; + unsigned int v; + u32 data_lanes[ARRAY_SIZE(link->mipi_csi_2.data_lanes)]; + bool data_lanes_present; + + memset(link, 0, sizeof(*link)); + + link->local_node = node; + + /* Doesn't matter, whether the below two calls succeed */ + of_property_read_u32(port_node, "reg", &link->port); + of_property_read_u32(node, "reg", &link->addr); + + if (!of_property_read_u32(node, "bus-width", &v)) + link->parallel.bus_width = v; + + if (!of_property_read_u32(node, "data-shift", &v)) + link->parallel.data_shift = v; + + if (!of_property_read_u32(node, "hsync-active", &v)) + link->mbus_flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH : + V4L2_MBUS_HSYNC_ACTIVE_LOW; + + if (!of_property_read_u32(node, "vsync-active", &v)) + link->mbus_flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH : + V4L2_MBUS_VSYNC_ACTIVE_LOW; + + if (!of_property_read_u32(node, "data-active", &v)) + link->mbus_flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH : + V4L2_MBUS_DATA_ACTIVE_LOW; + + if (!of_property_read_u32(node, "pclk-sample", &v)) + link->mbus_flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING : + V4L2_MBUS_PCLK_SAMPLE_FALLING; + + if (!of_property_read_u32(node, "field-even-active", &v)) + link->mbus_flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH : + V4L2_MBUS_FIELD_EVEN_LOW; + + if (of_get_property(node, "slave-mode", &size)) + link->mbus_flags |= V4L2_MBUS_SLAVE; + + /* If any parallel-bus properties have been found, skip serial ones */ + if (link->parallel.bus_width || link->parallel.data_shift || + link->mbus_flags) { + /* Default parallel bus-master */ + if (!(link->mbus_flags & V4L2_MBUS_SLAVE)) + link->mbus_flags |= V4L2_MBUS_MASTER; + return; + } + + if (!of_property_read_u32(node, "clock-lanes", &v)) + link->mipi_csi_2.clock_lane = v; + + if (!of_property_read_u32_array(node, "data-lanes", data_lanes, + ARRAY_SIZE(data_lanes))) { + int i; + for (i = 0; i < ARRAY_SIZE(data_lanes); i++) + link->mipi_csi_2.data_lanes[i] = data_lanes[i]; + data_lanes_present = true; + } else { + data_lanes_present = false; + } + + if (of_get_property(node, "clock-noncontinuous", &size)) + link->mbus_flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; + + if ((link->mipi_csi_2.clock_lane || data_lanes_present) && + !(link->mbus_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) + /* Default CSI-2: continuous clock */ + link->mbus_flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK; +} +EXPORT_SYMBOL(v4l2_of_parse_link); + +/* + * Return a refcounted next "link" DT node. Contrary to the common OF practice, + * we do not drop the reference to previous, users have to do it themselves, + * when they're done with the node. + */ +struct device_node *v4l2_of_get_next_link(const struct device_node *parent, + struct device_node *previous) +{ + struct device_node *child, *port; + + if (!parent) + return NULL; + + if (!previous) { + /* + * If this is the first call, we have to find a port within this + * node + */ + for_each_child_of_node(parent, port) { + if (!of_node_cmp(port->name, "port")) + break; + } + if (port) { + /* Found a port, get a link */ + child = of_get_next_child(port, NULL); + of_node_put(port); + } else { + child = NULL; + } + if (!child) + pr_err("%s(): Invalid DT: %s has no link children!\n", + __func__, parent->name); + } else { + port = of_get_parent(previous); + if (!port) + /* Hm, has someone given us the root node?... */ + return NULL; + + /* Avoid dropping previous refcount to 0 */ + of_node_get(previous); + child = of_get_next_child(port, previous); + if (child) { + of_node_put(port); + return child; + } + + /* No more links under this port, try the next one */ + do { + port = of_get_next_child(parent, port); + if (!port) + return NULL; + } while (of_node_cmp(port->name, "port")); + + /* Pick up the first link on this port */ + child = of_get_next_child(port, NULL); + of_node_put(port); + } + + return child; +} +EXPORT_SYMBOL(v4l2_of_get_next_link); + +/* Return a refcounted DT node, owning the link, referenced by "remote" */ +struct device_node *v4l2_of_get_remote(const struct device_node *node) +{ + struct device_node *remote, *tmp; + + /* Get remote link DT node */ + remote = of_parse_phandle(node, "remote", 0); + if (!remote) + return NULL; + + /* remote port */ + tmp = of_get_parent(remote); + of_node_put(remote); + if (!tmp) + return NULL; + + /* remote DT node */ + remote = of_get_parent(tmp); + of_node_put(tmp); + + return remote; +} +EXPORT_SYMBOL(v4l2_of_get_remote); diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h new file mode 100644 index 0000000..6fafedb --- /dev/null +++ b/include/media/v4l2-of.h @@ -0,0 +1,62 @@ +/* + * V4L2 OF binding parsing library + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski <g.liakhovetski@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 _V4L2_OF_H +#define _V4L2_OF_H + +#include <linux/list.h> +#include <linux/types.h> + +#include <media/v4l2-mediabus.h> + +struct device_node; + +struct v4l2_of_link { + unsigned int port; + unsigned int addr; + struct list_head head; + const struct device_node *local_node; + const __be32 *remote; + unsigned int mbus_flags; + union { + struct { + unsigned char bus_width; + unsigned char data_shift; + } parallel; + struct { + unsigned char data_lanes[4]; + unsigned char clock_lane; + } mipi_csi_2; + }; +}; + +#ifdef CONFIG_OF +void v4l2_of_parse_link(const struct device_node *node, + struct v4l2_of_link *link); +struct device_node *v4l2_of_get_next_link(const struct device_node *parent, + struct device_node *previous); +struct device_node *v4l2_of_get_remote(const struct device_node *node); +#else +static inline void v4l2_of_parse_link(const struct device_node *node, + struct v4l2_of_link *link) +{ +} +static inline struct device_node *v4l2_of_get_next_link(const struct device_node *parent, + struct device_node *previous) +{ + return NULL; +} +static inline struct device_node *v4l2_of_get_remote(const struct device_node *node) +{ + return NULL; +} +#endif + +#endif
Add a V4L2 OF parser, implementing bindings, documented in Documentation/devicetree/bindings/media/v4l2.txt. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- drivers/media/v4l2-core/Makefile | 3 + drivers/media/v4l2-core/v4l2-of.c | 190 +++++++++++++++++++++++++++++++++++++ include/media/v4l2-of.h | 62 ++++++++++++ 3 files changed, 255 insertions(+), 0 deletions(-) create mode 100644 drivers/media/v4l2-core/v4l2-of.c create mode 100644 include/media/v4l2-of.h