Re: [PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Niklas Söderlund]

Hi Kieran,

Thanks for your comments,

On 2017-12-14 23:27:57 +, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/12/17 19:08, Niklas Söderlund wrote:
> > To support multiplexed streams the internal routing between the
> > adv748x sink pad and its source pad needs to be described.
> 
> The adv748x has quite a few sink and source pads... I presume here you mean 
> the
> adv748x csi2 sink and source pad :D

Yes :-) Will fix for next version, thanks.

> 
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c 
> > b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 291b35bef49d41fb..dbefb53f5b8c414d 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct 
> > v4l2_subdev *sd, unsigned int pad,
> > return 0;
> >  }
> >  
> > +static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
> > +   struct v4l2_subdev_routing *routing)
> > +{
> > +   struct v4l2_subdev_route *r = routing->routes;
> > +
> > +   if (routing->num_routes < 1) {
> > +   routing->num_routes = 1;
> > +   return -ENOSPC;
> > +   }
> > +
> > +   routing->num_routes = 1;
> > +
> > +   r->sink_pad = ADV748X_CSI2_SINK;
> > +   r->sink_stream = 0;
> > +   r->source_pad = ADV748X_CSI2_SOURCE;
> > +   r->source_stream = 0;
> > +   r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
> > +
> > +   return 0;
> > +}
> > +
> 
> So - I think this is fine - but it seems a lot of code to define a static
> default route which describes a single link between it's sink pad - and its
> source pad ...
> 
> I suspect this should/could be wrapped by some helpers in core for cases like
> this, as it's the simple case - but as we don't currently have that I guess we
> have to put this in here for now ?

Yes for now we need to fill in the information here.

> 
> Maybe we should have a helper to make this

I'm sure there could be v4l2 helpers for such a case. I don't even think 
you wound need to prime it with any information. If there is only one 
source and one sink I'm sure a helper function can figure it out :-)

> 
> return v4l2_subdev_single_route(subdev, routing,
> ADV748X_CS2_SINK, 0,
> ADV748X_CSI2_SOURCE, 0,
>   V4L2_SUBDEV_ROUTE_FL_ACTIVE | 
> V4L2_SUBDEV_ROUTE_FL_IMMUTABLE);
> 
> Or maybe even define these static routes in a struct somehow?

For more complex setups a struct could be used together with a helper 
function to decode it. But then again maybe it's easier to just define a 
const v4l2_subdev_route array and 'routing->routes = my_const_routes' ?

> 
> >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > .get_fmt = adv748x_csi2_get_format,
> > .set_fmt = adv748x_csi2_set_format,
> > .get_frame_desc = adv748x_csi2_get_frame_desc,
> > +   .get_routing = adv748x_csi2_get_routing,
> > .s_stream = adv748x_csi2_s_stream,
> >  };
> >  
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Kieran Bingham]

Hi Niklas,

On 14/12/17 19:08, Niklas Söderlund wrote:
> To support multiplexed streams the internal routing between the
> adv748x sink pad and its source pad needs to be described.

The adv748x has quite a few sink and source pads... I presume here you mean the
adv748x csi2 sink and source pad :D

> 
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c 
> b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 291b35bef49d41fb..dbefb53f5b8c414d 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct 
> v4l2_subdev *sd, unsigned int pad,
>   return 0;
>  }
>  
> +static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_routing *routing)
> +{
> + struct v4l2_subdev_route *r = routing->routes;
> +
> + if (routing->num_routes < 1) {
> + routing->num_routes = 1;
> + return -ENOSPC;
> + }
> +
> + routing->num_routes = 1;
> +
> + r->sink_pad = ADV748X_CSI2_SINK;
> + r->sink_stream = 0;
> + r->source_pad = ADV748X_CSI2_SOURCE;
> + r->source_stream = 0;
> + r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
> +
> + return 0;
> +}
> +

So - I think this is fine - but it seems a lot of code to define a static
default route which describes a single link between it's sink pad - and its
source pad ...

I suspect this should/could be wrapped by some helpers in core for cases like
this, as it's the simple case - but as we don't currently have that I guess we
have to put this in here for now ?

Maybe we should have a helper to make this

return v4l2_subdev_single_route(subdev, routing,
ADV748X_CS2_SINK, 0,
ADV748X_CSI2_SOURCE, 0,
  V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE);

Or maybe even define these static routes in a struct somehow?

>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>   .get_fmt = adv748x_csi2_get_format,
>   .set_fmt = adv748x_csi2_set_format,
>   .get_frame_desc = adv748x_csi2_get_frame_desc,
> + .get_routing = adv748x_csi2_get_routing,
>   .s_stream = adv748x_csi2_s_stream,
>  };
>  
> 

[PATCH/RFC v2 14/15] adv748x: csi2: add get_routing support

[Niklas Söderlund]

To support multiplexed streams the internal routing between the
adv748x sink pad and its source pad needs to be described.

Signed-off-by: Niklas Söderlund 
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c 
b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 291b35bef49d41fb..dbefb53f5b8c414d 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -262,10 +262,32 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev 
*sd, unsigned int pad,
return 0;
 }
 
+static int adv748x_csi2_get_routing(struct v4l2_subdev *subdev,
+   struct v4l2_subdev_routing *routing)
+{
+   struct v4l2_subdev_route *r = routing->routes;
+
+   if (routing->num_routes < 1) {
+   routing->num_routes = 1;
+   return -ENOSPC;
+   }
+
+   routing->num_routes = 1;
+
+   r->sink_pad = ADV748X_CSI2_SINK;
+   r->sink_stream = 0;
+   r->source_pad = ADV748X_CSI2_SOURCE;
+   r->source_stream = 0;
+   r->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE | V4L2_SUBDEV_ROUTE_FL_IMMUTABLE;
+
+   return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
.get_fmt = adv748x_csi2_get_format,
.set_fmt = adv748x_csi2_set_format,
.get_frame_desc = adv748x_csi2_get_frame_desc,
+   .get_routing = adv748x_csi2_get_routing,
.s_stream = adv748x_csi2_s_stream,
 };
 
-- 
2.15.1