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

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Thu, Dec 22, 2016 at 09:53:17PM +0100, Pavel Machek wrote:
> > 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.
> 
> Ok, I see, where "the code" is basically in vbs_link_setup, right?

I'm not talking about the video bus switch, but about omap3isp.
omap3isp must update the buscfg registers when it starts streaming
instead of at probe time. I just checked and it actually seems to do
so already. So the problem is only updating the buscfg inside of
ccp2_s_stream() before writing the device registers. See next
paragraph for more details.

> > 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 basically... (struct isp_bus_cfg *) isd->bus would change in
> isp_pipeline_enable()...? 

isp_of_parse_node_csi1(), which is called by isp_of_parse_node()
inits buscfg using DT information. This does not work for N900,
which needs two different buscfg settings based on the selected
camera. I suggest to add a callback to the subdevice instead.

So something like pseudocode is needed in ccp2_s_stream():

/* get current buscfg */
if (subdevice->get_buscfg)
    buscfg = subdevice->get_buscfg();
else
    buscfg = isp_of_parse_node_csi1();

/* configure device registers */
ccp2_if_configure(buscfg);

This new callback must be implemented in the video-bus-switch,
so that it returns the buscfg based upon the selected camera.

> > Apart from that Sakari told me at ELCE, that the port numbers
> > should be reversed to match the order of other drivers. That's
> > obviously very easy to do :)
> 
> Ok, I guess that can come later :-).
> 
> > Regarding the binding document. I actually did write one:
> > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/commit/?h=n900-camera&id=81e74af53fe6d180616b05792f78badc615e871f
> 
> Thanks, got it.
> 
> > So all in all it shouldn't be that hard to implement the remaining
> > bits.
> 
> :-).
> 
> > (*) Actually it does not for CCP2, but there are some old patches
> > from Sakari adding it for CCP2. It is implemented for parallel port
> > and CSI in this way.
> 
> I think I got the patches in my tree. Camera currently works for me.

If you have working camera you have the CCP2 DT patches in your tree.
They are not yet mainline, though. As far as I can see.

-- Sebastian

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