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

[PATCH] 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.

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 manual=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>
---
 drivers/staging/comedi/drivers/comedi_test.c | 140 ++++++++++++++++++++++++---
 1 file changed, 128 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
index ec5b9a2..92b3a4f1 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 select manual configuration mode, pass "manual=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_char"
+
+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(manual, config_mode, bool, 0444);
+MODULE_PARM_DESC(manual, "Enable manual configuration: (1=enable [defaults to auto])");
+
+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,76 @@ 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;
+
+	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);
+
+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);
 
 MODULE_AUTHOR("Comedi http://www.comedi.org");
 MODULE_DESCRIPTION("Comedi low-level driver");
-- 
2.7.4

[PATCH] Staging: comedi: drivers: comedi_test: Set max input value for auto config

[Cheah Kok Cheong]

Currently user can input any value for amplitude and period.
This patch set a sane max value for auto-configuration mode.

For manual configuration mode, it is assumed this is taken care of
by the COMEDI userspace tool since there's no limit set here from
day one in the staging tree. If otherwise then maybe this can be
looked at separately.

Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
---
Note: This patch is dependent upon an earlier pending patch.
[Staging: comedi: drivers: comedi_test: Add auto-configuration
capability]
 
 drivers/staging/comedi/drivers/comedi_test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
index 92b3a4f1..4e18e2c 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -715,6 +715,14 @@ static int waveform_attach(struct comedi_device *dev,
 static int waveform_auto_attach(struct comedi_device *dev,
 				unsigned long context_unused)
 {
+	/* limit max input voltage to 300V [typical oscilloscope max value] */
+	if (set_amplitude > 300000000)
+		set_amplitude = 300000000;
+
+	/* limit max input period [= 1 hertz] to avoid too low freq */
+	if (set_period > 1000000)
+		set_period = 1000000;
+
 	int amplitude = set_amplitude;
 	int period = set_period;
 
-- 
2.7.4

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

[Cheah Kok Cheong]

On Thu, Jan 26, 2017 at 01:31:29AM +0800, 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.
> 
> 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 manual=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>
> ---
>  drivers/staging/comedi/drivers/comedi_test.c | 140 ++++++++++++++++++++++++---
>  1 file changed, 128 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
> index ec5b9a2..92b3a4f1 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 select manual configuration mode, pass "manual=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_char"
> +
> +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(manual, config_mode, bool, 0444);
> +MODULE_PARM_DESC(manual, "Enable manual configuration: (1=enable [defaults to auto])");
> +
> +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,76 @@ 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;
> +
> +	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);
> +
> +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);
>  
>  MODULE_AUTHOR("Comedi http://www.comedi.org");
>  MODULE_DESCRIPTION("Comedi low-level driver");
> -- 
> 2.7.4
> 

Hi Ian and Hartley,
 I'm wondering whether you received this patch and a follow on one.
If you did, sorry for the distraction.

Thanks.
Brgds,
CheahKC

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

[Ian Abbott]

On 25/01/17 17:31, 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.

This looks like a useful feature.  It avoids the need to reserve 
"legacy" devices in the core comedi module (using the 
"comedi_num_legacy_minors" module parameter), and configuring a reserved 
device "manually" with the comedi_config utiity.

> 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 manual=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>
> ---
>  drivers/staging/comedi/drivers/comedi_test.c | 140 ++++++++++++++++++++++++---
>  1 file changed, 128 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c
> index ec5b9a2..92b3a4f1 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 select manual configuration mode, pass "manual=1" parameter for module
> + * loading. Refer modinfo or MODULE_PARM_DESC description below for details.

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.

> + *
> + * 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_char"

I'm not sure about the class name "comedi_char".  Perhaps "comedi_test" 
would be better to associate it with this module.

> +
> +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(manual, config_mode, bool, 0444);
> +MODULE_PARM_DESC(manual, "Enable manual configuration: (1=enable [defaults to auto])");
> +
> +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,76 @@ 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;
> +
> +	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.

> +
> +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);
>
>  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] Staging: comedi: drivers: comedi_test: Set max input value for auto config

[Ian Abbott]

On 27/01/17 15:55, Cheah Kok Cheong wrote:
> Currently user can input any value for amplitude and period.
> This patch set a sane max value for auto-configuration mode.
>
> For manual configuration mode, it is assumed this is taken care of
> by the COMEDI userspace tool since there's no limit set here from
> day one in the staging tree. If otherwise then maybe this can be
> looked at separately.
>
> Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>

I don't think there is any need to limit these unless it results in 
arithmetic overflow, since they only affect the fake sample data values 
produced by the driver, not system performance.

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

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

Re: [PATCH] Staging: comedi: drivers: comedi_test: Set max input value for auto config

[Cheah Kok Cheong]

On Thu, Feb 09, 2017 at 12:28:42PM +0000, Ian Abbott wrote:
> On 27/01/17 15:55, Cheah Kok Cheong wrote:
> >Currently user can input any value for amplitude and period.
> >This patch set a sane max value for auto-configuration mode.
> >
> >For manual configuration mode, it is assumed this is taken care of
> >by the COMEDI userspace tool since there's no limit set here from
> >day one in the staging tree. If otherwise then maybe this can be
> >looked at separately.
> >
> >Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
> 
> I don't think there is any need to limit these unless it results in
> arithmetic overflow, since they only affect the fake sample data values
> produced by the driver, not system performance.

You are right there's no real danger here. Before submitting, I have
tested with positive values larger than "int" and smaller than "uint".
Anything larger than "uint" will result in loading failure.

I was motivated by the "user experience". Extreme values will not
display properly on Xoscope therefore I "googled" for a typical
oscilloscope input range for this patch.

Most probably I'm off the target here as I have only tried one
application. Maybe other supported application will handle this
better and offer a better user experience.

Again there's no real danger here, this patch is optional.

Thks.
Brgds,
CheahKC