Hi Philipp, On Tuesday 07 Feb 2017 11:41:30 Philipp Zabel wrote: > On Tue, 2017-02-07 at 12:26 +0200, Laurent Pinchart wrote: > > 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. If the CSI2IPU gasket has four physical output buses then it can be modelled as an entity with four source pads. Only when VC/DT are multiplexed on the same physical bus do I think they should be handled without involving the device tree and the media controller graph. -- Regards, Laurent Pinchart -- 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
