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

From: Cheah Kok Cheong
Date: Wed Feb 08 2017 - 10:42:58 EST


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@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.
> + *
> + * 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