Re: [PATCH] Staging: comedi: drivers: comedi_test: Add auto-configuration capability

[Cheah Kok Cheong]

Dear Ian,
  Thank you for taking the trouble to review this.

On Thu, Feb 09, 2017 at 12:25:15PM +0000, Ian Abbott wrote:
> 
> I think the "manual" parameter is misnamed, since this parameter controls
> whether a dummy hardware device is created by the driver or not.  Reserved
> COMEDI devices can still be configured manually to use this driver
> irrespective of the setting of the "manual" parameter.
> 
> Some possible options are to rename the parameter "noauto" (or "nocreate"),
> or to name it "auto" (or "create") with default value 1.
> 

Info noted. "noauto" will be better in this case. Like this perhaps.

module_param_named(noauto, config_mode, bool, 0444);
MODULE_PARM_DESC(noauto, "Disable auto-configuration: (1=disable [defaults to enable])");

> >+#define DEV_NAME "comedi_testd"
> >+#define CLASS_NAME "comedi_char"
> 
> I'm not sure about the class name "comedi_char".  Perhaps "comedi_test"
> would be better to associate it with this module.
> 

The best name actually is "comedi_test". I was discouraged by the many
instances of it and went for a unique one. [proc entry,printks,driver name etc]
But I'm no good at naming. I'll switch to "comedi_test".

> >+static int __init comedi_test_init(void)
> >+{
> >+	int ret;
> >+
> >+	if (!config_mode) {
> >+		ctcls = class_create(THIS_MODULE, CLASS_NAME);
> >+		if (IS_ERR(ctcls)) {
> >+			pr_err("comedi_test: unable to create class\n");
> >+			return PTR_ERR(ctcls);
> >+		}
> >+
> >+		ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
> >+		if (IS_ERR(ctdev)) {
> >+			pr_err("comedi_test: unable to create device\n");
> >+			ret = PTR_ERR(ctdev);
> >+			goto clean3;
> >+		}
> >+	}
> >+
> >+	ret = comedi_driver_register(&waveform_driver);
> >+	if (ret) {
> >+		pr_err("comedi_test: unable to register driver\n");
> >+		if (!config_mode)
> >+			goto clean2;
> >+		else
> >+			return ret;
> >+	}
> >+
> >+	if (!config_mode) {
> >+		ret = comedi_auto_config(ctdev, &waveform_driver, 0);
> >+		if (ret) {
> >+			pr_err("comedi_test: unable to auto configure device\n");
> >+			goto clean;
> >+		}
> >+	}
> >+
> >+	return 0;
> >+
> >+clean:
> >+	comedi_driver_unregister(&waveform_driver);
> >+clean2:
> >+	device_destroy(ctcls, MKDEV(0, 0));
> >+clean3:
> >+	class_destroy(ctcls);
> >+
> >+	return ret;
> >+}
> >+module_init(comedi_test_init);
> 
> I think the error handling paths look a bit untidy.  I think calling
> comedi_driver_register() first would help.  It would also be nice if failure
> to create the device class and test device did not prevent the module from
> initializing completely, since the module could still be used by manually
> configured COMEDI devices even if they fail.
> 

You are right. I'll put comedi_driver_register() first.
Again your idea is better, I'll convert it to continue loading even if
auto-configuration is unsuccessful.

> >+static void __exit comedi_test_exit(void)
> >+{
> >+	if (!config_mode)
> >+		comedi_auto_unconfig(ctdev);
> >+
> >+	comedi_driver_unregister(&waveform_driver);
> >+
> >+	if (!config_mode) {
> >+		device_destroy(ctcls, MKDEV(0, 0));
> >+		class_destroy(ctcls);
> >+	}
> >+}
> >+module_exit(comedi_test_exit);

On second thought, the if statement here seems to be redundant.
They seems to be able to handle non existent device and class.
I'll remove it. Further more we are going to let the module
to continue loading even if auto-configuration is unsuccessful.

Thks.
Brgds,
CheahKC