Message ID | 20211206004811.1118-1-jammy_huang@aspeedtech.com (mailing list archive) |
---|---|
State | Rejected, archived |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1mu2QQ-008Hvd-TB; Mon, 06 Dec 2021 01:04:20 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234363AbhLFBHk (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sun, 5 Dec 2021 20:07:40 -0500 Received: from twspam01.aspeedtech.com ([211.20.114.71]:65393 "EHLO twspam01.aspeedtech.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240230AbhLFAwY (ORCPT <rfc822;linux-media@vger.kernel.org>); Sun, 5 Dec 2021 19:52:24 -0500 Received: from mail.aspeedtech.com ([192.168.0.24]) by twspam01.aspeedtech.com with ESMTP id 1B60NNMV022616; Mon, 6 Dec 2021 08:23:23 +0800 (GMT-8) (envelope-from jammy_huang@aspeedtech.com) Received: from JammyHuang-PC.aspeed.com (192.168.2.115) by TWMBX02.aspeed.com (192.168.0.24) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 6 Dec 2021 08:48:13 +0800 From: Jammy Huang <jammy_huang@aspeedtech.com> To: <hverkuil-cisco@xs4all.nl>, <sakari.ailus@linux.intel.com>, <gregkh@linuxfoundation.org>, <laurent.pinchart@ideasonboard.com>, <eajames@linux.ibm.com>, <mchehab@kernel.org>, <joel@jms.id.au>, <andrew@aj.id.au>, <linux-media@vger.kernel.org>, <openbmc@lists.ozlabs.org>, <linux-arm-kernel@lists.infradead.org>, <linux-aspeed@lists.ozlabs.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH v2] media: aspeed: move err-handling together to the bottom Date: Mon, 6 Dec 2021 08:48:11 +0800 Message-ID: <20211206004811.1118-1-jammy_huang@aspeedtech.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [192.168.2.115] X-ClientProxiedBy: TWMBX02.aspeed.com (192.168.0.24) To TWMBX02.aspeed.com (192.168.0.24) X-DNSRBL: X-MAIL: twspam01.aspeedtech.com 1B60NNMV022616 Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.4 (--) X-LSpam-Report: No, score=-2.4 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
[v2] media: aspeed: move err-handling together to the bottom
|
|
Commit Message
Jammy Huang
Dec. 6, 2021, 12:48 a.m. UTC
refine aspeed_video_setup_video() flow.
Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
v2:
- remove change-id in comment
---
drivers/media/platform/aspeed-video.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
Comments
Em Mon, 6 Dec 2021 08:48:11 +0800 Jammy Huang <jammy_huang@aspeedtech.com> escreveu: > refine aspeed_video_setup_video() flow. Why? It makes no difference where the error handling code is. Let's keep it as preferred by the driver's author ;-) Regards, Mauro > > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> > --- > v2: > - remove change-id in comment > --- > drivers/media/platform/aspeed-video.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c > index fea5e4d0927e..f5c40d6b4ece 100644 > --- a/drivers/media/platform/aspeed-video.c > +++ b/drivers/media/platform/aspeed-video.c > @@ -1641,11 +1641,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > > rc = video->ctrl_handler.error; > if (rc) { > - v4l2_ctrl_handler_free(&video->ctrl_handler); > - v4l2_device_unregister(v4l2_dev); > - > dev_err(video->dev, "Failed to init controls: %d\n", rc); > - return rc; > + goto err_ctrl_init; > } > > v4l2_dev->ctrl_handler = &video->ctrl_handler; > @@ -1663,11 +1660,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > > rc = vb2_queue_init(vbq); > if (rc) { > - v4l2_ctrl_handler_free(&video->ctrl_handler); > - v4l2_device_unregister(v4l2_dev); > - > dev_err(video->dev, "Failed to init vb2 queue\n"); > - return rc; > + goto err_vb2_init; > } > > vdev->queue = vbq; > @@ -1685,15 +1679,19 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > video_set_drvdata(vdev, video); > rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0); > if (rc) { > - vb2_queue_release(vbq); > - v4l2_ctrl_handler_free(&video->ctrl_handler); > - v4l2_device_unregister(v4l2_dev); > - > dev_err(video->dev, "Failed to register video device\n"); > - return rc; > + goto err_video_reg; > } > > return 0; > + > +err_video_reg: > + vb2_queue_release(vbq); > +err_vb2_init: > +err_ctrl_init: > + v4l2_ctrl_handler_free(&video->ctrl_handler); > + v4l2_device_unregister(v4l2_dev); > + return rc; > } > > static int aspeed_video_init(struct aspeed_video *video) Thanks, Mauro
Hi Mauro, On Tue, Dec 14, 2021 at 03:53:00PM +0100, Mauro Carvalho Chehab wrote: > Em Mon, 6 Dec 2021 08:48:11 +0800 > Jammy Huang <jammy_huang@aspeedtech.com> escreveu: > > > refine aspeed_video_setup_video() flow. > > Why? It makes no difference where the error handling code is. Let's > keep it as preferred by the driver's author ;-) Doing error handling can be done in different ways of course, but I think it's easier to see it's right as it's done by this patch. Of course the original author's --- like anyone else's --- review wouldn't hurt. But I see he hasn't reviewed other recent patches to the driver either. A better description would be nice though, including capital letter beginning a sentence. > > Regards, > Mauro > > > > > Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> > > --- > > v2: > > - remove change-id in comment > > --- > > drivers/media/platform/aspeed-video.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c > > index fea5e4d0927e..f5c40d6b4ece 100644 > > --- a/drivers/media/platform/aspeed-video.c > > +++ b/drivers/media/platform/aspeed-video.c > > @@ -1641,11 +1641,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > > > > rc = video->ctrl_handler.error; > > if (rc) { > > - v4l2_ctrl_handler_free(&video->ctrl_handler); > > - v4l2_device_unregister(v4l2_dev); > > - > > dev_err(video->dev, "Failed to init controls: %d\n", rc); > > - return rc; > > + goto err_ctrl_init; > > } > > > > v4l2_dev->ctrl_handler = &video->ctrl_handler; > > @@ -1663,11 +1660,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > > > > rc = vb2_queue_init(vbq); > > if (rc) { > > - v4l2_ctrl_handler_free(&video->ctrl_handler); > > - v4l2_device_unregister(v4l2_dev); > > - > > dev_err(video->dev, "Failed to init vb2 queue\n"); > > - return rc; > > + goto err_vb2_init; > > } > > > > vdev->queue = vbq; > > @@ -1685,15 +1679,19 @@ static int aspeed_video_setup_video(struct aspeed_video *video) > > video_set_drvdata(vdev, video); > > rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0); > > if (rc) { > > - vb2_queue_release(vbq); > > - v4l2_ctrl_handler_free(&video->ctrl_handler); > > - v4l2_device_unregister(v4l2_dev); > > - > > dev_err(video->dev, "Failed to register video device\n"); > > - return rc; > > + goto err_video_reg; > > } > > > > return 0; > > + > > +err_video_reg: > > + vb2_queue_release(vbq); > > +err_vb2_init: > > +err_ctrl_init: > > + v4l2_ctrl_handler_free(&video->ctrl_handler); > > + v4l2_device_unregister(v4l2_dev); > > + return rc; > > } > > > > static int aspeed_video_init(struct aspeed_video *video) > > > > Thanks, > Mauro
Hi Mauro, Because I saw similar error-handling in aspeed_video_init(), I just want to make it clear and identical. It's ok if not applied. Just style problem, as you said. On 2021/12/14 下午 10:53, Mauro Carvalho Chehab wrote: > Em Mon, 6 Dec 2021 08:48:11 +0800 > Jammy Huang <jammy_huang@aspeedtech.com> escreveu: > >> refine aspeed_video_setup_video() flow. > Why? It makes no difference where the error handling code is. Let's > keep it as preferred by the driver's author ;-) > > Regards, > Mauro > >> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> >> --- >> v2: >> - remove change-id in comment >> --- >> drivers/media/platform/aspeed-video.c | 24 +++++++++++------------- >> 1 file changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c >> index fea5e4d0927e..f5c40d6b4ece 100644 >> --- a/drivers/media/platform/aspeed-video.c >> +++ b/drivers/media/platform/aspeed-video.c >> @@ -1641,11 +1641,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >> >> rc = video->ctrl_handler.error; >> if (rc) { >> - v4l2_ctrl_handler_free(&video->ctrl_handler); >> - v4l2_device_unregister(v4l2_dev); >> - >> dev_err(video->dev, "Failed to init controls: %d\n", rc); >> - return rc; >> + goto err_ctrl_init; >> } >> >> v4l2_dev->ctrl_handler = &video->ctrl_handler; >> @@ -1663,11 +1660,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >> >> rc = vb2_queue_init(vbq); >> if (rc) { >> - v4l2_ctrl_handler_free(&video->ctrl_handler); >> - v4l2_device_unregister(v4l2_dev); >> - >> dev_err(video->dev, "Failed to init vb2 queue\n"); >> - return rc; >> + goto err_vb2_init; >> } >> >> vdev->queue = vbq; >> @@ -1685,15 +1679,19 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >> video_set_drvdata(vdev, video); >> rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0); >> if (rc) { >> - vb2_queue_release(vbq); >> - v4l2_ctrl_handler_free(&video->ctrl_handler); >> - v4l2_device_unregister(v4l2_dev); >> - >> dev_err(video->dev, "Failed to register video device\n"); >> - return rc; >> + goto err_video_reg; >> } >> >> return 0; >> + >> +err_video_reg: >> + vb2_queue_release(vbq); >> +err_vb2_init: >> +err_ctrl_init: >> + v4l2_ctrl_handler_free(&video->ctrl_handler); >> + v4l2_device_unregister(v4l2_dev); >> + return rc; >> } >> >> static int aspeed_video_init(struct aspeed_video *video) > > > Thanks, > Mauro
Hi Sakari, Thanks for your review. On 2021/12/15 上午 02:32, Sakari Ailus wrote: > Hi Mauro, > > On Tue, Dec 14, 2021 at 03:53:00PM +0100, Mauro Carvalho Chehab wrote: >> Em Mon, 6 Dec 2021 08:48:11 +0800 >> Jammy Huang <jammy_huang@aspeedtech.com> escreveu: >> >>> refine aspeed_video_setup_video() flow. >> Why? It makes no difference where the error handling code is. Let's >> keep it as preferred by the driver's author ;-) > Doing error handling can be done in different ways of course, but I think > it's easier to see it's right as it's done by this patch. Of course the > original author's --- like anyone else's --- review wouldn't hurt. But I > see he hasn't reviewed other recent patches to the driver either. > > A better description would be nice though, including capital letter > beginning a sentence. > >> Regards, >> Mauro >> >>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com> >>> --- >>> v2: >>> - remove change-id in comment >>> --- >>> drivers/media/platform/aspeed-video.c | 24 +++++++++++------------- >>> 1 file changed, 11 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c >>> index fea5e4d0927e..f5c40d6b4ece 100644 >>> --- a/drivers/media/platform/aspeed-video.c >>> +++ b/drivers/media/platform/aspeed-video.c >>> @@ -1641,11 +1641,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >>> >>> rc = video->ctrl_handler.error; >>> if (rc) { >>> - v4l2_ctrl_handler_free(&video->ctrl_handler); >>> - v4l2_device_unregister(v4l2_dev); >>> - >>> dev_err(video->dev, "Failed to init controls: %d\n", rc); >>> - return rc; >>> + goto err_ctrl_init; >>> } >>> >>> v4l2_dev->ctrl_handler = &video->ctrl_handler; >>> @@ -1663,11 +1660,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >>> >>> rc = vb2_queue_init(vbq); >>> if (rc) { >>> - v4l2_ctrl_handler_free(&video->ctrl_handler); >>> - v4l2_device_unregister(v4l2_dev); >>> - >>> dev_err(video->dev, "Failed to init vb2 queue\n"); >>> - return rc; >>> + goto err_vb2_init; >>> } >>> >>> vdev->queue = vbq; >>> @@ -1685,15 +1679,19 @@ static int aspeed_video_setup_video(struct aspeed_video *video) >>> video_set_drvdata(vdev, video); >>> rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0); >>> if (rc) { >>> - vb2_queue_release(vbq); >>> - v4l2_ctrl_handler_free(&video->ctrl_handler); >>> - v4l2_device_unregister(v4l2_dev); >>> - >>> dev_err(video->dev, "Failed to register video device\n"); >>> - return rc; >>> + goto err_video_reg; >>> } >>> >>> return 0; >>> + >>> +err_video_reg: >>> + vb2_queue_release(vbq); >>> +err_vb2_init: >>> +err_ctrl_init: >>> + v4l2_ctrl_handler_free(&video->ctrl_handler); >>> + v4l2_device_unregister(v4l2_dev); >>> + return rc; >>> } >>> >>> static int aspeed_video_init(struct aspeed_video *video) >> >> >> Thanks, >> Mauro
diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c index fea5e4d0927e..f5c40d6b4ece 100644 --- a/drivers/media/platform/aspeed-video.c +++ b/drivers/media/platform/aspeed-video.c @@ -1641,11 +1641,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) rc = video->ctrl_handler.error; if (rc) { - v4l2_ctrl_handler_free(&video->ctrl_handler); - v4l2_device_unregister(v4l2_dev); - dev_err(video->dev, "Failed to init controls: %d\n", rc); - return rc; + goto err_ctrl_init; } v4l2_dev->ctrl_handler = &video->ctrl_handler; @@ -1663,11 +1660,8 @@ static int aspeed_video_setup_video(struct aspeed_video *video) rc = vb2_queue_init(vbq); if (rc) { - v4l2_ctrl_handler_free(&video->ctrl_handler); - v4l2_device_unregister(v4l2_dev); - dev_err(video->dev, "Failed to init vb2 queue\n"); - return rc; + goto err_vb2_init; } vdev->queue = vbq; @@ -1685,15 +1679,19 @@ static int aspeed_video_setup_video(struct aspeed_video *video) video_set_drvdata(vdev, video); rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0); if (rc) { - vb2_queue_release(vbq); - v4l2_ctrl_handler_free(&video->ctrl_handler); - v4l2_device_unregister(v4l2_dev); - dev_err(video->dev, "Failed to register video device\n"); - return rc; + goto err_video_reg; } return 0; + +err_video_reg: + vb2_queue_release(vbq); +err_vb2_init: +err_ctrl_init: + v4l2_ctrl_handler_free(&video->ctrl_handler); + v4l2_device_unregister(v4l2_dev); + return rc; } static int aspeed_video_init(struct aspeed_video *video)