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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---
 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@xxxxxxxxx> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux