diff mbox series

[v2,1/5] staging: vchiq: Refactor vchiq cdev code

Message ID e786f8c7f92561065b840490c32959932526dac6.1624185152.git.ojaswin98@gmail.com (mailing list archive)
State New, archived
Headers show
Series vchiq: Patch to separate platform and cdev code | expand

Commit Message

Ojaswin Mujoo June 20, 2021, 12:55 p.m. UTC
Move the vchiq cdev initialization code to its own function for better
code organization. Call the initialization function during probe, thus
shifting the whole cdev creation logic (which was earlier split in
vchiq_probe() and vchiq_driver_init()) to vchiq_probe().

Signed-off-by: Ojaswin Mujoo <ojaswin98@gmail.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 137 +++++++++++-------
 1 file changed, 87 insertions(+), 50 deletions(-)

Comments

Dan Carpenter June 21, 2021, 8:21 a.m. UTC | #1
On Sun, Jun 20, 2021 at 06:25:46PM +0530, Ojaswin Mujoo wrote:
>  	vchiq_debugfs_init();
>  
>  	vchiq_log_info(vchiq_arm_log_level,
> -		"vchiq: initialised - version %d (min %d), device %d.%d",
> -		VCHIQ_VERSION, VCHIQ_VERSION_MIN,
> -		MAJOR(vchiq_devid), MINOR(vchiq_devid));
> +		       "vchiq: platform initialised - version %d (min %d)",
> +		       VCHIQ_VERSION, VCHIQ_VERSION_MIN);
> +
> +	/*
> +	 * We don't handle error here since the function handles
> +	 * cleanup in cases of failure. Further, we can proceed
> +	 * even if this function fails.
> +	 */
> +	vchiq_register_chrdev(&pdev->dev);

I feel like ignoring errors and just continuing seems helpful, but it's
actually doing the users a disservice.  If it's an error during, boot
that's different, in that case it's better to get some kind of minimally
useful boot so the user can debug the problem.  But if the error isn't
going to prevent the system from booting then it's better to just return
an error so they can fix the problem and try again.

regards,
dan carpenter
Ojaswin Mujoo June 21, 2021, 2:22 p.m. UTC | #2
Hello Dan,

On Mon, Jun 21, 2021 at 11:21:32AM +0300, Dan Carpenter wrote:
> On Sun, Jun 20, 2021 at 06:25:46PM +0530, Ojaswin Mujoo wrote:
> >  	vchiq_debugfs_init();
> >  
> >  	vchiq_log_info(vchiq_arm_log_level,
> > -		"vchiq: initialised - version %d (min %d), device %d.%d",
> > -		VCHIQ_VERSION, VCHIQ_VERSION_MIN,
> > -		MAJOR(vchiq_devid), MINOR(vchiq_devid));
> > +		       "vchiq: platform initialised - version %d (min %d)",
> > +		       VCHIQ_VERSION, VCHIQ_VERSION_MIN);
> > +
> > +	/*
> > +	 * We don't handle error here since the function handles
> > +	 * cleanup in cases of failure. Further, we can proceed
> > +	 * even if this function fails.
> > +	 */
> > +	vchiq_register_chrdev(&pdev->dev);
> 
> I feel like ignoring errors and just continuing seems helpful, but it's
> actually doing the users a disservice.  If it's an error during, boot
> that's different, in that case it's better to get some kind of minimally
> useful boot so the user can debug the problem.  But if the error isn't
> going to prevent the system from booting then it's better to just return
> an error so they can fix the problem and try again.
Got it, I'll fix this in the next revision.

Thank you,
Ojaswin

> 
> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index a936102dbc34..f2d8116913f2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2207,6 +2207,81 @@  vchiq_fops = {
 	.read = vchiq_read
 };
 
+/**
+ *	vchiq_register_chrdev - Register the char driver for vchiq
+ *				and create the necessary class and
+ *				device files in userspace.
+ *	@parent		The parent of the char device.
+ *
+ *	Returns 0 on success else returns the error code.
+ */
+static int vchiq_register_chrdev(struct device *parent)
+{
+	struct device *vchiq_dev;
+	int ret;
+
+	vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
+	if (IS_ERR(vchiq_class)) {
+		pr_err("Failed to create vchiq class\n");
+		ret = PTR_ERR(vchiq_class);
+		goto error_exit;
+	}
+
+	ret = alloc_chrdev_region(&vchiq_devid, 0, 1, DEVICE_NAME);
+	if (ret) {
+		pr_err("vchiq: Failed to allocate vchiq's chrdev region\n");
+		goto alloc_region_error;
+	}
+
+	cdev_init(&vchiq_cdev, &vchiq_fops);
+	vchiq_cdev.owner = THIS_MODULE;
+	ret = cdev_add(&vchiq_cdev, vchiq_devid, 1);
+	if (ret) {
+		vchiq_log_error(vchiq_arm_log_level,
+				"Unable to register vchiq char device");
+		goto cdev_add_error;
+	}
+
+	vchiq_dev = device_create(vchiq_class, parent, vchiq_devid, NULL,
+				  DEVICE_NAME);
+	if (IS_ERR(vchiq_dev)) {
+		vchiq_log_error(vchiq_arm_log_level,
+				"Failed to create vchiq char device node");
+		ret = PTR_ERR(vchiq_dev);
+		goto device_create_error;
+	}
+
+	vchiq_log_info(vchiq_arm_log_level,
+		       "vchiq char dev initialised successfully - device %d.%d",
+			MAJOR(vchiq_devid), MINOR(vchiq_devid));
+
+	return 0;
+
+device_create_error:
+	cdev_del(&vchiq_cdev);
+
+cdev_add_error:
+	unregister_chrdev_region(vchiq_devid, 1);
+
+alloc_region_error:
+	class_destroy(vchiq_class);
+
+error_exit:
+	return ret;
+}
+
+/**
+ *	vchiq_deregister_chrdev	- Deregister and cleanup the vchiq char
+ *				  driver and device files
+ */
+static void vchiq_deregister_chrdev(void)
+{
+	device_destroy(vchiq_class, vchiq_devid);
+	cdev_del(&vchiq_cdev);
+	unregister_chrdev_region(vchiq_devid, 1);
+	class_destroy(vchiq_class);
+}
+
 /*
  * Autosuspend related functionality
  */
@@ -2740,7 +2815,6 @@  static int vchiq_probe(struct platform_device *pdev)
 	struct device_node *fw_node;
 	const struct of_device_id *of_id;
 	struct vchiq_drvdata *drvdata;
-	struct device *vchiq_dev;
 	int err;
 
 	of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
@@ -2766,28 +2840,18 @@  static int vchiq_probe(struct platform_device *pdev)
 	if (err)
 		goto failed_platform_init;
 
-	cdev_init(&vchiq_cdev, &vchiq_fops);
-	vchiq_cdev.owner = THIS_MODULE;
-	err = cdev_add(&vchiq_cdev, vchiq_devid, 1);
-	if (err) {
-		vchiq_log_error(vchiq_arm_log_level,
-			"Unable to register device");
-		goto failed_platform_init;
-	}
-
-	vchiq_dev = device_create(vchiq_class, &pdev->dev, vchiq_devid, NULL,
-				  "vchiq");
-	if (IS_ERR(vchiq_dev)) {
-		err = PTR_ERR(vchiq_dev);
-		goto failed_device_create;
-	}
-
 	vchiq_debugfs_init();
 
 	vchiq_log_info(vchiq_arm_log_level,
-		"vchiq: initialised - version %d (min %d), device %d.%d",
-		VCHIQ_VERSION, VCHIQ_VERSION_MIN,
-		MAJOR(vchiq_devid), MINOR(vchiq_devid));
+		       "vchiq: platform initialised - version %d (min %d)",
+		       VCHIQ_VERSION, VCHIQ_VERSION_MIN);
+
+	/*
+	 * We don't handle error here since the function handles
+	 * cleanup in cases of failure. Further, we can proceed
+	 * even if this function fails.
+	 */
+	vchiq_register_chrdev(&pdev->dev);
 
 	vcsm_cma = vchiq_register_child(pdev, "vcsm-cma");
 	bcm2835_codec = vchiq_register_child(pdev, "bcm2835-codec");
@@ -2797,10 +2861,8 @@  static int vchiq_probe(struct platform_device *pdev)
 
 	return 0;
 
-failed_device_create:
-	cdev_del(&vchiq_cdev);
 failed_platform_init:
-	vchiq_log_warning(vchiq_arm_log_level, "could not load vchiq");
+	vchiq_log_warning(vchiq_arm_log_level, "could not initialize vchiq platform");
 	return err;
 }
 
@@ -2812,8 +2874,7 @@  static int vchiq_remove(struct platform_device *pdev)
 	platform_device_unregister(bcm2835_codec);
 	platform_device_unregister(vcsm_cma);
 	vchiq_debugfs_deinit();
-	device_destroy(vchiq_class, vchiq_devid);
-	cdev_del(&vchiq_cdev);
+	vchiq_deregister_chrdev();
 
 	return 0;
 }
@@ -2831,31 +2892,9 @@  static int __init vchiq_driver_init(void)
 {
 	int ret;
 
-	vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
-	if (IS_ERR(vchiq_class)) {
-		pr_err("Failed to create vchiq class\n");
-		return PTR_ERR(vchiq_class);
-	}
-
-	ret = alloc_chrdev_region(&vchiq_devid, 0, 1, DEVICE_NAME);
-	if (ret) {
-		pr_err("Failed to allocate vchiq's chrdev region\n");
-		goto class_destroy;
-	}
-
 	ret = platform_driver_register(&vchiq_driver);
-	if (ret) {
+	if (ret)
 		pr_err("Failed to register vchiq driver\n");
-		goto region_unregister;
-	}
-
-	return 0;
-
-region_unregister:
-	unregister_chrdev_region(vchiq_devid, 1);
-
-class_destroy:
-	class_destroy(vchiq_class);
 
 	return ret;
 }
@@ -2864,8 +2903,6 @@  module_init(vchiq_driver_init);
 static void __exit vchiq_driver_exit(void)
 {
 	platform_driver_unregister(&vchiq_driver);
-	unregister_chrdev_region(vchiq_devid, 1);
-	class_destroy(vchiq_class);
 }
 module_exit(vchiq_driver_exit);