Keep the details of the comedi subdevice `runflags` member local to
"comedi_fops.c". In particular, the usage of the
`COMEDI_SRF_FREE_SPRIV` run-flag doesn't really fit in all that well
with the others. It's used as a marker to indicate the subdevice's
`private` pointer can be automatically freed by the subdevice
clean-up code, whereas the others are associated with the operation of
asynchronous comedi commands. Abstract it's usage away in a couple of
new wrapper functions.
1) staging: comedi: wrap COMEDI_SRF_FREE_SPRIV usage
2) staging: comedi: move COMEDI_SRF_... macros to "comedi_fops.c"
drivers/staging/comedi/comedi_fops.c | 41 ++++++++++++++++++++--
drivers/staging/comedi/comedi_internal.h | 1 +
drivers/staging/comedi/comedidev.h | 18 +---------
drivers/staging/comedi/drivers.c | 2 +-
.../staging/comedi/drivers/amplc_dio200_common.c | 6 ++--
5 files changed, 45 insertions(+), 23 deletions(-)
The `COMEDI_SRF_FREE_SPRIV` flag in the `runflags` member of `struct
comedi_subdevice` indicates that the memory pointed to by the `private`
member can be automatically freed by the comedi core on subdevice
clean-up (when the low-level comedi device is being "detached"). the
flag doesn't really belong in `runflags`, but it was somewhere
convenient to keep it without having to add a new member to the
structure.
Rather than access the `COMEDI_SRF_FREE_SPRIV` flag directly, use some
new wrapper functions:
* comedi_can_auto_free_spriv(s) - checks whether the subdevice's
`s->private` points to memory that can be freed automatically.
* comedi_set_spriv_auto_free(s) - marks the subdevice as having a
`s->private` that points to memory that can be freed automatically.
Export `comedi_set_spriv_auto_free()` for use by the low-level comedi
driver modules, in particular the "amplc_dio200_common" module.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 24 ++++++++++++++++++++--
drivers/staging/comedi/comedi_internal.h | 1 +
drivers/staging/comedi/comedidev.h | 1 +
drivers/staging/comedi/drivers.c | 2 +-
.../staging/comedi/drivers/amplc_dio200_common.c | 6 +++---
5 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index e78ddbe..fc339c5 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -679,8 +679,28 @@ static bool comedi_is_subdevice_idle(struct comedi_subdevice *s)
return !(runflags & COMEDI_SRF_BUSY_MASK);
}
+bool comedi_can_auto_free_spriv(struct comedi_subdevice *s)
+{
+ unsigned runflags = __comedi_get_subdevice_runflags(s);
+
+ return runflags & COMEDI_SRF_FREE_SPRIV;
+}
+
+/**
+ * comedi_set_spriv_auto_free - mark subdevice private data as freeable
+ * @s: comedi_subdevice struct
+ *
+ * Mark the subdevice as having a pointer to private data that can be
+ * automatically freed by the comedi core during the detach.
+ */
+void comedi_set_spriv_auto_free(struct comedi_subdevice *s)
+{
+ __comedi_set_subdevice_runflags(s, COMEDI_SRF_FREE_SPRIV);
+}
+EXPORT_SYMBOL_GPL(comedi_set_spriv_auto_free);
+
/**
- * comedi_alloc_spriv() - Allocate memory for the subdevice private data.
+ * comedi_alloc_spriv - Allocate memory for the subdevice private data.
* @s: comedi_subdevice struct
* @size: size of the memory to allocate
*
@@ -691,7 +711,7 @@ void *comedi_alloc_spriv(struct comedi_subdevice *s, size_t size)
{
s->private = kzalloc(size, GFP_KERNEL);
if (s->private)
- s->runflags |= COMEDI_SRF_FREE_SPRIV;
+ comedi_set_spriv_auto_free(s);
return s->private;
}
EXPORT_SYMBOL_GPL(comedi_alloc_spriv);
diff --git a/drivers/staging/comedi/comedi_internal.h b/drivers/staging/comedi/comedi_internal.h
index 3b91853..cd9437f 100644
--- a/drivers/staging/comedi/comedi_internal.h
+++ b/drivers/staging/comedi/comedi_internal.h
@@ -33,6 +33,7 @@ struct comedi_buf_map *comedi_buf_map_from_subdev_get(
struct comedi_subdevice *s);
unsigned int comedi_buf_write_n_allocated(struct comedi_subdevice *s);
void comedi_device_cancel_all(struct comedi_device *dev);
+bool comedi_can_auto_free_spriv(struct comedi_subdevice *s);
extern unsigned int comedi_default_buf_size_kb;
extern unsigned int comedi_default_buf_maxsize_kb;
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index dfab5a8..52071f7 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -323,6 +323,7 @@ int comedi_dev_put(struct comedi_device *dev);
bool comedi_is_subdevice_running(struct comedi_subdevice *s);
void *comedi_alloc_spriv(struct comedi_subdevice *s, size_t size);
+void comedi_set_spriv_auto_free(struct comedi_subdevice *s);
int comedi_check_chanlist(struct comedi_subdevice *s,
int n,
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 57dcffe..ed0b60c 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -125,7 +125,7 @@ static void comedi_device_detach_cleanup(struct comedi_device *dev)
if (dev->subdevices) {
for (i = 0; i < dev->n_subdevices; i++) {
s = &dev->subdevices[i];
- if (s->runflags & COMEDI_SRF_FREE_SPRIV)
+ if (comedi_can_auto_free_spriv(s))
kfree(s->private);
comedi_free_subdevice_minor(s);
if (s->async) {
diff --git a/drivers/staging/comedi/drivers/amplc_dio200_common.c b/drivers/staging/comedi/drivers/amplc_dio200_common.c
index d15a3dc..3a8b3f2 100644
--- a/drivers/staging/comedi/drivers/amplc_dio200_common.c
+++ b/drivers/staging/comedi/drivers/amplc_dio200_common.c
@@ -593,10 +593,10 @@ static int dio200_subdev_8254_init(struct comedi_device *dev,
* There could be multiple timers so this driver does not
* use dev->pacer to save the i8254 pointer. Instead,
* comedi_8254_subdevice_init() saved the i8254 pointer in
- * s->private. Set the runflag bit so that the core will
- * automatically free it when the driver is detached.
+ * s->private. Mark the subdevice as having private data
+ * to be automatically freed when the device is detached.
*/
- s->runflags |= COMEDI_SRF_FREE_SPRIV;
+ comedi_set_spriv_auto_free(s);
/* Initialize channels. */
if (board->has_clk_gat_sce) {
--
2.1.4
The `COMEDI_SRF_...` macros define flag combinations in the `runflags`
member of `struct comedi_subdevice`. They are only used directly in
"comedi_fops.c", so move them to there.
Signed-off-by: Ian Abbott <[email protected]>
---
drivers/staging/comedi/comedi_fops.c | 17 +++++++++++++++++
drivers/staging/comedi/comedidev.h | 17 -----------------
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index fc339c5..6f269cc 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -44,6 +44,23 @@
#include "comedi_internal.h"
/**
+ * comedi_subdevice "runflags"
+ * @COMEDI_SRF_RT: DEPRECATED: command is running real-time
+ * @COMEDI_SRF_ERROR: indicates an COMEDI_CB_ERROR event has occurred
+ * since the last command was started
+ * @COMEDI_SRF_RUNNING: command is running
+ * @COMEDI_SRF_FREE_SPRIV: free s->private on detach
+ *
+ * @COMEDI_SRF_BUSY_MASK: runflags that indicate the subdevice is "busy"
+ */
+#define COMEDI_SRF_RT BIT(1)
+#define COMEDI_SRF_ERROR BIT(2)
+#define COMEDI_SRF_RUNNING BIT(27)
+#define COMEDI_SRF_FREE_SPRIV BIT(31)
+
+#define COMEDI_SRF_BUSY_MASK (COMEDI_SRF_ERROR | COMEDI_SRF_RUNNING)
+
+/**
* struct comedi_file - per-file private data for comedi device
* @dev: comedi_device struct
* @read_subdev: current "read" subdevice
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 52071f7..28f2606 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -303,23 +303,6 @@ void comedi_event(struct comedi_device *dev, struct comedi_subdevice *s);
struct comedi_device *comedi_dev_get_from_minor(unsigned minor);
int comedi_dev_put(struct comedi_device *dev);
-/**
- * comedi_subdevice "runflags"
- * @COMEDI_SRF_RT: DEPRECATED: command is running real-time
- * @COMEDI_SRF_ERROR: indicates an COMEDI_CB_ERROR event has occurred
- * since the last command was started
- * @COMEDI_SRF_RUNNING: command is running
- * @COMEDI_SRF_FREE_SPRIV: free s->private on detach
- *
- * @COMEDI_SRF_BUSY_MASK: runflags that indicate the subdevice is "busy"
- */
-#define COMEDI_SRF_RT BIT(1)
-#define COMEDI_SRF_ERROR BIT(2)
-#define COMEDI_SRF_RUNNING BIT(27)
-#define COMEDI_SRF_FREE_SPRIV BIT(31)
-
-#define COMEDI_SRF_BUSY_MASK (COMEDI_SRF_ERROR | COMEDI_SRF_RUNNING)
-
bool comedi_is_subdevice_running(struct comedi_subdevice *s);
void *comedi_alloc_spriv(struct comedi_subdevice *s, size_t size);
--
2.1.4
On Tuesday, April 21, 2015 5:18 AM, Ian Abbott wrote:
> Keep the details of the comedi subdevice `runflags` member local to
> "comedi_fops.c". In particular, the usage of the
> `COMEDI_SRF_FREE_SPRIV` run-flag doesn't really fit in all that well
> with the others. It's used as a marker to indicate the subdevice's
> `private` pointer can be automatically freed by the subdevice
> clean-up code, whereas the others are associated with the operation of
> asynchronous comedi commands. Abstract it's usage away in a couple of
> new wrapper functions.
>
> 1) staging: comedi: wrap COMEDI_SRF_FREE_SPRIV usage
> 2) staging: comedi: move COMEDI_SRF_... macros to "comedi_fops.c"
>
> drivers/staging/comedi/comedi_fops.c | 41 ++++++++++++++++++++--
> drivers/staging/comedi/comedi_internal.h | 1 +
> drivers/staging/comedi/comedidev.h | 18 +---------
> drivers/staging/comedi/drivers.c | 2 +-
> .../staging/comedi/drivers/amplc_dio200_common.c | 6 ++--
> 5 files changed, 45 insertions(+), 23 deletions(-)
Reviewed-by: H Hartley Sweeten <[email protected]>
It seems that most all of the comedi code cleanliness
issues have been resolved.
What's left before this is accepted into mainline?