Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> That self-referencing mux-controls property looks a bit superfluous:
> 
>   mux: video-multiplexer {
>   mux-controls = <>;
>   };
> 
> Other than that, I'm completely fine with splitting the compatible into
> something like video-mux-gpio and video-mux-mmio and reusing the
> mux-gpios property for video-mux-gpio.

Agreed, I overseen that.

> > You should be able to use code in drivers/mux as a library...
> 
> This is a good idea in principle, but this requires some rework of the
> mux subsystem, and that subsystem hasn't even landed yet. For now I'd
> like to focus on getting the DT bindings right.
> 
> I'd honestly prefer to not add this rework as a requirement for the i.MX
> media drivers to get into staging.

Hmm. staging/ normally accepts code with bigger design problems than
that.

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 v6 17/39] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

Hi Pavel,

On Fri, 2017-04-14 at 22:32 +0200, Pavel Machek wrote:
> Hi!
> 
> > > The MUX framework is already in linux-next. Could you use that instead of
> > > adding new driver + bindings that are not compliant with the MUX 
> > > framework?
> > > I don't think it'd be much of a change in terms of code, using the MUX
> > > framework appears quite simple.
> > 
> > It is not quite clear to me how to design the DT bindings for this. Just
> > splitting the video-multiplexer driver from the mux-mmio / mux-gpio
> > would make it necessary to keep the video-multiplexer node to describe
> > the of-graph bindings. But then we have two different nodes in the DT
> > that describe the same hardware:
> > 
> > mux: mux {
> > compatible = "mux-gpio";
> > mux-gpios = < 0>, < 1>;
> > #mux-control-cells = <0>;
> > }
> > 
> > video-multiplexer {
> > compatible = "video-multiplexer"
> > mux-controls = <>;
> > 
> > ports {
> > /* ... */
> > }
> > }
> > 
> > It would feel more natural to have the ports in the mux node, but then
> > how would the video-multiplexer driver be instanciated, and how would it
> > get to the of-graph nodes?
> 
> Device tree representation and code used to implement the muxing
> driver should be pretty independend, no? Yes, one piece of hardware
> should have one entry in the device tree,

I agree.

>  so it should be something like:
> 
>   video-multiplexer {
>   compatible = "video-multiplexer-gpio"   
>   mux-gpios = < 0>, < 1>;
>   #mux-control-cells = <0>;
> 
>   mux-controls = <>;
>  
>   ports {
>   /* ... */
>   }
>   }

That self-referencing mux-controls property looks a bit superfluous:

mux: video-multiplexer {
mux-controls = <>;
};

Other than that, I'm completely fine with splitting the compatible into
something like video-mux-gpio and video-mux-mmio and reusing the
mux-gpios property for video-mux-gpio.

> You should be able to use code in drivers/mux as a library...

This is a good idea in principle, but this requires some rework of the
mux subsystem, and that subsystem hasn't even landed yet. For now I'd
like to focus on getting the DT bindings right.

I'd honestly prefer to not add this rework as a requirement for the i.MX
media drivers to get into staging.

regards
Philipp

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> > The MUX framework is already in linux-next. Could you use that instead of
> > adding new driver + bindings that are not compliant with the MUX framework?
> > I don't think it'd be much of a change in terms of code, using the MUX
> > framework appears quite simple.
> 
> It is not quite clear to me how to design the DT bindings for this. Just
> splitting the video-multiplexer driver from the mux-mmio / mux-gpio
> would make it necessary to keep the video-multiplexer node to describe
> the of-graph bindings. But then we have two different nodes in the DT
> that describe the same hardware:
> 
>   mux: mux {
>   compatible = "mux-gpio";
>   mux-gpios = < 0>, < 1>;
>   #mux-control-cells = <0>;
>   }
> 
>   video-multiplexer {
>   compatible = "video-multiplexer"
>   mux-controls = <>;
> 
>   ports {
>   /* ... */
>   }
>   }
> 
> It would feel more natural to have the ports in the mux node, but then
> how would the video-multiplexer driver be instanciated, and how would it
> get to the of-graph nodes?

Device tree representation and code used to implement the muxing
driver should be pretty independend, no? Yes, one piece of hardware
should have one entry in the device tree, so it should be something
like:


video-multiplexer {
compatible = "video-multiplexer-gpio"   
mux-gpios = < 0>, < 1>;
#mux-control-cells = <0>;

mux-controls = <>;
 
ports {
/* ... */
}
}

You should be able to use code in drivers/mux as a library...

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 v6 17/39] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

Hi Sakari,

thank you for the review.

On Tue, 2017-04-04 at 15:47 +0300, Sakari Ailus wrote:
> Hi Steve, Philipp and Pavel,
> 
> On Mon, Mar 27, 2017 at 05:40:34PM -0700, 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.
> 
> The MUX framework is already in linux-next. Could you use that instead of
> adding new driver + bindings that are not compliant with the MUX framework?
> I don't think it'd be much of a change in terms of code, using the MUX
> framework appears quite simple.

It is not quite clear to me how to design the DT bindings for this. Just
splitting the video-multiplexer driver from the mux-mmio / mux-gpio
would make it necessary to keep the video-multiplexer node to describe
the of-graph bindings. But then we have two different nodes in the DT
that describe the same hardware:

mux: mux {
compatible = "mux-gpio";
mux-gpios = < 0>, < 1>;
#mux-control-cells = <0>;
}

video-multiplexer {
compatible = "video-multiplexer"
mux-controls = <>;

ports {
/* ... */
}
}

It would feel more natural to have the ports in the mux node, but then
how would the video-multiplexer driver be instanciated, and how would it
get to the of-graph nodes?

> In general the driver looks pretty good, especially regarding the user space
> API implementation which is important for use with other drivers.
> 
> I have some more detailed comments below.
> 
> > 
> > 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() 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)).
> > 
> > - removed [gs]_frame_interval ops. timeperframe is not used anywhwere
> >   in this subdev, and currently it has no control over frame rate.
> > 
> > - add link_validate to media_entity_operations.
> > 
> > - moved devicetree binding doc to a separate commit.
> > 
> > - 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 
> > ---
> >  drivers/media/platform/Kconfig |   8 +
> >  drivers/media/platform/Makefile|   2 +
> >  drivers/media/platform/video-multiplexer.c | 451 
> > +
> >  3 files changed, 461 insertions(+)
> >  create mode 100644 drivers/media/platform/video-multiplexer.c
> > 
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index ab0bb48..c9b8d9c 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 8959f6e..d418add 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)+= sh_veu.o
> >  
> >  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)+= m2m-deinterlace.o
> >  
> > +obj-$(CONFIG_VIDEO_MULTIPLEXER)+= video-multiplexer.o
> > +
> >  obj-$(CONFIG_VIDEO_S3C_CAMIF)  += s3c-camif/
> >  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) += exynos4-is/
> 

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Sakari Ailus]

Hi Steve,

On Tue, Apr 11, 2017 at 05:50:58PM -0700, Steve Longerbeam wrote:
> 
> 
> On 04/04/2017 05:47 AM, Sakari Ailus wrote:
> >Hi Steve, Philipp and Pavel,
> >
> >On Mon, Mar 27, 2017 at 05:40:34PM -0700, 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.
> >
> >The MUX framework is already in linux-next. Could you use that instead of
> >adding new driver + bindings that are not compliant with the MUX framework?
> >I don't think it'd be much of a change in terms of code, using the MUX
> >framework appears quite simple.
> 
> I would prefer to wait on this, and get what we have merged now so I can
> unload all these patches first.

The DT bindings will be different for this one and if you were using a MUX,
won't they? And you can't remove support for the existing bindings either,
you have to continue to support them going forward.

> 
> Also this is Philipp's driver, so again I would prefer to get this
> merged as-is and then Philipp can address these issues in a future
> patch. But I will add my comments below...

I bet there will be more issues to handle if you were to do the changes
later than now.

...

> >>+static int vidsw_s_stream(struct v4l2_subdev *sd, int enable)
> >>+{
> >>+   struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> >>+   struct v4l2_subdev *upstream_sd;
> >>+   struct media_pad *pad;
> >>+
> >>+   if (vidsw->active == -1) {
> >>+   dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> >>+   return -EINVAL;
> >>+   }
> >>+
> >>+   pad = media_entity_remote_pad(>entity.pads[vidsw->active]);
> >>+   if (!pad) {
> >>+   dev_err(sd->dev, "Failed to find remote source pad\n");
> >>+   return -ENOLINK;
> >>+   }
> >>+
> >>+   if (!is_media_entity_v4l2_subdev(pad->entity)) {
> >>+   dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> >>+   return -ENODEV;
> >>+   }
> >>+
> >>+   upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> >>+
> >>+   return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> >
> >Now that we'll have more than two drivers involved in the same pipeline it
> >becomes necessary to define the behaviour of s_stream() throughout the
> >pipeline --- i.e. whose responsibility is it to call s_stream() on the
> >sub-devices in the pipeline?
> 
> In the case of imx-media, the capture device calls set stream on the
> whole pipeline in the start_streaming() callback. This subdev call is
> actually a NOOP for imx-media, because the upstream entity has already
> started streaming. Again I think this should be removed. It also
> enforces a stream order that some MC drivers may have a problem with.

What I want to say here is that the order in which the different devices in
the pipeline need to be started may not be known in a driver for a
particular part of the pipeline.

In order to avoid trying to have a single point of decision making, the
s_stream() op implemented in sub-device drivers should serve the purpose.
I'll cc you for the documentation patch.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Steve Longerbeam]

On 04/04/2017 05:47 AM, Sakari Ailus wrote:

Hi Steve, Philipp and Pavel,

On Mon, Mar 27, 2017 at 05:40:34PM -0700, 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.


The MUX framework is already in linux-next. Could you use that instead of
adding new driver + bindings that are not compliant with the MUX framework?
I don't think it'd be much of a change in terms of code, using the MUX
framework appears quite simple.


I would prefer to wait on this, and get what we have merged now so I can
unload all these patches first.

Also this is Philipp's driver, so again I would prefer to get this
merged as-is and then Philipp can address these issues in a future
patch. But I will add my comments below...




In general the driver looks pretty good, especially regarding the user space
API implementation which is important for use with other drivers.

I have some more detailed comments below.






+
+struct vidsw {
+   struct v4l2_subdev subdev;
+   unsigned int num_pads;


You could use subdev.entity.num_pads instead of caching the value locally.


Agreed.




+   struct media_pad *pads;
+   struct v4l2_mbus_framefmt *format_mbus;
+   struct v4l2_of_endpoint *endpoint;
+   struct regmap_field *field;
+   struct gpio_desc *gpio;
+   int active;
+};
+
+static inline struct vidsw *v4l2_subdev_to_vidsw(struct v4l2_subdev *sd)
+{
+   return container_of(sd, struct vidsw, subdev);
+}
+
+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);
+}
+
+static int vidsw_link_setup(struct media_entity *entity,
+   const struct media_pad *local,
+   const struct media_pad *remote, u32 flags)
+{
+   struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+   struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
+
+   /* We have no limitations on enabling or disabling our output link */
+   if (local->index == vidsw->num_pads - 1)
+   return 0;
+
+   dev_dbg(sd->dev, "link setup %s -> %s", remote->entity->name,
+   local->entity->name);
+
+   if (!(flags & MEDIA_LNK_FL_ENABLED)) {
+   if (local->index == vidsw->active) {
+   dev_dbg(sd->dev, "going inactive\n");
+   vidsw->active = -1;
+   }
+   return 0;
+   }
+
+   if (vidsw->active >= 0) {
+   struct media_pad *pad;
+
+   if (vidsw->active == local->index)
+   return 0;
+
+   pad = media_entity_remote_pad(>pads[vidsw->active]);
+   if (pad) {
+   struct media_link *link;
+   int ret;
+
+   link = media_entity_find_link(pad,
+   >pads[vidsw->active]);
+   if (link) {
+   ret = __media_entity_setup_link(link, 0);


I wouldn't implicitly disable a link, even if only one can be active at a
given time. No other drivers do that either.

Perhaps returning an error might be a better thing to do: if you're
reconfiguring the pipeline anyway, there are likely issues elsewhere in it.

We could also change the behaviour later to allow implicit changes but we
can't later on go the other way without breaking the user space.


I think this whole if (vidsw->active >= 0) { ... } block should be
removed. This is left-over from the first implementation that tried
to propagate link setup upstream. This is not working yet, so for now
I think this should be removed.







+
+static bool vidsw_endpoint_disabled(struct device_node *ep)
+{
+   struct device_node *rpp;
+
+   if (!of_device_is_available(ep))


ep here is the endpoint, whereas the argument to of_device_is_available()
should correspond to the actual device.


Agreed, I think this if statement should be removed, and...




+   return true;
+
+   rpp = of_graph_get_remote_port_parent(ep);
+   if (!rpp)
+   return true;


this if statement can also be removed, since that is
handled automatically by of_device_is_available() below.


+
+   return !of_device_is_available(rpp);
+}
+
+static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)


I think I'd arrange this closer to probe as it's related to probe directly.
Up to you.


+{
+   struct device_node *ep;
+   u32 

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

On Wed 2017-04-05 13:58:39, Lucas Stach wrote:
> Am Mittwoch, den 05.04.2017, 13:18 +0200 schrieb Pavel Machek:
> > Hi!
> > 
> > > + * video stream multiplexer controlled via gpio or syscon
> > > + *
> > > + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> > > + * Copyright (C) 2016 Pengutronix, Philipp Zabel 
> > 
> > This is actually quite interesting. Same email address for two
> > people...
> > 
> > Plus, I believe this wants to say that copyright is with Pengutronix,
> > not Sascha and Philipp. In that case you probably want to list
> > copyright and authors separately?
> > 
> Nope, copyright doesn't get transferred to the employer within the rules
> of the German "Urheberrecht", but stays at the original author of the
> code.

Ok, then I guess it should be

Copyright (C) 2013 Sascha Hauer
Work sponsored by Pengutronix, use  as contact address

or something? I know license change is not on the table, but I guess
it is good to get legal stuff right.
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 v6 17/39] platform: add video-multiplexer subdevice driver

[Lucas Stach]

Am Mittwoch, den 05.04.2017, 13:18 +0200 schrieb Pavel Machek:
> Hi!
> 
> > + * video stream multiplexer controlled via gpio or syscon
> > + *
> > + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> > + * Copyright (C) 2016 Pengutronix, Philipp Zabel 
> 
> This is actually quite interesting. Same email address for two
> people...
> 
> Plus, I believe this wants to say that copyright is with Pengutronix,
> not Sascha and Philipp. In that case you probably want to list
> copyright and authors separately?
> 
Nope, copyright doesn't get transferred to the employer within the rules
of the German "Urheberrecht", but stays at the original author of the
code.
Same email is just to ensure that any requests regarding this code get
routed to the right people, even if someone leaves the company or is
temporarily unavailable. kernel@ is a list for the Pengutronix kernel
team.

Regards,
Lucas

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Pavel Machek]

Hi!

> + * video stream multiplexer controlled via gpio or syscon
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel 

This is actually quite interesting. Same email address for two
people...

Plus, I believe this wants to say that copyright is with Pengutronix,
not Sascha and Philipp. In that case you probably want to list
copyright and authors separately?

-- 
(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 v6 17/39] platform: add video-multiplexer subdevice driver

[Sakari Ailus]

Hi Steve, Philipp and Pavel,

On Mon, Mar 27, 2017 at 05:40:34PM -0700, 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.

The MUX framework is already in linux-next. Could you use that instead of
adding new driver + bindings that are not compliant with the MUX framework?
I don't think it'd be much of a change in terms of code, using the MUX
framework appears quite simple.

In general the driver looks pretty good, especially regarding the user space
API implementation which is important for use with other drivers.

I have some more detailed comments below.

> 
> 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() 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)).
> 
> - removed [gs]_frame_interval ops. timeperframe is not used anywhwere
>   in this subdev, and currently it has no control over frame rate.
> 
> - add link_validate to media_entity_operations.
> 
> - moved devicetree binding doc to a separate commit.
> 
> - 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 
> ---
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 451 
> +
>  3 files changed, 461 insertions(+)
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ab0bb48..c9b8d9c 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 8959f6e..d418add 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)  += sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)  += m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MULTIPLEXER)  += video-multiplexer.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF)+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS)   += exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
> diff --git a/drivers/media/platform/video-multiplexer.c 
> b/drivers/media/platform/video-multiplexer.c
> new file mode 100644
> index 000..b18c317
> --- /dev/null
> +++ b/drivers/media/platform/video-multiplexer.c
> @@ -0,0 +1,451 @@
> +/*
> + * video stream multiplexer controlled via gpio or syscon
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> 

Re: [PATCH v6 17/39] platform: add video-multiplexer subdevice driver

[Vladimir Zapolskiy]

Hi Steve,

On 03/28/2017 03:40 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() 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)).
> 
> - removed [gs]_frame_interval ops. timeperframe is not used anywhwere
>   in this subdev, and currently it has no control over frame rate.
> 
> - add link_validate to media_entity_operations.
> 
> - moved devicetree binding doc to a separate commit.
> 
> - 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 
> ---
>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 451 
> +
>  3 files changed, 461 insertions(+)
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ab0bb48..c9b8d9c 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

+ depends on OF

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

[snip]

> +static int vidsw_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct of_endpoint endpoint;
> + struct device_node *ep;
> + struct reg_field field;
> + struct vidsw *vidsw;
> + struct regmap *map;
> + unsigned int num_pads;
> + int ret;
> +
> + vidsw = devm_kzalloc(>dev, sizeof(*vidsw), GFP_KERNEL);
> + if (!vidsw)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, vidsw);
> +
> + v4l2_subdev_init(>subdev, _subdev_ops);
> + snprintf(vidsw->subdev.name, sizeof(vidsw->subdev.name), "%s",
> + np->name);

 or oops here   ^.

> + vidsw->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + vidsw->subdev.dev = >dev;
> +
> + /*
> +  * The largest numbered port is the output port. It determines
> +  * total number of pads
> +  */
> + num_pads = 0;
> + for_each_endpoint_of_node(np, ep) {
> + of_graph_parse_endpoint(ep, );
> + num_pads = max(num_pads, endpoint.port + 1);
> + }
> +
> + if (num_pads < 2) {
> + dev_err(>dev, "Not enough ports %d\n", num_pads);
> + return -EINVAL;
> + }

This unveils another runtime dependency on OF.

> +
> + ret = of_get_reg_field(np, );
> + if (ret == 0) {
> + map = syscon_node_to_regmap(np->parent);
> + if (!map) {
> + dev_err(>dev, "Failed to get syscon register 
> map\n");
> + return PTR_ERR(map);
> + }
> +
> + vidsw->field = devm_regmap_field_alloc(>dev, map, field);
> + if (IS_ERR(vidsw->field)) {
> + dev_err(>dev, "Failed to allocate regmap 
> field\n");
> + return PTR_ERR(vidsw->field);
> + }
> +
> + regmap_field_read(vidsw->field, >active);
> + } else {
> + if (num_pads > 3) {
> + dev_err(>dev, "Too many ports %d\n", num_pads);
> + return -EINVAL;
> + }
> +
> + vidsw->gpio = devm_gpiod_get(>dev, NULL, GPIOD_OUT_LOW);
> + if (IS_ERR(vidsw->gpio)) {
> +