lkml.org 
[lkml]   [2020]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 0/5] Introduce the Counter character device interface
On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:
> On 11/22/20 2:29 PM, William Breathitt Gray wrote:
> >
> > 1. Should standard Counter component data types be defined as u8 or u32?
> >
> > Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
> > have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
> > COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
> > Counter subsystem code as u8 data types.
> >
> > If u32 is used for these values instead, C enum structures could be
> > used by driver authors to implicitly cast these values via the driver
> > callback parameters.
> >
> > This question is primarily addressed to David Lechner. I'm somewhat
> > confused about how this setup would look in device drivers. I've gone
> > ahead and refactored the code to support u32 enums, and pushed it to
> > a separate branch on my repository called counter_chrdev_v6_u32_enum:
> > https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
> >
> > Please check it out and let me know what you think. Is this the
> > support you had in mind? I'm curious to see an example of how would
> > your driver callback functions would look in this case. If everything
> > works out fine, then I'll submit this branch as v7 of this patchset.
>
> I haven't had time to look at this in depth, but just superficially looking
> at it, it is mostly there. The driver callback would just use the enum type
> in place of u32. For example:
>
> static int ti_eqep_function_write(struct counter_device *counter,
> struct counter_count *count,
> enum counter_function function)
>
> and the COUNTER_FUNCTION_* constants would be defined as:
>
> enum counter_function {
> COUNTER_FUNCTION_INCREASE,
> ...
> };
>
> instead of using #define macros.
>
> One advantage I see to using u8, at least in the user API data structures,
> is that it increases the number of events that fit in the kfifo buffer by
> a significant factor.
>
> And that is not to say that we couldn't do both: have the user API structs
> use u8 for enum values and still use u32/strong enum types internally in
> the callback functions.

I'm including David Laight because he initially opposed enums in favor
of fixed size types when we discussed this in an earlier revision:
https://lkml.org/lkml/2020/5/3/159

However, there have been significant changes to this patchset so the
context now is different than those earlier discussions (i.e. we're no
longer discussing ioctl calls).

I think reimplementing these constants as enums as described could work.
If we do so, should the enum constants be given specific values? For
example:

enum counter_function {
COUNTER_FUNCTION_INCREASE = 0,
COUNTER_FUNCTION_DECREASE = 1,
...
};

>
> >
> > 2. How should we handle "raw" timestamps?
> >
> > Ahmad Fatoum brought up the possibility of returning "raw" timestamps
> > similar to what the network stack offers (see the network stack
> > SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).
> >
> > I'm not very familiar with the networking stack code, but if I
> > understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
> > values returned from the device. If so, I suspect we would be able to
> > support these "raw" timestamps by defining them as Counter Extensions
> > and returning them in struct counter_event elements similar to the
> > other Extension values.
>
> Is nanosecond resolution good enough? In the TI eQEP driver I considered
> returning the raw timer value, but quickly realized that it would not be
> very nice to expect the user code to know the clock rate of the timer. It
> was very easy to get the clock rate in the kernel and just convert the
> timer value to nanoseconds before returning it to userspace.
>
> So if there is some specialized case where it can be solved no other way
> besides using raw timestamps, then sure, include it. Otherwise I think we
> should stick with nanoseconds for time values when possible.

Given that the struct counter_event 'timestamp' member serves as the
identification vessel for correlating component values to a single event
(i.e. component values of a given event will share the same unique
timestamp), I believe it's prudent to standardize this timestamp format
on the kernel monotonic time as we have currently done so via our
ktime_get_ns() call.

There are cases where it is understandably better to use a timestamp
provided directly by the hardware (e.g. keeping timestamping close to
data collection). For these cases, we can retrieve these "raw"
timestamps via a Counter Extension: users would get their "raw"
timestamp via the struct counter_event 'value' member, and just treat
the 'timestamp' member as a unique event identification number.

William Breathitt Gray
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2020-12-21 05:34    [W:0.177 / U:1.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site