On Tue, 2017-02-07 at 12:26 +0200, Laurent Pinchart wrote: > Hi Steve, > > On Monday 06 Feb 2017 15:10:46 Steve Longerbeam wrote: > > On 02/06/2017 02:33 PM, Laurent Pinchart wrote: > > > On Monday 06 Feb 2017 10:50:22 Hans Verkuil wrote: > > >> On 02/05/2017 04:48 PM, Laurent Pinchart wrote: > > >>> On Tuesday 24 Jan 2017 18:07:55 Steve Longerbeam wrote: > > >>>> On 01/24/2017 04:02 AM, Philipp Zabel wrote: > > >>>>> On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote: > > >>>>>>> + > > >>>>>>> +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct > > >>>>>>> v4l2_mbus_config *cfg) > > [snip] > > > >>>>>> I am not certain this op is needed at all. In the current kernel this > > >>>>>> op is only used by soc_camera, pxa_camera and omap3isp (somewhat > > >>>>>> dubious). Normally this information should come from the device tree > > >>>>>> and there should be no need for this op. > > >>>>>> > > >>>>>> My (tentative) long-term plan was to get rid of this op. > > >>>>>> > > >>>>>> If you don't need it, then I recommend it is removed. > > >>>> > > >>>> Hi Hans, the imx-media driver was only calling g_mbus_config to the > > >>>> camera sensor, and it was doing that to determine the sensor's bus > > >>>> type. This info was already available from parsing a v4l2_of_endpoint > > >>>> from the sensor node. So it was simple to remove the g_mbus_config > > >>>> calls, and instead rely on the parsed sensor v4l2_of_endpoint. > > >>> > > >>> That's not a good point. > > > > > > (mea culpa, s/point/idea/) > > > > > >>> The imx-media driver must not parse the sensor DT node as it is not > > >>> aware of what bindings the sensor is compatible with. > > > > Hi Laurent, > > > > I don't really understand this argument. The sensor node has been found > > by parsing the OF graph, so it is known to be a camera sensor node at > > that point. > > All you know in the i.MX6 driver is that the remote node is a video source. > You can rely on the fact that it implements the OF graph bindings to locate > other ports in that DT node, but that's more or less it. > > DT properties are defined by DT bindings and thus qualified by a compatible > string. Unless you match on sensor compat strings in the i.MX6 driver (which > you shouldn't do, to keep the driver generic) you can't know for certain how > to parse the sensor node DT properties. For all you know, the video source > could be a bridge such as an HDMI to CSI-2 converter for instance, so you > can't even rely on the fact that it's a sensor. > > > >>> Information must instead be queried from the sensor subdev at runtime, > > >>> through the g_mbus_config() operation. > > >>> > > >>> Of course, if you can get the information from the imx-media DT node, > > >>> that's certainly an option. It's only information provided by the sensor > > >>> driver that you have no choice but query using a subdev operation. > > >> > > >> Shouldn't this come from the imx-media DT node? BTW, why is omap3isp > > >> using this? > > > > > > It all depends on what type of information needs to be retrieved, and > > > whether it can change at runtime or is fixed. Adding properties to the > > > imx-media DT node is certainly fine as long as those properties describe > > > the i.MX side. > > > > In this case the info needed is the media bus type. That info is most easily > > available by calling v4l2_of_parse_endpoint() on the sensor's endpoint > > node. > > I haven't had time to check the code in details yet, so I can't really comment > on what you need and how it should be implemented exactly. > > > The media bus type is not something that can be added to the > > imx-media node since it contains no endpoint nodes. > > Agreed. You have endpoints in the CSI nodes though. > > > > In the omap3isp case, we use the operation to query whether parallel data > > > contains embedded sync (BT.656) or uses separate h/v sync signals. > > > > > >> The reason I am suspicious about this op is that it came from soc-camera > > >> and predates the DT. The contents of v4l2_mbus_config seems very much > > >> like a HW description to me, i.e. something that belongs in the DT. > > > > > > Part of it is possibly outdated, but for buses that support multiple modes > > > of operation (such as the parallel bus case described above) we need to > > > make that information discoverable at runtime. Maybe this should be > > > considered as related to Sakari's efforts to support VC/DT for CSI-2, and > > > supported through the API he is working on. > > > > That sounds interesting, can you point me to some info on this effort? > > Sure. > > http://git.retiisi.org.uk/?p=~sailus/linux.git;a=shortlog;h=refs/heads/vc > > > I've been thinking the DT should contain virtual channel info for CSI-2 > > buses. > > I don't think it should. CSI-2 virtual channels and data types should be > handled as a software concept, and thus supported through driver code without > involving DT. I agree. The CSI2IPU gasket is a bit special in that it distributes its input data to four different parallel buses depending on the input's VC, but upstream of the MIPI CSI-2 receiver, any virtual channel information is purely a matter of the data sent over the CSI-2 link, and not board specific hardware description. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
