| Message ID | 20210413180253.2575451-5-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | Accepted |
| Commit | 27b9a6f9e8fe1403958b0a9fa16ed53cb3a5aa1d |
| Headers | show |
| Series | rcar-vin: Add r8a779a0 support | expand |
Hi Niklas, On Tue, Apr 13, 2021 at 08:02:46PM +0200, Niklas Söderlund wrote: > In preparation for adding a new media graph layout move the code reuse > of the parallel notifier setup from probe directly to the current media > graph initialization function. This is needed as there will be no > parallel interface in the new graph layout. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++----------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index da23d55aa72b7f0d..81574bf33116ad59 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin) > if (ret) > return ret; > > - /* If using mc, it's fine not to have any input registered. */ > if (!vin->parallel.asd) > - return vin->info->use_mc ? 0 : -ENODEV; > + return -ENODEV; Nit: isn't it better to keep the error handling here ? > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > to_of_node(vin->parallel.asd->match.fwnode)); > @@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > > static void rvin_csi2_cleanup(struct rvin_dev *vin) > { > - if (!vin->info->use_mc) > - return; > - > rvin_group_notifier_cleanup(vin); > rvin_group_put(vin); > + rvin_free_controls(vin); > } > > static int rvin_csi2_init(struct rvin_dev *vin) > @@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin) > if (ret) > goto err_controls; > > - ret = rvin_mc_parse_of_graph(vin); > - if (ret) > + /* It's OK to not have a parallel subdevice. */ > + ret = rvin_parallel_init(vin); > + if (ret && ret != -ENODEV) > goto err_group; > > + ret = rvin_mc_parse_of_graph(vin); > + if (ret) > + goto err_parallel; > + > return 0; > +err_parallel: > + rvin_parallel_cleanup(vin); > err_group: > rvin_group_put(vin); > err_controls: > @@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, vin); > > - if (vin->info->use_mc) { > + if (vin->info->use_mc) > ret = rvin_csi2_init(vin); > - if (ret) > - goto error_dma_unregister; > - } > + else > + ret = rvin_parallel_init(vin); > > - ret = rvin_parallel_init(vin); > - if (ret) > - goto error_group_unregister; > + if (ret) { > + rvin_dma_unregister(vin); > + return ret; > + } > > pm_suspend_ignore_children(&pdev->dev, true); > pm_runtime_enable(&pdev->dev); > > return 0; > -error_group_unregister: > - rvin_free_controls(vin); > - rvin_csi2_cleanup(vin); > -error_dma_unregister: > - rvin_dma_unregister(vin); > - > - return ret; This looks much much better and seems correct to me! > } > > static int rcar_vin_remove(struct platform_device *pdev) > @@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev) > > rvin_v4l2_unregister(vin); > > - rvin_parallel_cleanup(vin); > - > - rvin_csi2_cleanup(vin); > - > - rvin_free_controls(vin); > + if (vin->info->use_mc) > + rvin_csi2_cleanup(vin); > + else > + rvin_parallel_cleanup(vin); In the case use_mc == true but a parallel input was registered as well this won't clean up the parallel notifier it seems. Does it hurt to clean it up unconditionally ? Thanks j > > rvin_dma_unregister(vin); > > -- > 2.31.1 >
Hi Jacopo, Thanks for your feedback. On 2021-07-06 18:51:41 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Apr 13, 2021 at 08:02:46PM +0200, Niklas Söderlund wrote: > > In preparation for adding a new media graph layout move the code reuse > > of the parallel notifier setup from probe directly to the current media > > graph initialization function. This is needed as there will be no > > parallel interface in the new graph layout. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++----------- > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index da23d55aa72b7f0d..81574bf33116ad59 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin) > > if (ret) > > return ret; > > > > - /* If using mc, it's fine not to have any input registered. */ > > if (!vin->parallel.asd) > > - return vin->info->use_mc ? 0 : -ENODEV; > > + return -ENODEV; > > Nit: isn't it better to keep the error handling here ? I'm trying to reduce the number of use_mc checks and if needed only keep it in code paths where it's relevant. I like moving it to the _csi2_ functions. But I agree _csi2_ is a bad prefix, it should likely be renamed somewhen in the future as it now really means _gen3_ minus v3u :-) > > > > > vin_dbg(vin, "Found parallel subdevice %pOF\n", > > to_of_node(vin->parallel.asd->match.fwnode)); > > @@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) > > > > static void rvin_csi2_cleanup(struct rvin_dev *vin) > > { > > - if (!vin->info->use_mc) > > - return; > > - > > rvin_group_notifier_cleanup(vin); > > rvin_group_put(vin); > > + rvin_free_controls(vin); > > } > > > > static int rvin_csi2_init(struct rvin_dev *vin) > > @@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin) > > if (ret) > > goto err_controls; > > > > - ret = rvin_mc_parse_of_graph(vin); > > - if (ret) > > + /* It's OK to not have a parallel subdevice. */ > > + ret = rvin_parallel_init(vin); > > + if (ret && ret != -ENODEV) > > goto err_group; > > > > + ret = rvin_mc_parse_of_graph(vin); > > + if (ret) > > + goto err_parallel; > > + > > return 0; > > +err_parallel: > > + rvin_parallel_cleanup(vin); > > err_group: > > rvin_group_put(vin); > > err_controls: > > @@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, vin); > > > > - if (vin->info->use_mc) { > > + if (vin->info->use_mc) > > ret = rvin_csi2_init(vin); > > - if (ret) > > - goto error_dma_unregister; > > - } > > + else > > + ret = rvin_parallel_init(vin); > > > > - ret = rvin_parallel_init(vin); > > - if (ret) > > - goto error_group_unregister; > > + if (ret) { > > + rvin_dma_unregister(vin); > > + return ret; > > + } > > > > pm_suspend_ignore_children(&pdev->dev, true); > > pm_runtime_enable(&pdev->dev); > > > > return 0; > > -error_group_unregister: > > - rvin_free_controls(vin); > > - rvin_csi2_cleanup(vin); > > -error_dma_unregister: > > - rvin_dma_unregister(vin); > > - > > - return ret; > > This looks much much better and seems correct to me! > > > } > > > > static int rcar_vin_remove(struct platform_device *pdev) > > @@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev) > > > > rvin_v4l2_unregister(vin); > > > > - rvin_parallel_cleanup(vin); > > - > > - rvin_csi2_cleanup(vin); > > - > > - rvin_free_controls(vin); > > + if (vin->info->use_mc) > > + rvin_csi2_cleanup(vin); > > + else > > + rvin_parallel_cleanup(vin); > > In the case use_mc == true but a parallel input was registered as well > this won't clean up the parallel notifier it seems. Good catch! rvin_parallel_cleanup() should be called from rvin_csi2_cleanup() as rvin_parallel_init() is called form rvin_csi2_init(). I will fix. > > Does it hurt to clean it up unconditionally ? > > Thanks > j > > > > rvin_dma_unregister(vin); > > > > -- > > 2.31.1 > > -- Regards, Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index da23d55aa72b7f0d..81574bf33116ad59 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin) if (ret) return ret; - /* If using mc, it's fine not to have any input registered. */ if (!vin->parallel.asd) - return vin->info->use_mc ? 0 : -ENODEV; + return -ENODEV; vin_dbg(vin, "Found parallel subdevice %pOF\n", to_of_node(vin->parallel.asd->match.fwnode)); @@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin) static void rvin_csi2_cleanup(struct rvin_dev *vin) { - if (!vin->info->use_mc) - return; - rvin_group_notifier_cleanup(vin); rvin_group_put(vin); + rvin_free_controls(vin); } static int rvin_csi2_init(struct rvin_dev *vin) @@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin) if (ret) goto err_controls; - ret = rvin_mc_parse_of_graph(vin); - if (ret) + /* It's OK to not have a parallel subdevice. */ + ret = rvin_parallel_init(vin); + if (ret && ret != -ENODEV) goto err_group; + ret = rvin_mc_parse_of_graph(vin); + if (ret) + goto err_parallel; + return 0; +err_parallel: + rvin_parallel_cleanup(vin); err_group: rvin_group_put(vin); err_controls: @@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev) platform_set_drvdata(pdev, vin); - if (vin->info->use_mc) { + if (vin->info->use_mc) ret = rvin_csi2_init(vin); - if (ret) - goto error_dma_unregister; - } + else + ret = rvin_parallel_init(vin); - ret = rvin_parallel_init(vin); - if (ret) - goto error_group_unregister; + if (ret) { + rvin_dma_unregister(vin); + return ret; + } pm_suspend_ignore_children(&pdev->dev, true); pm_runtime_enable(&pdev->dev); return 0; -error_group_unregister: - rvin_free_controls(vin); - rvin_csi2_cleanup(vin); -error_dma_unregister: - rvin_dma_unregister(vin); - - return ret; } static int rcar_vin_remove(struct platform_device *pdev) @@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev) rvin_v4l2_unregister(vin); - rvin_parallel_cleanup(vin); - - rvin_csi2_cleanup(vin); - - rvin_free_controls(vin); + if (vin->info->use_mc) + rvin_csi2_cleanup(vin); + else + rvin_parallel_cleanup(vin); rvin_dma_unregister(vin);
In preparation for adding a new media graph layout move the code reuse of the parallel notifier setup from probe directly to the current media graph initialization function. This is needed as there will be no parallel interface in the new graph layout. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++----------- 1 file changed, 22 insertions(+), 26 deletions(-)