Message ID | 1527583688-314-7-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, On Tue, May 29, 2018 at 10:48:04AM +0200, Jacopo Mondi wrote: > The rcar-vin driver so far had a mutually exclusive code path for > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > use case supporting media-controller. As we add support for parallel > inputs to Gen3 media-controller compliant code path now parse both port@0 > and port@1, handling the media-controller use case in the parallel > bound/unbind notifier operations. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > v4 -> v5: > - Re-group rvin_mc_init() function > - Add error_group_unreg error path to clean up group registration > - Change rvin_parallel_init() return type to make sure Gen2 works as before > > v3 -> v4: > - Change the mc/parallel initialization order. Initialize mc first, then > parallel > - As a consequence no need to delay parallel notifiers registration, the > media controller is set up already when parallel input got parsed, > this greatly simplify the group notifier complete callback. > > --- > drivers/media/platform/rcar-vin/rcar-core.c | 53 +++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index fc98986..c1d6feb 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > + if (vin->info->use_mc) { > + vin->parallel->subdev = subdev; > + return 0; > + } > + > /* Find compatible subdevices mbus format */ > vin->mbus_code = 0; > code.index = 0; > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > { > rvin_v4l2_unregister(vin); > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - > - vin->vdev.ctrl_handler = NULL; > vin->parallel->subdev = NULL; > + > + if (!vin->info->use_mc) { > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > + } > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -552,18 +559,19 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > return 0; > } > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > +static int rvin_parallel_init(struct rvin_dev *vin) > { > int ret; > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > - vin->dev, &vin->notifier, > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > + 0, rvin_parallel_parse_v4l2); > if (ret) > return ret; > > + /* If using mc, it's fine not to have any input registered. */ > if (!vin->parallel) > - return -ENODEV; > + return vin->info->use_mc ? 0 : -ENODEV; > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > to_of_node(vin->parallel->asd.match.fwnode)); > @@ -1084,20 +1092,35 @@ static int rcar_vin_probe(struct platform_device *pdev) > return ret; > > platform_set_drvdata(pdev, vin); > - if (vin->info->use_mc) > + > + if (vin->info->use_mc) { > ret = rvin_mc_init(vin); > - else > - ret = rvin_parallel_graph_init(vin); > - if (ret < 0) > - goto error; > + if (ret) > + goto error_dma_unregister; > + } > + > + ret = rvin_parallel_init(vin); > + if (ret) > + goto error_group_unregister; > > pm_suspend_ignore_children(&pdev->dev, true); > pm_runtime_enable(&pdev->dev); > > return 0; > -error: > + > +error_group_unreg: I just noticed I forgot to add a change before formatting out patches. This label name is wrong. I'll wait for other comments, and will send v6 with this fixed. Sorry about that. Thanks j > + if (vin->info->use_mc) { > + mutex_lock(&vin->group->lock); > + if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { > + v4l2_async_notifier_unregister(&vin->group->notifier); > + v4l2_async_notifier_cleanup(&vin->group->notifier); > + } > + mutex_unlock(&vin->group->lock); > + rvin_group_put(vin); > + } > + > +error_dma_unreg: > rvin_dma_unregister(vin); > - v4l2_async_notifier_cleanup(&vin->notifier); > > return ret; > } > -- > 2.7.4 >
Hi Jacopo, I love your patch! Yet something to improve: [auto build test ERROR on linuxtv-media/master] [also build test ERROR on next-20180530] [cannot apply to v4.17-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/rcar-vin-Add-support-for-parallel-input-on-Gen3/20180531-182328 base: git://linuxtv.org/media_tree.git master config: x86_64-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/media//platform/rcar-vin/rcar-core.c: In function 'rcar_vin_probe': drivers/media//platform/rcar-vin/rcar-core.c:1122:1: warning: label 'error_dma_unreg' defined but not used [-Wunused-label] error_dma_unreg: ^~~~~~~~~~~~~~~ drivers/media//platform/rcar-vin/rcar-core.c:1111:1: warning: label 'error_group_unreg' defined but not used [-Wunused-label] error_group_unreg: ^~~~~~~~~~~~~~~~~ >> drivers/media//platform/rcar-vin/rcar-core.c:1104:3: error: label 'error_group_unregister' used but not defined goto error_group_unregister; ^~~~ >> drivers/media//platform/rcar-vin/rcar-core.c:1099:4: error: label 'error_dma_unregister' used but not defined goto error_dma_unregister; ^~~~ sparse warnings: (new ones prefixed by >>) >> drivers/media/platform/rcar-vin/rcar-core.c:1099:25: sparse: label 'error_dma_unregister' was not declared >> drivers/media/platform/rcar-vin/rcar-core.c:1104:17: sparse: label 'error_group_unregister' was not declared drivers/media/platform/rcar-vin/rcar-core.c: In function 'rcar_vin_probe': drivers/media/platform/rcar-vin/rcar-core.c:1122:1: warning: label 'error_dma_unreg' defined but not used [-Wunused-label] error_dma_unreg: ^~~~~~~~~~~~~~~ drivers/media/platform/rcar-vin/rcar-core.c:1111:1: warning: label 'error_group_unreg' defined but not used [-Wunused-label] error_group_unreg: ^~~~~~~~~~~~~~~~~ drivers/media/platform/rcar-vin/rcar-core.c:1104:3: error: label 'error_group_unregister' used but not defined goto error_group_unregister; ^~~~ drivers/media/platform/rcar-vin/rcar-core.c:1099:4: error: label 'error_dma_unregister' used but not defined goto error_dma_unregister; ^~~~ vim +/error_group_unregister +1104 drivers/media//platform/rcar-vin/rcar-core.c 1055 1056 static int rcar_vin_probe(struct platform_device *pdev) 1057 { 1058 const struct soc_device_attribute *attr; 1059 struct rvin_dev *vin; 1060 struct resource *mem; 1061 int irq, ret; 1062 1063 vin = devm_kzalloc(&pdev->dev, sizeof(*vin), GFP_KERNEL); 1064 if (!vin) 1065 return -ENOMEM; 1066 1067 vin->dev = &pdev->dev; 1068 vin->info = of_device_get_match_data(&pdev->dev); 1069 1070 /* 1071 * Special care is needed on r8a7795 ES1.x since it 1072 * uses different routing than r8a7795 ES2.0. 1073 */ 1074 attr = soc_device_match(r8a7795es1); 1075 if (attr) 1076 vin->info = attr->data; 1077 1078 mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); 1079 if (mem == NULL) 1080 return -EINVAL; 1081 1082 vin->base = devm_ioremap_resource(vin->dev, mem); 1083 if (IS_ERR(vin->base)) 1084 return PTR_ERR(vin->base); 1085 1086 irq = platform_get_irq(pdev, 0); 1087 if (irq < 0) 1088 return irq; 1089 1090 ret = rvin_dma_register(vin, irq); 1091 if (ret) 1092 return ret; 1093 1094 platform_set_drvdata(pdev, vin); 1095 1096 if (vin->info->use_mc) { 1097 ret = rvin_mc_init(vin); 1098 if (ret) > 1099 goto error_dma_unregister; 1100 } 1101 1102 ret = rvin_parallel_init(vin); 1103 if (ret) > 1104 goto error_group_unregister; 1105 1106 pm_suspend_ignore_children(&pdev->dev, true); 1107 pm_runtime_enable(&pdev->dev); 1108 1109 return 0; 1110 1111 error_group_unreg: 1112 if (vin->info->use_mc) { 1113 mutex_lock(&vin->group->lock); 1114 if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { 1115 v4l2_async_notifier_unregister(&vin->group->notifier); 1116 v4l2_async_notifier_cleanup(&vin->group->notifier); 1117 } 1118 mutex_unlock(&vin->group->lock); 1119 rvin_group_put(vin); 1120 } 1121 > 1122 error_dma_unreg: 1123 rvin_dma_unregister(vin); 1124 1125 return ret; 1126 } 1127 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jacopo, Thanks for your patch. On 2018-05-29 10:48:04 +0200, Jacopo Mondi wrote: > The rcar-vin driver so far had a mutually exclusive code path for > handling parallel and CSI-2 video input subdevices, with only the CSI-2 > use case supporting media-controller. As we add support for parallel > inputs to Gen3 media-controller compliant code path now parse both port@0 > and port@1, handling the media-controller use case in the parallel > bound/unbind notifier operations. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> I think this patch looks fine except the label name mismatch. I think it's ready but will hold off with my tag until I can review a complete version of the patch :-) > > --- > v4 -> v5: > - Re-group rvin_mc_init() function > - Add error_group_unreg error path to clean up group registration > - Change rvin_parallel_init() return type to make sure Gen2 works as before > > v3 -> v4: > - Change the mc/parallel initialization order. Initialize mc first, then > parallel > - As a consequence no need to delay parallel notifiers registration, the > media controller is set up already when parallel input got parsed, > this greatly simplify the group notifier complete callback. > > --- > drivers/media/platform/rcar-vin/rcar-core.c | 53 +++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index fc98986..c1d6feb 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > vin->parallel->sink_pad = ret < 0 ? 0 : ret; > > + if (vin->info->use_mc) { > + vin->parallel->subdev = subdev; > + return 0; > + } > + > /* Find compatible subdevices mbus format */ > vin->mbus_code = 0; > code.index = 0; > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, > static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) > { > rvin_v4l2_unregister(vin); > - v4l2_ctrl_handler_free(&vin->ctrl_handler); > - > - vin->vdev.ctrl_handler = NULL; > vin->parallel->subdev = NULL; > + > + if (!vin->info->use_mc) { > + v4l2_ctrl_handler_free(&vin->ctrl_handler); > + vin->vdev.ctrl_handler = NULL; > + } > } > > static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) > @@ -552,18 +559,19 @@ static int rvin_parallel_parse_v4l2(struct device *dev, > return 0; > } > > -static int rvin_parallel_graph_init(struct rvin_dev *vin) > +static int rvin_parallel_init(struct rvin_dev *vin) > { > int ret; > > - ret = v4l2_async_notifier_parse_fwnode_endpoints( > - vin->dev, &vin->notifier, > - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); > + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( > + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), > + 0, rvin_parallel_parse_v4l2); > if (ret) > return ret; > > + /* If using mc, it's fine not to have any input registered. */ > if (!vin->parallel) > - return -ENODEV; > + return vin->info->use_mc ? 0 : -ENODEV; > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > to_of_node(vin->parallel->asd.match.fwnode)); > @@ -1084,20 +1092,35 @@ static int rcar_vin_probe(struct platform_device *pdev) > return ret; > > platform_set_drvdata(pdev, vin); > - if (vin->info->use_mc) > + > + if (vin->info->use_mc) { > ret = rvin_mc_init(vin); > - else > - ret = rvin_parallel_graph_init(vin); > - if (ret < 0) > - goto error; > + if (ret) > + goto error_dma_unregister; > + } > + > + ret = rvin_parallel_init(vin); > + if (ret) > + goto error_group_unregister; > > pm_suspend_ignore_children(&pdev->dev, true); > pm_runtime_enable(&pdev->dev); > > return 0; > -error: > + > +error_group_unreg: > + if (vin->info->use_mc) { > + mutex_lock(&vin->group->lock); > + if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { > + v4l2_async_notifier_unregister(&vin->group->notifier); > + v4l2_async_notifier_cleanup(&vin->group->notifier); > + } > + mutex_unlock(&vin->group->lock); > + rvin_group_put(vin); > + } > + > +error_dma_unreg: > rvin_dma_unregister(vin); > - v4l2_async_notifier_cleanup(&vin->notifier); > > return ret; > } > -- > 2.7.4 >
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index fc98986..c1d6feb 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); vin->parallel->sink_pad = ret < 0 ? 0 : ret; + if (vin->info->use_mc) { + vin->parallel->subdev = subdev; + return 0; + } + /* Find compatible subdevices mbus format */ vin->mbus_code = 0; code.index = 0; @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin, static void rvin_parallel_subdevice_detach(struct rvin_dev *vin) { rvin_v4l2_unregister(vin); - v4l2_ctrl_handler_free(&vin->ctrl_handler); - - vin->vdev.ctrl_handler = NULL; vin->parallel->subdev = NULL; + + if (!vin->info->use_mc) { + v4l2_ctrl_handler_free(&vin->ctrl_handler); + vin->vdev.ctrl_handler = NULL; + } } static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier) @@ -552,18 +559,19 @@ static int rvin_parallel_parse_v4l2(struct device *dev, return 0; } -static int rvin_parallel_graph_init(struct rvin_dev *vin) +static int rvin_parallel_init(struct rvin_dev *vin) { int ret; - ret = v4l2_async_notifier_parse_fwnode_endpoints( - vin->dev, &vin->notifier, - sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2); + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port( + vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity), + 0, rvin_parallel_parse_v4l2); if (ret) return ret; + /* If using mc, it's fine not to have any input registered. */ if (!vin->parallel) - return -ENODEV; + return vin->info->use_mc ? 0 : -ENODEV; vin_dbg(vin, "Found parallel subdevice %pOF\n", to_of_node(vin->parallel->asd.match.fwnode)); @@ -1084,20 +1092,35 @@ static int rcar_vin_probe(struct platform_device *pdev) return ret; platform_set_drvdata(pdev, vin); - if (vin->info->use_mc) + + if (vin->info->use_mc) { ret = rvin_mc_init(vin); - else - ret = rvin_parallel_graph_init(vin); - if (ret < 0) - goto error; + if (ret) + goto error_dma_unregister; + } + + ret = rvin_parallel_init(vin); + if (ret) + goto error_group_unregister; pm_suspend_ignore_children(&pdev->dev, true); pm_runtime_enable(&pdev->dev); return 0; -error: + +error_group_unreg: + if (vin->info->use_mc) { + mutex_lock(&vin->group->lock); + if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) { + v4l2_async_notifier_unregister(&vin->group->notifier); + v4l2_async_notifier_cleanup(&vin->group->notifier); + } + mutex_unlock(&vin->group->lock); + rvin_group_put(vin); + } + +error_dma_unreg: rvin_dma_unregister(vin); - v4l2_async_notifier_cleanup(&vin->notifier); return ret; }
The rcar-vin driver so far had a mutually exclusive code path for handling parallel and CSI-2 video input subdevices, with only the CSI-2 use case supporting media-controller. As we add support for parallel inputs to Gen3 media-controller compliant code path now parse both port@0 and port@1, handling the media-controller use case in the parallel bound/unbind notifier operations. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- v4 -> v5: - Re-group rvin_mc_init() function - Add error_group_unreg error path to clean up group registration - Change rvin_parallel_init() return type to make sure Gen2 works as before v3 -> v4: - Change the mc/parallel initialization order. Initialize mc first, then parallel - As a consequence no need to delay parallel notifiers registration, the media controller is set up already when parallel input got parsed, this greatly simplify the group notifier complete callback. --- drivers/media/platform/rcar-vin/rcar-core.c | 53 +++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 15 deletions(-) -- 2.7.4