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

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

[Cheah Kok Cheong]

Currently this module needs to be manually configured by COMEDI
userspace tool before the test waveform can be read by a COMEDI
compatible application.

This patch adds auto-configuration capability and makes it the default
loading option. This is achieved by creating a device during init
to stand in for a real hardware device. This allows comedi_auto_config()
to perform auto-configuration. With this patch, the test waveform can
be read by a COMEDI compatible application without needing manual
configuration.

Previous behaviour is still selectable via module loading parameter.
Module loading without passing any parameter will default to
auto-configuration with the same default waveform amplitude and
period values. For auto-configuration, different amplitude and
period values can be set via module loading parameters.

Tested on Xubuntu 16.04 using Xoscope ver: 2.0 which is available
in the Ubuntu repository. Xoscope is a COMEDI compatible digital
oscilloscope application. For manual configuration, only module
loading/unloading is tested.

Here are the truncated dmesg output.
[sudo modprobe comedi_test]

comedi_test: 1000000 microvolt, 100000 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=2500000 period=150000]

comedi_test: 2500000 microvolt, 150000 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test noauto=1]

comedi_test: module is from the staging directory, the quality is unknown,
you have been warned.

For those without an actual hardware, the comedi_test module
is as close as one can get to test the COMEDI system.
Having both auto and manual configuration capability will broaden
the test function of this module.
Hopefully this will make it easier for people to check out the
COMEDI system and contribute to its development.

Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
---

V2:
-Rename module param - Ian
-Rename class - Ian
-Tidy up init error handling - Ian
-Allow module loading to continue when auto-configuration fails - Ian
-Remove redundant "if" statement from module exit
-Edit driver intro to reflect changes

 drivers/staging/comedi/drivers/comedi_test.c | 130 ++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
index ec5b9a2..027c972 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -36,7 +36,15 @@
  * generate sample waveforms on systems that don't have data acquisition
  * hardware.
  *
- * Configuration options:
+ * Auto-configuration is the default mode if no parameter is supplied during
+ * module loading. Manual configuration requires COMEDI userspace tool.
+ * To disable auto-configuration mode, pass "noauto=1" parameter for module
+ * loading. Refer modinfo or MODULE_PARM_DESC description below for details.
+ *
+ * Auto-configuration options:
+ *   Refer modinfo or MODULE_PARM_DESC description below for details.
+ *
+ * Manual configuration options:
  *   [0] - Amplitude in microvolts for fake waveforms (default 1 volt)
  *   [1] - Period in microseconds for fake waveforms (default 0.1 sec)
  *
@@ -53,8 +61,27 @@
 #include <linux/timer.h>
 #include <linux/ktime.h>
 #include <linux/jiffies.h>
+#include <linux/device.h>
+#include <linux/kdev_t.h>
 
 #define N_CHANS 8
+#define DEV_NAME "comedi_testd"
+#define CLASS_NAME "comedi_test"
+
+static bool config_mode;
+static unsigned int set_amplitude;
+static unsigned int set_period;
+static struct class *ctcls;
+static struct device *ctdev;
+
+module_param_named(noauto, config_mode, bool, 0444);
+MODULE_PARM_DESC(noauto, "Disable auto-configuration: (1=disable [defaults to enable])");
+
+module_param_named(amplitude, set_amplitude, uint, 0444);
+MODULE_PARM_DESC(amplitude, "Set auto mode wave amplitude in microvolts: (defaults to 1 volt)");
+
+module_param_named(period, set_period, uint, 0444);
+MODULE_PARM_DESC(period, "Set auto mode wave period in microseconds: (defaults to 0.1 sec)");
 
 /* Data unique to this driver */
 struct waveform_private {
@@ -607,13 +634,11 @@ static int waveform_ao_insn_write(struct comedi_device *dev,
 	return insn->n;
 }
 
-static int waveform_attach(struct comedi_device *dev,
-			   struct comedi_devconfig *it)
+static int waveform_common_attach(struct comedi_device *dev,
+				  int amplitude, int period)
 {
 	struct waveform_private *devpriv;
 	struct comedi_subdevice *s;
-	int amplitude = it->options[0];
-	int period = it->options[1];
 	int i;
 	int ret;
 
@@ -621,12 +646,6 @@ static int waveform_attach(struct comedi_device *dev,
 	if (!devpriv)
 		return -ENOMEM;
 
-	/* set default amplitude and period */
-	if (amplitude <= 0)
-		amplitude = 1000000;	/* 1 volt */
-	if (period <= 0)
-		period = 100000;	/* 0.1 sec */
-
 	devpriv->wf_amplitude = amplitude;
 	devpriv->wf_period = period;
 
@@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
 	return 0;
 }
 
+static int waveform_attach(struct comedi_device *dev,
+			   struct comedi_devconfig *it)
+{
+	int amplitude = it->options[0];
+	int period = it->options[1];
+
+	/* set default amplitude and period */
+	if (amplitude <= 0)
+		amplitude = 1000000;	/* 1 volt */
+	if (period <= 0)
+		period = 100000;	/* 0.1 sec */
+
+	return waveform_common_attach(dev, amplitude, period);
+}
+
+static int waveform_auto_attach(struct comedi_device *dev,
+				unsigned long context_unused)
+{
+	int amplitude = set_amplitude;
+	int period = set_period;
+
+	/* set default amplitude and period */
+	if (!amplitude)
+		amplitude = 1000000;	/* 1 volt */
+	if (!period)
+		period = 100000;	/* 0.1 sec */
+
+	return waveform_common_attach(dev, amplitude, period);
+}
+
 static void waveform_detach(struct comedi_device *dev)
 {
 	struct waveform_private *devpriv = dev->private;
@@ -692,9 +741,66 @@ static struct comedi_driver waveform_driver = {
 	.driver_name	= "comedi_test",
 	.module		= THIS_MODULE,
 	.attach		= waveform_attach,
+	.auto_attach	= waveform_auto_attach,
 	.detach		= waveform_detach,
 };
-module_comedi_driver(waveform_driver);
+
+/*
+ * For auto-configuration, a device is created to stand in for a
+ * real hardware device.
+ */
+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);
+
+	comedi_driver_unregister(&waveform_driver);
+}
+module_exit(comedi_test_exit);
 
 MODULE_AUTHOR("Comedi http://www.comedi.org");
 MODULE_DESCRIPTION("Comedi low-level driver");
-- 
2.7.4

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

[Ian Abbott]

On 11/02/17 10:37, Cheah Kok Cheong wrote:
> Currently this module needs to be manually configured by COMEDI
> userspace tool before the test waveform can be read by a COMEDI
> compatible application.
>
> This patch adds auto-configuration capability and makes it the default
> loading option. This is achieved by creating a device during init
> to stand in for a real hardware device. This allows comedi_auto_config()
> to perform auto-configuration. With this patch, the test waveform can
> be read by a COMEDI compatible application without needing manual
> configuration.
>
> Previous behaviour is still selectable via module loading parameter.
> Module loading without passing any parameter will default to
> auto-configuration with the same default waveform amplitude and
> period values. For auto-configuration, different amplitude and
> period values can be set via module loading parameters.
>
> Tested on Xubuntu 16.04 using Xoscope ver: 2.0 which is available
> in the Ubuntu repository. Xoscope is a COMEDI compatible digital
> oscilloscope application. For manual configuration, only module
> loading/unloading is tested.
>
> Here are the truncated dmesg output.
> [sudo modprobe comedi_test]
>
> comedi_test: 1000000 microvolt, 100000 microsecond waveform attached
> driver 'comedi_test' has successfully auto-configured 'comedi_test'.
>
> [sudo modprobe comedi_test amplitude=2500000 period=150000]
>
> comedi_test: 2500000 microvolt, 150000 microsecond waveform attached
> driver 'comedi_test' has successfully auto-configured 'comedi_test'.
>
> [sudo modprobe comedi_test noauto=1]
>
> comedi_test: module is from the staging directory, the quality is unknown,
> you have been warned.
>
> For those without an actual hardware, the comedi_test module
> is as close as one can get to test the COMEDI system.
> Having both auto and manual configuration capability will broaden
> the test function of this module.
> Hopefully this will make it easier for people to check out the
> COMEDI system and contribute to its development.
>
> Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
> ---
>
> V2:
> -Rename module param - Ian
> -Rename class - Ian
> -Tidy up init error handling - Ian
> -Allow module loading to continue when auto-configuration fails - Ian
> -Remove redundant "if" statement from module exit
> -Edit driver intro to reflect changes
>
>  drivers/staging/comedi/drivers/comedi_test.c | 130 ++++++++++++++++++++++++---
>  1 file changed, 118 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
> index ec5b9a2..027c972 100644
> --- a/drivers/staging/comedi/drivers/comedi_test.c
> +++ b/drivers/staging/comedi/drivers/comedi_test.c
> @@ -36,7 +36,15 @@
>   * generate sample waveforms on systems that don't have data acquisition
>   * hardware.
>   *
> - * Configuration options:
> + * Auto-configuration is the default mode if no parameter is supplied during
> + * module loading. Manual configuration requires COMEDI userspace tool.
> + * To disable auto-configuration mode, pass "noauto=1" parameter for module
> + * loading. Refer modinfo or MODULE_PARM_DESC description below for details.
> + *
> + * Auto-configuration options:
> + *   Refer modinfo or MODULE_PARM_DESC description below for details.
> + *
> + * Manual configuration options:
>   *   [0] - Amplitude in microvolts for fake waveforms (default 1 volt)
>   *   [1] - Period in microseconds for fake waveforms (default 0.1 sec)
>   *
> @@ -53,8 +61,27 @@
>  #include <linux/timer.h>
>  #include <linux/ktime.h>
>  #include <linux/jiffies.h>
> +#include <linux/device.h>
> +#include <linux/kdev_t.h>
>
>  #define N_CHANS 8
> +#define DEV_NAME "comedi_testd"
> +#define CLASS_NAME "comedi_test"
> +
> +static bool config_mode;
> +static unsigned int set_amplitude;
> +static unsigned int set_period;
> +static struct class *ctcls;
> +static struct device *ctdev;
> +
> +module_param_named(noauto, config_mode, bool, 0444);
> +MODULE_PARM_DESC(noauto, "Disable auto-configuration: (1=disable [defaults to enable])");
> +
> +module_param_named(amplitude, set_amplitude, uint, 0444);
> +MODULE_PARM_DESC(amplitude, "Set auto mode wave amplitude in microvolts: (defaults to 1 volt)");
> +
> +module_param_named(period, set_period, uint, 0444);
> +MODULE_PARM_DESC(period, "Set auto mode wave period in microseconds: (defaults to 0.1 sec)");
>
>  /* Data unique to this driver */
>  struct waveform_private {
> @@ -607,13 +634,11 @@ static int waveform_ao_insn_write(struct comedi_device *dev,
>  	return insn->n;
>  }
>
> -static int waveform_attach(struct comedi_device *dev,
> -			   struct comedi_devconfig *it)
> +static int waveform_common_attach(struct comedi_device *dev,
> +				  int amplitude, int period)
>  {
>  	struct waveform_private *devpriv;
>  	struct comedi_subdevice *s;
> -	int amplitude = it->options[0];
> -	int period = it->options[1];
>  	int i;
>  	int ret;
>
> @@ -621,12 +646,6 @@ static int waveform_attach(struct comedi_device *dev,
>  	if (!devpriv)
>  		return -ENOMEM;
>
> -	/* set default amplitude and period */
> -	if (amplitude <= 0)
> -		amplitude = 1000000;	/* 1 volt */
> -	if (period <= 0)
> -		period = 100000;	/* 0.1 sec */
> -
>  	devpriv->wf_amplitude = amplitude;
>  	devpriv->wf_period = period;
>
> @@ -678,6 +697,36 @@ static int waveform_attach(struct comedi_device *dev,
>  	return 0;
>  }
>
> +static int waveform_attach(struct comedi_device *dev,
> +			   struct comedi_devconfig *it)
> +{
> +	int amplitude = it->options[0];
> +	int period = it->options[1];
> +
> +	/* set default amplitude and period */
> +	if (amplitude <= 0)
> +		amplitude = 1000000;	/* 1 volt */
> +	if (period <= 0)
> +		period = 100000;	/* 0.1 sec */
> +
> +	return waveform_common_attach(dev, amplitude, period);
> +}
> +
> +static int waveform_auto_attach(struct comedi_device *dev,
> +				unsigned long context_unused)
> +{
> +	int amplitude = set_amplitude;
> +	int period = set_period;
> +
> +	/* set default amplitude and period */
> +	if (!amplitude)
> +		amplitude = 1000000;	/* 1 volt */
> +	if (!period)
> +		period = 100000;	/* 0.1 sec */
> +
> +	return waveform_common_attach(dev, amplitude, period);
> +}
> +
>  static void waveform_detach(struct comedi_device *dev)
>  {
>  	struct waveform_private *devpriv = dev->private;
> @@ -692,9 +741,66 @@ static struct comedi_driver waveform_driver = {
>  	.driver_name	= "comedi_test",
>  	.module		= THIS_MODULE,
>  	.attach		= waveform_attach,
> +	.auto_attach	= waveform_auto_attach,
>  	.detach		= waveform_detach,
>  };
> -module_comedi_driver(waveform_driver);
> +
> +/*
> + * For auto-configuration, a device is created to stand in for a
> + * real hardware device.
> + */
> +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.

> +
> +	comedi_driver_unregister(&waveform_driver);
> +}
> +module_exit(comedi_test_exit);
>
>  MODULE_AUTHOR("Comedi http://www.comedi.org");
>  MODULE_DESCRIPTION("Comedi low-level driver");
>


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

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

[Cheah Kok Cheong]

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

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@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

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

[Cheah Kok Cheong]

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