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

[Ian Abbott]

On 15/02/17 06:05, Cheah Kok Cheong wrote:
> On Mon, Feb 13, 2017 at 11:14:14AM +0000, Ian Abbott wrote:
> > On 11/02/17 10:37, Cheah Kok Cheong wrote:
[snip]
> > > +static void __exit comedi_test_exit(void)
> > > +{
> > > +	comedi_auto_unconfig(ctdev);
> > > +
> > > +	device_destroy(ctcls, MKDEV(0, 0));
> > > +
> > > +	class_destroy(ctcls);
> >
> > If the driver init returned successfully, but failed to set-up the
> > auto-configured device, the device and class will not exist at this point,
> > so those three calls need to go in an 'if' statement.  Perhaps you could use
> > 'if (ctcls) {', and set 'ctcls = NULL;' after calling
> > 'class_destroy(ctcls);' in the init function.
> >
> > Apart from that, it looks fine.
>
> Thanks for pointing out those two pointers have to be "NULL" if
> auto-configuration fails.
>
> Please review below snippet.
> Sorry for the inconvenience caused.
>
> [ Snip ]
>
> static int __init comedi_test_init(void)
> {
> 	int ret;
>
> 	ret = comedi_driver_register(&waveform_driver);
> 	if (ret) {
> 		pr_err("comedi_test: unable to register driver\n");
> 		return ret;
> 	}
>
> 	if (!config_mode) {
> 		ctcls = class_create(THIS_MODULE, CLASS_NAME);
> 		if (IS_ERR(ctcls)) {
> 			pr_warn("comedi_test: unable to create class\n");
> 			goto clean3;
> 		}
>
> 		ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
> 		if (IS_ERR(ctdev)) {
> 			pr_warn("comedi_test: unable to create device\n");
> 			goto clean2;
> 		}
>
> 		ret = comedi_auto_config(ctdev, &waveform_driver, 0);
> 		if (ret) {
> 			pr_warn("comedi_test: unable to auto-configure device\n");
> 			goto clean;
> 		}
> 	}
>
> 	return 0;
>
> clean:
> 	device_destroy(ctcls, MKDEV(0, 0));
> clean2:
> 	class_destroy(ctcls);
> 	ctdev = NULL;
> clean3:
> 	ctcls = NULL;
>
> 	return 0;
> }
> module_init(comedi_test_init);
>
> static void __exit comedi_test_exit(void)
> {
> 	if (ctdev)
> 		comedi_auto_unconfig(ctdev);
>
> 	if (ctcls) {
> 		device_destroy(ctcls, MKDEV(0, 0));
> 		class_destroy(ctcls);
> 	}
>
> 	comedi_driver_unregister(&waveform_driver);
> }
> module_exit(comedi_test_exit);
>
> [ Snip ]
>
> Thks.
> Brgds,
> CheahKC

Yes, that looks fine.

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx> )=-
-=(                          Web: http://www.mev.co.uk/  )=-