| 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