[RFC/PATCH] media: Add video bus switch
Commit Message
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...)
Comments
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
Hi!
> > 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.
Ok, got it to work, thanks for all the help. I'll clean it up now.
Best regards,
Pavel
@@ -702,6 +704,33 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
entity = &pipe->output->video.entity;
while (1) {
+ struct v4l2_of_endpoint vep;
+ pad = &entity->pads[0];
+ if (!(pad->flags & MEDIA_PAD_FL_SINK))
+ break;
+
+ pad = media_entity_remote_pad(pad);
+ if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+ break;
+
+ entity = pad->entity;
+ subdev = media_entity_to_v4l2_subdev(entity);
+
+ printk("Entity = %p\n", entity);
+ ret = v4l2_subdev_call(subdev, video, g_endpoint_config, &vep);
+ /* Is there better method than walking a list?
+ Can I easily get dev and isd pointers here? */
+#if 0
+ if (ret == 0) {
+ printk("success\n");
+ /* notifier->subdevs[notifier->num_subdevs] ... contains isd */
+ isp_endpoint_to_buscfg(dev, vep, isd->bus);
+ }
+#endif
+ }
+
+ entity = &pipe->output->video.entity;
+ while (1) {
pad = &entity->pads[0];
if (!(pad->flags & MEDIA_PAD_FL_SINK))
break;
@@ -2099,27 +2128,8 @@ static void isp_of_parse_node_csi2(struct device *dev,
buscfg->bus.csi2.crc = 1;
}
-static int isp_of_parse_node_endpoint(struct device *dev,
- struct device_node *node,
- struct isp_async_subdev *isd)
+static int isp_endpoint_to_buscfg(struct device *dev, struct v4l2_of_endpoint vep, struct isp_bus_cfg *buscfg)
{
- struct isp_bus_cfg *buscfg;
- struct v4l2_of_endpoint vep;
- int ret;
-
- isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
- if (!isd->bus)
- return -ENOMEM;
-
- buscfg = isd->bus;
-
- ret = v4l2_of_parse_endpoint(node, &vep);
- if (ret)
- return ret;
-
- dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
- vep.base.port);
-
switch (vep.base.port) {
case ISP_OF_PHY_PARALLEL:
buscfg->interface = ISP_INTERFACE_PARALLEL;
@@ -2147,10 +2157,35 @@ static int isp_of_parse_node_endpoint(struct device *dev,
break;
default:
+ return -1;
+ }
+ return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+ struct device_node *node,
+ struct isp_async_subdev *isd)
+{
+ struct isp_bus_cfg *buscfg;
+ struct v4l2_of_endpoint vep;
+ int ret;
+
+ isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
+ if (!isd->bus)
+ return -ENOMEM;
+
+ buscfg = isd->bus;
+
+ ret = v4l2_of_parse_endpoint(node, &vep);
+ if (ret)
+ return ret;
+
+ dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+ vep.base.port);
+
+ if (isp_endpoint_to_buscfg(dev, vep, buscfg))
dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
vep.base.port);
- break;
- }
return 0;
}
@@ -2262,6 +2297,10 @@ static int isp_of_parse_nodes(struct device *dev,
}
return notifier->num_subdevs;
+
+error:
+ of_node_put(node);
+ return -EINVAL;
}
static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
@@ -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 */
+
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;
+}
+
+
static const struct v4l2_subdev_internal_ops vbs_internal_ops = {
.registered = &vbs_registered,
};
@@ -265,6 +274,7 @@ static const struct media_entity_operations vbs_media_ops = {
/* subdev video operations */
static const struct v4l2_subdev_video_ops vbs_video_ops = {
.s_stream = vbs_s_stream,
+ .g_endpoint_config = vbs_g_endpoint_config,
};
static const struct v4l2_subdev_ops vbs_ops = {
@@ -415,6 +415,8 @@ struct v4l2_subdev_video_ops {
const struct v4l2_mbus_config *cfg);
int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
unsigned int *size);
+ int (*g_endpoint_config)(struct v4l2_subdev *sd,
+ struct v4l2_of_endpoint *cfg);
};
/**