Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

Hi Philipp,

On Wednesday 08 Feb 2017 10:47:15 Philipp Zabel wrote:
> On Tue, 2017-02-07 at 22:46 +0200, Sakari Ailus wrote:
> > On Fri, Jan 06, 2017 at 06:11:31PM -0800, Steve Longerbeam wrote:
> >> From: Philipp Zabel 
> >> 
> >> This driver can handle SoC internal and external video bus multiplexers,
> >> controlled either by register bit fields or by a GPIO. The subdevice
> >> passes through frame interval and mbus configuration of the active input
> >> to the output side.
> >> 
> >> Signed-off-by: Sascha Hauer 
> >> Signed-off-by: Philipp Zabel 
> >> 
> >> --
> >> 
> >> - fixed a cut error in vidsw_remove():
> >> v4l2_async_register_subdev()
> >>   should be unregister.
> >> 
> >> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
> >>   to vidsw_remove().
> >> 
> >> - there was a line left over from a previous iteration that negated
> >>   the new way of determining the pad count just before it which
> >>   has been removed (num_pads = of_get_child_count(np)).
> >> 
> >> - Philipp Zabel has developed a set of patches that allow adding
> >>   to the subdev async notifier waiting list using a chaining method
> >>   from the async registered callbacks (v4l2_of_subdev_registered()
> >>   and the prep patches for that). For now, I've removed the use of
> >>   v4l2_of_subdev_registered() for the vidmux driver's registered
> >>   callback. This doesn't affect the functionality of this driver,
> >>   but allows for it to be merged now, before adding the chaining
> >>   support.
> >> 
> >> Signed-off-by: Steve Longerbeam 
> >> ---
> >> 
> >>  .../bindings/media/video-multiplexer.txt   |  59 +++
> >>  drivers/media/platform/Kconfig |   8 +
> >>  drivers/media/platform/Makefile|   2 +
> >>  drivers/media/platform/video-multiplexer.c | 472 ++
> >>  4 files changed, 541 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/media/video-multiplexer.txt create
> >>  mode 100644 drivers/media/platform/video-multiplexer.c
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/media/video-multiplexer.txt
> >> b/Documentation/devicetree/bindings/media/video-multiplexer.txt new
> >> file mode 100644
> >> index 000..9d133d9
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> >> @@ -0,0 +1,59 @@
> >> +Video Multiplexer
> >> +=
> >> +
> >> +Video multiplexers allow to select between multiple input ports. Video
> >> received
> >> +on the active input port is passed through to the output port. Muxes
> >> described
> >> +by this binding may be controlled by a syscon register bitfield or by a
> >> GPIO.
> >> +
> >> +Required properties:
> >> +- compatible : should be "video-multiplexer"
> >> +- reg: should be register base of the register containing the control
> >> bitfield
> >> +- bit-mask: bitmask of the control bitfield in the control register
> >> +- bit-shift: bit offset of the control bitfield in the control register
> >> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO
> >> phandle
> >> +  may be given to switch between two inputs
> >> +- #address-cells: should be <1>
> >> +- #size-cells: should be <0>
> >> +- port@*: at least three port nodes containing endpoints connecting to
> >> the
> >> +  source and sink devices according to of_graph bindings. The last port
> >> is
> >> +  the output port, all others are inputs.
> >> +
> >> +Example:
> >> +
> >> +syscon {
> >> +  compatible = "syscon", "simple-mfd";
> >> +
> >> +  mux {
> > 
> > Could you use standardised properties for this, i.e. ones defined in
> > Documentation/devicetree/bindings/media/video-interfaces.txt ?
> 
> What do you mean? Should we add the optional bus-width property to
> describe the bit width of the parallel bus even though the driver
> doesn't care about any of the properties?
> 
> > This is very similar to another patch "[PATCH] devicetree: Add video bus
> > switch" posted by Pavel Machek recently. The problem with that is also
> > similar than with this one: how to pass the CSI-2 bus configuration to the
> > receiver.
> > 
> > There's some discussion here:
> > 
> > 
> 
> [Added Sebastian do Cc:]
> 
> Yes, this is essentially the same driver, except that this driver also
> handles MMIO-bitfield controlled muxes, and that the actual physical bus
> (which the drivers currently don't care about) is MIPI CSI-2 in the
> other case, and parallel this one.
> They should probably be combined, or maybe split into two separate
> drivers (MMIO controlled mux, GPIO controlled switch), possibly using
> the same v4l2_subdev boilerplate.

I'd split the bindings in two with two separate compat strings, but we can 
handle both in a single driver as most of the code would be identical 
otherwise.

> > As 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

Hi Sakari,

On Tue, 2017-02-07 at 22:46 +0200, Sakari Ailus wrote:
> Hi Steve,
> 
> On Fri, Jan 06, 2017 at 06:11:31PM -0800, Steve Longerbeam wrote:
> > From: Philipp Zabel 
> > 
> > This driver can handle SoC internal and external video bus multiplexers,
> > controlled either by register bit fields or by a GPIO. The subdevice
> > passes through frame interval and mbus configuration of the active input
> > to the output side.
> > 
> > Signed-off-by: Sascha Hauer 
> > Signed-off-by: Philipp Zabel 
> > 
> > --
> > 
> > - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
> >   should be unregister.
> > 
> > - added media_entity_cleanup() and v4l2_device_unregister_subdev()
> >   to vidsw_remove().
> > 
> > - there was a line left over from a previous iteration that negated
> >   the new way of determining the pad count just before it which
> >   has been removed (num_pads = of_get_child_count(np)).
> > 
> > - Philipp Zabel has developed a set of patches that allow adding
> >   to the subdev async notifier waiting list using a chaining method
> >   from the async registered callbacks (v4l2_of_subdev_registered()
> >   and the prep patches for that). For now, I've removed the use of
> >   v4l2_of_subdev_registered() for the vidmux driver's registered
> >   callback. This doesn't affect the functionality of this driver,
> >   but allows for it to be merged now, before adding the chaining
> >   support.
> > 
> > Signed-off-by: Steve Longerbeam 
> > ---
> >  .../bindings/media/video-multiplexer.txt   |  59 +++
> >  drivers/media/platform/Kconfig |   8 +
> >  drivers/media/platform/Makefile|   2 +
> >  drivers/media/platform/video-multiplexer.c | 472 
> > +
> >  4 files changed, 541 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/media/video-multiplexer.txt
> >  create mode 100644 drivers/media/platform/video-multiplexer.c
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> > b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > new file mode 100644
> > index 000..9d133d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > @@ -0,0 +1,59 @@
> > +Video Multiplexer
> > +=
> > +
> > +Video multiplexers allow to select between multiple input ports. Video 
> > received
> > +on the active input port is passed through to the output port. Muxes 
> > described
> > +by this binding may be controlled by a syscon register bitfield or by a 
> > GPIO.
> > +
> > +Required properties:
> > +- compatible : should be "video-multiplexer"
> > +- reg: should be register base of the register containing the control 
> > bitfield
> > +- bit-mask: bitmask of the control bitfield in the control register
> > +- bit-shift: bit offset of the control bitfield in the control register
> > +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO 
> > phandle
> > +  may be given to switch between two inputs
> > +- #address-cells: should be <1>
> > +- #size-cells: should be <0>
> > +- port@*: at least three port nodes containing endpoints connecting to the
> > +  source and sink devices according to of_graph bindings. The last port is
> > +  the output port, all others are inputs.
> > +
> > +Example:
> > +
> > +syscon {
> > +   compatible = "syscon", "simple-mfd";
> > +
> > +   mux {
> 
> Could you use standardised properties for this, i.e. ones defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt ?

What do you mean? Should we add the optional bus-width property to
describe the bit width of the parallel bus even though the driver
doesn't care about any of the properties?

> This is very similar to another patch "[PATCH] devicetree: Add video bus
> switch" posted by Pavel Machek recently. The problem with that is also
> similar than with this one: how to pass the CSI-2 bus configuration to the
> receiver.
>
> There's some discussion here:
> 
> 

[Added Sebastian do Cc:]

Yes, this is essentially the same driver, except that this driver also
handles MMIO-bitfield controlled muxes, and that the actual physical bus
(which the drivers currently don't care about) is MIPI CSI-2 in the
other case, and parallel this one.
They should probably be combined, or maybe split into two separate
drivers (MMIO controlled mux, GPIO controlled switch), possibly using
the same v4l2_subdev boilerplate.

> As Laurent already suggested, I think we should have a common solution for
> the problem that, besides conveying the bus parameters to the receiver, also
> encompasses CSI-2 virtual channels and data types.

Yes, that seems to be necessary, as certainly we can't configure the mux
output bus parameters in DT to a fixed setting.

> That would mean 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

Hi Benoit,

On Tuesday 07 Feb 2017 07:36:48 Benoit Parrot wrote:
> Laurent Pinchart wrote on Tue [2017-Feb-07 12:26:32 +0200]:
> > 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 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Sakari Ailus]

Hi Benoit,

On Tue, Feb 07, 2017 at 07:36:48AM -0600, Benoit Parrot wrote:
> Laurent Pinchart  wrote on Tue 
> [2017-Feb-07 12:26:32 +0200]:
> > 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 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Sakari Ailus]

Hi Steve,

On Fri, Jan 06, 2017 at 06:11:31PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> 
> --
> 
> - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
>   to vidsw_remove().
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  .../bindings/media/video-multiplexer.txt   |  59 +++
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 472 
> +
>  4 files changed, 541 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/video-multiplexer.txt
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> new file mode 100644
> index 000..9d133d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> @@ -0,0 +1,59 @@
> +Video Multiplexer
> +=
> +
> +Video multiplexers allow to select between multiple input ports. Video 
> received
> +on the active input port is passed through to the output port. Muxes 
> described
> +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> +
> +Required properties:
> +- compatible : should be "video-multiplexer"
> +- reg: should be register base of the register containing the control 
> bitfield
> +- bit-mask: bitmask of the control bitfield in the control register
> +- bit-shift: bit offset of the control bitfield in the control register
> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> +  may be given to switch between two inputs
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
> +
> +Example:
> +
> +syscon {
> + compatible = "syscon", "simple-mfd";
> +
> + mux {

Could you use standardised properties for this, i.e. ones defined in
Documentation/devicetree/bindings/media/video-interfaces.txt ?

This is very similar to another patch "[PATCH] devicetree: Add video bus
switch" posted by Pavel Machek recently. The problem with that is also
similar than with this one: how to pass the CSI-2 bus configuration to the
receiver.

There's some discussion here:



As Laurent already suggested, I think we should have a common solution for
the problem that, besides conveying the bus parameters to the receiver, also
encompasses CSI-2 virtual channels and data types.

That would mean finishing the series of patches in the branch I believe
Laurent already quoted here.

> + compatible = "video-multiplexer";
> + /* Single bit (1 << 19) in syscon register 0x04: */
> + reg = <0x04>;
> + bit-mask = <1>;
> + bit-shift = <19>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + mux_in0: endpoint {
> + remote-endpoint = <_source0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + mux_in1: endpoint {
> + remote-endpoint = <_source1_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + mux_out: endpoint {
> + remote-endpoint = <_interface_in>;
> +  

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Benoit Parrot]

Laurent Pinchart  wrote on Tue [2017-Feb-07 
12:26:32 +0200]:
> 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 
> 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

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 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

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 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

On 02/06/2017 02:33 PM, Laurent Pinchart wrote:

Hi Hans,

(CC'ing Sakari)

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)
+{
+   struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
+   struct media_pad *pad;
+   int ret;
+
+   if (vidsw->active == -1) {
+   dev_err(sd->dev, "no configuration for inactive

mux\n");

+   return -EINVAL;
+   }
+
+   /*
+* Retrieve media bus configuration from the entity connected

to the

+* active input
+*/
+   pad = media_entity_remote_pad(>pads[vidsw->active]);
+   if (pad) {
+   sd = media_entity_to_v4l2_subdev(pad->entity);
+   ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
+   if (ret == -ENOIOCTLCMD)
+   pad = NULL;
+   else if (ret < 0) {
+   dev_err(sd->dev, "failed to get source
configuration\n");
+   return ret;
+   }
+   }
+   if (!pad) {
+   /* Mirror the input side on the output side */
+   cfg->type = vidsw->endpoint[vidsw->active].bus_type;
+   if (cfg->type == V4L2_MBUS_PARALLEL ||
+   cfg->type == V4L2_MBUS_BT656)
+   cfg->flags = vidsw->endpoint[vidsw-
active].bus.parallel.flags;
+   }
+
+   return 0;
+}

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.



  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.

The media bus type is not something that can be added to the
imx-media node since it contains no endpoint nodes.



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?
I've been thinking the DT should contain virtual channel info for CSI-2
buses.

Steve





___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

Hi Hans,

(CC'ing Sakari)

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)
> > +{
> > +   struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> > +   struct media_pad *pad;
> > +   int ret;
> > +
> > +   if (vidsw->active == -1) {
> > +   dev_err(sd->dev, "no configuration for inactive 
mux\n");
> > +   return -EINVAL;
> > +   }
> > +
> > +   /*
> > +* Retrieve media bus configuration from the entity connected 
to the
> > +* active input
> > +*/
> > +   pad = media_entity_remote_pad(>pads[vidsw->active]);
> > +   if (pad) {
> > +   sd = media_entity_to_v4l2_subdev(pad->entity);
> > +   ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> > +   if (ret == -ENOIOCTLCMD)
> > +   pad = NULL;
> > +   else if (ret < 0) {
> > +   dev_err(sd->dev, "failed to get source
> > configuration\n");
> > +   return ret;
> > +   }
> > +   }
> > +   if (!pad) {
> > +   /* Mirror the input side on the output side */
> > +   cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> > +   if (cfg->type == V4L2_MBUS_PARALLEL ||
> > +   cfg->type == V4L2_MBUS_BT656)
> > +   cfg->flags = vidsw->endpoint[vidsw-
> > active].bus.parallel.flags;
> > +   }
> > +
> > +   return 0;
> > +}
>  
>  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. 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 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.

-- 
Regards,

Laurent Pinchart

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Hans Verkuil]

On 02/05/2017 04:48 PM, Laurent Pinchart wrote:
> Hi Steve,
> 
> 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)
> +{
> + struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> + struct media_pad *pad;
> + int ret;
> +
> + if (vidsw->active == -1) {
> + dev_err(sd->dev, "no configuration for inactive mux\n");
> + return -EINVAL;
> + }
> +
> + /*
> +  * Retrieve media bus configuration from the entity connected to the
> +  * active input
> +  */
> + pad = media_entity_remote_pad(>pads[vidsw->active]);
> + if (pad) {
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> + if (ret == -ENOIOCTLCMD)
> + pad = NULL;
> + else if (ret < 0) {
> + dev_err(sd->dev, "failed to get source 
> configuration\n");
> + return ret;
> + }
> + }
> + if (!pad) {
> + /* Mirror the input side on the output side */
> + cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> + if (cfg->type == V4L2_MBUS_PARALLEL ||
> + cfg->type == V4L2_MBUS_BT656)
> + cfg->flags = vidsw->endpoint[vidsw-
>> active].bus.parallel.flags;
> + }
> +
> + return 0;
> +}

 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. The imx-media driver must not parse the sensor DT 
> node as it is not aware of what bindings the sensor is compatible with. 
> 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?

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.

Regards,

Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

Hi Steve,

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)
> >>> +{
> >>> + struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> >>> + struct media_pad *pad;
> >>> + int ret;
> >>> +
> >>> + if (vidsw->active == -1) {
> >>> + dev_err(sd->dev, "no configuration for inactive mux\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + /*
> >>> +  * Retrieve media bus configuration from the entity connected to the
> >>> +  * active input
> >>> +  */
> >>> + pad = media_entity_remote_pad(>pads[vidsw->active]);
> >>> + if (pad) {
> >>> + sd = media_entity_to_v4l2_subdev(pad->entity);
> >>> + ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> >>> + if (ret == -ENOIOCTLCMD)
> >>> + pad = NULL;
> >>> + else if (ret < 0) {
> >>> + dev_err(sd->dev, "failed to get source 
configuration\n");
> >>> + return ret;
> >>> + }
> >>> + }
> >>> + if (!pad) {
> >>> + /* Mirror the input side on the output side */
> >>> + cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> >>> + if (cfg->type == V4L2_MBUS_PARALLEL ||
> >>> + cfg->type == V4L2_MBUS_BT656)
> >>> + cfg->flags = vidsw->endpoint[vidsw-
>active].bus.parallel.flags;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >> 
> >> 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. The imx-media driver must not parse the sensor DT 
node as it is not aware of what bindings the sensor is compatible with. 
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.

> > We currently use this to make the CSI capture interface understand
> > whether its source from the MIPI CSI-2 or from the parallel bus. That is
> > probably something that should be fixed, but I'm not quite sure how.
> > 
> > The Synopsys DesignWare MIPI CSI-2 reciever turns the incoming MIPI
> > CSI-2 signal into a 32-bit parallel pixel bus plus some signals for the
> > MIPI specific metadata (virtual channel, data type).
> > 
> > Then the CSI2IPU gasket turns this input bus into four separate parallel
> > 16-bit pixel buses plus an 8-bit "mct_di" bus for each of them, that
> > carries the MIPI metadata. The incoming data is split into the four
> > outputs according to the MIPI virtual channel.
> > 
> > Two of these 16-bit + 8-bit parallel buses are routed through a
> > multiplexer before finally arriving at the CSI on the other side.
> > 
> > We need to configure the CSI to either use or ignore the data from the
> > 8-bit mct_di bus depending on whether the source of the mux is
> > configured to the MIPI CSI-2 receiver / CSI2IPU gasket, or to a parallel
> > input.
> 
> Philipp, from my experience, the CSI_MIPI_DI register (configured
> by ipu_csi_set_mipi_datatype()) can only be given a virtual channel 0,
> otherwise no data is received from the MIPI CSI-2 sensor, regardless
> of the virtual channel the sensor is transmitting over.
> 
> So it seems the info on the 8-bit mct_di buses generated by the CSI2IPU
> gasket are ignored by the CSI's, at least the virtual channel number is
> ignored.
> 
> For example, if the sensor is transmitting on vc 1, the gasket routes
> the sensor data to the parallel bus to CSI1. But if CSI_MIPI_DI on CSI1
> is written with vc 1, no data is received.
> 
> Steve
> 
> > Currently we let g_mbus_config pretend that even the internal 32-bit +
> > metadata and 16-bit + 8-bit metadata parallel buses are of type
> > V4L2_MBUS_CSI so that the CSI can ask the mux, which propagates to the
> > CSI-2 receiver, if connected.
> > 
> > Without g_mbus_config we'd need to get that information from somewhere
> > else. One possibility would be to extend MEDIA_BUS formats to describe
> > these "parallelized MIPI data" buses separately.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

On 01/24/2017 04:44 AM, Javier Martinez Canillas wrote:

Hello Steve,

On Fri, Jan 6, 2017 at 11:11 PM, Steve Longerbeam  wrote:

From: Philipp Zabel 

[snip]


+config VIDEO_MULTIPLEXER
+   tristate "Video Multiplexer"
+   depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER

The driver can be build as a module...


+
+static const struct of_device_id vidsw_dt_ids[] = {
+   { .compatible = "video-multiplexer", },
+   { /* sentinel */ }
+};
+

... so you need a MODULE_DEVICE_TABLE(of, vidsw_dt_ids) here or
otherwise module autoloading won't work.


Hi Javier, thanks for catching, done.

Steve


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

On 01/24/2017 04:02 AM, Philipp Zabel wrote:

Hi Hans,

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)
+{
+   struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
+   struct media_pad *pad;
+   int ret;
+
+   if (vidsw->active == -1) {
+   dev_err(sd->dev, "no configuration for inactive mux\n");
+   return -EINVAL;
+   }
+
+   /*
+* Retrieve media bus configuration from the entity connected to the
+* active input
+*/
+   pad = media_entity_remote_pad(>pads[vidsw->active]);
+   if (pad) {
+   sd = media_entity_to_v4l2_subdev(pad->entity);
+   ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
+   if (ret == -ENOIOCTLCMD)
+   pad = NULL;
+   else if (ret < 0) {
+   dev_err(sd->dev, "failed to get source 
configuration\n");
+   return ret;
+   }
+   }
+   if (!pad) {
+   /* Mirror the input side on the output side */
+   cfg->type = vidsw->endpoint[vidsw->active].bus_type;
+   if (cfg->type == V4L2_MBUS_PARALLEL ||
+   cfg->type == V4L2_MBUS_BT656)
+   cfg->flags = 
vidsw->endpoint[vidsw->active].bus.parallel.flags;
+   }
+
+   return 0;
+}

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.


We currently use this to make the CSI capture interface understand
whether its source from the MIPI CSI-2 or from the parallel bus. That is
probably something that should be fixed, but I'm not quite sure how.

The Synopsys DesignWare MIPI CSI-2 reciever turns the incoming MIPI
CSI-2 signal into a 32-bit parallel pixel bus plus some signals for the
MIPI specific metadata (virtual channel, data type).

Then the CSI2IPU gasket turns this input bus into four separate parallel
16-bit pixel buses plus an 8-bit "mct_di" bus for each of them, that
carries the MIPI metadata. The incoming data is split into the four
outputs according to the MIPI virtual channel.

Two of these 16-bit + 8-bit parallel buses are routed through a
multiplexer before finally arriving at the CSI on the other side.

We need to configure the CSI to either use or ignore the data from the
8-bit mct_di bus depending on whether the source of the mux is
configured to the MIPI CSI-2 receiver / CSI2IPU gasket, or to a parallel
input.


Philipp, from my experience, the CSI_MIPI_DI register (configured
by ipu_csi_set_mipi_datatype()) can only be given a virtual channel 0,
otherwise no data is received from the MIPI CSI-2 sensor, regardless
of the virtual channel the sensor is transmitting over.

So it seems the info on the 8-bit mct_di buses generated by the CSI2IPU
gasket are ignored by the CSI's, at least the virtual channel number is
ignored.

For example, if the sensor is transmitting on vc 1, the gasket routes
the sensor data to the parallel bus to CSI1. But if CSI_MIPI_DI on CSI1
is written with vc 1, no data is received.

Steve



Currently we let g_mbus_config pretend that even the internal 32-bit +
metadata and 16-bit + 8-bit metadata parallel buses are of type
V4L2_MBUS_CSI so that the CSI can ask the mux, which propagates to the
CSI-2 receiver, if connected.

Without g_mbus_config we'd need to get that information from somewhere
else. One possibility would be to extend MEDIA_BUS formats to describe
these "parallelized MIPI data" buses separately.

regards
Philipp



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Javier Martinez Canillas]

Hello Steve,

On Fri, Jan 6, 2017 at 11:11 PM, Steve Longerbeam  wrote:
> From: Philipp Zabel 

[snip]

>
> +config VIDEO_MULTIPLEXER
> +   tristate "Video Multiplexer"
> +   depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER

The driver can be build as a module...

> +
> +static const struct of_device_id vidsw_dt_ids[] = {
> +   { .compatible = "video-multiplexer", },
> +   { /* sentinel */ }
> +};
> +

... so you need a MODULE_DEVICE_TABLE(of, vidsw_dt_ids) here or
otherwise module autoloading won't work.

Best regards,
Javier
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

Hi Hans,

On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote:
> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> > From: Philipp Zabel 
> > 
> > This driver can handle SoC internal and external video bus multiplexers,
> > controlled either by register bit fields or by a GPIO. The subdevice
> > passes through frame interval and mbus configuration of the active input
> > to the output side.
> > 
> > Signed-off-by: Sascha Hauer 
> > Signed-off-by: Philipp Zabel 
> > 
> > --
> > 
> > - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
> >   should be unregister.
> > 
> > - added media_entity_cleanup() and v4l2_device_unregister_subdev()
> >   to vidsw_remove().
> > 
> > - there was a line left over from a previous iteration that negated
> >   the new way of determining the pad count just before it which
> >   has been removed (num_pads = of_get_child_count(np)).
> > 
> > - Philipp Zabel has developed a set of patches that allow adding
> >   to the subdev async notifier waiting list using a chaining method
> >   from the async registered callbacks (v4l2_of_subdev_registered()
> >   and the prep patches for that). For now, I've removed the use of
> >   v4l2_of_subdev_registered() for the vidmux driver's registered
> >   callback. This doesn't affect the functionality of this driver,
> >   but allows for it to be merged now, before adding the chaining
> >   support.
> > 
> > Signed-off-by: Steve Longerbeam 
> > ---
> >  .../bindings/media/video-multiplexer.txt   |  59 +++
> >  drivers/media/platform/Kconfig |   8 +
> >  drivers/media/platform/Makefile|   2 +
> >  drivers/media/platform/video-multiplexer.c | 472 
> > +
> >  4 files changed, 541 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/media/video-multiplexer.txt
> >  create mode 100644 drivers/media/platform/video-multiplexer.c
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> > b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > new file mode 100644
> > index 000..9d133d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > @@ -0,0 +1,59 @@
> > +Video Multiplexer
> > +=
> > +
> > +Video multiplexers allow to select between multiple input ports. Video 
> > received
> > +on the active input port is passed through to the output port. Muxes 
> > described
> > +by this binding may be controlled by a syscon register bitfield or by a 
> > GPIO.
> > +
> > +Required properties:
> > +- compatible : should be "video-multiplexer"
> > +- reg: should be register base of the register containing the control 
> > bitfield
> > +- bit-mask: bitmask of the control bitfield in the control register
> > +- bit-shift: bit offset of the control bitfield in the control register
> > +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO 
> > phandle
> > +  may be given to switch between two inputs
> > +- #address-cells: should be <1>
> > +- #size-cells: should be <0>
> > +- port@*: at least three port nodes containing endpoints connecting to the
> > +  source and sink devices according to of_graph bindings. The last port is
> > +  the output port, all others are inputs.
> > +
> > +Example:
> > +
> > +syscon {
> > +   compatible = "syscon", "simple-mfd";
> > +
> > +   mux {
> > +   compatible = "video-multiplexer";
> > +   /* Single bit (1 << 19) in syscon register 0x04: */
> > +   reg = <0x04>;
> > +   bit-mask = <1>;
> > +   bit-shift = <19>;
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +
> > +   port@0 {
> > +   reg = <0>;
> > +
> > +   mux_in0: endpoint {
> > +   remote-endpoint = <_source0_out>;
> > +   };
> > +   };
> > +
> > +   port@1 {
> > +   reg = <1>;
> > +
> > +   mux_in1: endpoint {
> > +   remote-endpoint = <_source1_out>;
> > +   };
> > +   };
> > +
> > +   port@2 {
> > +   reg = <2>;
> > +
> > +   mux_out: endpoint {
> > +   remote-endpoint = <_interface_in>;
> > +   };
> > +   };
> > +   };
> > +};
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index d944421..65614b5 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
> >   To compile this driver as a module, choose M here: the
> >   module will be called arv.
> >  
> > +config VIDEO_MULTIPLEXER
> > +   tristate "Video Multiplexer"
> > +   depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
> > +   help

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Hans Verkuil]

On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> 
> --
> 
> - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
>   to vidsw_remove().
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  .../bindings/media/video-multiplexer.txt   |  59 +++
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 472 
> +
>  4 files changed, 541 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/video-multiplexer.txt
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> new file mode 100644
> index 000..9d133d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> @@ -0,0 +1,59 @@
> +Video Multiplexer
> +=
> +
> +Video multiplexers allow to select between multiple input ports. Video 
> received
> +on the active input port is passed through to the output port. Muxes 
> described
> +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> +
> +Required properties:
> +- compatible : should be "video-multiplexer"
> +- reg: should be register base of the register containing the control 
> bitfield
> +- bit-mask: bitmask of the control bitfield in the control register
> +- bit-shift: bit offset of the control bitfield in the control register
> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> +  may be given to switch between two inputs
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
> +
> +Example:
> +
> +syscon {
> + compatible = "syscon", "simple-mfd";
> +
> + mux {
> + compatible = "video-multiplexer";
> + /* Single bit (1 << 19) in syscon register 0x04: */
> + reg = <0x04>;
> + bit-mask = <1>;
> + bit-shift = <19>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + mux_in0: endpoint {
> + remote-endpoint = <_source0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + mux_in1: endpoint {
> + remote-endpoint = <_source1_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + mux_out: endpoint {
> + remote-endpoint = <_interface_in>;
> + };
> + };
> + };
> +};
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index d944421..65614b5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>  
> +config VIDEO_MULTIPLEXER
> + tristate "Video Multiplexer"
> + depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
> + help
> +   This driver provides support for SoC internal N:1 video bus
> +   multiplexers controlled by register bitfields as well as external
> +   2:1 video multiplexers controlled by a single GPIO.
> +
>  config VIDEO_OMAP3
>   tristate "OMAP 3 

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Rob Herring]

On Fri, Jan 06, 2017 at 06:11:31PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> 
> --
> 
> - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
>   to vidsw_remove().
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  .../bindings/media/video-multiplexer.txt   |  59 +++
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 472 
> +
>  4 files changed, 541 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/video-multiplexer.txt
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> new file mode 100644
> index 000..9d133d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> @@ -0,0 +1,59 @@
> +Video Multiplexer
> +=
> +
> +Video multiplexers allow to select between multiple input ports. Video 
> received
> +on the active input port is passed through to the output port. Muxes 
> described
> +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> +
> +Required properties:
> +- compatible : should be "video-multiplexer"

This should have an SoC/chip specific compatible string additionally. 
Just need a note to that effect here, but i.MX will need a compat 
string.

> +- reg: should be register base of the register containing the control 
> bitfield
> +- bit-mask: bitmask of the control bitfield in the control register
> +- bit-shift: bit offset of the control bitfield in the control register
> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> +  may be given to switch between two inputs
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

From: Philipp Zabel 

This driver can handle SoC internal and external video bus multiplexers,
controlled either by register bit fields or by a GPIO. The subdevice
passes through frame interval and mbus configuration of the active input
to the output side.

Signed-off-by: Sascha Hauer 
Signed-off-by: Philipp Zabel 

--

- fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
  should be unregister.

- added media_entity_cleanup() and v4l2_device_unregister_subdev()
  to vidsw_remove().

- there was a line left over from a previous iteration that negated
  the new way of determining the pad count just before it which
  has been removed (num_pads = of_get_child_count(np)).

- Philipp Zabel has developed a set of patches that allow adding
  to the subdev async notifier waiting list using a chaining method
  from the async registered callbacks (v4l2_of_subdev_registered()
  and the prep patches for that). For now, I've removed the use of
  v4l2_of_subdev_registered() for the vidmux driver's registered
  callback. This doesn't affect the functionality of this driver,
  but allows for it to be merged now, before adding the chaining
  support.

Signed-off-by: Steve Longerbeam 
---
 .../bindings/media/video-multiplexer.txt   |  59 +++
 drivers/media/platform/Kconfig |   8 +
 drivers/media/platform/Makefile|   2 +
 drivers/media/platform/video-multiplexer.c | 472 +
 4 files changed, 541 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/media/video-multiplexer.txt
 create mode 100644 drivers/media/platform/video-multiplexer.c

diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
b/Documentation/devicetree/bindings/media/video-multiplexer.txt
new file mode 100644
index 000..9d133d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
@@ -0,0 +1,59 @@
+Video Multiplexer
+=
+
+Video multiplexers allow to select between multiple input ports. Video received
+on the active input port is passed through to the output port. Muxes described
+by this binding may be controlled by a syscon register bitfield or by a GPIO.
+
+Required properties:
+- compatible : should be "video-multiplexer"
+- reg: should be register base of the register containing the control bitfield
+- bit-mask: bitmask of the control bitfield in the control register
+- bit-shift: bit offset of the control bitfield in the control register
+- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
+  may be given to switch between two inputs
+- #address-cells: should be <1>
+- #size-cells: should be <0>
+- port@*: at least three port nodes containing endpoints connecting to the
+  source and sink devices according to of_graph bindings. The last port is
+  the output port, all others are inputs.
+
+Example:
+
+syscon {
+   compatible = "syscon", "simple-mfd";
+
+   mux {
+   compatible = "video-multiplexer";
+   /* Single bit (1 << 19) in syscon register 0x04: */
+   reg = <0x04>;
+   bit-mask = <1>;
+   bit-shift = <19>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+
+   mux_in0: endpoint {
+   remote-endpoint = <_source0_out>;
+   };
+   };
+
+   port@1 {
+   reg = <1>;
+
+   mux_in1: endpoint {
+   remote-endpoint = <_source1_out>;
+   };
+   };
+
+   port@2 {
+   reg = <2>;
+
+   mux_out: endpoint {
+   remote-endpoint = <_interface_in>;
+   };
+   };
+   };
+};
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index d944421..65614b5 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
  To compile this driver as a module, choose M here: the
  module will be called arv.
 
+config VIDEO_MULTIPLEXER
+   tristate "Video Multiplexer"
+   depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
+   help
+ This driver provides support for SoC internal N:1 video bus
+ multiplexers controlled by register bitfields as well as external
+ 2:1 video multiplexers controlled by a single GPIO.
+
 config VIDEO_OMAP3
tristate "OMAP 3 Camera support"
depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 5b3cb27..7cf0ee5 100644
--- a/drivers/media/platform/Makefile