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

[Cheah Kok Cheong]

On Mon, Feb 13, 2017 at 11:14:14AM +, Ian Abbott wrote:
> On 11/02/17 10:37, Cheah Kok Cheong wrote:
> >+static int __init comedi_test_init(void)
> >+{
> >+int ret;
> >+
> >+ret = comedi_driver_register(&waveform_driver);
> >+if (ret) {
> >+pr_err("comedi_test: unable to register driver\n");
> >+return ret;
> >+}
> >+
> >+if (!config_mode) {
> >+ctcls = class_create(THIS_MODULE, CLASS_NAME);
> >+if (IS_ERR(ctcls)) {
> >+pr_warn("comedi_test: unable to create class\n");
> >+return ret;
> >+}
> >+
> >+ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
> >+if (IS_ERR(ctdev)) {
> >+pr_warn("comedi_test: unable to create device\n");
> >+goto clean2;
> >+}
> >+
> >+ret = comedi_auto_config(ctdev, &waveform_driver, 0);
> >+if (ret) {
> >+pr_warn("comedi_test: unable to auto-configure 
> >device\n");
> >+goto clean;
> >+}
> >+}
> >+
> >+return 0;
> >+
> >+clean:
> >+device_destroy(ctcls, MKDEV(0, 0));
> >+clean2:
> >+class_destroy(ctcls);
> >+
> >+return 0;
> >+}
> >+module_init(comedi_test_init);
> >+
> >+static void __exit comedi_test_exit(void)
> >+{
> >+comedi_auto_unconfig(ctdev);
> >+
> >+device_destroy(ctcls, MKDEV(0, 0));
> >+
> >+class_destroy(ctcls);
> 
> If the driver init returned successfully, but failed to set-up the
> auto-configured device, the device and class will not exist at this point,
> so those three calls need to go in an 'if' statement.  Perhaps you could use
> 'if (ctcls) {', and set 'ctcls = NULL;' after calling
> 'class_destroy(ctcls);' in the init function.
> 
> Apart from that, it looks fine.
> 

Thanks for pointing out those two pointers have to be "NULL" if
auto-configuration fails.

Please review below snippet.
Sorry for the inconvenience caused.

[ Snip ]

static int __init comedi_test_init(void)
{
int ret;

ret = comedi_driver_register(&waveform_driver);
if (ret) {
pr_err("comedi_test: unable to register driver\n");
return ret;
}

if (!config_mode) {
ctcls = class_create(THIS_MODULE, CLASS_NAME);
if (IS_ERR(ctcls)) {
pr_warn("comedi_test: unable to create class\n");
goto clean3;
}

ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
if (IS_ERR(ctdev)) {
pr_warn("comedi_test: unable to create device\n");
goto clean2;
}

ret = comedi_auto_config(ctdev, &waveform_driver, 0);
if (ret) {
pr_warn("comedi_test: unable to auto-configure 
device\n");
goto clean;
}
}

return 0;

clean:
device_destroy(ctcls, MKDEV(0, 0));
clean2:
class_destroy(ctcls);
ctdev = NULL;
clean3:
ctcls = NULL;

return 0;
}
module_init(comedi_test_init);

static void __exit comedi_test_exit(void)
{
if (ctdev)
comedi_auto_unconfig(ctdev);

if (ctcls) {
device_destroy(ctcls, MKDEV(0, 0));
class_destroy(ctcls);
}

comedi_driver_unregister(&waveform_driver);
}
module_exit(comedi_test_exit);

[ Snip ]

Thks.
Brgds,
CheahKC
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Cheah Kok Cheong]

On Thu, Feb 16, 2017 at 10:10:34AM +, Ian Abbott wrote:
> On 15/02/17 06:05, Cheah Kok Cheong wrote:
> >On Mon, Feb 13, 2017 at 11:14:14AM +, Ian Abbott wrote:
> >>On 11/02/17 10:37, Cheah Kok Cheong wrote:
> [snip]
> >>>+static void __exit comedi_test_exit(void)
> >>>+{
> >>>+  comedi_auto_unconfig(ctdev);
> >>>+
> >>>+  device_destroy(ctcls, MKDEV(0, 0));
> >>>+
> >>>+  class_destroy(ctcls);
> >>
> >>If the driver init returned successfully, but failed to set-up the
> >>auto-configured device, the device and class will not exist at this point,
> >>so those three calls need to go in an 'if' statement.  Perhaps you could use
> >>'if (ctcls) {', and set 'ctcls = NULL;' after calling
> >>'class_destroy(ctcls);' in the init function.
> >>
> >>Apart from that, it looks fine.
> >>
> >
> >Thanks for pointing out those two pointers have to be "NULL" if
> >auto-configuration fails.
> >
> >Please review below snippet.
> >Sorry for the inconvenience caused.
> >
> >[ Snip ]
> >
> >static int __init comedi_test_init(void)
> >{
> > int ret;
> >
> > ret = comedi_driver_register(&waveform_driver);
> > if (ret) {
> > pr_err("comedi_test: unable to register driver\n");
> > return ret;
> > }
> >
> > if (!config_mode) {
> > ctcls = class_create(THIS_MODULE, CLASS_NAME);
> > if (IS_ERR(ctcls)) {
> > pr_warn("comedi_test: unable to create class\n");
> > goto clean3;
> > }
> >
> > ctdev = device_create(ctcls, NULL, MKDEV(0, 0), NULL, DEV_NAME);
> > if (IS_ERR(ctdev)) {
> > pr_warn("comedi_test: unable to create device\n");
> > goto clean2;
> > }
> >
> > ret = comedi_auto_config(ctdev, &waveform_driver, 0);
> > if (ret) {
> > pr_warn("comedi_test: unable to auto-configure 
> > device\n");
> > goto clean;
> > }
> > }
> >
> > return 0;
> >
> >clean:
> > device_destroy(ctcls, MKDEV(0, 0));
> >clean2:
> > class_destroy(ctcls);
> > ctdev = NULL;
> >clean3:
> > ctcls = NULL;
> >
> > return 0;
> >}
> >module_init(comedi_test_init);
> >
> >static void __exit comedi_test_exit(void)
> >{
> > if (ctdev)
> > comedi_auto_unconfig(ctdev);
> >
> > if (ctcls) {
> > device_destroy(ctcls, MKDEV(0, 0));
> > class_destroy(ctcls);
> > }
> >
> > comedi_driver_unregister(&waveform_driver);
> >}
> >module_exit(comedi_test_exit);
> >
> >[ Snip ]
> >
> >Thks.
> >Brgds,
> >CheahKC
> >
> 
> Yes, that looks fine.
> 

Thanks. I'll send V3 shortly.

Brgds,
CheahKC
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

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: 100 microvolt, 10 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=250 period=15]

comedi_test: 250 microvolt, 15 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test noauto=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 
---

V3:
-Ensure struct class and struct device pointers are "NULL"
 if auto-configuration fails - Ian

V2:
-Rename module param - Ian
-Rename class - Ian
-Tidy up init error handling - Ian
-Allow module loading to continue when auto-configuration fails - Ian
-Remove redundant "if" statement from module exit
-Edit driver intro to reflect changes

 drivers/staging/comedi/drivers/comedi_test.c | 135 ---
 1 file changed, 123 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index ec5b9a2..2a063f0 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 disable auto-configuration mode, pass "noauto=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 
 #include 
 #include 
+#include 
+#include 
 
 #define N_CHANS 8
+#define DEV_NAME "comedi_testd"
+#define CLASS_NAME "comedi_test"
+
+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(noauto, config_mode, bool, 0444);
+MODULE_PARM_DESC(noauto, "Disable auto-configuration: (1=disable [defaults to 
enable])");
+
+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 *

[PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Cheah Kok Cheong]

Fix checkpatch warning "Avoid multiple line dereference"
using a local variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/drivers/comedi_test.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..fde83e0 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short d = devpriv->ao_loopbacks[chan];
 
-   if (comedi_buf_read_samples(s,
-   &devpriv->
-ao_loopbacks[chan],
-   1) == 0) {
+   if (!comedi_buf_read_samples(s, &d, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Cheah Kok Cheong]

On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:
> On 20/02/17 08:28, Cheah Kok Cheong wrote:
> >Fix checkpatch warning "Avoid multiple line dereference"
> >using a local variable to avoid line wrap.
> >
> >Signed-off-by: Cheah Kok Cheong 
> >---
> > drivers/staging/comedi/drivers/comedi_test.c | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
> >b/drivers/staging/comedi/drivers/comedi_test.c
> >index 2a063f0..fde83e0 100644
> >--- a/drivers/staging/comedi/drivers/comedi_test.c
> >+++ b/drivers/staging/comedi/drivers/comedi_test.c
> >@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
> > /* output the last scan */
> > for (i = 0; i < cmd->scan_end_arg; i++) {
> > unsigned int chan = CR_CHAN(cmd->chanlist[i]);
> >+unsigned short d = devpriv->ao_loopbacks[chan];
> >
> >-if (comedi_buf_read_samples(s,
> >-&devpriv->
> >- ao_loopbacks[chan],
> >-1) == 0) {
> >+if (!comedi_buf_read_samples(s, &d, 1)) {
> > /* unexpected underrun! (cancelled?) */
> > async->events |= COMEDI_CB_OVERFLOW;
> > goto underrun;
> >
> 
> NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.
> 

Thanks for pointing this out. In that case will assigning the variable to
devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.

Otherwise I'll just drop the variable and adjust the lines to avoid
checkpatch warning.

Sorry for the inconvenience caused.

[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short data;

if (!comedi_buf_read_samples(s, &data, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}

devpriv->ao_loopbacks[chan] = data;
}
/* advance time of last scan */

[ Snip ]

Thks.
Brgds,
CheahKC
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Cheah Kok Cheong]

On Mon, Feb 20, 2017 at 05:36:52PM +, Ian Abbott wrote:
> On 20/02/17 16:02, Cheah Kok Cheong wrote:
> >On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:
> >>On 20/02/17 08:28, Cheah Kok Cheong wrote:
> >>>Fix checkpatch warning "Avoid multiple line dereference"
> >>>using a local variable to avoid line wrap.
> >>>
> >>>Signed-off-by: Cheah Kok Cheong 
> >>>---
> >>>drivers/staging/comedi/drivers/comedi_test.c | 6 ++
> >>>1 file changed, 2 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
> >>>b/drivers/staging/comedi/drivers/comedi_test.c
> >>>index 2a063f0..fde83e0 100644
> >>>--- a/drivers/staging/comedi/drivers/comedi_test.c
> >>>+++ b/drivers/staging/comedi/drivers/comedi_test.c
> >>>@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg)
> >>>   /* output the last scan */
> >>>   for (i = 0; i < cmd->scan_end_arg; i++) {
> >>>   unsigned int chan = CR_CHAN(cmd->chanlist[i]);
> >>>+  unsigned short d = devpriv->ao_loopbacks[chan];
> >>>
> >>>-  if (comedi_buf_read_samples(s,
> >>>-  &devpriv->
> >>>-   ao_loopbacks[chan],
> >>>-  1) == 0) {
> >>>+  if (!comedi_buf_read_samples(s, &d, 1)) {
> >>>   /* unexpected underrun! (cancelled?) */
> >>>   async->events |= COMEDI_CB_OVERFLOW;
> >>>   goto underrun;
> >>>
> >>
> >>NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.
> >>
> >
> >Thanks for pointing this out. In that case will assigning the variable to
> >devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet.
> >
> >Otherwise I'll just drop the variable and adjust the lines to avoid
> >checkpatch warning.
> >
> >Sorry for the inconvenience caused.
> >
> >[ Snip ]
> >
> > /* output the last scan */
> > for (i = 0; i < cmd->scan_end_arg; i++) {
> > unsigned int chan = CR_CHAN(cmd->chanlist[i]);
> > unsigned short data;
> >
> > if (!comedi_buf_read_samples(s, &data, 1)) {
> > /* unexpected underrun! (cancelled?) */
> > async->events |= COMEDI_CB_OVERFLOW;
> > goto underrun;
> > }
> >
> > devpriv->ao_loopbacks[chan] = data;
> > }
> > /* advance time of last scan */
> >
> >[ Snip ]
> 
> It will work, but you could just use a pointer variable set to
> &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples().
> 

Thanks for the suggestion. I tried below snippet 1 with the shortest pointer
name but 80 characters is exceeded. The declaration and initialisation
will have to be splitted. Will this be acceptable or am I doing it wrong
again?

Sorry for the trouble.

Snippet 1:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *p = 
&devpriv->ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, p, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}
/* advance time of last scan */

[ Snip ]

Snippet 2:
[ Snip ]

/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
unsigned short *pd;

pd = &devpriv->ao_loopbacks[chan];

if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
}
}

[ Snip ]

Thks.
Brgds,
CheahKC
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Cheah Kok Cheong]

On Tue, Feb 21, 2017 at 11:20:08AM +0100, Valentin Rothberg wrote:
> On Feb 21 '17 10:12, Ian Abbott wrote:
> > On 21/02/2017 09:33, Cheah Kok Cheong wrote:
> > > On Mon, Feb 20, 2017 at 05:36:52PM +, Ian Abbott wrote:
> > > > On 20/02/17 16:02, Cheah Kok Cheong wrote:
> > > > > On Mon, Feb 20, 2017 at 10:03:39AM +, Ian Abbott wrote:
> > > > > > On 20/02/17 08:28, Cheah Kok Cheong wrote:
> > > > > > > Fix checkpatch warning "Avoid multiple line dereference"
> > > > > > > using a local variable to avoid line wrap.
> > > > > > > 
> > > > > > > Signed-off-by: Cheah Kok Cheong 
> > > > > > > ---
> > > > > > > drivers/staging/comedi/drivers/comedi_test.c | 6 ++
> > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
> > > > > > > b/drivers/staging/comedi/drivers/comedi_test.c
> > > > > > > index 2a063f0..fde83e0 100644
> > > > > > > --- a/drivers/staging/comedi/drivers/comedi_test.c
> > > > > > > +++ b/drivers/staging/comedi/drivers/comedi_test.c
> > > > > > > @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long 
> > > > > > > arg)
> > > > > > >   /* output the last scan */
> > > > > > >   for (i = 0; i < cmd->scan_end_arg; i++) {
> > > > > > >   unsigned int chan = 
> > > > > > > CR_CHAN(cmd->chanlist[i]);
> > > > > > > + unsigned short d = 
> > > > > > > devpriv->ao_loopbacks[chan];
> > > > > > > 
> > > > > > > - if (comedi_buf_read_samples(s,
> > > > > > > - &devpriv->
> > > > > > > -  
> > > > > > > ao_loopbacks[chan],
> > > > > > > - 1) == 0) {
> > > > > > > + if (!comedi_buf_read_samples(s, &d, 1)) 
> > > > > > > {
> > > > > > >   /* unexpected underrun! 
> > > > > > > (cancelled?) */
> > > > > > >   async->events |= 
> > > > > > > COMEDI_CB_OVERFLOW;
> > > > > > >   goto underrun;
> > > > > > > 
> > > > > > 
> > > > > > NAK.  This leaves devpriv->ao_loopbacks[chan] unchanged.
> > > > > > 
> > > > > 
> > > > > Thanks for pointing this out. In that case will assigning the 
> > > > > variable to
> > > > > devpriv->ao_loopbacks[chan] be acceptable? Please review below 
> > > > > snippet.
> > > > > 
> > > > > Otherwise I'll just drop the variable and adjust the lines to avoid
> > > > > checkpatch warning.
> > > > > 
> > > > > Sorry for the inconvenience caused.
> > > > > 
> > > > > [ Snip ]
> > > > > 
> > > > >   /* output the last scan */
> > > > >   for (i = 0; i < cmd->scan_end_arg; i++) {
> > > > >   unsigned int chan = 
> > > > > CR_CHAN(cmd->chanlist[i]);
> > > > >   unsigned short data;
> > > > > 
> > > > >   if (!comedi_buf_read_samples(s, &data, 
> > > > > 1)) {
> > > > >   /* unexpected underrun! 
> > > > > (cancelled?) */
> > > > >   async->events |= 
> > > > > COMEDI_CB_OVERFLOW;
> > > > >   goto underrun;
> > > > >   }
> > > > > 
> > > > >   devpriv->ao_loopbacks[chan] = data;
> > > > >   }
> > > > >   

[PATCH v2] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference

[Cheah Kok Cheong]

Fix checkpatch warning "Avoid multiple line dereference"
using a pointer variable to avoid line wrap.

Signed-off-by: Cheah Kok Cheong 
---

V2:
-Use pointer instead of normal variable - Ian
-Variable is to be used as "write destination" and
 not as "read source" - Ian

 drivers/staging/comedi/drivers/comedi_test.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index 2a063f0..ccfd642 100644
--- a/drivers/staging/comedi/drivers/comedi_test.c
+++ b/drivers/staging/comedi/drivers/comedi_test.c
@@ -480,11 +480,11 @@ static void waveform_ao_timer(unsigned long arg)
/* output the last scan */
for (i = 0; i < cmd->scan_end_arg; i++) {
unsigned int chan = CR_CHAN(cmd->chanlist[i]);
+   unsigned short *pd;
 
-   if (comedi_buf_read_samples(s,
-   &devpriv->
-ao_loopbacks[chan],
-   1) == 0) {
+   pd = &devpriv->ao_loopbacks[chan];
+
+   if (!comedi_buf_read_samples(s, pd, 1)) {
/* unexpected underrun! (cancelled?) */
async->events |= COMEDI_CB_OVERFLOW;
goto underrun;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

Change to unsigned to allow removal of negative value check in
init section. Use smaller data type since the max possible
value currently is 48.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 57e8599..354d264 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -76,8 +76,8 @@ struct comedi_file {
 #define COMEDI_NUM_SUBDEVICE_MINORS\
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)
 
-static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, 0444);
+static unsigned short comedi_num_legacy_minors;
+module_param(comedi_num_legacy_minors, ushort, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for 
non-auto-configured devices (default 0)"
);
@@ -2857,8 +2857,7 @@ static int __init comedi_init(void)
 
pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n";);
 
-   if (comedi_num_legacy_minors < 0 ||
-   comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
   COMEDI_NUM_BOARD_MINORS);
return -EINVAL;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail.

In this case a default to auto-configuration comedi_test module failed
to load with the following messages.

comedi_test comedi_testd: ran out of minor numbers for board device files
comedi_test comedi_testd: driver 'comedi_test' could not create device.
comedi_test: unable to auto-configure device

This is due to changes in commit 38b9722a4414
("staging: comedi: avoid releasing legacy minors automatically") which
will not allocate a minor number when comedi_num_legacy_minors equals
COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
0x30 which is 48.

This goes for a simple fix which limit comedi_num_legacy_minors to 47
instead of tinkering with comedi_alloc_board_minor() and
comedi_release_hardware_device().

Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
automatically")

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 354d264..339854f 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2857,9 +2857,9 @@ static int __init comedi_init(void)
 
pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n";);
 
-   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors >= COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
-  COMEDI_NUM_BOARD_MINORS);
+  COMEDI_NUM_BOARD_MINORS - 1);
return -EINVAL;
}
 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

Change to unsigned to allow removal of negative value check in
init section. Use smaller data type since the max possible
value currently is 48.

Signed-off-by: Cheah Kok Cheong 
---

V2:
-No changes.

 drivers/staging/comedi/comedi_fops.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 57e8599..354d264 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -76,8 +76,8 @@ struct comedi_file {
 #define COMEDI_NUM_SUBDEVICE_MINORS\
(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)
 
-static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, 0444);
+static unsigned short comedi_num_legacy_minors;
+module_param(comedi_num_legacy_minors, ushort, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 "number of comedi minor devices to reserve for 
non-auto-configured devices (default 0)"
);
@@ -2857,8 +2857,7 @@ static int __init comedi_init(void)
 
pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n";);
 
-   if (comedi_num_legacy_minors < 0 ||
-   comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
   COMEDI_NUM_BOARD_MINORS);
return -EINVAL;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail at auto-configuration. If there's no fall back in
place then module loading will fail.

In this case, a default to auto-configure comedi_test module failed
to auto-configure with the following messages. It loaded but fell back
to unconfigured mode.

comedi_test comedi_testd: ran out of minor numbers for board device files
comedi_test comedi_testd: driver 'comedi_test' could not create device.
comedi_test: unable to auto-configure device

This is due to changes in commit 38b9722a4414
("staging: comedi: avoid releasing legacy minors automatically") which
will not allocate a minor number when comedi_num_legacy_minors equals
COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
0x30 which is 48.

This goes for a simple fix which limit comedi_num_legacy_minors to 47
instead of tinkering with comedi_alloc_board_minor() and
comedi_release_hardware_device().

Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
automatically")

Signed-off-by: Cheah Kok Cheong 
---

V2:
-Amend commit log to specify that comedi_test module failed
 to auto-configure and fell back to unconfigured mode.
 For other devices with no fall back, module loading will fail.

 drivers/staging/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 354d264..339854f 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2857,9 +2857,9 @@ static int __init comedi_init(void)
 
pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n";);
 
-   if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+   if (comedi_num_legacy_minors >= COMEDI_NUM_BOARD_MINORS) {
pr_err("invalid value for module parameter 
\"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
-  COMEDI_NUM_BOARD_MINORS);
+  COMEDI_NUM_BOARD_MINORS - 1);
return -EINVAL;
}
 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

Dear Greg,
  Thanks for taking the time to review.

On Tue, Mar 07, 2017 at 08:01:38PM +0100, Greg KH wrote:
> On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> > Change to unsigned to allow removal of negative value check in
> > init section.
> 
> Why?
> 

User can input a -ve number as parameter for module loading.
This will be caught by the mentioned check and cause loading to fail.
I think the original intention here is to inform user via kernel log
the acceptable input range.

Now if a user doesn't know how to access the log, it's of no help.

If a user does know how to access the log, probably also know how
to use modinfo or know that reserving a -ve number of minors is
not acceptable.

IMHO, this check is pointless and best enforced in module_param.
 
> > Use smaller data type since the max possible value currently is 48.
> 
> Does it matter?
> 

You are right it doesn't matter and not worth the effort doing unless
concurrent work is being done in this area.
Which is what patch 2/2 is trying to do here.
Please note that I've sent V2. Patch 1/2 unchanged and 2/2 with amended
commit log.

Thanks,
Brgds,
CheahKC

> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

Dear Dan,
  Thanks for reviewing this patch.

On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:
> On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
> > If comedi module is loaded with the following max allowed parameter
> > [comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> > device will fail.
> 
> Don't set comedi_num_legacy_minors=48, then?
> 
> This doesn't seem like the right fix at all.  Why only allow one auto
> configured board?  Why not 5 or 10?
> 

Let me explain, the original intended behaviour is to allow user to
reserve up to 48 minor numbers for legacy devices.

Therefore [sudo modprobe comedi comedi_num_legacy_minors=3] 
will allocate minor number 0, 1, 2 for legacy devices.
Subsequent loading of an auto-configured device will use minor number 3.
And the next one number 4 so on and so forth.

Now for the corner case of [comedi_num_legacy_minors=48] which
is supposed to reserve minor number 0 till 47 for legacy devices,
and is supposed to allocate number 48 and so on for auto-configured
devices, does not allocate number 48 anymore after commit
38b9722a4414.

This is due to the changes in comedi_alloc_board_minor().

As to why I chose to limit [comedi_num_legacy_minors=47], is given
in the commit log.

This will allow user to allocate 0 till 46 for legacy devices and
subsequent auto-configured devices will start from 47 and so forth.

I don't think anybody will miss one less number for legacy devices
otherwise there'll be complains earlier on.

Thanks.

Brgds,
CheahKC 

> regards,
> dan carpenter
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

On Wed, Mar 08, 2017 at 10:45:26AM +0100, Greg KH wrote:
> On Wed, Mar 08, 2017 at 05:38:12PM +0800, Cheah Kok Cheong wrote:
> > Dear Greg,
> >   Thanks for taking the time to review.
> > 
> > On Tue, Mar 07, 2017 at 08:01:38PM +0100, Greg KH wrote:
> > > On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> > > > Change to unsigned to allow removal of negative value check in
> > > > init section.
> > > 
> > > Why?
> > > 
> > 
> > User can input a -ve number as parameter for module loading.
> 
> Then they are foolish to do so :)
> 
> > This will be caught by the mentioned check and cause loading to fail.
> > I think the original intention here is to inform user via kernel log
> > the acceptable input range.
> 
> Either is fine.
> 
> > Now if a user doesn't know how to access the log, it's of no help.
> 
> They know how to set a module parameter as root but do not know of the
> kernel log?  That's trying a bit too hard :)
> 
> > If a user does know how to access the log, probably also know how
> > to use modinfo or know that reserving a -ve number of minors is
> > not acceptable.
> > 
> > IMHO, this check is pointless and best enforced in module_param.
> 
> Ok, but it's really a minor, or no, real issue at all here.
> 

I agree with you and that's why I mentioned it's not worth doing
unless there's concurrent work in this area.

Thanks.
Brgds,
CheahKC

> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

Dear Ian,
  Thanks for taking the trouble to reply.

On Wed, Mar 08, 2017 at 11:13:49AM +, Ian Abbott wrote:
> On 08/03/17 10:08, Cheah Kok Cheong wrote:
> >Dear Dan,
> >  Thanks for reviewing this patch.
> >
> >On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:
> >>On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
> >>>If comedi module is loaded with the following max allowed parameter
> >>>[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> >>>device will fail.
> >>
> >>Don't set comedi_num_legacy_minors=48, then?
> >>
> >>This doesn't seem like the right fix at all.  Why only allow one auto
> >>configured board?  Why not 5 or 10?
> >>
> >
> >Let me explain, the original intended behaviour is to allow user to
> >reserve up to 48 minor numbers for legacy devices.
> >
> >Therefore [sudo modprobe comedi comedi_num_legacy_minors=3]
> >will allocate minor number 0, 1, 2 for legacy devices.
> >Subsequent loading of an auto-configured device will use minor number 3.
> >And the next one number 4 so on and so forth.
> >
> >Now for the corner case of [comedi_num_legacy_minors=48] which
> >is supposed to reserve minor number 0 till 47 for legacy devices,
> >and is supposed to allocate number 48 and so on for auto-configured
> >devices, does not allocate number 48 anymore after commit
> >38b9722a4414.
> 
> Both legacy and auto-configured devices will have minor numbers less than
> 48, which is the total number of devices currently supported by Comedi.
> Minor numbers 48 and above have been reserved for dynamic allocation to
> those Comedi subdevices that support asynchronous commands.
> 

Thanks for the explanation.

> >This is due to the changes in comedi_alloc_board_minor().
> >
> >As to why I chose to limit [comedi_num_legacy_minors=47], is given
> >in the commit log.
> >
> >This will allow user to allocate 0 till 46 for legacy devices and
> >subsequent auto-configured devices will start from 47 and so forth.
> >
> >I don't think anybody will miss one less number for legacy devices
> >otherwise there'll be complains earlier on.
> 
> As Dan implies above, if you want auto-configured devices to work, set
> comedi_num_legacy_minors to a value less than 48.  Further limiting
> comedi_num_legacy_minors to 47 seems a bit arbitrary.
> 
> The comedi_test module mentioned in your commit can still be used when
> comedi_num_legacy_minors=48 by configuring a "legacy" comedi_test device.
> Most of the other Comedi drivers support "legacy" devices exclusive-or
> auto-configured devices.
> 

Ah ok now I understand what Dan is trying to say actually. Thanks for the
detailed clarification. Sorry for the inconvenience caused.

Thanks.

Brgds,
CheahKC

> >Thanks.
> >
> >Brgds,
> >CheahKC
> >
> >>regards,
> >>dan carpenter
> >>
> 
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.E-mail:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

Dear Ian,
  Thanks for taking the time to reply.

On Wed, Mar 08, 2017 at 12:36:41PM +, Ian Abbott wrote:
> On 07/03/17 18:13, Cheah Kok Cheong wrote:
> >If comedi module is loaded with the following max allowed parameter
> >[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> >device will fail at auto-configuration. If there's no fall back in
> >place then module loading will fail.
> >
> >In this case, a default to auto-configure comedi_test module failed
> >to auto-configure with the following messages. It loaded but fell back
> >to unconfigured mode.
> >
> >comedi_test comedi_testd: ran out of minor numbers for board device files
> >comedi_test comedi_testd: driver 'comedi_test' could not create device.
> >comedi_test: unable to auto-configure device
> >
> >This is due to changes in commit 38b9722a4414
> >("staging: comedi: avoid releasing legacy minors automatically") which
> >will not allocate a minor number when comedi_num_legacy_minors equals
> >COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
> >0x30 which is 48.
> 
> Sorry, I don't consider this to be a bug.  The number of minor device
> numbers available for auto-configured devices is 48 minus
> comedi_num_legacy_minors.  Using up all available minor device numbers is a
> useful test case for running out of minor device numbers, although this
> relies on knowing the limit is 48.  Perhaps the description of the module
> parameter could be improved to mention the limits, as could the error
> message when running out of minor device numbers.
> 
> Commit 38b9722a4414 is irrelevant here.  Prior to that commit, the first
> 'comedi_num_legacy_minors' minor numbers were still allocated to legacy
> devices created during module initialization, leaving 48 minus
> comedi_num_legacy_minors minors (possibly none) available for
> auto-configured devices.
>

Thanks for the detailed explanation. It is rather strange to allow a user
to use up number 0 to 47 for legacy devices and leaving none for auto
configured devices. Since it's intentional as you've mentioned as a test
case, I agree it's best if some form of description or documentation
is in place. I'm sure this will be in good hands.

Sorry for any inconvenience caused.

Thanks.

Brgds,
CheahKC
  
> >This goes for a simple fix which limit comedi_num_legacy_minors to 47
> >instead of tinkering with comedi_alloc_board_minor() and
> >comedi_release_hardware_device().
> >
> >Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
> >automatically")
> >
> >Signed-off-by: Cheah Kok Cheong 
> >---
> >
> >V2:
> >-Amend commit log to specify that comedi_test module failed
> > to auto-configure and fell back to unconfigured mode.
> > For other devices with no fall back, module loading will fail.
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.E-mail:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Staging: comedi: kcomedilib: Add module_init/exit function

[Cheah Kok Cheong]

Add init/exit function to follow LKM semantics.
Apparently this module can still load/unload without
the init/exit function.

Tested loading/unloading with and without this patch.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/kcomedilib/kcomedilib_main.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c 
b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
index d0a8a28..55d43c0 100644
--- a/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
+++ b/drivers/staging/comedi/kcomedilib/kcomedilib_main.c
@@ -250,3 +250,15 @@ int comedi_get_n_channels(struct comedi_device *dev, 
unsigned int subdevice)
return n;
 }
 EXPORT_SYMBOL_GPL(comedi_get_n_channels);
+
+static int __init kcomedilib_module_init(void)
+{
+   return 0;
+}
+
+static void __exit kcomedilib_module_exit(void)
+{
+}
+
+module_init(kcomedilib_module_init);
+module_exit(kcomedilib_module_exit);
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] Staging: comedi: comedidev.h: Drop old style zero-length array

[Cheah Kok Cheong]

According to Documentation/Changes, the minimum gcc version required
to compile the kernel is 3.2 (this is probably outdated too).

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedidev.h | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index 0c7c37a..9713d96 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -612,12 +612,6 @@ extern const struct comedi_lrange range_unknown;
 
 #define range_digital  range_unipolar5
 
-#if __GNUC__ >= 3
-#define GCC_ZERO_LENGTH_ARRAY
-#else
-#define GCC_ZERO_LENGTH_ARRAY 0
-#endif
-
 /**
  * struct comedi_lrange - Describes a COMEDI range table
  * @length: Number of entries in the range table.
@@ -631,7 +625,7 @@ extern const struct comedi_lrange range_unknown;
  */
 struct comedi_lrange {
int length;
-   struct comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
+   struct comedi_krange range[];
 };
 
 /**
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/5] Staging: comedi: Proc FS related cleanup

[Cheah Kok Cheong]

This series does trivial cleanup for COMEDI proc fs related stuff.

Cheah Kok Cheong (5):
  Staging: comedi: comedi_fops: Avoid orphaned proc entry
  Staging: comedi: proc: Change file permission to read only
  Staging: comedi: proc: Add __init prefix
  Staging: comedi: proc: Add module owner
  Staging: comedi: proc: Warn if unable to create proc entry

 drivers/staging/comedi/comedi_fops.c | 6 +++---
 drivers/staging/comedi/proc.c| 6 --
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/5] Staging: comedi: comedi_fops: Avoid orphaned proc entry

[Cheah Kok Cheong]

Move comedi_proc_init to the end to avoid orphaned proc entry
if module loading failed.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 64b3966..02df354 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2898,9 +2898,6 @@ static int __init comedi_init(void)
 
comedi_class->dev_groups = comedi_dev_groups;
 
-   /* XXX requires /proc interface */
-   comedi_proc_init();
-
/* create devices files for legacy/manual use */
for (i = 0; i < comedi_num_legacy_minors; i++) {
struct comedi_device *dev;
@@ -2917,6 +2914,9 @@ static int __init comedi_init(void)
mutex_unlock(&dev->mutex);
}
 
+   /* XXX requires /proc interface */
+   comedi_proc_init();
+
return 0;
 }
 module_init(comedi_init);
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/5] Staging: comedi: proc: Change file permission to read only

[Cheah Kok Cheong]

As there's no write operation, change to read only.
Was inadvertantly switched to 0644 in commit 1f817b86d5e6
("comedi: Don't use create_proc_read_entry()").

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
index 91dea25..3513f4c 100644
--- a/drivers/staging/comedi/proc.c
+++ b/drivers/staging/comedi/proc.c
@@ -88,7 +88,7 @@ static const struct file_operations comedi_proc_fops = {
 
 void comedi_proc_init(void)
 {
-   proc_create("comedi", 0644, NULL, &comedi_proc_fops);
+   proc_create("comedi", 0444, NULL, &comedi_proc_fops);
 }
 
 void comedi_proc_cleanup(void)
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/5] Staging: comedi: proc: Add __init prefix

[Cheah Kok Cheong]

Add __init prefix so that symbol will be discarded after
module loading.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
index 3513f4c..6d13b51 100644
--- a/drivers/staging/comedi/proc.c
+++ b/drivers/staging/comedi/proc.c
@@ -86,7 +86,7 @@ static const struct file_operations comedi_proc_fops = {
.release= single_release,
 };
 
-void comedi_proc_init(void)
+void __init comedi_proc_init(void)
 {
proc_create("comedi", 0444, NULL, &comedi_proc_fops);
 }
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] Staging: comedi: proc: Add module owner

[Cheah Kok Cheong]

Since this is a loadable kernel module, add module ownership
to follow LKM semantics.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
index 6d13b51..99b23b2e 100644
--- a/drivers/staging/comedi/proc.c
+++ b/drivers/staging/comedi/proc.c
@@ -80,6 +80,7 @@ static int comedi_proc_open(struct inode *inode, struct file 
*file)
 }
 
 static const struct file_operations comedi_proc_fops = {
+   .owner  = THIS_MODULE,
.open   = comedi_proc_open,
.read   = seq_read,
.llseek = seq_lseek,
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/5] Staging: comedi: proc: Warn if unable to create proc entry

[Cheah Kok Cheong]

The proc entry is not essential for the comedi system as
evident by the support for !CONFIG_PROC_FS. So for failure
to create, just warn and continue loading.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/proc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/proc.c b/drivers/staging/comedi/proc.c
index 99b23b2e..2644dd4 100644
--- a/drivers/staging/comedi/proc.c
+++ b/drivers/staging/comedi/proc.c
@@ -89,7 +89,8 @@ static const struct file_operations comedi_proc_fops = {
 
 void __init comedi_proc_init(void)
 {
-   proc_create("comedi", 0444, NULL, &comedi_proc_fops);
+   if (!proc_create("comedi", 0444, NULL, &comedi_proc_fops))
+   pr_warn("comedi: unable to create proc entry\n");
 }
 
 void comedi_proc_cleanup(void)
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] Staging: comedi: comedi_fops: Remove unused stat.h header

[Cheah Kok Cheong]

Unused after commit 6e3029397698 ("staging: comedi: comedi_fops:
coding style fixes") - Fixed coding style in comedi_fops.c
Symbolic to octal permission.

Anyway it's included in module.h

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 5c06ba6..2e8c6d9 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -33,7 +33,6 @@
 #include 
 #include "comedidev.h"
 #include 
-#include 
 
 #include 
 #include 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/4] Staging: comedi: comedi_fops: Header cleanup

[Cheah Kok Cheong]

This series does trivial header cleanup for comedi_fops.c

Cheah Kok Cheong (4):
  Staging: comedi: comedi_fops: Remove unused kmod.h header
  Staging: comedi: comedi_fops: Remove redundant init.h header
  Staging: comedi: comedi_fops: Remove unused vmalloc.h header
  Staging: comedi: comedi_fops: Remove unused stat.h header

 drivers/staging/comedi/comedi_fops.c | 4 
 1 file changed, 4 deletions(-)

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] Staging: comedi: comedi_fops: Remove unused vmalloc.h header

[Cheah Kok Cheong]

Unused after commit d18431325be0 ("staging: comedi:
deprecate loading firmware with comedi_config").

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 8bcb1cc..5c06ba6 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -30,7 +30,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include "comedidev.h"
 #include 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/4] Staging: comedi: comedi_fops: Remove unused kmod.h header

[Cheah Kok Cheong]

Unused after commit f30f2c2d417b ("staging: comedi:
remove check for CONFIG_KMOD").

Anyway it's included in module.h

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 64b3966..7ad9092 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -28,7 +28,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] Staging: comedi: comedi_fops: Remove redundant init.h header

[Cheah Kok Cheong]

After commit 0fd972a7d91d ("module: relocate module_init
from init.h to module.h"), including module.h will do and
init.h is also thrown in.

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/comedi_fops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 7ad9092..8bcb1cc 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -29,7 +29,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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: 100 microvolt, 10 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=250 period=15]

comedi_test: 250 microvolt, 15 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 
---
 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 
 #include 
 #include 
+#include 
+#include 
 
 #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 = 100;/* 1 volt */
-   if (period <= 0)
-   period = 10;/* 0.1 sec */
-
devpriv->wf_amplitude = amplitude;
devpriv->w

[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 
---
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 > 3)
+   set_amplitude = 3;
+
+   /* limit max input period [= 1 hertz] to avoid too low freq */
+   if (set_period > 100)
+   set_period = 100;
+
int amplitude = set_amplitude;
int period = set_period;
 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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: 100 microvolt, 10 microsecond waveform attached
> driver 'comedi_test' has successfully auto-configured 'comedi_test'.
> 
> [sudo modprobe comedi_test amplitude=250 period=15]
> 
> comedi_test: 250 microvolt, 15 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #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 ampl

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 +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 +, 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 
> 
> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

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: 100 microvolt, 10 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test amplitude=250 period=15]

comedi_test: 250 microvolt, 15 microsecond waveform attached
driver 'comedi_test' has successfully auto-configured 'comedi_test'.

[sudo modprobe comedi_test noauto=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 
---

V2:
-Rename module param - Ian
-Rename class - Ian
-Tidy up init error handling - Ian
-Allow module loading to continue when auto-configuration fails - Ian
-Remove redundant "if" statement from module exit
-Edit driver intro to reflect changes

 drivers/staging/comedi/drivers/comedi_test.c | 130 ---
 1 file changed, 118 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_test.c 
b/drivers/staging/comedi/drivers/comedi_test.c
index ec5b9a2..027c972 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 disable auto-configuration mode, pass "noauto=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 
 #include 
 #include 
+#include 
+#include 
 
 #define N_CHANS 8
+#define DEV_NAME "comedi_testd"
+#define CLASS_NAME "comedi_test"
+
+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(noauto, config_mode, bool, 0444);
+MODULE_PARM_DESC(noauto, "Disable auto-configuration: (1=disable [defaults to 
enable])");
+
+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 ampl

[PATCH] staging: comedi: drivers: replace le16_to_cpu() with usb_endpoint_maxp()

[Cheah Kok Cheong]

Use macro introduced in commit 939f325f4a0f
("usb: add usb_endpoint_maxp() macro")

Signed-off-by: Cheah Kok Cheong 
---
 drivers/staging/comedi/drivers/dt9812.c |  4 ++--
 drivers/staging/comedi/drivers/ni_usb6501.c |  4 ++--
 drivers/staging/comedi/drivers/vmk80xx.c| 12 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt9812.c 
b/drivers/staging/comedi/drivers/dt9812.c
index 3295bb4..7ebca86 100644
--- a/drivers/staging/comedi/drivers/dt9812.c
+++ b/drivers/staging/comedi/drivers/dt9812.c
@@ -660,12 +660,12 @@ static int dt9812_find_endpoints(struct comedi_device 
*dev)
case 1:
dir = USB_DIR_OUT;
devpriv->cmd_wr.addr = ep->bEndpointAddress;
-   devpriv->cmd_wr.size = le16_to_cpu(ep->wMaxPacketSize);
+   devpriv->cmd_wr.size = usb_endpoint_maxp(ep);
break;
case 2:
dir = USB_DIR_IN;
devpriv->cmd_rd.addr = ep->bEndpointAddress;
-   devpriv->cmd_rd.size = le16_to_cpu(ep->wMaxPacketSize);
+   devpriv->cmd_rd.size = usb_endpoint_maxp(ep);
break;
case 3:
/* unused write stream */
diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c 
b/drivers/staging/comedi/drivers/ni_usb6501.c
index 95b537a..5036eeb 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -465,12 +465,12 @@ static int ni6501_alloc_usb_buffers(struct comedi_device 
*dev)
struct ni6501_private *devpriv = dev->private;
size_t size;
 
-   size = le16_to_cpu(devpriv->ep_rx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_rx);
devpriv->usb_rx_buf = kzalloc(size, GFP_KERNEL);
if (!devpriv->usb_rx_buf)
return -ENOMEM;
 
-   size = le16_to_cpu(devpriv->ep_tx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_tx);
devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
if (!devpriv->usb_tx_buf) {
kfree(devpriv->usb_rx_buf);
diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
b/drivers/staging/comedi/drivers/vmk80xx.c
index 8c7393e..a004aed 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -177,7 +177,7 @@ static void vmk80xx_do_bulk_msg(struct comedi_device *dev)
 * The max packet size attributes of the K8061
 * input/output endpoints are identical
 */
-   size = le16_to_cpu(devpriv->ep_tx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_tx);
 
usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf,
 size, NULL, devpriv->ep_tx->bInterval);
@@ -199,7 +199,7 @@ static int vmk80xx_read_packet(struct comedi_device *dev)
ep = devpriv->ep_rx;
pipe = usb_rcvintpipe(usb, ep->bEndpointAddress);
return usb_interrupt_msg(usb, pipe, devpriv->usb_rx_buf,
-le16_to_cpu(ep->wMaxPacketSize), NULL,
+usb_endpoint_maxp(ep), NULL,
 HZ * 10);
 }
 
@@ -220,7 +220,7 @@ static int vmk80xx_write_packet(struct comedi_device *dev, 
int cmd)
ep = devpriv->ep_tx;
pipe = usb_sndintpipe(usb, ep->bEndpointAddress);
return usb_interrupt_msg(usb, pipe, devpriv->usb_tx_buf,
-le16_to_cpu(ep->wMaxPacketSize), NULL,
+usb_endpoint_maxp(ep), NULL,
 HZ * 10);
 }
 
@@ -230,7 +230,7 @@ static int vmk80xx_reset_device(struct comedi_device *dev)
size_t size;
int retval;
 
-   size = le16_to_cpu(devpriv->ep_tx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_tx);
memset(devpriv->usb_tx_buf, 0, size);
retval = vmk80xx_write_packet(dev, VMK8055_CMD_RST);
if (retval)
@@ -684,12 +684,12 @@ static int vmk80xx_alloc_usb_buffers(struct comedi_device 
*dev)
struct vmk80xx_private *devpriv = dev->private;
size_t size;
 
-   size = le16_to_cpu(devpriv->ep_rx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_rx);
devpriv->usb_rx_buf = kzalloc(size, GFP_KERNEL);
if (!devpriv->usb_rx_buf)
return -ENOMEM;
 
-   size = le16_to_cpu(devpriv->ep_tx->wMaxPacketSize);
+   size = usb_endpoint_maxp(devpriv->ep_tx);
devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
if (!devpriv->usb_tx_buf) {
kfree(devpriv->usb_rx_buf);
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel