Re: [RFC/PATCH] media: Add video bus switch

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]

Hi,

On Thu, Dec 22, 2016 at 11:42:26PM +0100, Pavel Machek wrote:
> On Thu 2016-12-22 15:32:44, Sebastian Reichel wrote:
> > Hi Pavel,
> > 
> > On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> > > N900 contains front and back camera, with a switch between the
> > > two. This adds support for the swich component.
> > > 
> > > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > --
> > > 
> > > I see this needs dts documentation, anything else than needs to be
> > > done?
> > 
> > Yes. This driver takes care of the switch gpio, but the cameras also
> > use different bus settings. Currently omap3isp gets the bus-settings
> > from the link connected to the CCP2 port in DT at probe time (*).
> > 
> > So there are two general problems:
> > 
> > 1. Settings must be applied before the streaming starts instead of
> > at probe time, since the settings may change (based one the selected
> > camera). That should be fairly easy to implement by just moving the
> > code to the s_stream callback as far as I can see.
> > 
> > 2. omap3isp should try to get the bus settings from using a callback
> > in the connected driver instead of loading it from DT. Then the
> > video-bus-switch can load the bus-settings from its downstream links
> > in DT and propagate the correct ones to omap3isp based on the
> > selected port. The DT loading part should actually remain in omap3isp
> > as fallback, in case it does not find a callback in the connected driver.
> > That way everything is backward compatible and the DT variant is
> > nice for 1-on-1 scenarios.
> 
> So... did I understood it correctly? (Needs some work to be done...)

I had a quick look and yes, that's basically what I had in mind to
solve the issue. If callback is not available the old system should
be used of course.

> [...]
>
>  static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
> index 1a5d944..3a2d442 100644
> --- a/drivers/media/platform/video-bus-switch.c
> +++ b/drivers/media/platform/video-bus-switch.c
> @@ -247,12 +247,21 @@ static int vbs_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
>  
> +	/* FIXME: we need to set the GPIO here */
> +

The gpio is set when the pad is selected, so no need to do it again.
The gpio selection actually works with your branch (assuming its
based on Ivo's).

>  	if (IS_ERR(subdev))
>  		return PTR_ERR(subdev);
>  
>  	return v4l2_subdev_call(subdev, video, s_stream, enable);
>  }
>  
> +static int vbs_g_endpoint_config(struct v4l2_subdev *sd, struct isp_bus_cfg *cfg)
> +{
> +	printk("vbs_g_endpoint_config...\n");
> +	return 0;
> +}

Would be nice to find something more abstract than isp_bus_cfg,
which is specific to omap3isp.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]