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
Attachment:
signature.asc
Description: PGP signature
