Message-ID: <a52ea0e6-ab56-2243-78d8-d21d5fb8cd48@mev.co.uk>
Date:   Thu, 9 Feb 2017 12:25:15 +0000
From:   Ian Abbott <abbotti@....co.uk>
To:     Cheah Kok Cheong <thrust73@...il.com>,
        hsweeten@...ionengravers.com, gregkh@...uxfoundation.org
Cc:     devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: comedi: drivers: comedi_test: Add
 auto-configuration capability

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

Powered by blists - more mailing lists