Changes in v3:
- Squash code changes to single patch to avoid compilation error
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 (2):
counter: Simplify the count_read and count_write callbacks
docs: driver-api: generic-counter: Update Count and Signal data types
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
Count data is now always represented as an unsigned integer, while
Signal data is either SIGNAL_LOW or SIGNAL_HIGH.
Signed-off-by: William Breathitt Gray <[email protected]>
---
Documentation/driver-api/generic-counter.rst | 22 +++++++-------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index 8382f01a53e3..161652fc1025 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -39,10 +39,7 @@ There are three core components to a counter:
COUNT
-----
A Count represents the count data for a set of Signals. The Generic
-Counter interface provides the following available count data types:
-
-* COUNT_POSITION:
- Unsigned integer value representing position.
+Counter interface represents the count data as an unsigned integer.
A Count has a count function mode which represents the update behavior
for the count data. The Generic Counter interface provides the following
@@ -93,19 +90,16 @@ SIGNAL
A Signal represents a counter input data; this is the input data that is
evaluated by the counter to determine the count data; e.g. a quadrature
signal output line of a rotary encoder. Not all counter devices provide
-user access to the Signal data.
-
-The Generic Counter interface provides the following available signal
-data types for when the Signal data is available for user access:
+user access to the Signal data, so exposure is optional for drivers.
-* SIGNAL_LEVEL:
- Signal line state level. The following states are possible:
+When the Signal data is available for user access, the Generic Counter
+interface provides the following available signal values:
- - SIGNAL_LEVEL_LOW:
- Signal line is in a low state.
+* SIGNAL_LOW:
+ Signal line is in a low state.
- - SIGNAL_LEVEL_HIGH:
- Signal line is in a high state.
+* SIGNAL_HIGH:
+ Signal line is in a high state.
A Signal may be associated with one or more Counts.
--
2.23.0
Hi William,
This all makes sense to me. Do you want to wait for some more reviews
or should I pick them up now through IIO? We are really early in
the cycle so plenty of time, unless there are new drivers coming you
want to use these from the start.
Thanks,
Jonathan
On Wed, 18 Sep 2019 23:22:44 +0900
William Breathitt Gray <[email protected]> wrote:
> Changes in v3:
> - Squash code changes to single patch to avoid compilation error
>
> 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 (2):
> counter: Simplify the count_read and count_write callbacks
> docs: driver-api: generic-counter: Update Count and Signal data types
>
> 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(-)
>
On Sat, Oct 05, 2019 at 03:33:08PM +0100, Jonathan Cameron wrote:
> Hi William,
>
> This all makes sense to me. Do you want to wait for some more reviews
> or should I pick them up now through IIO? We are really early in
> the cycle so plenty of time, unless there are new drivers coming you
> want to use these from the start.
>
> Thanks,
>
> Jonathan
Getting this in sooner would be better since that will save Fabien from
having to introduce the COUNTER_COUNT_TALLY type in the cros_ec patch
submission.
The only concern left now is that the TI eQEP driver needs to be updated
as well for these changes, but it's not in the IIO testing branch yet.
Do you want to merge this patchset first, or wait until TI eQEP makes it
into the testing branch? Alternatively, I can merge the cros_ec patchset
and Intel QEP patchset into my personal repository when they are ready,
then later submit a git pull request to you with these changes if you
prefer that route.
William Breathitt Gray
> On Wed, 18 Sep 2019 23:22:44 +0900
> William Breathitt Gray <[email protected]> wrote:
>
> > Changes in v3:
> > - Squash code changes to single patch to avoid compilation error
> >
> > 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 (2):
> > counter: Simplify the count_read and count_write callbacks
> > docs: driver-api: generic-counter: Update Count and Signal data types
> >
> > 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(-)
> >
>
On Sat, 5 Oct 2019 13:19:38 -0400
William Breathitt Gray <[email protected]> wrote:
> On Sat, Oct 05, 2019 at 03:33:08PM +0100, Jonathan Cameron wrote:
> > Hi William,
> >
> > This all makes sense to me. Do you want to wait for some more reviews
> > or should I pick them up now through IIO? We are really early in
> > the cycle so plenty of time, unless there are new drivers coming you
> > want to use these from the start.
> >
> > Thanks,
> >
> > Jonathan
>
> Getting this in sooner would be better since that will save Fabien from
> having to introduce the COUNTER_COUNT_TALLY type in the cros_ec patch
> submission.
>
> The only concern left now is that the TI eQEP driver needs to be updated
> as well for these changes, but it's not in the IIO testing branch yet.
>
> Do you want to merge this patchset first, or wait until TI eQEP makes it
> into the testing branch? Alternatively, I can merge the cros_ec patchset
> and Intel QEP patchset into my personal repository when they are ready,
> then later submit a git pull request to you with these changes if you
> prefer that route.
OK, so I need to do a pull request to be able to move the testing branch
to the point where I can do the immutable branch needed for that TI set.
Should do that later this week. So I'll take the TI eQEP series at that
point. If you want to prep a version of this that includes this driver
that would be great. Should be able to pull both in next weekend at
the latest. If it takes a bit longer for you to be ready, no problem!
Thanks,
Jonathan
>
> William Breathitt Gray
>
> > On Wed, 18 Sep 2019 23:22:44 +0900
> > William Breathitt Gray <[email protected]> wrote:
> >
> > > Changes in v3:
> > > - Squash code changes to single patch to avoid compilation error
> > >
> > > 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 (2):
> > > counter: Simplify the count_read and count_write callbacks
> > > docs: driver-api: generic-counter: Update Count and Signal data types
> > >
> > > 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(-)
> > >
> >