Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576052 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-02-07 22:00 +0100
    Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice  driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-02-08 10:50 +0100
      Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-08 11:30 +0100

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

Hi Steve,

On Fri, Jan 06, 2017 at 06:11:31PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> 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 <s.hauer@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> --
> 
> - fixed a cut&paste 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 <steve_longerbeam@mentor.com>
> ---
>  .../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 0000000..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:

<URL:http://www.spinics.net/lists/linux-media/msg109493.html>

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 = <&video_source0_out>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			mux_in1: endpoint {
> +				remote-endpoint = <&video_source1_out>;
> +			};
> +		};
> +
> +		port@2 {
> +			reg = <2>;
> +
> +			mux_out: endpoint {
> +				remote-endpoint = <&capture_interface_in>;
> +			};
> +		};
> +	};
> +};

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [next] | [standalone]

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

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 <p.zabel@pengutronix.de>
> > 
> > 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 <s.hauer@pengutronix.de>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > --
> > 
> > - fixed a cut&paste 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 <steve_longerbeam@mentor.com>
> > ---
> >  .../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 0000000..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:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg109493.html>

[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 finishing the series of patches in the branch I believe
> Laurent already quoted here.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1576416

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 <p.zabel@pengutronix.de>
> >> 
> >> 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 <s.hauer@pengutronix.de>
> >> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> 
> >> --
> >> 
> >> - fixed a cut&paste 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 <steve_longerbeam@mentor.com>
> >> ---
> >> 
> >>  .../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 0000000..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:
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg109493.html>
> 
> [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 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 finishing the series of patches in the branch I believe
> > Laurent already quoted here.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web