Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

On 02/27/2017 06:41 AM, Rob Herring wrote:

On Wed, Feb 15, 2017 at 06:19:17PM -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().

- added missing MODULE_DEVICE_TABLE().
   Suggested-by: Javier Martinez Canillas 

- 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 +++

Please make this a separate commit.


Done.

Steve

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Rob Herring]

On Wed, Feb 15, 2017 at 06:19:17PM -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().
> 
> - added missing MODULE_DEVICE_TABLE().
>   Suggested-by: Javier Martinez Canillas 
> 
> - 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 +++

Please make this a separate commit.

>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 474 
> +
>  4 files changed, 543 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>;
> + };
> + };
> + };
> +};

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> > Plus you might want to describe which port correspond to which gpio
> > state/bitfield values...
> > 
> > > +struct vidsw {
> > 
> > I knew it: it is secretely a switch! :-).
> 
> This driver started as a two-input gpio controlled bus switch.
> I changed the name when adding support for bitfield controlled
> multiplexers with more than two inputs.

We had discussion with Sakari / Rob whether gpio controlled thing is a
switch or a multiplexer :-).

> > > + 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;
> > > + }
> > 
> > Will this need support for other V4L2_MBUS_ values?
> 
> To support CSI-2 multiplexers, yes.

Can you stick switch () {  default: dev_err() } there, to help
future hackers?

Thank,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

On Sun, 2017-02-19 at 23:02 +0100, Pavel Machek wrote:
> Hi!
> 
> > 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 
> > --
> >
> 
> Again, this is slightly non-standard format. Normally changes from v1
> go below ---, but in your case it would cut off the signoff...
> 
> > 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.
> 
> At least three? I guess it is exactly three with the gpio?

Yes. With the mmio bitfield muxes there can be more.

> Plus you might want to describe which port correspond to which gpio
> state/bitfield values...
> 
> > +struct vidsw {
> 
> I knew it: it is secretely a switch! :-).

This driver started as a two-input gpio controlled bus switch.
I changed the name when adding support for bitfield controlled
multiplexers with more than two inputs.

> > +static void vidsw_set_active(struct vidsw *vidsw, int active)
> > +{
> > +   vidsw->active = active;
> > +   if (active < 0)
> > +   return;
> > +
> > +   dev_dbg(vidsw->subdev.dev, "setting %d active\n", active);
> > +
> > +   if (vidsw->field)
> > +   regmap_field_write(vidsw->field, active);
> > +   else if (vidsw->gpio)
> > +   gpiod_set_value(vidsw->gpio, active);
> 
>  else dev_err()...?

If neither field nor gpio are set, probe will have failed and this will
never be called.

> > +static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
> > +{
> > +   struct device_node *ep;
> > +   u32 portno;
> > +   int numports;
> 
> numbports is int, so I guess portno should be, too?

We could change both to unsigned int, as both vidsw->num_pads and
endpoint.base.port are unsigned int, and they are only compared/assigned
to those and each other.

> > +   portno = endpoint.base.port;
> > +   if (portno >= numports - 1)
> > +   continue;
> 
 I. 
> > +   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;
> > +   }
> 
> Will this need support for other V4L2_MBUS_ values?

To support CSI-2 multiplexers, yes.

> > +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> > +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> 
> Normally, MODULE_AUTHOR contains comma separated names of authors,
> perhaps with . Not sure two MODULE_AUTHORs per file
> will work.
> 
> Thanks,
>   Pavel

regards
Philipp

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> 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 
> --
>

Again, this is slightly non-standard format. Normally changes from v1
go below ---, but in your case it would cut off the signoff...

> 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.

At least three? I guess it is exactly three with the gpio?

Plus you might want to describe which port correspond to which gpio
state/bitfield values...

> +struct vidsw {

I knew it: it is secretely a switch! :-).

> +static void vidsw_set_active(struct vidsw *vidsw, int active)
> +{
> + vidsw->active = active;
> + if (active < 0)
> + return;
> +
> + dev_dbg(vidsw->subdev.dev, "setting %d active\n", active);
> +
> + if (vidsw->field)
> + regmap_field_write(vidsw->field, active);
> + else if (vidsw->gpio)
> + gpiod_set_value(vidsw->gpio, active);

 else dev_err()...?
 
> +static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
> +{
> + struct device_node *ep;
> + u32 portno;
> + int numports;

numbports is int, so I guess portno should be, too?

> + portno = endpoint.base.port;
> + if (portno >= numports - 1)
> + continue;


> + 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;
> + }

Will this need support for other V4L2_MBUS_ values?

> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");

Normally, MODULE_AUTHOR contains comma separated names of authors,
perhaps with . Not sure two MODULE_AUTHORs per file
will work.

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[PATCH v4 15/36] 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().

- added missing MODULE_DEVICE_TABLE().
  Suggested-by: Javier Martinez Canillas 

- 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 | 474 +
 4 files changed, 543 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 c9106e1..3d60d4c 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