Re: [PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[jacopo mondi]

Hi Niklas,

On Fri, May 11, 2018 at 01:01:24PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work!
>
> The comments here apply to both 2/5 and 3/5.
>
> On 2018-05-11 11:59:38 +0200, Jacopo Mondi wrote:
> > Add support for digital input subdevices to Gen-3 rcar-vin.
> > The Gen-3, media-controller compliant, version has so far only accepted
> > CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> > accepted number of subdevices, and allow digital input connections on 
> > port@0.
>
> I feel this much more complex then it has to be. You defer parsing the
> DT graph until all VIN's are probed just like it's done for the port@1
> parsing. For the CSI-2 endpoints in port@1 this is needed as they are
> shared for all VIN's in the group. While for the parallel input this is
> local for each VIN and could be parsed at probe time for each individual
> VIN. As a bonus for doing that I think you could reuse the parallel
> parsing functions from the Gen2 code whit small additions.
>
> Maybe something like this can be done:
>
> - At each VIN probe() rework the block
>
> if (vin->info->use_mc)
>   ret = rvin_mc_init(vin);
> else
>   ret = rvin_digital_graph_init(vin);
>
>   To:
> ret = rvin_digital_graph_init(vin);
> ...
> ret = rvin_mc_init(vin);
> ...
>
> - Then in rvin_digital_graph_init() don't call
>   v4l2_async_notifier_register() if vin->info->use_mc is set.
>
>   And instead as a last step in rvin_mc_init() call
>   v4l2_async_notifier_register() for each vin->notifier that contains a
>   subdevice.
>
> - As a last step add a check for vin->info->use_mc in
>   rvin_digital_notify_complete() and handle the case for Gen2 behavior
>   (what it does now) and Gen3 behavior (adding the MC link).
>
>
> I think my doing this you could greatly simplify the process of handling
> the parallel subdevice.
>
> Please see bellow for one additional comment.

Thanks for the suggestion.
My understanding was that the 'Gen2' was planned for removal in the
long term, and we should have paved the road for a
media-controller-only version of the driver. But I admit I didn't even
try to re-use that code part, I'll give your suggestions a spin, if
this won't prevent future removal of the non-mc part of the driver.
>
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 166 
> > +++-
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
> >  2 files changed, 153 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index e547ef7..105b6b6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations 
> > rvin_group_notify_ops = {
> > .complete = rvin_group_notify_complete,

[snip]

> > +
> > +   switch (vep->base.port) {
> > +   case RVIN_PORT_DIGITAL:
> > +   ret = rvin_mc_parse_of_digital(vin, vep, asd);
> > +   break;
> > +   case RVIN_PORT_CSI2:
> > +   default:
> > +   ret = rvin_mc_parse_of_csi2(vin, vep, asd);
> > +   break;
> > +   }
> > +   /*

[snip]

> > +
> > +   switch (ep.port) {
> > +   case RVIN_PORT_DIGITAL:
> > +   asd_struct_size = sizeof(struct rvin_graph_entity);
> > +   break;
> > +   case RVIN_PORT_CSI2:
> > +   asd_struct_size = sizeof(struct v4l2_async_subdev);
> > +   break;
> > +   default:
> >  };

[snip]

> >
> >  /**
> > + * enum rvin_port_id
> > + *
> > + * List the available VIN port functions.
> > + *
> > + * RVIN_PORT_DIGITAL   - Input port for digital video connection
> > + * RVIN_PORT_CSI2  - Input port for CSI-2 video connection
> > + */
> > +enum rvin_port_id {
> > +   RVIN_PORT_DIGITAL,
> > +   RVIN_PORT_CSI2
> > +};
>
> This enum is never used in the patch-set is it used later by a different
> patch-set or a left over from refactoring?

I see two occurencies each of this enumeration members in this patch.
I left them un-cut above.

Thanks
   j


signature.asc
Description: PGP signature

Re: [PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work!

The comments here apply to both 2/5 and 3/5.

On 2018-05-11 11:59:38 +0200, Jacopo Mondi wrote:
> Add support for digital input subdevices to Gen-3 rcar-vin.
> The Gen-3, media-controller compliant, version has so far only accepted
> CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> accepted number of subdevices, and allow digital input connections on port@0.

I feel this much more complex then it has to be. You defer parsing the 
DT graph until all VIN's are probed just like it's done for the port@1 
parsing. For the CSI-2 endpoints in port@1 this is needed as they are 
shared for all VIN's in the group. While for the parallel input this is 
local for each VIN and could be parsed at probe time for each individual 
VIN. As a bonus for doing that I think you could reuse the parallel 
parsing functions from the Gen2 code whit small additions.

Maybe something like this can be done:

- At each VIN probe() rework the block

if (vin->info->use_mc)
ret = rvin_mc_init(vin);
else
ret = rvin_digital_graph_init(vin);

  To:
ret = rvin_digital_graph_init(vin);
...
ret = rvin_mc_init(vin);
...

- Then in rvin_digital_graph_init() don't call 
  v4l2_async_notifier_register() if vin->info->use_mc is set.

  And instead as a last step in rvin_mc_init() call 
  v4l2_async_notifier_register() for each vin->notifier that contains a 
  subdevice.

- As a last step add a check for vin->info->use_mc in 
  rvin_digital_notify_complete() and handle the case for Gen2 behavior 
  (what it does now) and Gen3 behavior (adding the MC link).


I think my doing this you could greatly simplify the process of handling 
the parallel subdevice.

Please see bellow for one additional comment.

> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 166 
> +++-
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
>  2 files changed, 153 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index e547ef7..105b6b6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations 
> rvin_group_notify_ops = {
>   .complete = rvin_group_notify_complete,
>  };
>  
> -static int rvin_mc_parse_of_endpoint(struct device *dev,
> -  struct v4l2_fwnode_endpoint *vep,
> -  struct v4l2_async_subdev *asd)
> +static int rvin_mc_parse_of_csi2(struct rvin_dev *vin,
> +  struct v4l2_fwnode_endpoint *vep,
> +  struct v4l2_async_subdev *asd)
>  {
> - struct rvin_dev *vin = dev_get_drvdata(dev);
> -
> - if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> + if (vep->base.id >= RVIN_CSI_MAX)
>   return -EINVAL;
>  
> - if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> -
> - vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> - to_of_node(asd->match.fwnode));
> - return -ENOTCONN;
> -
> - }
> -
>   if (vin->group->csi[vep->base.id].fwnode) {
>   vin_dbg(vin, "OF device %pOF already handled\n",
>   to_of_node(asd->match.fwnode));
>   return -ENOTCONN;
>   }
>  
> + vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> + vin->mbus_cfg.flags = 0;
>   vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>   vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -728,9 +720,97 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>   return 0;
>  }
>  
> +static int rvin_mc_parse_of_digital(struct rvin_dev *vin,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> +{
> + if (vep->base.id)
> + return -EINVAL;
> +
> + vin->mbus_cfg.type = vep->bus_type;
> + if (vin->mbus_cfg.type == V4L2_MBUS_PARALLEL)
> + vin->mbus_cfg.flags = vep->bus.parallel.flags;
> + else
> + vin->mbus_cfg.flags = 0;
> +
> + /*
> +  * 'v4l2_async_notifier_fwnode_parse_endpoint()' has reserved
> +  * enough memory for a whole 'rvin_grap_entity' structure, whose 'asd'
> +  * is the first member of. Safely cast it to the 'digital' field of
> +  * the selected vin instance.
> +  */
> + vin->digital = (struct rvin_graph_entity *)asd;
> +
> + vin_dbg(vin, "Add OF device %pOF as VIN%u digital input\n",
> + to_of_node(asd->match.fwnode), vin->id);
> +
> + return 0;
> +}
> +
> +static int rvin_mc_parse_of_endpoint(struct device *dev,
> +  struct v4l2_fwnode_endpoint *vep,
> +  

[PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[Jacopo Mondi]

Add support for digital input subdevices to Gen-3 rcar-vin.
The Gen-3, media-controller compliant, version has so far only accepted
CSI-2 input subdevices. Remove assumptions on the supported bus_type and
accepted number of subdevices, and allow digital input connections on port@0.

Signed-off-by: Jacopo Mondi 
---
 drivers/media/platform/rcar-vin/rcar-core.c | 166 +++-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
 2 files changed, 153 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
b/drivers/media/platform/rcar-vin/rcar-core.c
index e547ef7..105b6b6 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations 
rvin_group_notify_ops = {
.complete = rvin_group_notify_complete,
 };
 
-static int rvin_mc_parse_of_endpoint(struct device *dev,
-struct v4l2_fwnode_endpoint *vep,
-struct v4l2_async_subdev *asd)
+static int rvin_mc_parse_of_csi2(struct rvin_dev *vin,
+struct v4l2_fwnode_endpoint *vep,
+struct v4l2_async_subdev *asd)
 {
-   struct rvin_dev *vin = dev_get_drvdata(dev);
-
-   if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
+   if (vep->base.id >= RVIN_CSI_MAX)
return -EINVAL;
 
-   if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
-
-   vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
-   to_of_node(asd->match.fwnode));
-   return -ENOTCONN;
-
-   }
-
if (vin->group->csi[vep->base.id].fwnode) {
vin_dbg(vin, "OF device %pOF already handled\n",
to_of_node(asd->match.fwnode));
return -ENOTCONN;
}
 
+   vin->mbus_cfg.type = V4L2_MBUS_CSI2;
+   vin->mbus_cfg.flags = 0;
vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
 
vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
@@ -728,9 +720,97 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
return 0;
 }
 
+static int rvin_mc_parse_of_digital(struct rvin_dev *vin,
+   struct v4l2_fwnode_endpoint *vep,
+   struct v4l2_async_subdev *asd)
+{
+   if (vep->base.id)
+   return -EINVAL;
+
+   vin->mbus_cfg.type = vep->bus_type;
+   if (vin->mbus_cfg.type == V4L2_MBUS_PARALLEL)
+   vin->mbus_cfg.flags = vep->bus.parallel.flags;
+   else
+   vin->mbus_cfg.flags = 0;
+
+   /*
+* 'v4l2_async_notifier_fwnode_parse_endpoint()' has reserved
+* enough memory for a whole 'rvin_grap_entity' structure, whose 'asd'
+* is the first member of. Safely cast it to the 'digital' field of
+* the selected vin instance.
+*/
+   vin->digital = (struct rvin_graph_entity *)asd;
+
+   vin_dbg(vin, "Add OF device %pOF as VIN%u digital input\n",
+   to_of_node(asd->match.fwnode), vin->id);
+
+   return 0;
+}
+
+static int rvin_mc_parse_of_endpoint(struct device *dev,
+struct v4l2_fwnode_endpoint *vep,
+struct v4l2_async_subdev *asd)
+{
+   struct rvin_dev *group_vin = dev_get_drvdata(dev);
+   struct rvin_group *group = group_vin->group;
+   struct fwnode_handle *fw_vin;
+   struct fwnode_handle *fw_port;
+   struct rvin_dev *vin;
+   unsigned int i;
+   int ret;
+
+   if (!fwnode_device_is_available(asd->match.fwnode)) {
+   vin_dbg(group_vin, "OF device %pOF disabled, ignoring\n",
+   to_of_node(asd->match.fwnode));
+   return -ENOTCONN;
+   }
+
+   /*
+* Find out to which VIN instance this ep belongs to.
+*
+* While the list of async subdevices and the async notifier
+* belong to the whole group, the bus configuration properties
+* are instance specific; find the instance by matching its fwnode.
+*/
+   fw_port = fwnode_get_parent(vep->base.local_fwnode);
+   fw_vin = fwnode_graph_get_port_parent(fw_port);
+   fwnode_handle_put(fw_port);
+   if (!fw_vin)
+   return -ENOENT;
+
+   for (i = 0; i < RCAR_VIN_NUM; i++) {
+   if (!group->vin[i])
+   continue;
+
+   if (fw_vin == dev_fwnode(group->vin[i]->dev))
+   break;
+   }
+   fwnode_handle_put(fw_vin);
+
+   if (i == RCAR_VIN_NUM) {
+   vin_err(group_vin, "Unable to find VIN device for %pOF\n",
+   to_of_node(asd->match.fwnode));
+   return -ENOENT;
+   }
+   vin = group->vin[i];
+
+   switch (vep->base.port) {
+