Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581339 > unrolled thread

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

Started byCheah Kok Cheong <thrust73@gmail.com>
First post2017-02-15 15:10 +0100
Last post2017-02-16 15:10 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add  auto-configuration capability Cheah Kok Cheong <thrust73@gmail.com> - 2017-02-15 15:10 +0100
    Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add  auto-configuration capability Ian Abbott <abbotti@mev.co.uk> - 2017-02-16 11:20 +0100
      Re: [PATCH v2] Staging: comedi: drivers: comedi_test: Add  auto-configuration capability Cheah Kok Cheong <thrust73@gmail.com> - 2017-02-16 15:10 +0100

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

FromCheah Kok Cheong <thrust73@gmail.com>
Date2017-02-15 15:10 +0100
SubjectRe: [PATCH v2] Staging: comedi: drivers: comedi_test: Add auto-configuration capability
Message-ID<tb8rp-107-47@gated-at.bofh.it>
On Mon, Feb 13, 2017 at 11:14:14AM +0000, Ian Abbott wrote:
> On 11/02/17 10:37, Cheah Kok Cheong wrote:
> >+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");
> >+			return ret;
> >+		}
> >+
> >+		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);
> >+
> >+	return 0;
> >+}
> >+module_init(comedi_test_init);
> >+
> >+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

[toc] | [next] | [standalone]

#1582436

FromIan Abbott <abbotti@mev.co.uk>
Date2017-02-16 11:20 +0100
Message-ID<tbrkm-4KJ-11@gated-at.bofh.it>
In reply to#1581339
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@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

[toc] | [prev] | [next] | [standalone]

#1582602

FromCheah Kok Cheong <thrust73@gmail.com>
Date2017-02-16 15:10 +0100
Message-ID<tbuUW-7dp-17@gated-at.bofh.it>
In reply to#1582436
On Thu, Feb 16, 2017 at 10:10:34AM +0000, Ian Abbott wrote:
> 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.
> 

Thanks. I'll send V3 shortly.

Brgds,
CheahKC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web