Changes in v2:
- Update the rest of the drivers under drivers/counter
The changes in this patchset will not affect the userspace interface.
Rather, these changes are intended to simplify the kernelspace Counter
callbacks for counter device driver authors.
The following main changes are proposed:
* Retire the opaque counter_count_read_value/counter_count_write_value
structures and simply represent count data as an unsigned integer.
* Retire the opaque counter_signal_read_value structure and represent
Signal data as a counter_signal_value enum.
These changes should reduce some complexity and code in the use and
implementation of the count_read, count_write, and signal_read
callbacks.
The opaque structures for Count data and Signal data were introduced
originally in anticipation of supporting various representations of
counter data (e.g. arbitrary-precision tallies, floating-point spherical
coordinate positions, etc). However, with the counter device drivers
that have appeared, it's become apparent that utilizing opaque
structures in kernelspace is not the best approach to take.
I believe it is best to let userspace applications decide how to
interpret the count data they receive. There are a couple of reasons why
it would be good to do so:
* Users use their devices in unexpected ways.
For example, a quadrature encoder counter device is typically used to
keep track of the position of a motor, but a user could set the device
in a pulse-direction mode and instead use it to count sporadic rising
edges from an arbitrary signal line unrelated to positioning. Users
should have the freedom to decide what their data represents.
* Most counter devices represent data as unsigned integers anyway.
For example, whether the device is a tally counter or position
counter, the count data is represented to the user as an unsigned
integer value. So specifying that one device is representing tallies
while the other specifies positions does not provide much utility from
an interface perspective.
For these reasons, the count_read and count_write callbacks have been
redefined to pass count data directly as unsigned long instead of passed
via opaque structures:
count_read(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
count_write(struct counter_device *counter,
struct counter_count *count, unsigned long val);
Similarly, the signal_read is redefined to pass Signal data directly as
a counter_signal_value enum instead of via an opaque structure:
signal_read(struct counter_device *counter,
struct counter_signal *signal,
enum counter_signal_value *val);
The counter_signal_value enum is simply the counter_signal_level enum
redefined to remove the references to the Signal data "level" data type.
William Breathitt Gray (7):
counter: Simplify the count_read and count_write callbacks
counter: Simplify the signal_read callback
docs: driver-api: generic-counter: Update Count and Signal data types
counter: 104-quad-8: Update count_read/count_write/signal_read
callbacks
counter: ftm-quaddec: Update count_read and count_write callbacks
counter: stm32-lptimer-cnt: Update count_read callback
counter: stm32-timer-cnt: Update count_read and count_write callbacks
Documentation/driver-api/generic-counter.rst | 22 ++--
drivers/counter/104-quad-8.c | 33 ++----
drivers/counter/counter.c | 101 +++----------------
drivers/counter/ftm-quaddec.c | 14 +--
drivers/counter/stm32-lptimer-cnt.c | 5 +-
drivers/counter/stm32-timer-cnt.c | 17 +---
include/linux/counter.h | 74 ++------------
7 files changed, 53 insertions(+), 213 deletions(-)
--
2.23.0
On Wed, Sep 18, 2019 at 04:52:41PM +0900, William Breathitt Gray wrote:
> Changes in v2:
> - Update the rest of the drivers under drivers/counter
Jonathan,
The TI eQEP driver also needs a patch for these changes if this patchset
is merged.
How would you like to handle the merge? We have an full cycle until the
5.5 merge window, so I can keep this patchset in my personal repository,
adding in the ChromeOS EC driver and Intel QEP driver when they are
ready, then send you a git pull request during the 5.5 merge window. Or
we can keep going as usual and merge this into your IIO repository, then
handle the TI eQEP driver when the time comes to merge.
William Breathitt Gray
> The changes in this patchset will not affect the userspace interface.
> Rather, these changes are intended to simplify the kernelspace Counter
> callbacks for counter device driver authors.
>
> The following main changes are proposed:
>
> * Retire the opaque counter_count_read_value/counter_count_write_value
> structures and simply represent count data as an unsigned integer.
>
> * Retire the opaque counter_signal_read_value structure and represent
> Signal data as a counter_signal_value enum.
>
> These changes should reduce some complexity and code in the use and
> implementation of the count_read, count_write, and signal_read
> callbacks.
>
> The opaque structures for Count data and Signal data were introduced
> originally in anticipation of supporting various representations of
> counter data (e.g. arbitrary-precision tallies, floating-point spherical
> coordinate positions, etc). However, with the counter device drivers
> that have appeared, it's become apparent that utilizing opaque
> structures in kernelspace is not the best approach to take.
>
> I believe it is best to let userspace applications decide how to
> interpret the count data they receive. There are a couple of reasons why
> it would be good to do so:
>
> * Users use their devices in unexpected ways.
>
> For example, a quadrature encoder counter device is typically used to
> keep track of the position of a motor, but a user could set the device
> in a pulse-direction mode and instead use it to count sporadic rising
> edges from an arbitrary signal line unrelated to positioning. Users
> should have the freedom to decide what their data represents.
>
> * Most counter devices represent data as unsigned integers anyway.
>
> For example, whether the device is a tally counter or position
> counter, the count data is represented to the user as an unsigned
> integer value. So specifying that one device is representing tallies
> while the other specifies positions does not provide much utility from
> an interface perspective.
>
> For these reasons, the count_read and count_write callbacks have been
> redefined to pass count data directly as unsigned long instead of passed
> via opaque structures:
>
> count_read(struct counter_device *counter,
> struct counter_count *count, unsigned long *val);
> count_write(struct counter_device *counter,
> struct counter_count *count, unsigned long val);
>
> Similarly, the signal_read is redefined to pass Signal data directly as
> a counter_signal_value enum instead of via an opaque structure:
>
> signal_read(struct counter_device *counter,
> struct counter_signal *signal,
> enum counter_signal_value *val);
>
> The counter_signal_value enum is simply the counter_signal_level enum
> redefined to remove the references to the Signal data "level" data type.
>
> William Breathitt Gray (7):
> counter: Simplify the count_read and count_write callbacks
> counter: Simplify the signal_read callback
> docs: driver-api: generic-counter: Update Count and Signal data types
> counter: 104-quad-8: Update count_read/count_write/signal_read
> callbacks
> counter: ftm-quaddec: Update count_read and count_write callbacks
> counter: stm32-lptimer-cnt: Update count_read callback
> counter: stm32-timer-cnt: Update count_read and count_write callbacks
>
> Documentation/driver-api/generic-counter.rst | 22 ++--
> drivers/counter/104-quad-8.c | 33 ++----
> drivers/counter/counter.c | 101 +++----------------
> drivers/counter/ftm-quaddec.c | 14 +--
> drivers/counter/stm32-lptimer-cnt.c | 5 +-
> drivers/counter/stm32-timer-cnt.c | 17 +---
> include/linux/counter.h | 74 ++------------
> 7 files changed, 53 insertions(+), 213 deletions(-)
>
> --
> 2.23.0
The count_read and count_write callbacks pass unsigned long now, while
the signal_read callback passes an enum counter_signal_value.
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/counter/104-quad-8.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 00b113f4b958..17e67a84777d 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -562,11 +562,10 @@ static const struct iio_chan_spec quad8_channels[] = {
};
static int quad8_signal_read(struct counter_device *counter,
- struct counter_signal *signal, struct counter_signal_read_value *val)
+ struct counter_signal *signal, enum counter_signal_value *val)
{
const struct quad8_iio *const priv = counter->priv;
unsigned int state;
- enum counter_signal_level level;
/* Only Index signal levels can be read */
if (signal->id < 16)
@@ -575,22 +574,19 @@ static int quad8_signal_read(struct counter_device *counter,
state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
& BIT(signal->id - 16);
- level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
-
- counter_signal_read_value_set(val, COUNTER_SIGNAL_LEVEL, &level);
+ *val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
return 0;
}
static int quad8_count_read(struct counter_device *counter,
- struct counter_count *count, struct counter_count_read_value *val)
+ struct counter_count *count, unsigned long *val)
{
const struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
unsigned int flags;
unsigned int borrow;
unsigned int carry;
- unsigned long position;
int i;
flags = inb(base_offset + 1);
@@ -598,36 +594,27 @@ static int quad8_count_read(struct counter_device *counter,
carry = !!(flags & QUAD8_FLAG_CT);
/* Borrow XOR Carry effectively doubles count range */
- position = (unsigned long)(borrow ^ carry) << 24;
+ *val = (unsigned long)(borrow ^ carry) << 24;
/* Reset Byte Pointer; transfer Counter to Output Latch */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
base_offset + 1);
for (i = 0; i < 3; i++)
- position |= (unsigned long)inb(base_offset) << (8 * i);
-
- counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &position);
+ *val |= (unsigned long)inb(base_offset) << (8 * i);
return 0;
}
static int quad8_count_write(struct counter_device *counter,
- struct counter_count *count, struct counter_count_write_value *val)
+ struct counter_count *count, unsigned long val)
{
const struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
- int err;
- unsigned long position;
int i;
- err = counter_count_write_value_get(&position, COUNTER_COUNT_POSITION,
- val);
- if (err)
- return err;
-
/* Only 24-bit values are supported */
- if (position > 0xFFFFFF)
+ if (val > 0xFFFFFF)
return -EINVAL;
/* Reset Byte Pointer */
@@ -635,7 +622,7 @@ static int quad8_count_write(struct counter_device *counter,
/* Counter can only be set via Preset Register */
for (i = 0; i < 3; i++)
- outb(position >> (8 * i), base_offset);
+ outb(val >> (8 * i), base_offset);
/* Transfer Preset Register to Counter */
outb(QUAD8_CTR_RLD | QUAD8_RLD_PRESET_CNTR, base_offset + 1);
@@ -644,9 +631,9 @@ static int quad8_count_write(struct counter_device *counter,
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
/* Set Preset Register back to original value */
- position = priv->preset[count->id];
+ val = priv->preset[count->id];
for (i = 0; i < 3; i++)
- outb(position >> (8 * i), base_offset);
+ outb(val >> (8 * i), base_offset);
/* Reset Borrow, Carry, Compare, and Sign flags */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_FLAGS, base_offset + 1);
--
2.23.0
The signal_read callback is simplified to pass val as a
counter_signal_val enum rather than as an opaque data structure. The
opaque counter_signal_read_value structure and relevant
counter_signal_read_value_set function are removed as they are no longer
used. In addition, the counter_signal_level enum is replaced by the
similar counter_signal_value enum.
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/counter/counter.c | 35 +++++++----------------------------
include/linux/counter.h | 31 +++++--------------------------
2 files changed, 12 insertions(+), 54 deletions(-)
diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 1d08f1437b1b..6a683d086008 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -220,32 +220,6 @@ ssize_t counter_device_enum_available_read(struct counter_device *counter,
}
EXPORT_SYMBOL_GPL(counter_device_enum_available_read);
-static const char *const counter_signal_level_str[] = {
- [COUNTER_SIGNAL_LEVEL_LOW] = "low",
- [COUNTER_SIGNAL_LEVEL_HIGH] = "high"
-};
-
-/**
- * counter_signal_read_value_set - set counter_signal_read_value data
- * @val: counter_signal_read_value structure to set
- * @type: property Signal data represents
- * @data: Signal data
- *
- * This function sets an opaque counter_signal_read_value structure with the
- * provided Signal data.
- */
-void counter_signal_read_value_set(struct counter_signal_read_value *const val,
- const enum counter_signal_value_type type,
- void *const data)
-{
- if (type == COUNTER_SIGNAL_LEVEL)
- val->len = sprintf(val->buf, "%s\n",
- counter_signal_level_str[*(enum counter_signal_level *)data]);
- else
- val->len = 0;
-}
-EXPORT_SYMBOL_GPL(counter_signal_read_value_set);
-
struct counter_attr_parm {
struct counter_device_attr_group *group;
const char *prefix;
@@ -315,6 +289,11 @@ struct counter_signal_unit {
struct counter_signal *signal;
};
+static const char *const counter_signal_value_str[] = {
+ [COUNTER_SIGNAL_LOW] = "low",
+ [COUNTER_SIGNAL_HIGH] = "high"
+};
+
static ssize_t counter_signal_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -323,13 +302,13 @@ static ssize_t counter_signal_show(struct device *dev,
const struct counter_signal_unit *const component = devattr->component;
struct counter_signal *const signal = component->signal;
int err;
- struct counter_signal_read_value val = { .buf = buf };
+ enum counter_signal_value val;
err = counter->ops->signal_read(counter, signal, &val);
if (err)
return err;
- return val.len;
+ return sprintf(buf, "%s\n", counter_signal_value_str[val]);
}
struct counter_name_unit {
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 7e40796598a6..32fb4d8cc3fd 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -290,24 +290,16 @@ struct counter_device_state {
const struct attribute_group **groups;
};
-/**
- * struct counter_signal_read_value - Opaque Signal read value
- * @buf: string representation of Signal read value
- * @len: length of string in @buf
- */
-struct counter_signal_read_value {
- char *buf;
- size_t len;
+enum counter_signal_value {
+ COUNTER_SIGNAL_LOW = 0,
+ COUNTER_SIGNAL_HIGH
};
/**
* struct counter_ops - Callbacks from driver
* @signal_read: optional read callback for Signal attribute. The read
* value of the respective Signal should be passed back via
- * the val parameter. val points to an opaque type which
- * should be set only by calling the
- * counter_signal_read_value_set function from within the
- * signal_read callback.
+ * the val parameter.
* @count_read: optional read callback for Count attribute. The read
* value of the respective Count should be passed back via
* the val parameter.
@@ -332,7 +324,7 @@ struct counter_signal_read_value {
struct counter_ops {
int (*signal_read)(struct counter_device *counter,
struct counter_signal *signal,
- struct counter_signal_read_value *val);
+ enum counter_signal_value *val);
int (*count_read)(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
int (*count_write)(struct counter_device *counter,
@@ -452,19 +444,6 @@ struct counter_device {
void *priv;
};
-enum counter_signal_level {
- COUNTER_SIGNAL_LEVEL_LOW = 0,
- COUNTER_SIGNAL_LEVEL_HIGH
-};
-
-enum counter_signal_value_type {
- COUNTER_SIGNAL_LEVEL = 0
-};
-
-void counter_signal_read_value_set(struct counter_signal_read_value *const val,
- const enum counter_signal_value_type type,
- void *const data);
-
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
int devm_counter_register(struct device *dev,
--
2.23.0