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

[Cheah Kok Cheong <thrust73@gmail.com>]

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